feat: report safety-citation coverage deltas - #140
rajasimman-madhivanan wants to merge 1 commit into
Conversation
|
Coverage before: 32/40 cited.
|
|
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 coverage-delta change-control tool that runs each revision's own checked-in safety linter at the PR base and head in detached git worktree checkouts, then reports citation gains, losses, and newly unresolvable citations without asserting that any cited test actually ran. Publishes the result as a single marker-owned PR comment via upsert_coverage_comment (patch-if-present, else create), scrubs GH_TOKEN/GITHUB_TOKEN from the linter subprocess environment so untrusted revision code can't observe Actions credentials, and treats a base predating the tools/safety_lint dependency as unavailable rather than zero coverage. Wires this into a pull_request workflow that cancels superseded runs per PR and drops to --no-comment for fork PRs.
Worth a look
- Write-scoped GitHub token is exposed to PR-controlled Python code —
.github/workflows/coverage-delta.yml:26· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Attacker-controlled linter code executed from PR head revision —
tools/change_control/coverage_delta.py:74· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Citation extraction runs on new-revision code path, not the checked-out worktree code —
tools/change_control/coverage_delta.py:76· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Coverage comment upsert is a non-atomic GET-then-POST race —
tools/change_control/coverage_delta.py:126· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- citations subprocess JSONDecodeError not handled —
tools/change_control/coverage_delta.py:84· 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 — 35 functions depend on the 35 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 35 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: 35 function(s) in the blast radius were not formally verified this run
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| result = subprocess.run( | ||
| [sys.executable, '-m', 'tools.safety_lint', '--json'], | ||
| cwd=worktree, | ||
| env=child_environment, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if result.returncode == 2: | ||
| raise RuntimeError(result.stderr.strip() or 'safety linter could not run') | ||
| try: | ||
| report = json.loads(result.stdout) | ||
| except json.JSONDecodeError as error: | ||
| raise RuntimeError('safety linter emitted invalid JSON') from error | ||
| citation_code = ( | ||
| 'import json; from tools.safety_lint.runner import analyze; ' | ||
| 'print(json.dumps({r.sr_id: sorted(set(r.test_refs)) for r in analyze(".").trace}, sort_keys=True))' | ||
| ) | ||
| citations = subprocess.run( | ||
| [sys.executable, '-c', citation_code], | ||
| cwd=worktree, | ||
| env=child_environment, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
There was a problem hiding this comment.
🟡 (optional) Any PR author can hang the coverage-delta workflow indefinitely: the PR's own tools/safety_lint (fully attacker-controlled at the head revision) is run via subprocess.run at coverage_delta.py:71-78 and :89-96 with no timeout. The job has no timeout-minutes either, so a stray infinite loop in the checked-out linter blocks the runner up to GitHub Actions' default 360-minute job cap, wasting a runner and delaying the report/comment for that PR (base branch has no equivalent CI step at all). Fix: pass timeout= to both subprocess.run calls (and to the git worktree add in report_at_revision), catch subprocess.TimeoutExpired alongside the existing RuntimeError handling in main(), and terminate the child so a hung revision fails fast instead of occupying the runner.
Extended reasoning...
coverage-delta.yml runs on every pull_request and checks the head SHA into a detached worktree via report_at_revision. run_linter_at_tree then does subprocess.run([sys.executable, '-m', 'tools.safety_lint', '--json'], cwd=worktree, ...) at line 71 with no timeout keyword. Since the head revision is the PR's own tree, a PR can edit tools/safety_lint/main.py or any module it imports to add an infinite loop or heavy sleep. subprocess.run blocks the parent process waiting for the child to exit; nothing external kills it. The workflow job has no timeout-minutes set (grep across .github/workflows/*.yml shows none), so the job runs until GitHub Actions' own default maximum. The credential-scrubbing mitigation only protects against secret exfiltration, not against a hang, so it does not help here. The same unguarded pattern repeats for the second subprocess.run at lines 89-96 that extracts citations.
Verification: normal, security-relevant (resource exhaustion / CI DoS newly introduced by this change; base branch has no such workflow). The candidate is accurate. Both subprocess.run calls that execute the head revision's attacker-controlled linter code run with no timeout: - coverage_delta.py:71-78 runs [sys.executable, '-m', 'tools.safety_lint', '--json'] with cwd=worktree, env, check=False,… | nit. The…
Why: A headline requirements-coverage fraction can stay flat while individual safety requirements gain or lose cited evidence. Reviewers need a deterministic comparison that exposes those changes without claiming that citation presence proves test execution or safety sufficiency. What changed: - Run each revision's own safety linter in isolated detached worktrees. - Report coverage before and after, citation gains and losses, and newly unresolvable citations. - Update one marker-owned pull-request comment instead of appending duplicate reports. - Remove Actions credentials from revision-controlled linter subprocesses. - Support stdout-only reporting for fork pull requests and serialize comment writers per PR. - Add nine focused delta, isolation, pagination, fork, and workflow tests. Impact: This is advisory reporting only: it has no threshold and never gates a pull request based on the coverage delta. An unavailable head report fails explicitly, while a base that predates the safety linter is identified as unavailable rather than misreported as zero coverage. No runtime, wire, policy-mode, or safety-document content changes. Safety lifecycle: Verification traceability and change impact review. Bears on IEC 61508-1:2010 sections 7.14 and 7.16, and IEC 61508-3:2010 sections 7.4, 7.8, and 7.9. Co-Authored-By: OpenCode <noreply@opencode.ai>
6eb0861 to
2bf91e6
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 — 35 functions depend on the 35 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 35 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 595d172, 5 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: 35 function(s) in the blast radius were not formally verified this run
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding below, I checked two other angles and ruled them out: citation gains/losses are read directly from analyze(".").trace test_refs (a separate subprocess call), which bypasses the baseline-filtered findings list entirely, so a PR pairing a real citation regression with a matching docs/safety/lint-baseline.json suppression entry would still surface as a lost citation, not be hidden. Also, the workflow only wires the job-scoped GH_TOKEN into the step's env (no other secrets), so popping GH_TOKEN/GITHUB_TOKEN before running the PR's own linter code is sufficient for this specific job.
Extended reasoning...
Verified two of the candidate concerns from this run's ruled-out list directly against the code rather than trusting the harness summary. For the baseline-suppression scenario (coverage_delta.py:53-59, 85-99): run_linter_at_tree computes coverage via compute_coverage(analysis) in tools/safety_lint/__main__.py, which operates on the raw trace/analysis and is never filtered by apply_baseline; separately, the citations dict is built by a standalone python -c invocation of analyze(".").trace, again untouched by baseline suppression (baseline only filters the findings list used for C3/C4 "newly unresolvable" detection). So a lost citation would still show up in the "gained/lost citation(s)" section of compare_reports even if a matching baseline entry suppressed the corresponding finding message — the proposed hiding scenario doesn't hold. For the credential-stripping scope concern: .github/workflows/coverage-delta.yml only sets GH_TOKEN: ${{ github.token }} in the step env (no other repository/organization secrets are referenced), so run_linter_at_tree's pop of GH_TOKEN/GITHUB_TOKEN removes the only credential actually present before running the PR's own (fully attacker-controlled at head) linter code; there is no broader credential-leak surface in this specific job to worry about. Since a CONFIRMED finding (the duplicate-SR-ID dict comprehension at coverage_delta.py:87) is already being posted inline, and a prior review of an earlier version of this PR already flagged the missing-timeout issue on the same subprocess calls (still unaddressed in this diff, per the code read), this run adds only the narrow ruled-out note above rather than a full review body — no need to duplicate what the inline comment and prior review already communicate.
| raise RuntimeError('safety linter emitted invalid JSON') from error | ||
| citation_code = ( | ||
| 'import json; from tools.safety_lint.runner import analyze; ' | ||
| 'print(json.dumps({r.sr_id: sorted(set(r.test_refs)) for r in analyze(".").trace}, sort_keys=True))' |
There was a problem hiding this comment.
🟡 (optional) Reviewers can get a misleading coverage-delta comment when a safety requirement has more than one row in TRACEABILITY.md for the same SR ID (a state the existing C1 mismatch check flags but never blocks). The dict comprehension {r.sr_id: sorted(set(r.test_refs)) ...} at coverage_delta.py:87 keys citations by sr_id, so a later duplicate row silently overwrites an earlier row's test_refs before compare_reports() diffs base vs head. Fix: aggregate test_refs per sr_id across every matching TraceRow (e.g. union the sets) instead of a dict comprehension that keeps only the last row, so no real citation is lost when an SR ID appears more than once.
Extended reasoning...
analyze('.').trace is a tuple of TraceRow, one per table row; parse_traceability.py never enforces one row per sr_id. checks.py's mismatch check (around line 65-74) only appends a finding when trace_counts[sr_id]!=1; it does not remove the extra row or stop the run. coverage_delta.py:87 builds {r.sr_id: sorted(set(r.test_refs)) for r in analyze('.').trace}, so for a duplicated sr_id only the last row in file order survives in the map. compute_coverage() in coverage.py (unrelated, native linter output) iterates all rows without keying by sr_id, so only this new citations map loses data. compare_reports() then diffs base_citations vs head_citations per sr_id built this way: if the surviving row lacks a test ref an earlier, now-hidden row had, the PR comment reports a false 'lost citation' or hides a real 'gained citation' for that requirement.
Verification: nit. The mechanism is real and reachable. coverage_delta.py:87 keys the citation map by sr_id: {r.sr_id: sorted(set(r.test_refs)) for r in analyze(".").trace}. analyze('.').trace is a per-row tuple with no sr_id dedup — parse_traceability.py:227-253 simply appends one TraceRow per table row (unlike the reverse map at :258-262, which raises LintError on duplicate function IDs; trace rows have…
|
Fine with this once #139 is ready |
2 participants
Why
A headline requirements-coverage fraction can remain unchanged while individual safety requirements gain or lose cited evidence. This PR gives reviewers a deterministic before/after report without treating citation presence as proof that tests ran or passed.
What changed
Impact
This reporter is advisory only. It defines no threshold and never gates a pull request based on the delta. An unavailable head report is explicit failure; a base predating the safety linter is reported as unavailable rather than zero. Runtime behavior, wire format, change-control mode, and safety documents are unchanged.
Stack
change-0002-change-control-warnchange-0002-coverage-deltaReview and merge only after #139. Before merge, refresh this branch against the final #139 head and review the resulting diff.
Verification
32/40before and after, no citation changespre-commit run --all-files: all hooks passOriginal PR #120 is retained as closed recovery and review history.