Repository navigation
feat(t3layer): phase 5 ephemeral worker hierarchy overlay - #5
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6c81dda2-5ec5-4a20-9a7e-507a70c4028f) |
📝 WalkthroughWalkthroughThe PR adds a process-local worker hierarchy overlay. The facade supports overlay-aware spawning, reconciliation, attachment, lookup, and listing while preserving direct native-runtime behavior for stock-only calls. ChangesWorker overlay lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant StockT3Facade
participant WorkerOverlay
participant T3NativeRuntime
Caller->>StockT3Facade: request overlay-aware spawn
StockT3Facade->>WorkerOverlay: validate parent and reserve identity
StockT3Facade->>T3NativeRuntime: dispatch native spawn without overlay metadata
T3NativeRuntime-->>StockT3Facade: return SpawnResult
StockT3Facade->>WorkerOverlay: commit worker reference
StockT3Facade-->>Caller: return spawn result
Possibly related PRs
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 |
|
@coderabbitai review |
|
@codex 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: 8
🤖 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 `@src/facade.ts`:
- Around line 156-167: Update the spawn flow at src/facade.ts lines 156-167 and
the corresponding pending flow at lines 181-192: wrap each reservation.commit
call so a WorkerOverlayError from post-write validation is rethrown with the
created ref in its details, preserving the stock result reference. In the
pending flow, verify whether reconciledRef can differ from
pending.provisionalRef; if so, reserve with null rather than the provisional ref
to avoid overlay_invalid_ref.
- Around line 61-65: Update StockFacadeSpawnInput so role and parentRef are
represented as an all-or-nothing union: one branch omits both overlay fields for
stock-only behavior, and the other requires both. Then update
requestedOverlayIdentity to use input.role without the unnecessary cast, while
preserving the existing overlay identity behavior.
In `@src/overlay.ts`:
- Around line 308-320: Replace locale-dependent sorting in listChildren and
listWorkers with a shared compareScopedKeys helper defined alongside scopedKey.
Implement the helper using deterministic relational comparison of scoped keys,
and use it in both public listing methods so their ordering is
environment-independent. Apply the changes at src/overlay.ts lines 308-320 and
321-327.
- Around line 210-222: Update materialize to accept a precomputed depth map
rather than calling depths() internally. In listWorkers and listChildren,
compute depths() once per listing operation and pass the resulting map to every
materialize call, preserving the existing unknown-record error behavior.
In `@test/facade.stock-http.test.ts`:
- Line 902: Extend the “facade worker hierarchy overlay” tests with coverage for
both missing branches: create one WorkerOverlay, pass it through
StockT3FacadeOptions.overlay to two facades, and verify the second facade can
read a record written through the shared overlay; also add an attach case whose
attached reference is absent from stock T3 and assert the
overlay_canonical_not_found error.
- Around line 1006-1008: Update the test around the result.kind assertion to
narrow the spawned result with a guard, then pass result.agentRef directly to
facade.getWorker; remove the unreachable pending.provisionalRef ternary
fallback.
- Around line 1031-1036: Extract the existing captureOverlayError helper from
test/overlay.test.ts into a shared test utility, then update the test around
restartedProcess.getWorker(ref) in test/facade.stock-http.test.ts to invoke
getWorker once through that helper and assert the captured error code. Replace
the duplicated toThrow plus bare try/catch pattern, and update
test/overlay.test.ts to import and reuse the shared helper.
In `@test/overlay.test.ts`:
- Around line 145-161: Extend the overlay tests around createWorkerOverlay and
the existing attach/commit/release flows to cover the remaining error branches:
reject roles over MAX_ROLE_BYTES and untrimmed roles, reject negative or
non-integer maxDepth with overlay_invalid_policy, return
overlay_reservation_closed for repeated commit and commit after release, and
return overlay_invalid_ref when committing a ref different from the reserved
ref. Assert each error code explicitly while preserving the existing
duplicate-ref and invalid-role coverage.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 025aa893-8b53-4898-9012-24a9896a798d
📒 Files selected for processing (4)
src/facade.tssrc/overlay.tstest/facade.stock-http.test.tstest/overlay.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (29)
src/overlay.ts (8)
1-60: LGTM!
75-96: LGTM!
98-120: LGTM!
137-150: LGTM!
152-184: LGTM!
186-208: LGTM!
224-287: LGTM!
289-307: LGTM!test/overlay.test.ts (6)
9-17: LGTM!
20-43: LGTM!
45-66: LGTM!
68-96: LGTM!
98-114: LGTM!
116-143: LGTM!src/facade.ts (9)
5-17: LGTM!Also applies to: 46-59
67-69: LGTM!
71-95: LGTM!
97-120: LGTM!
131-136: LGTM!
148-155: LGTM!Also applies to: 170-180
195-218: LGTM!
138-146: 🗄️ Data Integrity & IntegrationNo change needed.
runtime.observe(ref, operation)already rejects mismatchedref.environmentIdwithenvironment_changedbefore returning a thread snapshot, sorequireCanonicalonly needs the thread-id comparison after this call.
122-124: 🩺 Stability & AvailabilityNo change needed.
partialresults use the nativerefafter create receipt projection, while conflicts return acreate_protocol_failurewith aprovisionalRef.test/facade.stock-http.test.ts (6)
18-18: LGTM!Also applies to: 83-89
902-957: LGTM!
959-1005: LGTM!Also applies to: 1009-1013
1015-1030: LGTM!Also applies to: 1037-1038
1040-1066: LGTM!
1068-1103: LGTM!
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_58a337a5-1a6a-4998-91e3-64dffb0d0404) |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
1 participant
Summary
{environmentId, threadId}, with opaque roles, parent refs, creation metadata, cycle/depth/capacity enforcement, and typedoverlay_unknownlossgetWorker,listChildren, andlistWorkers; invalid hierarchy state is rejected before dispatchTest plan
bun test test/overlay.test.ts test/facade.stock-http.test.ts— 45 pass, 0 fail, 150 assertionsbun run typecheck— exit 01d77af7bfd811aebb7d37bec496a5eed14fe227ded3ab7866d2f39786e8107b6git diff --checkReview provenance
Local CodeRabbit review was bounded to 180 seconds and timed out before findings. Red-team and blue-team fallback review found no open HIGH/MEDIUM issue; it added the mutation-proven provisional-create/read-only-resume regression before commit.
Remote CodeRabbit round 1 posted eight findings. Commit
df4f312fixes all eight; every thread was replied to and independently acknowledged as addressed. Incremental re-review completed with no new actionable comments. Codex review was requested on both heads but returned no bot response during the bounded review windows.Note
Medium Risk
Touches the spawn/resume path and ordering (validate before dispatch), but stock-only callers are unchanged and hierarchy failures cannot mutate T3; overlay loss after restart is explicit and tested.
Overview
Adds a process-local worker overlay (
createWorkerOverlay) that tracks role, parent ref, and creation metadata per{environmentId, threadId}, with reservations, depth/capacity limits, cycle and environment checks, and typedWorkerOverlayErrorcodes.createStockT3Facadenow accepts optional overlay options and, when spawn/resume input includes bothroleandparentRef, validates the parent against canonical T3 viaobserve, reserves overlay capacity, runs the unchanged stock spawn path, and commits overlay metadata only after a reconciledspawned/partialresult—provisional creates stay invisible togetWorker. New facade methods:attach,getWorker,listChildren,listWorkers. Omitting overlay fields preserves prior stock-only behavior; overlay state is not persisted and is lost on restart (explicit re-attachrequired).Overlay types and constants are re-exported from the facade public surface.
Reviewed by Cursor Bugbot for commit df4f312. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Note
Add ephemeral worker hierarchy overlay to the T3 facade layer
createWorkerOverlayin src/overlay.ts) that tracks parent/child relationships between workers with bounded depth and capacity, cycle detection, cross-environment rejection, and a reservation lifecycle.createStockT3Facadein src/facade.ts to accept an optional overlay, validaterole/parentReffields on spawn inputs, reserve overlay slots before dispatch, commit them on reconciliation, and expose new methods:attach,getWorker,listChildren,listWorkers.Macroscope summarized df4f312.
🤖 Generated with Claude Code