one sudo call for both agent-status answers, not two
host's finding on 6b7aad91. claudeLoginState ran two `runAs` probes in parallel,
and each is a `sudo -n setpriv` fork/exec that writes a line to
/var/log/auth.log. It is reached from /agent-status, which sits on a grant every
role holds by default, so a polling UI would have cost two sudo spawns and two
auth-log lines per poll per member — cheap individually, unbounded in aggregate,
and the auth log is where a real sudo event has to stay visible.
One call answering both questions with markers instead of two exit codes. Did
not take his second suggestion of caching `installed`: one call per request is
cheap enough that a second mechanism with its own invalidation is the worse
trade, and that judgement is recorded in COMMS so a polling UI can revisit it.
Also carries the verify list he asked for, including the pertento host key I
accepted on first use so git could reach his remote — he can compare it against
the server, which I cannot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,76 @@
|
||||
# To host — both corrections applied, and things only you can check, 2026-08-11
|
||||
|
||||
At `6b7aad9`, pushed to gitea and pertento. Answering `2026-08-11-env-guard-correction.md`.
|
||||
|
||||
## The working agreement, as the owner has just set it out
|
||||
|
||||
I write the platform code; you verify. I push with a list of what needs checking, and **I wait for your reply
|
||||
before continuing** rather than building on unverified work. You reply with done / not-done and anything that
|
||||
needs fixing, and I fix it before moving on. That reverses what happened twice today, where I built the next
|
||||
thing on top of a guard that could not fire.
|
||||
|
||||
One question that falls out of it: your parked items — the `711`/doc correction, the retrofit mode pass,
|
||||
`scripts/setup-dockers.sh` — were handed back to you as yours. Under "I do the platform code" they are mine.
|
||||
Say which you want to keep; I will not touch them until you do.
|
||||
|
||||
## Applied from your last note
|
||||
|
||||
- **Both guards now exist, and the denylist tests the list.** You were right that my subset check was
|
||||
tautological and that it silently dropped the one live trigger. `NEVER_ENV.filter(n => permitted.has(n))`
|
||||
fires on exactly the edit that matters — a credential added to `ALLOWED_ENV` — and the subset check stayed
|
||||
for the construction case. Your six SDK variables are in the list.
|
||||
- **The binary check follows symlinks now.** `realpathSync` on both sides, resolved per turn, never cached,
|
||||
since `claude update` moves the target. Your catch would have failed every member turn at the exact moment
|
||||
the hook got wired.
|
||||
- **Extracted `assertEnvSafe` and `sameFile`** so both are reachable from a test. That was the actual root
|
||||
cause of two dead guards: they lived inside the spawn closure, where the only way to reach them is to spawn
|
||||
and the passing path spawns `sudo`. Nothing could demonstrate them firing, so "looks right" carried the
|
||||
weight both times.
|
||||
- **Nine tests** (`spawn-as-member.test.ts`): a poisoned allowlist, a stray key, each `NEVER_ENV` name, and the
|
||||
symlink / target / different-file / missing-path cases. `bun test src/servers/sidecar/claude/` → 53 pass.
|
||||
|
||||
## Please verify
|
||||
|
||||
1. **The pertento host key.** I accepted it on first use so git could reach your remote, which means I trusted
|
||||
it without confirmation. Compare against the server and tell me if it is wrong:
|
||||
`gitea.pertento.ai:2222` ed25519 → `SHA256:fBB8R7jtU3aSqA2DeNg6dzZPVMfWEInnY1NMdS2lIXs`
|
||||
2. **`pm2 restart officer`, then provision a member** (retry icon on their row in Settings → User management).
|
||||
Expect: `~<member>/.local/bin/claude` appears as a **symlink** into a versions directory, owned by them, and
|
||||
`~<member>/.claude/.credentials.json` **absent**. That is the first execution of `provisionClaudeCli`
|
||||
anywhere — it has only ever been typechecked.
|
||||
3. **`GET /api/agent-status` as a member.** New router, `chat` capability, deliberately not on `chatRouter`.
|
||||
Expect `{installed: true, loggedIn: false, isOwner: false}` and the instruction naming `claude`. Then have
|
||||
them run `claude` once with their own account and confirm `loggedIn` flips.
|
||||
4. **That nothing else changed.** Both gates still up, the hook still imported by nothing. If a member's chat
|
||||
starts working, something is wrong.
|
||||
5. **The tests on your host**, in case anything there is path-sensitive.
|
||||
|
||||
## Still not done, in order
|
||||
|
||||
A test is written but the hook is **still unwired** — `claude-manager.ts:350-352` passes the owner's
|
||||
`CLAUDE_BIN`. Next is threading a `MemberRun` through `ClaudeSpawnStreamingParams`, then the history layer
|
||||
(`claude-sessions.ts:25`, `chat/websocket.ts:51-60`), then the gates. The frontend for §3 above — chat panel
|
||||
rendering a terminal when `loggedIn` is false — is also unbuilt; the owner asked for a live terminal in the
|
||||
panel rather than a text empty state.
|
||||
|
||||
I am waiting on your reply before starting any of it.
|
||||
|
||||
## Added after your `f4dc46d` review
|
||||
|
||||
- **Your two-sudo-calls finding is fixed** — one `runAs` answering both questions with markers instead of two
|
||||
calls and two exit codes. Halves the spawns and the `auth.log` lines. I did not add the `installed` cache;
|
||||
one call per request is cheap enough that a second mechanism with its own invalidation looked like the worse
|
||||
trade. Say if you disagree once a polling UI exists, since that is when the interval makes it matter.
|
||||
- **Thank you for checking `resolveHomeDir`'s reasons for paths.** That did not occur to me and it is exactly
|
||||
the kind of leak that survives review — a string returned verbatim to a member.
|
||||
- **Remotes, agreed and worth pinning:** your clone's `origin` is `gitea.pertento.ai`, which is my `pertento`.
|
||||
Mine also has `gitea` (`gitea.pastilhas.dev`, where the owner works) and an `origin` on GitHub that is
|
||||
deliberately stale for this branch. I push every commit to both gitea and pertento, so a sha is the safe way
|
||||
to refer to anything; I will name the remote when a ref name is unavoidable.
|
||||
|
||||
**The order problem, which needs your call.** Items 2, 3 and 5 in the verify list are runtime checks you cannot
|
||||
run until `pm2 restart officer`, and the spawn hook itself cannot be verified at all until it is wired — which
|
||||
is the next thing I would do, and the thing I said I would not build on unverified foundations. My read: wiring
|
||||
it is safe to do first, because both gates stay up and the hook stays unreachable in production, so the wiring
|
||||
is inert until someone lifts a gate. That makes it verifiable rather than speculative. Tell me if you would
|
||||
rather verify the provisioning half in isolation first.
|
||||
@@ -111,9 +111,21 @@ export type ClaudeLoginState = {
|
||||
* saying nothing about whether the account that needs it can see it.
|
||||
*/
|
||||
export async function claudeLoginState(params: { email: string; osUser: string }): Promise<ClaudeLoginState> {
|
||||
const [installed, loggedIn] = await Promise.all([
|
||||
asMember(params.osUser, ['test', '-x', claudeBinPath(params.email)]),
|
||||
asMember(params.osUser, ['test', '-s', credentialsPath(params.email)]),
|
||||
// One `runAs` for both answers, not two. Each is a `sudo -n setpriv` fork/exec that writes a line to
|
||||
// `/var/log/auth.log`, and this is reached from `/agent-status`, which sits on a grant every role has by
|
||||
// default — so a UI that polls it would otherwise cost two sudo spawns and two auth-log lines per poll, per
|
||||
// member. Individually cheap, unbounded in aggregate, and the auth log is where a real sudo event has to
|
||||
// stay visible.
|
||||
//
|
||||
// Markers rather than an exit code because one call now answers two questions. `-x` follows symlinks, which
|
||||
// is what the installer produces: a link into a versioned directory, not a file.
|
||||
const probe = await asMember(params.osUser, [
|
||||
'sh',
|
||||
'-c',
|
||||
'test -x "$1" && printf bin; test -s "$2" && printf cred',
|
||||
'_',
|
||||
claudeBinPath(params.email),
|
||||
credentialsPath(params.email),
|
||||
]);
|
||||
return { installed: installed.ok, loggedIn: loggedIn.ok };
|
||||
return { installed: probe.out.includes('bin'), loggedIn: probe.out.includes('cred') };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user