feat: report modification-control findings - #139
rajasimman-madhivanan wants to merge 2 commits into
Conversation
|
mode: warn
These checks verify artifact existence and ordering only, not truth, adequacy, or safety sufficiency. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a change-control tool and its CI workflow that gate pull requests on the existence and ordering of modification records rather than judging their adequacy. __main__.py collects a full GitHub snapshot via a fail-closed GhApi gh-api subprocess adapter, evaluates policy, prints the report, and upserts a PR comment; it exits 1 only on findings in enforce mode, 2 on any collection or (in enforce) comment-publication failure, and skips commenting under --no-comment. load_mode reads docs/process/enforcement-mode and rejects anything but exactly warn or enforce, so the mode currently ships as warn (findings reported but non-blocking). cr_number resolves a single local change-request issue and rejects foreign or ambiguous links, while minimum_class, impact_analysis_complete, and the authorizer/label checks derive the classification floor and record completeness.
Worth a look
- Pull request workflow executes checked-out PR code with write-scoped token —
.github/workflows/change-control.yml:35· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Marker comment can be hijacked by any commenter —
tools/change_control/checks.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Negative authorization wording is accepted as approval —
tools/change_control/checks.py:157· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- evaluate now raises KeyError when PR head SHA is absent —
tools/change_control/checks.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- PR head is not revalidated after multi-call snapshot collection —
tools/change_control/__main__.py:67· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 218 functions depend on the 218 functions this change touches.
Health — this change adds coupling hotspots:
- new:
evaluate()— 37 callers, 15 callees - new:
main()— 1 callers, 6 callees
Verification — 218 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 6f415e0 (diverged from this PR's base — delta is approximate).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 218 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below.
| } | ||
|
|
||
|
|
||
| def main(argv=None): |
There was a problem hiding this comment.
main()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| } | ||
|
|
||
|
|
||
| def evaluate(root, data): |
There was a problem hiding this comment.
evaluate()
fans out to 15 callees (efferent coupling); 37 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a change-control enforcement system that runs on pull-request and review events, checking that each PR links exactly one local change request and that its modification record exists, is complete, and was authorized in the correct order before merge. evaluate runs pure policy checks over a GitHub snapshot gathered by collect through the fail-closed GhApi gh-api adapter, cr_number resolves a single unambiguous local CR while rejecting foreign or duplicate links, and minimum_class derives a classification floor from changed paths; findings are rendered and posted via upsert_comment on a marker comment. Mode is read from docs/process/enforcement-mode (shipped as warn), where warn reports without blocking and enforce exits non-zero on findings or comment-publication failures, and forked PRs run with --no-comment since they can't write comments.
Worth a look
- before_implementation/after_analysis pass vacuously when authorization times are absent —
tools/change_control/checks.py:240· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Later COMMENTED review erases an existing approval —
tools/change_control/checks.py:197· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- First incidental heading mention hides later IA —
tools/change_control/checks.py:134· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Evidence checks do not rerun when workflows finish —
.github/workflows/change-control.yml:5· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Comment-dependent checks do not rerun on comments —
.github/workflows/change-control.yml:5· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 218 functions depend on the 218 functions this change touches.
Health — this change adds coupling hotspots:
- new:
evaluate()— 37 callers, 15 callees - new:
main()— 1 callers, 6 callees
Verification — 218 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 6f415e0 (diverged from this PR's base — delta is approximate).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 218 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below.
| } | ||
|
|
||
|
|
||
| def main(argv=None): |
There was a problem hiding this comment.
main()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| } | ||
|
|
||
|
|
||
| def evaluate(root, data): |
There was a problem hiding this comment.
evaluate()
fans out to 15 callees (efferent coupling); 37 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked two related angles: whether minimum_class() under-classifies changes to the change-control tooling itself (it does, but that's the same root-cause gap as the classifier finding above, not a distinct one), and whether E1's authorization-ordering check fails open when a PR response is missing created_at (it does via not pr.get('created_at') or (...), but every other field this check needs is fetched from a live PR object where GitHub always populates that field, unlike the other partial-response cases the code explicitly guards against).
Extended reasoning...
This PR adds a large, security/governance-relevant feature (a GitHub-Actions-driven change-control enforcement tool with hardcoded authorizer logins, comment-based authorization parsing, and a policy engine deciding PR pass/fail). Five CONFIRMED findings are already being posted inline covering: crashes on deleted-account API responses (checks.py:196/153), an unauthenticated comment-hijack in upsert_comment (checks.py:385), an authorization-timestamp overwrite bug (checks.py:245), a missing subprocess timeout on all gh api calls (main.py:34), and an under-classification of safety-critical bond/heartbeat/arming code paths (checks.py:86). Given findings of this severity on security- and safety-adjacent logic, this is squarely a defer/no-approve case, and the bug-hunt exit reason (max_bugs, not dry_streak) further rules out approval per the guidelines. I independently re-read checks.py and main.py to confirm the two ruled-out candidates noted above are real code patterns and not fabricated, so the short note records genuine additional scrutiny rather than restating what's already inline. No prior review body exists on this PR to avoid duplicating, so a short informational note recording this extra scrutiny adds value without repeating the inline findings.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| login = comment.get('user', {}).get('login', '').lower() | ||
| return ( | ||
| login in AUTHORIZERS | ||
| and login != author.lower() | ||
| and bool(re.search(r'(?im)^\s*(?:decision:\s*)?authori[sz]ed\b', comment.get('body', ''))) | ||
| ) | ||
|
|
||
|
|
||
| def _named_tests(text): | ||
| sections = re.findall(r'(?ms)^# 7\. Verification plan for this change\s*$\n(.*?)(?=^# 8\.|\Z)', text) | ||
| names = [] | ||
| for section in sections: | ||
| names.extend(re.findall(r'`([^`]+)`', section)) | ||
| for line in section.splitlines(): | ||
| if line.lstrip().startswith(('-', '+')) and ':' in line: | ||
| value = line.split(':', 1)[1].strip() | ||
| if value and value.lower() not in ('none', 'n/a'): | ||
| names.append(value) | ||
| if line.strip().startswith('|'): | ||
| cells = [cell.strip() for cell in line.strip().strip('|').split('|')] | ||
| if len(cells) >= 2 and cells[0] not in ('Purpose', '---'): | ||
| value = cells[1] | ||
| if value and value not in ('---', 'None', 'N/A'): | ||
| names.extend(part.strip() for part in re.split(r'<br\s*/?>|,', value) if part.strip()) | ||
| if not sections: | ||
| match = re.search(r'(?im)^\s*(?:which\s+)?tests(?:\s+will\s+be\s+run)?\s*:\s*(.+)$', text) | ||
| if match: | ||
| names.extend(part.strip(' `') for part in match.group(1).split(',') if part.strip(' `')) | ||
| return list(dict.fromkeys(name.strip() for name in names if name.strip())) | ||
|
|
||
|
|
||
| def _evidence_matches(name, evidence_name): | ||
| """Match an IA entry to one complete check or workflow name, never a substring.""" | ||
| planned = ' '.join(name.casefold().split()) | ||
| observed = ' '.join(evidence_name.casefold().split()) | ||
| return bool(planned and observed and planned == observed) | ||
|
|
||
|
|
||
| def _approvers(data, require_head=True): | ||
| head = data['pr']['head']['sha'] | ||
| author = data['pr']['user']['login'].lower() | ||
| latest = {} | ||
| for review in data.get('reviews', []): | ||
| login = review.get('user', {}).get('login', '').lower() |
There was a problem hiding this comment.
🔴 A maintainer sees the whole change-control check crash uncaught instead of the intended pass/fail/warn report, when any past CR-issue comment, PR review, or PR comment came from a deleted GitHub account. review.get('user', {}) (and comment.get('user', {}) at checks.py:153) only substitutes {} when the key is absent; GitHub sets "user": null for deleted accounts, so .get('login', ...) is then called on None, raising an uncaught AttributeError. main()'s except tuple in main.py:128 only catches (OSError, RuntimeError, ValueError, KeyError, LintError), so this propagates as an unhandled crash even in warn mode, contradicting the PR's own claim that warn-mode findings never unexpectedly block PRs. … [also at: tools/change_control/checks.py:327 - A maintainer sees the whole change-control job crash with an uncaught AttributeError instead of its warn/enforce report, when a successful workflow run on the head SHA has name: null (a valid GitHub Actions workflow-run field value). checks.py:327's item.get('name', item.get('path', '')) only…]
Extended reasoning...
…sweep:\.get\('user', \{\}\)\.get\( Fix: guard both sites, e.g. (comment.get('user') or {}).get('login', ''), covering any reviewer/commenter whose account is deleted.
GitHub returns "user": null (not a missing key) for issue comments, PR reviews, and PR comments authored by since-deleted accounts; this is common on any repo with multi-year history. collect() in main.py fetches issue_comments, reviews, and pr_comments raw from the GitHub API with no transformation of the user field. evaluate() calls _is_authorization(comment, author) for every comment in data['issue_comments'] (checks.py:232-234) and _approvers(data) iterates data['reviews'] (checks.py:195-198). For any entry with user=null, comment.get('user', {}) returns None because the key IS present, just with value None -- dict.get's default only applies…
Verification: normal. Real, reachable, uncaught crash on GitHub's "user": null (deleted-account content). dict.get('user', {}) returns the default {} only when the key is absent; when the key is present with value null it returns None, so the chained .get('login', '') runs on None -> AttributeError. Sites (both new code, hit before any guard): - checks.py:153 _is_authorization:…
| def upsert_comment(api, pr_number, report, comments): | ||
| """Create or update at most one marker-owned report comment.""" | ||
| body = f'{COMMENT_MARKER}\n{report}' | ||
| existing = next((comment for comment in comments if COMMENT_MARKER in comment.get('body', '')), None) | ||
| if existing: | ||
| api('PATCH', f'repos/{{repo}}/issues/comments/{existing["id"]}', {'body': body}) | ||
| else: | ||
| api('POST', f'repos/{{repo}}/issues/{pr_number}/comments', {'body': body}) |
There was a problem hiding this comment.
🔴 upsert_comment can silently overwrite another user's PR comment, not just the bot's own report, because it identifies the "existing" comment purely by substring match on COMMENT_MARKER (checks.py:381) with no author check. Any comment on the PR whose body happens to contain the literal text <!-- change-control --> — e.g. a reviewer pasting checks.py:15, which defines that exact constant — is treated as the bot's own comment and gets PATCHed away with the compliance report, destroying the original content. Fix: only match comments authored by the workflow's own identity (e.g. compare comment.user.login/type to the actions bot) before reusing an id for PATCH, falling back to POST otherwise.
Extended reasoning...
collect() fetches data['pr_comments'] via GET .../issues/{pr}/comments (all PR conversation comments, any author). upsert_comment (checks.py:378-386) does existing = next(c for c in comments if COMMENT_MARKER in c.get('body','')) with no author filter, then on match calls api('PATCH', f'repos/{{repo}}/issues/comments/{existing["id"]}', {'body': body}). GITHUB_TOKEN with issues:write can edit any comment on the repo, not just ones it authored. A reviewer who quotes tools/change_control/checks.py source (which contains COMMENT_MARKER = '<!-- change-control -->' at line 15) in a PR review comment now has that comment matched first (since next() returns the earliest list match) and overwritten on the next workflow run, losing their original text with no warning.
Verification: nit. The defect is real: upsert_comment (checks.py:378-385) locates the "existing" report comment purely by substring match on COMMENT_MARKER (checks.py:381: next((comment for comment in comments if COMMENT_MARKER in comment.get('body', '')), None)) with no author/bot-identity filter, then PATCHes that comment id (checks.py:383). The comments argument is data['pr_comments'], fetched in…
| authorization_comments = {} | ||
| for comment in comments: | ||
| if _is_authorization(comment, author): | ||
| authorization_comments[comment['user']['login'].lower()] = comment | ||
| authorized_comment = len(authorization_comments) >= required_authorizers | ||
| authorization_times = [ | ||
| comment.get('created_at') for comment in authorization_comments.values() if comment.get('created_at') | ||
| ] | ||
| ia_times = [ | ||
| comment.get('created_at') | ||
| for comment in comments | ||
| if comment.get('body', '') == ia and comment.get('created_at') | ||
| ] | ||
| before_implementation = not pr.get('created_at') or ( | ||
| len(authorization_times) >= required_authorizers and max(authorization_times) <= pr['created_at'] |
There was a problem hiding this comment.
🟡 (optional) A valid, pre-PR Change Request authorization can be reported as missing (E1 fails) if the same authorizer comments "Authorized: proceed" again later, e.g. to reconfirm. authorization_comments (line 231-234) is keyed by login and each new matching comment overwrites the prior one, so authorization_times holds each authorizer's LATEST match, not their first. before_implementation (line 245) then compares max(authorization_times) to pr['created_at'], so a later reaffirming comment erases evidence of the original pre-implementation approval and E1 incorrectly fails. Fix: record the earliest authorization timestamp per login for ordering purposes while still using presence/distinct-login count for the required_authorizers check.
Extended reasoning...
Authorizer X posts "Authorized: proceed" on the CR issue on day 1, before the PR is opened on day 3 (compliant). On day 5 X posts the same phrase again to reconfirm after a discussion. The loop at checks.py:232-234 re-assigns authorization_comments['x'] to the day-5 comment, discarding the day-1 one. authorization_times now contains only day 5. before_implementation at line 244-246 computes max(authorization_times) <= pr['created_at'], i.e. day5 <= day3, which is False, so E1 reports the Change Request as not properly ordered even though the real authorization predates the PR. In enforce mode this would block a compliant PR; in warn mode it produces a misleading advisory failure that erodes trust in the report.
Verification: nit. Real logic defect, but advisory-only in the shipped warn mode (does not block a merge; it produces a wrong line in the report). At checks.py:232-234 authorization_comments is a dict keyed by lowercased login, and iterating comments (collected oldest-first via __main__.py:77 with no reversal) makes each new matching comment OVERWRITE the prior one for that login, so the stored…
| if body: | ||
| for key, value in body.items(): | ||
| command.extend(['--field', f'{key}={value}']) | ||
| result = subprocess.run(command, check=False, capture_output=True, text=True) |
There was a problem hiding this comment.
🟡 (optional) GhApi.call runs gh api via subprocess.run with no timeout, unlike every other subprocess.run call in tools/ (flash_station.py, chaos_soak.py, soak_disconnect_monitor.py all pass timeout=). A stalled gh api call (rate-limit wait, network stall, GitHub API degradation) blocks the whole change-control job indefinitely, up to the workflow's default 360-minute job timeout since no timeout-minutes is set in change-control.yml. Fix: pass a bounded timeout to subprocess.run and raise a RuntimeError (already handled fail-closed by main()) on TimeoutExpired, matching the repo's existing subprocess timeout convention.
Extended reasoning...
collect() in main.py calls api(...) 7 times per PR run, each going through GhApi.call at line 34 which does subprocess.run(command, check=False, capture_output=True, text=True) with no timeout kwarg. If gh api stalls (GitHub API partial outage, secondary rate-limit backoff, DNS/network hiccup), this call never returns. The GitHub Actions concurrency group in change-control.yml only cancels a run when a NEW event fires for the SAME PR number; it does not bound or kill a hang caused by an external API stall, and no step- or job-level timeout-minutes is configured, so the default 360-minute job timeout is the only backstop. During any GitHub API degradation affecting many open PRs simultaneously, each PR's own change-control job independently hangs in this call, each holding a runner slot for up to 6 hours, exhausting the repository's/org's concurrent-job quota and delaying or blocking other required workflows (firmware-build, safety-lint, pstop_c_build) that need runners. This is new exposure: before this diff there was no change-control job to hang.
Verification: nit. The fact is accurate: tools/change_control/main.py:34 result = subprocess.run(command, check=False, capture_output=True, text=True) passes no timeout=, and collect() (lines 70-80) invokes api() ~7 times per run. Peer tools consistently bound their subprocess calls (flash_station.py:104 timeout=15, :132 timeout=10, :143 timeout=5, chaos_soak.py:69 timeout=3,… | nit. Real but…
| def minimum_class(paths, wire_changed=False): | ||
| """Return the mechanical classification floor for changed repository paths.""" | ||
| floor = 'C' if wire_changed else 'A' | ||
| for path in paths: | ||
| if ( | ||
| path.startswith('pstop_c/') | ||
| or path in ('firmware/main/main.c', 'machn/main/main.c') | ||
| or path.startswith('docs/safety/') | ||
| ): | ||
| candidate = 'C' | ||
| elif path.startswith(('components/', 'common/')) or path in ( | ||
| 'firmware/sdkconfig.defaults', | ||
| 'machn/sdkconfig.defaults', | ||
| ): | ||
| candidate = 'B' | ||
| elif path.startswith('.github/workflows/') or path.startswith('scripts/'): | ||
| candidate = 'B' | ||
| else: | ||
| candidate = 'A' | ||
| if CLASS_RANK[candidate] > CLASS_RANK[floor]: | ||
| floor = candidate | ||
| return floor |
There was a problem hiding this comment.
🟡 (optional) minimum_class() gives changes to the ROS2 bond/heartbeat/arming code the lowest review floor (Class A), unlike pstop_c, letting such PRs pass E4 with minimal authorization even though the repo's own procedure puts them at Class C. The classifier only special-cases pstop_c/, firmware|machn main.c, docs/safety/, components/, common/, sdkconfig.defaults, .github/workflows/, scripts/ (checks.py:69-81); anything under ros2/protective_stop_machine/ (machine_bridge_node.cpp, hardware_backend.cpp) or ros2/protective_stop_msg/ (ProtectiveStopHeartbeat.msg, BondedRemote.msg) falls to the else branch at checks.py:83 and gets candidate='A'. …
Extended reasoning...
…MODIFICATION_PROCEDURE.md:14 lists the host machine wrapper in scope, :33 defines safety modules to include "the heartbeat and timeout paths", and :167 makes any change to "the bond or arming state machine" Class C regardless of size. Fix: extend the C-tier prefix list to cover ros2/protective_stop_machine/** and ros2/protective_stop_msg/** (and any other in-scope paths the procedure names), not just pstop_c and firmware/machn.
minimum_class(paths, wire_changed) at checks.py:65 sets floor='A' when wire_changed is False, then loops changed paths raising the floor only for the listed prefixes. A PR that edits ros2/protective_stop_machine/src/hardware_backend.cpp (the arming-policy hardware backend) or ros2/protective_stop_msg/msg/ProtectiveStopHeartbeat.msg (the heartbeat wire message) does not match pstop_c/, firmware/main/main.c, machn/main/main.c, docs/safety/, components/, common/, sdkconfig.defaults, .github/workflows/, or scripts/, so it falls to the else branch (candidate='A') at checks.py:82-83. E4 in evaluate() (checks.py:296-308) then only requires the PR to carry a…
Verification: nit. The mechanism is real: checks.py:65-86 (minimum_class) starts floor='A' and only raises it for pstop_c/, firmware|machn/main/main.c, docs/safety/, components/, common/, *sdkconfig.defaults, .github/workflows/, scripts/. There is no ros2/ prefix, and wire_changed at checks.py:294 is scoped to pstop_c/pstop/include/pstop/ only, so it misses… | nit — the mechanical floor really does give ros2…
There was a problem hiding this comment.
Beyond the confirmed inline findings, I also checked two other candidates from this run and ruled them out: a PR response missing created_at would require GitHub itself to return a malformed PR object (not something a PR author can trigger, and collect() already fails closed on the fields that are attacker-reachable); and _find_short_ia's loose three-phrase match only feeds the emergency-only fallback path, which still requires all three specific phrases to co-occur in one CR-issue comment.
Extended reasoning...
This run reported four new CONFIRMED findings (self-modifiable enforcement-mode/policy inputs read from the PR's own checkout, oldest-first substring matching for the Impact Analysis comment, renamed-file classification bypass via ignoring previous_filename, and GFM alignment-colon table rows not being skipped in the verification-plan parser) that are distinct from the five inline comments I posted on the prior version of this PR (checks.py:196, 385, 245, main.py:34, checks.py:86). Those inline findings will be posted separately and are not restated here. Two additional candidates surfaced this run — a hypothetical missing pr.created_at field and a loose emergency short-form IA phrase match — were investigated and ruled out as not attacker-exploitable or scoped to a narrow fallback path, so I'm recording that as a brief note per the ruled-out-items convention rather than duplicating the inline findings.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| def load_mode(root): | ||
| """Read the auditable mode switch and reject every value except warn or enforce.""" | ||
| path = Path(root) / 'docs/process/enforcement-mode' | ||
| try: | ||
| value = path.read_text(encoding='utf-8') | ||
| except OSError as error: | ||
| raise RuntimeError(f'cannot read enforcement mode: {error}') from error | ||
| if value not in ('warn\n', 'enforce\n'): | ||
| raise RuntimeError('enforcement-mode must contain exactly warn or enforce followed by a newline') | ||
| return value.strip() |
There was a problem hiding this comment.
🔴 The change-control check reads its own policy inputs (enforcement mode, IA headings, safety-requirement list, issue-form fields) from the PR's own checked-out tree, so a PR author can weaken the check on the very PR that changes it. load_mode (checks.py:27-36) reads docs/process/enforcement-mode from root, which actions/checkout populates from the PR's merge commit; the same applies to _headings (checks.py:218, IMPACT_ANALYSIS.md), parse_srs (checks.py:281, SAFETY_REQUIREMENTS.md) and _issue_fields_complete (checks.py:120, change-request.yml). …
Extended reasoning...
…Fix: evaluate these governance files against the protected base branch (fetch base-ref content via the GitHub API) rather than the PR's own working tree, or force a hard fail/max classification whenever a PR modifies any of them, covering all four sites. Same pattern at checks.py:120, checks.py:218, checks.py:281.
Once enforcement-mode is later flipped to 'enforce', a contributor opens a PR that both makes an unrelated change and, in the same diff, edits docs/process/enforcement-mode back to 'warn'. actions/checkout@ v7 (change-control.yml:25) checks out the PR's merge commit, so tools/change_control/checks.py:29-31 reads the PR's own edited copy of the file, not the protected branch's value. load_mode returns 'warn' for this run, so evaluate()'s E1-E7 fail results never produce a nonzero exit (main() at main.py:127 only returns 1 when mode=='enforce'), and the PR merges without any real gate. The same trick works against docs/process/templates/IMPACT_ANALYSIS.md headings (checks.py:218) or docs/safety/SAFETY_REQUIREMENTS.md (checks.py:281): shrinking required headings or the…
Verification: nit (latent security/governance flaw; inert in the shipped warn config). The self-referential read is real and reachable: change-control.yml:25 uses actions/checkout@ v7 with no ref:, so on a pull_request event the workflow checks out the PR merge commit (github.sha) containing the PR's own edits; change-control.yml:36 invokes python3 -m tools.change_control with no --root, and main.py:106…
| def _find_ia(comments, headings): | ||
| for comment in comments: | ||
| body = comment.get('body', '') | ||
| if headings and headings[0] in body: | ||
| return body | ||
| return '' |
There was a problem hiding this comment.
🟡 (optional) A maintainer can see E2 fail with "IA sections missing" for a PR that actually has a complete, correctly-ordered Impact Analysis, because _find_ia (checks.py:132-137) picks the first comment (oldest-first, per GitHub's default order) that merely contains the literal substring of the template's first heading, e.g. # Identification, not the comment that is actually the completed IA. Any earlier comment quoting or discussing that heading text (a maintainer's reminder, a partial draft) gets locked in as ia, so the real, later, complete IA is never checked. …
Extended reasoning...
…Fix: identify the IA comment by a stronger anchor (e.g. requiring the whole ordered heading sequence to start the match, or restricting to the requester's/authorizer's comments) instead of an unscoped substring test on only the first heading.
Comments on the linked CR issue are fetched oldest-first (main.py:77, no reversal) into data['issue_comments']. _find_ia (checks.py:132-137) iterates comments in that order and returns the body of the first one containing headings[0] (# Identification) anywhere as a substring. If a maintainer earlier posts a reply that quotes or references # Identification (e.g. asking the requester to fill it in) before the requester posts the real, complete Impact Analysis, that earlier partial comment is selected as ia. impact_analysis_complete(ia, headings) at checks.py:270 then correctly reports missing headings for that wrong body, and E2 fails even though a fully compliant IA exists later in the same thread.
Verification: nit. Real, reachable logic defect. _find_ia (checks.py:132-137) returns the body of the FIRST comment whose body contains headings[0] as a bare substring: if headings and headings[0] in body: return body. headings[0] is # Identification (IMPACT_ANALYSIS.md:3, extracted by _headings at checks.py:39-42, matching lines ^#(?:\s|\d)). Comments are collected oldest-first…
| paths = [item['filename'] for item in data.get('files', [])] | ||
| wire_changed = any(path.startswith('pstop_c/pstop/include/pstop/') for path in paths) | ||
| floor = minimum_class(paths, wire_changed) |
There was a problem hiding this comment.
🟡 (optional) A PR that renames a safety-critical file out of a protected directory gets under-classified: E4's mechanical class floor never rises, so Class A/B is accepted for moving pstop_c/, docs/safety/, or firmware main.c content elsewhere. checks.py:293 builds paths from each PR file's filename only; for a GitHub PR-files entry with status=renamed, filename is the NEW path and the OLD path is in previous_filename, which is never read. minimum_class (checks.py:65-86) and wire_changed (checks.py:294) both match only against this filename list, so a rename from e.g. pstop_c/pstop/foo.c to tools/foo.c matches no C/B prefix and floors at A. …
Extended reasoning...
…Fix: also test each file's previous_filename (when status is 'renamed') against the same prefixes so moving safety-relevant content out of a protected directory still raises the floor.
GitHub's pulls/{n}/files API returns, for a renamed file, filename=new path and previous_filename=old path (well-documented shape); status='renamed' entries can also carry content changes. checks.py:293 paths = [item['filename'] for item in data.get('files', [])] drops previous_filename entirely. minimum_class (checks.py:68-83) loops paths and only raises the floor when a NEW path matches pstop_c/, firmware/main/main.c, machn/main/main.c, docs/safety/, components/, common/, *sdkconfig.defaults, .github/workflows/, scripts/. wire_changed (checks.py:294) similarly only checks new-path prefix pstop_c/pstop/include/pstop/. A contributor renames pstop_c/pstop/include/pstop/protocol.h to shared/protocol.h in the same PR that edits its content: new path matches none of the prefixes, floor stays 'A', wire_changed stays False. E4 (checks.py:296-308) then accepts a class-a label for a change that actually…
Verification: nit. Real and reachable, but advisory-only in the shipped warn mode and needs an unusual rename-out-of-protected-dir. checks.py:293 paths = [item['filename'] for item in data.get('files', [])] reads only the new path; a grep of the whole package shows previous_filename is never referenced. main.py:79/97 store the raw /pulls/{n}/files response, so previous_filename/status are…
| if line.strip().startswith('|'): | ||
| cells = [cell.strip() for cell in line.strip().strip('|').split('|')] | ||
| if len(cells) >= 2 and cells[0] not in ('Purpose', '---'): | ||
| value = cells[1] | ||
| if value and value not in ('---', 'None', 'N/A'): | ||
| names.extend(part.strip() for part in re.split(r'<br\s*/?>|,', value) if part.strip()) |
There was a problem hiding this comment.
🟡 (optional) A maintainer gets a permanently failing E6 evidence check for a complete, correctly verified change, if the CR issue's Impact Analysis section 7 table uses standard GFM alignment-colon separators (e.g. |:---|:---|) instead of plain |---|---|. _named_tests (checks.py:171-176) only skips the table's separator row when cells[0] == '---' exactly; an aligned separator like ':---' or ':---:' fails that check, so the whole separator row is treated as data and its second cell (e.g. ':---') is injected into the required test-name list. That literal string can never match a real check-run or workflow name, so missing_tests in evaluate() is never empty and E6 fails forever for that PR, regardless of real evidence. …
Extended reasoning...
…Fix: treat any row whose cells are each ^:?-+:?$ as the separator, not only literal '---', covering left/right/center-aligned Markdown tables.
checks.py:161 _named_tests extracts the section between '# 7. Verification plan for this change' and '# 8.' from the IA text. checks.py:171-176 iterates lines starting with '|', splits into cells, and skips the row only if cells[0] is exactly 'Purpose' or '---' (the template's own separator at docs/process/templates/IMPACT_ANALYSIS.md:73 is '|---|---|'). A CR author who writes or reformats the table with GFM alignment syntax, e.g. '|:---|:---|', produces cells[0]==':---', which is not in ('Purpose','---'), so the branch at line 173 does not skip it. value = cells[1] = ':---' is then added to names via line 176 (no
or comma to split on, so the whole string is kept). evaluate() at checks.py:319-338 computes names = _named_tests(ia); for each name it requires a check-run or workflow-run with a matching name at the PR head SHA (_evidence_matches, exact casefold match). ':---' will never equal any real CI job or workflow name, so…
Verification: nit. Real and reachable, but advisory-only in the shipped warn mode (docs/process/enforcement-mode = "warn"), so it produces a wrong E6 report line rather than blocking a merge, and it requires the CR author to deviate from the template's plain |---|---| separator. Trace: _named_tests (checks.py:161-181) parses IA section 7. For a table separator written with standard GFM alignment colons,…
davidt315
left a comment
davidt315
left a comment
There was a problem hiding this comment.
I think this is duplication of a lot of github's native features. Let's review what github natively supports that we can control via pulumi and compare against what is extra that we have to implement ourselves
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - name: Run tests | ||
| run: python3 tools/change_control/self_test.py |
| run: |- | ||
| args=() | ||
| [ "$CAN_COMMENT" = 'true' ] || args+=(--no-comment) | ||
| python3 -m tools.change_control --repository '${{ github.repository }}' --pr "$PR_NUMBER" "${args[@]}" |
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, labeled, unlabeled, edited] | ||
| pull_request_review: |
There was a problem hiding this comment.
I'm not quite following. what exactly is this necessary for? what does it do after someone reviews?
| return bool(planned and observed and planned == observed) | ||
|
|
||
|
|
||
| def _approvers(data, require_head=True): |
There was a problem hiding this comment.
This function is effectively a re-implementation of github's provided branch protection which we can manage in pulumi for this repo. I want to challenge this to reduce code maintenance of something that is already a feature we get with github. https://github.com/polymathrobotics/protective-stop/pull/136/changes#r4041803294 notes where you could control that
There was a problem hiding this comment.
I think this entire thing is just running github's required: true
Why: The modification procedure defines evidence and authorization gates, but reviewers need a consistent mechanical check for missing records, stale approvals, and incomplete verification evidence. Introducing the checker in warn mode makes those gaps visible without unexpectedly blocking active work. What changed: - Added E1-E7 checks for authorization, impact analysis, requirement references, classification, approvals, verification evidence, and emergencies. - Added fail-closed GitHub data collection, pagination, response validation, and deterministic report-comment updates. - Added a constrained parser that validates the repository Change Request form. - Added a fork-safe pull-request workflow and an auditable mode file set to warn. - Added 78 focused policy, API, parser, workflow, and CLI tests. Impact: Policy findings are advisory in warn mode and do not fail pull requests. Collection or configuration errors still return an unable-to-run failure so missing evidence cannot be mistaken for a clean result. Runtime code, wire behavior, safety requirements, and coverage calculations are unchanged. Safety lifecycle: Modification authorization, impact analysis, verification evidence, and configuration management. Bears on IEC 61508-1:2010 sections 6.2 and 7.16, and IEC 61508-3:2010 sections 7.4, 7.8, and 7.9. Co-Authored-By: OpenCode <noreply@opencode.ai>
Why: The focused checker test module was split from an executable monolithic runner. It retained that runner's shebang even though the new module is imported by unittest and is intentionally not executable. Two lines also did not match the repository's Python formatter. What changed: - Removed the script shebang from the imported test module. - Applied the repository formatter's line wrapping. - Re-ran all 78 focused checker tests and the applicable pre-commit hooks. Impact: There is no checker, workflow, policy, or test-behavior change. The module now satisfies executable-bit and formatting validation when later stacked files cause repository-wide hooks to inspect it. Co-Authored-By: OpenCode <noreply@opencode.ai>
11ca722 to
c166738
Compare
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 218 functions depend on the 218 functions this change touches.
Health — this change adds coupling hotspots:
- new:
evaluate()— 37 callers, 15 callees - new:
main()— 1 callers, 6 callees
Verification — 218 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 595d172, 3 commit(s) behind this PR's base.
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 218 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below.
| } | ||
|
|
||
|
|
||
| def main(argv=None): |
There was a problem hiding this comment.
main()
fans out to 6 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| } | ||
|
|
||
|
|
||
| def evaluate(root, data): |
There was a problem hiding this comment.
evaluate()
fans out to 15 callees (efferent coupling); 37 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| authorization_comments = {} | ||
| for comment in comments: | ||
| if _is_authorization(comment, author): | ||
| authorization_comments[comment['user']['login'].lower()] = comment | ||
| authorized_comment = len(authorization_comments) >= required_authorizers | ||
| authorization_times = [ | ||
| comment.get('created_at') for comment in authorization_comments.values() if comment.get('created_at') | ||
| ] | ||
| ia_times = [ | ||
| comment.get('created_at') | ||
| for comment in comments | ||
| if comment.get('body', '') == ia and comment.get('created_at') | ||
| ] | ||
| before_implementation = not pr.get('created_at') or ( | ||
| len(authorization_times) >= required_authorizers and max(authorization_times) <= pr['created_at'] | ||
| ) | ||
| after_analysis = not ia_times or ( | ||
| len(authorization_times) >= required_authorizers and max(ia_times) <= min(authorization_times) | ||
| ) |
There was a problem hiding this comment.
🟡 (optional) Maintainers can get an E1 'authorized before implementation' pass even though the real authorization was added long after review began, unlike a trustworthy ordering check. authorization_times and ia_times (checks.py:236-243) read comment['created_at'], which GitHub freezes at a comment's original post time; any AUTHORIZERS login can edit an old, unrelated comment on the CR issue after the fact to add 'Authorized: proceed', and _is_authorization/_find_ia match the current (edited) body while before_implementation/after_analysis (checks.py:244-249) still compare against the stale created_at. Fix: order by updated_at (or treat updated_at != created_at as disqualifying) for both authorization_times and ia_times, covering edited IA comments the same way, not just edited authorizations.
Extended reasoning...
checks.py:231-234 builds authorization_comments from comments whose CURRENT body matches _is_authorization (checks.py:152-158), keyed by login. checks.py:236-238 pulls created_at from those same comment dicts. GitHub's issue-comment API keeps created_at fixed at the first POST; only body and updated_at change on an edit. Trigger: an authorizer posted an unrelated comment on the CR issue long before the PR existed, then edits that same comment after implementation/review to add 'Authorized: proceed.'. _is_authorization now matches the edited body. checks.py:244-246 before_implementation computes max(authorization_times) <= pr['created_at'] using the frozen, pre-PR created_at, so it returns True even though the real authorization only exists after the edit. The same flaw hits ia_times/after_analysis at checks.py:239-243/247-249 for an edited Impact Analysis comment. Result: E1 (checks.py:260-265) reports a properly ordered, pre-implementation authorization for a change that was never actually authorized before implementation began, defeating E1's core purpose once enforcement flips to…
Verification: nit. Real and reachable, but advisory-only in the shipped warn mode and the trigger is a trusted AUTHORIZER deliberately backdating their own authorization — not an outside attacker. Mechanism (all lines confirmed): _is_authorization (checks.py:152-158) matches on comment.get('body','') — the CURRENT, editable body. authorization_comments is built from that (checks.py:231-234), then… | nit. The…
| return ( | ||
| login in AUTHORIZERS | ||
| and login != author.lower() | ||
| and bool(re.search(r'(?im)^\s*(?:decision:\s*)?authori[sz]ed\b', comment.get('body', ''))) |
There was a problem hiding this comment.
🟡 (optional) An authorizer who explicitly declines a Change Request is still counted as having authorized it, so E1 reports 'authorized' for a CR the authorizer rejected. _is_authorization (checks.py:152-158) only checks that a comment starts with the word "authorized" via ^\s*(?:decision:\s*)?authori[sz]ed\b; it never inspects the decision's polarity. A reply like "Authorized: No, needs more analysis before Class C" matches this regex and counts as a positive authorization. Fix: require an actual affirmative decision (e.g. reject the match when the next token is no/not/denied/declined), not merely the presence of the word "authorized", covering every phrasing an authorizer uses to decline.
Extended reasoning...
The CR issue template's own Authorization field says "Authorizer, decision, date and basis", so a realistic decline reads "Authorized: No — wire-format PR must land first" or "Authorized: No, insufficient hazard analysis". _is_authorization at checks.py:152-158 runs re.search(r'(?im)^\s*(?:decision:\s*)?authori[sz]ed\b', body) on this comment. The line starts with "Authorized" then a boundary (colon), so the regex matches regardless of what follows. In evaluate() (checks.py:232-234) this comment gets stored in authorization_comments keyed by the authorizer's login, counted toward required_authorizers, and E1 reports 'pass' (checks.py:265) with detail claiming a distinct pre-implementation authorizer exists. The existing test test_rejection_text_does_not_count_as_authorization (test_change_control.py:417-421) only covers a comment starting with "Rejected:", not one starting with "Authorized: No", so this gap is untested. In today's warn mode this produces a misleading passing advisory hiding an actual rejection; once enforcement-mode flips to enforce, E1 would let a…
Verification: nit (advisory in shipped warn mode; docs/process/enforcement-mode = "warn", so a wrong E1 result reports rather than blocks). The polarity blindness is real and reachable. _is_authorization (checks.py:152-158) only tests re.search(r'(?im)^\s*(?:decision:\s*)?authori[sz]ed\b', comment.get('body','')). For a decline written as "Authorized: No — wire-format PR must land first" the line…
3 participants
Why
The repository modification procedure defines authorization, analysis, classification, approval, and verification gates. This PR adds a consistent mechanical report for missing artifacts and ordering problems while deliberately keeping policy findings advisory during rollout.
What changed
docs/process/enforcement-modeset exactly towarnImpact
Policy findings return success in warn mode, so this PR does not unexpectedly block active pull requests. Missing configuration, partial API data, and collection failures still return exit 2 so an incomplete check cannot look clean. Runtime behavior, protocol signatures, safety requirements, and coverage calculations are unchanged.
Stack
change-0002-wire-breakchange-0002-change-control-warnchange-0002-coverage-delta)Review and merge only after #138. Before merge, refresh this branch against the wire branch final head and review the resulting diff.
Verification
GH_TOKENpre-commit run --all-files: all hooks passOriginal PR #120 is retained as closed recovery and review history.