Repository navigation
changelog: the fixes from the second review - #78
Merged
Merged
Conversation
A read that found a key only in its session keyring (a copy keyring-rs's keyutils store left) linked it into the user and persistent keyrings without the lock a write takes. A store from another session between the search and the links lost to the old copy, in both keyrings. A read that finds the key linked into both keyrings now changes nothing. Any other read takes the lock, looks again, and links what it finds then, so a key stored meanwhile is the one read and kept. An older tokenstash still running in another login session can still put an old value back. Its read links its own session copy into the persistent keyring, and its write does the same after updating that copy. No other session may write that copy, and the kernel records no write time, so the two cases leave the same state and no read rule is right for both. Reads keep the user keyring's value, as before. doctor now names each key whose two keyrings hold different values, or that has a copy in another live login session (found in /proc/keys), and says to stop the older process. troubleshooting.md says what cannot be fixed. Co-Authored-By: Claude <[email protected]>
store_generated kept another process's value only when the key's index row was created at or after the stash read that found the key missing. tokenstash import keeps the created time from its bundle, so an import that landed between that read and the store looked unfinished, and need replaced the imported signing or encryption key with a new one. The read now takes a mark, the newest audit row id, and the store keeps the value when a store, import or adoption of the key was recorded after the mark. Each of those writes its index row and audit row in one transaction; the adoption in need now does too. Audit ids only grow, so the check no longer rests on one-second timestamps either. Co-Authored-By: Claude <[email protected]>
A compact service-account JSON holds its private key on one line, with the key's line breaks written as the two characters \n. Redactor::add made line patterns only for values with real line breaks, so a child that decoded the JSON and printed the key put each line of it on the agent's stream unmasked. add now also splits on an escaped \n or \r, with the same length rules as real lines. Co-Authored-By: Claude <[email protected]>
The older copies line failed doctor whenever another login session held its own copy of a key. Someone upgrading from 0.2.0 or 0.3.0 has idle sessions holding such copies for a while, and they do nothing until an older tokenstash runs there, so doctor exited 1 for no fault. The line now fails only when a key's user keyring and persistent keyring hold different values, which is when a read can return the old key. A copy another session holds is printed as a passing line that says when it matters and what to do. stray_copies returns each case separately so doctor can tell them apart. Co-Authored-By: Claude <[email protected]>
The older copies check looked only at the keys this home's index lists. A key stored under this home's service name with no index row (a lost index, another home with the same service name) is still read and adopted by need, and was never checked. The check now also takes every key /proc/keys lists under this home's service name. When /proc/keys could not be read, the check counted no copies in other sessions and doctor said nothing about it. It now still compares the user and persistent keyrings for the indexed keys, and the line says what it could not check and why, without failing. The line was printed only when a copy turned up, and could be printed twice. doctor now prints one older copies line whenever the backend is the kernel keyring: it fails when two keyrings hold different values for a key, and otherwise passes with what it found, or with how many keys it checked. reference.md describes the line. Co-Authored-By: Claude <[email protected]>
Greptile on #63, #65 and #67: - An agent's need --force passed force for every key, even when its first check found no denial. A "no" given after that check was skipped, so a broad grant could deliver the key with no second-ask card. NeedOpts.ask_again now lists the NAME@identity entries that hold a reserved extra ask, and need sets aside only their denials. force is now only the person's own --force. - In a multi-key call, every new card said "Asked again after you declined". need now adds that line only to the card of a key in ask_again. - Running the same need NAME --force while its second-ask card waited failed with "already asked for again once". It now returns that card as a pending result with its link and next (Db::force_card). Once the person answers the card, the same command is an ordinary request and delivers the key. - The extra ask was reserved by project and key name, while the denial check is per identity, so asking again for one declined identity used up the other's. The reservation now records the identity, and only a card for that identity spends it. A row written before this change has no identity and still counts for every identity. denied_here returns the identity it found the "no" under, resolved the way need resolves it, a generated secret included. Co-Authored-By: Claude <[email protected]>
…ntity's card Greptile on #73: - An agent asking again for a declined key that is stored gets a pairing or sensitive approval card, and only paste cards said it was a second ask. create_approval_task now takes the entries asked again and puts a sentence in front of the card's why for each one ("Asked again after you declined NAME, because you asked AGENT to."), on a new card and when the entry joins an open card. A key asked for the first time on the same card gets no such sentence. A new card that holds only keys asked again leaves out "First time this directory asks for stored keys". - force_card returned the card that a reservation without an identity named, whatever identity the card asked for. Such rows were written before reservations recorded one, so a retry for personal could come back with the pending card for work. force_card now returns a card only if it asks for the identity being retried. The old row still spends the ask for every identity, so a retry for another identity gets the "already asked" error. Co-Authored-By: Claude <[email protected]>
Deny on an approval card called tasks::deny, which closes the card as it is now. If an agent added a key to the card after the page loaded, Deny closed the grown card and recorded the added key as denied without the person having seen it. Allow already compared the card with the page's `seen` list under the index write lock in answer_approval. Deny on an approval card now takes the same path: answer_approval with Decision::Deny and the page's `seen` list. A card that grew is refused with "this card changed since you read it" and stays open. The agent's card link still cannot close an approval card. Other cards' Deny is unchanged. The new inbox test grows a pairing card after the page loads and checks that a Deny on the old list is refused and one on the current list lands. Co-Authored-By: Claude <[email protected]>
…s about A confirmed action card is claimed while its action runs. The claim lapsed after CLAIM_HOLDS_SECS (120) whatever the action was doing, so an action that ran longer (`claude mcp add` on a slow machine) lost its card. Another confirm could take it over and a decline could land while the first command still finished. `confirm` now renews the claim from a thread with its own index connection every CLAIM_RENEW_SECS (20) until the action returns. A process that stops stops renewing, and the claim runs out as before. The claim's token is now what identifies it, so a renewed claim still finishes or releases its card. expire_overdue marked a card expired while its confirmed action ran, so finish_action, which needs a pending card, could not close it, and the person saw "expired" for a change that happened. A card with a live claim no longer expires. Once the claim runs out or is given back, it expires on the next call. If a confirm stopped after forget deleted the key but before the card was closed, the card stayed open, and confirming it again deleted whatever was stored under that name by then, including a key stored since. Every store now gives the value it writes a random value id (new column secrets.value_id). Before deleting anything, the first confirm of a forget card records the stored value's id and time on the card (new column tasks.acts_on) and commits that on its own. A later confirm of the same card deletes only that value. If the name holds a different value by then, it is kept and the card says so. forget deleted the stash value and then the index row as separate statements. A store landing between the two kept its new value and lost its index row. Both deletions now run under the index write lock that the store path holds around its own stash write and index row. The forget logic moves to tokenstash_core::actions::forget. New tests cover a claim kept past its hold time and freed once nothing renews it, a card that does not expire while its action runs, forget retried after a stop at either point with and without a key stored since, and a store that starts between forget's two deletions. Co-Authored-By: Claude <[email protected]>
recent_audit_for showed an agent the rows for its path with ts at or after the workspace's creation time. Both times are whole seconds, so a directory replaced and paired again within the second of an old event showed the new agent the old directory's key names and events. A workspace now records the id of the last audit row written before it (new column workspaces.audit_from, set when the workspace is inserted), and the agent's view holds only rows after that id. Row ids only grow, so no earlier row shows whatever its time. Workspaces recorded before this column existed, or by an older version, get the last row up to and including their creation second when the index opens; until then their view starts after that second. The new test pairs a directory again in the same second as the old one's event, and covers its own events in that second and the fill-in for older records. Co-Authored-By: Claude <[email protected]>
init ran `claude mcp add` and `claude mcp remove` with no time limit. An action card that runs one keeps its claim renewed while the command runs, so a `claude` that hung kept the card claimed for as long as the inbox ran, and the person could not decline it. claude_mcp now polls the child against a 60-second deadline, the way tailscale_json does. Its output goes to /dev/null, so it needs no reader threads. Past the deadline it kills the child, waits for it, and returns an error, so the confirm gives the card back. A command that finishes in time reports how it exited, as before, and one that cannot start is still a failure. When `claude mcp add` times out, init keeps its record of the registration, because the stopped command may have made it and `--undo` can remove a recorded one. A new test runs a fake `claude` that sleeps past a 300 ms limit. Co-Authored-By: Claude <[email protected]>
…d whose keeper fails When the first confirm of a forget card found no index row, it recorded "-" on the card. A later confirm then deleted whatever stash value was under the name, which could be a replacement written by a store that stopped before its index row. Such a value has no id to tell it apart, and the database must not hold a hash of it. A later confirm of a card recorded as "-" now deletes no value that lacks an index row. It keeps the value, closes the card, and says that a value is still stored under the name and that a new forget card removes it. pin_action_target now also says whether this call made the record, so the first confirm still deletes as before. confirm claims the card before keep_claim opens a second connection to the index. If that open failed, nothing ran, but the card had to wait for the claim to run out before it could be confirmed again or declined. keep_claim now gives the claim back before it returns the error, so every caller gets this. confirm calls it before perform and returns its error directly. New tests cover a retry that finds a replacement with no index row, and a keeper that cannot open the index because the schema version is newer than this build. Co-Authored-By: Claude <[email protected]>
The inbox asks tailscale whois who sent a tailnet request, and gives the full session to a device signed in as the owner. It ran whatever TOKENSTASH_TAILSCALE named, or the first tailscale on PATH, with the caller's environment. The inbox often inherits an agent's environment, so an agent could run a script that gave a local address to status and the owner's login to whois, then approve its own cards from a second local address. Release builds now run only a tailscale from where Tailscale installs it, with a fixed PATH and no other variable but HOME. TOKENSTASH_TAILSCALE is read by debug builds only, for the tests. link_base kept a successful ownership proof of the tailnet listener for 30 seconds. If the inbox stopped, or remote access went off and on, and another process took the address and port, links with a card credential or the session could name it. Every link now proves the listener again. The inbox's own "Send the link to my desktop" link is on loopback, since the inbox cannot answer a proof of itself while it builds that link. whois kept a failed lookup for 60 seconds, so one Tailscale hiccup turned the owner's device away for a minute. Only answers are kept now. The inbox ran whois, for up to 3 seconds, on the thread that answers every request, so a slow lookup for a new device held up local page loads. The reader thread that read the request now asks before it hands the request over. A device Tailscale cannot name still gets a 404. Co-Authored-By: Claude <[email protected]>
Every link proved the Tailscale listener on its own, one after another. A need that filed eight cards waited on nine proofs (the notification and each card) before it printed anything. Links are now built on a util::Links, made where a command already looked at the inbox on loopback. It proves the Tailscale address the first time a link needs it and reuses that answer for the rest of the batch. It is a value the command holds, so nothing outlives the command. need, the MCP secrets_request and human_request, run and rotate reuse the notification's Links when they print their own links before any wait, and make a new one after a wait. task_check builds all its Replace links on one. A new test turns remote access on at an address held by a relay that counts connections and passes them to the inbox. One need for three keys makes one proof there; with a proof per link it made four. Co-Authored-By: Claude <[email protected]>
…e home, undo a failed card A plain init took every marked section out of a recorded AGENTS.md but saved only the first for undo, so a section the person added or edited after a shipped one was gone for good. Each section that is not shipped text now gets its own undo record, from the file and from its backup. Undo puts each one back unless the file already holds that text, and undo straight from an old record restores every section the backup held. Undo records written before init kept the entry it wrote have no `wrote`, and undo then took out any tokenstash MCP entry as init's. With such a record undo now takes an entry out only when it has exactly the shape init writes: an absolute command, args of just "mcp", an optional type "stdio" and an env holding only TOKENSTASH_HOME. Any other entry stays, and undo says so. The skill names TOKENSTASH_HOME for every later agent session, and an agent may run a plain init. A path holding a backtick, a line break or another control character could close the quote and add instructions, so init, init --print-skill and a confirmed card now refuse it before writing anything. The MCP entries escape the path and were already safe. A mode or MCP card saved the new setting before rewiring the agents. If the rewiring failed, the card went back to pending with the setting still changed. The setting now goes back, unless someone changed it since, and the skills return to the restored mode. Co-Authored-By: Claude <[email protected]>
…rd re-registers Undo straight from an older record put the backup's AGENTS.md sections back only when it found a shipped section to take out. If the person had removed that section by hand and kept the file, nothing came back, and the record was dropped, so the sections were lost for good. They now come back whenever the file does not already hold that exact text. A card turning the MCP server off could take some agents' entries out before a config it could not read stopped it. The setting went back to on, but the recovery left registrations alone, so the card could be declined with an agent disconnected. When the setting was put back and the card changed the MCP setting, the recovery now rewires with the restored one, registering the server again (or taking it out again when the restored setting is off). Co-Authored-By: Claude <[email protected]>
The recovery for a failed mode or MCP card rewired every agent for the restored setting. A card turning the server off that stopped at Codex's config then re-registered Cursor too, replacing an entry the person had edited and the run never reached. A failed card turning the server on took out registrations it had never added. Before a card rewires the agents, init now notes each shared config's tokenstash entries and its own records of those configs. If the card fails and the setting goes back, each config whose entries changed gets them back as they were, entry by entry, along with the records. A config the run never changed is not written. The skills still return to the restored mode. Co-Authored-By: Claude <[email protected]>
One entry for the five follow-up PRs, and "once per key and identity" in CHANGELOG.md and SECURITY.md where `need --force` from an agent is described. Co-Authored-By: Claude <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
`tailscale` still gets `HOME`. `init` saves the person's sections for undo rather than leaving them in the file. A failed card tries to put things back and leaves a newer setting, and `doctor` shows what is out of step. Co-Authored-By: Claude <[email protected]>
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.
One changelog entry for the five follow-up PRs (#73, #74, #75, #76, #77), which fix what Greptile found when it reviewed #59 to #71 together.
It also changes "once per key and project" to "once per key and identity in each project" where
CHANGELOG.mdandSECURITY.mddescribeneed --forcefrom an agent, to match #73.This sits on top of #74 in the stack #77, #73, #76, #75, #74.
mainis fast-forwarded to the top once CI passes.🤖 Generated with Claude Code
The documentation changes appear safe to merge.
Summary
This PR documents the second-review fixes and updates the descriptions of an agent’s
need --forcelimit.Reviews (2) · Last reviewed commit: "changelog: say what the second-review fi..."