Repository navigation
init: keep every section and unknown entries on undo, refuse an unsafe home, undo a failed card - #74
Merged
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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]>
| let back = match put_back_choice(before, chosen) { | ||
| Ok(false) => return Err(e.context("the agents could not be rewired; the setting was changed again meanwhile and stays as it is now")), | ||
| // MCP registrations are touched only when the card changed that setting. | ||
| Ok(true) if before != chosen => wire(manifest, w, before.0, (before.1 != chosen.1).then_some(before.1)).map(|_| ()), |
There was a problem hiding this comment.
If a card turns MCP off and Codex’s config cannot be parsed, the removal stops before it reaches Cursor. Recovery then calls register_mcp for every agent, replacing Cursor’s untouched, user-edited tokenstash entry. Declining the failed card will not bring that entry back. Restore only entries the failed attempt removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli/src/cmd/init.rs
Line: 1194
Comment:
**Failed card replaces Cursor entry**
If a card turns MCP off and Codex’s config cannot be parsed, the removal stops before it reaches Cursor. Recovery then calls `register_mcp` for every agent, replacing Cursor’s untouched, user-edited `tokenstash` entry. Declining the failed card will not bring that entry back. Restore only entries the failed attempt removed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Owner
Author
There was a problem hiding this comment.
Fixed in 75d75ad. Before a card rewires the agents, apply_choice_with now takes an McpSnapshot of each shared config's tokenstash entries and init's records of them. If the card fails and the setting goes back, only configs whose entries changed are put back, entry by entry. A config the run never reached is not written, so Cursor's edited entry stays byte for byte. A failed "server on" card likewise takes out only what it added. Tests: a_failed_card_puts_back_only_what_it_took_out and a_failed_card_turning_the_server_on_takes_out_nothing_it_did_not_add.
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]>
krishhgg
force-pushed
the
fix/init-followups
branch
from
October 6, 2026 00:15
75d75ad to
619b3cc
Compare
Comment on lines
+1360
to
+1363
| if McpEntries::read(p, then.claude).is_ok_and(|now| now.found == then.found) { | ||
| continue; | ||
| } | ||
| if let Err(e) = then.restore(p) { |
There was a problem hiding this comment.
If an agent or another command updates a tokenstash MCP entry while a card is being applied, and the card then fails, McpSnapshot::put_back replaces that entry with its pre-card copy. It checks only whether the entry differs from the old copy, not whether the failed card changed it. The person loses their newer registration. Restore only entries changed by the failed card.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli/src/cmd/init.rs
Line: 1360-1363
Comment:
**Newer MCP entries get overwritten**
If an agent or another command updates a `tokenstash` MCP entry while a card is being applied, and the card then fails, `McpSnapshot::put_back` replaces that entry with its pre-card copy. It checks only whether the entry differs from the old copy, not whether the failed card changed it. The person loses their newer registration. Restore only entries changed by the failed card.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Owner
Author
There was a problem hiding this comment.
Leaving this one. The recovery runs only when the card itself failed, and the race needs another writer to change the tokenstash MCP entry in the seconds between the card's snapshot and its recovery. A compare-and-swap would need what the failed attempt itself wrote to each file, which the run does not keep for files it never reached. tokenstash doctor shows an agent whose registration is not what the setting says, and init sets it right.
There was a problem hiding this comment.
That trade-off is reasonable here. The finding is technically valid—the snapshot cannot distinguish a failed attempt’s write from a concurrent writer’s update in every path—but the race requires a narrow timing window, recovery only runs after the card’s wiring fails, and tokenstash doctor plus a subsequent init provide an explicit repair path. I’ll leave this finding as-is and won’t require a compare-and-swap change for this PR.
Comment on lines
+1242
to
+1245
| if (cfg.agent_mode, cfg.mcp) != chosen { | ||
| return Ok(false); | ||
| } | ||
| (cfg.agent_mode, cfg.mcp) = before; |
There was a problem hiding this comment.
If a card fails after setting mcp = true, another person can deliberately save that same choice with init --no-agents --mcp. put_back_choice sees matching values and changes mcp back to false, even though the later command chose true. The person's newer choice is lost. Matching values alone cannot show which command last saved them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli/src/cmd/init.rs
Line: 1242-1245
Comment:
**Later setting gets undone**
If a card fails after setting `mcp = true`, another person can deliberately save that same choice with `init --no-agents --mcp`. `put_back_choice` sees matching values and changes `mcp` back to false, even though the later command chose true. The person's newer choice is lost. Matching values alone cannot show which command last saved them.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Owner
Author
There was a problem hiding this comment.
Leaving this one. It needs a second init --mcp saving the same value in the moment between a card failing and its recovery, which runs right away in the same process. Matching values are the best signal the config holds without a write counter, and if it does happen, rerunning init --mcp restores the choice and doctor shows the mismatch until then.
There was a problem hiding this comment.
That’s reasonable. With recovery happening immediately and no config generation/write counter, matching the chosen values is the strongest available guard against rolling back a later different choice; a same-value write is indistinguishable from the card’s write. I’ll leave this finding as-is and won’t require a change.
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.
Fixes four Greptile findings on
tokenstash init, all incrates/cli/src/cmd/init.rs, with a doc note incrates/cli/src/cmd/actions.rs.Every person's AGENTS.md section is kept
Greptile on #60, #61, #64, #65 and #66. A plain
inittook every marked section out of a recordedAGENTS.mdbutsection_ofsaved only the first. If a shipped section came first, the person's section after it was deleted with no undo record.reinsertalso gave up as soon as the file held any marked section, so a second saved section could never come back.sections_ofreturns every marked section.retire_legacyrecords each one that is not exactly shipped text, andretirerecords each section a backup held.reinsertputs a section back unless the file already holds that same text, so every saved section returns and a retry after a crash adds nothing twice.undo_shared, run straight from an old record, restores every non-shipped section the backup held.Tests:
every_persons_section_is_kept_whichever_comes_first(shipped section first and last),every_section_an_old_init_replaced_comes_back(with and without a plaininitbefore the undo).Undo from an older record leaves entries it cannot recognise
Greptile on #60, #65 and #67. A record from before init kept
wrotehas none, andundo_sharedtreated any tokenstash MCP entry as init's. With such a record, undo now takes an entry out only when it has exactly the shapemerge_mcp_json_typedandmerge_codex_tomlwrite: an absolute command, args of justmcp, an optionaltype: stdio, and anenvholding onlyTOKENSTASH_HOME. Any other entry stays, and undo prints a line saying so. It also prints that line when an entry changed since init wrote it.Test:
undo_from_an_older_record_takes_out_only_an_entry_shaped_like_inits(a changed Codex entry and a hand-added Cursor entry stay, init's own entries go, plus direct checks of the shape test).A home path that could add skill instructions is refused
Greptile on #61 and #71.
skill_textcopiedTOKENSTASH_HOMEinto every globalSKILL.md, and an agent may run a plaininit.check_skill_homerefuses a path holding a backtick, a control character (line feed, carriage return and the rest) or a Unicode line or paragraph separator.skill_textnow returns aResult.initchecks before it sets anything up unless--no-agentsis given, andwirechecks before it touches any file.--print-skilland a confirmed card refuse too. The MCP entries pass the path through JSON and TOML escaping and were already safe. The other places that print the path go to the terminal only.Tests:
a_home_path_that_could_add_skill_instructions_is_refused(unit, checks that no file is written and no undo record is made) anda_plain_init_refuses_a_home_that_could_add_skill_instructionsintests/init_mode.rs(the binary from a pipe, checks that no skill, database or undo record appears and config.toml is unchanged).A card that fails to rewire changes no setting
Greptile on #60.
apply_choicesaved the new mode or MCP setting beforewireran. Ifwirefailed,confirmreleased the card for another try or a decline with the setting still changed. The logic now lives inapply_choice_with. Ifwirefails,put_back_choicerestores the previous mode and MCP setting under the config lock, but only if the config still holds what the card chose. When the mode changed, the skills are then rewired to the restored mode, with MCP registrations left as they are. The error on the card says what happened.Test:
a_card_that_cannot_rewire_the_agents_leaves_the_setting_as_it_was(Cursor'smcp.jsoncannot be parsed, so an explicit-mode card fails; the config and every skill copy are back in auto mode, and a setting changed meanwhile stands).Checks
cargo clippy --workspace --all-targets --locked -- -D warnings,cargo test --workspace --lockedandbash scripts/leak-test.shall pass. CHANGELOG.md is not changed here.🤖 Generated with Claude Code
This PR is not ready to merge because failed-card rollback can undo changes another writer made while the card ran.
Fix with agent prompt
Summary
This PR updates
initto save and restore multiple marked sections, preserve unfamiliar MCP entries on legacy undo, reject unsafe home paths in skills, and roll back failed cards.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Confirm mode or MCP card"] --> B["Save chosen setting"] B --> C["Snapshot agent entries"] C --> D["Rewire agents"] D --> E{"Did wiring succeed?"} E -- Yes --> F["Finish card"] E -- No --> G["Restore old setting"] G --> H["Restore saved MCP entries"] H --> I["Rewire old skill mode"] I --> J["Release card for retry"]Reviews (3) · Last reviewed commit: "init: a failed card puts back only the M..."