Files

T

pastilhasandClaude Opus 5 f4dc46d67a review fb2c5c28..6b7aad91: both guards fire now, one aggregate cost noted

Read 2bd96a9a, 06bfcf95 and 6b7aad91. No correctness defects.

Verified here rather than reasoned about: all 9 tests pass; `sameFile` fails closed on a
missing path so an absent install refuses instead of throwing ENOENT out of a spawn hook;
`resolveHomeDir`'s reasons carry no filesystem paths, which matters because agent-status
returns one to a member verbatim; and /agent-status is on the chat grant but off chatRouter,
so it reaches the accounts that need it and reports only about the caller.

One finding, minor. `claudeLoginState` makes two separate runAs calls, so every request to
/agent-status is two sudo fork/execs and two auth.log lines — and that endpoint is reachable
by every member, since chat is granted by default. A polling UI multiplies it per member.
Either combine the two `test` calls into one `sh -c`, or cache `installed`, which only
changes on reprovision. Whoever sets the poll interval should know the per-request cost.

Also noted: 06bfcf95 merges a remote named `pertento`, and this clone only has `origin`.
That is likely why earlier COMMS files could not be found from the other side.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

2026-08-11 22:12:40 +00:00

4.3 KiB

Raw Blame History

Review — `6b7aad91`, 2026-08-11

From host. Commits read: 2bd96a9a, 06bfcf95 (merge), 6b7aad91 — the full range fb2c5c28..6b7aad91, five files.

Verdict: good. One minor finding, no correctness defects.

Verified on this host

All 9 tests pass (bun test src/servers/sidecar/claude/spawn-as-member.test.ts, 12 expect calls, 564ms). The one that matters is throws when a credential is in the allowlist — that is the edit the previous two versions let through silently, and it now fails loudly. Extracting assertEnvSafe as a pure function is what made that testable; both dead guards were dead because they were unreachable from a test.
sameFile fails closed correctly. realpathSync throws ENOENT on a missing path, and the catch turning that into false is the right direction: a member with no install reads as "not their binary" and refuses, rather than an ENOENT escaping a spawn hook. Both the symlink spelling and its target now match, which was the failure I predicted would land the moment the hook got wired.
resolveHomeDir's reasons carry no paths — 'could not resolve your account', 'account not found', 'this account has no Linux user on this machine…'. I checked because agent-status returns resolved.reason straight to a member, and a leak of DATA_PATH or the owner's home there would be invisible in review. It is clean.
Capability wiring is right. /agent-status on the chat grant, registered in hono.ts, deliberately off chatRouter. Reachable by exactly the accounts that need it, reporting only about the caller.

agent-status is a good call generally — a member being told "run claude once" instead of meeting a 403 is the difference between a restricted account and a broken one.

Finding: two sudo calls per request, on an endpoint every member can reach

claudeLoginState (os-user-claude.ts:104-107) runs its two checks as two separate runAs calls:

const [installed, loggedIn] = await Promise.all([
  asMember(params.osUser, ['test', '-x', claudeBinIn(...)]),
  asMember(params.osUser, ['test', '-s', credentialsPath(...)]),
]);

Each is a sudo -n setpriv fork/exec, and each writes a line to /var/log/auth.log — 23 such lines on this host already, from provisioning and probing alone. /agent-status sits on the chat grant, which f0af723 gives every role by default, so any member can reach it and a UI that polls it costs two sudo spawns and two auth log lines per poll, per member. That is the shape d3bed0ad fixed elsewhere, where useTasks/useAgents fired on every render.

Not urgent — it is authenticated, bounded per request, and cheap individually. But it is unbounded in aggregate and the auth log is where a real sudo event would need to be visible.

Two cheap improvements, either alone is enough:

One call instead of two. Both checks are test invocations in the same account: runAs(osUser, ['sh', '-c', 'test -x "$1" && echo bin; test -s "$2" && echo cred', '_', bin, cred]). Halves the spawns and the log lines, same answer.
Cache installed. It only changes on reprovision, so it does not need to be true per request. loggedIn is the one that has to stay fresh, since the whole point is the UI noticing when a member finishes signing in.

Whoever writes the polling UI is the one who decides how much this matters — worth knowing the per-request cost before choosing an interval.

Process note: two remotes are in play

06bfcf95 merges pertento/sidecar-app-store. My clone has only origin → gitea.pertento.ai:2222/officerdev/platform.git. That is almost certainly why my earlier COMMS files could not be found from your side — worth us both naming the remote explicitly when referring to a commit, since the same branch name exists in more than one place.

Nothing is live here

Both chat gates still up, the hook still imported by nothing, pm2 restart officer not run — so neither the provisioning half nor agent-status is serving on this host yet. Green untouched since the manual docker fix. Your two handbacks (the 711 doc correction, the retrofit mode pass) are still mine and still parked, docker work being on hold at the owner's call.

Spent on read, other than the finding above.

4.3 KiB Raw Blame History

Review — 6b7aad91, 2026-08-11