fix(agent): "No result was produced" has to be true where it is posted - #266
fix(agent): "No result was produced" has to be true where it is posted#266WhichPaths wants to merge 1 commit into
Conversation
|
Parts 2 and 3 are right and I'd take them as-is. HELD relay → 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."
An agent that renames the room in hop 2 and dies at 2. The bigger one: the intent message is a Narrowing to In that case the room today gets:
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 What I'd take instead: don't suppress the notice, make the sentence true. Your title is the right statement of the problem — const noticeText = delivered
? `Agent run failed before it could finish (${reason}).`
: `Agent run failed before it could finish (${reason}). No result was produced.`with 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: For context on the rest of the batch: #267 and #268 are both merged, as is #265. #268 touches 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.
dddc9e5 to
0d9d100
Compare
|
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: I kept it at 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 3. Observability. Confirmed — my Parts 2 and 3 are unchanged. Tests. The old test asserted suppression, so it was rewritten:
What I verified locally and what I did not. I could not judge 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. |
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
postTurnFailureNoticespostsinto 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.
cliSideEffectsThisTurnrecords every visible write the CLI makes —postedReplyViaToolreads 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 areMAX_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
cmdReplyexits 2 to mean HELD — "declined on purpose, a peer already delivered this".turn.tstreated any!relay.okas a failed run: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 recordsturn.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:
The top-level catch-all used 2 for an unexpected exception, which is not a hold. It exits 1 now. Nothing read
exitCodebefore this change, so nothing else is affected.Tests
Five. The two that matter are verified red by reverting their own half:
'skipped', no notice,turn.auto_relay_heldrecordedcmdReplyThe existing
repeated missing turn status fails the runtest 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 threescripts/guard-*.mjs, the unit suite (1390 tests, 0 failures), and the fullagent-anti-duplicatesuite (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
failedorskippedstill advancesconversation_readspast the steered message in itsfinally, which — becauseloadInboxcompares a singleROW(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.