fix(pointer): adopt the worker's attested session so factory PRs stop emitting session_ref=missing - #494
fix(pointer): adopt the worker's attested session so factory PRs stop emitting session_ref=missing#494khaliqgant wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds optional attested session references to Relay agent messages. Factory uses these references to fill missing remote-worker session data, preserves existing values, records adoption metrics, and includes regression tests. ChangesSession reference propagation
Priority: ⬇️ Low — Defer this narrow Relay placement fix because it only backfills missing session references for remotely placed workers. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Remote Relay workers can now publish their attested session reference in factory-opened PR metadata without overwriting an existing reference. The change is covered by regression tests and is ready to merge. Sequence Diagram(s)sequenceDiagram
participant RelayFleetClient
participant Factory
participant TrackedAgent
participant PullRequest
RelayFleetClient->>Factory: Emit AgentMessage with sessionRef
Factory->>TrackedAgent: Look up message.from
Factory->>TrackedAgent: Set sessionRef when absent
Factory->>PullRequest: Publish tracked sessionRef
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. A rabbit carries a session bright Comment |
Codex Review SummaryThis 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. |
|
@coderabbitai review Requested for exact head |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a36057033
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // installs the babysitter critical-section fence synchronously before | ||
| // its first await, and an adoption read ahead of it would open a gap | ||
| // the fence exists to close. | ||
| void this.#adoptAttestedSessionRef(message) |
There was a problem hiding this comment.
A standard worker can follow the generated instructions without ever triggering this callback: src/dispatch/templates.ts tells it to report completion through invoke_action and explicitly not through a DM or shared channel, and that event is routed through onAgentLifecycleSignal, not onAgentMessage. If the worker sends no incidental Factory-visible message, its attested ref is never adopted and the completion handler still publishes the PR with session_ref=missing; carry the ref through the lifecycle invocation or otherwise make attestation part of the required flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this was the most important of the four — thank you.
I verified the claim rather than taking it on faith: src/dispatch/templates.ts line 404 says Do not send completion to a named control agent or shared channel, 408 dictates the exact invoke_action input, and 409 says Do not replace it with a DM or shared-channel post, including #general. The implementer’s only other outbound is a DM to the reviewer (line 178), which Factory’s own client never observes. I also checked the SDK: replayMessageMetadata is applied in messaging/relaycast.js at lines 171/186/198/213 — the send/reply/dm paths — and not to commands.invoke. So a compliant worker completes with no Factory-visible message and no attestation, exactly as you describe.
Fixed by carrying the ref through the lifecycle invocation, which is symmetric with how lifecycleUsageFromInvocation already lifts an optional usage record out of the same worker-supplied input:
lifecycleSignalFromInvocationnow readssessionRef/session_reffrominvocation.input;AgentLifecycleSignalcarries it.#handleAgentLifecycleSignaladopts it before any branch that can end in a publish.lifecycleInstructionsnow requires the key, sourced from the worker’sRELAY_ATTEST_SESSION_ID, with an explicit instruction to omit it rather than invent one.
Tests: carries a worker-attested session ref off the lifecycle invocation input (relay client) and adopts the attested session from the lifecycle completion signal (factory) — I confirmed both fail with the fix disabled (expected undefined to be 0190f75d-…), so neither is vacuous. Plus a templates assertion so the instruction cannot be dropped silently.
| const record = (await this.#batch()).getIssueByAgent(message.from) | ||
| const tracked = record?.agents.get(message.from) | ||
| if (!tracked || tracked.sessionRef) return | ||
| tracked.sessionRef = sessionRef |
There was a problem hiding this comment.
When the daemon crashes after this in-memory assignment but before the next lifecycle phase save, the durable lifecycle still contains the original missing ref. Startup reconstructs the tracked agent from that row, so if the worker exited during the outage or does not repeat a message after reconnect, reconciliation publishes its branch with session_ref=missing despite the attestation having already arrived. Checkpoint this mutation through the serialized lifecycle persistence path so restart recovery cannot discard it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and fair to call out — I had flagged the same window in the PR body and then argued myself out of it on the grounds that the next phase transition persists the mutation for free. Your failure case is the one that argument does not cover: a crash inside the window, with the worker exiting during the outage, leaves startup rebuilding the tracked agent from a row that still says missing.
#applyAttestedSessionRef now checkpoints through the same serialized path the rest of the lifecycle uses (#saveDispatchLifecycle with the record’s current phase), so the adopted ref is durable at adoption time rather than at the next transition.
Two deliberate bounds:
- It is skipped for terminal phases (
isTerminalDispatchPhase), since re-saving there would drag in#drainAgentUsageand the run-cost finalisation for what is only a field update. - A failed checkpoint is logged and swallowed, not thrown. Adoption already happened in memory and the publish path reads the in-memory record, so losing a race for lifecycle ownership must not turn a successful adoption into a failed one. That is a deliberate trade: the durable write is best-effort, the in-memory one is not.
One write per agent, not per message — it is gated behind the tracked.sessionRef gap check, so it fires once per generation at most.
…g session_ref=missing
Every factory-opened PR carries `session_ref=missing`. The cause is not
Factory's parsing: a real placement spawn's completed invocation
(inv_223040291642302464) reports
"output": { "agent_id": "223040362062057472", "name": "ws10-placement-probe",
"invocation_id": "inv_223040291642302464", "session_ref": null }
while the node's own broker reported that same agent as
sessionId 0190f75d-2915-4c9c-a31b-6354234eee29 at that moment.
That output is the engine's FleetInventoryAgent record, materialised when the
spawn action completes — before the broker's `agent.register`/`inventory.sync`
frame delivers the worker's session to the engine. Re-reading the same
invocation ten minutes later still returns null; it never backfills. `name` is
present, so a spawn that lost its session looks entirely healthy.
The internal broker path is unaffected: driving the real InternalFleetClient
against a live broker returns { name, sessionRef: "bb49fadc-…" }, because
/api/spawn carries sessionId at the top level.
No engine read surfaces the ref (agents.list/get/presence, sessionEvents,
relay:live-agents:v1 all lack it), and seeding our own UUID as the spawn input's
`session_ref` means *resume* — that spawn never confirms.
What does reach us is the worker itself: the broker stamps
RELAY_ATTEST_SESSION_ID into its environment and the Relay SDK carries it as
`metadata.session_ref` on the worker's messages. `#emitAgentMessage` was
discarding `message.metadata`. Carry it through and adopt it into
tracked.sessionRef the first time a worker speaks, which is what already feeds
canonicalTrajectorySessionRef -> githubPullRequestBody.
Adoption only fills a gap — a tracked ref (resumed lineage, or an internal-path
spawn result) is never displaced. It runs off the listener rather than inside
`#handleAgentMessage`, which installs its babysitter fence synchronously before
any await.
opencode is NOT covered: that engine reports no sessionId at all, so no ref is
attested and its implementers still render `missing`. That hole is upstream.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LPR3Wioz9nUmoPPWem5JUN
Session-Id: 7ac6ab72-3998-4367-9fcd-4c099c9bc6d6
Session-Id: 7ac6ab72-3998-4367-9fcd-4c099c9bc6d6
9a36057 to
a465f65
Compare
|
@coderabbitai review Requested for exact head |
… messages Review of #494 found the adoption watched the wrong channel for the flow that matters. `lifecycleInstructions` tells a worker to report completion through `invoke_action` and explicitly NOT to DM or post to a shared channel (templates.ts:404/408/409), and its only other outbound is a DM to the reviewer that Factory never observes. The Relay SDK stamps `session_ref` onto sent messages only (messaging/relaycast.js:171/186/198/213), never onto `commands.invoke`. So a worker that follows its task emitted no attestation at all and still published `session_ref=missing`. Carry it through the lifecycle invocation, symmetric with the `usage` record already lifted off the same worker-supplied input: - lifecycleSignalFromInvocation reads sessionRef/session_ref - AgentLifecycleSignal carries it; #handleAgentLifecycleSignal adopts it before any branch that can end in a publish - the rendered task requires the key, sourced from RELAY_ATTEST_SESSION_ID, and says to omit rather than invent it Also from review: Stale generations. Agent names are deterministic and reused, and a remote respawn reopens the gap (recordSpawn takes result.sessionRef ?? spec.sessionRef, both undefined), so a late message from the previous worker could fill it with a dead session. #emitAgentMessage now drops the attestation when the message predates the placement holding that name, keyed on the spawnedAtMs the client already tracks. The message is still delivered; only the claim is dropped. Exit ordering. Adoptions are serialized per agent and #handleAgentExit awaits the pending one before branching on tracked.sessionRef. Durability. #applyAttestedSessionRef checkpoints through #saveDispatchLifecycle at adoption time instead of waiting for the next phase transition, so a crash in that window cannot leave the durable row saying `missing`. Skipped on terminal phases; a failed checkpoint is logged, never thrown, since the in-memory adoption already succeeded. Counter renamed to agentSessionRefsAdoptedFromAttestation now that it covers both routes. Tests: the lifecycle-invocation read, the factory lifecycle adoption, and the stale-generation guard were each confirmed failing with their fix disabled. The attest-then-exit test is NOT proof of the exit guard — the fake dispatches both callbacks synchronously and it stays green with the guard removed; it is labelled as covering the end-to-end shape only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPR3Wioz9nUmoPPWem5JUN Session-Id: 7ac6ab72-3998-4367-9fcd-4c099c9bc6d6
|
@coderabbitai review Requested for exact head |
|
@coderabbitai review Context for this pass: the earlier run was rate-limited and never read the code, so this is the first look. The branch has since been rebased onto Two places where an extra pair of eyes would be most useful:
Also flagging honestly: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 494 --repo AgentWorkforce/software-garden --json number,title,headRefName,baseRefName,commits
gh pr diff 494 --repo AgentWorkforce/software-garden > /tmp/pr494.diff
wc -l /tmp/pr494.diffLength of output: 6334 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,705p' /tmp/pr494.diffLength of output: 33316 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd '\.ts$' src/orchestrator -x echo {} | grep -i factory | head
echo "---outline `#saveDispatchLifecycle---`"
ast-grep run --pattern 'async `#saveDispatchLifecycle`($$$) {
$$$
}' --lang typescript src/orchestrator/factory.ts | head -150Length of output: 1005 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'async `#saveDispatchLifecycle`' src/orchestrator/factory.ts
rg -n 'function isTerminalDispatchPhase|isTerminalDispatchPhase' src/orchestrator/factory.ts | head -20
rg -n 'lifecycleEpoch|epoch' src/orchestrator/factory.ts | head -60Length of output: 4146 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '7980,8120p' src/orchestrator/factory.tsLength of output: 6394 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n '`#scheduleDispatchLifecycleRetry`' src/orchestrator/factory.ts | head -20
sed -n '/#scheduleDispatchLifecycleRetry(record: InFlightIssue)/,/^ }/p' src/orchestrator/factory.ts | head -80Length of output: 1925 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'spawnedAtMs' src/fleet/relay-fleet-client.ts | head -30
rg -n 'createdAt' src/fleet/relay-fleet-client.ts | head -30Length of output: 1339 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'class FakeFleetClient' -A5 src/orchestrator/factory.test.ts | head -20
rg -n 'emitAgentMessage|emitAgentExit|onAgentMessage|onAgentExit' src/orchestrator/factory.test.ts | sed -n '1,40p'Length of output: 531 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'FakeFleetClient' -l src | head
rg -n 'emitAgentMessage\s*[:(]|emitAgentExit\s*[:(]' src -g '!*.ts.map' | head -30Length of output: 3589 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'emitAgentMessage|emitAgentExit|onAgentMessage|onAgentExit' src/testing/fakes.ts
sed -n '1,50p' src/testing/fakes.ts | head -5Length of output: 619 (370, 445) You are interacting with an AI system. |
1 participant
Every factory-opened PR carries
session_ref=missing. Measured across this repoon 2026-09-08: #493, #465, #464, #461, #457 all
missing; #469, #460, #444, #430carry no pointer at all. Zero PRs carry a resolvable session UUID, which leaves
the
githublens thatrelayhistory-cloudPR #41 specified with nothing to keysession_linksrows on.The seam, with the captured spawn output
I placed a real
spawn:claudethroughmessaging.placement.spawn({confirm:true})against a live node, exactly as
RelayFleetClient.#spawndoes. This is theRelayActionInvocationthatspawnResultFromInvocationreads — invocationinv_223040291642302464, verbatim:{ "invocationId": "inv_223040291642302464", "actionName": "spawn", "input": { "name": "ws10-placement-probe", "agent": "ws10-placement-probe", "clone_path": "/tmp", "cwd": "/tmp", "spawn_mode": "task_exit", "exit_after_task": true, "capability": "spawn:claude", "node": "chief-broker", "target_node": "chief-broker", "cli": "claude" }, "output": { "agent_id": "223040362062057472", "name": "ws10-placement-probe", "invocation_id": "inv_223040291642302464", "session_ref": null }, "status": "completed", "error": null, "createdAt": "2026-09-08T11:22:29.000Z", "completedAt": "2026-09-08T11:23:10.000Z" }At that same moment, the node's own broker reported that agent as:
So the id exists and the payload Factory reads reports it as null. Factory's
parsing is not at fault:
spawnResultFromInvocationalready reads all four keyvariants off both
outputandoutput.agent, and there is simply nothing in theoutput to read.
nameis present, which is exactly why a spawn that lost itssession still looks completely healthy — nothing anywhere reports a problem.
Why it is null. That output shape is the engine's
FleetInventoryAgentrecord —
{agent_id, name, invocation_id, session_ref}— materialised when thespawn action completes. The broker delivers the worker's session to the engine
afterwards, on the
agent.register/inventory.syncframes (both carrysession_refin@relaycast/types' fleet wire schema). Re-reading the very sameinvocation ten minutes later still returns
session_ref: null; it neverbackfills.
The two paths differ, and only one is broken
InternalFleetClient(broker/api/spawndirect)RelayFleetClient(remote placement)I drove the real
InternalFleetClientagainst a live broker rather thanreasoning about it:
The broker's
/api/spawnresponse carriessessionIdat the top level — Iconfirmed it is returned even when the caller supplies a full Factory-shaped
harnessConfig, and that it matches the roster entry exactly. So the internallane was never the problem, and this change does not alter it.
Repairs I ruled out empirically, not by reasoning
agents.list,agents.get,agents.presenceandsessionEvents.listcarry no session ref for abroker-spawned agent —
metadata.fleetholdsnode_id,invocation_id,registered_atand nothing else. The node'srelay:live-agents:v1capabilityadvertises agent names only and is not invokable as an action.
session_ref. That key meansresume, not set. The spawn never confirmed: the invocation stuck at status
dispatchedand the placement failedspawn_unconfirmedafter 120s.The fix
The broker stamps
RELAY_ATTEST_SESSION_IDinto every spawned worker'senvironment, and the Relay SDK carries it as
metadata.session_refon themessages that worker sends. Verified live in this workspace — a running agent's
messages carry
{"session_ref":"707f2b00-3cee-41ce-b976-153bc9afdabf"}, matchingits roster
sessionIdexactly.RelayFleetClient.#emitAgentMessagewas buildingAgentMessageas{from, target, body, threadId, eventId}and droppingmessage.metadataon thefloor. So the one readable copy of the session id was arriving and being
discarded. This change:
sessionReftoAgentMessage(src/ports/fleet.ts);metadata.session_refinRelayFleetClient.#emitAgentMessage;tracked.sessionRefthe first time a worker speaks(
FactoryLoop.#adoptAttestedSessionRef), from which the existingcanonicalTrajectorySessionRef(implementer.sessionRef)call already feedsgithubPullRequestBody.The adoption only fills a gap — a ref already tracked (a resumed lineage, or
a spawn result that did carry one, as the internal path does) is never moved by
a message. It runs as its own task off the inbound-message listener rather than
inside
#handleAgentMessage, because that function installs the babysittercritical-section fence synchronously before its first await and must keep doing
so.
Adoption writes to the in-memory tracked agent and takes no durable write of its
own:
cloneTrackedAgentalready serialisessessionRef, so the next#saveDispatchLifecycle— which every phase transition between dispatch andpublish performs — persists it for free, without adding a write per inbound
message or reaching for lifecycle ownership from a listener task.
relay_inboundbroker events carry no message metadata, so the internal clientis deliberately left alone: it already gets the ref from the spawn result.
opencode is NOT covered, and this does not pretend otherwise
ws6-nightcto-commitsha(opencode) reportssessionId: nullon the broker whileevery claude and codex lane reports a UUID. No session is minted, so no
RELAY_ATTEST_SESSION_IDis stamped, so no message can attest one. Anopencode-spawned implementer will still render
session_ref=missingafter thischange. That is an engine-side hole, not a Factory one, and closing it belongs
upstream — it is called out here rather than silently fixed only for the engines
that already work.
Contracts left alone
canonicalTrajectorySessionRefstill rejects the nil UUID as well as an absentone; the fleet layer passes the ref through opaquely and does no UUID
filtering, so the two cases stay distinguishable exactly where they were.
session_ref=missingstill parse toundefinedviatrajectoryPointerFromBody. Unchanged.session_sourceinsrc/trajectory.ts,src/index.tsanddocs/pr-session-replay.md. This one touches only the fleetseam and the factory adoption. The two compose — [garden] Carry the session source in the PR trajectory pointer #493 supplies the source, this
supplies the id that makes the source mean something.
Tests
renders the pointer from a worker-attested session when the spawn result carries nonefails on today's code and passes after:Also added:
keeps a tracked session ref when a worker attests a different one(adoption never displaces), and
surfaces a worker-attested session ref from inbound message metadataat theRelayFleetClientseam itself, which alsoasserts an unattested message stays absent rather than becoming a blank.
Review round (rebased onto #493)
Four threads, all addressed in code. The P1 was right and it undercut the
original fix, so it is worth restating rather than burying:
The adoption watched the wrong channel for the flow that matters.
lifecycleInstructionstells a worker to complete throughinvoke_actionandexplicitly NOT to DM or post to a shared channel (templates.ts:404/408/409), and
its only other outbound is a DM to the reviewer, which Factory never observes.
The SDK stamps
session_refonto sent messages only(
messaging/relaycast.js:171/186/198/213) and never ontocommands.invoke. So aworker following its own task emitted no attestation and still published
session_ref=missing. The ref is now carried through the lifecycle invocation —symmetric with the
usagerecord already lifted off that same worker-suppliedinput — and the rendered task requires the key.
Also fixed: a stale-generation guard (deterministic names are reused, and a
remote respawn reopens the gap, so a late message from a previous worker could
attach a dead session); per-agent serialization with
#handleAgentExitawaitingthe pending adoption; and a durable checkpoint at adoption time rather than at
the next phase transition.
One test does not prove its fix, and says so. The attest-then-exit test stays
green with the exit guard removed —
FakeFleetClientdispatches both callbackssynchronously, so the adoption always wins in-process. It is labelled in the file
as covering the end-to-end shape only; the guard is retained on the reviewer's
reasoning, not on evidence I produced. The other three new tests were each
confirmed failing with their fix disabled.
Suite
After the rebase and the review fixes, the five affected suites
(
factory,relay-fleet-client,internal-fleet-client,templates,trajectory) run 985 passed, 0 failed.tsc -p tsconfig.build.jsonclean.Counts across the rebase:
trajectory.test.ts11 -> 33 (all of the +22 is#493's),
factory.test.ts735 -> 738 (#493's one test plus two of mine here).Earlier, before the rebase:
2585 passed | 1 skippedon the full suite, then11 passedfor the two files that need a build first.src/__tests__/dist-entrypoints.test.tsimportsdist/, so it fails on avitest runthat was not preceded bynpm run build; afternpm run buildbothit and
src/git/agent-worktree.test.tspass.One note for whoever owns the babysitter tests: under machine load, tests in the
FactoryLoop PR babysitterblock time out non-deterministically — a differentset each run. I checked this against the unmodified base rather than assuming,
and
origin/mainwith this change reverted fails four of them on the samemachine and the same command, two of which (
retries internal babysitter wakes at a confirmed safe boundary,restarts an unreachable babysitter and delivers the preserved wake) fail on every loaded run with or without this change. They allpass unloaded. Pre-existing and unrelated to this PR, but it is a wider flake
than the
src/cli/teammate-mcp.test.tsone that is already known.🤖 Generated with Claude Code
https://claude.ai/code/session_01LPR3Wioz9nUmoPPWem5JUN