refactor(runner): move session decisions out of server.ts - #5749
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds normalized facet digests to applied environment state, introduces facet-based reconciliation plans, and centralizes keep-alive session coordination in a lifecycle module. ChangesKeep-alive lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRunRequest
participant runWithKeepalive
participant SessionPool
participant KeepaliveEngine
AgentRunRequest->>runWithKeepalive: dispatch request
runWithKeepalive->>SessionPool: validate and checkout session
SessionPool-->>runWithKeepalive: warm session or pool miss
runWithKeepalive->>KeepaliveEngine: execute continuation or cold run
KeepaliveEngine-->>runWithKeepalive: return turn result
runWithKeepalive->>SessionPool: park, evict, or destroy environment
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
| * else (miss, mismatch, busy, no mount, remote) evicts as needed and runs today's cold path. | ||
| * A validation failure never fails the turn: it degrades to cold. | ||
| */ | ||
| export async function runWithKeepalive( |
There was a problem hiding this comment.
Read this function as a code move, not as a rewrite. The warm gate, the approval resume path, the miss path, the eviction choice, and the re-park policy all moved here unchanged.
The proof that behavior is preserved is external to this file. The keep-alive suites pass unedited, and the characterization suite added in the lane below pins the same decisions from outside. An extraction that also changes behavior cannot be reviewed, so this one changes none.
If you find a decision here that you think is wrong, it was wrong before this PR as well. Say so, and it gets fixed in a lane where the diff shows only that change.
| * The engine seam the keep-alive dispatch drives. The default wires to the real engine; tests | ||
| * inject a fake to exercise the pool/dispatch policy without a live harness. | ||
| */ | ||
| export interface KeepaliveEngine { |
There was a problem hiding this comment.
This interface is the seam the tests need. A fake engine exercises every session decision with no live harness, no sandbox, and no model.
realKeepaliveEngine stays in server.ts on purpose. If the coordinator imported the live engine, the seam would be pointless, because a test importing the coordinator would pull the engine in with it. server.ts is the composition root and does the wiring.
A unit test in this PR asserts that separation directly, so a later import cannot quietly undo it.
| * changed and the plan is a rebuild. That is the honest answer, and it matches what the | ||
| * coordinator does today. | ||
| */ | ||
| export function planReconcile( |
There was a problem hiding this comment.
This function is pure. It reads two values and returns a third. It touches no environment, starts nothing, and cannot fail a turn.
That purity is what makes shadow mode possible without a shadow mode. The router builds a plan on every dispatch and the plan is logged beside the decision the coordinator actually took. Nothing acts on it in this PR. This is Terraform's plan and apply, with apply arriving in a later lane.
Because a plan is a value, a test can assert the whole plan without an environment. The suite in this PR does exactly that, which is why the later lane that acts on plans is reviewable at all.
| // lines is the signal that the router's naming or ownership is still wrong. | ||
| const marker = agree ? "agree" : "DISAGREE"; | ||
| log( | ||
| `shadow key=${input.key} harness=${harnessKind(input.request)} ` + |
There was a problem hiding this comment.
This line carries facet names and action kinds only. It never carries configuration, and it never carries credentials.
That is a constraint on every reason string in this file, not a property of this line alone. The reasons are written to be content free for this purpose.
Expect disagreement markers from the first day. The router answers one question, which is whether the environment needs rebuilding. The coordinator answers a broader one, and some of its reasons are about the conversation rather than the environment. Those are logged with a continuity scope and are not counted, because counting them would produce a permanent false signal that no router work could drive to zero.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
services/runner/tests/unit/lifecycle-session-coordinator.test.ts (1)
135-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving the fake environment fixture to
tests/utils/.
FakeEnv,makeEngine, andmakeCtxhere repeat the fixtures insession-keepalive-dispatch.test.ts,session-keepalive-approval.test.ts, andsession-lifecycle-characterization.test.ts. All four now seed applied state the same way throughappliedStateForRequest(request). A shared factory intests/utils/would keep the next lifecycle-contract change to one edit instead of four.Treat this as optional for this PR. The duplication is pre-existing, and this file only adds one more copy.
As per coding guidelines, "Place runner unit tests under
tests/unit/**/*.test.ts; use Vitest, and keep shared helpers and fixtures undertests/utils/."Source: Coding guidelines
services/runner/src/lifecycle/reconciliation-router.ts (2)
245-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe doc block is stale, and
appliedDigestsFromhas no consumer.The comment states that
AppliedEnvironmentStatecarries one whole-request fingerprint and no per-facet digests. Layer 1 of this stack changed that:applied-state.tsstores facet digests, andsession-coordinator.tsline 279 passesexisting.environment.appliedState.facetsintologReconcileShadow. So the router already receives real applied digests, and the recompute-from-the-acquiring-request fallback described here is not used.Remove
appliedDigestsFromand the stale paragraph, or state why the fallback is retained.
46-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mcpServersis an unreachable capability field.
actionForFacetreadscapabilities.workspace,capabilities.model, andcapabilities.toolCatalogonly. MCP servers live in theharnessSessionfacet, which routes throughcapabilities.model, somcpServerscan never affect a plan. Remove the field, or route a facet to it.services/runner/src/lifecycle/session-coordinator.ts (1)
630-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree dispatch paths emit no shadow line.
The module header states that the router runs on every dispatch. The supersede branch at lines 793-800 and both lost-checkout fall-throughs at lines 630-631 and 792 reach
coldAndParkwithout callingshadowRoute. The logged sample then omits exactly the contended paths, so a DISAGREE burst on them stays invisible.Add a
shadowRoutecall on each path, or narrow the header claim to the paths that log.♻️ Proposed fix
} else if (existing) { klog(`evict (supersede-${existing.state}) key=${key}; cold`); + shadowRoute(existing, "rebuild", `supersede:${existing.state}`); await pool.evict(key, `supersede-${existing.state}`, "failed-turn"); } else {Also applies to: 792-804
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5301565-838f-4465-8d2e-4847102ba282
📒 Files selected for processing (15)
services/runner/src/engines/sandbox_agent/applied-state.tsservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/lifecycle/desired-state.tsservices/runner/src/lifecycle/reconcile-plan.tsservices/runner/src/lifecycle/reconciliation-router.tsservices/runner/src/lifecycle/session-coordinator.tsservices/runner/src/server.tsservices/runner/tests/unit/lifecycle-desired-state.test.tsservices/runner/tests/unit/lifecycle-reconcile-plan.test.tsservices/runner/tests/unit/lifecycle-session-coordinator.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-lifecycle-characterization.test.tsservices/runner/tests/unit/session-pool.test.ts
212e428 to
690283b
Compare
6793e05 to
b6ec133
Compare
690283b to
df8b834
Compare
b6ec133 to
de02f74
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a64ace6-eb1c-44b5-9864-6839c803ebc6
📒 Files selected for processing (15)
services/runner/src/engines/sandbox_agent/applied-state.tsservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/lifecycle/desired-state.tsservices/runner/src/lifecycle/reconcile-plan.tsservices/runner/src/lifecycle/reconciliation-router.tsservices/runner/src/lifecycle/session-coordinator.tsservices/runner/src/server.tsservices/runner/tests/unit/lifecycle-desired-state.test.tsservices/runner/tests/unit/lifecycle-reconcile-plan.test.tsservices/runner/tests/unit/lifecycle-session-coordinator.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-lifecycle-characterization.test.tsservices/runner/tests/unit/session-pool.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- services/runner/tests/unit/session-keepalive-approval.test.ts
- services/runner/src/engines/sandbox_agent/runtime-contracts.ts
- services/runner/tests/unit/session-pool.test.ts
- services/runner/src/engines/sandbox_agent/environment-setup.ts
- services/runner/tests/unit/lifecycle-session-coordinator.test.ts
- services/runner/tests/unit/lifecycle-reconcile-plan.test.ts
- services/runner/tests/unit/lifecycle-desired-state.test.ts
- services/runner/src/lifecycle/reconcile-plan.ts
- services/runner/tests/unit/session-lifecycle-characterization.test.ts
- services/runner/src/engines/sandbox_agent/applied-state.ts
- services/runner/src/server.ts
- services/runner/src/lifecycle/session-coordinator.ts
- services/runner/tests/unit/session-keepalive-dispatch.test.ts
- services/runner/src/lifecycle/desired-state.ts
df8b834 to
d047462
Compare
de02f74 to
1ab8e68
Compare
Railway Preview Environment
Updated at 2026-08-07T09:43:33.743Z |
d047462 to
e02e5e3
Compare
c6b6b4a to
5a9d60b
Compare
…tate doc updated (CodeRabbit on #5749)
5a9d60b to
4536b2a
Compare
…tate doc updated (CodeRabbit on #5749)
4536b2a to
54a7d8d
Compare
cbc8fab to
84a9bb8
Compare
…tate doc updated (CodeRabbit on #5749)
54a7d8d to
29807f8
Compare
84a9bb8 to
24018cb
Compare
…tate doc updated (CodeRabbit on #5749)
29807f8 to
39d989c
Compare
24018cb to
8099b56
Compare
…tate doc updated (CodeRabbit on #5749)
39d989c to
540f3de
Compare
…ion router (slice S6) Pure code motion: the warm gate, approval resume, miss path, eviction, and re-parking move from server.ts into lifecycle/session-coordinator.ts with every keepalive behavior assertion unedited. Beside it, the shadow router: desired-state facets, an explicit ReconcilePlan with apply-live as a first-class action kind (unused in v1), uniform reopen-session capability rows, and side-by-side logging with a disagreement marker. The old decision still rules. Two disagreements documented: transcript mismatches over-rebuild today (router right, fix waits for step 6); credential rotation is invisible to facet digests (router wrong by design until it reads the credential epoch; pinned as a known gap).
…tate doc updated (CodeRabbit on #5749)
8099b56 to
cf6316c
Compare
540f3de to
4768cf9
Compare
…tate doc updated (CodeRabbit on #5749)
refactor(runner): move session decisions out of server.ts
1 participant
Context
Part of the agent-config-editing stack. Targets
agent-config-editing-s1a. Read the stack bottom up.Expect no behavior change from this PR. The warm gate, the approval-resume path, the miss path, the eviction choice, and the re-parking policy move out of
server.tsbyte for byte. The whole keep-alive suite passes unedited, and the characterization tests added in s5 pin the behavior across the move. An extraction that also changes behavior cannot be reviewed, so this one does not.server.tshad grown to hold both transport concerns and every session decision. The later lanes need to change those decisions, and they cannot do that safely while the logic lives inside the HTTP handler with no seam to test it through.Changes
src/lifecycle/session-coordinator.tsnow holdsrunWithKeepaliveand the seams around it:KeepaliveEnginefor tests to inject a fake,KeepaliveContextfor the engine, pool, config, and per-turn accessors, and the two provider and dispatch resolvers.server.tskeeps HTTP, authentication, request decoding, concurrency, and the watchdog. It drops about 740 lines and re-exports the seams the existing tests import, so no import path breaks.realKeepaliveEnginestays inserver.tson purpose. If the coordinator imported the live engine, the seam it exists to provide would be pointless.Beside the extraction, three new pure modules describe reconciliation without performing it:
desired-state.tsnormalizes a request into semantic facets and gives each one a digest. One whole-request hash cannot answer "which part changed", and the later lanes need that answer per facet.reconcile-plan.tsis the plan value: an ordered action list, the most expensive action in it, and the outcome it implies. Building a plan runs nothing and needs no environment, so a plan can be logged and asserted on in a unit test.reconciliation-router.tsbuilds a plan from the request and the environment's applied facets.This is Terraform's plan-then-apply. Shadow mode falls out of it rather than being a special mode: the router builds a plan on every dispatch and logs it beside the decision the coordinator actually took, with an agreement or disagreement marker. The plan decides nothing in this PR.
Tests / notes
[reconcile] shadow key=... decision=... plan=... agree|DISAGREE facets=[...]. It carries facet names and action kinds only, never configuration and never credentials.DISAGREElines from the start. The router answers "does this environment need rebuilding" while the coordinator answers a broader question, and continuity-scoped decisions are logged but not counted for that reason.