Repository navigation
Conversation
…its own state dir Evidence was cached one record per transcript in `.backpass/evidence/`, so training two root memory files in turn with `--memory-file` (or a memory `--target`) overwrote one file's evidence and proposal with the other's, and switching back re-analyzed every transcript though nothing had changed. A run whose trained root file is not the one a default run would train now keeps its per-surface files (evidence/, evidence-summary.json, proposal.json, prompts/, synthesis/, raw/, apply/) in `.backpass/targets/<file>-<hash>/`, keyed on the resolved primary path so `--memory-file X` and `--target X` share one directory. The scan cache, rejections, gap ledger (already keyed by memory path), probe cache, and fetched host transcripts stay shared in the scope root. A default run's layout is unchanged, and user-scope target state stays inside the user state directory. `backpass apply --memory-file X` reads X's proposal; the propose/run hint names that command, and a bare `apply` or `status` lists target proposals still waiting. Fixes kunchenguid#189 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 [Medium impact] The PR appears safe to merge, with a non-blocking issue in how overlapping reviews update the waiting list. Reviews (3) · Last reviewed commit: "no-mistakes(ci): I fixed ci-1 the way yo..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…d, with a regression test for each. All three tests fail without the fix and pass with it. `pnpm run check` passes: lint, format, typecheck and all 936 tests. - **ci-1 (fully reviewed proposals stayed listed):** A proposal waiting for review must be one that is neither applied nor fully reviewed. `cmdApply` in `src/commands/apply.js` now saves a `reviewedAt` timestamp on the proposal when every edit was decided (accepted or rejected), nothing failed, and it was not a dry run. `listTargetProposals()` in `src/state.js` now skips proposals with `reviewedAt`, so `status`, `status --json` and the bare-`apply` hints all stop listing them. A proposal with any undecided edit stays listed. Test: a dry run and a partial `--decisions e1=rejected` leave the proposal listed; deciding both edits removes it from `status --json` and from bare `apply`'s hint. - **ci-2 (suggested commands could break on unusual filenames):** Every suggested command must give the shell the exact filename as one argument. All the apply, propose and re-run hints are built by `stateCommand` in `src/target.js`. It now leaves plainly safe paths unquoted (so `CLAUDE.md` still prints as before) and wraps anything else in POSIX single quotes, escaping any `'` inside. Test: a file named ``O'Reilly $HOME `id` notes.md`` gives a command that `sh` splits back into the exact filename, and bare `apply` prints the same command. - **ci-3 (`status --json` missed target proposals):** The JSON output must show the same waiting proposals as the text output. `status --json` now includes `targetProposals: [{ path, edits, apply }]`, and the text output prints from the same list. Test: with only a `CLAUDE.md` target proposal saved, `status --json` shows `proposal: null` alongside that entry and its apply command. The default `.backpass/` layout is unchanged, and I didn't edit `AGENTS.md`
… writes `proposal.json`, so a newer proposal saved while you're reviewing can't be overwritten. `pnpm run check` passes: lint, format, typecheck and all 937 tests. **The rule:** marking a proposal as reviewed must hide only the exact proposal that was reviewed, and must never change the saved proposal. Two places have to agree on this: the code that records a finished review (`cmdApply`) and the code that decides what is still waiting (`listTargetProposals`). The only users of that list are `status`, `status --json` and the hints printed by bare `apply`, so they all follow it. **What changed:** - `src/state.js`: a new `proposalHash(proposal)` hashes the proposal as it was read. A new `State.markReviewed(hash)` writes a small `reviewed.json` (`{ proposal: <hash> }`) in the same state folder as the proposal. `listTargetProposals()` hides a target proposal only when that hash matches the current `proposal.json`. The `reviewedAt` field from last round is gone. - `src/commands/apply.js`: the hash is taken when the proposal is first read, before the review starts. Once every edit is decided, nothing failed, and it wasn't a dry run, apply writes the marker instead of rewriting the proposal. Proposals with undecided edits stay listed, as before. - One choice of mine: the marker is written only for named-file proposals under `targets/`. Only those are ever listed, so writing it for the default proposal would add an unused file to `.backpass/`, whose layout you said must not change. **Tests** (in `test/target-state.test.js`): - New: "a newer proposal saved during the review stays intact and waiting". A fake `lavish-axi` saves a newer 3-edit proposal for `CLAUDE.md` while the review is open, then sends the decision for the 1-edit proposal that was shown. The newer proposal survives byte-for-byte and is still listed in `status --json` with 3 edits. - Updated the existing completed-review test to check that `proposal.json` is left exactly as it was. - Both fail on the previous commit and pass now. `AGENTS.md` was not edited
| markReviewed(hash) { | ||
| this.writeJsonFile(this.reviewedPath, { proposal: hash }); |
There was a problem hiding this comment.
markReviewed() replaces the only saved review hash. If an old review stays open while a newer proposal is saved and fully reviewed, finishing the old review last replaces the newer completion record. Both reviews can reject every edit, so file freshness checks do not prevent this.
Bare status and apply then advertise the completed newer proposal as waiting, prompting users to review the same rejected edits again. Keep completion records per proposal hash, or prevent an older review from replacing the current proposal's completion record.
|
Speaking as Kun's firstmate: Thanks @Pantoria, this is a thorough fix. I reviewed the diff against main. CI is green on both OSes and the no-mistakes attestation is bound to the current head ( The only open item is the one you already mentioned on #189: live evidence from a real A, B, A contract-class: restore. I checked the unconfigured path: VISION (per rule):
|
2 participants
Intent
Fix issue #189: the evidence cache held one record per transcript (evidence/.json) and one proposal.json, so alternating --memory-file / --target between different root memory files (A, then B, then A) overwrote evidence and re-analyzed every transcript though nothing changed, and B's run wiped A's pending proposal. The user (issue author, Pantoria) asked to implement their own ready-for-pr backpass issues, each in its own worktree and PR. Follow AGENTS.md invariants and VISION.md. No live model calls were made while implementing.
Deliberate decisions: per-target state directory (mirroring nestedStateDir) rather than keying the evidence path on identity+hash, because only that also fixes the single proposal.json. Runs whose resolved primary root file differs from the default run's file keep evidence/, evidence-summary.json, proposal.json, prompts/, synthesis/, raw/, apply/ under /targets/-/; --memory-file X and --target X share one dir. A default run (and --memory-file/--target naming the default file, and skill --target) keeps .backpass/ exactly as today - the maintainer required that. Rejections, gap ledger, scan cache, and probe cache stay shared via the root State (maintainer asked rejections be shared; the ledger is already scoped by memoryPath). User scope target dirs live under the user state root with mode 0700, never in the repo. apply/status/analyze with --memory-file X read X's state; bare apply and status list pending target proposals with the exact command. AGENTS.md got a one-clause update under 'Scopes and targets' to record the new layout. Known, accepted limitations: one-time re-analysis on upgrade for users who already used non-default --memory-file (no migration); skill --target still shares the default proposal.json (out of scope). Maintainer wants a test that fails if the per-surface path is removed (A,B,A with no re-analysis of A) - included in test/target-state.test.js - and live evidence of a real A,B,A --memory-file run showing reuse.
What Changed
--memory-file Xor a memory--target X) now keeps its evidence, evidence summary, proposal, prompts, synthesis staging, raw, and apply surface under<state root>/targets/<file>-<sha8>/(surfaceStateinsrc/target.js,targetStateDirinsrc/state.js). Training A, then B, then A no longer re-analyzes unchanged transcripts or overwrites the other file's pending proposal. Default runs, runs that name the default file, and skill targets still use.backpass/exactly as before. User-scope target dirs sit under the user state root with mode 0700.state.scopeRoot), so they also covertargets/.apply,status,propose,analyze, and full-run hints now show the exact command for the state being read, e.g.backpass apply --memory-file X(with--scope userwhen needed). Bareapplyandstatusalso list pending target proposals that are unapplied and passed their gates. README and AGENTS.md describe the new layout, andtest/target-state.test.jscovers it, including an A, B, A run where A's evidence is reused without re-analysis.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The fix round fully addresses both earlier findings: every apply/status hint is now built from the state being read, so a target state gives
--memory-file <file>, and bare apply also lists waiting target proposals when the default proposal is already applied or failed its gates. I traced the default, target, skill-target and user-scope cases and found no wrong output or behavior change for default runs.Testing
Ran the real CLI in throwaway repos with an isolated HOME and a stand-in acpx/pi harness, so no real model was called. Covered: the A,B,A
--memory-fileruns, compared with the same runs on the base commit; the default layout;--targetand--memory-filesharing one state; the status and apply hints; review-2's bare-apply listing; the fixed synthesis-failure hint in both the propose and full-run paths; and user-scope permissions. Every live-driven scenario passed. The targeted regression test file was also run, but it is not a live run, so its scenario is recorded as untested. The gate-failed and bootstrap-applied states for review-2 were seeded by editing the disposable proposal.json, because the stand-in harness can't produce them reliably. This is a CLI-only change, so the evidence is CLI transcripts rather than screenshots.backpass apply --memory-file CLAUDE.md; status --memory-file CLAUDE.md reads CLAUDE.md's stateapply --target CLAUDE.mdon a --memory-file proposal suggestsbackpass apply --memory-file CLAUDE.md; apply --memory-file writes only CLAUDE.md; bare apply writes only AGENTS.mdbackpass apply --memory-file CLAUDE.md(gate-failed and applied states seeded in proposal.js…propose --memory-fileand the full--targetrun) and the hint saysbackpass propose --memory-file CLAUDE.md; in the default state it stays…node --test test/target-state.test.jsunit/integration test, not live against the running product, so no live result was recorded. The same A,B,A behavior was…Evidence: A,B,A,B --memory-file runs on fixed source (reuse + two proposals)
Evidence: Same A,B,A,B on base commit (bug reproduced: always re-analyzes, one proposal)
Evidence: status/apply hints and --target sharing
Evidence: Default layout + synthesis-failure hints (target vs default)
Evidence: Bare apply lists waiting target proposals
Evidence: User scope A,B,A and 0700 target dirs
Evidence: target-state.test.js run
~/.no-mistakes/evidence/01M503TV5DA5S5E2XH135BV7XX/r2-driver)Evidence: Synthesis-failure hint in target state
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
src/commands/apply.js:93---memory-file Xand--target Xshare one state directory, but apply's hints still assume a single shared proposal. Example: a user runsbackpass --memory-file CLAUDE.md(saved target issurface) and thenbackpass apply --target CLAUDE.md. That opens CLAUDE.md's target state, fails the sameTarget check, and suggests "apply it without --target". A barebackpass applythen reads the default AGENTS.md proposal, not the CLAUDE.md one. Before this change both commands read the same proposal.json, so the hint was correct. Fix: build the hint fromapplyCommand(config.state, ctx.scope). Other hints in a target state still point at the default state and should get the same treatment: apply.js:~107 "runbackpassagain to produce a fresh one", apply.js:~101 "runbackpass proposeagain", and status.js:181 "none yet - runbackpass". Each should name--memory-file <surface>whenstate.surfaceis set.src/commands/apply.js:75- Barebackpass applylists pending target proposals only when no default proposal.json exists. Once the default proposal has been applied (appliedAt set) or failed its gates, apply throws the 'already applied' or 'failed its mechanical gates' error and does not mention a waitingtargets/proposal. The intent says bare apply lists pending target proposals with the exact command. Consider adding thelistTargetProposals()hint to those two errors as well.statusalready lists them unconditionally.🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Test** - 3 issues found → auto-fixed ✅
src/commands/propose.js:334- In a target state, synthesis-failure hints still say to run barebackpass proposeagain (propose.js:316, 319, 322, 325, 334). Repro:backpass --target CLAUDE.md(or--memory-file CLAUDE.md) where synthesis fails its gates prints "runbackpass proposeagain". Running that command proposes for the default AGENTS.md, not CLAUDE.md. This is the same kind of misleading hint review-1 asked to fix in apply/status. Fix: build these hints withstateCommand("propose", config.state, ctx.scope).src/target.js:126- In a repo with a separate CLAUDE.md, a bare default run and--memory-file AGENTS.mdshare .backpass/ (as designed) but have different memory-set hashes, so switching between them still re-analyzes every transcript. This matches base behavior and the stated design (naming the default file keeps the default state); noted for awareness only.node --test test/target-state.test.jsanalyze --memory-file CLAUDE.mdafter a--target CLAUDE.mdrun: 0 newly analyzed · 3 cached)backpass proposeagain; proposals for other memory files are waiting:backpass apply --memory-file CLAUDE.md"backpass apply --memory-file CLAUDE.md(review-1 fix)backpass propose --memory-file CLAUDE.mdagain')backpass --target CLAUDE.mdsynthesis failure prints "runbackpass proposeagain" without --memory-file CLAUDE.mdnode --test test/target-state.test.js(4/4 pass, including the A,B,A no-re-analysis regression test)Fullbackpass --memory-file AGENTS.md/--memory-file CLAUDE.md/ AGENTS.md / CLAUDE.md runs (scan→analyze→propose) in a disposable git repo with an isolated HOME and pi sessions, using a stand-in acpx at the model boundarySame A,B,Abackpass analyzesequence on base commit e6a030a (extracted with git archive) to reproduce the original bugbackpass statusandbackpass status --memory-file CLAUDE.mdwith both proposals pendingbackpass apply --memory-file CLAUDE.md --decisions e1=acceptedand barebackpass apply --decisions e1=accepted: each wrote only its own filebackpass --target CLAUDE.md, thenbackpass analyze --memory-file CLAUDE.md, to check the two flags share one state directoryBarebackpass applywith a gate-failed default proposal and a pending CLAUDE.md target proposalbackpass apply --target CLAUDE.mdagainst a proposal made with--memory-file, plusapply --target CLAUDE.mdagainst a--targetproposalUser scope:backpass analyze --scope user --memory-file .claude/CLAUDE.md/.codex/AGENTS.md/.claude/CLAUDE.md, then inspected the state-dir layout and modesDefault-layout check:backpass analyze, then--memory-file AGENTS.mdand--target AGENTS.md, then listed.backpass/🔧 Fix applied.
✅ Re-checked - no issues remain.
backpass apply --memory-file CLAUDE.md; status --memory-file CLAUDE.md reads CLAUDE.md's stateapply --target CLAUDE.mdon a --memory-file proposal suggestsbackpass apply --memory-file CLAUDE.md; apply --memory-file writes only CLAUDE.md; bare apply writes only AGENTS.mdbackpass apply --memory-file CLAUDE.md(gate-failed and applied states seeded in proposal.js…propose --memory-fileand the full--targetrun) and the hint saysbackpass propose --memory-file CLAUDE.md; in the default state it stays…node --test test/target-state.test.jsunit/integration test, not live against the running product, so no live result was recorded. The same A,B,A behavior was…Live A,B,A,Bbackpass --memory-file AGENTS.md|CLAUDE.mdfull runs on the fixed source (r2-driver/s1.sh)Same A,B,A,B sequence on base commit e6a030a extracted viagit archive(before/after reproduction)backpass analyze --target CLAUDE.mdafter--memory-file CLAUDE.mdruns (shared state)backpass statusandbackpass status --memory-file CLAUDE.mdbackpass apply --target CLAUDE.mdon a --memory-file proposal,apply --memory-file CLAUDE.md, barebackpass apply(r2-driver/s2.sh)Fresh repo bare run: inspect .backpass layout for absence of targets/ (r2-driver/s3.sh)Adversarial: synthesis returns no text in CLAUDE.md state viabackpass propose --memory-file CLAUDE.mdandbackpass --target CLAUDE.md; control in default stateBarebackpass applywith seeded gate-failed, seeded bootstrap-applied, and missing default proposal while a CLAUDE.md proposal is pending (r2-driver/s4.sh)User scopebackpass analyze --scope user --memory-file .claude/CLAUDE.md|.codex/AGENTS.mdA,B,A,B plus mode/location check (r2-driver/s5.sh)node --test test/target-state.test.js✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.