[PARKED] feat(agents): project scope rides runContext; mount becomes the pool-key fallback#5180
[PARKED] feat(agents): project scope rides runContext; mount becomes the pool-key fallback#5180mmabrouk wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a project identity field to run-context DTOs, wire models, and protocol interfaces across the Python SDK and runner service. It threads ChangesProject-scoped keep-alive pooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as server.ts (runWithKeepalive)
participant MountResolver as resolveKeepaliveMount
participant SessionPool as session-pool.ts (poolKeyFor)
participant Engine as KeepaliveEngine
Client->>Server: run request with runContext.project.id
Server->>MountResolver: resolveKeepaliveMount()
MountResolver-->>Server: signed (mount or null/undefined)
Server->>SessionPool: poolKeyFor(request, signed?.projectId)
SessionPool->>SessionPool: prefer runContext.project.id, else mountProjectId
SessionPool-->>Server: PoolScope{key, source} or null
alt PoolScope resolved
Server->>Engine: acquireEnvironment(session, signal, signed)
Engine-->>Server: environment
Server->>SessionPool: park session under PoolScope.key
else no scope available
Server->>Engine: run cold (coldAndPark)
end
Server-->>Client: run result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
| :class:`~agenta.sdk.agents.connections.models.RuntimeAuthContext.project_id`). The runner uses | ||
| it as the preferred project scope for session keep-alive. Best-effort: no baggage / no | ||
| ``project_id`` returns ``None`` and the field is simply omitted.""" | ||
| baggage = TracingContext.get().baggage or {} |
There was a problem hiding this comment.
The stamp seam. The project id is read from the SERVER-derived request state (the authenticated OTel baggage on TracingContext, the same source the auth middleware scopes its permission check on), never from anything the caller sends. Stamping it here keeps the trust boundary at the service; the runner then trusts it because the service is trusted.
| } | ||
| if run: | ||
| out["run"] = run | ||
| if self.project is not None: |
There was a problem hiding this comment.
Omit-when-empty, matching the run/workflow/trace blocks: an unset or empty project serializes to nothing and to_wire drops the key. This is what keeps a run with no stamped project byte-identical on the wire to before this change.
| if (!sessionId || !project) return null; | ||
| return `${project}:${sessionId}`; | ||
| if (!sessionId) return null; | ||
| const runContextProject = request.runContext?.project?.id?.trim(); |
There was a problem hiding this comment.
The preference order: the service-stamped runContext.project.id wins over the mount-derived scope. This is the whole point of the change — the trustworthy id no longer depends on a durable mount existing.
| } | ||
| const mount = mountProjectId?.trim(); | ||
| if (mount) return { key: `${mount}:${sessionId}`, source: "mount" }; | ||
| return null; |
There was a problem hiding this comment.
The fallback + no-park rule. After trying runContext then the mount, if NEITHER yields a scope we return null and the dispatch never parks (there is no safe key that separates callers). This safety invariant is unchanged from before.
| assert.equal(req.runContext!.run!.kind, "test"); | ||
| // The server-stamped owning project scope (F5). The runner prefers `runContext.project.id` | ||
| // over the mount-derived scope when keying its keep-alive pool (see `poolKeyFor`). | ||
| assert.equal(req.runContext!.project!.id, "proj_abc"); |
There was a problem hiding this comment.
Contract-test pinning (TS consumer side). This plus the Python producer assertion and the shared golden pin the new runContext.project field across both languages, so the two hand-mirrored sides cannot drift silently.
| "run": {"kind": "test"}, | ||
| # The server-stamped project scope (F5), snake_case inner key `project.id` — the | ||
| # `$ctx.project.id` binding namespace and the runner's preferred keep-alive pool scope. | ||
| "project": {"id": "proj_abc"}, |
There was a problem hiding this comment.
Contract-test pinning (Python producer side), asserted against the same golden the TS test loads. snake_case project.id is deliberate: it is the $ctx.project.id binding namespace, matching the other runContext inner keys.
| * only project scope the runner can trust for this request (the /run wire carries no project | ||
| * id today), so session keep-alive keys its pool on `<projectId>:<sessionId>`. Absent when the | ||
| * response omitted the mount object; keep-alive then refuses to park (no safe key source). | ||
| * FALLBACK project scope for session keep-alive: the pool prefers the service-stamped |
There was a problem hiding this comment.
Audit-driven doc update. The mount audit (Mahmoud's ask) found the mount-sign response was overloaded: beyond durable storage (bucket/prefix/creds) and the legitimate credential-expiry it folds into the credential epoch, its project_id was doing IDENTITY duty as the pool-key scope. That was the one overload; this change demotes it to the fallback and moves the trustworthy identity to runContext. Every other mount consumer is a storage or credential concern.
mmabrouk
changed the title
|
Parked on Mahmoud's call: JP is moving warm sessions to the backend, and this slice changes the pool keying that work touches. Keep as draft; do not merge until the landing zone is coordinated with JP. The F2 structural cleanup of sandbox_agent.ts is held entirely for the same reason. |
mmabrouk
force-pushed
the
feat/keepalive-project-scope
branch
from
d5a8aa9 to
1b949d9
Compare
|
Review round 1 (independent reviewer): FAIL with one blocking bug and one accuracy nit. Both are fixed and amended into the F5 commit (now 1b949d9). B1 (blocking): parking without a mount crashed the turn. resolveKeepaliveMount can legitimately return null (store unconfigured, 503). Before this PR that also meant no scope, so the dispatch bailed to cold. With the run-context scope, control passed the guard and dereferenced the null mount ( S1 (nit): the docstrings claimed the project id is "never caller-supplied". Textually the id arrives on the caller's W3C baggage header. What makes it trustworthy is authorization gating: the auth middleware scopes its permissions check on that same project_id and denies any request whose credential is not authorized for it, so a forged id cannot cross tenants. The docstrings in tracing.py, dtos.py, wire_models.py, and protocol.ts now say exactly that, so nobody weakens the auth check unaware it backstops the pool key. Tests: runner 743 passed (same 2 known qa-transcript-replay fixture failures, unrelated), typecheck clean. Python 598 passed (same 2 known pre-existing failures, unrelated), ruff clean. |
| // restart, never a failed turn. | ||
| const cfgFp = configFingerprint(request); | ||
| const incomingEpoch = computeCredentialEpoch(request, mountCreds.expiresAt); | ||
| const incomingEpoch = computeCredentialEpoch(request, signed?.expiresAt); |
There was a problem hiding this comment.
The B1 null-handling seam. With the run-context scope, a request can reach this point with no mount at all (store unconfigured, 503, or the sign threw). Parking mount-less must not fail the turn: the keep-alive invariant is that any gap here may only cost a cold restart. So the epoch takes signed?.expiresAt (no mount means no mount-expiry eviction, TTL and rotation-hash still bound the park), and the acquire receives the sign result verbatim, preserving the null vs undefined distinction (do-not-re-sign vs retry-the-sign). The dispatch regression test pins the null-sign + run-context-scope combination.
mmabrouk
changed the base branch from
fix/keepalive-approval-ttl-default
to
big-agents
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad50b7ca-f6f9-4514-98aa-6afab12a5275
📒 Files selected for processing (16)
docs/design/agent-workflows/documentation/running-the-agent.mdsdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/tracing.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonsdks/python/oss/tests/pytest/unit/agents/test_tracing.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pyservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/wire-contract.test.ts
| /** | ||
| * `presignedMount` follows the same convention as `runCold`: a value threads the up-front | ||
| * sign in, null = signed with no mount (do not re-sign, run mount-less), undefined = the | ||
| * up-front sign attempt threw (the acquire retries the sign itself). | ||
| */ | ||
| acquireEnvironment( | ||
| request: AgentRunRequest, | ||
| signal: AbortSignal | undefined, | ||
| presignedMount: MountCredentials | null, | ||
| presignedMount: MountCredentials | null | undefined, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment on resolveKeepaliveMount at line 227.
The adjacent interface comment says Null = no mount = never park, but the new behavior (and the updated resolveKeepaliveMount JSDoc in sandbox_agent.ts lines 480–482) is that null no longer forces a cold run by itself — parking proceeds when runContext.project.id supplies a scope. This comment now contradicts the implementation and the sibling documentation.
📝 Proposed fix for stale comment
- /** Sign the session's durable mount once, up front. Null = no mount = never park. */
+ /** Sign the session's durable mount once, up front. Null = no mount; parking still
+ * proceeds when `runContext.project.id` supplies a scope (see `poolKeyFor`). */
…er it over the mount Decision 1 option (b): the runner's keep-alive pool scopes sessions by project id. Until now that scope came only from the mount-sign response, so a run without a durable mount could never park. The trustworthy project id now rides runContext: the service stamps it server-side from the request's authenticated baggage (never from a caller-supplied wire field), and the runner PREFERS runContext.project.id, falling back to the mount scope. The no-scope-no-park safety rule is unchanged. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
mmabrouk
force-pushed
the
feat/keepalive-project-scope
branch
from
1b949d9 to
1c583c2
Compare
1 participant
PARKED, do not merge: pending coordination with JP's warm-sessions-to-backend work. This slice touches the session-pool keying and the sandbox_agent engine that JP is relocating. The code is complete, tested, and reviewed as a draft; re-evaluate the landing zone once JP's design settles.
The runner keeps a live agent session parked between turns so the next message in the same
conversation continues the same harness process. Parked sessions are pooled by project scope,
so a session can never be resumed across a project boundary. Decision 1 of the keep-alive
design chose the mount-sign response as the interim source of that scope, because at the time
no trustworthy project id rode the
/runwire. The recorded cleanup (Decision 1 option b) wasto stamp a server-derived project id into
runContextand prefer it. This PR is that cleanup.What changed
runContext.project.id. It readsthe id from its own request state (the authenticated OTel baggage, the same source the auth
middleware scopes its permission check on), never from a caller-supplied field. This is the
same "from the request state, never the request body" discipline
RuntimeAuthContext.project_iddocuments.
runContext.projectsub-object. Added deliberately across everymirror: the golden fixture,
protocol.ts,dtos.py+wire.pysto_wire, the wire-modelschema, and BOTH contract tests (Python
test_wire_contract.pyand TSwire-contract.test.ts).runContext.project.idand FALLS BACK to the mount-derivedscope. If neither source yields a scope, the session never parks (unchanged safety rule).
scope=run-contextvsscope=mount.the preference order, and the agent-workflows runtime doc explains where the scope comes from.
Scope and risk
byte-identical to before. Old requests keep working; the runner just uses the mount fallback.
source still runs fully cold and is never pooled.
projectIdwire field (which theplayground never populates and the runner never reads) is left untouched by design.
How to QA
cd services/runner && pnpm test && pnpm typecheck. NewpoolKeyForcases coverrun-context-wins, mount-fallback, and neither-source-never-park. (Two pre-existing failures in
sandbox-agent-qa-transcript-replay.test.tsare unrelated fixture drift.)cd sdks/python && uv run pytest oss/tests/pytest/unit/agents/ -q. Newtest_tracing.pycases cover the server-baggage stamp and the project failure domain; the contract test + golden
pin the wire field. (Two pre-existing failures in
test_capabilities.py/test_dtos_agent_template.pyare unrelated permission-default drift.)grep 'scope='showsscope=run-contextwhen the stamp is present and
scope=mounton the fallback path.The Decision 1 / follow-up 5 status rows in the session-keepalive design workspace land after
#5153 merges (that folder only exists on the
docs/session-keepalive-planlane, not in the base).https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv