fix(native-chat): record which Codex agent produced each journal row - #22532
Conversation
…evises The reducer rebuilds a row's producer linkage from its NEWEST revision, and absence is a positive claim: no agent id means the session's own agent wrote the row. So any revision written without the stamp hands a subagent's row back to its parent, permanently. Three host paths revise rows they did not write, from the render item they already hold, and all three dropped the stamp: - answering a prompt re-appended the asker's row with the fence only; - dead-generation settlement failed running tool calls and cancelled pending prompts through a lifecycle batch; - stale-session settlement on acquire cancelled lost prompts the same way. The batch path could not carry a producer at all: linkage was removed from the batch row because one row covers N mutations, with a note that a mixed batch would have to stamp per mutation. Dead-generation settlement is such a batch already, and Codex settlement is about to become one. So each item mutation now names its own producer, inline like the row base. A mutation that names none falls back to the row's linkage, which is what a batch read before. Parse sanitizes a bad per-mutation id the same way it does a row's: the field is dropped and the mutation kept. No schema version bump. An older host's mutation validator ignores unknown keys, so it reads a stamped mutation as the session's own, which is exactly what it shows today. Old journals carry no stamp and read as before. Turn revisions still carry nothing: a turn is the session's unit of work, and the live-turn scans rely on a turn row never carrying linkage. The note recording that invariant is updated to the new write sites.
…bagent Codex journals every thread on its app-server connection into the session's journal, and a spawned subagent's items arrive on the child's own thread. None of those rows carried producer linkage, so under the journal's rule that absence means the session's own agent wrote a row, every child's command, message, reasoning, prompt and status row read as the PARENT's: the parent could show its child's running command, its child's reasoning as "thinking", and its child's prose as its own latest line. The Claude lane's model is reused, not reinvented: the same fields and the same absence rule. What differs is how the producer is known. Orca opens exactly one thread per app-server, so any other thread is one Codex spawned. That decides WHETHER a row is a child's from its first frame, announced or not, and the thread id is final at once: it is never re-minted the way a tool-call reference is, so no correction ledger is needed for identity. - agentId: the child thread id, the same id the status side keys a Codex child on. - parentAgentId: the thread whose stream carried the child's `started` activity. Codex emits that item on the spawning agent's own session, so a child that spawned a grandchild is named; the session's own thread is not. Other activity kinds ride whichever agent acted and are not used. - producerKind: 'agent'. - attempt: which run of the child the row's own turn was, counted from the child turns the roster already observes; absent on the first run. Taken from the row's turn rather than the child's latest, so a persistent shell that outlives its turn keeps its run across revisions. - providerParentRef is omitted: a Codex child's frames carry no parent reference of their own beyond the thread id, which is already agentId. One resolver, owned by the roster (which already owns what is known about each child thread), is handed to every writer: items, streams, generic and summary rows, prompts, compactions, goals, and the three settlement batches. The session-end settlement mixes every thread's rows in one batch, so each mutation names its own producer. Turn rows stay unstamped: Codex writes them only for the primary thread. The spawn-group roster row stays unstamped on purpose: a child's frame can trigger its write, but it is the parent's list of its children. The translator's construction moves to a parts module so the translator stays a router under the line cap, and the item streams reuse one append-and-publish helper instead of two copies. Children are never swept at turn end; nothing here changes that.
… every parent reader Two layers, so a stamp that is correct in the store and never read, or read and never persisted, cannot pass. The readers, through the real path: translator, deferred sink, on-disk journal, snapshot. Each is a defect on main: the parent named its child's running command as its own tool, read its child's reasoning as itself thinking, showed its child's compaction as its activity line, and quoted its child's prose as its latest line (checked after closing and reopening the journal, so the stamp is read back from disk). The transcript still renders the child's rows. The writers, through a sink that records the linkage of every plain append, batch mutation and lifecycle transition: start, streamed checkpoint and completion of one command all restate the child; a row that beats the spawn announcement is still the child's; a grandchild names the child that announced it, while an `interacted` activity names no parent; a follow-up turn is the child's second run, and a shell that outlives its turn keeps its own; the exit batch settles each thread's rows under its own producer and the turn row under none; a child's provider frames, approval and goal rows are its own; nothing is stamped while the session thread is still opening; and the spawn-group row stays the parent's.
…nsition path A Codex child's goal row is written through a lifecycle transition, so a sink that forwarded only the fence there would file the child's goal as the session's own.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds subagent lineage and producer-linkage resolution to Codex journal writes. It carries linkage through lifecycle batches, journal parsing, reducer updates, and rendered revisions. Tests cover linked child rows, parent-facing readers, settlement, prompt resolution, and persistence after journal reopen. Merge Risk: ⚪ Minimal · up to No identified attribution or journal-persistence issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, testing, visual proof, limitations, and checklist. However, the required Linked Issue section states "None" even though the template requires an issue link.
✨ 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- Codex producer resolver:
codex-subagent-linkage.tsdecides attribution from the carrying thread — any non-primary thread is a child, withparentAgentIdfrom the spawner thread andattemptfrom the row's own turn ordinal. - One resolver, every writer:
codex-structured-journal-translation-parts.tsbuilds the writers once and threadslinkageForinto items, item streams, generic frames, prompts, compactions, goals, oversized settlement, and turn settlement. Item streams keep a per-stream producer so a parent learned after the first checkpoint reaches later revisions. - Roster bookkeeping:
CodexSubagentExecutionsrecordsspawnerThreadId(fromstartedonly) and capped per-childturnOrdinals; the roster's group row stays deliberately unstamped. - Batch mutations carry a producer:
JournalLifecycleMutationgains per-mutation linkage; the reducer readsjournalBatchMutationProducerwith a batch-row fallback, and the host revision paths (prompt answer, dead-generation and stale-session sweeps) restate the producer so a child's row cannot be re-filed as root. - Compatibility: no schema-version bump; unusable producer ids are dropped per mutation at parse; no new wire field.
Verification in this run: the new linkage and reader tests plus the reducer/schema tests pass (73 total). Forcing linkageFor to {} turns 11 assertions red, confirming the tests pin attribution rather than passing vacuously. The wider src/main/codex, agent-session-journal, and agent-session-wire suites pass except three environment-dependent files (--expose-gc retention tests and the codex app-server teardown integration) that are unrelated to this diff.
I did not find any correctness issue in the producer resolution, the batch/revision restatement, or the turn-row invariant. The one inline note is a stale comment count.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
The reducer took a row's producer linkage from its newest revision, so every writer that revised a row it did not write - a prompt answer, a dead-generation or stale-session settlement, the reopen sweep of stale subagent rosters - had to restate the producer or silently hand a subagent's row to the session's own agent. Three of those writers had been patched to restate it; the next one to forget would reintroduce the bug. Attribution is now fixed by a row's first write. A revision that names no producer keeps the row's existing linkage; one that names any replaces the whole bundle, which is how a provisional stamp is still corrected in place. A row re-created after a tombstone starts with nothing. The reducer runs the same fold on replay, so the kept producer survives a reopen. The three restatements are removed. Per-mutation linkage on lifecycle batches stays: a batch can create a row (a Codex child's prompt, or a child's item settled before any checkpoint landed) and one batch can mix producers.
…eopen A revision naming no producer keeps the row's, on the plain item path and in a batch settling a child's row beside the session's own; one naming any replaces the bundle wholesale; a tombstone clears it; a stale revision cannot touch it; and a reopened journal replays it exactly as it was folded live.
…reachable The stale revision is dropped whole by two independent guards before the inheritance rule runs, so its producer assertion could never fail; the reducer's own stale-revision tests already cover the drop. Also say what the batch sink does forward: each mutation's own producer.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the last pullfrog review (4 commits) makes producer attribution a property of the write that created a journal row, rather than of its newest revision, and removes the writer-side restatements that assumption made unnecessary.
- Reducer owns inheritance:
upsertItemnow keeps a row's whole producer bundle when a revision names none, and replaces it wholesale when a revision names any member.namesAgentJournalProduceris the presence check (any of the five fields), andjournalRenderItem/journalLifecycleItemMutation/journalBatchMutationProducerroute through it. - Host restatements removed: the prompt answer, dead-generation settlement, and stale-session sweep no longer re-state a producer on rows they revise; each revised row now keeps the producer its creating write named.
- Module rename: the translator's writer factory moved from
codex-structured-journal-translation-parts.tstocodex-structured-journal-translation-writers.ts(createCodexJournalTranslatorWriters). - Coverage: added
journal-producer-inheritance.test.ts(reducer inheritance, wholesale replacement, re-creation, stale revision, and a real on-disk reopen), plus end-to-end tests for the prompt-answer path and the dead-generation settlement, and refreshed the invariant comments inagent-session-journal-producer.ts,journal-reducer.ts,journal-row-builders.ts, andstructured-agent-session-live-turn.ts.
I re-read the full diff and verified the new rule cannot clear attribution it should not: no writer can name root explicitly, so a producer can only ever be added or corrected by a named revision, never dropped by an unstamped one. I had a focused look for any writer that revises an existing row without naming a producer while intending the attribution to change (the one way this refactor could regress) and found none — the Claude provisional-correction and streamed-checkpoint writers always name a producer when they restamp, and the Codex turn-row invariant (never stamped) holds, so the turn rows produced by the sweeps stay root.
The one inline note from the prior review (a stale "five append paths / all four" comment) is fully retired: the comment in structured-agent-session-journal-append-options.ts was rewritten and no longer states a count.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
brennanb2025
changed the title
Review status: ready as the attribution layer; the visible part is #22619Head: Review loops (3; the last one found nothing to change)
Readiness checklist: passed at the start (two small items, both fixed) and at the end (nothing at any severity). Compatibility checks:
Live app run. Screenshots and journal evidence are in the description under Visual Proof.
Known limits (in the description): the run number restarts after a re-acquire, as in the Claude lane. A grandchild's spawner is unknown after a re-acquire or in default mode. |
Resolve journal-row-schema.ts: keep both read-time repairs (per-mutation producer linkage and context usage). Move tryAppendLifecycleTransition into the resolved-append module beside the other resolve-then-append operations, keeping the event sink under the line cap.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior pullfrog review is the single merge commit 1ac26c16 that brings origin/main into the branch. git show --remerge-diff confirms only three files differ from the clean auto-merge, and all three are conflict resolution rather than new behavior:
- Conflict resolved in
journal-row-schema.ts: kept both read-time sanitisers — this PR's per-mutationdropUnusableProducerLinkageloop and main'sdropUnusableContextUsage(context-usage feature). Final order is outer sanitise → per-mutation sanitise →dropUnusableContextUsage; a bad field still degrades the row rather than dropping the whole journal. tryAppendLifecycleTransitionrelocated: moved verbatim from the event sink intostructured-agent-session-resolved-append.tsto keep the event sink under the line cap, preserving producer-linkage forwarding throughstructuredAgentSessionJournalAppendOptions. The event sink spreads the resolved-append module as before, soCodexJournalGoalsstill reaches it with its linkage.
Verification on the merged tree: pnpm tc:node clean; 104 tests across journal-row-schema, journal-reducer, journal-producer-inheritance, the event-sink, dead-generation, and stale-turn-verdict suites pass, plus 16 Codex linkage/reader/retention tests.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Conflict: structured-agent-session-dead-generation-settlement.test.ts, where main (#22532) and this branch each added a test at the same spot. Both tests are kept whole, with this branch's import and journal-stub changes.
…wn asks only The session's status is now `attention` only for its own pending prompt, so the clock's fallback to a subagent's ask could no longer be reached, and it read the journal by a different rule than the status it dates. Both now read root prompts. The journal also stamps a Codex subagent's prompt with its thread (#22532), so a Codex child's approval is that child's wait in the Codex lane too. Two tests written for the earlier rule are updated: a subagent's ask leaves a running session `working` on its turn's clock, and a Codex child's answered approval leaves the settled parent's Activity row done with nothing unread.
…wn asks only The session's status is now `attention` only for its own pending prompt, so the clock's fallback to a subagent's ask could no longer be reached, and it read the journal by a different rule than the status it dates. Both now read root prompts. The journal also stamps a Codex subagent's prompt with its thread (#22532), so a Codex child's approval is that child's wait in the Codex lane too. Two tests written for the earlier rule are updated: a subagent's ask leaves a running session `working` on its turn's clock, and a Codex child's answered approval leaves the settled parent's Activity row done with nothing unread.
…wn asks only The session's status is now `attention` only for its own pending prompt, so the clock's fallback to a subagent's ask could no longer be reached, and it read the journal by a different rule than the status it dates. Both now read root prompts. The journal also stamps a Codex subagent's prompt with its thread (#22532), so a Codex child's approval is that child's wait in the Codex lane too. Two tests written for the earlier rule are updated: a subagent's ask leaves a running session `working` on its turn's clock, and a Codex child's answered approval leaves the settled parent's Activity row done with nothing unread.
1 participant
ELI5
When a Codex chat starts helper agents (subagents), everything those helpers do is saved into the same chat history as the main agent's work. Nothing on those saved rows said which agent did the work, so Orca treated all of it as the main agent's. The parent-facing status readers (thinking indicator, running-tool line, activity line, latest assistant line, thread goal) therefore read a helper's rows as the main agent's. Each saved row now says which agent wrote it, the same way Claude rows already do, and later edits to a row (settling it, answering it) can no longer lose that.
This PR is the attribution layer: it makes the saved data correct and the shared readers skip helper rows. On its own it does not visibly change the chat in Codex's default multi-agent mode (see Visual Proof). The visible follow-up is #22619, which registers default-mode helpers and labels a helper's commands in the running-commands dock.
What Changed
Before: every row from a Codex child thread (commands, messages, reasoning, approvals, provider notices, compaction notes, goals) had no producer on it. The journal reads that as "the session's own agent wrote this." So the parent-facing readers (thinking indicator, running-tool line, activity line, latest assistant line, thread goal) reported a child's work as the parent's. Separately, for every provider, any later edit of a row that did not repeat its producer silently moved a subagent's row to the parent.
After: every saved row from a Codex child thread names the child, and the shared parent-scoped readers skip it. The transcript still shows every agent's rows. The running-commands dock above the composer is fed separately (by the host's background-command tracker, which deliberately lists a child's commands) and is not changed here.
1. Codex rows name their producer. Every row a Codex child thread produces carries the producer linkage the Claude lane already writes, with the same fields and the same rule that absence means the session's own agent:
agentId: the child's thread id, the same id the status side keys a Codex child on.parentAgentId: set only when another child spawned this one. It comes from the thread whose stream carried the child'sstartedactivity, because Codex emits that item on the spawning agent's own session.producerKind: 'agent'.attempt: which run of the child the row's own turn was (a follow-up turn is run 2). Absent on the first run.Orca opens exactly one thread per Codex app-server, so any other thread on the connection is one Codex spawned. That decides whether a row is a child's from the child's first frame. The thread id is final at once; unlike a tool-call reference it is never re-minted. One resolver, owned by the subagent roster, goes to every Codex writer: items, streamed checkpoints and patches, generic and summary rows, prompts, compactions, goals, and the settlement batches. The spawn-group roster row and turn rows are deliberately left unstamped: the roster row is the parent's list of its children, and a turn is the session's unit of work.
2. The journal keeps a row's producer across edits (all providers). The journal reducer used to take a row's producer from its newest revision. So any writer that revises a row it did not write (answering a prompt, the settlement that runs after an unexpected provider exit, the stale-session settlement on acquire, and the sweep that marks orphaned subagent rosters on reopen) handed a subagent's row back to the parent unless it remembered to repeat the producer. Now:
Replay from disk runs the same fold, so the kept producer survives a reopen. Every path that hands items to a client (live delta batches, history pages, snapshots and resets) reads them from the reduced state, never from the appended row, and clients replace an item wholesale by revision, so the parent's surfaces are right live, not only after a reload. The cost is one constant-time check per accepted revision; there are no extra scans.
Lifecycle batches can also name a producer per mutation, because a batch can create a row. Two examples: a Codex child's approval is first written inside a batch, and a child's streamed item can be settled before any checkpoint of it landed. One batch can also mix producers (Codex's exit settlement covers every thread).
Compatibility. There is no schema version bump. The new keys ride the row base (already shipped) and batch mutations. An older host ignores unknown keys on a mutation and reads it as the session's own, which is what it shows today. Journals written before producer linkage carry no stamp anywhere and render exactly as before. Journals that already hold Claude subagent rows which a settlement or prompt answer later revised: on replay those rows now show as the subagent's, which is the correct attribution and was previously lost. Render items already carry these optional fields over the wire, and older clients ignore them.
Why
The parent-facing readers already ask "is this row NOT mine" and skip anything with an
agentId, so correct producer data fixes all of them at once. Filtering Codex child threads inside each reader would add a second, Codex-only attribution rule next to the journal's own, and would still leave the saved rows wrong.For the edit problem, the first version of this PR made each writer that revises someone else's row repeat the producer (three host paths). That fixes the known writers but not the class: the next writer to forget brings the bug back, and at least one existing writer (the reopen roster sweep) was only safe because roster rows happen to be the parent's. Making "an edit does not change who wrote the row" a rule of the reducer removes the need for every writer to know, so those three restatements are gone. Before relying on it, every journal write site under
src/main(Claude, Codex, and the host) was checked for one that expects an unstamped edit to move a row from a subagent back to the parent. None does: no writer can name "the parent" explicitly, the Claude linkage has no path back to the parent by design, and a Codex thread id never changes. A test-time probe that flagged any such edit fired zero times across the Claude, Codex and native-chat suites, and fired correctly on a deliberate positive control.Splitting settlement into one batch per thread was rejected: it would break the exit settlement's all-or-nothing batch.
Linked Issue
None. This is part of the structured chat status/orchestration program.
Visual Proof
Live run in the Electron app, codex-cli 0.155.0-alpha (default multi-agent mode, which reports helpers through
collabAgentToolCall), GPT-5.5. Prompt: spawn one helper that runssleep 45; echo CHILD_DONEand repliesCHILD_REPLY, while the main agent waits and then repliesPARENT_DONE.What the screen shows is the same before and after. While only the helper runs, the activity line says "Coordinating with another agent" (the main agent's own spawn call) and the running-commands dock says "1 shell command — working" with the helper's command in it, unlabeled. The dock and the default-mode helper registration are #22619.
Before (main), while only the helper runs:
After (this PR), same moment, dock expanded:
What changed is the saved data, read from copies of the session journal:
agentId, including the helper's shell command andCHILD_REPLY.CHILD_REPLYcarryagentId= the helper's thread id andproducerKind: 'agent'. The main agent's turn rows andPARENT_DONEcarry none.agentId.The red "Reconnecting... 2/5" row visible in one after-run is Codex's own retry of its model stream (models were reporting capacity errors at the time), not an Orca reconnect.
Testing
All tests were run with
env -u ORCA_STRUCTURED_SESSION ORCA_BACKGROUND_LAUNCH=1.codex-structured-journal-subagent-readers.test.ts): translator, real deferred sink, real on-disk journal, snapshot, then the shared readers. The parent names its own tool, not the child's running command; the child's reasoning is not the parent "thinking"; the child's compaction is not the parent's activity line; and the parent's own latest line is quoted, read back after closing and reopening the journal. All 4 fail on main's code and show the defect itself.codex-structured-journal-translation-subagent-linkage.test.ts): plain appends, batch mutations and lifecycle transitions, including a row that arrives before the spawn announcement, a grandchild naming its spawner, run numbering, the exit batch settling each thread's rows under its own producer and the turn row under none, child approvals, provider frames and goals, nothing stamped while the session thread is opening, and the group row staying the parent's.journal-producer-inheritance.test.ts, new): an edit naming no producer keeps the whole set, on a plain row and in one batch settling a child's row beside the parent's; an edit naming one replaces it wholesale; a row re-created after removal inherits nothing; and a real on-disk journal replays the kept producer exactly as it was folded live after close and reopen.agentIdcomes back empty): the two reducer inheritance tests, the reopen test, and the prompt-answer, unexpected-exit and stale-session paths;parentAgentIdandattemptsurvive a correction);parentAgentId,attempt, the frame and goal stamps, and the sink's transition forwarding still apply to that unchanged code.src/main/codex,src/main/claude,src/main/native-chat, and the shared producer/live-turn/projection/status-line tests): 4,722 passed and 5 failed. All 5 are real-binary Claude CLI tests (claude-structured-real-cli,claude-tui-resume-real-binary) that fail the same way (provider close unproven) onorigin/main's code on this machine.pnpm tcis clean. The changed-code quality gate and fulloxlinton every changed file (including max-lines) pass.Not verified:
multi_agent_v2was not run live. The v2 frame shapes rely on:agentThreadIdandagentPath);startedactivity, and a child's stream carrying aninteractedactivity about the root;startedon the spawning session.AI Disclosure
Review
Reviewers, please check:
journal-reducer.ts(upsertItem): an edit naming no producer keeps the row's, one naming any replaces the whole set;structured-agent-session-live-turn.tsis updated);Agent skill upstream boundary
Notes
attemptcounts a child's turns seen by the current connection to the Codex app-server. After Orca re-acquires the session, a resumed child's next turn is recorded as run 1 again. The Claude lane counts the same way.parentAgentIdabsent means "the session's own agent spawned it". After a re-acquire, or under Codex's v1 multi-agent mode (the default on current Codex builds, which reports children throughcollabAgentToolCallrather thansubAgentActivity), Orca cannot see who spawned a grandchild, and it is recorded as the session's own child. Whether a row is a child's does not depend on either: it is decided from the thread id alone, so v1 child rows are still stamped withagentIdand kept off the parent's status.startedactivity, those early rows carry noparentAgentIduntil a later revision rewrites them. That is a narrow race, and only with non-default depth.attemptis bounded to the last 64 runs per child; rows of older runs read as the first run.providerParentRef, because its frames have no parent reference beyond the thread id, which is alreadyagentId.codex-structured-journal-translation-writers.tsand the item streams share one append-and-publish helper. Both original files were already at the line cap, and this keeps them under it.Checklist
N/Awith reason (attached; no visible change, by design of this layer)pnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (typecheck and targeted suites run locally; CI will cover the rest)