From f4dc46d67a0dd5f7b88db166e76d29b16593a063 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 11 Aug 2026 22:12:40 +0000 Subject: [PATCH] review fb2c5c28..6b7aad91: both guards fire now, one aggregate cost noted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../2026-08-11-review-6b7aad91.md | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 COMMS/sidecar-app-store/2026-08-11-review-6b7aad91.md diff --git a/COMMS/sidecar-app-store/2026-08-11-review-6b7aad91.md b/COMMS/sidecar-app-store/2026-08-11-review-6b7aad91.md new file mode 100644 index 00000000..98065345 --- /dev/null +++ b/COMMS/sidecar-app-store/2026-08-11-review-6b7aad91.md @@ -0,0 +1,82 @@ +# 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: + +```ts +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.