Skip to content

fix: checkpoint reuse now tree-checks cached verdicts against current HEAD - #225

Open
schmug wants to merge 1 commit into
mainfrom
claude/relaxed-sagan-c818as
Open

schmug wants to merge 1 commit into
mainfrom
claude/relaxed-sagan-c818as

Conversation

@schmug

@schmug schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • ckptReusable (byte-identical across issue-triage-fanout, issue-research-fanout, pr-triage-fanout) previously reused a cached entry purely because the issue/PR's updatedAt was unchanged — but a triage verdict is a claim about repo state, not just issue text, so an untouched issue could keep serving an arbitrarily stale verdict forever (the #15 repro in the issue).
  • Each checkpoint entry now also carries the repo HEAD sha it was computed against. On reuse, an entry whose cached sha differs from the current HEAD is kept only if a new, conditionally-spawned ckpt-treecheck agent's batched git diff --name-only <sha> HEAD (one call per run, per distinct stale sha — never per item) proves the diff does not touch that entry's own files[] footprint. An unknown/empty footprint (every pr-triage-fanout entry, and every non-GREEN issue-research-fanout verdict) fails closed to any non-empty diff — the same direction as a plain HEAD-sha key.
  • SPINE_VERSION bumped 1.0.01.1.0 in all three files so every pre-Checkpoint reuse ignores repo HEAD, re-serving verdicts about a stale tree #223 cached entry (none carries headSha) recomputes exactly once and is stamped going forward.

Closes #223

Test plan

  • npm test1259 passing, 0 failing (up from the base commit; includes ~50 new tests for the tree-check across the three sims, plus a test asserting ckptReusable's md5 stays identical across all three files).
  • New tests cover: a diff touching the cached footprint invalidates it; a disjoint diff still reuses; an unknown footprint fails closed on any tree change; a byte-identical diff (net-zero) still reuses; an unresolved diff or unresolved current HEAD fails closed; the tree-check agent is spawned exactly once per run (batched across distinct stale shas) and never at all when nothing is stale or under args.fresh; a fresh entry is stamped with the resolved HEAD sha; the exact #15-shaped repro (a cached BLOCKED verdict recomputes once its blocker's file lands, with no updatedAt change).

Choices made

  • Direction of the tradeoff (per the issue's "Reasoning guidance"): chosen Option 2 (key on a scoped tree hash via each entry's own files[]) with Option 1 (plain HEAD-sha key) as the fail-closed fallback when files[] is unknown — exactly the issue's own recommendation. Every failure mode (unresolved current HEAD, unresolved historical sha, unknown footprint) fails closed to a recompute rather than a silent reuse, since the bug this fixes is itself a silent-stale-reuse.
  • pr-triage-fanout has no files[] field at all (its schema never carried one — a PR's mergeability/CI verdict isn't framed as "which files does this depend on"). Rather than special-case it, the same uniform ckptReusable applies: every entry there has an unknown footprint, so any non-empty tree diff invalidates it. This is actually correct, not just a fallback — a PR's mergeability/CI can change from an unrelated commit landing on the base branch alone, with zero change to the PR itself or its updatedAt, which was itself a real (if unremarked) instance of the same bug class. Since ckptReusable still needed the normFiles/fileKey/normPath comparison helpers to stay byte-identical across all three files, I copied that leaf-function trio (not the full file-overlap engine, which pr-triage-fanout has no use for) into pr-triage-fanout.js too.
  • Batching: the tree-check agent resolves current HEAD once (folded into the existing ckpt-meta call, no extra agent) and is only spawned at all when at least one candidate's cached headSha differs from the current HEAD — one call for the whole run, deduped over distinct stale shas, never one per item. A no-change re-run still spawns zero relay/classify and zero tree-check agents.
  • "Demonstrated by a real two-commit run (not a sim)": this repo's test philosophy (see CLAUDE.md) is offline AsyncFunction-wrapped simulators with stubbed runtime globals — there's no harness in this repo for spawning a live two-commit Workflow run, and every other checkpoint/injection-hardening behavior here is verified the same way. I've encoded that exact scenario as a sim test (#223 the exact repro: a cached BLOCKED verdict is recomputed once its blocker's file lands) using fixture git-diff output instead of a real commit pair. Flagging this rather than silently claiming the literal acceptance wording is met.

Deferred

None of the issue's Acceptance items are unshipped — see the choice above on how the "real two-commit run" item was interpreted given this repo's sim-only test infra.

Note on review

This diff doesn't touch plugin.json, .claude-plugin/, or the untrusted-issue-text injection fence/preamble/nonce mechanics of any workflow, but it does touch the checkpoint logic of all three security-sensitive read-only fan-outs and adds a new agent (ckpt-treecheck) whose prompt embeds model-generated files[] path strings into a git diff -- <paths> pathspec. That's read-only and terminated by -- (no shell injection surface), but it's close enough to this repo's security-critical surface that I'm leaving auto-merge off for a human look, per the repo's own auto-merge policy erring toward review for anything security-adjacent in these workflows.


🤖 Generated with Claude Code

https://claude.ai/code/session_01NnAzwVH94FkU5tpsrjr3hs


Generated by Claude Code

… HEAD (#223)

ckptReusable (byte-identical in issue-triage-fanout, issue-research-fanout,
pr-triage-fanout) previously reused a cached entry purely because the issue/PR's
`updatedAt` was unchanged — but a triage verdict is a claim about REPO STATE
("is this already done", "does this file exist"), not just about the issue
text, so an untouched issue kept serving an arbitrarily stale verdict forever.

Each entry now also carries the HEAD sha it was computed against. On reuse,
an entry whose cached sha differs from the current HEAD is only kept if a
batched `git diff --name-only <sha> HEAD` (one ckpt-treecheck agent call per
run, never per item) proves the diff does not touch that entry's own files[]
footprint; an unknown/empty footprint (every pr-triage entry, and non-GREEN
research verdicts) fails closed to any non-empty diff. SPINE_VERSION is
bumped 1.0.0 -> 1.1.0 so every pre-#223 entry (no headSha) recomputes once.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnAzwVH94FkU5tpsrjr3hs

schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

The github-advanced-security check (GitHub's Copilot-based automated code-scanning review, autofind.js) failed on this PR's head commit (2c61674), but not on anything in this diff — its job log shows it died during Copilot session setup, before analyzing any repo content:

Creating copilot-sdk session with model: claude-opus-5[ReasoningEffort=medium] ...
Error creating PR review request: SessionModelError: Execution failed: CAPIError: 400 The requested model is not supported.

That's a GitHub-side model-routing/availability error in the Copilot backend (api.individual.githubcopilot.com), not a finding about this PR's code — it never got as far as reading the diff. No fix exists to port; there's nothing in this repo that could cause a 400: model not supported from GitHub's own Copilot API. Re-running the job once to confirm.

All other checks are green: test (Node 20), test (Node 22), CodeQL, Analyze (actions), Analyze (javascript-typescript).


Generated by Claude Code


Generated by Claude Code

schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Follow-up: the re-run attempt itself was blocked — rerun-failed-jobs on that run returned 403 This workflow run cannot be retried. This is GitHub's own Copilot code-scanning review job, not a workflow file in this repo, so it isn't retriable through the normal Actions API from here. I don't have another way to force it to re-run.

Leaving this PR subscribed and watched; if the check re-fires on its own (e.g. on the next push, or GitHub retries it server-side) I'll act on the result then. This is not blocking merge — it's not a required status check, and every check that actually reviews this PR's code (test (Node 20), test (Node 22), CodeQL, both Analyze jobs) is green.


Generated by Claude Code


Generated by Claude Code

schmug commented Sep 14, 2026

Copy link
Copy Markdown
Owner Author

Auto-merge routine: blocked — confirmed medium-severity findings

pr-review-fanout (read-only deep review) surfaced 2 confirmed medium-severity findings (of 10 confirmed total; 8 low). No confirmed security finding survived verification (2 candidates were refuted).

  1. .claude/workflows/pr-triage-fanout.js:676 — missing-branch-coverage (tests). The SHAS_TO_CHECK batched-tree-check pattern is copy-pasted verbatim into pr-triage-fanout.js and issue-research-fanout.js from issue-triage-fanout.js, but the dedicated "multiple candidates at DISTINCT stale shas are tree-checked in ONE batched call" regression test exists only in tests/issue-triage-sim.test.mjs — the two sibling suites have no equivalent, so a regression in either file's batching would go undetected.
  2. tests/issue-triage-sim.test.mjs:637 — assertion-gap (tests). The new byte-identity drift guard (ckptReusable is BYTE-IDENTICAL across all three checkpointed workflows) only compares ckptReusable's body; it never covers the new normPath/fileKey/normFiles helper trio that PR fix: checkpoint reuse now tree-checks cached verdicts against current HEAD #225 also copies verbatim into pr-triage-fanout.js, so drift in those helpers across the three files would go silently unguarded.

Additionally, security-diff-scan could not complete this run — the gh CLI binary is unavailable in this session's environment (exit 127), so its fixed-command relay had nothing to fetch and returned "0 files changed" (explicitly flagged by the workflow itself as "nothing to review, NOT clean," not a passing scan). Treat the security-scan gate as not run, not as clean.

Per the merge routine's gate, a confirmed medium+ finding blocks auto-merge regardless of CI/risk-path status. Applying needs-you — please review the two findings above and confirm whether security-diff-scan needs to be re-run manually before this merges.


Generated by Claude Code

@schmug schmug added the needs-you Escalated to a human; agents must stop and not act label Sep 14, 2026 — with Claude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-you Escalated to a human; agents must stop and not act

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Checkpoint reuse ignores repo HEAD, re-serving verdicts about a stale tree

2 participants