Skip to content

feat(accounts): manage agent accounts on an authenticated self-hosted cockpit - #970

Open
piotrchabros wants to merge 5 commits into
open-mercato:mainfrom
piotrchabros:cez/99fc8106
Open

piotrchabros wants to merge 5 commits into
open-mercato:mainfrom
piotrchabros:cez/99fc8106

Conversation

@piotrchabros

Copy link
Copy Markdown
Contributor

Summary

  • separate "the cockpit is remote" from "the operator may manage host-side agent accounts", so a self-hosted deployment behind an authenticated perimeter is no longer treated like an untrusted public client
  • add CEZ_REMOTE_AGENT_ACCOUNTS=1 (exact 1), which re-enables listing, adding, renaming, selecting, status-probing, details and removal of agent accounts in remote mode
  • keep the default off and keep desktop-opening actions local-only regardless of the flag — Connect still returns the login command to run on the host
  • replace the dead-end hosted copy in Settings → Agent accounts with the flag to set, and document the variable in README.md and .env.example

Problem

Every route in the agent-profiles family was gated on capabilities().localHandoff, which is false for any non-loopback bind or CEZ_REMOTE=1. That conflates two different questions: "can this server open a desktop app for the person looking at the page?" (genuinely no on a VPS) and "is this deployment trusted enough to read and write ~/.cezar/agent-accounts.json?" (a deployment behind authenticated Caddy/nginx can be). The result was a self-hosted cockpit that could never use more than one Claude/Codex account, with the UI offering no way forward.

Approach

agentAccountsEnabled(env, bindHost) in capabilities.ts is deliberately a separate predicate rather than a new field on resolveCapabilities: localHandoff still means "this server can open a desktop app", and that answer does not change. The flag requires exact 1, matching the other opt-ins in this codebase, so a stray true or 0 cannot silently widen the surface.

The disclosure this guards is real — account folders are absolute host paths carrying the username, and the details route can expose account identity — so the flag is off by default and both the README row and the .env.example comment say plainly that it belongs only behind an authenticated perimeter.

warmAgentKnowledge now uses the same predicate, so an opted-in remote cockpit warms per-account auth status instead of only the defaults.

Verification

  • NODE_ENV=test npx vitest run packages/cezar/src/server/capabilities.test.ts packages/cezar/src/server/agent-profiles-api.test.ts packages/web/src/routes/settings/accounts-section.test.tsx — 204 tests passed
  • npm run typecheck — clean across the workspace
  • new coverage: agentAccountsEnabled accepts only exact 1; an opted-in remote server can create an account, have it appear in an editable: true listing, and select it, while POST .../open still returns 409
  • exercised on a live self-hosted instance behind authenticated Caddy: the accounts API reports editable: true and the multi-account controls appear in Settings

🤖 Generated with Claude Code

… cockpit

Agent accounts were gated on `localHandoff`, so every remote deployment —
including a self-hosted cockpit behind an authenticated reverse proxy — was
refused the whole agent-profiles family and shown "managed from the machine
that owns the checkout".

Separate "remote UI" from "may manage host-side accounts": `CEZ_REMOTE_AGENT_ACCOUNTS=1`
lets an operator who has put the cockpit behind an authenticated perimeter list,
add, rename, select, probe, inspect and remove accounts. The default stays off,
and desktop-opening actions remain local-only regardless of the flag — Connect
still returns the login command to run on the host.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pat-lewczuk pat-lewczuk self-assigned this Sep 12, 2026
@pat-lewczuk pat-lewczuk added the in-progress Cezar agent is actively working this issue label Sep 12, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-12T17:15:53Z. Other auto-skills will skip this PR until the lock is released.

@pat-lewczuk pat-lewczuk added changes-requested Reviewer requested changes feature New capability security Security-sensitive priority-medium Ordinary bug or feature risk-high Wide blast radius, review deeply needs-qa Requires manual QA before merge labels Sep 12, 2026
@pat-lewczuk

pat-lewczuk commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr — 🏷️ label rationale

  • changes-requested — the re-review found two blockers: the new browse-root confinement applies only its lexical half, so a symlink under the root escapes it and the existence oracle the confinement was added to close is still open; and npm test is red on this head (one deterministic failure inherited from main).
  • feature — this adds a new capability (CEZ_REMOTE_AGENT_ACCOUNTS=1) rather than fixing a defect: multi-account management becomes reachable on a self-hosted cockpit for the first time.
  • 🔒 security — the change moves the gate on seven /api/v1/workspace/agent-profiles* routes from "local machine only" to an operator-set env flag, and the remaining blocker is a path-confinement bypass on that widened surface.
  • 🔹 priority-medium — unchanged: the flag is off unless set to exact 1, so nothing is released-blocked or broken by default, and the blockers are only reachable on a deployment that opted in.
  • ⚠️ risk-high — unchanged: it widens what a remote deployment exposes, touches a shared contract surface (agentProfilesResponseSchema's editable semantics) across server, contract and web, and the outstanding finding is in the confinement code itself.
  • 🧪 needs-qa — the Settings → Agent accounts pane changes visibly in both deployment modes, and the confinement behaviour is only observable by driving the opted-in remote server, so a human should exercise it before merge.

@pat-lewczuk pat-lewczuk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: feat(accounts): manage agent accounts on an authenticated self-hosted cockpit

Verdict: Request changes — 2 majors, no blockers.

The split this PR makes is the right one. localHandoff genuinely means "this server can open a desktop app for the person looking at the page", agentAccountsEnabled means "this deployment is trusted to read and write ~/.cezar/agent-accounts.json", and conflating them is what left a self-hosted cockpit permanently single-account with no way forward. Keeping the new predicate off resolveCapabilities, requiring exact 1, and leaving /open and the terminal handoff in /providers/connect on localHandoff are all correct calls, and the PR body argues them honestly.

What holds it up is the other side of the flag. Once it is on, the newly-revealed UI contains affordances that can never succeed and whose error message tells the operator to set the flag they just set (Major 1); and the newly-remote POST /workspace/agent-profiles accepts an unconfined absolute host path whose existence it then reports back, which is the disclosure the hosted browse-root narrowing exists to prevent and which the flag's own documentation does not mention (Major 2).


Major

1. An opted-in remote cockpit renders open-in-app menus that always fail, with an error that misdiagnoses the causepackages/web/src/routes/settings/accounts-section.tsx:648-687, packages/cezar/src/server/server.ts:319 and :2172

With CEZ_REMOTE_AGENT_ACCOUNTS=1, editable is true, so AccountsPane now renders account rows and expanding one renders AccountDetails with its Config files and Folder menus. Those menus are not gated on localHandoff:

  • useOpenTargets()GET /open-targets returns { targets: [] } in remote mode (server.ts:4368), so fileChoices and folderChoices are empty;
  • but OpenInMenu always renders its trigger and its leading item (packages/web/src/components/open-in-menu.tsx:128-136), so "System default" is the only item in both menus;
  • picking it calls POST /workspace/agent-profiles/:id/open, still localHandoff-gated at server.ts:2172, which returns the shared hostedProfileRefusal;
  • errorFor prefers the server's json.error (packages/web/src/api/client.ts:199-207), so onError: (error) => toast(error.message, …) shows that text verbatim.

The operator therefore sees: "agent accounts are disabled in hosted mode; set CEZ_REMOTE_AGENT_ACCOUNTS=1 only behind an authenticated perimeter" — on a deployment where CEZ_REMOTE_AGENT_ACCOUNTS=1 is already set. It names the wrong cause and prescribes an action that changes nothing; worse, it points a security-conscious operator at a security flag and implies it is not taking effect.

This PR creates the state. Before it, remote mode replaced the whole pane with the hosted copy, so these controls were unreachable. The new test at packages/cezar/src/server/agent-profiles-api.test.ts:748 walks exactly this path but asserts only expect(opened.status).toBe(409), never the body, so the wording slips through.

Two fixes, both small:

  • give /open its own refusal instead of reusing hostedProfileRefusal/providers/connect already models the right shape one screen up at server.ts:1821 ('Run this command on the machine hosting cezar.');
  • hide or disable the two OpenInMenus when health.capabilities.localHandoff is false, the way packages/web/src/routes/task-git/task-changes.tsx:214 already does.

2. The flag makes an unconfined absolute host path remotely postable, and the response reports whether it existspackages/cezar/src/server/server.ts:1926-1935 and :1993-2042

checkProfileDir validates only "no control characters" and "absolute after expandTilde". That was sufficient while POST /workspace/agent-profiles was reachable only from the local machine, whose owner can read the filesystem anyway. With the flag on, an authenticated remote client can post any absolute host path and read back what the server learned about it:

  • agentProfileBody (server.ts:1877-1904) embeds await profileDirState(provider, profile.path), which readdirs the path and returns { exists, looksValid } (packages/cezar/src/workspace/agent-profiles.ts:141-152);
  • and files[], each with a path and an exists (server.ts:1858-1874);
  • both in the 201 body and in every subsequent GET /workspace/agent-profiles.

So POST {"provider":"claude","configDir":"/root/.ssh"} answers whether /root/.ssh is a readable directory, and nothing confines the path to the hosted browse root. That is precisely the disclosure fs-browse's hosted narrowing is built to deny — server.ts:2595-2605 says so in its own words: "an out-of-root path must answer the SAME way whether or not it exists, or the route becomes the existence oracle fs-browse narrows the tree to prevent" — and .env.example documents CEZ_BROWSE_ROOT for exactly "if the remote viewer should not enumerate the whole home". An operator who set both flags gets the second one quietly defeated by the first.

The flag's documentation does not cover this. The README row and the .env.example comment describe disclosure in the outbound direction only ("exposes absolute paths and account identity"); the inbound direction — the caller naming any host path — is not mentioned, so the consent the PR body is careful to obtain is incomplete on the point that matters.

There is a real counterargument, and it may be the right resolution: an authenticated cockpit user can already launch a run with unrestricted Bash on the host (CODE_REVIEW.md:40), so this grants nothing they could not already obtain. If that is the accepted reasoning, state it — in the README row, or as a BACKWARD_COMPATIBILITY.md entry (see Minor 3). A waiver with that rationale written down resolves this finding; what should not ship is the gap left implicit in a change whose entire premise is informed operator consent.

Otherwise: confine checkProfileDir to workspaceBrowseRoot() when !capabilities().localHandoff, reusing isLexicallyInsideBrowseRoot and its fail-identically-on-absent rule.


Minor

3. No BACKWARD_COMPATIBILITY.md entry, unlike every prior capability flagBACKWARD_COMPATIBILITY.md

CEZ_FOLLOWUPS (§"Follow-up inbox default flip (#471)"), CEZ_SINGLE_PROJECT (§"Single-project workspace mode", :205-226) and CEZ_AUTOMATIONS (§"GitHub automations — opt-in gating (#801)", :228-263) each get an entry documenting activation strictness, exactly which routes change, and non-destructive rollback. CEZ_REMOTE_AGENT_ACCOUNTS changes the conditional answer of seven /api/v1 routes and gets none.

Nothing breaks by default, so this is not the blocker CODE_REVIEW.md:58 describes — but that file is the repo's index of what env flags do to /api/v1, and it is now missing one. The same gap exists in packages/cezar/src/server/capabilities.ts:1-36, whose module doc catalogues localHandoff, followups, singleProject and automations but not the flag whose predicate was just added to that module.

4. agentAccountsEnabled re-derives localHandoff instead of reusing itpackages/cezar/src/server/capabilities.ts:139

return (env.CEZ_REMOTE !== '1' && isLoopbackHost(bindHost)) || env.CEZ_REMOTE_AGENT_ACCOUNTS === '1';

is a second spelling of :161's localHandoff: env.CEZ_REMOTE !== '1' && isLoopbackHost(bindHost). Twenty lines below, the same file argues against exactly this: "Deliberately not re-derived here: RunManager enforces the same predicate, and two spellings of 'is the inbox on' would eventually disagree." (:162-163). Same hazard, and the two now have to stay in step through any future change to what "remote" means.

resolveCapabilities(env, bindHost).localHandoff || env.CEZ_REMOTE_AGENT_ACCOUNTS === '1', or a shared isLocalDeployment(env, bindHost) that both call, keeps one spelling.

5. The two highest-consequence changes are the untested onespackages/cezar/src/server/agent-profiles-api.test.ts

:748-772 covers create → editable: true listing → select → /open 409 under the flag, which is good coverage of the happy path. Not covered:

  • GET /workspace/agent-profiles/:id/details under the flag — the route that returns the account's signed-in identity, and the disclosure the PR body itself calls "real";
  • warmAgentKnowledge's changed gate (server.ts:1622), where an opted-in remote server now spawns per-account auth probes at boot that it previously did not.

CODE_REVIEW.md:53 asks for tests on changed behavior; these are the two changes where a future regression would be least visible.


Nit

6. Stub line left by a comment rewrappackages/cezar/src/server/server.ts:1769-1771

// A NAMED account is refused in hosted mode unless the operator opted in, before anything is
// resolved, exactly like every
// sibling route in the agent-profiles family. …

Rewrap the paragraph.


Validation gate

Run on the PR head (f3c380c8) in an isolated worktree, base main.

Command Result
npm run typecheck ✅ pass — clean across contract, client, server, web
npm test ⚠️ 6394/6395 — one unrelated flake, see note
npm run test:unit ✅ pass — 12/12
npm run build ✅ pass — check:pack ok — 481 files, 85 under web/dist
npm run test:package ✅ pass

Note on npm test: two environmental results, neither attributable to this PR.

  1. A first run showed 6 failures in health-forge.test.ts / projects-api.test.ts, all asserting that a mkdtemp directory is not a git repository. Artifact of this reviewer's sandbox, which sets TMPDIR inside the checkout (/home/cezar/cezar/.ai/cezar/tmp/…), so os.tmpdir() resolved into the repo and git detection legitimately found it. With a real TMPDIR all six pass.
  2. That clean re-run then failed 1 test in packages/cezar/src/workflows/run.test.ts, with unhandled ENOENT rejections from RunManager.rescueStalledQueueRunStore.appendEvent writing to an NDJSON file in a temp run store that teardown had already removed — a teardown race under full-suite parallelism. Re-run in isolation the file passes 94/94. The PR touches neither workflows/run.ts nor runs/store.ts, so this is a pre-existing flake rather than a regression; worth a separate issue, not a change here.

Net: every test relevant to this diff passes, including the new capabilities.test.ts and agent-profiles-api.test.ts cases and the pre-existing hosted-mode refusal tests.

CI on the PR: license/cla ✅. No other checks are configured and branch protection is not readable on main, so the table above is the only functional evidence this change has.

Other signals

  • No merge conflictsmergeable: MERGEABLE. The head is behind origin/main (e8c95f3a vs 9ea303eb) but merges cleanly.
  • No inherited review feedback — no prior reviews, conversation comments, or inline comments on the PR.
  • Backward compatibility — no protected surface from BACKWARD_COMPATIBILITY.md §2 is removed or reshaped. The /api/v1 route set is unchanged; agentProfilesResponseSchema is unchanged (only its doc comment moved); editable keeps z.boolean(). The hostedProfileRefusal body text changes, which is not a protected shape. Default behavior with the flag unset is byte-identical — verified by the pre-existing hosted-mode tests still passing.

What is right here, for the record

  • The predicate is deliberately not a new Capabilities field, and the PR explains why rather than leaving it to the reader.
  • Exact-1 activation matches CEZ_FOLLOWUPS/CEZ_SINGLE_PROJECT/CEZ_AUTOMATIONS, and capabilities.test.ts:147-159 pins it — including the 'true' rejection.
  • /providers/connect keeps its two-stage gate: the named-account refusal moves to the new predicate, the terminal handoff at :1820 stays on localHandoff. That is the correct split and it is easy to get wrong.
  • The listing's single-return shape is preserved, with its reasoning intact.
  • warmAgentKnowledge was not forgotten.

🤖 om-auto-review-pr — reviewed at PR head f3c380c8. Autofix: skipped (not my PR — re-run with --autofix to fix it here).

@pat-lewczuk

Copy link
Copy Markdown
Collaborator

@piotrchabros, two majors to clear before this can go in — full detail in the review:

  1. The opted-in remote UI has dead buttons with a wrong error. With CEZ_REMOTE_AGENT_ACCOUNTS=1, "Show details" on any account row reveals Config files and Folder menus whose only item is "System default" (/open-targets returns [] remotely, but OpenInMenu still renders its leading item). Picking it hits POST .../open, still localHandoff-gated, which returns the shared hostedProfileRefusal — so the toast reads "set CEZ_REMOTE_AGENT_ACCOUNTS=1 only behind an authenticated perimeter" on a box where it is already set. Give /open its own refusal (server.ts:1821 has the right shape) and gate the two menus on health.capabilities.localHandoff, as task-changes.tsx:214 does.

  2. configDir is now remotely postable and unconfined. checkProfileDir only requires "absolute, no control chars", and the 201 body echoes profileDirState's exists plus per-file exists — so POST {"configDir":"/root/.ssh"} answers whether that directory is readable, bypassing the CEZ_BROWSE_ROOT narrowing that fs-browse applies for exactly this reason. Either confine the path to workspaceBrowseRoot() when !localHandoff, or — if the accepted reasoning is that an authenticated user already has Bash on the host — write that into the README row and a BACKWARD_COMPATIBILITY.md entry. The flag's current copy describes only outbound disclosure, so operator consent is incomplete on the inbound half.

Three minors (missing BACKWARD_COMPATIBILITY.md entry, agentAccountsEnabled re-deriving localHandoff, no test on /details or warmAgentKnowledge under the flag) and one nit are in the review too.

The core design — splitting "can open a desktop app" from "may manage host-side accounts" — is right, and the validation gate is green on your head commit. Push the update and re-request review.

@pat-lewczuk pat-lewczuk removed the in-progress Cezar agent is actively working this issue label Sep 12, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr completed: CHANGES REQUESTED. Lock released.

Two majors (dead open-in-app affordances with a misdiagnosing 409 on an opted-in remote cockpit; unconfined remotely-postable configDir whose existence the response reports, unbounded by CEZ_BROWSE_ROOT and undocumented), three minors, one nit. Validation gate green on f3c380c8 — typecheck, build/check:pack, test:unit 12/12, test:package 16/16, vitest 6394/6395 with the one failure confirmed a pre-existing flake in workflows/run.test.ts (passes 94/94 in isolation; file untouched by this PR). No merge conflicts; license/cla is the only CI check and it is green. No inherited reviewer feedback existed on the PR.

Autofix: skipped (not my PR — re-run with --autofix to fix it here).

@piotrchabros

Copy link
Copy Markdown
Contributor Author

Addressed all review findings in 7689dff:

  • hide desktop-open controls when localHandoff is false and return a dedicated hosted /open refusal
  • confine remotely supplied account folders to CEZ_BROWSE_ROOT before probing existence
  • document the conditional API behavior and rollback in BACKWARD_COMPATIBILITY.md
  • share one local-handoff predicate
  • cover opted-in identity reads and boot warming, plus red/green regression tests

Validation: 207 focused tests pass; workspace typecheck, unit tests, build/check:pack, and package tests pass. The full suite reached 6394 passing with only the already-documented ambient root/env failures and queue teardown race outside this diff.

# Conflicts:
#	packages/cezar/src/server/capabilities.ts
@pat-lewczuk pat-lewczuk self-assigned this Sep 14, 2026
@pat-lewczuk pat-lewczuk added the in-progress Cezar agent is actively working this issue label Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-14T10:27:26Z. Other auto-skills will skip this PR until the lock is released.

@pat-lewczuk pat-lewczuk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: feat(accounts): manage agent accounts on an authenticated self-hosted cockpit

Verdict: Request changes — 2 blockers, no majors.

All six findings from the previous review were addressed, and Major 1 is fully closed: the two OpenInMenus are gated on localHandoff (accounts-section.tsx:652), /open got its own refusal (hostedProfileOpenRefusal, server.ts:2194), and both sides have tests. localHandoffEnabled is now one spelling shared by localHandoff and agentAccountsEnabled (capabilities.ts:155), the BACKWARD_COMPATIBILITY.md entry is there with an honest rollback paragraph, and the two untested paths — /details and warmAgentKnowledge under the flag — are covered.

Major 2 is where it stops. The confinement that was added implements the lexical half of the repo's own two-half containment rule and not the realpath half, so a symlink under the browse root walks straight out of it — I reproduced it, below. That reinstates exactly the existence oracle the fix was for, and makes the new BACKWARD_COMPATIBILITY.md sentence "an outside path is rejected before its existence is probed" untrue as written.

The second blocker is not yours: npm test is red on this head because it is red on main.


⛔ Blocker

1. The browse-root confinement is only half applied — a symlink under the root escapes itpackages/cezar/src/server/server.ts:1947-1955

checkProfileDir calls isLexicallyInsideBrowseRoot and stops there. fs-browse.ts:101-111 says in its own words why that is only half the gate:

This split lets the register route reject out-of-root paths UNIFORMLY (the existence oracle stays shut …), then answer honestly about existence, then still catch symlink escapes with the realpath gate.

and the sibling route — POST /api/projects, the one other place that re-asks containment — applies both, in that order, with the reason spelled out at server.ts:2645-2648:

The REALPATH half, now that the path is known to exist: a symlink inside the root pointing out of it spells as contained and is not. Same message as the lexical rejection, so the two halves stay indistinguishable from outside.

Verified on this head (917fd587), remote + CEZ_REMOTE_AGENT_ACCOUNTS=1, CEZ_BROWSE_ROOT set to a fixture home, with a throwaway vitest case driving the real route:

configDir posted symlink target response
<root>/link-to-outside an existing dir outside the root 201, exists: true, files[].path rooted at the link
<root>/link-to-missing an absent path outside the root 201, exists: false

Two requests that spell identically-in-root, two different answers about the world outside the root. Three consequences:

  • the existence oracle is back for anything reachable through a symlink under the root — profileDirState readdirs the path (workspace/agent-profiles.ts:141-152) and accountFiles stats each file under it, and both land in the 201 body and in every later GET /workspace/agent-profiles;
  • the escaped path is persisted and becomes the account's path, which profileEnv hands to the agent as its config dir — so a run reads and writes outside the root, not merely probes it;
  • BACKWARD_COMPATIBILITY.md's new "a remotely posted account folder must be inside CEZ_BROWSE_ROOT; an outside path is rejected before its existence is probed" is a guarantee the code does not keep. A compatibility doc that overstates a confinement is worse than one that says nothing.

Fix — mirror server.ts:2643-2654 inside checkProfileDir: keep the lexical gate first (it is what keeps the answer uniform for a path that is not there), then, once the candidate is known to exist, re-ask with isInsideBrowseRoot(root, expanded) and return the same message on failure so the two halves stay indistinguishable. Tests to add: a symlink under the root pointing out (both a live and a dead target, asserting identical responses), and the same case through PATCH /workspace/agent-profiles/:id, which shares checkProfileDir and has no confinement test at all today.

2. The validation gate is red — npm test fails 1 of 6600packages/cezar/src/workflows/agent-profile-wiring.test.ts:82

FAIL |server| RunManager agent-profile resolution >
  adds NOTHING for the default account — the zero-config env is untouched
AssertionError: expected [ 'CEZ_API_URL', 'CEZ_BIN', …(6) ] to deeply equal [ 'CEZ_HANDOFF_FILE', …(5) ]
+ "CEZ_API_URL"
+ "CEZ_BIN"

This is not your change. It reproduces deterministically on af7e8289 itself — the main commit this branch merged, #972 feat(dispatch) — which added CEZ_API_URL and CEZ_BIN to the base run env without updating the assertion that exists to pin that env. main has been red since.

It is still a blocker here, because the gate rule takes no "pre-existing" exception: this head fails, and nothing on the PR catches it (license/cla is the only check and main is not branch-protected). The fix is one line — add both keys to the expected array with a note that dispatch owns them — landed either on main and merged forward, or here. Your call which; a maintainer waiver on the grounds that main owns it would also clear this finding.


Minor

3. The new localHandoff derivation fails open, against the rule the rest of the web package followspackages/web/src/routes/settings/accounts-section.tsx:603

const localHandoff = health.data?.capabilities?.localHandoff !== false

Unknown ⇒ local. Every other consumer treats unknown as hosted, and packages/web/src/lib/git-actions.ts:29 states it as the rule: "False (or unknown) = hosted mode". task-changes.tsx:112 spells it ?? false; queries.ts:688 spells it === true.

The comment justifies the choice by "an older server response", but that case and "health has not resolved yet" are the same expression here, and only one of them is old-server compatibility. On a cold load of Settings → Agent accounts on a remote cockpit, the Config files and Folder menus render until useHealth settles and then disappear — a flash of the exact affordance Major 1 removed. Server-side nothing is at risk; this is UI honesty and consistency.

Fix: branch on health.data first, and apply the old-server fallback only once data has actually arrived — health.data ? (health.data.capabilities?.localHandoff ?? true) : false.

4. The out-of-root rejection names the absolute browse root; the sibling route deliberately does notpackages/cezar/src/server/server.ts:1953

return `folder must be inside the browsable root: ${root}`;

server.ts:2632-2637 refuses to do this on purpose: "No resolved path in the message (fs-browse's rule): saying where the root is would hand a remote viewer the layout the narrowing hides." The practical exposure is small — a caller who reached this line is already opted in and already sees absolute paths in the listing — but it is the same route family answering the same question two different ways, and the narrow one is the documented one. Use 'folder is outside the browsable root', which also gives Blocker 1's realpath half the identical string it needs.


Nit

5. The rewrapped comment still has a stub linepackages/cezar/src/server/server.ts:1782-1784

// A NAMED account is refused in hosted mode unless the operator opted in. This happens
// before resolution, exactly like every sibling route in the agent-profiles family;
// checking later would already have read
// `~/.cezar/agent-accounts.json`, built a command carrying …

Line three is still short. Rewrap the paragraph as a whole.


🧪 Validation Gate

Run on the PR head (917fd587) in an isolated worktree, base main, with TMPDIR outside the checkout.

Command Status Evidence
npm run typecheck ✅ PASS clean across contract, client, server, web; inline-contract ok — 19 file(s) repointed
npm test FAIL Test Files 1 failed | 346 passed (347), Tests 1 failed | 6599 passed (6600)agent-profile-wiring.test.ts:82; Blocker 2
npm run test:unit ✅ PASS pass 35, fail 0, skipped 1 (the skip is setsid is not available on this platform)
npm run build ✅ PASS check:pack ok — 497 files, 85 under web/dist (shell + assets present)
npm run test:package ✅ PASS tests 16, pass 16, fail 0

The one failure is isolated and attributed: it reproduces alone (vitest run packages/cezar/src/workflows/agent-profile-wiring.test.ts1 failed | 6 passed) and reproduces identically on af7e8289 with this PR's work absent. Everything this diff touches is green, including the new capabilities.test.ts cases (exact-1, 'true' rejected, bind-host variants), the opted-in create/select/open/details walk, the out-of-root rejection pair, and the it.each boot-warm test in both modes.

Last review's workflows/run.test.ts teardown flake did not recur.

Other signals

  • No merge conflictsmergeable: MERGEABLE. mergeStateStatus: BLOCKED reflects the standing changes-requested review, not a conflict.
  • CIlicense/cla ✅ is the only check; main is not branch-protected, so it is treated as the required set. Nothing failing, nothing pending.
  • No inherited reviewer feedback — the only prior review is this reviewer's own, re-checked above; the author's status comment carries no unaddressed ask.
  • Not duplicated workagentAccountsEnabled does not exist on origin/main; no parallel PR landed this.

💥 Breaking changes

No protected surface from BACKWARD_COMPATIBILITY.md §2 is removed or reshaped: the /api/v1 route set is unchanged, agentProfilesResponseSchema keeps editable: z.boolean(), and behavior with the flag unset is unchanged (the pre-existing hosted-mode tests still pass untouched in substance). The hosted refusal text changed on the agent-profiles family and /open gained a distinct one; error strings are not a protected shape here and the only in-repo consumers — the toast and the tests — were updated. editable's meaning widens from "local machine" to "local machine or opted in", which is additive for every existing client.

One thing the new BACKWARD_COMPATIBILITY.md entry should gain alongside the Blocker 1 fix: the default browseRoot is ~/ (workspace/config.ts:184), so an operator who sets CEZ_REMOTE_AGENT_ACCOUNTS=1 without also narrowing CEZ_BROWSE_ROOT is confined to their whole home directory. That is the same posture /api/fs/browse has and the .env.example comment already advises narrowing — but the entry currently reads as if the confinement is meaningful by default, and it is worth one clause saying what it is worth when the root is unset.


🤖 om-auto-review-pr — re-reviewed at PR head 917fd587 (previous review: f3c380c8). Autofix: skipped (not my PR — re-run with --autofix to fix it here).

@pat-lewczuk pat-lewczuk removed their assignment Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

@piotrchabros, all six earlier findings are closed — the open-in menus, the shared localHandoffEnabled predicate, the BACKWARD_COMPATIBILITY.md entry and the two missing tests are all good. Two blockers left before this can go in; full detail in the re-review:

  1. The confinement is half applied. checkProfileDir (server.ts:1947-1955) calls isLexicallyInsideBrowseRoot and stops; the realpath half that POST /api/projects applies at server.ts:2649 — the one fs-browse.ts:101-111 says exists "to still catch symlink escapes" — is missing. I drove the real route on your head with CEZ_REMOTE_AGENT_ACCOUNTS=1: POST {configDir: "<root>/link-to-outside"} where the symlink points at an existing directory outside the root returns 201, exists: true, and the same shape with a dead target returns 201, exists: false. So the existence oracle is back for anything reachable through a symlink under the root, the escaped path is persisted as the account's config dir, and BACKWARD_COMPATIBILITY.md's "rejected before its existence is probed" is not true as written. Mirror server.ts:2643-2654: lexical first, then isInsideBrowseRoot once the path is known to exist, same message for both. Worth a test with a live and a dead symlink, and one through PATCH, which shares the function and has no confinement test today.

  2. npm test is red on this head — and it is not yours. agent-profile-wiring.test.ts:82 fails 1 of 6600 because feat(dispatch): a task may dispatch other tasks (replaces the missions experiment) #972 (af7e8289) added CEZ_API_URL and CEZ_BIN to the base run env without updating the assertion that pins it; it reproduces on af7e8289 itself with none of your work present. main has been red since, and license/cla is the only check so nothing caught it. One line either here or on main — your call — or a maintainer waiver saying main owns it.

Two minors (the localHandoff !== false fail-open on unknown health, and the rejection message echoing the absolute browse root where the sibling route deliberately does not) and one nit are in the review too. The rest of the gate is green on 917fd587: typecheck, build/check:pack, test:unit 35/36, test:package 16/16.

Push the update and re-request review.

@pat-lewczuk pat-lewczuk removed the in-progress Cezar agent is actively working this issue label Sep 14, 2026
@pat-lewczuk

Copy link
Copy Markdown
Collaborator

🤖 om-auto-review-pr completed: CHANGES REQUESTED. Lock released.

Re-review of 917fd587 (review). All six findings from the first pass are closed. Two blockers remain: the browse-root confinement applies only its lexical half, so a symlink under the root escapes it (reproduced against the real route — 201 with exists: true for a live target, 201 with exists: false for a dead one), and npm test is red on this head from a deterministic failure inherited from main (#972 added CEZ_API_URL/CEZ_BIN without updating agent-profile-wiring.test.ts:82; reproduces on af7e8289 itself). Plus two minors and one nit.

Validation on 917fd587: typecheck ✅, npm test ❌ 1/6600, test:unit ✅ 35/36 (1 platform skip), build/check:pack ✅ 497 files, test:package ✅ 16/16. No merge conflicts; license/cla green and it is the only check (main is not branch-protected), so nothing was pending and no CI follow-up is owed. No inherited reviewer feedback beyond this reviewer's own prior pass.

Autofix: skipped (not my PR — re-run with --autofix to fix it here).

@piotrchabros

Copy link
Copy Markdown
Contributor Author

@pat-lewczuk addressed the re-review findings in 7d51d0a:

  • Remote folder validation now checks lexical containment first, then resolves the nearest existing entry. This rejects live and dangling symlink escapes, including missing folders beneath an escaping symlink. POST and PATCH regressions cover these cases and verify rejected paths are not persisted; valid new folders remain accepted.
  • Both containment checks return the same refusal without disclosing the browse root. The compatibility entry now explains that the default root is the host user's whole home.
  • Desktop-open controls stay hidden until health data arrives, while retaining the older-server fallback after a response loads.
  • The default-account environment test explicitly covers dispatch on and off, including its CEZ_API_URL/CEZ_BIN variables. The stub comment is rewrapped.
  • Fixed two test fixtures that deleted their directories without disposing their RunManager. Keeping the original PR's tests alive past the 60-second watchdog reproduced the five teardown errors; the cleanup removes them.

Validation on the pushed head: typecheck passed; Vitest 347 files passed, 6,604 tests passed, 1 skipped, no unhandled errors; unit 36/36; build/check:pack passed; package 16/16. The full suite ran with inherited CEZ_* deployment variables cleared, Linux DAC permission bypass disabled for permission tests, and four workers. The new symlink and unknown-health regressions were confirmed red before the fixes.

Ready for another review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Reviewer requested changes feature New capability needs-qa Requires manual QA before merge priority-medium Ordinary bug or feature risk-high Wide blast radius, review deeply security Security-sensitive

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants