Repository navigation
feat(server): acknowledge-only threads answer messages without a provider - #96
Conversation
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
chrisohearn
left a comment
There was a problem hiding this comment.
Independent Claude Opus review of 7b11d3e (review of record)
Read-only review of the diff, the run-start path, McpEvents and McpEventRelay, recovery, and PRs #90, #91 and #94. I ran AcknowledgeOnly.integration.test.ts under the heavy lock: 1/1 passed. Merge state at review: MERGEABLE/UNSTABLE. Build and Typecheck, Test and Test Web 2 were pending, and Agent context budget had failed.
P2
- Restart mid-queue holds the inbox. This is base behaviour, not a regression, but this mode is meant to fix it.
ProviderRuntimeRecoveryService.ts:420-442setsqueueHeld: trueon queued runs at shutdown and startup reconcile. Only web or mobile user messages release a hold (Orchestrator.ts:7429-7440); MCP sends do not. After a deploy restart, the ack-only inbox appears to stay silent, and Jarvis stops waking, until someone resumes the queue. There is no double ack:writeIfRunCurrentwithexpectedStatus: "starting"plus derived ids (AcknowledgeOnly.ts:64-68,156-162) make a replayed effect a no-op. Suggested fix: exempt ack-only threads from the hold or auto-resume them. At minimum, add this to the 0009 known limits and add a restart test. - Merge conflicts. #91 and #94 both edit the
t3_thread_configurepredicate, result struct, description and params (thread/handlers.ts:240-315,thread/tools.ts:187-207). This PR re-indents the whole block, so textual conflicts are certain. Semantically, once #91 lands, the ack branch must also rejectautoReviewsent alongsideacknowledgeOnly(handlers.ts:253-257); otherwise autoReview is silently dropped.agentContextBudget.jsonconflicts with all three PRs; re-measure. - The
Agent context budgetcheck fails:mcpToolListJsonBytes57258 -> 57391 (+133). The mcp-tool-use evals were not run. Merging needs a cut, or a maintainer'sagent-context-increaselabel.
P3
- Excerpt (
AcknowledgeOnly.ts:41-45). It is bounded: 120 code points, whitespace collapsed to one line, fixed prefix. But it relabels sender text as agent-authored output (createdBy: "agent",creationSource: "provider") and makes it the run's result text: assistant text and the relayresultExcerpt(McpEventRelay.ts:308). There is no secret masking and no Markdown escaping, sorenders.Array.fromcan also split grapheme clusters. Consider a code span and a token-pattern mask. setAcknowledgeOnly(AcknowledgeOnly.ts:27-39) does an unlocked read-modify-write on settings, so two toggles at once on different threads can lose one.- Likely: the hook (
ProviderTurnStartService.ts:461) returns before handoff delivery. A ready context handoff aimed at an acknowledged run (a provider switch or side chat) is never delivered, andisPendingForRunwill not pick it up after the mode is turned off. - At restart, a
startingrun whose message is not a retryable user message is cancelled, not acknowledged. The wake still fires asthread.run.resultwith state cancelled. The window is tiny. - The
settings:writeRPC scope can also setacknowledgeOnlyThreadIds(settings.ts:1969); this is documented. The hard-coded "Queued for Jarvis" text is odd in a generic per-thread mode.
Checked, no issue found:
- (a) Same
run.updatedevents through the same sink.observeEvent(subscriptions and webhooks) and the relay key only on status and runId. A relay binding's cursor starts at the run's first sequence minus one (McpEventRelay.ts:432-435), so a run that finishes in milliseconds is still relayed. - (d)
eitherpicks one access rule per call.writesEnvironmentappliesassertNotLinkedandassertFullAccessfor both on and off. - (e) Settings are re-read on every start, and the ack path never holds the queue.
- (f) Freeze holds the queue via
thread.stop, so the hook is never reached on a frozen thread. Snooze-respect is send-side only.
Verdict: no blocking findings
…ider A thread listed in the new acknowledgeOnlyThreadIds server setting answers each message with a server-written "Queued for Jarvis: <excerpt>" reply. The run starts, runs and completes with the same run and assistant-message events a provider turn emits, so inbox subscriptions and the notify relay see an ordinary run result, but no provider session opens and no model is called. Full-access MCP callers turn the mode on and off with t3_thread_configure acknowledgeOnly; reading the thread's configuration shows it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7b11d3e to
6cb51fa
Compare
Review fixes for the acknowledge-only mode:
- Restart recovery no longer holds an acknowledge-only thread's queue, and
releases a hold left from before, so the inbox keeps waking Jarvis after a
deploy.
- The reply is neutral ("Queued: `<excerpt>`"); the excerpt sits in a code
span so no link or image renders, and token-like strings are redacted.
- Toggles are serialized, so concurrent toggles cannot drop one.
- Context handoffs addressed to an acknowledged run are superseded explicitly.
- t3_thread_configure branches inside the existing write handler, keeping
upstream lines unindented.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chrisohearn
left a comment
There was a problem hiding this comment.
Independent delta review of c94302d (review of record, follows 7b11d3e review)
Read-only diff check: blobless clone, git range-diff 7eddc7fc..7b11d3e f0e6e4a325..c94302d. No tests run. Commit 1 changed only in the budget numbers; commit 2 holds the review fixes.
- Restart:
ProviderRuntimeRecoveryService.ts:425-448runs in the onereconcileProjectionused for startup and shutdown.holdQueueis false only when the thread is inacknowledgeOnlyThreadIds, and a failed settings read falls back to holding. Normal threads take the same condition andqueueHeld: trueas before. The skip of an existing hold depends onholdQueue, so older holds are re-emitted asqueueHeld: false. The test rowqueued-acknowledge-onlyasserts that the queue is not held. It starts from an unheld run, so I checked the release of an older hold by reading the code only. - Reply:
\s+collapses every line terminator, including U+2028 and U+2029. Every U+0060 becomes', so no backtick, and no run of backticks, can close the span. Redaction runs before the 120-code-point cut, and an empty excerpt givesQueued.. P3: thesk-,AKIAandASIAprefixes have no word boundary, so the redaction over-matches (task-listbecomesta[redacted]). This only hides extra text. - Lock:
Semaphore.make(1)wraps onlysetAcknowledgeOnly. That callsgetSettings/updateSettings, which never call back into it, so there is no nested or reentrant permit and no deadlock.withPermitsreleases on failure and on interrupt. - Handoffs:
context-handoff.updatedwith statussupersededgoes into the samewriteIfRunCurrentbatch as the completion, forhandoffswhosetargetRunId === run.id. Side-chat handoffs are outsidehandoffsand stay ready. This is minor and matches the documented carry-forward. - The prefix is neutral (
Queued:), and the docs match. handlers.tsagainst f0e6e4a shows additions only, with no re-indent.requireFullAccessmatches thewritesEnvironmentlocal check:orchestratingCaller, thenassertNotLinked, thenassertFullAccess. It now runs insidewritesThread, so the local checks are stricter than before. The linked-environment pre-check becomeswritingCaller, but the peer still refuses linked callers, so authority is not widened.- Budget:
mcpToolListJsonBytes57928 -> 58061 (+133).
At review: MERGEABLE/UNSTABLE, with every check pending.
Verdict: no blocking findings
|
Maintainer decision on the agent-context budget, from the round-four coordinator: approved with the |
09d1130
into
integration/mcp-events-20261109
…ol-reliability t3_thread_configure reads its configuration only when none of modelSelection, runtimeMode, applyAt and acknowledgeOnly is given. Budget re-measured on the tip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Jarvis inbox acknowledges every message with a full model run. On 2026-11-12 a long-lived acknowledger session acted on messages meant for Jarvis: it sent 19 messages as Jarvis, wrote the ledger 8 times and used a one-run approval. Each acknowledgement also costs a model call and about 10 s, and the queue backs up in bursts.
This PR adds a per-thread acknowledge-only mode. For a thread in the mode, the server completes each run itself with a written reply,
Queued: `<first 120 characters, one line>`(the excerpt is in a code span with token-like strings redacted). No provider session opens, no tools run and no tokens are spent. The run emits the same run lifecycle and result events as a provider turn, so inbox subscriptions, relay and notify wake Jarvis unchanged.The mode is not enabled anywhere. The coordinator enables it on the inbox after deploy and verifies that Jarvis still wakes.
Item 12 (revised): acknowledge-only inbox
Evidence (live DB, read-only)
run.createdqueued →run.updatedstarting → running → assistant node, message and turn item → attempt completed →run.updatedwaiting → checkpoint →run.updatedcompleted.Change
acknowledgeOnlyThreadIdsserver setting (packages/contracts/src/settings.ts). It needs no migration, and a bad hand-edited entry drops alone.ProviderTurnStartService.startgains a 20-line hook. It runs before slash-command handling and before any session open. For an acknowledge-only thread it callssettleAcknowledgeOnlyRunin the new fork-only fileorchestration-v2/AcknowledgeOnly.ts, which writes these events in one guarded commit:run.updatedrunning, attempt and root node running, the assistant node,message.updated(assistant), theassistant_messageturn item, attempt completed,run.updatedcompleted, root node completed, and the provider-thread status.waitingstep and no end-of-turn checkpoint (the live acknowledger's checkpoint wasmissinganyway). The queue advances through the existingrun.updatedcompleted reactor.t3_thread_configuregainsacknowledgeOnly, set alone. It requires a full-access caller (writesEnvironment) both to turn the mode on and to turn it off; read-only andautocallers are refused. Reading the thread's configuration reportsacknowledgeOnly(optional in the result, so linked environments on other builds still decode).ThreadManagementServicegainsacknowledgeOnlyandsetAcknowledgeOnly, so the thread toolkit needs no new dependency.docs/jarvis/decisions/0009-jarvis-inbox.mdis updated with what the mode does, its events, what gets acknowledged, who can change it, turning it off and its known limits.docs/user/outside-agents.mdand thedocs/jarvis/GLOSSARY.mdterm are updated too.run.updated, so a run that finishes in milliseconds is still relayed.Tests
AcknowledgeOnly.integration.test.tsuses the orchestrator, the effect worker and a fake provider adapter. Three messages queue before the worker starts. The test checks that:run.createdqueued →run.updatedstarting → running → completed;McpEvents.observeEventmaps each completion tothread.run.resultfor that run id.The mode is turned on and off through
AcknowledgeOnly.setAcknowledgeOnlywhile another thread id stays in the list, and the next message is then an ordinary provider turn. The test fails with the hook disabled.core.test.ts(existing configure access test, extended) checks that read-only andautoclients are refused both ways, that full access sets and clears the mode, and that the read shows it.settings.test.ts(existing hand-edit test, extended) checks that bad entries drop alone.Locally, under the round-four resource rule (shared lock,
--maxWorkers=2): server and contracts typecheck, plus 7 focused server test files (161 tests) and the contracts settings test. CI is the full gate.Acceptance check (read-only; set
sinceto the time the mode was enabled on the inbox)Pass:
completed = runs;provider_turns = 0andnon_ack_replies = 0;max_request_to_reply_s < 1when the queue is quiet, otherwise about 1 s per queued message ahead of it;jq .acknowledgeOnlyThreadIds ~/.t3/userdata/settings.jsonlists the inbox;bridge_statusshowslastSuccessfulEventDeliveryAtadvancing after the enable, which means Jarvis still wakes.Review fixes (independent review of record, 7b11d3e)
ProviderRuntimeRecoveryServiceno longer marks an acknowledge-only thread's queued runsqueueHeldat shutdown or startup, and it releases a hold left from before. The restart case is added to the existing "recovers $0 work after restart" table test.Queued: `<excerpt>`. The excerpt sits in a code span, so no Markdown link, image or autolink renders, and backticks become'. Token-like strings (32+ characters, and the update guard's known prefixes) become[redacted]. The integration test covers an image, a link, a backtick and aghp_token.ThreadManagementService.setAcknowledgeOnlyruns under a one-permit lock.withSettingsSnapshotcan't be used here, because it holds the settings write lock thatupdateSettingstakes.supersededin the same commit as the reply. Handoffs carried forward to later runs are untouched.t3_thread_configurenow branches inside the existing write handler through a newMcpToolAccess.requireFullAccess(thewritesEnvironmentcheck). Upstream lines are no longer re-indented: handlers.ts goes from 122 changed lines to about 40.Notes for the merger
agentContextBudget.jsonmcpToolListJsonBytesis provisional (this PR adds 133 bytes on top of the tip's 57,928; still 58,061 after the review fixes). Other round-four PRs change tool text too, so re-measure at merge.mcp-tool-useeval after tool-text changes. This worktree has no eval API keys, and the runs would spend money. The change only adds an optional admin parameter and one description sentence.ThreadManagementServicemocks that readt3_thread_configureneedacknowledgeOnly(three updated here). Expect the same on upstream syncs.Model: Claude Opus 5.5 (Claude Code harness in T3 Code).
🤖 Generated with Claude Code