feat(agentbox): record who authorized a run, not just who it ran as - #1700
Conversation
Closes #1699. RunRecord shipped in #1672 with no actor. #1678 then landed Actor + DelegationChain on audit rows, so the platform can attribute an API call to a human — but a detached agent run holding a credential recorded nobody. The system could answer "who called this API?" and not "who started the agent that has been running on this box for six hours?" Adds Actor + DelegationChain to RunRecord, threaded through both transports (process_start's MCP args and SpawnRequest's new fields), surfaced by process_list, and carried in the run's on-disk record so a reconnecting client sees it. These fields are CALLER-ASSERTED, and are documented that way everywhere they appear. The issue's own acceptance criteria said to populate them "from the authenticated context, never from caller input" — that is not implementable here: agent-box has no authenticated context on either transport. It is reached over SSH (authenticated by the SSH session) or a resident unix socket (by filesystem permissions); SpawnServer.Spawn does not even take its context. So the honest contract is provenance at the same trust level as the command string beside it, explicitly weaker than #1678's server-resolved audit column. Recording that distinction matters more than the field: two things both called "actor", one verified and one not, will otherwise be read as equivalent evidence. An empty actor stays empty rather than defaulting to the box user — fabricating attribution nobody supplied is worse than recording none. RunRecordVersion 1 -> 2. readRunRecord previously rejected any version != current, so the bump alone would have made every in-flight run unreadable at upgrade time — including the "unknown"-outcome records that are the only evidence of runs that died unresolved, which is precisely what #1672 exists to preserve. It now accepts a RANGE: v1 records load with empty actor fields and report as unattributed, while a newer-than-supported record is still rejected rather than guessed at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds caller-asserted actor and delegation-chain fields to process spawning, persists them in version 2 run records, preserves version 1 readability, and exposes asserted actors in process listings. ChangesRun provenance attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds caller-asserted run attribution across spawn paths, persistence, and process listings while retaining v1 record compatibility and rejecting unsupported future versions. No merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant SpawnRequest
participant process_start
participant spawnBackgroundProcess
participant RunRecord
participant process_list
SpawnRequest->>process_start: actor and delegation_chain
process_start->>spawnBackgroundProcess: provenance values
spawnBackgroundProcess->>RunRecord: persist provenance
process_list->>RunRecord: read run record
RunRecord-->>process_list: asserted actor
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements actor persistence, transport propagation, process listing, and v1 compatibility. However, it uses caller-asserted provenance instead of authenticated context, and the reviewed changes do not add run start and exit audit events carrying actor data. These requirements are explicit in issue Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Closes #1699.
The gap
RunRecordshipped in #1672 with no actor. #1678 then landedActor+DelegationChainon audit rows — so the platform can attribute an API callto a human, but a detached agent run holding a credential recorded nobody.
The system could answer "who called this API?" and not "who started the agent
that has been running on this box for six hours?" — which is the asymmetry epic
#1680 exists to close, in the one place the work was already underway.
The correction this PR makes to its own issue
#1699's acceptance criteria said to populate the field "from the authenticated
context, never from caller-supplied input." That is not implementable here.
agent-box has no authenticated context on either transport:
and
SpawnServer.Spawndoes not even take its context (_ context.Context)So the honest contract is caller-asserted provenance, at the same trust level
as the
commandstring beside it, and explicitly weaker than #1678'sserver-resolved audit column. That is documented at every point it appears — the
struct field, the proto field, the MCP tool description, and
process_list'soutput, which renders
Actor: alice@example.com (asserted).Recording that distinction matters more than the field itself: two things both
called "actor", one cryptographically derived and one not, will otherwise be read
as equivalent evidence by whoever inherits this.
An empty actor stays empty rather than defaulting to the box user —
fabricating attribution nobody supplied is worse than recording none.
The upgrade hazard
RunRecordVersion1 → 2.readRunRecordpreviously rejected any version !=current:
So the bump alone would have made every in-flight run unreadable at upgrade
time — including the
unknown-outcome records that are the only evidence of runsthat died unresolved, which is exactly what #1672 exists to preserve.
It now accepts a range: v1 records load with empty actor fields and report as
unattributed; a newer-than-supported record is still rejected rather than guessed
at. Both directions are tested, and the range check was confirmed to fail the
test when neutered.
Tests
v1-reads-after-the-bump, future-version-rejected, actor round-trip, and
empty-actor-is-not-fabricated. Pre-existing on this branch and unrelated: two
TestStartSpawnListener_*failures on macOS only (socket path exceeds the104-char
sun_pathlimit; added by #1517, green on Linux CI).Generated
.pb.gw.gochurn dropped per the repo convention; only.pb.goandthe
.protoare committed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Compatibility