fix(machine): enforce allowPageAgents + visibleToGlobalAssistant machine access toggles#2044
Conversation
…ine access toggles
Both toggles were persisted by the Machine Settings route but never read on
the machine-access path, so flipping either did nothing (GA blocker, deferred
items 1-2 of the Machine/Terminal arc).
- isMachineAccessible (createMachineDirectory) now returns a
MachineAccessDecision ({ allowed } with an optional LLM-facing denial
reason) instead of a bare boolean, and enforces both toggles after the
existing page-view check: a page-scoped agent (agentPageId, own or
inherited via parentAgentId) is denied when allowPageAgents=false; the
global assistant (no agentPageId) is denied when
visibleToGlobalAssistant=false. Toggle reasons surface only to actors who
can already view the page (no title leak).
- resolveGlobalConfiguredMachines additionally EXCLUDES hidden machines at
resolution, so the global assistant never lists them and the default-active
fallback (machines[0]) lands on the next visible machine instead of
dead-ending on a hidden one. Applies to the resolved own-machine page too.
- bash/file/git tool gates and switch_machine surface the denial reason to
the model; list_machines filters on the decision. Git tools reuse the same
machineDirectory singleton, so they are covered with no extra wiring.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3
|
Warning Review limit reached
Next review available in: 17 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 Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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 |
![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: 2fd1e95642
ℹ️ 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".
| // canViewPage so a toggle reason (which names the machine) is never | ||
| // surfaced to an actor who can't view the page in the first place. | ||
| const agentPageId = activeMachineAgentPageId(rawContext); | ||
| if (agentPageId && !page.allowPageAgents) { |
There was a problem hiding this comment.
When a page agent has userScopedAccess=true, the permission layer treats it like the invoking user (canActorViewPage falls through via resolveActingAgentId), but this new check only tests for the presence of an agent page id. In that setup, a personal/user-scoped agent configured with an existing Machine the user can view is still blocked whenever that Machine has "Allow page agents" off, even though user-scoped/global actors should not be subject to the page-agent toggle. Use the same acting-agent resolution, or include userScopedAccess in the directory deps, before applying allowPageAgents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8f0b5e2. canActorViewPage already resolves through resolveActingAgentId, which treats a userScopedAccess=true page agent as the invoking user for view permissions — the toggle classification did not know about this and blocked such an agent whenever allowPageAgents was off, contradicting the task's own stated scope.
Fix: isMachineAccessible now resolves the agent's own chatSource.agentPageId (getAgentPageId — the same seam resolveActingAgentId uses) and checks a new isUserScopedAgent dep (wired to hasAgentUserScopedAccess) before applying either toggle. A user-scoped agent bypasses the toggle decision entirely — exempt from BOTH toggles, not reclassified as the global assistant (visibleToGlobalAssistant would apply an unrelated gate, since its machine list comes from its own agent config, not the global config repository).
Added 4 tests: user-scoped agent allowed despite allowPageAgents=false; the exemption does not bypass the view-permission check; a non-user-scoped agent is still gated; the exemption keys off chatSource.agentPageId only, not a sub-agent's parentAgentId. vitest/tsc --noEmit green.
Leaving this thread open for your verification per policy.
…active machine fallback
Review-driven hardening of the toggle enforcement (8-angle self-review):
- resolveActiveMachine now performs the access check itself on every call and
returns {machine, access}: the default falls back past blocked machines to
the first ACCESSIBLE configured one (a blocked machines[0] no longer
dead-ends bash/git/file while a usable machine sits one slot over — for
BOTH toggles and both actor kinds), an explicitly switched machine is
denied with its specific reason rather than silently rerouted, and when
every machine is blocked the first one's real reason surfaces instead of
the misleading "Terminal access is not enabled".
- Dropped the resolution-time visibleToGlobalAssistant filter:
isMachineAccessible is the single policy site, so switch_machine to a
hidden machine now explains the toggle instead of claiming "unconfigured",
and the N-findPage-per-listMachines fan-out is gone.
- Toggle policy extracted to @pagespace/lib machines/machine-access.ts
(decideMachineToggleAccess, pure + machine-readable denial codes) beside
canViewMachine/canEditMachine; the directory renders the LLM-facing prose.
switch_machine returns the code (page_agents_disabled/hidden_from_global)
instead of miscategorizing policy denials as 'inaccessible'.
- Denial rendering shared via machineAccessDeniedError (bash/file + git
boundaries can no longer drift); findPage row type reuses
Pick<MachineSettings, ...>; trashed machine pages now denied at the gate;
stale SettingsTab doc comment ("not yet enforced anywhere") corrected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3
… toggle
canActorViewPage already resolves through resolveActingAgentId, which treats
a page agent with userScopedAccess=true (owner-toggled via update_agent_config)
as the INVOKING USER for view permissions — it falls through to the user's
own access rather than the agent's narrower membership. isMachineAccessible's
toggle-actor classification didn't know about this: it only checked
agentPageId presence, so a user-scoped agent (which the machine owner can
already see via canViewPage as themselves) was still misclassified as a
narrow page-scoped actor and blocked whenever allowPageAgents was off —
contradicting the task's own stated scope ("a user-scoped/global agent is
unaffected by this flag").
Fix: isMachineAccessible now resolves the agent's OWN chatSource.agentPageId
(getAgentPageId — the same seam resolveActingAgentId/canActorViewPage use,
deliberately NOT the parentAgentId sub-agent fallback) and checks the new
isUserScopedAgent dep (wired to hasAgentUserScopedAccess) before applying
either toggle. A user-scoped agent bypasses the toggle decision entirely —
it is exempt from BOTH toggles, not reclassified as the global assistant
(visibleToGlobalAssistant would apply an unrelated gate: its machine list
comes from its own agent config, not globalMachineConfigRepository).
Colocated tests: user-scoped agent allowed despite allowPageAgents=false;
exemption doesn't bypass the view-permission check; a non-user-scoped agent
is still gated; the exemption keys off chatSource.agentPageId only, not a
sub-agent's parentAgentId.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3
Self-review finding: list_machines called resolveActiveMachine (its own listMachines + a short-circuited isMachineAccessible loop) AND separately mapped isMachineAccessible over every configured machine to build the accessible-only display list — up to 2x the isMachineAccessible calls (each now a findPage + canViewPage + isUserScopedAgent DB round trip) per list_machines invocation versus before this PR's active-machine fallback was added. Fix: compute the isMachineAccessible decision for every configured machine ONCE, then derive both the active machine (selectActiveFromDecisions — no I/O, mirrors resolveActiveMachine's switched/first-accessible/first-denied policy) and the accessible-only filter from that single decision set. open() (bash/file/git boundary) is untouched — it keeps resolveActiveMachine's short-circuited lookup, which only needs ONE accessible machine, not all of them. Test asserts the call count directly: 2 configured machines -> exactly 2 isMachineAccessible calls (was up to 4 in the worst case where machines[0] is blocked). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3
…ified fine locally) No code change. gh run rerun on the previous CI run stalled identically across 4 attempts (ESLint step / Build packages step never completing, independent of fleet queue depth). Pushing a fresh commit to get a brand-new workflow run rather than reusing the existing run's possibly-corrupted state. Local bun run lint / vitest / tsc all pass — see PR #2044 description. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3
1 participant
Summary
GA-blocker remediation for deferred items 1 and 2 of the Machine/Terminal arc (page
wm76dkca6cd2jwcldumys5c5): the two Machine access toggles were persisted but never enforced — the Settings switches wroteallowPageAgents/visibleToGlobalAssistantto the page row, and nothing on the machine-access path read them back.What's enforced now
Policy (pure, machine-readable codes):
decideMachineToggleAccessinpackages/lib/src/services/machines/machine-access.ts— the module the codebase designates for shared machine access checks.allowPageAgentsgates page-scoped agents (ownagentPageIdor a sub-agent'sparentAgentId— the same discriminatorresolveConfiguredMachinesuses to pick whose machine list applies);visibleToGlobalAssistantgates the global assistant. Each toggle applies only to its own actor kind.Exemption — user-scoped page agents: a page agent with
userScopedAccess=true(owner-toggled) already acts with the invoking user's own reach for view permissions (canActorViewPage→resolveActingAgentId).isMachineAccessiblemirrors that fallthrough via a newisUserScopedAgentdep: such an agent bypassesallowPageAgentsentirely (it isn't reclassified as the global assistant either —visibleToGlobalAssistantdoesn't apply to it, since its machine list comes from its own agent config, not the global config repository). Matches the task's original scope ("a user-scoped/global agent is unaffected by this flag").Enforcement (single site, every call):
isMachineAccessibleincreateMachineDirectory(sandbox-tools-runtime.ts) consumes the policy and renders an LLM-facing denial reason naming the machine and the exact Settings toggle. Every bash/file/git tool call,switch_machine, andlist_machinesgoes through it. Trashed machine pages are also denied. Toggle checks run aftercanViewPage, so the reason never leaks to an actor who can't view the page.Active-machine resolution (
resolveActiveMachine,sandbox-tools.ts) performs the access check itself and returns{machine, access}:machines[0]doesn't dead-end the whole tool group while a usable machine sits one slot over (both toggles, both actor kinds).list_machinescomputes each machine's access decision once and derives both the active-machine selection (selectActiveFromDecisions, no extra I/O) and its accessible-only display filter from that single pass — avoiding a doubleisMachineAccessiblecall per configured machine.switch_machinereturns the denial code (page_agents_disabled/hidden_from_global) as its machine-readablereasoninstead of miscategorizing policy denials asinaccessible. Denial rendering is shared (machineAccessDeniedError) so the bash/file and git execution boundaries cannot drift. Git tools reuse the samemachineDirectorysingleton — no extra wiring.The flag's existing AI_CHAT/page-agent consumer (
agent-awareness.ts) is untouched. Whole surface remains behindCODE_EXECUTION_ENABLED(OFF).Tests
packages/libmachine-access.test.ts: pure policy matrix (each toggle gates only its own actor kind, denial codes).sandbox-tools-runtime.test.ts: page agent denied (code + reason) / allowed; sub-agent treated as page-scoped; global assistant denied (code + reason) / allowed; cross-flag non-interference; no-title-leak; trashed machine denied; user-scoped agent exempt fromallowPageAgents(but still subject to view-permission checks, and the exemption keys off its ownchatSource.agentPageIdonly — not a sub-agent'sparentAgentId).sandbox-tools.test.ts: fallback past a blockedmachines[0]routes bash to the usable machine; all-blocked surfaces the first machine's specific reason; an explicitly switched blocked machine denies without silent rerouting;list_machinesmarks the fallback target active AND callsisMachineAccessibleexactly once per configured machine (asserted directly);switch_machinereturns the toggle code.vitest: 236/236 (web sandbox tool files, includes the new call-count assertion) + 22/22 (lib machines).tsc --noEmitgreen inpackages/libandapps/web.🤖 Generated with Claude Code
https://claude.ai/code/session_01DrdKH5t1RYZrVo9CnE8Jm3