feat: [ENG-2326] session-end banner for accepted harness refinements

[RyanNg1403]

Summary

• Problem: Users have no visibility into when the harness improves — refinements happen silently in the background after session end.
• Why it matters: Seeing "harness updated: v3 → v4 (H: 0.58 → 0.64)" after a session reinforces that the system is learning, building trust in the auto-harness feature.
• What changed:
  • New HarnessBannerListener class subscribes to harness:refinement-completed on the agent event bus, buffers accepted refinements during a session, and prints a one-line banner to stderr on session end.
  • SessionManager wires the listener per session (create on session start, end on session cleanup).
  • Extended harness:refinement-completed event with fromHeuristic/toHeuristic fields so the banner can display H values without store access.
  • Synthesizer emit updated to include heuristic values.
• What did NOT change (scope boundary): No rejected-refinement surfacing, no configurable banner toggle, no cross-session aggregation.

Type of change

• New feature

Scope (select all touched areas)

• Agent / Tools
• Shared (constants, types, transport events)

Linked issues

• Closes ENG-2326
• Related ENG-2327 (7.7 integration test — will exercise banner end-to-end)

Root cause (bug fixes only, otherwise write N/A)

N/A

Test plan

• Coverage added:
  • Unit test
• Test file(s): test/unit/agent/session/harness-banner-listener.test.ts
• Key scenario(s) covered:
  1. Accepted + TTY + enabled → banner prints
  2. Accepted + TTY + disabled → no print
  3. Accepted + not TTY → no print
  4. Rejected refinement → no print
  5. Multiple accepteds → only last one prints
  6. Zero refinements → no print
  7. Banner format matches v{from} → v{to} (H: {fromH} → {toH})
  8. Idempotent onSessionEnd — second call does not re-print
  9. Listener unsubscribes after session end — no leak

User-visible changes

New stderr line after sessions where a harness refinement was accepted:

harness updated: v3 → v4 (H: 0.58 → 0.64)

Only appears when: harness enabled + TTY + at least one accepted refinement during the session. Written to stderr so it doesn't pollute stdout/JSON output.

Evidence

  HarnessBannerListener
    ✔ prints banner when accepted refinement fires with TTY and enabled
    ✔ does not print when harness is disabled
    ✔ does not print when not a TTY
    ✔ does not print for rejected refinements
    ✔ prints only the last accepted refinement when multiple fire
    ✔ does not print when no refinements occurred
    ✔ formats banner as v{from} → v{to} (H: {fromH} → {toH})
    ✔ does not re-print on second onSessionEnd call
    ✔ stops listening to events after onSessionEnd

  9 passing (4ms)

Full suite: 7230 passing, 0 failing. Typecheck, lint, build all clean.

Acceptance criteria evidence

AC	Evidence
HarnessBannerListener class + subscription wire-up	src/agent/infra/session/harness-banner-listener.ts:26
Banner prints ONLY when: enabled + TTY + accepted	Tests 1-4, 6 — 9/9 pass
Rejected refinements never trigger a banner	Test 4 — writeLine not called
Multiple refinements: only last-accepted wins	Test 5 — only v2→v3 printed
Output goes to stderr, matches documented format	Test 7 asserts exact string
SessionManager calls onSessionEnd() in cleanup path	session-manager.ts:413
Unit tests cover 7+ scenarios	9 passing
npm run typecheck / lint / test / build all clean	0 errors across all

Implementation decisions

Spec says	What was done	Why
Constructor takes SessionEventBus	Uses AgentEventBus	harness:refinement-completed exists only on AgentEventMap, not SessionEventMap
Banner format includes H values	Extended event with fromHeuristic/toHeuristic	Event lacked heuristic data; synthesizer already had values at emit time
Modify service-initializer.ts	Modified SessionManagerOptions	TTY/writeLine are runtime globals; SessionManagerOptions.bannerOverrides is the correct injection point

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable)
• No breaking changes (or clearly documented above)
• Branch is up to date with proj/autoharness-v2

Risks and mitigations

• Risk: Extended harness:refinement-completed event adds 2 required fields to the accepted variant — could break consumers that construct the event manually.
  • Mitigation: Only the synthesizer emits this event (single emit site at harness-synthesizer.ts:215). All existing tests pass. The change is backward-compatible for consumers that only read the event.
• Risk: Banner listener subscribes to agent-level event bus — if many sessions are created simultaneously, many listeners accumulate on one bus.
  • Mitigation: Each listener registers a single handler. onSessionEnd is called in all cleanup paths (endSession, deleteSession, dispose), ensuring unsubscription. No leak path.

[github-actions]

Claude finished @RyanNg1403's task in 2m 35s —— View job

---

PR Review: feat: [ENG-2326] session-end banner for accepted harness refinements

Todo List

• Gather context and understand the request
• Read changed files
• Review implementation for correctness, security, and quality
• Post inline comments for specific issues
• Post summary with overall score

---

Summary

Score: 4 / 5

Clean, well-scoped feature. The design is sound — discriminated union extension to carry heuristic values, a lean listener class with idempotent onSessionEnd, correct unsubscription, and 9 tests that precisely match the acceptance criteria. All CI gates pass.

What's good

• Event shape: Extending only the accepted: true branch of harness:refinement-completed with fromHeuristic/toHeuristic is the right call. TypeScript enforces it at the only emit site (harness-synthesizer.ts:213).
• Idempotency: The this.ended guard in onSessionEnd cleanly prevents double-print and double-unsubscription across all three cleanup paths (endSession, deleteSession, dispose).
• Fail-safe wiring: createBannerListener early-returns if agentEventBus is absent, and the corresponding endBannerListener handles the missing-listener case gracefully.
• TTY detection: process.stderr.isTTY ?? false is correct and avoids false positives in piped or CI environments.
• Tests: Real AgentEventBus (not a mock), clear scenarios, strict string assertion on format — solid.

Issues

Severity	Location	Summary
Moderate	harness-banner-listener.ts:39–44	4-param constructor violates CLAUDE.md rule: >3 params → must use object parameter
Nitpick	session-manager.ts:419	Comment understates the timing subtlety — banner shows refinements from concurrent/preceding sessions, never from this session's own trigger
Nitpick	harness-banner-listener.ts:62	writeLine name implies line-termination ownership is inside the function, but the default impl uses process.stderr.write and the caller appends \n
Nitpick	harness-banner-listener.test.ts:54–55	Numbered // Test N: inline comments duplicate the it() description; CLAUDE.md convention is to omit self-evident comments

Design note (not a bug, just worth knowing)

Because HarnessBannerListener subscribes to the agent-level bus (not scoped to a session), the banner displayed at the end of session N captures refinements that completed during session N's lifetime — which are actually refinements triggered by earlier sessions whose synthesizer ran concurrently. Session N's own refinement fires after its banner listener has already unsubscribed. This is the correct behavior per spec, but it's subtle enough to trip up a future maintainer. The expanded comment suggestion on session-manager.ts:419 helps.

The moderate issue (constructor arity) should be fixed before merge. The rest are polish.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4bd911b65

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude finished @RyanNg1403's task in 1m 54s —— View job

---

Todo List

• Gather context and understand the request
• Read changed files
• Review implementation for correctness, security, and quality
• Post inline comments for specific issues
• Post summary with overall score

---

PR Review: feat: [ENG-2326] session-end banner for accepted harness refinements

Score: 4.5 / 5

The refactor commit (31b4b59) cleanly addressed the previous round's moderate issue — the constructor now correctly takes a single HarnessBannerListenerOptions object. The design is sound, the cleanup paths are complete, and the tests are precise. What remains are minor naming and style observations.

---

What changed vs. the first review

The previous review flagged 4 items. Here's their status:

Prior Issue	Status
Constructor arity (4 positional params)	✅ Fixed — now uses HarnessBannerListenerOptions
Numbered // Test N: comments in test file	✅ Removed
writeLine naming smell	⚠️ Still present (nitpick)
session-manager.ts comment understates timing	✅ Comment is now accurate and clear

---

What's good

• Object constructor: HarnessBannerListenerOptions is clean and follows the project convention.
• All three cleanup paths covered: endSession, deleteSession, and dispose each call the appropriate endBannerListener or iterate and flush remaining listeners.
• dispose() iteration is correct: iterates bannerListeners.values() and calls listener.onSessionEnd() directly (not endBannerListener), avoiding mutation-during-iteration. The clear() at the end is safe.
• Timing comment at session-manager.ts:419: Now accurate and explains the async ordering subtlety clearly.
• Event discriminated union: The accepted: true branch correctly carries fromHeuristic/toHeuristic without touching the accepted: false branch. TypeScript enforces this at the only emit site.
• Tests: Real AgentEventBus, strict string assertion on format, all 9 scenarios covering both suppression rules and idempotency.

---

Remaining observations

Nitpick — writeLine naming still ambiguous (harness-banner-listener.ts:26 / harness-banner-listener.ts:64)

The callback receives a string with \n already embedded (the caller appends it). This means writeLine doesn't write a line — it writes a raw string that happens to end in \n. A future override that also appends \n would produce double newlines. The existing test (Test 7) pins the exact string, but the type's contract is silent on line-termination ownership.

Consider either:

• Rename to write (neutral — caller owns the string completely), or
• Add a JSDoc note to writeLine that s already includes the trailing newline.

Fix this →

Nitpick — bannerOverrides type duplicated (session-manager.ts:50–53 / session-manager.ts:72)

The type {isTty?: boolean; writeLine?: (s: string) => void} is written out twice: once inline in SessionManagerOptions and once on the private field declaration. Extracting it to type BannerOverrides = {…} would keep them in sync and make the intent clearer — but this is purely cosmetic.

Design note (acknowledged, not actionable for this PR)

The Codex P2 concern about cross-session contamination is real but accepted by design: since harness:refinement-completed carries no sessionId, two concurrent sessions on the same (projectId, commandType) pair will each see the other's accepted refinements and could both print a banner referencing a refinement they didn't trigger. The PR author documented this tradeoff explicitly. ENG-2327 should verify the end-to-end behavior under concurrent sessions before this becomes load-bearing in production.

---

Summary

The main blocker from the first review is fixed. The remaining items are polish-level. This is ready to merge once the team confirms the cross-session banner behavior is acceptable for the initial rollout (likely fine per the scoping note in the PR).