Repository navigation
secrets-collect: add --yes for agents without a terminal - #5
Conversation
…, self-clear), report partial stores, no value chars in transcript, tests with timeouts
…--yes protections on pty too, refuse piped answers, LEN required, clear in finally, SIGTERM/SIGHUP partial report, pbpaste locale, stronger tests
…ait, user-level lock path, signals blocked around the clear, tests for surviving mutants
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 8d93c2cb-3878-44e8-a345-e10fbab90a34) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e620892b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except StoreError as e: | ||
| print(str(e), file=sys.stderr) | ||
| report_partial(stored, [n for n, _ in collected if n not in stored]) |
There was a problem hiding this comment.
If sudo-secretspec set returns 125 or 126, this handler incorrectly lists the current name as not_stored and can even claim that nothing was stored. These are explicitly ambiguous outcomes: sudo-secretspec-cli/src/broker.rs returns 125 when rollback fails and 126 when recording the terminal audit event fails after the operation has already committed or rolled back; the client preserves these exit codes. Preserve the return code in StoreError and report the current name as unknown for these cases rather than telling the operator that its previous value is unchanged.
Useful? React with 👍 / 👎.
| """secrets-collect.py with stub `sudo-secretspec`, `pbpaste`, `pbcopy` on PATH; no real vault or clipboard. | ||
|
|
||
| Every subprocess has a hard timeout: a mutant that loops must fail, not hang CI. |
There was a problem hiding this comment.
On a checkout without an installed privileged boundary, tests/sudo_postinstall/conftest.py:132-140 marks every collected test as skipped, including all 44 new tests in this file. Running python3 -m pytest tests/sudo_postinstall/test_secrets_collect.py -q here therefore exits successfully with 44 skips, even though these tests use stubs and require no live installation. Move this module outside the gated directory or exempt it from the collection hook so ordinary development and automated runs actually exercise the new safety checks.
Useful? React with 👍 / 👎.
What
skills/sudo-secretspec/scripts/secrets-collect.pygains--yesso an agent without a terminal can collect one secret per run from the clipboard, safely:--clipboard --yesrequires an exact:LEN, refuses stale, sentinel or already-consumed copies, and clears the clipboard right after the read.flock(~/Library/Caches/secrets-collect.lock), so parallel runs cannot store one copy twice. A loser is told which run consumed the copy and that both names need re-collecting, one at a time.PARTIAL STORE ... not_stored=[..]), SIGINT/SIGTERM/SIGHUP are handled, and value characters never reach stdout, stderr or argv.--yes, and--yeswithout--clipboard, are refused.SKILL.mddocuments the protocol, including "never run two at once" and the--literalexception for/+=keys.Tests: 44 in
tests/sudo_postinstall/test_secrets_collect.py(stubsudo-secretspec/pbpaste/pbcopy, every run has a timeout); the adversary re-ran them, 44 passed.Adversary reviews (four rounds)
Verdict:
APPROVEon68105c47807d44bd66fe77ff0b4b9b93b2c5a7d9(round 4, delta review of the third repair; round 3 approved6cb0341). Headf6e6208adds only the CHANGELOG entry on top of that reviewed head.Rounds 1 and 2 returned BLOCK (stale-clipboard store; then a parallel read-before-clear race storing one copy under two names); both are fixed and tested, with mutation checks.
Deferred low findings (round 4), to be fixed in a follow-up:
printf 'cleared' | cliperases the consumer name when runs do not overlap.SECRETS_COLLECT_LOCK_DIR).🤖 Generated with Claude Code
Note
Medium Risk
Changes secret-ingestion semantics (partial stores, clipboard clearing, and locking) on a security-sensitive operator script; mistakes in parallel runs or wrong-name assignment are documented but not automatically recoverable.
Overview
Adds
--yestosecrets-collect.pyso agents without a TTY can ingest one secret per run from the clipboard (--clipboard --yes), with mandatory:LEN, sentinel-based stale/consumed detection, per-user flock around read→clear, and abort paths that never echo secret bytes (only lengths and character classes).Behavior changes beyond the agent path: collection is no longer described as all-or-nothing once the store loop starts—failures and interrupts emit
PARTIAL STOREwithstored/not_stored/unknownand do not roll back prior writes. Interactive docs inSKILL.mdandCHANGELOG.mdspell out the no-terminal protocol (sequential runs,--literalfor base64-style keys).Tests: new
tests/sudo_postinstall/test_secrets_collect.py(stubsudo-secretspec/pbpaste/pbcopy) covering locks, sentinels, signals, partial stores, and refusal modes.Reviewed by Cursor Bugbot for commit f6e6208. Bugbot is set up for automated code reviews on this repo. Configure here.