feat(t3layer): thread lifecycle commands (settle/archive/snooze/meta) - #17
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_cf837d51-2caf-4575-ac67-4fc1f1edee1e) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds seven thread lifecycle operations (archive, unarchive, settle, unsettle, snooze, unsnooze, updateMeta) through the native runtime, facade, and MCP layers. It introduces read-model snapshot support (StockReadModelSnapshot, decodeReadModelSnapshot, getSnapshot HTTP method) for reconciliation, extends thread identity contracts with lifecycle metadata, and adds corresponding tests. ChangesThread lifecycle operations
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServer as t3layer-mcp-server
participant MCP as mcp.ts dispatcher
participant Facade as StockT3Facade
participant Runtime as StockT3Runtime
participant Client as StockT3HttpClient
MCPServer->>MCP: call tool (e.g. archive, snooze, updateMeta)
MCP->>MCP: validate arguments against schema
MCP->>Facade: facade.archive/snooze/updateMeta(threadId, args)
Facade->>Runtime: runControlLifecycleMutation
Runtime->>Runtime: check no-op, validate, dispatch command
alt ambiguous or timed-out dispatch
Runtime->>Client: getSnapshot(minimumSequence)
Client-->>Runtime: StockReadModelSnapshot
Runtime->>Runtime: reconcile projection state
end
Runtime-->>Facade: ControlOperationResult (applied/pending/no-op)
Facade-->>MCP: result
MCP-->>MCPServer: tool response
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 241-244: Update the mutation lifecycle handling around
endLifecycleMutation so terminal restoration is tracked per ref and occurs only
when the final active mutation for that ref completes, rather than after every
mutation. Apply the same coordination to the error restoration path, preserving
the original terminal state across concurrent mutations. Add a test covering
concurrent pending and failure mutations.
In `@src/nativeRuntime.ts`:
- Around line 3151-3158: No code change is required for this PR; retain the full
snapshot read in observeFullThread because archived and deleted threads cannot
currently be observed through narrower endpoints. Track a server-side endpoint
returning a single thread across all states as follow-up work, then migrate
observeFullThread and preflightFullThread once that contract exists.
- Around line 3201-3255: The dispatch-and-retry policy is duplicated between
dispatchControl and dispatchFullLifecycle. Extract the shared
acceptedSequence/retryState, failure classification, ambiguity reconciliation,
stopCode, and identical-retry gate into one helper that accepts a reconciliation
callback returning matched and detail; adapt dispatchControl with
reconcileControlAfterAmbiguous and dispatchFullLifecycle with observeFullThread,
preserving each caller’s predicate handling and early applied-result behavior.
- Around line 3365-3379: Update the validatePreflight callback to treat the
session as active when either preflight.shellThread.session or
preflight.detail.thread.session has status "starting" or "running", while
continuing to read pending approvals, pending user input, and queued turn state
from the shell projection. Match the two-projection session check already used
by stop.
- Around line 3159-3187: Update the ambiguous-error branch in the snapshot-read
loop to call waitBetweenFullSnapshotReads(operation) before continuing, while
preserving mapReceivedError behavior for non-ambiguous errors and the existing
retry bound.
- Around line 3403-3432: Update snoozeUntil and the snooze flow to parse
accepted input into a canonical ISO string once, rather than returning the
original representation. Use that normalized value for the future-date
validation, the dispatched snoozedUntil payload, and the confirmation predicate
in disposition, so server-normalized ISO values compare reliably.
- Around line 3550-3560: Update the updateMeta function to call
validateModelSelectionAvailable for parsed.modelSelection before dispatching the
thread.meta.update operation, while preserving the existing field-shape parsing
and matching behavior. Ensure unavailable model selections are rejected at the
update call just as they are in spawn and resumeCreateReconciliation.
- Around line 162-181: Rename the runtime operation identifier from "updateMeta"
to "update_meta" in ControlOperationName and the LifecycleOperationName Extract
union, and update the disposition call in updateMeta to pass "update_meta".
Preserve the MCP tool name updateMeta unchanged.
In `@src/stockT3Contracts.ts`:
- Around line 221-262: Extract the repeated optional-nullable decoding pattern
into a shared helper near the decoder used by these fields, accepting the input
value, decoder, and path and returning either an empty object, a null-valued
property result, or the decoded value. Update the six fields in the surrounding
stock contract decoder to use this helper while preserving conditional spreads
and omitting keys when values are undefined, including the existing path strings
and field names.
In `@test/facade-lifecycle.test.ts`:
- Around line 811-813: Update the tool registration assertion in the lifecycle
test to use an array-matching assertion with the expected tool names, such as
arrayContaining, instead of wrapping every in a boolean check. Preserve the
existing seven-tool expectation so failures identify any missing registration.
- Around line 544-573: Replace the whole-object StockT3RuntimeClient casts in
test/facade-lifecycle.test.ts:544-573 and test/facade-lifecycle.test.ts:575-605
with explicit StockT3RuntimeClient annotations on each client fixture, and cast
only the getThread member to StockT3RuntimeClient["getThread"] because it may
return undefined. Leave getDescriptor, getSnapshot, getShell, and dispatch
directly typed so their contracts remain checked.
- Around line 340-364: Replace the single looping test around invoke and
runtime.close with a parameterized test generated from cases, matching the
existing block around the parameterized tests at lines 272-338. Keep each
entry’s setup and assertions unchanged while naming each test with entry.method
so failures identify the operation.
- Around line 386-402: Update the “never emits thread.delete” test to assert
that the commands array contains at least one recorded command before checking
command types. Keep the existing every-command assertion so the test still
verifies that none of the dispatched commands has type “thread.delete”.
- Around line 273-283: Introduce a small cleanup helper for runtimes created in
test/facade-lifecycle.test.ts that executes the test body and always calls
runtime.close() in a finally block. Replace the manual close() calls in all
runtime-creating tests with this helper, preserving each test’s existing
assertions and behavior while guaranteeing cleanup when an assertion fails.
In `@test/stock-t3-contracts.test.ts`:
- Around line 162-183: Extend the lifecycle omission test to also decode the
same thread through decodeReadModelSnapshot, then assert archivedAt,
settledOverride, and settledAt are absent from the decoded read-model thread.
Keep the existing decodeThreadDetailSnapshot assertions and input omission setup
unchanged.
- Around line 144-160: Expand the assertions in the “decodes archived threads
from the full read model” test to verify snapshotSequence, the thread count, and
all lifecycle fields on the decoded thread: archivedAt, settledOverride,
settledAt, snoozedUntil, and snoozedAt, along with its id. Ensure the test fails
if the decoder drops any of these values.
🪄 Autofix
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: 809d90d4-09c5-4cf9-82e1-31ac4705647a
📒 Files selected for processing (9)
scripts/t3layer-mcp-server.tssrc/facade.tssrc/mcp.tssrc/nativeRuntime.tssrc/stockT3Contracts.tssrc/stockT3HttpClient.tstest/facade-lifecycle.test.tstest/stock-t3-contracts.test.tstest/stock-t3-http-client.test.ts
📜 Review details
🔇 Additional comments (25)
src/facade.ts (2)
10-10: LGTM!Also applies to: 50-50
415-431: LGTM!src/mcp.ts (1)
8-8: LGTM!Also applies to: 26-32, 185-194, 277-306, 531-552
scripts/t3layer-mcp-server.ts (1)
49-55: LGTM!test/facade-lifecycle.test.ts (12)
1-195: LGTM!
197-269: LGTM!
285-337: LGTM!
366-384: LGTM!
404-424: LGTM!
426-456: LGTM!
458-505: LGTM!
507-518: LGTM!
607-630: LGTM!Also applies to: 632-682, 684-708
710-735: LGTM!Also applies to: 737-765, 767-793
815-841: LGTM!Also applies to: 843-877
520-542: 🗄️ Data Integrity & IntegrationNo change needed.
deletedAtis declared onStockThreadIdentityand decoded intoStockThreadDetail;validateFullThreadIdentityrejects soft-deleted full-snapshot tombstones before dispatch.test/stock-t3-contracts.test.ts (1)
8-8: LGTM!test/stock-t3-http-client.test.ts (1)
39-39: LGTM!Also applies to: 55-62, 85-85, 97-104
src/stockT3Contracts.ts (2)
112-117: LGTM!Also applies to: 156-162
364-391: LGTM!src/stockT3HttpClient.ts (1)
7-12: LGTM!Also applies to: 343-355
src/nativeRuntime.ts (4)
3284-3346: LGTM!
4174-4180: LGTM!
3546-3548: 🎯 Functional CorrectnessNo change needed.
canonicalemits object keys in insertion order by relying onjsonValue, so same key-valueRuntimeModelSelectionvalues serialize to the same canonical string.
3045-3051: 🗄️ Data Integrity & IntegrationClarify that lifecycle commands do not need
createdAt.The lifecycle tests assert the dispatch shape as
type,commandId,threadId, andexpectedPayloadonly, so this omission matches the in-repo tests. If the server requirescreatedAton these command types, this should be added consistently acrossdispatchLifecycle,disposition, anddispatchFullLifecycle.
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_cf2fd3ee-c0e8-4fcd-a623-8cb14879bf6f) |
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_26ffadaf-779b-4341-95ae-a70e61dfe986) |
1 participant
Summary
update_metaoperation identifier while the public method/tool remainsupdateMetaVerification
1.3.11SHA-2561d77af7bfd811aebb7d37bec496a5eed14fe227ded3ab7866d2f39786e8107b60 pass, 24 fail; MCP RED:24 pass, 2 fail; lag regression RED:0 pass, 1 fail479 pass, 2 skip, 0 fail,2,134 assertions, 30 files, 145.02s74 pass, 0 fail,192 assertionsbun run typecheckandgit diff --check: cleanAPPROVED9 passedAccepted residuals
ControlOperationResultwidens the existingno_opunion for the seven lifecycle reasons.pendingafter bounded full-snapshot reads rather than polling to the entire operation deadline./api/orchestration/snapshotis heavier than shell/detail, but it is the only stock HTTP projection containing archived threads; reads are bounded, delayed, deadline-aware, and abort-aware.thread_not_foundidentity refusal.environment_changedtest was added; both preflight paths retain the shipped environment check.