feat(resume): opt-in grounded retraction of fixed findings#773
feat(resume): opt-in grounded retraction of fixed findings#773seanturner83 wants to merge 2 commits into
Conversation
On --resume, prior findings are rehydrated so the cumulative report/SARIF carries forward across runs. That set is append-only today: if a later run sees a finding has been FIXED, there's no way to drop it, so a resolved finding re-emits into every subsequent SARIF forever — a scanner alert that can never auto-resolve (fail-CLOSED). This adds the inverse of report creation, behind an explicit opt-in. - `ReportState.retract_vulnerability_report(id, reason)` — inverse of add_vulnerability_report: drops the report, deletes its stale MD, and lets save_run_data() rewrite CSV/JSON/SARIF from the reduced set. Idempotent. - `retract_vulnerability_report` tool, exposed on the root agent ONLY on a resumed run AND only when `STRIX_RESUME_RETRACT` is set. Default OFF — resume stays append-only (unchanged behaviour) unless opted in. - Groundedness guard: before dropping, re-verify against the live workspace tree (via an injected sandbox reader) that the vulnerable sink is actually gone. If a fix_before sink is still present verbatim → REFUSE; if no reader is wired → REFUSE (fail-safe). A confident-but-wrong agent can't silently drop a real finding. - id stability: allocate the next `vuln-NNNN` from the max ordinal ever seen (incl. retracted ids retained in _saved_vuln_ids), not the list length. Length-based ids recycle once a report can be removed — a later finding would reuse a retracted id and collide with its MD / SARIF fingerprint. No-op for append-only runs (max-seen == len there). Tests: retract removal/idempotency/reason-required/MD-deletion, id stability under removal, and the guard's refuse/allow paths. Existing report + state tests unchanged (46 pass).
Greptile SummaryThis PR adds opt-in retraction of fixed findings during resumed scans. The main changes are:
Confidence Score: 4/5The retraction path can remove live findings, and report IDs can be reused after a later resume.
strix/tools/reporting/retract_tool.py, strix/core/runner.py, strix/report/state.py
|
| Filename | Overview |
|---|---|
| strix/tools/reporting/retract_tool.py | Adds the retraction tool and guard, but missing sink data and process-global reader state can produce incorrect retractions. |
| strix/core/runner.py | Injects the sandbox reader, but failed or misdirected reads are reported as confirmed file absence. |
| strix/report/state.py | Adds report removal and max-based allocation, but the ID high-water mark does not survive rehydration. |
| strix/agents/factory.py | Adds default-off, resume-only registration of the root retraction tool. |
| strix/config/settings.py | Adds the default-off environment setting for resume retraction. |
| tests/test_retract_vulnerability.py | Covers basic retraction but omits cross-process ID reuse, failed reads, and shared-reader cases. |
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
strix/tools/reporting/retract_tool.py:78-79
**Missing Sink Bypasses Verification**
`fix_before` is optional in existing reports, but this branch allows retraction when no location contains it. A resumed finding without `fix_before` can therefore be removed without reading the current source, even when the vulnerability is still present.
### Issue 2 of 4
strix/core/runner.py:262-275
**Read Failures Become Missing Files**
The reader returns `None` after every candidate path fails, and the guard treats `None` as proof that the sink is gone. A changed or omitted `workspace_subdir`, a permission failure, or another nonzero `cat` result can therefore approve retraction while the vulnerable file still exists elsewhere in the workspace.
### Issue 3 of 4
strix/tools/reporting/retract_tool.py:46-52
**Reader State Crosses Scan Boundaries**
This single module-level reader is shared by every scan in the process and is never reset. Concurrent resumed scans can overwrite it and verify a finding against another scan's sandbox, while a later scan can retain a reader bound to an old session, producing an incorrect retraction decision.
### Issue 4 of 4
strix/report/state.py:224-229
**Retracted Maximum ID Is Reused**
The “ever seen” IDs exist only in memory and hydration rebuilds `_saved_vuln_ids` from live reports. If `vuln-0003` is the highest ID and is retracted, the next process loads only `0001` and `0002`, so this allocator issues `vuln-0003` again and breaks the promised stable report and SARIF identity.
Reviews (1): Last reviewed commit: "feat(resume): opt-in grounded retraction..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not sinks: | ||
| return True, "no fix-site (fix_before) locations to verify — allowing" |
There was a problem hiding this comment.
fix_before is optional in existing reports, but this branch allows retraction when no location contains it. A resumed finding without fix_before can therefore be removed without reading the current source, even when the vulnerability is still present.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/reporting/retract_tool.py
Line: 78-79
Comment:
**Missing Sink Bypasses Verification**
`fix_before` is optional in existing reports, but this branch allows retraction when no location contains it. A resumed finding without `fix_before` can therefore be removed without reading the current source, even when the vulnerability is still present.
How can I resolve this? If you propose a fix, please make it concise.| except Exception as exc: # noqa: BLE001 — unreadable ≠ proof of absence | ||
| logger.debug("retract reader: %s/%s unreadable: %r", base, rel, exc) | ||
| continue | ||
| if getattr(result, "exit_code", 1) == 0: | ||
| # ExecResult.stdout is bytes; the guard does a str | ||
| # substring check, so decode here (a raw bytes return | ||
| # would raise TypeError on every retract). | ||
| stdout = result.stdout | ||
| return ( | ||
| stdout.decode("utf-8", errors="replace") | ||
| if isinstance(stdout, bytes | bytearray) | ||
| else stdout | ||
| ) | ||
| return None # not found under any workspace root → treat as gone |
There was a problem hiding this comment.
The reader returns None after every candidate path fails, and the guard treats None as proof that the sink is gone. A changed or omitted workspace_subdir, a permission failure, or another nonzero cat result can therefore approve retraction while the vulnerable file still exists elsewhere in the workspace.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/core/runner.py
Line: 262-275
Comment:
**Read Failures Become Missing Files**
The reader returns `None` after every candidate path fails, and the guard treats `None` as proof that the sink is gone. A changed or omitted `workspace_subdir`, a permission failure, or another nonzero `cat` result can therefore approve retraction while the vulnerable file still exists elsewhere in the workspace.
How can I resolve this? If you propose a fix, please make it concise.| _reader: list[ReadTargetFileFn | None] = [None] | ||
|
|
||
|
|
||
| def set_target_file_reader(reader: ReadTargetFileFn | None) -> None: | ||
| """Wire the sandbox-backed target-file reader (called at agent build time on | ||
| resume). Passing None disables grounded retraction (fail-safe refuse).""" | ||
| _reader[0] = reader |
There was a problem hiding this comment.
This single module-level reader is shared by every scan in the process and is never reset. Concurrent resumed scans can overwrite it and verify a finding against another scan's sandbox, while a later scan can retain a reader bound to an old session, producing an incorrect retraction decision.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/reporting/retract_tool.py
Line: 46-52
Comment:
**Reader State Crosses Scan Boundaries**
This single module-level reader is shared by every scan in the process and is never reset. Concurrent resumed scans can overwrite it and verify a finding against another scan's sandbox, while a later scan can retain a reader bound to an old session, producing an incorrect retraction decision.
How can I resolve this? If you propose a fix, please make it concise.| max_ordinal = 0 | ||
| for rid in ( | ||
| *(r.get("id") for r in self.vulnerability_reports), | ||
| *self._saved_vuln_ids, | ||
| ): | ||
| if isinstance(rid, str) and rid.startswith("vuln-"): |
There was a problem hiding this comment.
The “ever seen” IDs exist only in memory and hydration rebuilds _saved_vuln_ids from live reports. If vuln-0003 is the highest ID and is retracted, the next process loads only 0001 and 0002, so this allocator issues vuln-0003 again and breaks the promised stable report and SARIF identity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/report/state.py
Line: 224-229
Comment:
**Retracted Maximum ID Is Reused**
The “ever seen” IDs exist only in memory and hydration rebuilds `_saved_vuln_ids` from live reports. If `vuln-0003` is the highest ID and is retracted, the next process loads only `0001` and `0002`, so this allocator issues `vuln-0003` again and breaks the promised stable report and SARIF identity.
How can I resolve this? If you propose a fix, please make it concise.… docs
Address Greptile review on the retract PR:
- Trust model (the guard never vetoes): Strix trusts the LLM to find a vuln,
so it trusts the LLM's judgement that it's fixed. The retract now ALWAYS
proceeds on the agent's cited reason; the guard is an audit signal, not a
gate. It returns a 3-way verdict — gone / present / inconclusive — recorded
on the result as `grounded` + `guard_verdict`. This fixes the previous
fail-OPEN paths (no fix_before sink, failed read → silently allowed) AND the
opposite fail-CLOSED risk (stubbornly refusing a genuine fix on a moved file
or a finding with no structured sink). A wrong drop is self-correcting:
findings are re-derived every scan, so it re-appears next run.
- id high-water mark now PERSISTS (run_record["max_vuln_ordinal"], restored by
hydrate_from_run_dir). Without it a retracted top id was reused after a
resume in a fresh process — _saved_vuln_ids is rebuilt only from live reports
on rehydration. (Greptile issue 4.)
- reader hygiene: the sandbox reader now returns None ONLY on positively-
confirmed absence ("No such file") and RAISES on any other failed/ambiguous
read, so "couldn't read" is scored inconclusive, never mistaken for "gone"
(issue 2). Lifted to a module-level `_make_workspace_file_reader` (testable,
and no longer a 50-line closure in the scan body). The reader is reset to
None in the run's finally block so a torn-down session can't leak into a
later scan's grounding (issue 3).
- docs: STRIX_RESUME_RETRACT in configuration.mdx; a "Resuming a scan" +
"Retracting fixed findings" section in usage/cli.mdx documenting the opt-in
and the trust model.
Tests updated to the 3-way verdict + tool-always-retracts behaviour; 60 tests
pass (14 retract + 46 existing), ruff clean.
|
Thanks @greptile-apps — this was a genuinely useful pass. Addressed in 986c974: Issue 4 (retracted max ID reused after resume) — real bug, fixed. You're right: Issue 3 (reader crosses scan boundaries) — fixed. The reader is reset to Issues 1 & 2 (missing sink / failed read → retraction allowed) — reworked, but by design rather than as a hard block. These flagged a real tension, and the resolution is a deliberate trust-model choice worth stating explicitly: Strix trusts the LLM to find a vulnerability, so it trusts the LLM's judgement that one is fixed. The guard therefore never vetoes the agent — a hard refuse would swing into the opposite failure: stubbornly keeping a genuinely-fixed finding the guard just can't re-locate (moved/renamed file, a finding with no structured
The reader change from Issue 2 still matters for honesty here: it now returns Documented the behaviour + trust model in 60 tests pass (14 retract + 46 existing), ruff clean. |
|
The rework in 986c974 is well-executed. All four original issues are resolved, and the trust-model rationale is clearly documented at both the code and docs level. A few observations on the final shape: Trust model (Issues 1 & 2) — the framing is correct. The 3-way verdict with One minor audit-log gap worth considering: ID high-water persistence (Issue 4) — correct. Reader hygiene (Issue 3) — clean. Lifting Overall this is solid — the feature is opt-in, fail-safe by default, self-correcting by design, and now correctly auditable. Happy to approve from a review standpoint. |
1 participant
Problem
On
--resume, prior findings are rehydrated so the cumulative report/SARIF carries forward across runs. That set is append-only today: if a later resumed run sees a finding has been fixed, there's no way to drop it. The resolved finding re-emits into every subsequent SARIF forever — a scanner alert (e.g. GHAS) that can never auto-resolve (fail-closed).This matters when resume is driven as a PR-lifecycle loop (rescan on each push): the cumulative finding set needs to shrink as fixes land, not just grow.
What this adds (opt-in, default off)
ReportState.retract_vulnerability_report(id, reason)— the inverse ofadd_vulnerability_report: drops the report, deletes its stale per-finding MD, and letssave_run_data()rewrite CSV/JSON/SARIF from the reduced set. Idempotent.retract_vulnerability_reporttool — exposed on the root agent only on a resumed run and only whenSTRIX_RESUME_RETRACTis set. Default OFF: with the flag unset, resume behaves exactly as today (append-only) — this PR is a no-op for existing users until they opt in.Groundedness guard — before dropping, the tool re-verifies against the live
/workspacetree (via an injected sandbox reader) that the vulnerable sink is actually gone. If a finding'sfix_beforesink is still present verbatim → REFUSE; if no reader is wired → REFUSE (fail-safe). A confident-but-wrong agent cannot silently drop a real finding.Prerequisite bugfix: id stability under removal (ungated)
add_vulnerability_reportallocated ids asvuln-{len(reports)+1}. That's only correct while the set is append-only — once a report can be removed, length-based ids recycle: retractvuln-0002from[0001,0002,0003]and the next add computeslen+1 = 3→vuln-0003, colliding with a live id (overwriting its MD) and letting a retracted id later re-emit under a different finding (SARIF fingerprint collision).Fixed by allocating from the max ordinal ever seen (including retracted ids retained in
_saved_vuln_ids), so ids stay monotonic and stable under removal. This is a no-op for append-only runs (max-seen == len there), so it doesn't change existing behaviour — it just makes retraction safe.Why this shape
The retraction capability lives in strix (report state + a resume-gated tool), not in the SDK — it's specific to how strix manages a cumulative, resumable finding set. Kept fully behind an env flag so it lands dormant and opt-in.
Tests
tests/test_retract_vulnerability.py(9 tests): retract removal / idempotency / reason-required / MD-deletion, id stability under removal, and the guard's refuse-without-reader / refuse-when-sink-present / allow-when-gone / allow-when-nothing-verifiable paths. Existing report + state tests unchanged (46 pass). ruff clean.