feat(traces): truthful recover-then-succeed outcome + persisted phenotype fingerprint (PHNX-3387, PHNX-3327)

[muqsitnawaz]

Fresh redo of PHNX-3387 (truthful recover-then-succeed outcome) + PHNX-3327
(fold failure phenotype into the traces insight fingerprint), superseding the
ACTUALLY-BROKEN #3240 (branch agents/traces). This PR fixes the three defects the
review of #3240 named, reproduced against live code.

The three #3240 defects and how this PR avoids them

1. Outcome via last-tool-success (false-positived human-takeover + incidental
   ls). feat(traces): truthful recover-then-succeed outcomes + phenotype insight fingerprint (PHNX-3387, PHNX-3327) #3240 derived completed from "the last tool call returned ok". This PR
   reuses the causal-recovery logic in phenotype.ts — a run is completed only
   when a substantive, non-human-facing tool step succeeded strictly after the
   last error AND that success resolves the failed work: its work signature —
   the effective shell program (bun, git, …) for a shell step, the tool identity
   otherwise — matches an errored step's (recoveredAfterErrors, the exact inverse of
   isFalseTermination). A punt to AskUserQuestion is excluded (human-facing); an
   incidental later success of unrelated work (a failed bun test followed by an
   ls) does not resolve the failure, so the run stays errored.
2. Phenotype from an in-memory, this-run-only batch (fragmented identical
   signatures at scale). This PR persists per-session phenotype in a new
   mtime+size-keyed session_phenotypes cache (same shape as
   session_insights / session_topics, the pattern the review pointed to) and
   reads it for the whole corpus every sync, so two identically-signatured
   sessions cluster as one regardless of which incremental batch first saw each.
3. Empty PR body / no evidence. Real BEFORE/AFTER below.

BEFORE / AFTER — meta.outcome (PHNX-3387)

Run against the real buildSessionDetail code path on this branch. errorCount(main)
is today's derivation on origin/main; lastTool-ok(#3240) is the broken heuristic;
AFTER(this PR) is what ships here.

case                          errorCount(main)   lastTool-ok(#3240)   AFTER(this PR)   correct?
recover-then-succeed          errored            completed            completed        yes ✓
human-takeover (punt)         errored            completed            errored          yes ✓
incidental `ls` AFTER error   errored            completed            errored          yes ✓
incidental `ls` BEFORE error  errored            errored              errored          yes ✓
ends unresolved (error)       errored            errored              errored          yes ✓

• recover-then-succeed (bun test fails → Edit fixes → bun test passes):
  main mislabels it errored; this PR reports completed and still surfaces the
  recovered-from failure. The retried command shares the failure's work signature
  (Bash:bun), so it counts as resolving it.
• human-takeover (bun test fails → agent punts to AskUserQuestion): the feat(traces): truthful recover-then-succeed outcomes + phenotype insight fingerprint (PHNX-3387, PHNX-3327) #3240
  heuristic mislabels it completed; this PR keeps it errored (the human-facing
  tool is excluded from the recovery test).
• incidental ls AFTER error (bun test fails → ls succeeds): the ls runs
  after the failure but does not resolve it — its work signature (Bash:ls) differs
  from the failure's (Bash:bun) — so this PR keeps it errored. This is the
  regression the review of THIS PR reproduced: keying recovery on "any success after
  the last error" wrongly called it completed; requiring the success to match the
  failed work's signature fixes it.
• ends unresolved: stays errored — no regression.

surfacedToolFailures is retained on the completed recover-then-succeed run, so the
Evals console's "green run with hidden tool failures" callout can finally fire honestly:

recover-then-succeed  outcome=completed  errorCount=1  surfacedToolFailures=["bun test"]

Phenotype fingerprint across the incremental boundary (PHNX-3327)

New test drives buildIndexShard twice: session A is classified + persisted in the
first sync, then in the second sync A's transcript is made unreadable so the only
way it can carry a phenotype into grouping is the persisted cache. Result: A and B (same
(tool, cause, error) signature, same phenotype) fold into one cluster with
sessions: 2 — the exact fragmentation #3240 hit (length === 2) does not occur.
signature output is unchanged; FailurePattern gains a phenotype field and the
pattern id incorporates it.

Tests (all real critical path, no mocks)

• buildSessionDetail truthful run outcome: recover→completed (+surfaced),
  human-takeover→errored, incidental ls AFTER the error→errored (the causal
  work-signature guard), genuine same-program retry→completed,
  incidental-success-BEFORE-error→errored, clean→completed.
• phenotype grouping across the incremental boundary: two matching signatures across
  separate buildIndexShard syncs → 1 cluster, A served from the persisted cache
  (transcript unreadable in the 2nd sync).
• insights.test.ts: phenotype splits one signature into two clusters; no-map caller
  reproduces the exact prior grouping.

$ node_modules/.bin/vitest run src/lib/traces/ src/lib/session/db
 Test Files  28 passed (28)
      Tests  187 passed (187)

Scope note

OWNS: cli/src/lib/traces/**. The persisted cache accessors
(readSessionPhenotypes / writeSessionPhenotypes + the session_phenotypes table)
live in cli/src/lib/session/db.ts because that is where every sibling
session_topics / session_insights cache accessor lives — the review explicitly
prescribed "the same way InsightFacets is cached." Docs (cli/AGENTS.md) and
.changelog/next/ are updated per repo convention for a user-visible change.

Relates to PHNX-3328, PHNX-3300 (Phoenix Evals console).

[muqsitnawaz]

REQUEST CHANGES

Independent non-author review of PR #3250 (redo of the broken #3240). Reviewed the full diff (gh api .../pulls/3250/files), the code in worktree .agents/worktrees/traces2 (branch agents/traces2, commit 35ada343f), both Linear tickets (PHNX-3387, PHNX-3327), and #3240's REQUEST CHANGES review. Verified every claim against real code execution (temporary repro test added, run with vitest, then removed — no trace left in the branch) rather than static reading alone.

Conformance vs the three #3240 defects

1. Outcome via last-tool-success → causal recovery. PARTIAL. deriveRunOutcome (cli/src/lib/traces/sync.ts:680-683) now calls recoveredAfterErrors (cli/src/lib/traces/phenotype.ts:253-263), the shared predicate the false-termination phenotype uses — genuinely not "last tool call ok" anymore, and the human-takeover case is fixed (verified below). But the predicate is looser than the ticket's own acceptance test, and reproducibly still flips a genuinely-unresolved run to completed. See BLOCKER below — this is exactly item (b) from the review brief, unfixed.
2. In-memory batch-only phenotype map → persisted full-corpus cache. FIXED. Confirmed by reading and executing the code, not just the diff.
3. Real tests on the actual syncTraces/buildIndexShard wiring. Present and real for PHNX-3327 (see below). For PHNX-3387, real but incomplete — see BLOCKER.

BLOCKER — deriveRunOutcome still flips a genuinely-unresolved run to completed when an unrelated action runs after the failure

File: cli/src/lib/traces/sync.ts:680-683, cli/src/lib/traces/phenotype.ts:253-263
Anchor: IN-DIFF

// sync.ts:680-683
function deriveRunOutcome(traj: SessionTrajectory): 'completed' | 'errored' {
  if (traj.errorCount === 0) return 'completed';
  return recoveredAfterErrors({ steps: traj.steps }) ? 'completed' : 'errored';
}

// phenotype.ts:253-263
export function recoveredAfterErrors(session: Pick<SessionDetail, 'steps'>): boolean {
  const substantive = substantiveSteps(session);
  if (substantive.length === 0) return false;
  const last = substantive[substantive.length - 1];
  if (last.outcome === 'error') return false;
  const lastErrorOrdinal = lastStepOrdinalOf(session, (s) => s.outcome === 'error');
  if (lastErrorOrdinal === undefined) return true;
  return substantive.some(
    (s) => s.ordinal > lastErrorOrdinal && s.outcome === 'ok' && !HUMAN_FACING_TOOLS.has(s.tool ?? s.lane),
  );
}

The review brief's exact case (b) is "a run ending on an incidental ls after an unresolved failure must stay errored" — the error happens, then a trailing, unrelated action runs afterward and happens to succeed. I built that scenario against the real buildSessionDetail/deriveRunOutcome path (temporary test in sync.test.ts, executed with vitest, then reverted):

const d = buildSessionDetail(traj([
  step(1, 'Bash', 'error', 'bun test'), // the task fails
  step(2, 'Bash', 'ok', 'ls'),          // unrelated, incidental, runs AFTER the failure
]));
expect(d.meta.outcome).toBe('errored');

AssertionError: expected 'completed' to be 'errored'
Expected: "errored"
Received: "completed"

Trace: both steps are non-human-facing tool steps, so both are substantive. last (the trailing ls) has outcome === 'ok', so the early false return is skipped. lastErrorOrdinal is 1. The final line asks only "did ANY substantive, non-human-facing step succeed at a later ordinal than the last error" — the trailing ls (ordinal 2 > 1, ok, not human-facing) satisfies that trivially, so recoveredAfterErrors returns true and deriveRunOutcome returns 'completed'. Under the old errorCount > 0 ? errored : completed derivation this session was errored — so this is a genuine regression, not a pre-existing limitation carried over inertly.

The docstring on recoveredAfterErrors (phenotype.ts:238-244) explicitly claims the opposite of what the code does: "A trailing incidental success unrelated to the failure (an ls)... is NOT recovery — the first is caught because a lone unrelated ok that is itself the last substantive step still requires a later ordinal than the last error." That's backwards: requiring a later ordinal is exactly what makes the trailing ls COUNT as recovery, not what excludes it. Same false claim repeats in sync.ts:673-678's docstring ("It never flips a run that ended unresolved to completed... no regression"), the PR body's BEFORE/AFTER table ("ends unresolved (error) → errored... yes ✓" — but the only "ends unresolved" case actually tested has the incidental success before the error, not after), and cli/.changelog/next/PHNX-3387.md ("ended on an incidental unrelated call stays errored — no regression"). All four of these are contradicted by the real behavior.

The PR's own test suite doesn't catch this because its scenario is different: sync.test.ts:407-416's "an incidental success does not rescue a run that ends unresolved" puts the incidental ls before the error and ends the trajectory on the error itself — the easier case, where last.outcome === 'error' short-circuits correctly. The harder case the ticket named — trailing success after the last error — has no test and is broken.

Failure: any recover-then-succeed heuristic based on "did any non-human tool succeed later" (not "did the run's last substantive action resolve the failure") will silently reclassify a real failure as completed whenever the agent's turn ends on something incidental (a git status, a stray ls, a read) after giving up on the actual task — precisely the "green run hiding a real failure" trap PHNX-3387 exists to close, reintroduced through the new path.

Fix: tighten recoveredAfterErrors to require the recovery step be the run's last substantive step (not merely any later one), or otherwise anchor it to a signal that the specific failure was addressed (e.g., a retry of the same tool/command that errored) rather than "some other non-human tool happened to run afterward and succeeded." Add a test for this exact ordering (error, then unrelated trailing success → errored) alongside the existing one.

Confirmed fixed / working (verified by execution, not just reading)

• Human-takeover excluded correctly. sync.test.ts's "human-takeover (punt to AskUserQuestion after a failure) → errored, not completed" passes for real (node_modules/.bin/vitest run src/lib/traces/, 28 files / 185 tests, all green — matches the PR body's claimed run exactly).
• PHNX-3327 persisted, full-corpus phenotype cache — genuinely fixed. syncTraces passes the full-corpus allRows (not the incremental batch) into buildIndexShard (sync.ts:246), which reads readSessionPhenotypes over that whole rows set (sync.ts:471) against the new mtime+size-keyed session_phenotypes table (cli/src/lib/session/db.ts:380-401, accessors at db.ts:3375 / db.ts:3404) — the same shape as session_topics/session_insights, exactly what the feat(traces): truthful recover-then-succeed outcomes + phenotype insight fingerprint (PHNX-3387, PHNX-3327) #3240 review asked for. sync.test.ts's "phenotype grouping across the incremental boundary" test proves this with a real two-sync scenario (second sync makes session A's transcript unreadable, so its phenotype can only reach the grouping via the persisted cache) and gets 1 cluster, not 2. signature output is confirmed unchanged (insights.ts's "with no phenotype map" test).
• Evidence in the PR body. Real BEFORE/AFTER table and a real quoted test run are present (the feat(traces): truthful recover-then-succeed outcomes + phenotype insight fingerprint (PHNX-3387, PHNX-3327) #3240 defect this PR set out to fix). The AFTER column for "human-takeover" is correct; the AFTER column for "ends unresolved" is not proven by the case actually run, per the BLOCKER above.

NICE — promotional footer in the PR body

File: PR #3250 body
Anchor: OUT-OF-DIFF

The body ends with 🤖 Generated with [Claude Code](https://claude.com/claude-code). Not a code defect, just flagging since the repo owner's stated policy is no generated-by-Claude footers on commits/PRs/issues.

Verdict

REQUEST CHANGES
Clears when: recoveredAfterErrors/deriveRunOutcome no longer classifies a trailing, unrelated post-error success as recovery (verified with a test for exactly that ordering: error, then unrelated success, must stay errored), and the docstrings/changelog/PR body claims are corrected to match.
Filtered: 2 candidates — PHNX-3327 in-memory-only cache (refuted: real persisted, full-corpus session_phenotypes cache, confirmed by executing the two-sync boundary test); empty/missing PR evidence (refuted: real before/after table and a real quoted vitest run are present, matching the actual vitest run src/lib/traces/ src/lib/session/db output — 28 files / 185 tests passed).

[muqsitnawaz]

Non-author code review — commit 22d402e (follow-up to PHNX-3387)

Verdict: READY TO MERGE

Scope: single commit on top of an already-reviewed PR. Review focused on this commit's changes to cli/src/lib/traces/phenotype.ts, cli/src/lib/traces/sync.test.ts, cli/src/lib/traces/sync.ts, cli/AGENTS.md, and cli/.changelog/next/PHNX-3387.md.

---

Causal logic — correct

workSignature + recoveredAfterErrors correctness traced end-to-end.

workSignature at phenotype.ts:245-249:

function workSignature(step: SessionDetail['steps'][number]): string {
  const tool = step.tool ?? step.lane;
  if (SHELL_TOOLS.has(tool) && step.program) return `${tool}:${step.program}`;
  return tool;
}

• Shell step with program='bun' → "Bash:bun". Shell step with program='ls' → "Bash:ls". Different — so a ls success does not resolve a bun failure. Correct.
• Non-shell step (e.g. Edit) → "Edit". A successful Edit after a failed Edit matches. Correct.
• Shell step with no program (unparseable) → "Bash". Any subsequent Bash step (also no program) would match. This is the documented degradation ("degrades to the bare tool name, matching the pre-program behavior only for that unparseable minority" at phenotype.ts:242-243) — it is a known-acceptable precision loss, not a silent regression.

recoveredAfterErrors at phenotype.ts:270-288: builds failedSignatures over ALL error steps (not just the last), then checks if any substantive post-last-error success matches one. The ordinal guard (> lastErrorOrdinal) is still present. Logic is sound.

---

Test coverage — adequate, with one gap worth naming

The four new/modified tests at sync.test.ts:385-441 correctly cover:

1. Recover-then-succeed (no program): both Bash steps without program → both get signature "Bash" → match → completed. This is the correct no-regression result for the existing test. However, this test now silently relies on the degraded-signature fallback behavior rather than the full fix: two different Bash programs with program absent would also pass. This makes the existing test a weaker guard than it looks — it would still pass even if workSignature accidentally returned only "Bash" for everything. Not a blocker because the NEW test at line 432 (a genuine retry of the failed work after the error → completed) explicitly uses program='bun' on both the failure and retry, which exercises the full work-signature path.

2. Incidental ls after error (new test, line 418): program='bun' fails, program='ls' succeeds → different signatures → errored. This is the primary regression case and it is correctly covered.

3. Genuine retry (new test, line 432): program='bun' fails, Edit fix, program='bun' retries → completed. Correctly covers the recovery case with program populated.

Gap (NICE): No test for a failed non-shell tool (Edit, Read) followed by a successful same-tool recovery. The code path is obviously correct (workSignature returns the bare tool name for non-shell tools, and identity matching works), but adding one test would close the statement coverage gap.

---

isAbandoned / isInvalidEnv — inconsistency is acceptable

Both functions at phenotype.ts:456-458 and phenotype.ts:485-486 still use the old "any substantive ok after last error" check (no work-signature matching). The commit message asks whether this is a real inconsistency or acceptable.

Acceptable, with clear reasoning:

• meta.outcome (the authoritative reported surface) is driven by recoveredAfterErrors with the work-signature fix — correctly errored for the bun test → ls case.
• isFalseTermination calls recoveredAfterErrors and is therefore also fixed.
• isAbandoned and isInvalidEnv are medium-confidence OutcomeRule classifiers that produce a descriptive console label, not the primary outcome. They fire only when meta.outcome === 'errored' is already established.
• For the bun test → ls case: isAbandoned sees ls ok after last error → recoveryAfter = true → isAbandoned = false. The run falls through to the partial label. The isFalseTermination phenotype (which uses recoveredAfterErrors) correctly fires. So the run is: outcome=errored, phenotype=false-termination, outcome-label=partial. The outcome label is slightly imprecise (partial rather than abandoned) but meta.outcome and the phenotype are both correct.
• Propagating the work-signature check to isAbandoned and isInvalidEnv would be a separate, medium-confidence heuristic refinement, not a correctness fix for the surfaces this PR is responsible for. Filing a follow-up would be appropriate.

---

Docstrings and docs — accurate

• phenotype.ts:232-249 docstring for workSignature: accurate, covers the degraded-fallback case explicitly.
• phenotype.ts:251-268 docstring for recoveredAfterErrors: accurately describes the new semantics. The "excluded twice over" note for human-facing tools is correct (they are outside substantive and would never match a non-human-facing failed signature anyway).
• sync.ts:662-680 docstring for deriveRunOutcome: updated, accurate.
• cli/AGENTS.md: updated, accurately describes work-signature matching and the bun test → ls example.
• cli/.changelog/next/PHNX-3387.md: updated, accurately reflects the new behavior. No stale claims detected.

---

isAbandoned at phenotype.ts:485-486 — OUT-OF-DIFF inconsistency, non-blocking

const recoveryAfter = substantive.some(
  (s) => s.ordinal > lastErrorOrdinal && s.outcome === 'ok' && !HUMAN_FACING_TOOLS.has(s.tool ?? s.lane),
);

This was not updated to use work-signature matching. As argued above, this is acceptable given the surface it affects (medium-confidence console label, not meta.outcome). But it is the same logical gap this PR fixed in recoveredAfterErrors, and a future PR should align it.

---

Filtered candidates

Filtered: 3 candidates.

• "Recover-then-succeed test passes only due to degraded signature" — killed: the NEW genuine-retry test at line 432 exercises the full work-signature path with program populated; the old test's behavior is a documented, acceptable fallback.
• "Non-shell recovery has no test" — killed: the code path is a single identity comparison on the tool string; obvious correctness; NICE-only.
• "isAbandoned update missing = BLOCKER" — killed: isAbandoned is a medium-confidence console hint, not the authoritative meta.outcome surface; the outcome surface is correct; the inconsistency is a follow-up improvement, not a correctness defect in this PR's scope.

[muqsitnawaz]

APPROVE (updated) — the standing REQUEST CHANGES is resolved. Re-reviewed the 4 new commits (f67d75f59, 35ada343f, 22d402ee3, 4ed007a6a) against a fresh worktree at agents/traces2.

The regression is fixed

recoveredAfterErrors() (cli/src/lib/traces/phenotype.ts:270-288) now requires the post-error success to resolve the failed work via a new workSignature() helper (phenotype.ts:245-249): a shell step's signature is tool:program (the parsed effective program — bun, git, ls, …, from the real TrajectoryStep.program field populated in trajectory.ts:357), any other tool's signature is just the tool id. A later success only counts as recovery when its signature matches a failed step's signature.

I built and ran the exact regression scenario against live code, not just the PR's description:

• Bash error(bun test) → Bash ok(ls), no resolution → outcome: 'errored'. Verified two ways: (1) the added unit test sync.test.ts:418-430 ("an incidental later success of unrelated work does not rescue the failure → errored") passes; (2) I independently called the real extractShellPrograms() parser (shell-programs.ts) on the literal strings 'bun test' and 'ls' — it resolves program: 'bun' vs program: 'ls', confirming the work-signature distinction holds on real command text, not just a test double with a hand-set program field.
• Genuine recover-then-succeed (bun test fails → Edit → bun test passes, same program) → outcome: 'completed', surfacedToolFailures still lists the recovered-from failure. Test: sync.test.ts:432-441, passes. Also covered for non-shell tools (failed Edit → Read → same-tool Edit succeeds → completed; failed Edit → unrelated Read only → errored): sync.test.ts:443-462.
• Human-takeover (bun test fails → AskUserQuestion ok) → outcome: 'errored' — AskUserQuestion is excluded from the substantive set twice over (never enters substantiveSteps, and HUMAN_FACING_TOOLS is checked again in the recovery predicate). Test: sync.test.ts:397-406, passes.
• Ran the full targeted suite for real: bun run vitest run src/lib/traces/phenotype.test.ts src/lib/traces/sync.test.ts src/lib/traces/insights.test.ts → 45/45 passed.
• tsc --noEmit clean on the touched files.

Docs/changelog no longer overclaim

cli/AGENTS.md, .changelog/next/PHNX-3387.md, and the PR body's BEFORE/AFTER table all now state the precise "resolves the failed work" / work-signature condition and give the incidental-ls-after-error case explicitly as staying errored — matches the shipped code, no lingering "any later success" claim.

One correct consequential fix worth flagging positively

isPrematureCompletion dropped its errorCount > 0 → premature branch (phenotype.ts:331-339, with a docblock explaining why: under truthful outcomes a completed run can legitimately carry errorCount > 0 for a resolved recovery, and the old branch would have mislabeled every genuine recovery as also premature-completion). This is in-scope, correctly reasoned, and tested — not scope creep.

No new regressions found. Verdict: ready to merge.