diff --git a/.abcd/development/brief/01-product/01-purpose.md b/.abcd/development/brief/01-product/01-purpose.md index 8183702..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 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 diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 7386bdd..9788c8b 100644 --- a/.abcd/work/DECISIONS.md +++ b/.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). 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 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) } diff --git a/internal/review/review.go b/internal/review/review.go index 0fe5d8b..2fa31a4 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 @@ -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) { diff --git a/internal/review/review_test.go b/internal/review/review_test.go index bf0c295..bdfdfbf 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 { @@ -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