feat: enforce reviewed wire-format changes - #138
rajasimman-madhivanan wants to merge 1 commit into
Conversation
![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 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a wire-format change-control guard that flags unannounced pstop_c protocol breaks in PRs. The wire-break workflow runs the self-tests, then labels a PR wire-break when the recorded wire_format.sha256 signature moves — detected either by a watched-file diff (falling back to a live check when no prior expectation exists) — and check_wire_format.sh enforces the declaration, refusing to run (exit 2) unless PSTOP_BASE_SHA is exactly 40 hex chars so option-like or short values never reach git. Signatures are computed to ignore comment-only edits while catching field reordering, byte-order, and CRC-polynomial changes across the watched headers and sources.
Worth a look
- Comment stripping ignores C string literals and can hide source changes —
tools/change_control/wire_format.py:37· 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 — 75 functions depend on the 75 functions this change touches.
Health — this change adds coupling hotspots:
- new:
compute_signature()— 12 callers, 3 callees
Verification — 75 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: 75 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below.
| return match.group(1) | ||
|
|
||
|
|
||
| def compute_signature(root): |
There was a problem hiding this comment.
compute_signature()
12 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 checked two other candidate issues and ruled them out: the bootstrap label-detection logic (wire-break.yml:44-47) uses grep -Fqx -f against the full multi-line watched_paths list, so it actually matches all 11 watched wire files, not just config.h. I also looked at whether a label name containing a literal comma could spoof the comma-joined wire-break/class-c label check (wire_format.py:152), but exploiting it would require a maintainer to have created an unusually named label first, making it a low-probability edge case rather than an externally exploitable gap.
Extended reasoning...
Two CONFIRMED findings already exist for this PR (the pull_request trigger missing types: so label additions don't rerun the required check, and unconditional gh api calls with no retry/fallback that can fail any PR's required check on transient GitHub API errors), so a full approve/defer verdict is not appropriate here — the inline comments already signal that human review is needed. I independently re-read .github/workflows/wire-break.yml, scripts/check_wire_format.sh, and tools/change_control/wire_format.py to confirm the mechanics behind two candidate issues that a prior pass had ruled out: (1) whether the bootstrap detection path (expectation_preexisted=false) only catches config.h changes — tracing the grep -Fqx -f <(printf '%s\n' "$watched_paths") invocation shows the pattern file has all 11 watched paths on separate lines, so -Fqx fixed/exact-line matching correctly covers all of them, not just config.h; and (2) whether a label whose name contains a comma could satisfy the comma-joined --labels string and get mis-split into extra "labels" at wire_format.py:152, which is technically true only if a maintainer had already created and attached an oddly-named label, making it a narrow, low-likelihood edge case rather than something an external PR author could trigger. Both are worth recording briefly since they are not obvious from the PR thread and add information beyond what's in the inline comments, without restating those comments' content.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| on: | ||
| pull_request: |
There was a problem hiding this comment.
🔴 Maintainers who add the required wire-break/class-c labels to unblock a failing PR find the required check stays red, because pull_request here has no types: list, so it defaults to opened/synchronize/reopened only. GitHub does not rerun a workflow on a labeled event, so adding the labels the check itself demands (wire_format.py:125-129) never produces a new passing run. Fix: add labeled (and unlabeled) to the pull_request trigger's types: so a label change retriggers wire-break and the guard reevaluates with the new labels, without needing an extra commit or a manual Actions re-run.
Extended reasoning...
A PR edits a watched wire file without the wire-break/class-c labels. The 'Enforce wire-format declaration' step calls check_wire_format(), which returns exit 1 with 'required PR labels missing: class-c, wire-break' (wire_format.py:125-129), and this is a required status check per docs/process/BRANCH_PROTECTION.md. A maintainer adds both labels via the GitHub UI. Since .github/workflows/wire-break.yml:4-5 declares only pull_request: with no types:, GitHub's default trigger set (opened, synchronize, reopened) does not include labeled, so no new workflow run starts. The failing 'wire-break' check from the earlier run remains the latest result. Branch protection keeps the merge button blocked until a new commit (synchronize) is pushed or someone manually re-runs the job from the Actions tab, even though the guard's own label condition is now satisfied.
Verification: normal (with a manual workaround qualifying the candidate's "never" wording). The workflow at .github/workflows/wire-break.yml:4-5 uses on: pull_request: with no types:, so it fires only on GitHub's default set (opened, synchronize, reopened); labeled/unlabeled are excluded. The wire-break job is a required status check per docs/process/BRANCH_PROTECTION.md:11 ("Required status…
| set -eu | ||
| signature_changed=false | ||
| expectation_preexisted=false | ||
| if git cat-file -e "$BASE_SHA:tools/change_control/wire_format.sha256" 2>/dev/null; then | ||
| expectation_preexisted=true | ||
| fi | ||
| watched_paths='pstop_c/pstop/include/pstop/config.h | ||
| pstop_c/pstop/include/pstop/constants.h | ||
| pstop_c/pstop/include/pstop/protocol.h | ||
| pstop_c/pstop/include/pstop/protocol_data.h | ||
| pstop_c/pstop/include/pstop/pstop_msg.h | ||
| pstop_c/pstop/include/pstop/checksum.h | ||
| pstop_c/pstop/include/pstop/device_id.h | ||
| pstop_c/pstop/include/pstop/endian.h | ||
| pstop_c/pstop/src/pstop/pstop_msg.c | ||
| pstop_c/pstop/src/pstop/checksum.c | ||
| pstop_c/pstop/src/pstop/endian.c' | ||
| if { [ "$expectation_preexisted" = true ] && \ | ||
| git diff --name-only "$BASE_SHA"...HEAD | grep -Eq '^tools/change_control/wire_format\.sha256$'; } || \ | ||
| { [ "$expectation_preexisted" = false ] && \ | ||
| git diff --name-only "$BASE_SHA"...HEAD | grep -Fqx -f <(printf '%s\n' "$watched_paths"); }; then | ||
| signature_changed=true | ||
| elif ! python3 -m tools.change_control.wire_format check --root . >/dev/null; then | ||
| signature_changed=true | ||
| fi | ||
| if [ "$signature_changed" = true ] && [ "$CAN_LABEL" = true ]; then | ||
| gh api --method POST 'repos/${{ github.repository }}/issues/'"$PR_NUMBER"'/labels' -f 'labels[]=wire-break' >/dev/null | ||
| fi | ||
| labels=$(gh api 'repos/${{ github.repository }}/issues/'"$PR_NUMBER" --jq '[.labels[].name] | join(",")') |
There was a problem hiding this comment.
🟡 (optional) Any PR, even ones that never touch pstop_c wire files, can now fail its required check purely from a transient GitHub API error, because this new workflow has no retry or fallback. The gh api label GET at line 55 (and the POST at line 53) run unconditionally under set -eu (line 27) for every pull_request event, since on: pull_request (lines 4-5) has no path filter; if gh api returns a transient failure (rate limit, 5xx), the labels=$(...) assignment fails and set -e aborts the whole step, failing the job for a PR unrelated to wire-format. Fix: wrap both gh api calls with retry/backoff (or continue-on-error) and fall back to an empty/last-known label set on failure instead of letting the step abort the job.
Extended reasoning...
The workflow triggers on every pull_request event for every PR (no paths: filter at .github/workflows/wire-break.yml lines 4-5). The 'Detect watched changes and apply wire-break label' step sets set -eu at line 27, then unconditionally runs gh api 'repos/.../issues/$PR_NUMBER' --jq '[.labels[].name] | join(",")' at line 55 into labels=$(...). If gh api fails transiently (secondary rate limit, network blip, GH 5xx), the command substitution's exit status propagates to the assignment. Under set -e, a failing simple command (including var=$(cmd) assignments) terminates the script immediately, so the step exits nonzero and the whole job fails. This happens for every PR regardless of whether it touches any watched wire file, since the step runs before any wire-specific gating. Before this diff no such workflow existed, so this failure mode is entirely new, and it has no retry, continue-on-error, or default fallback for labels.
Verification: nit. The mechanism is real and reachable as described: line 27 set -eu, line 55 labels=$(gh api 'repos/.../issues/'"$PR_NUMBER" --jq '...'). In bash a simple assignment whose command substitution exits non-zero returns that status, and under set -e the step aborts; a transient gh api failure (5xx, secondary rate limit, network blip) therefore fails the step. on: pull_request: (lines…
| [ -z "$path" ] || args+=(--changed-file "$path") | ||
| done <<< "$changed" | ||
| fi | ||
| python3 -m tools.change_control.wire_format "${args[@]}" |
There was a problem hiding this comment.
we'll want to use uv here now that I've landed #137
There was a problem hiding this comment.
I added a test/ directory in #137 so we can throiw this in there and the tools CI will run this test with it
| { [ "$expectation_preexisted" = false ] && \ | ||
| git diff --name-only "$BASE_SHA"...HEAD | grep -Fqx -f <(printf '%s\n' "$watched_paths"); }; then | ||
| signature_changed=true | ||
| elif ! python3 -m tools.change_control.wire_format check --root . >/dev/null; then |
| with: | ||
| fetch-depth: 0 | ||
| - name: Run wire-format self-tests | ||
| run: python3 tools/change_control/self_test.py |
Protective-stop participants depend on a byte-exact protocol, but source review alone does not reveal every compatibility break. Add a deterministic signature and an enforcing pull-request guard so incompatible changes require an explicit coordinated rollout declaration. ## What changed - Added normalized signatures for eight protocol headers and three encoding sources. - Added an enforcing wire-break workflow with automatic labeling and fork-safe behavior. - Required wire-break and class-c declarations when the reviewed expectation changes. - Added 25 focused contract, bootstrap, CLI, and workflow tests. - Added a discovery runner for the stacked change-control test modules. ## Safety lifecycle Modification implementation, verification, 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>
d1b0035 to
f2269bb
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 — 75 functions depend on the 75 functions this change touches.
Health — this change adds coupling hotspots:
- new:
compute_signature()— 12 callers, 3 callees
Verification — 75 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 595d172, 2 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: 75 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below.
| return match.group(1) | ||
|
|
||
|
|
||
| def compute_signature(root): |
There was a problem hiding this comment.
compute_signature()
12 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Beyond the inline finding on self_test.py not surfacing under pytest, I also checked two other angles from this run's investigation: the workflow's bootstrap watched-path list (wire-break.yml:33-43) matches wire_format.py's WIRE_PATHS exactly, so first-snapshot auto-labeling isn't limited to config.h as one candidate suggested; and test_wire_format.py:304's bootstrap-matcher test only asserts the path list itself, not that the guard actually blocks a simultaneous wire change end-to-end, so it doesn't add coverage for that specific guarantee. Neither changes the outcome of the confirmed finding above.
Extended reasoning...
This run's inline finding (self_test.py silently producing no pytest output) is a real gap in the local dev feedback loop and stands on its own as a reason a human should look. Beyond it, I re-examined two items from the ruled-out list to make sure nothing was missed: the bootstrap label matcher's watched-path list in wire-break.yml (lines 33-43) is textually identical to wire_format.py's WIRE_PATHS tuple, so the claim that it skips files other than config.h does not hold up against the current code. Separately, test_wire_format.py:304's docstring implies it verifies the "cannot hide simultaneous wire changes" guarantee, but on inspection it only checks that the watched-path regex/list construction includes all wire files — it does not invoke check_wire_format or the workflow logic to confirm enforcement actually fires, so it is a narrower test than its name suggests without being incorrect. Prior review rounds already flagged the wire-break.yml trigger (missing labeled/unlabeled types) and the unguarded gh api calls, both of which remain unaddressed in the current single-commit diff; I am not repeating those since they were already posted and nothing here addresses them, per the no-repeat guidance.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
|
||
|
|
||
| def main(): | ||
| suite = unittest.defaultTestLoader.discover(Path(__file__).parent, pattern='test_*.py') |
There was a problem hiding this comment.
🟡 (optional) Developers running uv run pytest in tools/ (the project's documented local check floor) get zero feedback from the 25 new wire-format tests, unlike every other tools/ suite. tools/change_control/self_test.py:12 only calls unittest.defaultTestLoader.discover() and defines no TestCase itself, so pytest finds nothing there even if pointed at it; tools/safety_lint/self_test.py instead defines TestCase subclasses directly, which is why its pyproject.toml testpaths entry actually makes pytest run it. Fix: make tools/change_control's tests pytest-discoverable too (list tools/change_control/test_wire_format.py in tools/pyproject.toml testpaths, or give self_test.py real TestCase classes like safety_lint's) so local uv run pytest exercises them, not only CI's explicit self_test.py step.
Extended reasoning...
tools/pyproject.toml testpaths currently lists only 'safety_lint/self_test.py' and 'test' (this diff does not touch pyproject.toml). A developer edits pstop_c wire files and the new wire_format.sha256, then runs the documented uv run pytest before pushing to get a clean local signal. pytest never collects tools/change_control/test_wire_format.py because it is outside testpaths, and even adding 'change_control/self_test.py' to testpaths the same way safety_lint's entry works would not help: self_test.py has no test_* functions or TestCase classes of its own, only a discover()+TextTestRunner main(). Only CI's explicit python3 tools/change_control/self_test.py step (wire-break.yml:19) ever runs these 25 tests; a broken wire-format check or self-test regression is invisible until CI, unlike safety_lint's suite which safety_lint/self_test.py:TestCase classes make locally visible via plain pytest.
Verification: nit. The mechanism the candidate describes is factually real and verifiable, but nothing breaks and it is not a regression against base. Confirmed mechanics: - tools/change_control/self_test.py:11-13 only calls unittest.defaultTestLoader.discover(Path(__file__).parent, pattern='test_*.py') inside main(); the module defines no TestCase and no top-level test_* functions, so pytest pointed at…
2 participants
What this delivers
The wire-format enforcement layer from PR #120, isolated as the first child of #136. It fingerprints the protocol contract and blocks undeclared compatibility changes while retaining the existing wire-break and Class C review requirements.
No general change-control policy, enforcement mode, coverage delta, runtime source modification, or safety requirement change is included.
Stack
change-0002-process-foundation)change-0002-wire-breakchange-0002-change-control-warn)change-0002-coverage-delta)Review and merge this PR only after #136. Before merge, refresh it against #136 final head and review the resulting diff.
Classification
Class C tooling because this guard enforces review declarations for the byte-exact protocol boundary. It does not change the current runtime protocol or any
pstop_c/source.Review scope
pstop_msg.c,checksum.c, andendian.cwire-breaklabelingVerification
0x02Uand PSTOP_MESSAGE_SIZE48Ud0819037896320c1f40b88573770809b42e4a11b7055d65c4991c598116eee4epre-commit run --all-files: all hooks passpstop_c/, safety requirement, enforcement-mode, or coverage changesOriginal PR #120 is retained as closed recovery and review history.