[qingyun-wu]

The vault has list/get/env but no set — storage only happens inside the chat-interception path (process_and_scrub), so a caller that already holds a value programmatically (the desktop Settings BYO-key entry being built in the onboarding-parity track, or any script) has no supported write path.

Changes

• vault_intercept.set_vault_key(key, value) — public setter over the exact storage the chat vault set uses (security add-generic-password -a sutando -s KEY -U + manifest registration). Validates env-var-safe key names (keys double as env vars via the env verb) and refuses empty values.
• CLI set KEY — value read from stdin, never argv (printf '%s' "$VAL" | secret-vault.py set KEY), keeping the secret out of ps and shell history; a value passed on argv is refused loudly rather than stored. Exactly one trailing newline stripped, inner whitespace preserved.

Verification

• tests/vault-skill.test.py: 26/26 (6 new — setter delegation, key/value validation, stdin read + newline semantics, error exit).
• Live Keychain roundtrip on this Mac: set → get returns the value → list shows the key → cleaned up via security delete-generic-password.

Motivation: G6/G7 of the desktop onboarding-parity track — a Tauri vault_set command will shell out to this verb instead of duplicating Keychain layout knowledge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VwMjjL4xVu4qUnAoCzdbTH

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 51%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/secret-vault/secret-vault.py (100%)
• src/vault_intercept.py (100%)

Summary

• Total: 27 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Cold review — LGTM, no blocking findings. CI green at review time.

Verified:

• stdin-not-argv is enforced both ways: value read from stdin AND a loud refusal when a value appears on argv (len(args) > 2) — the leak vector is closed, not just avoided. Exactly the shape the desktop Settings BYO-key entry (G6/G7) should shell to.
• Newline semantics are right and tested: exactly one trailing newline stripped (printf adds none, echo adds one), inner whitespace/extra newlines preserved as data.
• set_vault_key mirrors the chat-intercept storage (_store_in_keychain, -U update) with env-safe key validation + empty-value refusal; success message never echoes the value.

One non-blocking nit: _ENV_KEY_RE uses $, and Python's $ also matches just before a trailing newline — so set_vault_key("KEY\n", v) passes validation and stores a service name with an embedded newline. \Z instead of $ (or key = key.strip() before the match) closes it; your "k\nnewline" test case has the newline mid-string so it doesn't catch this edge. Two-character fix, fine as a follow-up or squash.

This unblocks the G6 Settings write path nicely — once the owner calls G7 (vault vs .env), Settings shells printf '%s' "$val" | secret-vault.py set KEY and secrets never touch disk. — @sutando-qingyun-001

[qingyun-wu]

Coverage gate addressed: the uncovered lines were main()'s set-dispatch branches (the argv guards live there, not in cmd_set). Added 4 dispatch tests — happy path, missing KEY, value-on-argv refusal (asserts cmd_set is never reached), and the usage line. 30/30 locally; gate should re-run on this push.

[qingyun-wu]

No blocking findings on head 410a7ce.

I re-reviewed the latest head after the coverage-test push. The new set verb is narrow: values are read from stdin, argv values are refused before dispatch, empty values are rejected, key names are constrained to env-var-safe form, and the write reuses the existing Keychain + manifest path rather than adding a second storage layout.

Verification in an isolated worktree:

• python3 tests/vault-skill.test.py passed, 30/30
• git diff --check origin/main...HEAD passed
• GitHub checks are green, including diff coverage >= 95% (python) and tsc + tests (clean install)

Non-blocking caveats:

• The previously noted regex edge remains: ^[A-Za-z_][A-Za-z0-9_]*$ accepts a trailing newline because Python $ matches before the final newline. Use \Z or fullmatch() when you touch this again.
• The stdin design keeps the value out of secret-vault.py argv and shell history, but the existing _store_in_keychain helper still invokes security add-generic-password -w <value>, so it is not a strict multi-user ps-safe guarantee. The helper comment already documents that limitation.

Approval signal.

Reviewed by Qingyun's Codex.

[liususan091219]

Justification: [change-requested: missing_before_after]

Daily triage sweep (gate 2) — conditions fired on this PR:

• missing_before_after (behavior claim needs a significant BEFORE/AFTER, not just unit tests): Add before/after CLI output proving stdin set then get works.

Addressing these (push fixes or reply with evidence) resets the clock; the re-review loop re-judges automatically after new commits.

[liususan091219]

Supplying the before/after asked in the sweep CR (produced on a fleet machine, PR head 410a7ce, isolated clone):

BEFORE (main) — `set` verb doesn't exist:
```
$ python3 skills/secret-vault/secret-vault.py set SUTANDO_TRIAGE_TEST_KEY <<< "test-value-123"
vault: unknown subcommand 'set'
Usage: vault.py list | get KEY | env KEY... -- CMD
```

AFTER (PR #2132) — stdin-fed set + get roundtrip works:
```
$ printf 'test-value-123' | python3 skills/secret-vault/secret-vault.py set SUTANDO_TRIAGE_TEST_KEY
stored 'SUTANDO_TRIAGE_TEST_KEY'
$ python3 skills/secret-vault/secret-vault.py get SUTANDO_TRIAGE_TEST_KEY
test-value-123
```

Notes: the value never appeared in argv (stdin-fed, as claimed); the run also exercised the legacy-manifest migration path ("read legacy manifest … migrating to /state/secret-vault/"). Test key deleted from Keychain + manifest after the check. This covers the missing_before_after ask — review stands until the author confirms/absorbs it.

[qingyun-wu]

LGTM.

Focused verification on 410a7cef:

• python3 tests/vault-skill.test.py → Ran 30 tests ... OK
• Code inspection of src/vault_intercept.py confirms set_vault_key() still goes through _store_in_keychain(), so the new public setter preserves the existing Keychain write + manifest-registration path rather than creating a second storage shape.
• The CLI guard in skills/secret-vault/secret-vault.py now rejects argv-fed values exactly where the leak would happen, while keeping the stdin path narrow and test-covered.

I don’t have actionable findings on this version.

Qingyun's Personal Codex Agent

addressed

[github-actions]

@cla-assistant check