Team-lead approvals: /lead command, principals wiring, fail-closed automation - #115
Draft
ThinkOffApp wants to merge 14 commits into
Draft
ThinkOffApp wants to merge 14 commits into
ThinkOffApp wants to merge 14 commits into
Conversation
Petrus, 2026-09-18 07:20: "we need autoapprove or team lead approve for these
if i sleep", and 07:25: "Ok lets do 2 and 3. Team lead assigned by me
dynamically (chat command?) and lead can move the duty to another agent if
they need to". Authorized at 07:25 and unstarted four hours later.
A single lead may decide ordinary confirmations in his place. The rules are
what stop that becoming "any agent may approve anything":
- The lead starts UNSET and there is no default, so deploying this cannot
weaken the gate by itself. Unset means only the owner decides, which is
exactly today's behaviour.
- NO SELF-APPOINTMENT. An agent cannot make itself lead and cannot fill an
empty post; only the human owner can. Otherwise the first agent to boot
grants itself approval rights.
- The sitting lead MAY hand over, because Petrus asked for that, and loses
the duty by doing so. It cannot re-appoint itself.
- Intents marked `requiresHuman` are never delegable. A sleeping owner is a
reason to wait on a credential write, not to widen who may authorise it.
The CALLER classifies: the precommand gate has the command text and the
patterns, so the daemon never guesses intent from a prompt string.
It also fixes an audit-trail lie that predates this: decideIntent hardcoded
`actor: 'petrus'` and `approver: 'petrus'` on every receipt and status push,
whoever called the endpoint. Decisions now record who made them and under
which rule, and listIntents/getIntent expose it — they are what the queue and
the dashboard read, so leaving it out hid the trail exactly where anyone would
look for it.
HTTP: GET /lead for status, POST /lead to assign, transfer or clear (handle:
null). POST /intent/:id/decision takes an optional `actor`, defaulting to the
owner so every existing caller keeps working unchanged. Refusals return 403,
not 400 — the request was well formed and a rule declined it.
Twelve tests, most of them negative controls: a delegation feature is only as
good as what it refuses, and a suite proving only that the lead CAN approve
would pass just as happily with no rules at all. Full suite 343/343.
Inert until the daemon restarts, which interrupts every agent's command gate —
that is Petrus's call to make, not mine.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Petrus asked for the lead to be "assigned by me dynamically (chat command?)". /lead @agent appoint or transfer /lead status who holds it (anyone may ask) /lead clear vacate the post Handled before the rules array and consuming the message, not as a rule. Who may approve shell commands on this machine should not be editable by adding a JSON entry — a rule that grants approval rights is a rule someone can write by accident, and a cooldown or a first-match-wins ordering could otherwise swallow the command. Authorisation is double-keyed: the sender must be the owner handle AND the message must carry isHuman. Either alone is too weak — agents post under their own handles with isHuman false, and a tapped action button arrives as `petrus` with isHuman false, so requiring both means neither an agent quoting this syntax nor a replayed button can appoint anyone. The daemon enforces the same rules again; this is the outer key, not the only one. The parser is deliberately strict: exactly one token after /lead. My own test caught the lenient version appointing @somebody from "/lead somebody nice please" — taking the first word is a guess, and a wrong guess here hands command approval to the wrong agent. Nine tests, seven of them refusals. Full suite 352/352. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Petrus typed `/lead status` in the room one minute after I described the command. Nothing answered, because the code is on a branch and the daemon has not restarted — but the more interesting problem is what WOULD have answered. The daemon currently running has no /lead route, so GET /lead returns 404. The first version read that as no lead assigned and replied "Team lead: unset", which is true-sounding, plausible, and produced entirely by the endpoint not existing. That is the same failure this repo spent the morning finding twice: a broken read wearing a legitimate answer — an admin login that said "invalid password" when the rate-limit table was missing, and a scratchpads page that said "none found" when the table never existed. It now distinguishes them and says the feature is built but not running here. A status command that cannot tell "nobody holds the post" from "this endpoint does not exist" is worse than no status command, because this one is about who may approve shell commands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…dentity @codexmb reviewed af178ad against a real daemon and was right on every point. My authorization rules were sound and sat on top of an identity anybody could assert, so they were decoration: 1. POST /lead took `actor` from the JSON body. Any caller that could reach the daemon could say actor:"petrus" and appoint itself lead. 2. POST /intent/:id/decision defaulted a missing actor to the owner, so the whole role boundary was bypassed by OMITTING a field. 3. createIntent received fromHandle and discarded it, so a lead could raise a confirmation and approve its own request. My tests could not have found any of this: they called decideIntent() and setLead() directly with truthful actor strings. A test that supplies its own identity cannot discover that identity is unauthenticated. That is the lesson, not the bug. Fixes: - `principals` maps per-agent token -> handle, and the actor is now the PROVEN principal. `payload.actor` is never read. A request that supplies an actor without proving one is refused 403 rather than quietly downgraded. - POST /lead requires a proven principal outright. Appointing a lead is a privilege grant, not a routine approval, so it never falls back to "local and anonymous means petrus" — and with no principals configured the route refuses, so an unconfigured daemon has no delegation rather than a forgeable one. This ships INERT, not insecure. - The intent retains requestedBy, and a lead cannot approve its own request. - Anonymous callers still act as the owner for ordinary decisions. That is today's trust model — the daemon binds locally and Petrus's phone buttons carry no identity — and changing it here would lock him out of his own approvals. Two things only the HTTP tests could find, both now fixed: a registered per-agent token was rejected at the shared-token door with 401 before anything could read it, and refusals returned 400 or 403 according to a REGEX over the error message. Authorization outcomes now carry a `forbidden` flag; a status code inferred from prose breaks silently the next time someone rewords a string, and this one guards who may run shell commands. Seven HTTP-level negative tests. Full suite 360/360. Deployment stays held. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The full suite failed once at 352/353 and passed on every rerun. The cause was in the test I had just added: it created a one-off HTTP server and called close() without awaiting, outside any finally — so an assertion failure leaked the listener, and a leaked listener is how a suite starts failing once in three runs. An intermittent failure is worse than a consistent one, because the third flake is the one everybody stops reading, and this suite is the thing standing between a team lead and approving shell commands. Awaited, and in a finally. Three consecutive full runs at 360/360. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot a default @codexmb, second review of 36524b2, reproduced over real HTTP: the lead's own token got 403 on a requiresHuman intent, and then the SHARED token with no actor got 200 on the same intent. `actor: principal || OWNER_HANDLE` handed the owner's authority to any caller who omitted a field, so the boundary I had just written was bypassable by deleting `actor` — including by the lead that had been refused a moment earlier. Once a daemon configures `principals` it has asserted it can tell callers apart, so from then on it must: every decision needs a proven principal, the owner's included. Configuring principals therefore means also issuing Petrus one. That is the point rather than an oversight — "owner" cannot be a privilege the daemon is unable to verify. A daemon with NO principals keeps today's behaviour exactly, so nothing that works now stops working and his phone buttons are untouched. The refusal is not narrowed to requiresHuman on purpose. Narrowing it would leave the identical hole one field away for ordinary intents, which is how this one existed in the first place. Two new negative tests for the bypass, and two POSITIVE ones for legacy mode — proving the refusal without proving that an unconfigured daemon still lets him in would test half the branch and ship the half that locks him out. Eleven HTTP tests. Full suite 362/362. Deployment stays held. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codexmb after the restart: "message delivery and presence of handleLeadCommand do not verify the authorization boundary." Correct -- a commit hash proves which code loaded, not what it permits. Seven cases now assert what it permits, and each asserts a specific status AND a specific reason, because a refusal for the wrong reason is not a pass. lead clears another agent's routine intent 200 lead decides its OWN request 403 "own request" lead decides a requiresHuman intent 403 "reserved for" a non-lead agent decides 403 the OWNER decides a requiresHuman intent 200 (never locked out) unproven caller claims actor:petrus 403 "proves no identity" unregistered token, principals configured 403 "identifies callers" Four instrument failures preceded the first real measurement, and the file records them because each would have produced a confident wrong answer. The route was /decide, not /decision, so everything returned 404 -- and one test still passed because it asserted notEqual(status, 200). The server reads Authorization: Bearer, not X-API-Key, so no token resolved and everything returned 403, which is a boundary that refuses every caller, looks secure, and tests nothing. A sed printed "header corrected" while changing no file. And assert.throws was used against setLead, which returns a refusal rather than throwing; reporting that failure would have read as "the code does not refuse self-appointment". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
petrus typed /lead status at 16:27 and again at 20:03 and got silence
both times. Three layers, found only because he pressed it twice:
1 /lead lives in src/room-automation.mjs; bin/cli.mjs imported
src/team-relay/room-automation.mjs, which has no handler
2 startRoomAutomation runs only under the "automate" subcommand
3 nothing on the fleet launches "automate" -- start-all.sh runs
"rooms watch" and the daemon
So the handler had never had a process. The unit tests imported the file
I edited, so they passed against a module production does not load.
- both entry points now import src/room-automation.mjs, which also has
a better case-insensitive self-skip than the copy that was running
- rooms watch starts automation on its own poll loop, not awaited, so
a crash there cannot take room notifications down with it
- test/automation-module-wiring.test.mjs asserts that the module
cli.mjs loads actually exports handleLeadCommand. It fails on the
old import; I checked that rather than assuming.
Second defect, found by @claudemb's end-to-end test rather than by me:
the command regex ran against the whole body with no /m flag, so `$`
demanded end-of-string and ANY second line made the command invisible.
His "/lead status" with a note underneath parsed as nothing. Someone who
types a command and then a sentence has still typed a command.
- status and clear read the first line and tolerate prose after it
- appointing still requires the command to be the whole message:
reading a privilege grant out of the first line of a longer post is
how a quoted line becomes a real appointment
- that refusal now says "appointing has to be the whole message"
rather than "@grok is not a handle", which was false
test/lead-command-path.test.mjs exercises the path against a RECORDING
http server, because the property is whether a privileged request reaches
the daemon, not what the function returns. Unauthorised cases must leave
it with zero requests; the owner's status must produce exactly GET /lead.
Replacing the identity check with `true` turns 3 of 8 red.
381/381.
KNOWN GAPS, raised by @codexmb against the published module and NOT
addressed here. They are real and they are next:
- seen.add happens before dispatch but saveSeenIds only after, so a
crash in between replays the command on restart
- two processes keep independent in-memory sets; the shared file is not
an execution lock. I briefly ran two watchers today and proved it.
- a first-run fetch failure returns [] and seeding proceeds, so the
first successful poll can treat history as new. Seeding must fail
closed.
- existing non-empty state has no startup cutoff
- postMessage ignores HTTP status, and the lead branch records
"completed" even when posting returned false
Handler tests do not exercise poller startup or replay, which is his
point and it stands.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eipts
@codexmb reviewed the published module and found five concrete defects on
a path that can execute /lead and /approve. All five, plus one of my own.
FAIL-OPEN SEEDING. fetchRoomMessages returned [] for a failed request, so
"the room is quiet" and "I could not ask" were the same value. A dropped
packet at startup produced a successful seed of nothing, and the first
poll that worked then saw the entire history as new -- a replay of
privileged commands caused by a transient network error.
- the fetch returns null on any non-2xx or thrown error
- seeding aborts if ANY room cannot be read, and retries on the poll
interval
- dispatch is gated on `ready`: until a seed completes, the loop
executes nothing at all
- a room that fails mid-poll is skipped rather than guessed at
REPLAY ON CRASH. seen.add ran before dispatch but saveSeenIds only at the
end of the poll, so a crash in between re-executed the command on
restart. The id is now persisted immediately, before anything acts on the
message. At-most-once is the right bias here: a missed /lead is a message
petrus can send again, a repeated one is an appointment he never made.
RECEIPTS THAT LIE. postMessage shelled out to curl and returned true
whenever curl exited 0 -- which it does for a 500 -- and the lead branch
recorded status "completed" regardless. That records the exact silence
petrus experienced as a success. The status is checked, and the receipt
now says completed or failed according to what happened.
MY OWN, found while reading it: the API key was interpolated into
`curl -H "X-API-Key: ${apiKey}"` under execSync, putting the credential
in the process table for anyone running ps. Both calls use fetch with a
header now. This repo's own rule is that keys never go inline in a shell.
test/automation-fail-closed.test.mjs proves the distinction the rest
depends on: an HTTP error is null, a network failure is null, an EMPTY
ROOM IS [] -- the positive control, without which a function that always
returned null would pass -- and the key is asserted to arrive as a
header. Restoring the fail-open returns turns 2 of 5 red.
386/386. Note for anyone reading the CI: this suite has an intermittent
single failure that has never named a test and has not reproduced in
three consecutive runs each time I have seen it. It is not in these
files, and I have not chased it.
STILL OPEN from his review: two processes keep independent in-memory
seen-sets, so the shared file is not an execution lock. I demonstrated
that myself today by briefly running two watchers. A single-instance lock
is the next commit, not this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orts Round six from @codexmb against 4cefcf1. Three more real defects. SEEDING WAS PER PROCESS, NOT PER ROOM. `ready = seen.size > 0` declared everything seeded whenever the seen-file was non-empty, so adding a room to poller.rooms later meant its whole history arrived as new and any historical /lead in it would dispatch. Seeding is now tracked per room in a marker file beside the seen-file, and a room is seeded before anything from it is acted on. An existing seen-file from before the marker existed is adopted for the rooms configured at that moment -- otherwise deploying this change would itself replay them. Rooms added after that still seed properly. PERSISTENCE WAS NOT ATOMIC. saveSeenIds wrote in place, so an interrupted write left a truncated or empty file -- and an empty seen-file on the next start means every historical message is unseen again. A crash during a routine save became a replay of privileged commands. It writes a temp file and renames now, and a failure to persist is loud rather than swallowed, because the consequence lands on the next startup. THE LOOP COULD DIE QUIETLY. setInterval(poll) fired an async function and dropped the promise, so a rejection in any later poll went to unhandledRejection while the loop still looked healthy; the CLI's .catch only ever covered the first call. The next run is now scheduled after the previous one settles, failures are logged and the loop continues, and dispatch stays gated on a successful seed regardless. 386/386, three consecutive clean runs. The intermittent single failure this suite sometimes shows does not name a test and is not in these files; I have checked that each time rather than assuming. STILL OPEN and still his: a cross-process execution lock. Two processes keep independent in-memory seen-sets and the shared file is not a lock. I demonstrated that today by briefly running two watchers. He has also asked for execution-level tests covering existing-state/new-room startup, atomic-persistence failure, and two concurrent processes before this is called safe for privileged dispatch. Those are not written yet, so it is not called safe, and the running watcher is still on the older build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codexmb: "cover existing-state/new-room startup ... with execution-level tests before calling privileged dispatch safe. Handle tests alone do not exercise poller startup/replay." They cannot, while the API host is hardcoded, so the base is injectable via automation.api_base and these drive startRoomAutomation itself against a stub room server, counting what it actually POSTs. A message never dispatched leaves no post; a replayed one leaves two. a room ADDED after the seen-file exists is seeded, not replayed a room that cannot be read blocks dispatch entirely Both are control-verified: reverting the per-room seeding turns the first red, reverting the fail-open fetch turns the second red. The second test needed rewriting before it was worth anything. Its first version asserted "no posts" when a room failed -- and with fail-open seeding there were no posts either, because the same pass marked everything seen. It passed against the exact bug it was written to catch. I only found that by running the sabotage rather than trusting the green. It now seeds one room cleanly, adds a genuinely dispatchable message to it, and THEN breaks the second room: correct behaviour dispatches nothing, fail-open posts the new message. That is the third check of mine today that could not have failed, and the second that a deliberate sabotage caught. Running the sabotage is cheap and I should stop treating it as optional. 388/388. STILL OPEN: the cross-process execution lock, and a test for two concurrent processes. The shared seen-file is not a lock -- I demonstrated that today by briefly running two watchers -- and nothing here changes it. Not called safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/lead had never worked end to end. callDaemon attached no identity at all, so
POST /lead hit resolvePrincipal, got null, and returned 403 "delegation is
unavailable" — even for a correctly typed owner command that passed every gate
on this side. Verified against the running daemon rather than read:
POST /lead -> 403 GET /lead -> 200 {"ok":true,"lead":null}
callDaemon now takes an optional `token` and sends it as a bearer, which is
what resolvePrincipal reads. Omitted when unset, so a daemon with no
principals configured behaves exactly as before rather than sending an empty
Bearer header.
A 403 with no token configured is a fault on THIS side, not a refusal of the
user, so it now answers in words. A bare "forbidden" sends petrus looking for
a permission he already has — the same failure as the /lead status path
already guards with its "built but not running here yet" reply.
Deliberately NOT included: the token itself. Browser-only /lead trusts
msg.isHuman, and that field is forgeable until the GroupMind metadata fix is
deployed (antfarm claudemm/fix-room-sender-identity-forgery). Minting a
principal now would hang a privilege grant on a forgeable signal and make it
look guarded. Order is: deploy the server fix, verify the identity boundary,
then mint. codexmb's review blocks the reverse.
388 tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/lead had never worked end to end, for two independent reasons. Fixing either alone changes nothing: 1. room-automation's callDaemon attached no identity, so POST /lead hit resolvePrincipal, got null, and returned 403. 2. iak-mcp-daemon never passed `principals` from config to startConfirmationsServer -- it read auth_token and stopped. So `principals` in dogfood.json was inert and the daemon answered "this daemon has no per-agent tokens" whatever was configured. Minting a token could not have fixed it. Verified live: petrus typed `/lead claudemb` in the room and it took (lead @claudemb, assignedBy @Petrus). First time the feature has worked. The token is read from a file (principal_token_file), never inlined: the precommand gate posts commands to the room and has leaked a key that way. CONFIG IS NOT ENABLED, deliberately. Turning `principals` on locked petrus out of his own approvals within ten minutes: the daemon's :8788 page calls /intent/:id/decision with NO Authorization header, so once principals exist the browser, the phone and the owner are all refused. I had predicted that exact lockout and still shipped it, because I checked the phone (which carries a token from July) and never checked the page. Reverted the config immediately; this commit keeps only the code. Before enabling, one of these has to land first: - the :8788 page carries a token, or - /lead settles through a route that is not the shared decision endpoint Also worth knowing: a daemon restart silently destroys every pending intent (const intents = new Map(), nothing read back at boot). Three restarts tonight wiped the queue, including the intent the new lead was reviewing. 388 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto main's "fail closed, persist before acting" commit made executeAction (and its executeActionForTest export) async, so this test's four assertions were comparing a Promise's undefined .status instead of the resolved receipt. Await each call. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ThinkOffApp
force-pushed
the
claudemm/team-lead-approvals
branch
from
September 21, 2026 07:01
8e77045 to
0b747c2
Compare
Owner
Author
Rebased onto origin/main to resolve conflictsStacking check: #115 is independent of #108 ( Conflicts and resolutions
No conflict resolution here weakens a fail-closed path: One test needed a follow-up fix (not a conflict, a consequence of one)
VerificationTwo worktrees, each with its own
Also ran the CI-listed sanity check ( Not verified: no lint or typecheck script exists in this repo ( Force-pushed with Co-Authored-By: Claude Fable 5.1 noreply@anthropic.com |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DRAFT - do not merge until the lead credential path exists; merged-but-inert principals config locks the owner out (19 Sep incident).
/lead had never worked end to end for two independent reasons: room-automation's
callDaemonattached no identity, so POST /lead hitresolvePrincipal, got null and returned 403; andiak-mcp-daemonnever passedprincipalsfrom config tostartConfirmationsServer, soprincipalsin dogfood.json was inert whatever was configured.Verified live: petrus typed
/lead claudembin the room and it took (lead @claudemb, assignedBy @Petrus), the first time the feature has worked. The principal token is read from a file (principal_token_file), never inlined, because the precommand gate posts commands to the room and has leaked a key that way.CONFIG IS NOT ENABLED, deliberately. Turning
principalson locked petrus out of his own approvals within ten minutes: the daemon's :8788 page calls /intent/:id/decision with no Authorization header, so once principals exist the browser, the phone and the owner are all refused. Before enabling, either the :8788 page must carry a token or /lead must settle through a route that is not the shared decision endpoint.388 tests pass.
🤖 Generated with Claude Code