Bug-hunt round 28: review id-lookup SafeText mismatch, file-constant reuse, Mode B tense

.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 supplying the referents as a future
+  goal (see [`04-analysis.md`](04-analysis.md)).
 
 ## Method grounding
 

.abcd/work/DECISIONS.md

@@ -694,3 +694,48 @@ 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. 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
+  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).

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

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)
 		}

internal/review/review.go

@@ -99,19 +99,37 @@ 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
 	}
 	fmt.Fprintln(opts.Out, describe(rec))
 	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
@@ -235,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) {
@@ -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) {

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 {
@@ -208,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