feat: apply source controls in remote sessions - #1828
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
📝 Walkthrough🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Code reviewConfidence: 2/7 — Significant Concerns Found 3 issues:
📐 Plan Adherence — 3 issuesThe observer migration does not preserve terminal not-found handling or bounded cleanup after failed terminal writes. Folded-claim restart handling also stops at the delivery boundary rather than covering the full lifetime of the running turn.🔍 Bugs — 3 issuesMissing-claim execution, indefinitely renewed stranded claims, and stale folded input reaching a running turn.🔁 Data Flow — 3 issuesClaim authority is not propagated from the observer's terminal state; observation ownership outlives abandoned terminal paths; folded dependencies are not retained through completion.📋 CLAUDE.md Compliance — CLEANNo clear invariant violation survived review.🏗️ Design — 1 concernThe observer handle terminalizes authoritative 404s internally but exposes no terminal state or callback that RemoteSession can use to revoke local authority.🔒 Security — CLEANNo high-confidence vulnerability found.🎛️ UX — NOT RUNNo frontend files changed.📱 Mobile — NOT RUNNo frontend files changed.🤖 Generated with unified-review automation |
95895d1 to
8023512
Compare
|
Addressed all three unified-review findings:
Added deterministic claim-loss, failed folded-close, and delivery→blocked-close→folded-update race tests. Verification:
|
4a9be04 to
d171b77
Compare
0a47d64 to
fc76916
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
extensions/remote-session/src/session.ts (1)
1122-1144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
partsaccumulation in the fold loop.Line 1144 rebuilds the content from
parts[0]plusliveFolded. Theparts.push(formatInvocationContent(extra))at Line 1127 is therefore never read. It also duplicates the formatting work that Line 1144 repeats.Replace
partswith a singleprimarystring so the canonical-content rule stays in one place.♻️ Proposed change
- folded.push(extra) - // formatInvocationContent, not buildTurnContent: the latter downloads - // attachments, and nothing claimed during the sweep is renewed yet, so - // a slow queue would expire every claim it is holding. The steer sweep - // folds prompt text only for the same reason. - parts.push(formatInvocationContent(extra)) + folded.push(extra)Change Line 1100 to
const primary = await this.buildTurnContent(invocation)and Line 1144 to useprimary. Keep theformatInvocationContentrationale comment next to the Line 1144 mapping, because that is where the attachment-free formatting now happens.🤖 Prompt for 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. In `@extensions/remote-session/src/session.ts` around lines 1122 - 1144, In the fold loop around the invocation content construction, replace the unused parts accumulation with a single primary string from buildTurnContent(invocation). Update buildSteerContent to use primary plus the liveFolded mapping, remove the redundant parts.push formatting, and keep the formatInvocationContent rationale comment beside that mapping.
🤖 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 `@extensions/remote-session/src/session.test.ts`:
- Line 2451: Update the fetch spies so each invocation creates a fresh Response
instead of reusing a consumed body: change
extensions/remote-session/src/session.test.ts:2451 to use a mock implementation
returning a new “ok” Response, and
extensions/remote-session/src/session.sealed.test.ts:365 and :456 to return new
Responses from encrypted.ciphertext and encryptedAttachment.ciphertext
respectively.
In `@extensions/remote-session/src/session.ts`:
- Around line 1330-1353: Update extensions/remote-session/src/session.ts lines
1330-1353 in the completion flow to remove the post-write currency checks after
complete/completeSealed, and release the observation after the successful
terminal write; retain the pre-write checks. In the same file, lines 1685-1709,
update the flow after complete returns to call releaseObservation(invocation.id)
before returning false.
- Around line 1056-1065: Extract the shared client.fail call and error-message
scrubbing into one private writer, then update failFencedInvocation and
failInvocation to delegate to it. Preserve the sealed-turn rule that always uses
“Sealed turn failed,” the 1000-character limit for unsealed messages, and the
existing failure-write logging behavior.
- Around line 2007-2012: Update the stale-source branch in the request handling
flow to call failContributors for the removed entry before returning, matching
the existing 409 path. Preserve the current cleanup, presence synchronization,
and closed-request response while ensuring contributor observations and claims
are resolved immediately.
- Around line 1494-1505: Extract the shared swept-item-to-content mapping from
steerRunningTurn and the corresponding steerByInterrupt and sweepQueuedForSteer
flows into one helper, ensuring non-steer control commands and empty prompt text
are filtered out. Replace all three duplicated mappings with the helper so
steerRunningTurn reaches the “Nothing to steer with” acknowledgement when no
usable content remains.
---
Outside diff comments:
In `@extensions/remote-session/src/session.ts`:
- Around line 1122-1144: In the fold loop around the invocation content
construction, replace the unused parts accumulation with a single primary string
from buildTurnContent(invocation). Update buildSteerContent to use primary plus
the liveFolded mapping, remove the redundant parts.push formatting, and keep the
formatInvocationContent rationale comment beside that mapping.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 658efe01-8f85-4143-9f1f-d9991cbbcf6f
📒 Files selected for processing (9)
extensions/claude-code-remote/src/channel-server.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/claude-code-remote/src/session-control.test.tsextensions/remote-session/src/attachments.tsextensions/remote-session/src/client.tsextensions/remote-session/src/index.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Frontend Tests (2/4)
- GitHub Check: Frontend Tests (3/4)
- GitHub Check: Frontend Tests (1/4)
- GitHub Check: Frontend Tests (4/4)
- GitHub Check: Tests
- GitHub Check: Typecheck
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names that clearly indicate purpose and avoid single letters except for loop counters
Add JSDoc comments for all public functions and exported classes to document purpose, parameters, return types, and usage examples
Use const by default, let when variable reassignment is needed, avoid var
Use environment variables for configuration instead of hardcoded values
Format code with Prettier and lint with ESLint according to project configuration
Files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use TypeScript interfaces for type definitions and avoid type assertions when possible
**/*.{ts,tsx}: Do not use language-specific heuristics or English-only literals and regexes for semantic decisions; use model-based decisions for language-dependent behavior (INV-54).
Keep comments absent by default; retain only durable explanations of ordering, concurrency, non-obvious constraints, or load-bearing values. Do not add speculative TODOs or change narration (INV-25, INV-36).
Limit nested ternaries to one level and colocate variant configuration while keeping shared behavior on one path (INV-29, INV-43, INV-47).
Files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Bun commands and runtime (
bun <file>,bun run test,bun install,bun build), not Node or dotenv.
Files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
**
⚙️ CodeRabbit configuration file
**: Architecture, invariants, and the full app inventory live in CLAUDE.md and
docs/system-overview.md. Treat those as the source of truth.What NOT to flag:
- Pre-existing issues not introduced by this PR
- Issues that TypeScript compilation or ESLint would catch (types, imports, lint)
- Stylistic preferences without a concrete rule violation in CLAUDE.md
- Theoretical risks or hypothetical edge cases without evidence of exploitability
- General best-practice suggestions that don't map to a specific project rule
Security calibration:
- React JSX is safe from XSS unless dangerouslySetInnerHTML is used
- ULIDs/UUIDs are cryptographically unguessable — do not flag as enumeration risks
- Environment variables are trusted — do not flag as hardcoded secrets
- DoS, rate limiting, log spoofing, regex complexity, missing audit logs, and
outdated dependency warnings are out of scopePlan adherence:
The implementation plan is in the PR description, inside the collapsible
"📋 Full implementation plan" details block (plans are not committed to the repo).
If that block exists, check that PR changes align with the plan.
Flag missing corresponding changes: API change without frontend/backoffice update,
type change without usage update, schema change without migration.
Files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write unit tests for all utility functions and business logic with at least 80% code coverage
Use descriptive test names that explain what is being tested and expected outcome
Files:
extensions/claude-code-remote/src/channel-server.test.tsextensions/claude-code-remote/src/session-control.test.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{ts,tsx}: Never ship unexecuted tests,.skip(), or.todo(); fix failing tests rather than dismissing them as pre-existing (INV-22, INV-26).
Assert specific event presence and content rather than counts; prefer one object comparison over chains of narrow assertions (INV-23, INV-24).
Do not mock shared modules withmock.module()orvi.mock(); use scopedspyOnagainst namespace imports. Frontend integration tests mount real components and test observable behavior (INV-39, INV-48).
Files:
extensions/claude-code-remote/src/channel-server.test.tsextensions/claude-code-remote/src/session-control.test.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.ts
🧠 Learnings (5)
📚 Learning: 2026-05-12T07:32:39.480Z
Learnt from: kristofferremback
Repo: threahq/threa PR: 500
File: apps/backend/src/features/agents/tools/linear/issues.ts:59-91
Timestamp: 2026-05-12T07:32:39.480Z
Learning: In the threahq/threa repository, do not recommend adding JSDoc comments for exported/public functions during code review. Treat such documentation suggestions as unnecessary unless there is a concrete, enforceable requirement—e.g., a specific lint rule or a documented convention explicitly requiring JSDoc for public/exported APIs. Only raise documentation concerns when that requirement can be verified from the repo’s tooling or docs.
Applied to files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
📚 Learning: 2026-05-12T07:33:44.564Z
Learnt from: kristofferremback
Repo: threahq/threa PR: 500
File: apps/backend/src/features/agents/tools/linear/format.ts:1-31
Timestamp: 2026-05-12T07:33:44.564Z
Learning: In the threahq/threa repository, do not raise any code review comments (of any severity) about missing, insufficient, or requested JSDoc/documentation comments. This applies to all exported/public TypeScript entities (e.g., exported functions, classes, interfaces, constants) across the repo—missing documentation should never be flagged as a review issue under any circumstances.
Applied to files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
📚 Learning: 2026-05-12T07:33:11.118Z
Learnt from: kristofferremback
Repo: threahq/threa PR: 500
File: apps/backend/src/features/agents/tools/linear/projects.ts:45-76
Timestamp: 2026-05-12T07:33:11.118Z
Learning: In the threahq/threa repository, do not flag missing JSDoc on exported functions during code review. The team does not require JSDoc for exported APIs and considers it review noise; only raise review issues for other concerns (e.g., correctness, types, tests, security), not for absent JSDoc comments on exports.
Applied to files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
📚 Learning: 2026-06-12T14:06:33.911Z
Learnt from: kristofferremback
Repo: threahq/threa PR: 882
File: packages/agent-runtime/src/runtime/negotiate-capabilities.ts:4-15
Timestamp: 2026-06-12T14:06:33.911Z
Learning: In this repo, code comments may include invariant/finding anchors (e.g., E2EE-11, INV-E1, E2EE-9, C-1, Phase 2.4) as stable references that map to docs/audits and docs/plans, following the documented convention in CLAUDE.md to “reference it in nearby code comments if the constraint is non-obvious.” When you see such anchor references, do not treat them as violations of INV-25 or INV-36. Also preserve “rollout-phase” notes that describe *current* tolerated behavior (e.g., “absent token tolerated today”)—these are load-bearing context for reviewers and should not be removed as if they were generic change-history narration.
Applied to files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
📚 Learning: 2026-05-12T07:33:41.940Z
Learnt from: kristofferremback
Repo: threahq/threa PR: 500
File: apps/backend/src/features/agents/tools/linear/deps.ts:3-10
Timestamp: 2026-05-12T07:33:41.940Z
Learning: In the threahq/threa repository, do not suggest adding JSDoc comments anywhere during code review (not for interfaces, exported functions, types, or other constructs). Treat missing JSDoc as intentional and acceptable; avoid any review comments recommending JSDoc additions across the codebase.
Applied to files:
extensions/remote-session/src/index.tsextensions/claude-code-remote/src/channel-server.test.tsextensions/remote-session/src/attachments.tsextensions/claude-code-remote/src/session-control.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/remote-session/src/client.tsextensions/remote-session/src/session.sealed.test.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.ts
🔇 Additional comments (20)
extensions/claude-code-remote/src/channel-server.test.ts (1)
92-100: LGTM!Also applies to: 322-322
extensions/claude-code-remote/src/session-control.test.ts (1)
2-2: LGTM!Also applies to: 27-32
extensions/claude-code-remote/src/channel-server.ts (1)
315-318: LGTM!Also applies to: 363-363
extensions/remote-session/src/session.ts (12)
21-23: LGTM!Also applies to: 249-257, 289-300, 323-328, 379-385
570-586: LGTM!Also applies to: 2227-2249
819-861: LGTM!
863-912: LGTM!
914-976: LGTM!
978-1054: LGTM!
720-750: LGTM!Also applies to: 1068-1082, 1099-1121, 1164-1194
1197-1270: LGTM!
1280-1329: LGTM!
1358-1385: LGTM!Also applies to: 1449-1493, 1506-1529, 1538-1589, 1601-1625
1657-1659: LGTM!Also applies to: 1733-1779, 1787-1813, 1816-1869
1882-1968: LGTM!Also applies to: 1980-2006, 2013-2056, 2071-2104, 2118-2131, 2152-2167, 2376-2376
extensions/remote-session/src/client.ts (1)
57-57: LGTM!Also applies to: 107-109, 127-127, 187-192, 241-249
extensions/remote-session/src/attachments.ts (1)
109-123: LGTM!Also applies to: 141-168, 329-359
extensions/remote-session/src/index.ts (1)
7-7: LGTM!extensions/remote-session/src/session.test.ts (1)
1-14: LGTM!Also applies to: 25-25, 82-163, 183-189, 199-199, 215-237, 967-974, 1013-1020, 1257-1393, 1576-1589, 1737-1739, 1998-2065, 2091-2106, 2216-2450, 2452-3061
extensions/remote-session/src/session.sealed.test.ts (1)
20-22: LGTM!Also applies to: 152-212, 249-249, 353-364, 366-455, 457-525
| if (id === "att_bad") throw new Error("per-file URL failure") | ||
| return `https://signed.example/${id}` | ||
| } | ||
| const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue(new Response("ok")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mockResolvedValue(new Response(...)) shares one body across all fetch calls. A Response body can be read only once. Each fixture downloads exactly one attachment today, so the tests pass; a second attachment makes them fail with "Body already used" instead of the behavior under test.
extensions/remote-session/src/session.test.ts#L2451: replace withmockImplementation(async () => new Response("ok")), matching Line 2375.extensions/remote-session/src/session.sealed.test.ts#L365: replace withmockImplementation(async () => new Response(encrypted.ciphertext)).extensions/remote-session/src/session.sealed.test.ts#L456: replace withmockImplementation(async () => new Response(encryptedAttachment.ciphertext)).
📍 Affects 2 files
extensions/remote-session/src/session.test.ts#L2451-L2451(this comment)extensions/remote-session/src/session.sealed.test.ts#L365-L365extensions/remote-session/src/session.sealed.test.ts#L456-L456
🤖 Prompt for 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.
In `@extensions/remote-session/src/session.test.ts` at line 2451, Update the fetch
spies so each invocation creates a fresh Response instead of reusing a consumed
body: change extensions/remote-session/src/session.test.ts:2451 to use a mock
implementation returning a new “ok” Response, and
extensions/remote-session/src/session.sealed.test.ts:365 and :456 to return new
Responses from encrypted.ciphertext and encryptedAttachment.ciphertext
respectively.
| private async failFencedInvocation(invocation: ClaimedInvocation, errorMessage: string): Promise<void> { | ||
| const scrubbed = invocation.sealing ? "Sealed turn failed" : errorMessage.slice(0, 1000) | ||
| await this.client | ||
| .fail(invocation.id, { | ||
| instanceId: this.config.instanceId, | ||
| claimToken: invocation.claimToken, | ||
| errorMessage: scrubbed, | ||
| }) | ||
| .catch((error) => this.log(`invocation fail write failed: ${this.summarize(error)}`)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the shared /fail write to remove the duplicated scrubbing rule.
failFencedInvocation and failInvocation (Lines 1761-1779) repeat the same scrub rule and the same client.fail body. The scrub rule is security-relevant: a sealed turn must never echo decrypted content. If one copy changes later, the other keeps the old behavior.
Keep the two entry points, but route both through one private writer.
♻️ Proposed refactor
+ private async writeFail(invocation: ClaimedInvocation, errorMessage: string): Promise<boolean> {
+ const scrubbed = invocation.sealing ? "Sealed turn failed" : errorMessage.slice(0, 1000)
+ try {
+ await this.client.fail(invocation.id, {
+ instanceId: this.config.instanceId,
+ claimToken: invocation.claimToken,
+ errorMessage: scrubbed,
+ })
+ return true
+ } catch (error) {
+ this.log(`invocation fail write failed: ${this.summarize(error)}`)
+ return false
+ }
+ }
+
private async failFencedInvocation(invocation: ClaimedInvocation, errorMessage: string): Promise<void> {
- const scrubbed = invocation.sealing ? "Sealed turn failed" : errorMessage.slice(0, 1000)
- await this.client
- .fail(invocation.id, {
- instanceId: this.config.instanceId,
- claimToken: invocation.claimToken,
- errorMessage: scrubbed,
- })
- .catch((error) => this.log(`invocation fail write failed: ${this.summarize(error)}`))
+ await this.writeFail(invocation, errorMessage)
}🤖 Prompt for 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.
In `@extensions/remote-session/src/session.ts` around lines 1056 - 1065, Extract
the shared client.fail call and error-message scrubbing into one private writer,
then update failFencedInvocation and failInvocation to delegate to it. Preserve
the sealed-turn rule that always uses “Sealed turn failed,” the 1000-character
limit for unsealed messages, and the existing failure-write logging behavior.
| if (signal?.aborted || !this.isOutputCurrent(invocation, sourceRevision)) { | ||
| throw new Error("invocation request is closed") | ||
| } | ||
| this.releaseObservation(invocation.id) | ||
| return | ||
| } | ||
| await this.client.complete(invocation.id, { | ||
| instanceId: this.config.instanceId, | ||
| claimToken: invocation.claimToken, | ||
| ...(body.markdown ? { finalMessageMarkdown: body.markdown } : { noResponse: true }), | ||
| ...(body.metadata ? { metadata: body.metadata } : {}), | ||
| }) | ||
| if (signal?.aborted || !this.isOutputCurrent(invocation, sourceRevision)) { | ||
| throw new Error("invocation request is closed") | ||
| } | ||
| await this.client.complete( | ||
| invocation.id, | ||
| { | ||
| instanceId: this.config.instanceId, | ||
| claimToken: invocation.claimToken, | ||
| sourceRevision, | ||
| ...(body.markdown ? { finalMessageMarkdown: body.markdown } : { noResponse: true }), | ||
| ...(body.metadata ? { metadata: this.completionMetadata(invocation, body.metadata) } : {}), | ||
| }, | ||
| signal | ||
| ) | ||
| if (signal?.aborted || !this.isOutputCurrent(invocation, sourceRevision)) { | ||
| throw new Error("invocation request is closed") | ||
| } | ||
| this.releaseObservation(invocation.id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Post-write currency checks treat a landed terminal write as a failure. Both sites re-check cancellation and source revision after complete / completeSealed returned successfully. The pre-write checks already prevent stale output, so the only effect of the post-write check is that a durable completion is reported as not completed and the observation is never released.
extensions/remote-session/src/session.ts#L1330-L1353: remove the post-write throws at Lines 1330-1332 and Lines 1350-1352, and release the observation. OtherwisefailAfterTerminalWriteposts/failon an already-completed invocation andreplyre-arms a closed turn asretryable.extensions/remote-session/src/session.ts#L1685-L1709: after thecompletecall returns, callreleaseObservation(invocation.id)before returningfalse. Otherwise the acknowledgement landed but the observation keeps renewing a claim that no longer exists.
📍 Affects 1 file
extensions/remote-session/src/session.ts#L1330-L1353(this comment)extensions/remote-session/src/session.ts#L1685-L1709
🤖 Prompt for 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.
In `@extensions/remote-session/src/session.ts` around lines 1330 - 1353, Update
extensions/remote-session/src/session.ts lines 1330-1353 in the completion flow
to remove the post-write currency checks after complete/completeSealed, and
release the observation after the successful terminal write; retain the
pre-write checks. In the same file, lines 1685-1709, update the flow after
complete returns to call releaseObservation(invocation.id) before returning
false.
| const liveSwept = swept.filter((item) => !this.isClaimCancelled(item)) | ||
| const currentParts = liveSwept.map((item) => { | ||
| if (!isSessionControlInvocation(item)) return item.promptMarkdown.trim() || "(empty message)" | ||
| const queued = parseSessionControlCommand(item) | ||
| return queued?.name === "steer" ? queued.args : "" | ||
| }) | ||
| if (text) currentParts.push(text) | ||
| if (currentParts.length === 0) { | ||
| await this.completeAck(invocation, "Nothing to steer with; the turn continues.") | ||
| return | ||
| } | ||
| combined = buildSteerContent(currentParts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
steerRunningTurn does not drop empty parts, unlike the other two derivations.
Lines 1495-1499 map a swept non-steer control command to "". sweepQueuedForSteer (Line 1632) and steerByInterrupt (Line 1574) both apply .filter(Boolean) to the same mapping. This path does not.
Result: with no steer text and only a swept non-steer control claim, currentParts is [""]. The check at Line 1501 passes, and Line 1505 builds steer content from an empty segment. The runtime is then steered with empty content instead of receiving the "Nothing to steer with; the turn continues." acknowledgement.
The same mapping now exists three times (Lines 1495-1500, Lines 1568-1574, Lines 1626-1631). Extract it to one helper so the filtering rule cannot diverge again.
🐛 Proposed fix
+ /** Canonical steer text for the live swept claims, derived after all awaits. */
+ private deriveSteerParts(liveSwept: ClaimedInvocation[], text: string): string[] {
+ const parts = liveSwept.map((item) => {
+ if (!isSessionControlInvocation(item)) return item.promptMarkdown.trim() || "(empty message)"
+ const queued = parseSessionControlCommand(item)
+ return queued?.name === "steer" ? queued.args : ""
+ })
+ if (text) parts.push(text)
+ return parts.filter(Boolean)
+ }Then in steerRunningTurn:
const liveSwept = swept.filter((item) => !this.isClaimCancelled(item))
- const currentParts = liveSwept.map((item) => {
- if (!isSessionControlInvocation(item)) return item.promptMarkdown.trim() || "(empty message)"
- const queued = parseSessionControlCommand(item)
- return queued?.name === "steer" ? queued.args : ""
- })
- if (text) currentParts.push(text)
+ const currentParts = this.deriveSteerParts(liveSwept, text)
if (currentParts.length === 0) {Use the same helper at Lines 1568-1574 and Lines 1626-1631.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const liveSwept = swept.filter((item) => !this.isClaimCancelled(item)) | |
| const currentParts = liveSwept.map((item) => { | |
| if (!isSessionControlInvocation(item)) return item.promptMarkdown.trim() || "(empty message)" | |
| const queued = parseSessionControlCommand(item) | |
| return queued?.name === "steer" ? queued.args : "" | |
| }) | |
| if (text) currentParts.push(text) | |
| if (currentParts.length === 0) { | |
| await this.completeAck(invocation, "Nothing to steer with; the turn continues.") | |
| return | |
| } | |
| combined = buildSteerContent(currentParts) | |
| const liveSwept = swept.filter((item) => !this.isClaimCancelled(item)) | |
| const currentParts = this.deriveSteerParts(liveSwept, text) | |
| if (currentParts.length === 0) { | |
| await this.completeAck(invocation, "Nothing to steer with; the turn continues.") | |
| return | |
| } | |
| combined = buildSteerContent(currentParts) |
🤖 Prompt for 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.
In `@extensions/remote-session/src/session.ts` around lines 1494 - 1505, Extract
the shared swept-item-to-content mapping from steerRunningTurn and the
corresponding steerByInterrupt and sweepQueuedForSteer flows into one helper,
ensuring non-steer control commands and empty prompt text are filtered out.
Replace all three duplicated mappings with the helper so steerRunningTurn
reaches the “Nothing to steer with” acknowledgement when no usable content
remains.
| if (!this.isOutputCurrent(entry.invocation, sourceRevision)) { | ||
| this.clearInflight(invocationId) | ||
| if (this.activeTurnStream === entry.invocation.responseStreamId) this.activeTurnStream = undefined | ||
| await this.syncPresence() | ||
| return { ok: false, message: `Request ${invocationId} is closed; its source changed.` } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The stale-source pre-check strands the owner's contributors.
This branch clears the owner's in-flight entry and returns, but it does not close the contributors. The 409 path at Lines 2031-2038 handles the same outcome and calls failContributors.
After Line 2008 removes the entry, nothing can reach those contributors again: completeContributors and failContributors both read from the entry, and abortRunningTurnForContext resolves the owner through this.inflight.get(ownerId), which is now empty. Their observations stay registered and their claims hold until the TTL expires.
Close the contributors on this path as well.
🐛 Proposed fix
if (!this.isOutputCurrent(entry.invocation, sourceRevision)) {
this.clearInflight(invocationId)
+ await this.failContributors(entry, "The owning turn input became stale before completion.")
if (this.activeTurnStream === entry.invocation.responseStreamId) this.activeTurnStream = undefined
await this.syncPresence()
return { ok: false, message: `Request ${invocationId} is closed; its source changed.` }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.isOutputCurrent(entry.invocation, sourceRevision)) { | |
| this.clearInflight(invocationId) | |
| if (this.activeTurnStream === entry.invocation.responseStreamId) this.activeTurnStream = undefined | |
| await this.syncPresence() | |
| return { ok: false, message: `Request ${invocationId} is closed; its source changed.` } | |
| } | |
| if (!this.isOutputCurrent(entry.invocation, sourceRevision)) { | |
| this.clearInflight(invocationId) | |
| await this.failContributors(entry, "The owning turn input became stale before completion.") | |
| if (this.activeTurnStream === entry.invocation.responseStreamId) this.activeTurnStream = undefined | |
| await this.syncPresence() | |
| return { ok: false, message: `Request ${invocationId} is closed; its source changed.` } | |
| } |
🤖 Prompt for 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.
In `@extensions/remote-session/src/session.ts` around lines 2007 - 2012, Update
the stale-source branch in the request handling flow to call failContributors
for the removed entry before returning, matching the existing 409 path. Preserve
the current cleanup, presence synchronization, and closed-request response while
ensuring contributor observations and claims are resolved immediately.
a950d73 to
64bd4b5
Compare
- Observe claimed inputs before delivery and propagate canonical revisions - Apply live updates through runtime steering with sealed output fences - Interrupt cancellations and schedule replacement claims without callback deadlocks 🤖 Generated with [Codex](https://github.com/openai/codex) Co-authored-by: codex <codex@users.noreply.github.com>
- Stop local work when authoritative observation ownership disappears - Bound failed terminal paths by failing or releasing their observations - Retain folded-input ownership until its terminal close commits 🤖 Generated with [Codex](https://github.com/openai/codex) Co-authored-by: codex <codex@users.noreply.github.com>
- abort live updates before stale attachment or steer actuation - keep folded contributors observed until their owner settles - preserve session-control metadata and terminal fences 🤖 Generated with [Codex](https://github.com/openai/codex) Co-authored-by: codex <codex@users.noreply.github.com>
completeTurn spelled one currency guard five times, completeAck and completeSealed each carried a near-duplicate sealed/plaintext branch, and failInvocation reimplemented failFencedInvocation — which is the shared /fail helper a reviewer asked for, so no third one was added. Every guard point is retained; only the duplication is gone. Also removes abort probes at points with no suspension between them and the previous check, a dependency phase promotion bindRunningOwner already performs, and a steer wrapper whose insertion had orphaned the jsdoc of the function below it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pKNVFpWu4VGPCh3GQ5hrB
Thirteen copies of the plaintext input-update literal and four copies of the blocked-steer actuator collapse to two builders. One test asserted a private claim object had been mutated in place; it now asserts what was actually delivered. One cancellation test was subsumed by two neighbours covering each of its halves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pKNVFpWu4VGPCh3GQ5hrB
64bd4b5 to
6bdf8ab
Compare
1 participant
Problem
PR #1825 exposes authoritative claim observation, renewal, source updates, and cancellation, but
RemoteSessionstill treats claims as immutable and runs its own renewal loop. Edits can therefore leave prepared/runtime input and completion revisions out of sync, while deletions or route changes cannot interrupt local work safely. Claude and future RemoteSession-based connectors also advertise no truthful live/restart input capability.Solution
Move the generic RemoteSession lifecycle onto
BotRuntimeTransport.observeClaim:input.updatesfrom actuator capability (steer→live, otherwiserestart) and send the same full manifest on hello and every presence update without branching on runtime kind.unstarted,processing,running, andterminalphases. Bootstrap-time updates replace local input; preparation-time updates request restart; running updates rebuild current attachments and steer exactly once.sourceRevisionthrough plaintext, sealed, no-response, timeout, interrupted, folded, interceptor, and session-control completion paths. A stale 409 closes locally instead of rearming captured output; non-retrying terminal-write failures fall back to/failor stop observation renewal.steerTextand interrupt actuator; no Claude-specific mutation state machine was added.Completed turns and already-posted interim/external side effects remain unchanged by design.
Files
extensions/remote-session/src/session.tsextensions/remote-session/src/client.tsextensions/remote-session/src/index.tsextensions/remote-session/src/session.test.tsextensions/remote-session/src/session.sealed.test.tsextensions/claude-code-remote/src/channel-server.tsextensions/claude-code-remote/src/{channel-server,session-control}.test.tsTest plan
extensions/remote-sessiontests — 191 passed, 0 failed, 562 assertions across 7 files.extensions/claude-code-remotetests — 127 passed, 0 failed, 315 assertions across 9 files.git diff --check.📋 Full implementation plan
Goal
Make the backend control protocol usable by RemoteSession-based runtimes. Claimed input is synchronized before processing; edits either steer a running runtime or trigger restart fallback; cancellations interrupt and remove local ownership; completion uses the exact applied source revision.
Five-layer stack
RemoteSession behavior
Manifest
sessionControl.steeradvertisesinput.updates = "live".restart.{}.Observation and processing boundary
observeClaimand await initialsync().unstartedmeans no interceptor, command, fold, attachment build, or delivery has begun and may accept an in-place update.Live update and output fence
SealingState.restart-required; local old output remains closed.Cancellation and recovery
finally, avoiding a cycle through PR3's global adapter queue andhandle.sync().WeakSet<ClaimedInvocation>fences references surviving map removal without retaining invocation IDs for process lifetime.Completion
sourceRevision, including sealed and no-response closes.409 INVOCATION_INPUT_STALEis terminal locally; backend reconciliation remains authoritative./failhas no revision field.Claude mapping
createClaudeSessionControl.steercontinues to callsteerTextonce with the scratchpad prefix and preserves carry-on absorption.runtime.kindchecks or separate Claude mutation state were introduced; Hermes/OpenClaw/custom descriptors get the same behavior for equal actuator capabilities.Explicit exclusions
Status
🤖 PR by Codex