Skip to content

fix(agent): "No result was produced" has to be true where it is posted - #266

Open
WhichPaths wants to merge 1 commit into
yetone:mainfrom
WhichPaths:fix/no-result-was-produced-when-there-was
Open

fix(agent): "No result was produced" has to be true where it is posted#266
WhichPaths wants to merge 1 commit into
yetone:mainfrom
WhichPaths:fix/no-result-was-produced-when-there-was

Conversation

@WhichPaths

Copy link
Copy Markdown
Collaborator

Two ways the room gets told a lie about a turn, and one exit-code fix that the second needs.

1. "No result was produced" — posted under the result

postTurnFailureNotices posts

Agent run failed before it could finish (…). No result was produced.

into every conversation in the inbox whenever finalStatus === 'failed'. It never asks whether the turn produced something there.

A turn can answer in hop 3 and die at MAX_HOPS, the hard token ceiling, or a provider error in hop 40. The room has the reply. The red line lands directly under it, saying the opposite of what the user can see.

The data was already being collected. cliSideEffectsThisTurn records every visible write the CLI makes — postedReplyViaTool reads those same side effects to suppress double-posting. The notice simply never consulted them.

Now it does, per conversation, because one turn can span several and only some may have been answered.

A missing turn status is deliberately not this case: a user-visible reply already infers done (turn.status_inferred), which is correct and stays. The shapes that genuinely fail after an answer landed are MAX_HOPS, the token ceiling, and a provider error — the test uses the last of those.

2. A HELD relay is a stand-down, not a crash

cmdReply exits 2 to mean HELD — "declined on purpose, a peer already delivered this". turn.ts treated any !relay.ok as a failed run:

if (!relay.ok) {
  finalStatus = 'failed'

So the coordination system working exactly as designed was reported as a crash, with a failure notice posted under the peer's answer.

A HELD relay now ends the turn as 'skipped' — the status this file already uses for a turn that intentionally does nothing — and records turn.auto_relay_held, so the stand-down stays visible in observability rather than becoming silent.

This is the remaining half of #263. That one let the relay reach the gates at all; this one stops the room being alarmed when a gate legitimately declines.

3. Exit 2 has to mean one thing

Five call sites use exit 2 for HELD (reply ×3, calendar, document), and the HELD sites document it:

2,  // exit code 2 = "held, retry with different content" (distinct from 1 = generic error)

The top-level catch-all used 2 for an unexpected exception, which is not a hold. It exits 1 now. Nothing read exitCode before this change, so nothing else is affected.

Tests

Five. The two that matter are verified red by reverting their own half:

a room that already has the answer was told "No result was produced":
  [{"text":"Agent run failed before it could finish (runtime error). No result was produced."}]

a deliberate decline is not a failed run
  • a turn that already answered does not get told it produced nothing — answers in hop 1, provider dies on the next call; asserts the run still fails, the answer is in the room, and no notice
  • a HELD relay is a stand-down, not a failed run'skipped', no notice, turn.auto_relay_held recorded
  • a relay that genuinely failed is still a failed run — the guard rail: only exit 2 is a hold, or this would swallow lost answers
  • a HELD reply exits 2, an ordinary refusal exits 1 — the convention from both sides, through real cmdReply
  • the top-level catch-all does not claim to be a hold — reads the source, so the ambiguity cannot come back

The existing repeated missing turn status fails the run test is the other guard rail: a turn that produced nothing must still tell the room, and it still does.

Green: tsc --noEmit (server + renderer), biome lint ., all three scripts/guard-*.mjs, the unit suite (1390 tests, 0 failures), and the full agent-anti-duplicate suite (16/16).

Found alongside, not in this PR

The same audit turned up a message-loss bug in the neighbouring code: a turn that drains a mid-turn steer and then ends failed or skipped still advances conversation_reads past the steered message in its finally, which — because loadInbox compares a single ROW(created_at, id) cursor — buries the turn's own unanswered inbox along with it. That directly contradicts the fingerprint contract twenty lines away ("failed turns do not update the fingerprint, so they remain retryable instead of disappearing into a silent skip"). It is a separate change and I am opening it separately rather than bundling.

@yetone

yetone commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Parts 2 and 3 are right and I'd take them as-is.

HELD relay → skipped is correct: exit 2 is a documented stand-down, and reporting the coordination system working as designed as a crash — then posting a failure notice under the peer's answer — is exactly backwards. Reading relay.output.exitCode rather than widening !relay.ok keeps the guard rail, and the a relay that genuinely failed is still a failed run test is the right one to have written.

Exit 2 has to mean one thing — agreed, and worth doing on its own.

Part 1 is a real bug and you diagnosed it correctly. I can't take this implementation of it, though.

1. The predicate is not "an answer landed here."

visibleToUser === true && typeof conversationId === 'string' is satisfied by a good deal that isn't a result:

  • conversation.membership_changedleave / invite / kick (cli.ts:1641, 1708, 1785)
  • conversation.topic_updated (cli.ts:3923)
  • conversation.renamed (cli.ts:4011)

An agent that renames the room in hop 2 and dies at MAX_HOPS in hop 40 now says nothing at all. cliResultHasReplySideEffect in cli-result.ts already encodes the narrower predicate you want (event === 'message.posted').

2. The bigger one: the intent message is a message.posted/reply too.

Narrowing to message.posted doesn't fix it, because the announce and the answer are the same shape. The rules mandate the intent message before long work (turn.ts:2229), and long multi-hop turns are exactly the ones that reach MAX_HOPS or the token ceiling — so "announced, then died" is drawn from the same population of turns as the case you're fixing, not a corner of it. #268 exists because that flow is real.

In that case the room today gets:

Drafting the summary now.
Agent run failed before it could finish (runtime error). No result was produced.

The second line is true there, and it's the only thing telling the user to re-ask. After this change the room gets the promise and then silence. Nothing wakes the agent on an unread inbox alone, so nobody retries it either. That's a worse failure than the one being fixed, and it lands on the flow the product mandates.

3. Skipping the conversation also drops the observability record.

The continue sits above the event recording, so nothing is written. Compare the hourly-cap path twenty lines down, which deliberately still records turn.failure_notice with noticePosted: false. "The notice was withheld, and why" is exactly what you want in the timeline when someone asks why the room was never told.

What I'd take instead: don't suppress the notice, make the sentence true.

Your title is the right statement of the problem — "No result was produced" has to be true where it is posted. The sentence is what's false, so fix the sentence, not the notice:

const noticeText = delivered
  ? `Agent run failed before it could finish (${reason}).`
  : `Agent run failed before it could finish (${reason}). No result was produced.`

with delivered computed per conversation from event === 'message.posted', and the event still recorded either way. That keeps your correctness claim intact — no red line under an answer claiming the opposite — while the announce-then-die case still gets told something, which it must.

If you want to go further and suppress outright, that needs a way to tell an announce from an answer. Post-#268 there is one: cmdReply now records <runId>:<posts> per (agent, conversation), so the second post of a run is the delivery and the first is the announce. I'd land the text fix first and treat that as a separate question.

For context on the rest of the batch: #267 and #268 are both merged, as is #265. #268 touches turn.ts around the relay executePodTool call and cli.ts in cmdReply, so this will need a rebase — the relay hunk in particular is adjacent to the runId line it added.

On the message-loss bug you found alongside: that was #267, and it's in. Good catch, and thank you for splitting it out instead of bundling it.

Three things, all about a turn that fails after doing something.

1. The sentence. A turn can answer in hop 3 and die at MAX_HOPS or the token
   ceiling in hop 40. The room has the reply, and a red line directly under it
   says "No result was produced." Fix the SENTENCE, not the notice: drop the
   false clause where a message was posted, keep the whole notice everywhere.

   Suppressing the notice outright would be worse. The rules mandate an intent
   message before long work, and long multi-hop turns are exactly the ones that
   reach MAX_HOPS -- so "announced, then died" is drawn from the same
   population, not a corner of it. There the room would get the promise and
   then silence, and nothing wakes an agent on an unread inbox alone, so nobody
   retries. The failure line is the only thing telling the user to re-ask.

   The predicate is `message.posted`, not "any visible side effect": leave,
   invite, kick, topic_updated and renamed all report visibleToUser too, and an
   agent that renamed the room has answered nobody. Per conversation, because
   one turn can span several. The event is still recorded either way, now with
   resultDelivered, so the timeline says which sentence went out and why.

2. A HELD relay is a stand-down, not a crash. Exit 2 is cmdReply deliberately
   declining because a peer already delivered. Reporting the coordination
   system working as a failed run put "Agent run failed ... No result was
   produced" under the peer's answer. Read relay.output.exitCode rather than
   widening !relay.ok, so a relay that genuinely failed still fails the run.

3. Exit 2 has to mean one thing. runCli's top-level catch-all returned 2 for
   any unexpected exception, which is the same code five call sites use for
   HELD. A crash is not a stand-down; it now exits 1.
@WhichPaths
WhichPaths force-pushed the fix/no-result-was-produced-when-there-was branch from dddc9e5 to 0d9d100 Compare September 11, 2026 01:21
@WhichPaths

Copy link
Copy Markdown
Collaborator Author

Rebased onto main, and part 1 is rewritten the way you asked. All three of your objections were right — I checked each against the code before changing anything.

1. The predicate matched far too much. Confirmed: conversation.membership_changed (cli.ts:1641, 1708, 1785), conversation.topic_updated (3923) and conversation.renamed (4011) all set visibleToUser: true alongside a conversationId, so an agent that renamed the room in hop 2 and died in hop 40 would have been silenced. It is now event === 'message.posted' && visibleToUser !== false.

I kept it at message.posted rather than reusing cliResultHasReplySideEffect's full triple, because that helper also requires command === 'reply' and poll emits message.posted too (cli.ts:3094). A poll in the room is a delivered artifact; trimming the clause there is right. The three emitters are the two reply sites and that one.

2. Announce-then-die. You are right that narrowing does not separate the announce from the answer, and that suppression would land on the flow the product mandates. Not suppressing any more — the notice always posts, only the clause moves. In the announce case the clause is dropped where it would arguably still be true; that is the cost of the text-only fix, and it is much cheaper than silence. The <runId>:<posts> distinction #268 makes possible is left for the separate question you described.

3. Observability. Confirmed — my continue sat above the recording, while the hourly-cap path twenty lines down deliberately keeps turn.failure_notice with noticePosted: false. The continue is gone, and the event now also carries resultDelivered, so the timeline says which sentence went out.

Parts 2 and 3 are unchanged.

Tests. The old test asserted suppression, so it was rewritten:

  • a failure notice under a delivered answer drops the false clause — the notice is still posted, and asserts doesNotMatch(/No result was produced/)
  • a rename is not an answer — the full sentence stays — new, and it is the one that pins your objection 1: an agent that renames the room then dies must still be told to re-ask. It fails against the predicate I had before.

What I verified locally and what I did not. tsc, biome, the three guards and the unit suite (1396 tests, 0 failures) are green, as is agent-anti-duplicate.test.ts (22/22) after resolving its rebase conflict — #268 and this branch both appended at EOF.

I could not judge agent-turn.test.ts here: it needs a real model endpoint and fails on clean main in my environment for that reason ([embed] failed Request timed out, 33s per test). Those four tests passed on this PR's previous CI run, so I am leaving the verdict to CI rather than claiming a local green.

On the rest: agreed on splitting #267 out, and thanks for the correction on #263's repro — I had the trigger wrong, and the cross-wake-up version is what I wrote into #268.

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.

2 participants