From 58f4914675e518f82bec07cbb50a8070db0bb51a Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:35:50 +0000 Subject: [PATCH 1/6] fix: match review's finding lookups on rendered, not raw, id findByID and contains compared a finding's raw ID with ==, while every display path (report, review's printFinding) renders it through session.SafeText. A hand-edited or exchanged findings.jsonl carrying an invisible character in an id (analyze.Load never re-validates ^F-\d{3}$; only ingest does) therefore displays as a clean id like "F-001" but is unreachable by that same id via -finding, and by an interactive duplicate-of target naming it. Recording a verdict now also stores the finding's actual raw id (not the operator's clean flag/typed value) so analyze.EffectiveStatus, keyed on the raw id, attaches the verdict instead of silently dropping it from the report. Assisted-by: Claude:claude-sonnet-5 --- internal/review/review.go | 43 +++++++++++++++++++++++++--------- internal/review/review_test.go | 31 ++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/internal/review/review.go b/internal/review/review.go index 0fe5d8b..af77c3d 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -99,7 +99,17 @@ func single(opts Options, findings []analyze.Finding) error { // checkTargets passed, so the id is present in the snapshot; bind the verdict // to that finding so AppendVerdict can confirm it is unchanged at write time. target := findByID(findings, opts.Finding) - rec := analyze.Verdict{Kind: "verdict", Finding: opts.Finding, Verdict: verdict, Of: of, At: opts.Today} + // The verdict's Finding/Of must carry the finding's actual (raw) id, not the + // operator's clean flag value: analyze.EffectiveStatus keys its map on each + // finding's raw id, so a verdict recorded under -finding's rendered form would + // silently fail to attach to a finding whose raw id findByID only matched via + // SafeText (see findByID). + if verdict == "duplicate" { + if dup := findByID(findings, of); dup != nil { + of = dup.ID + } + } + rec := analyze.Verdict{Kind: "verdict", Finding: target.ID, Verdict: verdict, Of: of, At: opts.Today} if err := AppendVerdict(opts.Dir, rec, target); err != nil { return err } @@ -107,11 +117,19 @@ func single(opts Options, findings []analyze.Finding) error { return nil } -// findByID returns a pointer to the finding with the given id, or nil. The -// returned pointer is into a copy, safe to retain. +// findByID returns a pointer to the finding with the given id, or nil. Ids are +// compared in their session.SafeText form, matching analyze.ParseRecords' load- +// time uniqueness check: a finding's id renders through SafeText everywhere it +// is shown (report, review's printFinding), so an operator matching it via +// -finding, or an interactive duplicate-of target, only ever has the rendered +// form to type. Comparing raw would leave a finding whose raw id carries a +// stripped byte (e.g. a hand-edited findings.jsonl with an invisible +// character) permanently unreachable by the id it displays as. The returned +// pointer is into a copy, safe to retain. func findByID(findings []analyze.Finding, id string) *analyze.Finding { + want := session.SafeText(id) for i := range findings { - if findings[i].ID == id { + if session.SafeText(findings[i].ID) == want { f := findings[i] return &f } @@ -202,7 +220,13 @@ func applyChoice(opts Options, findings []analyze.Finding, f analyze.Finding, ch if err := checkTargets(findings, f.ID, "duplicate", target); err != nil { return false, false, err } - return true, false, record(opts, f, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "duplicate", Of: target, At: opts.Today}) + // Resolve the typed target back to its actual (raw) id; see the matching + // comment in single() for why the clean typed form cannot be stored as-is. + of := target + if dup := findByID(findings, target); dup != nil { + of = dup.ID + } + return true, false, record(opts, f, analyze.Verdict{Kind: "verdict", Finding: f.ID, Verdict: "duplicate", Of: of, At: opts.Today}) case "s", "": fmt.Fprintln(opts.Out, " skipped.") return true, false, nil @@ -498,13 +522,10 @@ func describe(v analyze.Verdict) string { session.SafeText(v.Finding), session.SafeText(v.Verdict), session.SafeText(v.At)) } +// contains reports whether id (in its session.SafeText form) names one of +// findings; see findByID for why the comparison is SafeText, not raw. func contains(findings []analyze.Finding, id string) bool { - for _, f := range findings { - if f.ID == id { - return true - } - } - return false + return findByID(findings, id) != nil } func readLine(r *bufio.Reader) (string, error) { diff --git a/internal/review/review_test.go b/internal/review/review_test.go index bf0c295..64a5613 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -103,6 +103,37 @@ func TestNonInteractiveDuplicate(t *testing.T) { } } +// TestNonInteractiveConfirmMatchesRenderedID covers a finding whose raw id +// carries an invisible character (as a hand-edited or exchanged findings.jsonl +// might: analyze.Load never re-validates ^F-\d{3}$, only ingest does). The id +// still renders as "F-001" everywhere it is shown (report, review's +// printFinding), via session.SafeText, so -finding F-001 — the id an operator +// actually sees — must resolve to it, and the recorded verdict must attach to +// it in analyze.EffectiveStatus (keyed on the finding's raw id), not vanish +// because it was stored under the clean flag value instead. +func TestNonInteractiveConfirmMatchesRenderedID(t *testing.T) { + dir := t.TempDir() + dirty := "F-001​" // zero-width space, stripped by session.SafeText + fixture := `{"id":"` + dirty + `","t":22,"type":"bug","severity":3,"quote":"I clicked save and nothing happened","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(fixture), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + + var out bytes.Buffer + if err := Run(Options{Dir: dir, Finding: "F-001", Verdict: "confirmed", Out: &out, Today: "2026-07-17"}); err != nil { + t.Fatalf("Run: %v", err) + } + + findings, verdicts, err := analyze.Load(dir) + if err != nil { + t.Fatalf("Load: %v", err) + } + eff := analyze.EffectiveStatus(findings, verdicts) + if eff[dirty].Value != "confirmed" { + t.Fatalf("finding %q status: %+v, want confirmed", dirty, eff[dirty]) + } +} + func TestNonInteractiveErrors(t *testing.T) { dir := writeSession(t) cases := []struct { From 9d0cda55eaea9ae68a7aed9ad286b9d0d210135f Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:35:54 +0000 Subject: [PATCH 2/6] chore: reference session file constants in merge/report messages cli.go named timeline.jsonl/report.md as string literals in the merge and report success messages instead of session.TimelineFile/ReportFile, the constants already used elsewhere for the same filenames (report.go, tests). Same values today, but two independent spellings of one well-known name is exactly what the constants exist to prevent. Assisted-by: Claude:claude-sonnet-5 --- internal/cli/cli.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 4f48912..e7227f8 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -95,7 +95,7 @@ func Run(args []string) int { return fail(err) } fmt.Printf("merged %d utterances + %d events → %s\n", - speech, events, filepath.Join(*dir, "timeline.jsonl")) + speech, events, filepath.Join(*dir, session.TimelineFile)) return 0 case "report": @@ -123,7 +123,7 @@ func Run(args []string) int { if err != nil { return fail(err) } - out := filepath.Join(*dir, "report.md") + out := filepath.Join(*dir, session.ReportFile) if err := session.WriteFileNoFollow(out, []byte(md), 0o644); err != nil { return fail(err) } From fa008f27f5887ecdb5bd03057e0bd372a8705de7 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:36:00 +0000 Subject: [PATCH 3/6] docs: qualify Mode B keyframe extraction as planned 01-purpose.md described Mode B's keyframe extraction in unqualified present tense, as if shipped, while 04-analysis.md heads that section "Keyframes (planned)" and defers frame extraction to a later intent, and 01-phases.md marks Phase 4 (Mode B) not started. The same page already qualifies its other unshipped item ("codebase mapping as a future goal") one paragraph earlier; this brings Mode B's keyframe mention in line with that convention. Assisted-by: Claude:claude-sonnet-5 --- .abcd/development/brief/01-product/01-purpose.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.abcd/development/brief/01-product/01-purpose.md b/.abcd/development/brief/01-product/01-purpose.md index 8183702..4743c8c 100644 --- a/.abcd/development/brief/01-product/01-purpose.md +++ b/.abcd/development/brief/01-product/01-purpose.md @@ -23,9 +23,9 @@ Two modes share the same capture rig: a participant demos third-party applications while narrating what they like, dislike, or find notable. The goal is a tagged design-preferences corpus feeding requirements and design decisions. Mode B has no access to - the target app's internals, so the transcript carries the semantic load and - keyframes extracted from the video supply the referents - (see [`04-analysis.md`](04-analysis.md)). + the target app's internals, so the transcript carries the semantic load, with + keyframes extracted from the video to supply the referents as a planned + fallback (see [`04-analysis.md`](04-analysis.md)). ## Method grounding From c02498e230f868a8c1f18aac379178e93e26329f Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 13:37:00 +0000 Subject: [PATCH 4/6] docs: log round 28 in DECISIONS.md Assisted-by: Claude:claude-sonnet-5 --- .abcd/work/DECISIONS.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 7386bdd..50e3e93 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -694,3 +694,38 @@ Architecture-shaping decisions graduate to an ADR under `speaker` row is the counter-example, marked "no" despite `transcribe` always emitting it — so this is closer to established, if imprecise, usage than a fresh inconsistency worth acting on this round). +- 2026-08-05 — Bug-hunt round 28: `review`'s `findByID`/`contains` now compare + a finding id in its `session.SafeText` rendered form, matching every display + path — a hand-edited or exchanged `findings.jsonl` carrying an invisible + character in an id (`analyze.Load` never re-validates `^F-\d{3}$`; only + ingest does) displayed as a clean id but was unreachable by that same id via + `-finding` or an interactive duplicate-of target; a verdict recorded for such + a finding now also carries its actual raw id rather than the operator's + clean flag/typed value, so `analyze.EffectiveStatus` (keyed on the raw id) + attaches it instead of silently dropping it from the report. `cli.go`'s + merge/report success messages now reference `session.TimelineFile`/ + `ReportFile` instead of independently-spelled string literals for the same + filenames. `01-purpose.md`'s Mode B keyframe-extraction mention is qualified + as planned, matching `04-analysis.md`'s "Keyframes (planned)" heading and + the page's own "future goal" convention one paragraph earlier. Refuted: + `report.go`'s `orDash` fallback branch being unreachable (true, but a + deliberate locally-redundant guard against a future caller invariant + change, per the same rationale `review.go`'s `checkTargets` states for its + own SafeText calls); `demo`'s `-addr ":"` bypassing `CheckAddr`'s numeric + range guard (not silent — the real bound ephemeral port is reported — and + an empty port is `net.Listen`'s own deferred-to-runtime case, the same as + a named service port, settled by round 24's identical precedent); `itd-2`'s + AC2 wording ("the finding's status becomes...") contradicting the + append-only invariant (AC2's own second clause names the append-only + mechanism; "status" at intent altitude is the effective status the pipeline + derives and displays, not the stored field the invariant docs constrain); + `02-verification.md`'s "kept under `sessions/`" line being unverifiable + (`sessions/` is gitignored by design — committing a real captured session + would violate the repo's own privacy rule). Also excluded before + verification, as precedent duplicates of findings already discussed in + earlier rounds: `AGENTS.md`'s dangling `03-configuration.md` link inside the + abcd-managed fence (round 8); `AGENTS.md` claiming CI runs plain + `go test ./...` (round 13); persona role-label wording drift across intent + drafts (round 20); the intents-README "always they/them" + persona-quote rule read against `03-personas.md`'s gendered narrative + pronouns (round 24). From 662b9656d961bd7b1e4403f646fca102ec9c314c Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:14:52 +0000 Subject: [PATCH 5/6] fix: close self-duplicate gap left by review's SafeText id matching checkTargets' "cannot be a duplicate of itself" guard still compared of == id as raw bytes, while findByID/contains (and the verdict resolution in single()/applyChoice) now establish finding identity under session.SafeText. For a finding whose raw id carries an invisible character (a hand-edited or exchanged findings.jsonl), a duplicate-of target matching that finding's own rendered id slipped past the raw comparison and resolved back to the same finding's raw id, recording it as a duplicate of itself. checkTargets now compares of and id in their SafeText rendered form, matching every other identity check on this path. Covered by a new interactive-path regression test (TestInteractiveDuplicateRefusesRenderedSelfMatch); verified failing before the fix (records finding-duplicate-of-itself) and passing after. Assisted-by: Claude:claude-sonnet-5 --- internal/review/review.go | 2 +- internal/review/review_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/review/review.go b/internal/review/review.go index af77c3d..2fa31a4 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -259,7 +259,7 @@ func checkTargets(findings []analyze.Finding, id, verdict, of string) error { return fmt.Errorf("finding %s not found", session.SafeText(id)) } if verdict == "duplicate" { - if of == id { + if session.SafeText(of) == session.SafeText(id) { return fmt.Errorf("a finding cannot be a duplicate of itself") } if !contains(findings, of) { diff --git a/internal/review/review_test.go b/internal/review/review_test.go index 64a5613..bdfdfbf 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -239,6 +239,37 @@ func TestInteractiveDuplicateTargetMustExist(t *testing.T) { } } +// TestInteractiveDuplicateRefusesRenderedSelfMatch covers a finding whose raw +// id carries an invisible character (session.SafeText strips it on render; +// see TestNonInteractiveConfirmMatchesRenderedID). The analyst only ever sees +// the clean rendered id ("F-001"), so typing it back as a duplicate-of target +// for that same finding must be refused as a self-duplicate, the same as it +// would be for a clean id — not silently accepted because the raw bytes +// differ from what checkTargets compared. +func TestInteractiveDuplicateRefusesRenderedSelfMatch(t *testing.T) { + dir := t.TempDir() + dirty := "F-001​" // zero-width space, stripped by session.SafeText + fixture := `{"id":"` + dirty + `","t":22,"type":"bug","severity":3,"quote":"I clicked save and nothing happened","evidence":["utt-004"],"status":"unverified"}` + "\n" + if err := os.WriteFile(filepath.Join(dir, session.FindingsFile), []byte(fixture), 0o644); err != nil { + t.Fatalf("write findings: %v", err) + } + + // Mark the finding a duplicate of the rendered form of its own id, then + // skip; must be refused rather than recorded. + script := "d\nF-001\ns\n" + var out bytes.Buffer + if err := Run(Options{Dir: dir, In: strings.NewReader(script), Out: &out, IsTTY: true, Today: "2026-07-17"}); err != nil { + t.Fatalf("Run: %v", err) + } + if !strings.Contains(out.String(), "duplicate of itself") { + t.Fatalf("expected a self-duplicate refusal, got %q", out.String()) + } + _, verdicts, _ := analyze.Load(dir) + if len(verdicts) != 0 { + t.Fatalf("self-duplicate target wrote %d verdicts, want 0", len(verdicts)) + } +} + func TestInteractiveQuitStops(t *testing.T) { dir := writeSession(t) var out bytes.Buffer From b8882e9e99d248617a2acc940798e95dbd87a886 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:15:02 +0000 Subject: [PATCH 6/6] docs: fix round 28 CHANGELOG gap and keyframe wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues raised by this round's docs-accuracy adversarial review: - CHANGELOG.md had no entry for review's SafeText id-matching fix (this round's substantive finding), the same omission round 27 went back and repaired for round 26 — added alongside the sibling analyze.ParseRecords entry it parallels, and folded in the self-duplicate gap closed in the previous commit. - 01-purpose.md's Mode B keyframe-extraction mention was requalified as "a planned fallback", a phrase that exists nowhere else in the repo and is inaccurate in Mode B specifically: keyframes stand in for the missing event stream there (per the architecture note and itd-4), not a fallback from a cheaper primary path the way 04-analysis.md frames them for Mode A. Reworded to "a future goal", matching the same phrase the page's own opening paragraph already uses for its other unshipped item (codebase mapping). DECISIONS.md's round 28 entry is corrected to match: it previously claimed the "future goal" convention sat "one paragraph earlier" (it is in the page's opening paragraph, separated by a heading and the whole Mode A bullet) and that the edit adopted it (it had coined "planned fallback" instead). Assisted-by: Claude:claude-sonnet-5 --- .../brief/01-product/01-purpose.md | 6 ++--- .abcd/work/DECISIONS.md | 22 ++++++++++++++----- CHANGELOG.md | 12 ++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.abcd/development/brief/01-product/01-purpose.md b/.abcd/development/brief/01-product/01-purpose.md index 4743c8c..317dd42 100644 --- a/.abcd/development/brief/01-product/01-purpose.md +++ b/.abcd/development/brief/01-product/01-purpose.md @@ -23,9 +23,9 @@ Two modes share the same capture rig: a participant demos third-party applications while narrating what they like, dislike, or find notable. The goal is a tagged design-preferences corpus feeding requirements and design decisions. Mode B has no access to - the target app's internals, so the transcript carries the semantic load, with - keyframes extracted from the video to supply the referents as a planned - fallback (see [`04-analysis.md`](04-analysis.md)). + the target app's internals, so the transcript carries the semantic load, + with keyframes extracted from the video supplying the referents as a future + goal (see [`04-analysis.md`](04-analysis.md)). ## Method grounding diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 50e3e93..9788c8b 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -702,12 +702,22 @@ Architecture-shaping decisions graduate to an ADR under `-finding` or an interactive duplicate-of target; a verdict recorded for such a finding now also carries its actual raw id rather than the operator's clean flag/typed value, so `analyze.EffectiveStatus` (keyed on the raw id) - attaches it instead of silently dropping it from the report. `cli.go`'s - merge/report success messages now reference `session.TimelineFile`/ - `ReportFile` instead of independently-spelled string literals for the same - filenames. `01-purpose.md`'s Mode B keyframe-extraction mention is qualified - as planned, matching `04-analysis.md`'s "Keyframes (planned)" heading and - the page's own "future goal" convention one paragraph earlier. Refuted: + attaches it instead of silently dropping it from the report. The correctness + adversarial reviewer caught a gap the initial fix left open: `checkTargets`' + "cannot be a duplicate of itself" guard still compared raw ids, so a + duplicate-of target matching the finding's own dirty id under SafeText slid + past it and recorded a finding as a duplicate of itself; that guard now + compares the same rendered form, fixed before merge. `cli.go`'s merge/report + success messages now reference `session.TimelineFile`/`ReportFile` instead + of independently-spelled string literals for the same filenames. + `01-purpose.md`'s Mode B keyframe-extraction mention now reads "as a future + goal", matching the same phrase the page's own opening paragraph already + uses for its other unshipped item (codebase mapping) — the docs-accuracy + reviewer caught that the round's first pass had instead coined "a planned + fallback", a phrase found nowhere else in the repo and inaccurate in this + Mode B context (keyframes stand in for the missing event stream here, per + the architecture note and `itd-4`, not a fallback from a cheaper primary + path the way `04-analysis.md` frames them for Mode A). Refuted: `report.go`'s `orDash` fallback branch being unreachable (true, but a deliberate locally-redundant guard against a future caller invariant change, per the same rationale `review.go`'s `checkTargets` states for its diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e619be..7da9275 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,18 @@ Evidence integrity: distinct only by invisible-Unicode bytes (a zero-width space, say) used to load without error and render under the same visible id in `report` and `review`, one in the Confirmed group and the other in Unverified. +- `review`'s `findByID`/`checkTargets` compare a finding's id in its + `session.SafeText` rendered form, matching the duplicate-id check above: a + raw id carrying an invisible character (a hand-edited or exchanged + `findings.jsonl`; `analyze.Load` never re-validates `^F-\d{3}$`, only + `-ingest` does) rendered as a clean id everywhere it was shown but was + unreachable by that same id via `-finding` or an interactive duplicate-of + target, and a verdict recorded through it stored the operator's typed value + rather than the finding's actual id, so `analyze.EffectiveStatus` (keyed on + the raw id) silently failed to attach it. The "cannot be a duplicate of + itself" guard compares the same rendered form, so a duplicate-of target + matching a finding's own dirty id under `SafeText` is still refused rather + than recorded as a self-duplicate. - **Behaviour:** `timeline.ReadEntries` bounds an entry's `t` and a speech entry's payload `t1` to the same ±1e9s magnitude `merge` already enforces on `transcript.jsonl` — a hand-edited or exchanged `timeline.jsonl` reaches