fix(cli): an attached agent can name itself, and find its own kild - #675
Merged
Conversation
Two failures with one cause: the CLI knew nothing about the harness session
it runs inside.
Attribution. `attach` mints a bearer token and the engine writes that handle
as `from` when it is presented. The CLI took the token, printed it under
--json, and threw it away — so every send from an attached harness landed as
the unattributed label, indistinguishable from the operator typing and from
every other attached agent in the kild. `send` now mints through the
idempotent attach at call time and presents it. The token is deliberately not
stored: it lives in engine memory and dies with it, so a copy on disk would
outlive the thing it names and be rejected as unknown.
Discovery. An attached agent was reachable only if KILD_KILD_ID was in the
environment, and a process's environment is fixed at exec time — so a session
could not attach to a kild opened after it started, which is the normal case.
A resumed or forked session is worse: its environment is rebuilt from a
harness settings file no shell owns. `attach` now records {kild, handle}
against the session id, and inbox/send resolve it.
The record outranks the environment, which is the opposite of the obvious
ordering. A settings file is re-read on every start including resumes and
cannot be cleared from a shell, so a stale entry naming an archived kild is
invisible and drains nothing forever while reporting success. An attach is a
deliberate act naming a kild that existed; ambient config loses to it.
KILD_KILD_ID still works as the fallback.
Attaching supersedes any other session holding the same (kild, handle), so
forks cannot accumulate a record per relaunch and two sessions can never race
on one destructive inbox.
An agent the engine spawned is guarded out of all of it. The engine sets
KILD_KILD_ID and KILD_HANDLE on owned agents too, so without the guard a send
would have attached a shadow copy of the agent over its own handle.
Two things found by running it rather than testing it:
- `harnessSession` was a const arrow reached by `dispatch()` before
initialisation, which broke `kild attach` outright. It is a function
declaration now.
- cli.attached.test.ts inherited the ambient environment, so the suite
resolved the operator's own attachment and passed or failed depending on
whose machine it ran on. The harness scrubs identity vars and redirects
KILD_HOME to a temp dir; tests opt into an identity by name.
Gates: 400 tests, typecheck, lint, e2e 70/70. Verified live against the
running engine: attach, an attributed send, and a drain resolved with
KILD_KILD_ID and KILD_HANDLE unset.
Three real defects, all in the new slice. A concurrency bug that lost data. `recordAttachment` wrote its record and then scanned the directory deleting every OTHER record for the same (kild, handle). Two sessions attaching at once each saw the other's freshly written record as somebody else's and deleted it, leaving NEITHER attached rather than the last one — reproduced at 197 of 200 trials. That is exactly the fork case the record exists to serve. The write and the scan now run under a lock (an atomic mkdir, the smallest exclusion available without a dependency), with a stale-lock break so a crashed attach cannot wedge every later one. Failing to lock skips superseding and says so; the record itself is still written. A silent failure that reintroduced the very bug this branch fixes. The `.catch(() => null)` in resolveAttachment was written for the turn-end hook, which must degrade to silence — but resolveAttachment is also what `kild send` resolves through, so an unreadable record made a send post with no credential: exit 0, "Sent to the kild.", and the message on the log as `human`. Indistinguishable from a correctly attributed send. The swallow moves to the one caller entitled to it, matching the shape kildInbox already used for drainInbox. Underneath it, readRecord caught every fs.readFile failure as "not attached", so EACCES or an I/O error read as "this session never attached". Only ENOENT means that now; unreadable and malformed throw, and each caller decides. Docs that contradicted the code. The hook header and attached-agents.md still said KILD_HANDLE "defaults to claude" after this branch removed the default. Someone following them would have got a hook that silently did nothing. forgetAttachment is deleted rather than wired up. Nothing called it: it was a documented, tested function with no caller, and the cleanup story it implied (what happens to records when a kild stops) is not designed — it would need to drop every session's record for a kild, not one session's. Its tests go with it. A stale record already degrades correctly to silence. Gates: 405 tests, typecheck, lint, e2e 70/70. Verified live again after the change: attach, no lock left behind, and a no-argument drain still resolving.
The lock did not hold, and verification broke it two ways. Its staleness was measured from the waiter's own arrival rather than the lock's real age, so a slow-but-alive holder had its lock stolen; and the release was an unconditional rm with no ownership check, so that holder then deleted a lock somebody else legitimately held. Two critical sections could overlap — a narrower version of the bug the lock was added to fix. Worse, the give-up path wrote nothing at all. Both the record write and the supersede scan sat inside the lock callback, so failing to acquire skipped both, while `kild attach` still printed success from the engine's reply. An operator could be told "attached" with no local record and every later send and drain would behave as if the session had never attached. The doc comment and the previous commit message both claimed "the record itself is still written". That was simply false. Rather than add lock-age stat, an ownership token and a degraded write path, this drops the lock. Who holds a handle is now one file written by atomic rename: attached/<session>.json what this session attached to attached/claims/<kild>/<handle> which session currently holds that handle A session resolves only if the claim still names it. Concurrent attaches produce exactly one winner because rename is atomic and has no observable intermediate state — no timeout, no staleness heuristic, no acquisition that can fail. Kild and handle are separate path segments so no separator inside either value can make two pairs collide on one filename. Bounded growth is kept by a sweep that removes records the claim no longer names. It is safe under any race because it only deletes records that already resolve to null, and it runs after the attachment is complete, so a failure costs disk and never an answer. Verified by stress rather than by argument: 200 trials of 8 sessions racing one handle give exactly one attached, 200 times — no losses, no duplicates. The scan-and-delete this replaced lost both records in 197 of 200. Gates: 408 tests, typecheck, lint, e2e 70/70. Dogfooded live: a record whose claim is missing correctly reads as not attached, re-attach writes the claim, and a no-argument drain then resolves and delivers.
…ents Two defects verification reproduced in the atomic-claim rewrite. The sweep could delete a live record. It decided from a snapshot it had already read but unlinked the live path, so an ordinary re-attach racing any OTHER session's sweep removed a record whose claim still named it. That leaves a session holding a claim with no record, which resolves as "not attached", which makes the next `kild send` go out with no credential — the exact failure this branch exists to prevent, reached a third way. Every attach swept the whole directory, so the window was not rare. The sweep is removed rather than repaired. It was never a correctness mechanism — the claim is — and a race-free delete needs rename-aside plus a link-back protocol to buy nothing but disk. Superseded records now stay: one small inert file per forked session. Nothing scans this directory (every read is a direct path lookup), so the cost is bytes, never latency and never a wrong answer. Reclaiming them belongs in an explicit maintenance verb, not in an implicit delete running on somebody else's attach. `safeSegment` accepted `.` and `..`. The character class allows `.`, so the traversal tokens passed the check whose whole job was blocking traversal, and `path.join` collapses `..` against the preceding segment — so any (kild, `..`) pair targets the shared `claims` directory itself. One such call turned that directory into a plain file, after which every attach failed ENOTDIR for good. Both tokens are now refused by name; dots inside a value still work, because worktree names carry them. Verified rather than argued: the reported attack is blocked at all four positions, the claims directory is never created, and a normal attach still works afterwards. 200 trials of 8 sessions racing one handle still give exactly one attached. One test changed meaning and is worth naming: "forks do not accumulate records" asserted a file count, which is no longer the invariant. It is now "only the newest fork resolves, however many came before it" — the property that actually matters, and the one the claim provides. Gates: 414 tests, typecheck, lint, e2e 70/70.
Final verification cleared the invariant — no interleaving or crash point resolves a session to a kild it does not hold, and two sessions never both resolve for one handle — and found one real gap that does not touch it. On a case-insensitive filesystem, which is the macOS default, two handles differing only in case are distinct agents to the roster but fold to one claim file. The second attach overwrites the first, and the first session then reads as not attached. It fails closed rather than to somebody else's identity, so it cannot mis-attribute, but it does go quiet. Not fixed here, and the reason matters: the durable fix is for the engine to refuse case-variant handles when an agent joins the roster, since handles that differ only in case are ambiguous to a human reader too. Encoding around the collision in the storage layer would leave the ambiguity in place and hide it. That is a change to agent admission, not to this file, and it does not belong in a branch already rewritten three times. Written down in both places a reader would look.
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
1 participant
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.
Two failures with one cause: the CLI knew nothing about the harness session it runs inside.
Both were found by dogfooding — two agents (
@kildon the engine,@claudeon the helm rewrite) working the same kild during the helm port. Neither is theoretical.Attribution
attachmints a bearer token, and the engine writes that handle asfromwhen it is presented. The CLI took the token, printed it under--json, and threw it away —engineFetchnever set anAuthorizationheader. So every send from an attached harness landed as the unattributed labelhuman, indistinguishable from the operator typing and from every other attached agent in the kild.Observed live: two agents' messages and the operator's smoke tests were all
human → [...]on one log, while the operator was away.sendnow mints through the idempotentattachat call time and presents it. The token is deliberately not stored — it lives in engine memory (AttachTokens) and dies with the engine, so a copy on disk would outlive the thing it names and be rejected asunknown attach tokenon the next send.Discovery
An attached agent was reachable only if
KILD_KILD_IDwas in the environment, and a process's environment is fixed at exec time — so a session could not attach to a kild opened after it started, which is the normal case, since you decide to delegate mid-session.exportin a tool call changes a child shell, never the harness.A resumed or forked session is worse: its environment is rebuilt from a harness settings file no shell owns. The operator relaunched a session three times in one day trying to fix this from the shell, and could not.
attachnow records{kild, handle}against the harness session id — an opaque string kild stores and hands back.inboxandsendresolve it.kild attach <id> --as <handle>is now the entire setup.The record outranks the environment
This is the opposite of the obvious ordering, and it is the interesting part of the change.
A harness settings file (
.claude/settings.local.json) is re-read on every session start including resumes, outranks anything a shell exports, and cannot be cleared from the shell. A stale entry naming an archived kild is therefore invisible and unkillable — it would drain nothing forever while reporting success, which is precisely the failurehooks/claude-stop's own header warns about.An
attachis a deliberate act naming a kild that existed at the time; ambient config nobody re-reads loses to it.KILD_KILD_IDstill works, as the fallback.Resolution: explicit argument > this session's recorded attach > environment.
Supersession
Attaching supersedes any other session holding the same
(kild, handle). A session id is not stable —--fork-sessionmints a new one for what the operator experiences as the same session — so without this, records accumulate one per relaunch. And a handle is one identity: two live sessions sharing it would race on one destructive inbox, each eating mail meant for the other.Spawned agents are guarded out
The engine sets
KILD_KILD_IDandKILD_HANDLEon the agents it spawns too. Without theKILD_AGENT_IDguard,kild sendfrom an owned agent would have attached a second, shadow handle over the agent's own and spoken through it. Covered by a test.Two things found by running it, not by testing it
harnessSessionwas aconstarrow reached bydispatch()before initialisation, which brokekild attachoutright withCannot access 'harnessSession' before initialization.dispatch()runs at the top of the module, so anything it reaches must be hoisted. It is a function declaration now. No unit test would have caught this; running the binary did, immediately.cli.attached.test.tsinherited the ambient environment (env: { ...process.env }). Harmless before —cli.tsread none of those vars. Now the suite resolved the operator's own attachment and passed or failed depending on whose machine it ran on. The harness scrubs the identity vars and redirectsKILD_HOMEto a temp dir; tests opt into an identity by name.Gates
bun testbun run typecheckbun run lint./scripts/e2e.shVerified live against the running engine on 4517: attach → record on disk → an attributed send landing as
kild → [claude]instead ofhuman→ a drain resolved withKILD_KILD_IDandKILD_HANDLEexplicitly unset.Boundary
The engine learns nothing about any harness. It stores no session id and gains no new field; the record is written by the CLI, and only
hooks/claude-stopknows where the id comes from. Vendor knowledge stays in the vendor file.For an existing setup
docs/upgrading.mdhas the full list. In short: re-copyhooks/claude-stop, and check your harness settings file for a leftoverKILD_ROOM/KILD_PARTICIPANT— renaming the variables did not remove the stale entry, and it is exactly the invisible trap described above.