Repository navigation
Conversation
 [Medium impact] The PR appears safe to merge; no new actionable issues were found. Reviews (3) · Last reviewed commit: "no-mistakes(ci): Fixed the Greptile find..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: Thanks @Pantoria, this is solid work. I reviewed the diff against main: CI is green on both OSes, the no-mistakes attestation is bound to the current head ( contract-class: new-default (only for one part). Keeping a redacted head/tail, VISION (per rule):
|
…trying them forever An analysis reply with no parseable JSON now records, in its failed evidence record, the reply's length, its redacted first and last 300 characters, and a class judged from the text alone: empty, truncated, unparsed, or prose. acpx reports no stop reason in quiet mode, so none is recorded. After 3 unparseable replies in a row for the same evidence key and analysis call (agent, model, effort, prompt template), the transcript is skipped and named in the run summary and `backpass status`, keeping its last reply trace. A changed key or --force analyzes it again; other failures reset the streak. A skipped transcript stays `failed` and never counts as evidence. Fixes kunchenguid#193 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed. The broken rule: `backpass status` has to report every evidence store that analysis writes. Before this fix it read only the root store, so failures, reply classes and give-ups recorded for a nested memory file never appeared. Changes: - src/nested.js: added `layerNestedWeights(rootFile, weights, skills)`. It holds the ancestor and `nestedSurfaceHash` logic that was previously inline in `primaryMemoryFile`. - src/commands/analyze.js: now calls that helper, so analyze and status build each nested file's hash from the same code. - src/commands/status.js: builds one list of stores: the root store plus one per trained nested file (it exists, isn't a pointer, and its state is read from `nestedStateDir` without creating anything). It checks each record against that file's own memory hash. The changes show up in four places: - Evidence counts and the "skipped after N" note now include nested stores. - The failed-transcripts table gets a MEMORY column, but only when nested files are configured, so root-only output is unchanged. - The note about where reply traces are kept points to each file's own store. - `--json` `evidence` has a new `files` array of `{path, ok, failed, skipped, gaveUp}`, one entry per memory file. - README.md (line 416): one clause saying status also reads nested evidence and names the file. - test/analyze-failure-trace.test.js: the fake acpx can now give a nested file its own reply, and the harness can set up a nested `apps/api/AGENTS.md`. The new test runs the real CLI: the root analysis succeeds, the nested one gives up after 3 replies, and status text and JSON show which file it was (`1 ok · 0 skipped · 1 failed (1 skipped after 3 failures)`, an `apps/api/AGENTS.md` row marked skipped, and per-file JSON counts). Editing the nested file turns the row back to `retry`. I checked that this test fails against the old status.js and passes with the fix. Verification: `pnpm run check` passes (lint, format, typecheck, and all 937 tests)
8d68e6f to
214227c
Compare
|
Speaking as Kun's firstmate: Thanks @Pantoria. I re-reviewed the new head contract-class: still new-default (only for one part), so the hold is unchanged. The redacted, classed reply trace and the per-file VISION (per rule): the same verdicts as my last stamp: aligns on all seven. The new nested-store reporting strengthens "Failure is loud and named", because nested give-ups no longer go unseen. |
2 participants
Intent
Fix issue #193: when analysis output had no parseable JSON, the evidence record kept only status failed plus the error string, dropping the reply, and the transcript was re-analyzed every run forever. 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.
Implemented all three proposal parts. (1) Failed records keep reply {class, chars, head, tail}: the whole reply is passed through the existing redact() before cutting 300-char head/tail (so a secret can't be split at a snippet edge); chars is measured by backpass from the raw reply. No harness stop reason is stored because acpx runs --format quiet and exposes none - deliberately not invented (never trust/invent model numbers). (2) A deterministic class empty/prose/truncated/unparsed (rules documented in classifyUnparsedReply) shows in status and in the error text. empty is unreachable today because assertNonEmptyOutput fires first, kept for completeness (open PR #183 may change that path). (3) Skip-after-N, which the maintainer allowed only if named in status and the last trace kept: MAX_UNPARSED_REPLIES = 3 constant (deliberately no config option), counted per failureKey = evidence key + answering agent/model/effort + analysis prompt hash, so any change resets; only unparseable replies count (timeouts/exec failures reset and always retry so a flaky provider can't make backpass give up); --force re-analyzes. Skips are named in a run warning, summary suffix, summary.gaveUp in --json, and status text/JSON. Skipped records stay failed, so never count as evidence. No ANALYSIS_INDEX_VERSION bump: only failure records change, not what analysis accepts; legacy failed records still load.
What Changed
failedevidence record (src/analyze.js) now keepsreply: {class, chars, head, tail}. The whole reply is redacted first, then cut into a 300-char head and tail.charsis the raw reply length, counted by backpass. The class (empty/prose/truncated/unparsed) is worked out from the reply text alone, and it also appears in the error text.MAX_UNPARSED_REPLIES(3) unparseable replies in a row for the same evidence key, answering agent, model, effort and analysis prompt hash, the transcript is skipped instead of retried. Changing any of those inputs analyzes it again, and so does--force. A timeout or exec failure resets the count and is always retried. Skipped records stayfailed, keep their last trace, and never count as evidence. Skips are reported in a run warning, in theanalyze/run/bootstrapevidence line (skipped after 3 failures), and insummary.gaveUpin--json.backpass statusnow reads the root evidence store plus each nested memory file's own store. Each record is checked against its own file's memory hash, and the sharedlayerNestedWeightshelper insrc/nested.jsbuilds each nested file's hash for both analyze and status. Status shows the reply class and give-ups, and adds a MEMORY column when nested files are configured.--jsongets a per-fileevidence.filesarray. README and--forcehelp text are updated, andtest/analyze-failure-trace.test.jsis new.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The change only touches failure records and status reporting. The give-up streak is keyed on the evidence key plus the answering agent pick and the prompt hash, and give-ups never count as evidence. Status now reads each nested evidence store without writing anything, and it computes the same surface hashes as analyze through the shared layerNestedWeights helper.
Testing
I ran the real backpass CLI in a throwaway repo and HOME, with a stub acpx standing in for the model; no real model was called. A truncated reply containing a secret was saved with class
truncated, the character count measured by backpass, andTOKEN=[redacted]; the secret never reached disk. After 3 unparseable replies the 4th run made no model call, and it printed a warning naming the skip and askipped after 3 failuressuffix.statustext and--jsonshowed the give-up for the root file and the nested file separately (MEMORY column,evidence.files). Editing either memory file put the row back toretry, and a valid JSON reply then counted as ok evidence.--forceasked the model again. Four timeouts in a row never caused a skip and kept no reply trace. I also ran the targeted test file, which includes a case where the root succeeds and only the nested file gives up; all 9 tests passed. This is a CLI change, so the evidence is CLI transcripts rather than screenshots. The throwaway temp directory was deleted and the worktree is clean.Evidence: CLI transcript of all scenarios (analyze/status/status --json)
Evidence: Throwaway environment driver script
Evidence: Targeted test run output
Evidence: status after give-up (excerpt)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
src/commands/status.js:241-isGivenUpin status only checksunparsedReplies >= MAX_UNPARSED_REPLIES. It never checks that the record still matches the current evidence key. Here is a sequence that happens in normal use: a transcript gives up at memory hash H1, thenbackpass applyedits AGENTS.md so the memory surface becomes H2.backpass statusstill shows the record asskipped after 3 failures: in the NEXT RUN column, in the evidence summary suffix (status.js:147) and inevidence.gaveUpin --json (status.js:84). But the next run will retry it, becausereplyFailureKeyincludesevidenceKey(..., H2)and the streak resets (hasGivenUpin src/analyze.js). The label is wrong but no error is raised. The comment at status.js:236-240 accepts this for the agent dimension, since status does not resolve agents. The memory-surface and transcript-content parts of the key can be checked cheaply, though: status already resolves the memory files for its budgets. Smallest fix: treat a record as given up only whenrecord.memoryHashequals the currentmemorySurfaceHashfor its memory path, and optionally whenrecord.keymatches the currentevidenceKey. Otherwise show it asretry. The same predicate feeds three places: status.js:31 (count), status.js:147 (text suffix) and status.js:181 (row label).🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
node --test test/analyze-failure-trace.test.js: test 'status still lists an older failed record without the trace fields' drives the real CLInode --test test/analyze-failure-trace.test.js(8/8 pass; runs the real CLI end to end with a fake acpx)Manual CLI session in a throwaway HOME/repo with a fakepiand a fakeacpx(reply set from a file, each call logged): 3xbackpass analyze --harness pi --since all --analysis-agent pi --jobs 1, then inspected the evidence JSON and grepped it for the secret4thbackpass analyze(skip, model call count unchanged),--jsonsummary,backpass status,backpass status --jsonbackpass analyze --forceafter the skipEdited AGENTS.md, thenbackpass status(shows retry) andbackpass analyze(re-analyzes)Adversarial: 2 unparseable replies, then repeated acpx exit-3 timeouts: the count resets, no reply trace is kept, and every run still calls the modelRecovery: a parseable JSON reply turns the record into ok evidence; status --json reports gaveUp 0✅ No issues found.
node --test test/analyze-failure-trace.test.js(the 9 targeted tests for this change, which run the real CLI with a stub acpx)~/.no-mistakes/evidence/01M503V3YM500JB8EM5J3KKE5C/drive.shin a throwaway git repo and HOME with a nested apps/api/AGENTS.md:backpass analyzerun repeatedly,backpass status,backpass status --json,analyze --force, editing both memory files, a valid reply, and 4 timeout runs✅ **Document** - passed
✅ No issues found.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.