Repository navigation
fix(realtime): hoist cross-tier tenant/membership resolution; dead-code sweep; retire machine vocabulary - #2271
Conversation
|
Warning Review limit reached
Next review available in: 2 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8613fb20d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const MACHINE_MAX_ACTIVE_SECONDS_ENV = 'TERMINAL_MACHINE_MAX_ACTIVE_SECONDS'; | ||
| const DEFAULT_MACHINE_MAX_ACTIVE_SECONDS = 4 * 60 * 60; // 4 hours | ||
| const SESSION_MAX_ACTIVE_SECONDS_ENV = 'TERMINAL_SESSION_MAX_ACTIVE_SECONDS'; |
There was a problem hiding this comment.
When an existing deployment sets TERMINAL_MACHINE_MAX_ACTIVE_SECONDS, this rename silently stops reading that value and falls back to four hours; for example, a five-minute cost guardrail becomes a four-hour guardrail after deployment. Keep the old name as a compatibility fallback or provide a coordinated configuration migration and release note before removing it.
AGENTS.md reference: AGENTS.md:L223-L228
Useful? React with 👍 / 👎.
…de sweep; retire machine vocabulary Follow-up from the post-merge audit of PR #2258 (realtime + cross-tier consistency slice). - Hoist resolveSessionTenantId (fail-closed on vanished drive) and resolveDriveMembership/canRunCode reduction into ONE new lib module (packages/lib/src/services/agent-sessions/agent-session-tenant.ts), imported by both apps/realtime and apps/web. Kills the web tier's fail-open `drive?.ownerId ?? session.ownerId` fallback. - Delete dead imports (checkMachineRuntimeGuardrail, recordMachineActivity, conversations schema) and the unused resolveDriveOwnerContext helper in apps/realtime/src/index.ts. - Fix five stale doc comments left by prior re-keying (index.ts tenantId docblock, shell-access.ts payer prose, XtermTerminal.tsx, and the useAgentSessionChat test header). - Delete the permanently-undefined TerminalSession.pageId field; attribute driveId on shell trackUsage calls instead (SandboxBillingDeps.trackUsage gains an optional driveId alongside the AI-tool-runner path's pageId). - Export composeSocketKey from shell-handler instead of re-spelling the socket-key format inline in index.ts. - Rename SandboxSurface's 'machine' value and quota.ts's guardrail identifiers to session vocabulary (checkSessionRuntimeGuardrail, recordSessionActivity, SESSION_ACTIVITY_GRACE_MS, etc.), touching both tiers' call sites and tests. Keeps the machine_sprite_reclaims table name. Closes #2265 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DrdwiwAmX57oXSpiBcmAF
e8613fb to
a93e577
Compare
Summary
Follow-up from the post-merge audit of PR #2258 (realtime + cross-tier consistency slice). Fixes all 7 findings in #2265:
resolveSessionTenantIdintopackages/lib/src/services/agent-sessions/agent-session-tenant.ts, imported by bothapps/realtime/src/index.tsandapps/web/src/lib/agent-sessions/agent-sessions-runtime.ts. Kills the web tier's fail-opendrive?.ownerId ?? session.ownerIdfallback — a vanished drive now denies on both tiers via the same function.resolveDriveMembership+ thecanRunCodereduction also live in the new module, replacing the copy-pasted DB queries in both tiers.checkMachineRuntimeGuardrail/recordMachineActivityimports, the deadconversationsschema import, and the unusedresolveDriveOwnerContexthelper + type fromapps/realtime/src/index.ts.index.ts, the payer prose inshell-access.ts,XtermTerminal.tsx's reference to a deleted method, anduseAgentSessionChat.test.ts's header referencing deleted modules.pageIdplumbing (finding 5):deletedUPDATE: PR fix(billing): unify sandbox payer on the SESSION; retire page-keyed payer helper #2273 (billing payer unification, issue fix(billing): unify sandbox payer on the SESSION; retire page-keyed payer helper; first-class storage attribution [HIGH] #2260) merged to master mid-review and fully addressed this independently — it deletedTerminalSession.pageId...TerminalSession.pageId, addeddriveId/agentSessionIdfields, and threaded them through all threetrackUsagecall sites plus the first-classdriveId/sessionIdcolumns onAIMonitoring.trackUsage. After rebasing onto master, this branch's own (less complete) version of the same fix was dropped entirely in favor of fix(billing): unify sandbox payer on the SESSION; retire page-keyed payer helper #2273's — confirmed viagit diff origin/master...HEAD -- apps/realtime/src/terminal/terminal-session-map.ts packages/lib/src/services/sandbox/sandbox-billing.tsshowing zero diff against master. Finding 5 is fully satisfied by master; nothing left for this PR to do there.composeSocketKeyexport (finding 6): exported fromshell-handler.ts;index.tsnow calls it instead of re-spelling the socket-key format inline.SandboxSurface's'machine'value and everyquota.tsguardrail identifier (checkMachineRuntimeGuardrail→checkSessionRuntimeGuardrail,recordMachineActivity→recordSessionActivity,MACHINE_ACTIVITY_GRACE_MS→SESSION_ACTIVITY_GRACE_MS, etc.) renamed, touching both tiers' call sites and tests.machine_sprite_reclaimstable name deliberately untouched (migration-documented).Issue #2260's PR (#2273) merged with a more complete billing-attribution fix than finding 5 anticipated, so this branch defers to it entirely rather than duplicating.
Rebase note: this branch was rebased onto master twice during review to pick up PR #2273 (billing payer unification) and PRs #2269/#2268 (session-tools scoping, pane lifecycle) as they landed. All five overlapping files (
shell-handler.ts,terminal-session-map.ts,shell-handler.test.ts,sandbox-tools-runtime.ts,tool-runners.ts) plusagent-sessions-runtime.ts(overlap with #2269's boundedlistSessionConversationsBulk) were reconciled by hand — master's versions won wherever there was real overlap; this PR's renames/hoists/dead-code deletions sit cleanly on top.Test plan
agent-session-tenant.test.tspins fail-closed-on-vanished-drive forresolveSessionTenantId, plusresolveDriveMembership/canRunCodeForSessionapps/realtimefull suite passesapps/websandbox-tools-runtime + agent-sessions route tests passpackages/libfull suite passesbun run typecheck,bun run lint,bun run knip:checkall pass (monorepo + per-package for realtime/web/lib)cd packages/lib && bun run typecheckpassesbun run test:unit— 15122/15129 pass; the 1 failure (grouping.test.tsmidnight-crossing) is a pre-existing TZ-env issue in a file this branch never touchesbun run test:security— 44/50 suites pass; the 6 failures are pre-existing stale references inscripts/test-security.sh(test files deleted in fix(security): remove all password-based authentication #861, and vitest-config-excluded files), confirmed viagit log --diff-filter=D, unrelated to this branchCloses #2265
🤖 Generated with Claude Code
https://claude.ai/code/session_018DrdwiwAmX57oXSpiBcmAF