Skip to content

Commit 7d7d741

Browse files
committed
fix(tui): guarantee job/wake completion reports after review
Adversarial-panel findings: clean job exits must keep the alert-tier completion note (the wake card depends on odek >= v1.40 and wake_on_complete, so it may never fire), and a bg_wake during a busy operator turn must note the pending wake because openWakeTurn opens nothing then. Reworded stray subagent_log comment, updated docs/INTEGRATIONS.md and AGENTS.md to the new contract.
1 parent 7ffee94 commit 7d7d741

8 files changed

Lines changed: 72 additions & 51 deletions

File tree

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,4 @@
1313

1414
.plans/
1515
.odek-artifacts/
16+
.review-tui/

‎AGENTS.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,8 @@ modifier routes to the composer), and `Alt+A`/`Alt+D`/`Alt+T`
287287
and diffs status transitions into transient notes. odek ≥ v1.40 also
288288
pushes `bg_job` frames on start/exit — `handleEvent` routes them through
289289
`kickJobsFetch()` for an immediate snapshot, watcher tick as fallback;
290-
`bg_wake` frames become transient notes. Generation counters
290+
`bg_wake` frames arm the wake marker only (an alert-tier note appears
291+
just when a busy turn blocks the wake card). Generation counters
291292
(`jobsSeq`/`jobsWatchSeq`) drop stale ticks — keep both chains
292293
generation-guarded when touching the cadence. The same push-beats-poll
293294
pattern drives the other tabs: `subagent_state` and `memory_event` frames

‎docs/INTEGRATIONS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,4 +77,4 @@ These workflows have no server endpoint; bodek cannot offer them:
7777
| `cancelled` | ← | — | clean cancel close-out |
7878
| `approval_request` / `approval_ack` | ← | — | approval queue |
7979
| `clarify_request` / `clarify_ack` / `clarify_expired` | ← | `clarify_response` | principal-channel question card |
80-
| `skill_event` / `memory_event` / `agent_signal` | ← | — | transient notes (+ suggestion card; `agent_signal:context_trimmed` / `trim` / `tool_running` stay silent) |
80+
| `skill_event` / `memory_event` / `agent_signal` | ← | — | silent (drawer tabs own the state); only the skill suggestion card renders |

‎internal/tui/events.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -496,8 +496,9 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) {
496496
}
497497
line += eventTail(ev)
498498
// Nest the log under the in-flight sub-agent step when there is
499-
// one; a stray (resumed turn, idle, unwrapped) belongs to the
500-
// agents tab — not the strip.
499+
// one; strays (resumed turn, idle, unwrapped) are dropped — the
500+
// agents tab builds from card telemetry and state frames, not
501+
// stray log lines.
501502
if i := m.cur(); i >= 0 && m.attachSubLog(i, line) {
502503
break
503504
}
@@ -538,8 +539,14 @@ func (m *Model) handleEvent(ev client.Event) (tea.Model, tea.Cmd) {
538539
// the stamped session frame that follows opens the card. The
539540
// flag also arms the lazy marker (ensureWireTurn) so the wake keeps
540541
// its identity even when the stamped frame is missed. No strip
541-
// note: the wake card itself is the operator-visible signal.
542+
// note only when the wake card cannot open (an operator turn is
543+
// streaming — openWakeTurn opens nothing then, and the completion
544+
// must still be reported). While idle, the card itself is the
545+
// signal.
542546
m.wakeArmed = true
547+
if m.busy {
548+
m.addNote("background job finished · agent will wake after this turn")
549+
}
543550

544551
case "bg_job":
545552
// Push notification of a job start/exit (≥ v1.40): refresh the

‎internal/tui/jobs_notify_test.go‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -34,35 +34,41 @@ func seedRunning(t *testing.T) *Model {
3434
return m
3535
}
3636

37-
// TestJobExitAlertTier pins J1: a FAILED terminal transition posts an
38-
// alert-tier note (dwell in (noticeTTL, alertTTL]); a clean exit 0 is
39-
// owned by the wake card and jobs tab and stays out of the strip.
37+
// TestJobExitAlertTier pins J1: every terminal transition — clean or
38+
// failed — posts an alert-tier completion note (dwell in (noticeTTL,
39+
// alertTTL]); the wake card is a bonus, not a guaranteed report.
4040
func TestJobExitAlertTier(t *testing.T) {
41-
m := seedRunning(t)
42-
m.applyJobs([]client.Job{exitedJob("exited", 0)}, nil)
43-
if note, _ := lastNoteMatching(m, "bg_0000abcd"); note != "" {
44-
t.Fatalf("clean exit must not post a note: %q", note)
45-
}
46-
m = seedRunning(t)
47-
m.applyJobs([]client.Job{exitedJob("failed", 1)}, nil)
48-
49-
note, exp := lastNoteMatching(m, "bg_0000abcd")
50-
if note == "" {
51-
t.Fatal("failed exit transition posted no note")
52-
}
53-
if dwell := time.Until(exp); dwell <= noticeTTL || dwell > alertTTL {
54-
t.Errorf("exit note dwell = %v, want alert tier (%v, %v]", dwell, noticeTTL, alertTTL)
41+
for _, tc := range []struct {
42+
name string
43+
status string
44+
code int
45+
}{
46+
{"clean exit", "exited", 0},
47+
{"failed exit", "failed", 1},
48+
} {
49+
t.Run(tc.name, func(t *testing.T) {
50+
m := seedRunning(t)
51+
m.applyJobs([]client.Job{exitedJob(tc.status, tc.code)}, nil)
52+
53+
note, exp := lastNoteMatching(m, "bg_0000abcd")
54+
if note == "" {
55+
t.Fatal("exit transition posted no note")
56+
}
57+
if dwell := time.Until(exp); dwell <= noticeTTL || dwell > alertTTL {
58+
t.Errorf("exit note dwell = %v, want alert tier (%v, %v]", dwell, noticeTTL, alertTTL)
59+
}
60+
})
5561
}
5662
}
5763

5864
// TestJobExitNoteNamesCommand pins the command context: the exit note
5965
// carries the sanitized command head so the operator knows WHICH job.
6066
func TestJobExitNoteNamesCommand(t *testing.T) {
6167
m := seedRunning(t)
62-
m.applyJobs([]client.Job{exitedJob("failed", 2)}, nil)
68+
m.applyJobs([]client.Job{exitedJob("exited", 0)}, nil)
6369

6470
note, _ := lastNoteMatching(m, "bg_0000abcd")
65-
for _, want := range []string{"gh pr checks 63 --watch", "failed 2", "1m44s"} {
71+
for _, want := range []string{"gh pr checks 63 --watch", "exited 0", "1m44s"} {
6672
if !strings.Contains(note, want) {
6773
t.Errorf("exit note missing %q: %q", want, note)
6874
}

‎internal/tui/jobs_tab.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -114,11 +114,12 @@ func (m *Model) resetJobsState() {
114114
m.jobsPrev = nil
115115
}
116116

117-
// applyJobs stores a snapshot; watcher diffs become alert-tier notes — a
118-
// finished job is actionable, not housekeeping — and terminal transitions
119-
// fire the attention layer (bell / OSC 9). The first successful snapshot
120-
// baselines silently — jobs that predate the attach belong to the tab, not
121-
// the transcript. Returns the attention cmd (nil without a transition).
117+
// applyJobs stores a snapshot; terminal watcher diffs become alert-tier
118+
// notes — a finished job is a completion report the operator opted into,
119+
// not housekeeping — and fire the attention layer (bell / OSC 9). Job
120+
// starts stay on the tab. The first successful snapshot baselines
121+
// silently — jobs that predate the attach belong to the tab, not the
122+
// transcript. Returns the attention cmd (nil without a transition).
122123
func (m *Model) applyJobs(jobs []client.Job, err error) tea.Cmd {
123124
if err != nil {
124125
if errors.Is(err, client.ErrJobsUnavailable) {
@@ -154,13 +155,11 @@ func (m *Model) applyJobs(jobs []client.Job, err error) tea.Cmd {
154155
// transcript state — the start is already visible where the
155156
// job was launched (bg_start feedback).
156157
case old == "running" && j.Status != "running":
157-
// Alert tier + attention — but only when it demands a look:
158-
// a clean exit (code 0) is reported by the wake card and
159-
// the jobs tab; a failure (or an unreported code, which we
160-
// cannot call clean) must survive a glance-away.
161-
if j.ExitCode != nil && *j.ExitCode == 0 {
162-
break
163-
}
158+
// Alert tier + attention: a finished job must survive a
159+
// glance-away, and the operator opted into knowing. This is
160+
// the guaranteed completion report — the wake card is a
161+
// bonus that depends on odek ≥ v1.40 and wake_on_complete,
162+
// so it may never fire; this note must.
164163
m.addNote(jobExitNote(j))
165164
attn = m.attentionCmd(m.attentionFor(jobAttentionKind(j)))
166165
}

‎internal/tui/notice_quiet_test.go‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,23 @@ func TestEngineChatterNeverSurfaces(t *testing.T) {
3535
}
3636
}
3737

38-
// TestBgWakeStillArmsIdentity pins that suppression is render-only: the
39-
// wake marker must still arm so the unprompted turn keeps its identity
40-
// even though no note announces it.
41-
func TestBgWakeStillArmsIdentity(t *testing.T) {
38+
// TestBgWakeIdleSilentBusyNoted: while idle the wake card is the signal
39+
// (no note); while an operator turn streams the card cannot open, so the
40+
// completion must be reported as an alert-tier note instead.
41+
func TestBgWakeIdleSilentBusyNoted(t *testing.T) {
4242
m := newTestModel()
4343
m.handleEvent(client.Event{Type: "bg_wake"})
4444
if !m.wakeArmed {
4545
t.Fatal("bg_wake suppression must not lose the wakeArmed identity marker")
4646
}
4747
if len(m.notices) != 0 {
48-
t.Fatalf("bg_wake must not surface a note: %v", m.notices)
48+
t.Fatalf("idle bg_wake must not surface a note: %v", m.notices)
49+
}
50+
m.notices = nil
51+
m.busy = true
52+
m.handleEvent(client.Event{Type: "bg_wake"})
53+
if note, _ := lastNoteMatching(m, "background job finished"); note == "" {
54+
t.Fatalf("bg_wake during a busy turn must report the completion: %v", m.notices)
4955
}
5056
}
5157

@@ -60,15 +66,16 @@ func TestJobStartDiffSilenced(t *testing.T) {
6066
}
6167
}
6268

63-
// TestJobCleanExitSilenced: a clean (exit 0) terminal transition is
64-
// reported by the wake card / tab; the strip stays out of it.
65-
func TestJobCleanExitSilenced(t *testing.T) {
69+
// TestJobCleanExitStillReports: clean exits keep the alert-tier
70+
// completion note — the wake card depends on odek ≥ v1.40 and
71+
// wake_on_complete, so this note is the guaranteed report.
72+
func TestJobCleanExitStillReports(t *testing.T) {
6673
m := newTestModel()
6774
m.applyJobs([]client.Job{{ID: "j1", Status: "running", Command: "make test"}}, nil)
6875
zero := 0
6976
m.applyJobs([]client.Job{{ID: "j1", Status: "exited", Command: "make test", ExitCode: &zero}}, nil)
70-
if len(m.notices) != 0 {
71-
t.Fatalf("clean job exit must not reach the strip: %v", m.notices)
77+
if note, _ := lastNoteMatching(m, "j1"); note == "" {
78+
t.Fatalf("clean job exit must still be reported: %v", m.notices)
7279
}
7380
}
7481

‎internal/tui/wake_turn_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,11 @@ func TestWakeTurnLifecycleRendersMarker(t *testing.T) {
108108

109109
// The bg_wake frame is the operator's context for the unprompted activity:
110110
// surface it as a transient note.
111-
// TestBgWakeFrameNotifies: the bg_wake frame no longer posts a strip note
112-
// (the wake card itself is the operator-visible signal) but must still
113-
// arm the wake identity so the unprompted turn is never mistaken for an
114-
// operator exchange.
115-
func TestBgWakeFrameNotifies(t *testing.T) {
111+
// TestBgWakeFrameArmsMarkerSilently: the bg_wake frame no longer posts a
112+
// strip note while idle (the wake card is the operator-visible signal)
113+
// but must still arm the wake identity so the unprompted turn is never
114+
// mistaken for an operator exchange.
115+
func TestBgWakeFrameArmsMarkerSilently(t *testing.T) {
116116
m := newTestModel()
117117
m.handleEvent(client.Event{Type: "bg_wake", SessionID: "s1"})
118118
if !m.wakeArmed {

0 commit comments

Comments
 (0)