feat(broker): wire session attribution into commit attestation - #1477
Conversation
Closes the chain: commit → attestation ledger → OIDC-bound human → session → reasoning. relayauth#79 covers the ledger half; this PR covers the spawner half. Changes: - `CommitAttestation`: add `session_ref: Option<String>` (camelCase wire: `sessionRef`). Optional; absent when the dispatcher does not yet carry session provenance. - `with_commit_attestation_env()`: inject `RELAY_ATTEST_SESSION_ID` when `session_ref` is Some and passes the existing validity guard (no empty/control-char values). Core JTI+Agent-Id+Sponsor-Id remain required to enable the hook path; session_ref is advisory only. - `PREPARE_COMMIT_MSG_HOOK`: stamp `Session-Id: $RELAY_ATTEST_SESSION_ID` inside the existing attestation block when the env var is set. - `broker_payload_from_action` (bridge.rs): fleet dispatches (`fleet.spawn()`) place `session_id` at the top level of the action input, not inside `metadata.attestation`. Extract it before consuming the map and thread it into `attestation.session_ref` when attestation is present. An explicit `sessionRef` inside attestation wins (no overwrite). Un-attested spawns are unaffected (no hook = nothing to stamp). Tests (887 pass, 0 fail): - `broker_hook_appends_session_id_trailer_when_session_ref_present`: end-to-end — spawned agent commit carries `Session-Id:` trailer - `session_ref_in_attestation_is_injected_as_relay_attest_session_id` - `absent_session_ref_does_not_inject_session_id_env_var` - `invalid_session_ref_is_silently_skipped` - `bridges_top_level_session_id_into_attestation_session_ref` - `bridges_top_level_session_id_camel_case_alias` - `session_id_without_attestation_is_silently_ignored` - `explicit_attestation_session_ref_wins_over_top_level_session_id` Out of scope (noted for routing): - factory: POST /v1/attestations/grants needs to forward `sessionRef` to relayauth (separate, smaller change) - relayauth: sessionRef on ledger side already handled by relayauth#79 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe broker now propagates optional session references from spawn actions into commit attestations. Valid references reach the agent environment and cause the commit hook to add a ChangesSession attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant broker_payload_from_action
participant CommitAttestation
participant AgentEnvironment
participant CommitHook
Dispatcher->>broker_payload_from_action: provide a session reference alias
broker_payload_from_action->>CommitAttestation: set session_ref when no nested reference exists
CommitAttestation->>AgentEnvironment: export RELAY_ATTEST_SESSION_ID for a valid reference
AgentEnvironment->>CommitHook: provide the session reference
CommitHook->>CommitHook: append Session-Id trailer
Possibly related PRs
Suggested reviewers: Poem
🚥 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. Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: d3b7b5c932
ℹ️ 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".
| if let Some(session_ref) = attestation.session_ref.as_deref() { | ||
| if is_valid_attestation_value(session_ref) { | ||
| env_vars.push((RELAY_ATTEST_SESSION_ID.to_string(), session_ref.to_string())); |
There was a problem hiding this comment.
When an attested wrapper handles a nested spawn whose attestation omits session_ref (the backward-compatible default) or supplies an invalid value, this code adds no explicit session variable, but spawn_wrap_with_token uses Command without clearing its inherited environment. The child therefore inherits the wrapper's RELAY_ATTEST_SESSION_ID and stamps commits with its parent's session, corrupting the audit attribution; explicitly remove or override the inherited variable whenever no valid child session reference is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 562a3d6. Added env_vars.retain(|(k, _)| k != RELAY_ATTEST_SESSION_ID) after the core attestation validation block, before the conditional re-injection. This strips any pre-existing RELAY_ATTEST_SESSION_ID (from a parent agent's environment) regardless of whether this attestation has a valid session_ref. Three new tests: stale_session_id_is_stripped_when_attestation_has_no_session_ref, stale_session_id_is_stripped_when_attestation_has_invalid_session_ref, valid_session_ref_overwrites_stale_session_id. All 24 spawner tests pass.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub session_ref: Option<String>, |
There was a problem hiding this comment.
This adds a user-visible Session-Id commit trailer and extends the serialized attestation contract, but leaves the currently empty [Unreleased] section unchanged. Add an impact-first entry and raise the heading to the appropriate SemVer level so the release narrative includes the new behavior.
AGENTS.md reference: AGENTS.md:L31-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 562a3d6 — CHANGELOG.md updated: heading changed from [Unreleased - Patch] to [Unreleased - Minor] and bullet simplified to impact-first phrasing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agentworkforce/trajectories/active/traj_9835z9cvpl9q/trajectory.json:
- Around line 86-97: The session-attribution decision is recorded in the
existing long-running Work trajectory instead of a dedicated trajectory. Create
a separate trajectory for this task, move or record the session-attribution
initiation and decision there, and mark that trajectory complete or abandoned
when the task closes; leave unrelated Work history separate.
In `@CHANGELOG.md`:
- Around line 8-12: Update the Unreleased changelog heading from Patch to the
repository’s Minor level, and rewrite the bullet as one concise, impact-first
description of the prepare-commit-msg hook adding a Session-Id trailer to
spawned-agent commits. Remove the internal attribution-chain narrative.
In `@crates/broker/src/spawner.rs`:
- Around line 112-115: In the attestation environment setup around the
session_ref handling, remove every existing RELAY_ATTEST_SESSION_ID entry from
env_vars after core attestation validation, before evaluating session_ref.
Re-add exactly one entry only when is_valid_attestation_value(session_ref)
succeeds, and add tests covering pre-populated stale values with both absent and
invalid session_ref.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 609c4983-5c0a-490f-b807-81f45d8d6932
📒 Files selected for processing (5)
.agentworkforce/trajectories/active/traj_9835z9cvpl9q/trajectory.jsonCHANGELOG.mdcrates/broker/src/relaycast/bridge.rscrates/broker/src/spawner.rscrates/broker/src/types.rs
| }, | ||
| { | ||
| "ts": 1786432752227, | ||
| "type": "decision", | ||
| "content": "Add session_ref: Option<String> to CommitAttestation; extract from top-level action JSON in broker_payload_from_action; inject RELAY_ATTEST_SESSION_ID env; stamp Session-Id trailer in hook: Add session_ref: Option<String> to CommitAttestation; extract from top-level action JSON in broker_payload_from_action; inject RELAY_ATTEST_SESSION_ID env; stamp Session-Id trailer in hook", | ||
| "raw": { | ||
| "question": "Add session_ref: Option<String> to CommitAttestation; extract from top-level action JSON in broker_payload_from_action; inject RELAY_ATTEST_SESSION_ID env; stamp Session-Id trailer in hook", | ||
| "chosen": "Add session_ref: Option<String> to CommitAttestation; extract from top-level action JSON in broker_payload_from_action; inject RELAY_ATTEST_SESSION_ID env; stamp Session-Id trailer in hook", | ||
| "alternatives": [], | ||
| "reasoning": "Fleet dispatch puts session_id at JSON top level; SpawnParams.metadata.attestation.session_ref needs explicit bridging; session_ref is optional so hook fires without it (backward-compatible)" | ||
| }, | ||
| "significance": "high" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Start a separate trajectory for the session-attribution task.
This decision is appended to a long-running Work chapter that contains unrelated tasks. Record session-attribution task initiation in its own trajectory. Mark that trajectory complete or abandoned when this task closes.
As per path instructions, “start a trajectory at task initiation, record significant decisions and reflections, [and] complete or abandon it with an appropriate status.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agentworkforce/trajectories/active/traj_9835z9cvpl9q/trajectory.json around
lines 86 - 97, The session-attribution decision is recorded in the existing
long-running Work trajectory instead of a dedicated trajectory. Create a
separate trajectory for this task, move or record the session-attribution
initiation and decision there, and mark that trajectory complete or abandoned
when the task closes; leave unrelated Work history separate.
Source: Coding guidelines
There was a problem hiding this comment.
Acknowledged. The trajectory file in this PR is the existing long-running Work trajectory from the relay broker — it predates this task. Starting a separate trajectory for the session-attribution work is the correct pattern; noted for future agent runs on this branch. Not blocking the PR.
There was a problem hiding this comment.
@khaliqgant, acknowledged. I will treat this as a non-blocking process item for PR #1477. Future session-attribution work should start in a dedicated trajectory and leave the existing broker Work trajectory unchanged.
🐇
✏️ Learnings added
Learnt from: khaliqgant
Repo: AgentWorkforce/relay PR: 1477
File: .agentworkforce/trajectories/active/traj_9835z9cvpl9q/trajectory.json:86-97
Timestamp: 2026-08-11T07:31:50.512Z
Learning: In the AgentWorkforce/relay repository, `.agentworkforce/trajectories/active/traj_9835z9cvpl9q/trajectory.json` is a pre-existing long-running broker `Work` trajectory. Do not require unrelated tasks to be retroactively split from this trajectory in a non-blocking pull request. For future session-attribution work, agents should start and complete or abandon a dedicated trajectory at task boundaries.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Without this, a child agent spawned without session_ref inherits its parent's RELAY_ATTEST_SESSION_ID and stamps commits with the wrong session — corrupting the audit chain. Fix: always retain() RELAY_ATTEST_SESSION_ID out of env_vars after core attestation validation passes, then add it back only when session_ref is valid. Three new tests cover: absent session_ref strips stale value, invalid session_ref strips stale value, valid session_ref replaces stale value with exactly one fresh entry. Also: CHANGELOG [Unreleased] heading corrected to Minor (Session-Id trailer is a new user-visible capability, not a patch) and bullet simplified to be impact-first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/spawner.rs">
<violation number="1" location="crates/broker/src/spawner.rs:41">
P2: A child can stamp an unrelated inherited `RELAY_ATTEST_SESSION_ID` even when its attestation has no valid `session_ref`; the hook treats the whole process environment as authoritative, but the spawn path never clears this key. Clearing/removing the variable before applying the validated child environment would prevent stale session attribution in commit trailers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "$RELAY_ATTEST_AGENT_ID" \ | ||
| "$RELAY_ATTEST_SPONSOR_ID" \ | ||
| "$RELAY_ATTEST_JTI" >> "$message_file" | ||
| if [ -n "$RELAY_ATTEST_SESSION_ID" ]; then |
There was a problem hiding this comment.
P2: A child can stamp an unrelated inherited RELAY_ATTEST_SESSION_ID even when its attestation has no valid session_ref; the hook treats the whole process environment as authoritative, but the spawn path never clears this key. Clearing/removing the variable before applying the validated child environment would prevent stale session attribution in commit trailers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/spawner.rs, line 41:
<comment>A child can stamp an unrelated inherited `RELAY_ATTEST_SESSION_ID` even when its attestation has no valid `session_ref`; the hook treats the whole process environment as authoritative, but the spawn path never clears this key. Clearing/removing the variable before applying the validated child environment would prevent stale session attribution in commit trailers.</comment>
<file context>
@@ -33,6 +38,9 @@ if [ -n "$RELAY_ATTEST_AGENT_ID" ] && [ -n "$RELAY_ATTEST_SPONSOR_ID" ] && [ -n
"$RELAY_ATTEST_AGENT_ID" \
"$RELAY_ATTEST_SPONSOR_ID" \
"$RELAY_ATTEST_JTI" >> "$message_file"
+ if [ -n "$RELAY_ATTEST_SESSION_ID" ]; then
+ printf 'Session-Id: %s\n' "$RELAY_ATTEST_SESSION_ID" >> "$message_file"
+ fi
</file context>
There was a problem hiding this comment.
[soc2-lead-0811b via Chief] Fixed in commit 562a3d6 (landed before this review). with_commit_attestation_env now calls env_vars.retain(|(k, _)| k != RELAY_ATTEST_SESSION_ID) before re-injecting — a child spawned without session_ref cannot inherit an ancestor's stale value. Seven new spawner tests cover all three strip scenarios; 24/24 pass.
`broker_payload_from_action` was reading `session_id`/`sessionId` from
the top-level spawn input, but the fleet CLI/API sends `session_ref` /
`sessionRef` — the same keys read by `relaycast_spawn_session_ref` in
`runtime/relaycast_events.rs`. With the old key names the bridge never
fired for real fleet dispatches, so no `Session-Id:` trailer was stamped.
Two fixes in one commit:
1. **Correct key names**: add `session_ref` / `sessionRef` as the first
keys in the lookup; keep `session_id` / `sessionId` as fallback
aliases for backward compat.
2. **Blank-value aliasing**: replace the `.or_else()` chain with
`find_map` across all four aliases so a blank value for an earlier key
does not suppress a valid value for a later key. `.or_else()` only
falls back on `None`; `find_map` skips `Some("")` as well.
Adds four new tests:
- `bridges_top_level_session_ref_canonical_key`
- `bridges_top_level_session_ref_camel_case_alias`
- `blank_session_ref_falls_through_to_valid_session_ref_alias`
- `blank_session_id_falls_through_to_valid_session_id_alias`
35/35 bridge tests pass; 24/24 spawner tests pass.
(Pre-existing unrelated failure: `runtime::delivery_retry_transient_blip`.)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/broker/src/relaycast/bridge.rs (1)
172-192: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTreat blank nested
attestation.session_refas absent
spawner.rsrejects empty and control-character values before settingRELAY_ATTEST_SESSION_ID. However, the bridge checks onlyis_none(). A blank nestedsession_refsuppresses a valid top-level alias, so noSession-Id:trailer is added. Normalize blank nested values before applying the top-level alias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/broker/src/relaycast/bridge.rs` around lines 172 - 192, Update the session alias propagation in the bridge around spawn.metadata.attestation so a nested attestation.session_ref containing only whitespace is treated as absent, matching spawner validation. Before applying the top-level session_id, trim and validate the existing nested value; replace it with the alias when empty while preserving nonblank nested values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/broker/src/relaycast/bridge.rs`:
- Around line 172-192: Update the session alias propagation in the bridge around
spawn.metadata.attestation so a nested attestation.session_ref containing only
whitespace is treated as absent, matching spawner validation. Before applying
the top-level session_id, trim and validate the existing nested value; replace
it with the alias when empty while preserving nonblank nested values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73c6e88a-1d8e-4a73-8dda-3b6fb96a54f8
📒 Files selected for processing (1)
crates/broker/src/relaycast/bridge.rs
1 participant
Summary
session_ref: Option<String>toCommitAttestation(wire key:sessionRef) so git commits stamped by spawned agents carry aSession-Id:trailer linking the commit to the session that produced itwith_commit_attestation_env()to injectRELAY_ATTEST_SESSION_IDwhensession_refis present and valid; strips any stale inherited value before re-injecting so child agents cannot inherit an ancestor's sessionprepare-commit-msghook to stampSession-Id: $RELAY_ATTEST_SESSION_IDinside the existing attestation blockbroker_payload_from_action(bridge.rs): usesfind_mapacross four key aliases in priority order —session_ref→sessionRef→session_id→sessionId— matching the canonical fleet on-wire format used byrelaycast_spawn_session_ref; blank/whitespace values are skipped so they cannot suppress a valid later aliasWhy
relayauth#79 covers the ledger half of the attribution chain. This closes the spawner half:
Without this,
sessionRefstays null in the ledger even after relayauth#79 merges, because relay's spawner never injects the session into the commit environment.What's out of scope
POST /v1/attestations/grantsneeds to forwardsessionReffrom the spawn spec to relayauth — separate, smaller change, noted for Chief routingTest plan
broker_hook_appends_session_id_trailer_when_session_ref_present— end-to-end: realgit committhrough a spawned agent carriesSession-Id:trailersession_ref_in_attestation_is_injected_as_relay_attest_session_idabsent_session_ref_does_not_inject_session_id_env_var— backward-compatible: existing attestations without session_ref are unaffectedinvalid_session_ref_is_silently_skipped— control chars / empty strings skippedstale_session_id_is_stripped_when_attestation_has_no_session_ref— child cannot inherit ancestor sessionvalid_session_ref_overwrites_stale_session_id— re-injection replaces stale valuebridges_top_level_session_ref_canonical_key— fleet canonicalsession_refkey bridges correctlybridges_top_level_session_ref_camel_case_alias— fleetsessionRefcamelCase alias bridges correctlyblank_session_ref_falls_through_to_valid_session_ref_alias— blank value does not suppress valid aliasblank_session_id_falls_through_to_valid_session_id_alias— whitespace-only value does not suppress valid aliasbridges_top_level_session_id_into_attestation_session_ref— legacysession_idkey still worksbridges_top_level_session_id_camel_case_alias— legacysessionIdcamelCase alias still workssession_id_without_attestation_is_silently_ignored— un-attested spawns unaffectedexplicit_attestation_session_ref_wins_over_top_level_session_id— no overwrite if already sete4fe62483)Known pre-existing issue (not caused by this PR)
runtime::tests::delivery_retry_transient_blip_emits_failed_event_for_present_workeroccasionally fails under heavy local parallel test load. It does not fail in CI (passes on both main and this branch), passes cleanly when run in isolation, andruntime/tests.rswas last touched three commits before this branch diverged. Not a blocker.🤖 Generated with Claude Code