feat(t3layer): phase 6 control operations and review closures - #7
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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_5c23db43-bb55-4f12-bd0d-136373dbf654) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR adds interrupt, stop, approval, and user-input operations to the native runtime and facade. It tracks terminal workers in overlay capacity checks, preserves spawn fields, reports spawn results in errors, and validates policy scopes and deadlines. ChangesRuntime control operations
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 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 |
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 288-303: In src/facade.ts lines 288-303, update interrupt and stop
to reactivate the worker before invoking runtime.interrupt or runtime.stop, and
restore the prior terminal marker if either runtime call throws, following
send’s existing ordering. In src/facade.ts lines 314-320, update observe to
remove reactivation from the read path and call recordWorkerTerminalState only
when isTerminalSnapshot(snapshot) is true.
- Around line 165-172: Update isTerminalSnapshot to also return true when
snapshot.thread.latestTurn?.state is "interrupted", preserving the existing
completed, error, stopped, and session-error terminal conditions.
In `@src/nativeRuntime.ts`:
- Around line 2781-2788: Snapshot response.answers before constructing the
frozen command in respondToUserInput, ensuring the command owns a detached copy
rather than the caller’s mutable object. Preserve the existing command shape and
dispatchControl retry behavior so every retry reuses identical answer data.
- Around line 2734-2736: Update the decision validation in the response-handling
logic to derive its accepted values from the existing ApprovalDecision
declaration instead of duplicating the string literals. Ensure adding a new
ApprovalDecision value automatically updates both TypeScript typing and runtime
validation, while preserving the existing protocol_mismatch error for invalid
decisions.
In `@src/overlay.ts`:
- Around line 312-325: Update the reactivation capacity check in
setTerminalState to include pending.size alongside activeRecordCount() before
allowing a terminal record to become active. Preserve the existing capacity
error and terminal-record state transitions, ensuring reactivation is rejected
whenever active records plus pending reservations reaches maxWorkers.
In `@test/control-operations.test.ts`:
- Around line 691-727: Add a test covering the no-pending-request branches of
respondToApproval and respondToUserInput using a shellThread fixture with
pendingApproval and pendingInput disabled and client projections returning that
thread. Assert approval_not_pending and user_input_not_pending respectively, and
verify dispatches remains zero before closing the runtime.
In `@test/facade.stock-http.test.ts`:
- Around line 1230-1267: Add cleanup to the test “counts only non-terminal
overlay workers against capacity” by awaiting facade.close() after the
assertions. Ensure the real runtime’s adaptive poller and timers are released
before the test completes; leave the following plain-object-double tests
unchanged.
In `@test/policy.test.ts`:
- Around line 416-438: Add a direct test near the existing normalized-scope
capacity test for dispatching with a whitespace-only scopeId such as " ". Assert
that dispatch rejects with TypeError, the callback is not invoked, and the
policy reports zero active and queued work before cleanup.
🪄 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: 3b6730a1-5a7b-4431-a00a-6d105886e829
📒 Files selected for processing (7)
src/facade.tssrc/nativeRuntime.tssrc/overlay.tssrc/policy.tstest/control-operations.test.tstest/facade.stock-http.test.tstest/policy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (20)
src/policy.ts (2)
242-271: LGTM!
337-350: LGTM!test/policy.test.ts (2)
391-414: LGTM!
466-503: LGTM!src/nativeRuntime.ts (7)
146-192: LGTM!Also applies to: 265-266
2253-2404: LGTM!
2406-2525: LGTM!
2527-2609: LGTM!
2611-2680: LGTM!
2682-2726: LGTM!
3119-3122: LGTM!test/control-operations.test.ts (2)
19-136: LGTM!
139-689: LGTM!Also applies to: 729-755
src/overlay.ts (2)
74-74: LGTM!Also applies to: 143-151, 291-298, 370-377
247-249: LGTM!test/facade.stock-http.test.ts (3)
8-8: LGTM!Also applies to: 934-978, 1203-1219
1269-1315: LGTM!Also applies to: 1317-1352, 1373-1381
1367-1372: 📐 Maintainability & Code QualityNo duplicate
observeproperty to remove.The
runtimeobject attest/facade.stock-http.test.ts:1367declaresobserveonce beforewait, with no literal duplicate property to remove.src/facade.ts (2)
3-13: LGTM!Also applies to: 22-38, 56-56, 101-102, 145-145, 154-154, 211-211, 236-236
267-287: LGTM!Also applies to: 304-313
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_b0d26515-329e-49fe-a358-676c8b42d91b) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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_09fbd0ca-850c-4823-98c1-d25435eb2d7e) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Verification
1d77af7bfd811aebb7d37bec496a5eed14fe227ded3ab7866d2f39786e8107b6100 pass,0 fail,308 expect() calls, 5 filestsc --noEmitexit 0320 pass,2 skip,0 fail,1055 expect() calls, 322 tests across 24 filesd3037064Review status
maxWorkers