Repository navigation
actions: keep a running card's claim; forget only the value it was about - #76
Conversation
krishhgg
changed the title
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]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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]>
| if !first && about == NOTHING_STORED { | ||
| return Ok(if ctx.stash.get(&key)?.is_some() { Forgot::KeptUnrecorded } else { Forgot::NothingStored }); |
There was a problem hiding this comment.
If the first confirm finds a stash value with no index row, it records - on the card before trying to delete the value. If that deletion fails while the value remains, a retry treats the original value as one stored later. It closes the card without deleting what the person asked to forget. Keep the card open when the retry cannot tell whether the first deletion succeeded.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/actions.rs
Line: 203-204
Comment:
**Retry leaves original value stored**
If the first confirm finds a stash value with no index row, it records `-` on the card before trying to delete the value. If that deletion fails while the value remains, a retry treats the original value as one stored later. It closes the card without deleting what the person asked to forget. Keep the card open when the retry cannot tell whether the first deletion succeeded.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Leaving this as it is. The case needs the first confirm's stash delete to fail and leave the value in place. The retry then keeps the value and does not delete it, which is the safe direction. It also closes the card with a message saying a value is still stored under that name and that a new forget card removes it. Keeping the card open would bring back the problem this commit fixed, because the retry cannot tell the original value from one stored later. Telling the person plainly and letting them file a new forget card avoids that.
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]>
3183c01 to
ac88428
Compare
| pub fn stored_value_id(&self, name: &str, identity: &str) -> Result<Option<String>> { | ||
| Ok(self | ||
| .conn | ||
| .query_row("SELECT COALESCE(value_id, '') || ' ' || created FROM secrets WHERE name=?1 AND identity=?2", params![name, identity], |r| r.get(0)) |
There was a problem hiding this comment.
If an older tokenstash binary stores a replacement after a forget card records its target, it leaves the new value_id unchanged. Stores in the same second also have the same created time. The retry then treats the replacement as the original key and deletes it. The value check needs to account for writes by older binaries that share this database.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/db.rs
Line: 684
Comment:
**Retry deletes a replacement key**
If an older `tokenstash` binary stores a replacement after a forget card records its target, it leaves the new `value_id` unchanged. Stores in the same second also have the same `created` time. The retry then treats the replacement as the original key and deletes it. The value check needs to account for writes by older binaries that share this database.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Leaving this one. It needs four things at once: a forget card's first confirm stops after recording its target, an older tokenstash binary sharing this database stores a replacement, that store lands in the same second as the original, and the person confirms the old card again. Older binaries do not write value_id, which is why the check also compares the stored time. Closing that last gap would mean keeping something derived from the value itself, and this PR deliberately keeps nothing about secret values in the database.
| } | ||
| if cur.claude_mcp_registered { | ||
| let ok = claude_cli && claude_mcp(&["remove", "-s", "user", "tokenstash"]); | ||
| let ok = claude_cli && claude_mcp(&["remove", "-s", "user", "tokenstash"])?; |
There was a problem hiding this comment.
When claude mcp remove runs past the new limit during init --undo, ? exits undo_with before it restores saved entries for other agents. Those independent undo steps never run. Keep the Claude registration recorded for retry, but continue restoring the other entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli/src/cmd/init.rs
Line: 397
Comment:
**Claude timeout stops undo**
When `claude mcp remove` runs past the new limit during `init --undo`, `?` exits `undo_with` before it restores saved entries for other agents. Those independent undo steps never run. Keep the Claude registration recorded for retry, but continue restoring the other entries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Leaving this one. When claude mcp remove hits the limit, init --undo stops with that error before it touches the remaining records. Those records stay in the manifest, so running init --undo again carries on where it stopped. Nothing is lost. The person reruns undo after the hung claude is dealt with, and the error says what timed out.
There was a problem hiding this comment.
That’s fair. Since each undo record remains in the manifest until cleanup succeeds, propagating the timeout is a safe, resumable failure: the remaining records aren’t lost, and a subsequent init --undo retries them after the Claude registration is handled. I’m withdrawing this finding; no change is needed here.
| conn.execute( | ||
| "UPDATE workspaces SET audit_from=(SELECT COALESCE(MAX(id), 0) FROM audit WHERE ts <= workspaces.created) WHERE audit_from IS NULL", | ||
| [], | ||
| )?; |
There was a problem hiding this comment.
For an existing workspace, the audit_from backfill puts every event from its creation second behind the new boundary. A workspace can write its own event immediately after it is created, while timestamps still show the same whole second. After the upgrade, the agent cannot see that event in its audit view. The backfill needs to preserve events written after the workspace was created.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/db.rs
Line: 379-382
Comment:
**Upgrade hides recent audit events**
For an existing workspace, the `audit_from` backfill puts every event from its creation second behind the new boundary. A workspace can write its own event immediately after it is created, while timestamps still show the same whole second. After the upgrade, the agent cannot see that event in its audit view. The backfill needs to preserve events written after the workspace was created.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Leaving this one. The backfill runs once, for workspaces paired before this version. Its effect is that the agent's audit view leaves out an event written in the same whole second as its directory's pairing. The event is still in the database, and the person sees it with tokenstash audit. Leaving out a same-second row is the price of never showing an old checkout's events to a new one, which is what this change is for.
There was a problem hiding this comment.
That trade-off is reasonable. For workspaces paired before this version, the backfill cannot distinguish an old checkout event from a new pairing event within the same second, so the inclusive timestamp boundary appropriately prioritizes preventing stale events from appearing in the agent view. The event remains available through tokenstash audit.
I’m withdrawing this finding; no change is needed.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
1 participant
Fixes six Greptile findings on action cards, approval cards and the agent's audit view, and adds a time limit to
claude mcp addandremove. Four commits, one per area.A running action keeps its claim (#65, comment 4189557496)
An action card's claim lapsed after
CLAIM_HOLDS_SECS(120) whatever the action was doing. An action that ran longer, such asclaude mcp addon a slow machine, lost its card: another confirm could take it over and a decline could land while the first command still finished.confirmnow renews the claim from a thread with its own index connection everyCLAIM_RENEW_SECS(20) until the action returns (Db::keep_claim). A process that stops stops renewing, and the claim runs out as before. The claim's token identifies it, so a renewed claim still finishes or releases its card.Test:
a_running_action_keeps_its_claim_past_the_hold_time(core).Renewal alone would let a
claudethat hangs keep its card claimed for as long as the inbox runs. Soinitnow stopsclaude mcp addandclaude mcp removeafter 60 seconds (CLAUDE_MCP_TIMEOUT). It kills the child, waits for it, and returns an error, so the confirm gives the card back and the person can decline it. Whenclaude mcp addtimes out,initkeeps its record of the registration, because the stopped command may have made it and--undocan remove a recorded one.Test:
a_hung_claude_mcp_is_stopped_at_the_time_limit(crates/cli/src/cmd/init.rs), with a fakeclaudethat sleeps past a 300 ms limit.A card being carried out does not expire (#60, comment 4189643130)
expire_overduemarked a card expired while its confirmed action ran, sofinish_actioncould 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.Test:
a_card_being_carried_out_does_not_expire(core).A forget retried after a stop keeps a key stored since (#66, #67, #71; comments 4189603933, 4189595193, 4189613108)
If a confirm stopped, or its database update failed, after
forget_keydeleted the key but before the card was closed, confirming the card again deleted whatever was stored under the name by then.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 columntasks.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's result says so. The record is made at the first confirm, not when the agent files the card, so a person who stores a new value and then confirms an older forget card still gets what they confirmed.Test:
a_forget_confirmed_again_after_a_stop_keeps_a_key_stored_since(core), which stops the first confirm after the whole deletion and between the stash and the index, with and without a key stored since.Forget holds the index write lock (#65, comment 4189557467)
forget_keydeleted the stash value and the index row as separate statements, so a store landing between them kept its value and lost its row. Both deletions now run under the index write lock the store path holds. The logic moved totokenstash_core::actions::forget.Test:
forget_and_a_concurrent_store_do_not_interleave(core). It fails without the lock.Deny on an approval card checks what the page showed (#65, comment 4189557447)
The inbox's Deny on an approval card called
tasks::deny, which closed the card as it was, including keys an agent added after the page loaded. It now goes throughanswer_approvalwithDecision::Denyand the page'sseenlist, the same locked comparison Allow uses. A grown card is refused and stays open. The agent's card link still cannot close an approval card.Test:
denying_an_approval_card_that_grew_since_the_page_loaded_is_refused(crates/cli/tests/inbox_scope.rs). It fails with the old routing.The agent's audit view is bounded by row id (#65, #61, #71; comments 4189557511, 4189642121, 4189613102)
recent_audit_forfiltered by path andts >= ws.created, both whole seconds, so a directory paired again within the second of an old event showed the new agent the old directory's events. A workspace now records the id of the last audit row written before it (new columnworkspaces.audit_from), and the agent sees only rows after that id. Workspaces recorded before the column existed, or by an older version, get the last row up to and including their creation second when the index opens.Test:
a_directory_paired_again_in_the_same_second_does_not_see_the_old_ones_events(core).Schema
Three columns, added the way
Db::openadds the others (probepragma_table_info,ALTER TABLE ADD COLUMN, a "duplicate column" race is success):secrets.value_id,tasks.acts_on,workspaces.audit_from.SCHEMA_VERSIONis unchanged; an older binary ignores the new columns.Checks
cargo clippy --workspace --all-targets --locked -- -D warnings,cargo test --workspace --lockedandbash scripts/leak-test.shpass. No user-facing docs describe the changed behaviour, so README, SECURITY.md and the skill files are unchanged.🤖 Generated with Claude Code
The PR is not ready to merge: a forget retry can delete a replacement key, undo can stop partway, and the upgrade can hide valid audit events.
Fix with agent prompt
Summary
This PR renews running action claims, limits Claude MCP commands, makes forget retries target a recorded value, checks approval denials against the page, and bounds agent audit views by row ID.
krishhggaccepted the unindexed-value retry risk in previous comment 1: they prefer keeping the value and directing the person to a new forget card over risking deletion of a replacement.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Person confirms action card] --> B[Claim card and start renewal] B --> C[Run action] C --> D[Stop renewal] D --> E{Action succeeded?} E -->|Yes| F[Close claimed card] E -->|No| G[Release claim for retry]Reviews (3) · Last reviewed commit: "actions: keep a value with no index row ..."