Repository navigation
Translate ACP traffic into shared timeline events - #25090
Conversation
…al timeline folder Pure moves so a shared timeline assembler can use them: Codex's message ordinal counter becomes ProviderTurnMessageOrdinals and Claude's turn-row revision becomes the provider-neutral agent-journal turn-row revision. Only names and import paths change.
…found again after a restart - A sink transition is admitted whole or not at all; its steps run back to back at their turn in the journal's write queue, and each resolver reads the fold with every earlier write landed. A resolver may also say where the row belongs (turn scope, provider reference), and the writer always hears how the transition landed. A resolved lifecycle batch chooses its settlement mutations from the fold at execution. - New optional row field providerItemRef: the provider's own reference for the item a row is, written only where the row's identity cannot spell it (Codex keys messages by their place in the turn and renumbers its item ids on resume). Set by the creating write, kept by revisions, indexed by the journal fold, never read by clients. A downgrade test shows an older host and client render such rows unchanged. - Provider timeline identity schemes (shared legacy arm, Codex) and the join index that resolves a provider item to its row from memory or the fold: ordinals and request incarnations are read back from the rows, so a restart or an evicted entry finds the original row instead of placing a new one.
09ac32c to
bdbd547
Compare
… joins read from a replaced epoch A fresh join index continued a turn's messages at the first free place, so a journal holding only a later ordinal (an imported or removed earlier row) had its sequence back-filled. The place is now one past the highest ordinal any row or echoed send holds there, read through a pure scheme reader. The join caches also drop what they read when the journal's epoch is replaced.
…t its turn in the journal Adapters translate their provider's dialect into a small grammar (turns, items, streamed text, requests, context facts, session end/reset); one shared assembler turns it into the journal rows every structured lane writes. Each event is planned as one sink transition. Which row a write lands on, whether a replay writes anything, and every change to what the assembler knows (its ledger) are decided by the transition's resolvers at the event's turn in the journal's write queue, against the fold as it stands then. A forecast (the ledger plus admitted events still queued) only answers apply() at once. So a refused event allocates nothing, a write the journal rejects leaves no trace in memory, and a restart or evicted cache finds the same rows again. Text and full snapshots of one provider item share one row and one lifecycle; reset always flushes text and settles the old session from the journal; the open-work budget is derived from what is actually open. Codex migration contracts compare against the existing Codex translator, including a restart mid-stream and a repeat that outlives the join cache.
…nd background work A third review found two blockers with the earlier rounds' cause, a remembered interpretation trusted after the journal moved on: - A reused request id was judged by its earlier prompt's settled turn before asking which turn the new one lands in, so a real approval in a later turn was dropped. The target turn now decides: the old turn again is a replay; a different live turn opens the next prompt beside it. - A text stream checked its row's turn only on its first write, and a turn's end released streams by the turn planning expected. Every write now checks the row, a turn's end stops the streams whose rows are in it, and turn status reads the journal first, so another writer's Stop wins. Also: a message boundary drawn by an event the journal held as a replay no longer splits an anonymous message; a send naming a turn not yet open waits for that turn; the budget charges a stream's thread and turn strings and the turn caches are byte-bounded; the open turn ends when the journal shows it settled; a turn's opener is read from the journal's row. Background work is now Orca's existing background-task row instead of a tool call flagged `outlivesTurn` (a flag remembered only in memory, so a restart failed the task). A turn's end never settles that row, so it survives restarts; session end leaves one in flight unverifiable. Three tests that opened a background tool call with `outlivesTurn` now open a background-task row and keep their original expectations about which turn the row stays in.
… streams A row kept for the anonymous stream that may continue it, and the marker that a stopped stream's queued writes write nothing, lived in the live-stream map and were never removed when no stream followed. They now live in their own bounded maps, so the live map holds open streams only.
…to brennanb2025/acp-d2-rebase2
c897071 to
d52d38b
Compare
…embler' into brennanb2025/acp-d2-rebase2
The common pattern discards the history an agent replays during session/load, keeping only what it says about the context window. Remove the adoption path (acp-history-adoption.ts, the adopt option, and the historical background-task liveness rewrite it fed) so load replay is always dropped except usage.
Review follow-up to the adoption removal: drop the comment naming the provider's saved message, and make requestedAt required since every pending input comes from input.accepted.
…embler' into brennanb2025/acp-d2-rebase2
Review follow-up to the adoption removal: the result-status mapping was only tested through adopted history, so run the same table on live frames, and cover an unmarked task notice during a load being dropped.
- journal-store.ts imports: main #24576 dropped journalStoreLoadedFields; this branch types appendResolvedItem off JournalItemAppender, so neither it nor JournalResolvedItem is imported. - event-sink-queue: keeps this branch's journalItems; journalStopDecidesTurn takes main #24864's two arguments (openedBy plumbing removed on main).
…ansitions' into brennanb2025/acp-c1-timeline-assembler
…embler' into brennanb2025/acp-d2-rebase2
|
Removed saved-history adoption ( What was removed: Why: the common pattern discards replayed history on load, so Orca does the same and has no adoption path. Verified: 19 ACP test files (174 tests) and 13 assembler test files (100 tests) pass, plus the other changed tests (33 files, 288 tests in total); one gated node typecheck found only this machine's stale-install For #25225: its |
|
Merged current main after #25064 landed; head |
📝 WalkthroughWalkthroughAdds ACP-to-provider timeline translation for prompts, turns, session updates, tool calls, context usage, requests, and background tasks. Adds Grok-specific notification and request mapping. Adds bounded snapshots for tool and task updates, JSONL recordings, a fixture test rig, and tests for translation, replay, routing, and fixture privacy. Also adds a shared journal-body helper for background-task rows and uses it in the Claude background-task journal. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The ACP translation is not yet connected to any user-facing path. The remaining issue is narrow: when the active Grok model has no window metadata, context-usage rows may show another model's window. This can be handled as a follow-up and should not block the merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 29 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/acp/acp-timeline-fixture.test-support.ts (1)
111-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the pending row by request identity, not by kind.
The rig resolves the first pending row whose kind matches the reply's kind. Suppose one fixture has two pending approvals at the same time. The rig can then resolve the wrong row, and the tests still pass. The current fixtures answer each request before the next one opens, so no test fails today. The
requestid is already stored inrequestsat Line 102. Matchitem.itemIdagainst the row that this request opened, so the rig resolves the row the provider actually asked about.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b28b989a-f1d3-4cba-8d90-b6c724846a3d
📒 Files selected for processing (36)
src/main/acp/acp-background-task-timeline.tssrc/main/acp/acp-context-usage.tssrc/main/acp/acp-dialects/acp-dialect.tssrc/main/acp/acp-dialects/grok-background-tasks.tssrc/main/acp/acp-dialects/grok-dialect.tssrc/main/acp/acp-dialects/grok-requests.tssrc/main/acp/acp-fixture-privacy.test.tssrc/main/acp/acp-prompt-turns.tssrc/main/acp/acp-session-update.tssrc/main/acp/acp-timeline-background-task-evidence.test.tssrc/main/acp/acp-timeline-background-task-results.test.tssrc/main/acp/acp-timeline-background-task-words.test.tssrc/main/acp/acp-timeline-background-tasks.test.tssrc/main/acp/acp-timeline-dialect.test.tssrc/main/acp/acp-timeline-fixture.test-support.tssrc/main/acp/acp-timeline-generic.test.tssrc/main/acp/acp-timeline-joins.test.tssrc/main/acp/acp-timeline-open-enums.test.tssrc/main/acp/acp-timeline-recordings.test.tssrc/main/acp/acp-timeline-recovery.test.tssrc/main/acp/acp-timeline-requests.tssrc/main/acp/acp-timeline-routing.test.tssrc/main/acp/acp-timeline-translator.tssrc/main/acp/acp-timeline-turn-failures.test.tssrc/main/acp/acp-tool-timeline.tssrc/main/acp/acp-turn-failures.tssrc/main/acp/acp-turn-messages.tssrc/main/acp/fixtures/s1-basic.jsonlsrc/main/acp/fixtures/s1-full-notifications.jsonlsrc/main/acp/fixtures/s2-permission.jsonlsrc/main/acp/fixtures/s3-cancel.jsonlsrc/main/acp/fixtures/s4-resume.jsonlsrc/main/acp/fixtures/s5-plan-approved.jsonlsrc/main/acp/fixtures/s6-background.jsonlsrc/main/claude/claude-background-task-row-journal.tssrc/shared/native-chat-background-task-row.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| function contextWindow(models: unknown): number | undefined { | ||
| const parsed = modelsSchema.safeParse(models) | ||
| if (!parsed.success) { | ||
| return undefined | ||
| } | ||
| const { currentModelId, availableModels = [] } = parsed.data | ||
| return ( | ||
| availableModels.find((model) => model.modelId === currentModelId)?._meta?.totalContextTokens ?? | ||
| availableModels.find((model) => model._meta)?._meta?.totalContextTokens | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the fallback window only when Grok names no current model.
contextWindow falls back to the first model that has _meta.totalContextTokens, and it does this even when currentModelId is set. Suppose currentModelId names a model without _meta, or a model absent from availableModels. The function then reports the window of a different model. AcpContextTimeline.update stores that value. Every later context.usage event then carries a wrong window.tokens, so the occupancy ratio is wrong for the rest of the session. Grok's x.ai/models/update notifications can change models mid-session, so this state can happen.
When currentModelId is present, return undefined if that model has no window. An unknown window is safer than the window of another model. Keep the first-model fallback only for the case where Grok does not name a current model. If you keep the fallback on purpose, add a comment that explains why it is safe.
🐛 Proposed fix
const { currentModelId, availableModels = [] } = parsed.data
- return (
- availableModels.find((model) => model.modelId === currentModelId)?._meta?.totalContextTokens ??
- availableModels.find((model) => model._meta)?._meta?.totalContextTokens
- )
+ if (currentModelId !== undefined) {
+ return availableModels.find((model) => model.modelId === currentModelId)?._meta
+ ?.totalContextTokens
+ }
+ return availableModels.find((model) => model._meta)?._meta?.totalContextTokens
}Based on learnings: "avoid silently swallowing errors, falling back to defaults ... If a fallback is truly intentional and safe, add a comment explaining why it's safe; otherwise flag the silent fallback as a bug."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function contextWindow(models: unknown): number | undefined { | |
| const parsed = modelsSchema.safeParse(models) | |
| if (!parsed.success) { | |
| return undefined | |
| } | |
| const { currentModelId, availableModels = [] } = parsed.data | |
| return ( | |
| availableModels.find((model) => model.modelId === currentModelId)?._meta?.totalContextTokens ?? | |
| availableModels.find((model) => model._meta)?._meta?.totalContextTokens | |
| ) | |
| } | |
| function contextWindow(models: unknown): number | undefined { | |
| const parsed = modelsSchema.safeParse(models) | |
| if (!parsed.success) { | |
| return undefined | |
| } | |
| const { currentModelId, availableModels = [] } = parsed.data | |
| if (currentModelId !== undefined) { | |
| return availableModels.find((model) => model.modelId === currentModelId)?._meta | |
| ?.totalContextTokens | |
| } | |
| return availableModels.find((model) => model._meta)?._meta?.totalContextTokens | |
| } |
Source: Learnings
Main now carries ACP-ALIGN #25810 (squash 0bcd49c) and D2 #25090 (squash 670c59d); both squashes equal the branch heads D3 already merged (20f7f8a, 9bd27f9), so the merge was computed against main's tree with those heads as extra parents (scratch commit, never pushed) and committed as an ordinary two-parent merge of origin/main. No conflicts.
ELI5
Orca has a client for the Agent Client Protocol (the open protocol Grok and other agents speak), but nothing turns what an agent sends into native chat history yet. Recorded Grok traffic shows the translation needs explicit rules: Grok's internal notifications would flood the conversation, separate replies would merge, context usage would be overstated, and a background command would lose its status when the tool that launched it closes.
Reopening a saved chat is the other hard case. When an agent loads a saved session, it first replays that session's whole history. An earlier version of this branch tried to merge that replay into whatever Orca had already recorded, to finish a reply an Orca crash had cut off. That made the translator read the whole recorded chat on many frames, and it decided "is this chat new?" from a view that is always empty at that point in a real resume. This version drops the merge and, like the common pattern, never writes a replayed history at all: everything an agent replays while a saved session loads is dropped, except what it says about context usage.
No user-visible change. Nothing here is connected to an agent or to the chat UI yet; the Grok runtime adapter (#25225) does that. Once connected:
Dependencies #25064, #25141 and #24990 have landed on main. This diff now contains only Agent Client Protocol event translation and its tests; the shared assembler, transitions and protocol client come from main. Its base is
main.A few terms used below: the journal is Orca's stored record of a chat; the assembler (#25064) is the shared code that turns an agent's events into journal rows; a turn is one request and everything the agent does for it.
What Changed
Translation (unchanged by this revision):
[monitor]or[monitor:is a monitor, because that prefix is the only way those results name one.kill_command_or_subagentcall still ends the tasks named in its input: an inference no captured kill confirms yet.Failed turns (this revision):
errororrate_limitstop reason, the translator writes one status row inside that turn, before its end. It is a plain status row in the error tone whose text is Grok's own reason: the same shape the Codex lane writes for an error that ends a Codex turn already running. It carries none of Orca's "message was refused" wording, because that is for a message the provider never started.rate_limit. Any other agent gets " ended this turn with an error.", from a name the runtime adapter passes in ("The agent" until it does).data.message) and the Grok-named sentences stay in the Grok dialect; the translator only knows "this turn failed, here is the provider's reason".Saved history:
Why
The agent parser should describe what it receives; the shared assembler enforces lifecycle rules. Dropping replayed history entirely, as the common pattern does, removes a whole class of merge bugs (overwriting a reply a person stopped, regressing a settled turn, duplicating text) instead of guarding each one. The alternative, finishing a crash-cut reply from the agent's saved history, would make Grok behave differently from Claude and Codex and needs the translator to read the whole record; the trade is that a Grok reply cut by an Orca crash now reads as cut off, like the other agents.
A failed turn's reason goes into a status row, which clients already render, rather than a new kind of notice, and it is worded the way the Codex lane words an error that ends a running turn: the provider's own text. Writing it from the translator, inside the failed turn, covers the real order Grok uses, where the turn has already started when it fails. A prompt refused before its turn starts is a different case: the runtime adapter reports it on the message, in Orca's refusal words.
Differences from the common pattern
[monitor]or[monitor:is a monitor, and a task keeps the kind it started with.Linked Issue
Part of the Agent Client Protocol integration following #25064, #25141 and #24990; no separate issue.
Visual Proof
N/A — nothing here reaches the renderer or a running agent.
Testing
src/main/acp(translation, fixture privacy scan, protocol client), and 100 across the 13 assembler test files (72 files, 756 tests across every test file this PR's stack changes insrc/main/acpandsrc/main/native-chat).acp-history-adoption.ts, the translator'sadoptoption, and the "replayed task start is unverifiable" rewrite only adoption reached): the adoption tests (generic, multi-turn user history, two on the recorded Grok resume, replayed task-output results, and the replayed-task outcome cases).5c4a89f0613, which also merges Add a shared timeline assembler for structured agent chats (not wired yet) #25064's latest branch so it merges cleanly with currentmain): no errors in this branch's code; its only 5 errors are a dependency (stream-json) missing from the shared, stale local install.What I verified / didn't
Verified by tests through the real assembler and serialized record, using the existing scrubbed Grok recordings plus synthetic traffic. Each fix in this revision was removed in turn and its test failed without it; the placement test fails when the record of a finished tool's turn is removed. I also replayed third-party live Grok recordings (Grok 1.0.41 and 1.0.44, 15 scenarios, outside this PR) through the translator: the failed-prompt recording now shows Grok's reason in the failed turn, and the monitor recording keeps its task a monitor. I read the assembler's open-work accounting to confirm it counts a running tool at no fewer bytes than the translator does.
Not verified:
No agent CLI, Electron app or headless runtime was run. Passing tests show nothing regressed; they don't prove the design is right.
Review
@BrennanKB5
Agent skill upstream boundary
Notes
This remains a draft. Connecting it is #25225; the cancelled-tool display is #25181.
Checklist
5c4a89f0613): green, including "static analysis and typecheck", all five unit-test shards and cross-version wire compatibility (two jobs that never got a runner were re-run)Earlier main refresh (67e12c3)
Merged main at
4e64fa9940f5a4a9bfb7b021a3c405fd76643ca5; head67e12c3046b36da60175d59b5902b4d1d3a0c947. Main supplies the landed transition and protocol client code, including the current Stop and steer handling. All 69 remaining translation/assembler patch sections are unchanged. Four conflicts take main verbatim, including its required ACP boundary-check lane and seven-file fixture. Local targeted verification: 297 passing tests across 34 explicit files; two boundary-check cases cannot resolve stream-json/stream-chain from the stale symlinked node_modules. Scoped format/oxlint passed on 66 code files. The lockfile and submission-position test exactly match merged main with exactly one used provider-handle import. Fresh CI is pending; no local typecheck was run.Refresh after the shared assembler landed
Merged current main at
1de3aa405f71ea693b7138c40dac62e76883a10f; current head9bd27f9d16b524ffa2dd83ee3c60a9fc1b9c4aba. All 36 translation-only sections and 4116 added/deleted feature lines are preserved exactly, with no copies of landed dependency changes. All 14 explicit suites / 97 tests and scoped format/oxlint on 29 code files passed. The root lockfile and entire submission-position test match merged main, with exactly one used provider-handle import. Main’s corrections to the release-checkout and cache-scan tests were inherited through the merge. Current-head CI is fully terminal and green; no local typecheck was run.Current-head CI finished
At
9bd27f9d16b524ffa2dd83ee3c60a9fc1b9c4aba, all four current-head runs are complete: 14 checks passed and 18 were skipped. Static analysis/typecheck, all five unit shards, both packaging jobs, mobile and relay integration passed; the compatibility check was skipped by its path filter. The unchanged main Windows host-job test missed its startup marker on the first run and failed cleanup; one targeted Windows retry passed the test, packaging and smoke checks without source changes. Final verification passed.