Skip to content

fix(credstore): return exact bytes from macOS Keychain reads - #78

Merged
piekstra merged 4 commits into
mainfrom
fix/credstore-keychain-read-bytes
Nov 3, 2026
Merged

fix(credstore): return exact bytes from macOS Keychain reads#78
piekstra merged 4 commits into
mainfrom
fix/credstore-keychain-read-bytes

Conversation

@piekstra

@piekstra piekstra commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Problem

The stable read added in 86498ad runs security find-generic-password -w. -w prints a secret holding any non-printable byte as bare hex, with no marker, so the read returned the hex string as the secret and reported success.

Measured against real /usr/bin/security in a throwaway keychain:

Stored secret -w prints -g prints
abc123 abc123 password: "abc123"
deadbeef deadbeef password: "deadbeef"
line1\nline2 6c696e65310a6c696e6532 password: 0x6C696E65310A6C696E6532 "line1…
tok\n 746f6b0a password: 0x746F6B0A "tok…
UTF-8 päss 70c3a47373 password: 0x70C3A47373 "p\303\244ss"
\xff\xfe fffe password: 0xFFFE

Rows 2 and 6 show why -w output cannot be repaired after the fact: both print only hex digits, and only one is hex-encoded.

Consumers feel this as a silently wrong credential: a PEM private key fails pem.Decode, for example in codereview-cli's GitHub App auth. The existing test used a fake runner that echoed raw bytes, so it passed.

scripts/repair-macos-keychain-credentials.sh had the same read and it destroyed data: it deleted the item and recreated it holding the hex text.

docs/distribution.md §2A also claimed that a stable designated requirement makes a grant survive rebuilds. On a binary with no TeamIdentifier the partition list gets a per-build cdhash: entry, so a native read prompts again.

Change

  • The read uses -g and parses its stderr, which marks the hex form with 0x. Parsing returns an error for any other shape, and that error never quotes the output.
  • A read that times out names the pending approval of /usr/bin/security.
  • The repair script reads with -g too, and refuses any unrecognized shape before the delete. It also refuses a secret too long for one security -i line, which the old flow truncated after deleting the original. It reads each recreated item back and restores on a mismatch, and recreated items trust /usr/bin/security. Its top-level flow moved into main so a test can source its functions.
  • Darwin tests run real /usr/bin/security against a throwaway keychain in a temp directory: one pins the parser to actual output, the other runs the script's delete-and-recreate step and reads each secret back. They touch no real keychain and raise no dialog. The first found the 0xFFFE shape, which has no display text.
  • §2A now states the ACL and partition-list behaviour, and what a user sees: one prompt for security per item that does not yet trust it, then none across upgrades.

Verification

Check Result
make check Pass: tidy, 0 lint issues, go test -race, build
go test -tags keyring_no1password,keyring_nopassage ./credstore Pass
GOOS=windows go vet ./credstore, CGO_ENABLED=0 go vet ./credstore, shellcheck on the script Pass
Two ad-hoc-signed builds with different hashes, Store.Get on synthetic login-keychain items created with -T /usr/bin/security Both read the exact bytes of a printable value and a PEM shape with no prompt. Items removed afterwards.

Not verified: the interactive prompt and Always Allow click, because a probe raises a dialog on the owner's screen, and a real upgrade of an installed CLI.

Release

This is a behaviour change to credstore, so the tag waits for the consumer matrix in docs/development.md. The consumer test results against the merge commit go on this PR before v0.5.1 is cut.

@piekstra
fix(credstore): return exact bytes from macOS Keychain reads
fb45be6 
The read through /usr/bin/security used -w, which prints a secret holding any non-printable byte as bare hex. A PEM key, a multi-line value, a trailing newline or UTF-8 came back as a hex string with no error. -g marks the hex form with a 0x prefix, so the read now parses that and returns the stored bytes.

Correct distribution.md section 2A: a stable designated requirement does not carry a grant across rebuilds on a binary without a TeamIdentifier, because the partition list gets a per-build cdhash entry.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev reviewed Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: fb45be6e4693
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:drift 0
go:implementation-tests 1
privacy:captured-data 0
policies:conventions 0
security:code-auditor 0
go:implementation 0
documentation:docs 0
architecture:system 1
go:implementation-tests (1 finding)

Minor - credstore/osbackend_keychain_stable_read_darwin_test.go:27

The security helper closes over the parent test's t and calls t.Fatalf on it. It is also invoked from inside t.Run subtests (the add-generic-password calls at lines 61 and 70). FailNow and Fatalf must run on the goroutine of the test that owns the t. Calling the parent's Fatalf from a subtest goroutine marks the parent failed and then Goexits the subtest goroutine. That can surface as a confusing 'test executed panic(nil) or runtime.Goexit' failure instead of a clean subtest failure. Fix: have the helper take a *testing.T (security(t, args...)) and pass the subtest's t, or define the helper as a function with t as its first parameter. Keep the parent's t only for the keychain creation and cleanup.

architecture:system (1 finding)

Major - credstore/osbackend_keychain_stable_read.go:56

This fix moves the library off security -w because -w prints any secret holding a non-printable byte as bare hex, but the family's other Keychain reader still uses -w, and there the wrong value is written back and the original destroyed. The script is unchanged by this PR, so the defect is pre-existing; this PR is what establishes the root cause, and docs/distribution.md §2A and docs/working-with-secrets.md both point users at the script, the latter in the same paragraph that now says -w is not usable.

Scenario: a user runs scripts/repair-macos-keychain-credentials.sh --apply with a multi-line secret stored locally, such as the PEM private key this PR names for codereview-cli's GitHub App auth (add_cr_targets enrols cr credential refs). repair_item captures the value with -w (line 266), so it holds the hex text. It hex-encodes that text again (line 270), deletes the original item (line 279), and recreates it with -X (line 291). The item now stores the ASCII hex text as its secret, and the script prints REPAIRED. Because this PR makes reads return exact bytes, every later read returns that hex text, so pem.Decode fails, with no copy of the original left to restore. The same $(...) capture would also strip trailing newlines from any value.

I confirmed the capture step against real <path> in a throwaway keychain: a four-byte secret ending in a newline came back from $(security ... -w) as its 8-character hex text, which to_hex then re-encodes into 16 hex digits. I did not run the script's --apply path end to end.

Fix: read with -g in the script too, and take the 0x hex digits directly when present, otherwise hex-encode the quoted text, so -X receives the stored bytes. Abort before the delete on any unrecognized shape, mirroring parseKeychainPassword. Do this in this PR or as a tracked blocker for v0.5.1, because the docs now present the two paths as consistent.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:drift — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; context (not coverage): 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; constraints: Compared against docs/distribution.md §2A, docs/working-with-secrets.md and the repair script by grep; did not execute the code or
  • go:implementation-tests — complete (broad); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; skipped: none; constraints: Static review only; the darwin test against real was not run.
  • privacy:captured-data — complete (constrained); scope: 5 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; body-inspected: 5 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: none
  • policies:conventions — complete (broad); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Convention review only; the parser's correctness against real security output was not executed. Could not verify the consumer-matrix release gate in docs/development.md beyond the PR description.
  • security:code-auditor — complete (broad); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; skipped: none; constraints: Only the first 1500 bytes of the darwin test file were read; the rest of that file was not read. The main source file was read in full. Static review only; tests were not executed and real output was not observed.
  • go:implementation — complete (broad); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; constraints: No code was executed; output shapes were taken from the PR description. Review limited to the assigned file body; test files (including the darwin real-security test) were listed but not read in detail.
  • documentation:docs — complete (broad); scope: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; body-inspected: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; skipped: none; context (not coverage): 1 file; examples: credstore/osbackend_keychain_stable_read.go; constraints: Did not run the interactive Keychain prompt flow; ACL and partition-list behavior claims rest on the PR description and were not independently verified. Only the 60s timeout claim was checked against code (credstore/osbackend_keychain_stable_read.go).
  • architecture:system — complete (broad); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; context (not coverage): 7 files; examples: credstore/migration.go, credstore/osbackend_byteness.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md; 2 omitted; constraints: Not verified: output and exit code of security -g when the user denies the prompt or the keychain is locked, because that raises a dialog. An exit 0 with no password line would surface as the format error, not a denial; nothing observed shows this happens. Not verified: the 60-second timeout against a real pending prompt, or a real CLI upgrade, for the same reason. Ran the PR's real-security tests plus an added probe in a temp copy against a throwaway keychain: every byte value alone and betw…

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 4m 15s | $1.37 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.363
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:drift, go:implementation-tests, privacy:captured-data, policies:conventions, security:code-auditor, go:implementation, documentation:docs, architecture:system
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 4m 15s wall · 6m 27s compute
Cost $1.37
Tokens 98 in / 23.9k out

Per-workstream usage

  • orchestrator-selection — claude-sonnet-5-5
    • In: 6
    • Out: 1.6k
    • Cache read: 21.0k
    • Cache create: 13.2k
    • Cost: $0.07
    • Duration: 14s
  • documentation:drift — claude-sonnet-5-5
    • In: 12
    • Out: 1.7k
    • Cache read: 75.6k
    • Cache create: 16.3k
    • Cost: $0.09
    • Duration: 25s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 8
    • Out: 2.0k
    • Cache read: 40.1k
    • Cache create: 16.1k
    • Cost: $0.09
    • Duration: 22s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 43.7k
    • Cache create: 16.9k
    • Cost: $0.08
    • Duration: 15s
  • policies:conventions — claude-sonnet-5-5
    • In: 10
    • Out: 1.2k
    • Cache read: 60.7k
    • Cache create: 17.7k
    • Cost: $0.09
    • Duration: 17s
  • security:code-auditor — claude-sonnet-5-5
    • In: 10
    • Out: 1.8k
    • Cache read: 52.6k
    • Cache create: 12.4k
    • Cost: $0.07
    • Duration: 19s
  • go:implementation — claude-sonnet-5-5
    • In: 8
    • Out: 1.2k
    • Cache read: 36.7k
    • Cache create: 11.7k
    • Cost: $0.06
    • Duration: 20s
  • documentation:docs — claude-sonnet-5-5
    • In: 14
    • Out: 1.7k
    • Cache read: 84.6k
    • Cache create: 13.0k
    • Cost: $0.08
    • Duration: 32s
  • architecture:system — claude-opus-5-5
    • In: 16
    • Out: 11.0k
    • Cache read: 203.3k
    • Cache create: 42.4k
    • Cost: $0.60
    • Duration: 3m 22s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 497
    • Cache read: 66.5k
    • Cache create: 4.6k
    • Cost: $0.13
    • Duration: 15s

Comment thread credstore/osbackend_keychain_stable_read_darwin_test.go Resolved
Comment thread credstore/osbackend_keychain_stable_read.go Resolved
@piekstra
fix(scripts): keep secret bytes when repairing Keychain items
44f62c4 
repair_item read the secret with security -w, so a multi-line value came back as hex text. The script then deleted the item and recreated it holding that text, destroying the credential. It now reads with -g and refuses any unrecognized shape before the delete.

Fail the darwin test on the subtest that owns the failure rather than on its parent.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev requested changes Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 44f62c4fbd7d
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:drift 0
go:implementation-tests 0
privacy:captured-data 0
policies:conventions 0
security:code-auditor 0
go:implementation 0
documentation:docs 0
architecture:system 1
architecture:system (1 finding)

Blocking - credstore/osbackend_keychain_stable_read.go:59

44f62c4 fixes how scripts/repair-macos-keychain-credentials.sh reads a secret, so the read now matches this line, but the script's write path still destroys any secret longer than about 1.9 KB. The defect is in the script, which is outside this review's assigned file, so it is anchored here. The delete-then-recreate order is older than this PR, but this PR rewrites that path, describes it as keeping secret bytes, and tests it only with short values.

Scenario: a user runs the script with --apply on a long credential, such as an RSA-4096 PEM private key of about 3 KB, the same class of credential this PR cites for codereview-cli. repair_item reads the secret and deletes the original item (security delete-generic-password). It then pipes one add-generic-password ... -X <hex> line into security -i. Hex doubles the length, and security -i splits lines at a limit near 4096 characters. The first fragment still parses, with its hex cut short, and is stored as the secret; the rest fails as an unknown command. The restore attempt builds an equally long line and hits the same limit, and its failure is ignored by || true. The original is gone and the item holds a silently truncated secret.

Measured with the test's repair_item driver and real <path>, using synthetic values: 1600- and 1900-byte secrets were repaired. 2000-, 2100-, 3300- and 6000-byte secrets each ended with a 1983-byte stored value. The script printed the recreate failed error in the runs where that line was visible in my capture, and it prints no warning that the value was truncated. The cutoff shifts with the lengths of service, account, label and app path.

The script's restore comment and §2A's description of it as a repair path imply the item survives a failure; here it does not. TestRepairScriptKeepsSecretBytes has no long case.

Fix: in repair_item, build the full command before the delete and refuse with an error, leaving the item untouched, if the line exceeds a conservative limit such as 4000 characters. Also read the recreated item back and compare its hex to the original, reporting a mismatch loudly and without printing the value. Add a test that an over-limit secret is refused and the item is unchanged.

Caution for that test: the driver appends the keychain path at the end of the line, so a line that reaches security -i over the limit loses the path and writes to the user's default keychain. The refusal must happen before any security -i call, or the test will pollute the real login keychain.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:drift — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; skipped: none; context (not coverage): 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; constraints: Compared by reading and grep only; did not execute the script, the code, or
  • go:implementation-tests — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; skipped: none; constraints: Static review only; the darwin tests against real were not run. The repair shell script and docs are outside this agent's assigned scope; only the Go test that drives the script was reviewed.
  • privacy:captured-data — complete (constrained); scope: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; body-inspected: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; skipped: none; constraints: none
  • policies:conventions — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Convention review only; nothing was executed, including the parser and the repair script tests. The consumer-matrix release gate in docs/development.md was not verified beyond the PR description. The repair script and the darwin test files are outside the assigned scope and were not inspected as review targets.
  • security:code-auditor — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; skipped: none; constraints: Static review only; tests and real is outside assignment scope and was not reviewed.
  • go:implementation — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; constraints: No code was executed; output shapes were taken from the PR description. Review limited to the assigned file's diff and body; tests and the repair script were not reviewed.
  • documentation:docs — complete (constrained); scope: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; body-inspected: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Did not run the interactive Keychain prompt flow; ACL and partition-list behavior claims rest on the PR description and were not independently verified. The 60s timeout claim was checked against code only in the earlier review of the previous head; the docs diff is unchanged at this head.
  • architecture:system — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; context (not coverage): 5 files; examples: credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md, scripts/repair-macos-keychain-credentials.sh; constraints: Not verified: security -g output and exit code when a prompt is denied or the keychain is locked, the 60-second timeout against a real prompt, or a real CLI upgrade, because each needs a dialog or an installed release. The assigned Go file is unchanged since the previous review of fb45be6; this pass covered 44f62c4's script and test changes as context. The PR's repair, read and parser tests pass in a temp copy. The length probe used the test's driver, which appends a keychain path, so the exa…

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 3m 05s | $2.31 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.363
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:drift, go:implementation-tests, privacy:captured-data, policies:conventions, security:code-auditor, go:implementation, documentation:docs, architecture:system
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 3m 05s wall · 4m 37s compute
Cost $2.31
Tokens 76 in / 17.3k out

Per-workstream usage

  • documentation:drift — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 105.2k
    • Cache create: 13.7k
    • Cost: $0.17
    • Duration: 21s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 103.1k
    • Cache create: 13.2k
    • Cost: $0.16
    • Duration: 18s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 111.1k
    • Cache create: 16.1k
    • Cost: $0.17
    • Duration: 18s
  • policies:conventions — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 105.7k
    • Cache create: 10.7k
    • Cost: $0.15
    • Duration: 21s
  • security:code-auditor — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 88.7k
    • Cache create: 13.2k
    • Cost: $0.14
    • Duration: 14s
  • go:implementation — claude-sonnet-5-5
    • In: 8
    • Out: 864
    • Cache read: 84.5k
    • Cache create: 11.9k
    • Cost: $0.13
    • Duration: 27s
  • documentation:docs — claude-sonnet-5-5
    • In: 8
    • Out: 1.0k
    • Cache read: 89.3k
    • Cache create: 11.9k
    • Cost: $0.14
    • Duration: 17s
  • architecture:system — claude-opus-5-5
    • In: 14
    • Out: 9.4k
    • Cache read: 416.9k
    • Cache create: 26.6k
    • Cost: $1.08
    • Duration: 2m 02s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 567
    • Cache read: 79.9k
    • Cache create: 4.3k
    • Cost: $0.16
    • Duration: 16s

Comment thread credstore/osbackend_keychain_stable_read.go Resolved
@piekstra
fix(scripts): refuse Keychain repairs that would truncate a secret
024735d 
security -i splits input lines near 4096 characters, so a long secret was truncated after the delete had removed the original. repair_item now builds the command first and refuses an over-long one with the item unchanged. It reads the recreated item back, compares it with the original, and restores on a mismatch.

Recreated items also trust /usr/bin/security, which credstore reads through, so a repair no longer leaves one more prompt behind.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev approved these changes Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 024735dd3f2a
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:drift 0
go:implementation-tests 0
privacy:captured-data 0
policies:conventions 0
security:code-auditor 0
go:implementation 0
documentation:docs 0
architecture:system 2
architecture:system (2 findings)

Minor - credstore/osbackend_keychain_stable_read.go:3

The guard added to runRepairItem in credstore/osbackend_keychain_stable_read_darwin_test.go (outside this review's assigned file, so anchored here) measures the line before the keychain path is appended, so it does not fully hold the settled thread's claim that the driver cannot write to the default keychain.

Scenario: a developer or tool runs go test ./credstore on darwin with a long TMPDIR. This is not hypothetical: cr's own review subprocess sets a TMPDIR of 258 characters, and t.TempDir() adds more. The "long secret that fits one line" case builds a line of roughly 3840 characters, which passes the > 4000 check. Adding the quoted keychain path takes it past the roughly 4096-character limit of security -i, so the path is cut. The add-generic-password then no longer names the intended throwaway keychain.

The consequence is likely a synthetic item in the user's real login keychain, plus a confusing test failure, which contradicts the test's stated guarantee of touching no real keychain. My evidence is from the previous review pass: a line cut inside the appended path left its item, with the full secret, in the login keychain. I did not reproduce this against the current driver, because doing so would write to the real keychain again.

Fix: check the final length in the driver, meaning ${#line} plus the keychain path and its quotes, and stop before calling security -i if it is over the limit. Also create this keychain under a short fixed root, such as os.MkdirTemp("<path>", ...), so the 1800-byte case fits whatever TMPDIR is set to.

Minor - credstore/osbackend_keychain_stable_read.go:3

The new over-limit refusal in scripts/repair-macos-keychain-credentials.sh (outside this review's assigned file, so anchored here) stops the whole batch, not just the one item. The script's usage text says only that such a secret "is refused and the item is left unchanged".

Scenario: a user runs --apply with a credential over about 1.9 KB, such as a long PEM under a cr ref, followed by other targets. repair_item returns 1 at the refusal. main calls it bare inside its loop under set -euo pipefail, so the script exits there. Every later target is left unrepaired and the Notes section is never printed. Because the refusal depends only on secret length, every re-run stops at the same item, and the script has no option to skip it, so those later items can never be repaired by this script.

I confirmed this by sourcing the script with security replaced by a stub, so no keychain was involved: with three targets and an over-limit first item, only the first was processed, and the script ended right after its refusal message. Other repair_item errors already abort the same way, but they are transient, whereas this one is permanent for that credential.

Fix: in main, run repair_item ... || failed=1, finish the loop, print the notes, then exit non-zero if any item failed. No item's state depends on an earlier one, so continuing is safe.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:drift — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; skipped: none; context (not coverage): 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; constraints: Compared by reading and grep only; did not execute the script, the code, or
  • go:implementation-tests — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; skipped: none; constraints: Static review only; the darwin tests against real were not run. The repair shell script and docs are outside this agent's assigned scope; only the Go test that drives the script was reviewed.
  • privacy:captured-data — complete (constrained); scope: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; body-inspected: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; skipped: none; constraints: none
  • policies:conventions — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Convention review only; nothing was executed, including the parser and the repair script tests. The consumer-matrix release gate in docs/development.md was not verified beyond the PR description. The repair script and darwin test files are outside the assigned scope and were not inspected as review targets.
  • security:code-auditor — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; skipped: none; constraints: Static review only; tests and real is outside assignment scope and was not reviewed; the darwin test's repair-script driver was read only as test code.
  • go:implementation — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; constraints: No code was executed; output shapes were taken from the PR description. Review limited to the assigned Go file; tests and the repair script were not reviewed.
  • documentation:docs — complete (constrained); scope: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; body-inspected: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Did not run the interactive Keychain prompt flow; ACL and partition-list behavior claims rest on the PR description and were not independently verified. Docs files are unchanged since the previous reviewed head (44f62c4); the 60s timeout claim was checked against code in an earlier review.
  • architecture:system — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; context (not coverage): 2 files; examples: credstore/osbackend_keychain_stable_read_darwin_test.go, scripts/repair-macos-keychain-credentials.sh; constraints: I did not run the PR's darwin tests or any real security command this pass: with this environment's long TMPDIR the repair test could write to the real login keychain. The script's behaviour was checked with a stubbed security, and the fix's real-keychain paths were read, not executed. Not verified: security -g output and exit code when a prompt is denied or the keychain is locked, the 60-second timeout against a real prompt, or a real CLI upgrade, because each needs a dialog or an instal…

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 14s | $3.25 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.363
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:drift, go:implementation-tests, privacy:captured-data, policies:conventions, security:code-auditor, go:implementation, documentation:docs, architecture:system
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 2m 14s wall · 3m 34s compute
Cost $3.25
Tokens 70 in / 17.3k out

Per-workstream usage

  • documentation:drift — claude-sonnet-5-5
    • In: 8
    • Out: 1.3k
    • Cache read: 157.7k
    • Cache create: 11.3k
    • Cost: $0.24
    • Duration: 16s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 8
    • Out: 1.0k
    • Cache read: 155.8k
    • Cache create: 12.7k
    • Cost: $0.24
    • Duration: 17s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 173.5k
    • Cache create: 13.8k
    • Cost: $0.25
    • Duration: 16s
  • policies:conventions — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 147.3k
    • Cache create: 8.9k
    • Cost: $0.21
    • Duration: 17s
  • security:code-auditor — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 143.9k
    • Cache create: 15.5k
    • Cost: $0.23
    • Duration: 12s
  • go:implementation — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 130.8k
    • Cache create: 10.2k
    • Cost: $0.19
    • Duration: 18s
  • documentation:docs — claude-sonnet-5-5
    • In: 8
    • Out: 1.0k
    • Cache read: 135.1k
    • Cache create: 9.9k
    • Cost: $0.21
    • Duration: 13s
  • architecture:system — claude-opus-5-5
    • In: 8
    • Out: 9.0k
    • Cache read: 317.6k
    • Cache create: 18.5k
    • Cost: $1.48
    • Duration: 1m 33s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 593
    • Cache read: 93.1k
    • Cache create: 4.6k
    • Cost: $0.20
    • Duration: 9s

Comment thread credstore/osbackend_keychain_stable_read.go Resolved
Comment thread credstore/osbackend_keychain_stable_read.go Resolved
@piekstra
fix(scripts): continue the repair batch after a refused item
97ddc05 
A refused secret stayed refused on every run, so stopping at it left later items unrepairable. main now finishes the batch, prints the notes and exits non-zero if any item failed.

The test driver counts the keychain path in its line limit and uses a short temp root, so a long TMPDIR cannot push a line past security -i and into the default keychain.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev approved these changes Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 97ddc0535855
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:drift 0
go:implementation-tests 0
privacy:captured-data 0
policies:conventions 0
security:code-auditor 0
go:implementation 0
documentation:docs 0
architecture:system 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:drift — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, scripts/repair-macos-keychain-credentials.sh; skipped: none; context (not coverage): 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; constraints: Compared by reading and grep only; did not execute the script, the code, or
  • go:implementation-tests — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go; skipped: none; constraints: Static review only; the darwin tests against real were not run. The repair shell script and docs are outside this agent's assigned scope; only the Go test that drives the script was reviewed.
  • privacy:captured-data — complete (constrained); scope: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; body-inspected: 6 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go, credstore/osbackend_keychain_stable_read_test.go, docs/distribution.md, docs/working-with-secrets.md; 1 omitted; skipped: none; constraints: none
  • policies:conventions — complete (constrained); scope: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; body-inspected: 3 files; examples: credstore/osbackend_keychain_stable_read.go, docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Convention review only; nothing was executed, including the parser and the repair script tests. The consumer-matrix release gate in docs/development.md was not verified beyond the PR description. The repair script and darwin test files are outside the assigned scope and were not inspected as review targets.
  • security:code-auditor — complete (constrained); scope: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; body-inspected: 2 files; examples: credstore/osbackend_keychain_stable_read.go, credstore/osbackend_keychain_stable_read_darwin_test.go; skipped: none; constraints: Of the darwin test file, only lines 100-266 were read in this pass. Lines 1-99 changed in this commit (likely shortTempDir use) and were not re-read; I read an earlier version of them in a prior pass. Static review only; tests and real is unchanged since the previous head 024735d, which I reviewed; this pass confirmed that only via the commit diff stat, not a fresh read. scripts/repair-macos-keychain-credentials.sh is o…
  • go:implementation — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; constraints: Manifest stats for the assigned Go file (74 additions, 11 deletions) match the earlier reviewed commits; I did not re-read the file at this head, relying on the earlier read. Tests and the repair script were not reviewed; no code was executed, and output shapes were taken from the PR description.
  • documentation:docs — complete (constrained); scope: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; body-inspected: 2 files; examples: docs/distribution.md, docs/working-with-secrets.md; skipped: none; constraints: Did not run the interactive Keychain prompt flow; ACL and partition-list behavior claims rest on the PR description and were not independently verified. Docs files are unchanged since the previously reviewed head (024735d); the 60s timeout claim was checked against code in an earlier review.
  • architecture:system — complete (constrained); scope: 1 file; examples: credstore/osbackend_keychain_stable_read.go; body-inspected: 1 file; examples: credstore/osbackend_keychain_stable_read.go; skipped: none; context (not coverage): 2 files; examples: credstore/osbackend_keychain_stable_read_darwin_test.go, scripts/repair-macos-keychain-credentials.sh; constraints: Not verified: security -g output and exit code when a prompt is denied or the keychain is locked, the 60-second timeout against a real prompt, or a real CLI upgrade, because each needs a dialog or an installed release. Ran TestRepairScriptKeepsSecretBytes and the real-security round-trip test in a temp copy with the long TMPDIR still set: both pass. Attribute-only lookups before and after found no probe service items in the keychain search list. held no leftover test directories. The ass…

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 1m 14s | $4.07 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.363
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:drift, go:implementation-tests, privacy:captured-data, policies:conventions, security:code-auditor, go:implementation, documentation:docs, architecture:system
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 1m 14s wall · 3m 11s compute
Cost $4.07
Tokens 70 in / 12.0k out

Per-workstream usage

  • documentation:drift — claude-sonnet-5-5
    • In: 8
    • Out: 1.2k
    • Cache read: 203.5k
    • Cache create: 11.4k
    • Cost: $0.32
    • Duration: 18s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 206.4k
    • Cache create: 12.3k
    • Cost: $0.32
    • Duration: 23s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.0k
    • Cache read: 228.5k
    • Cache create: 13.3k
    • Cost: $0.34
    • Duration: 21s
  • policies:conventions — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 183.5k
    • Cache create: 9.1k
    • Cost: $0.28
    • Duration: 14s
  • security:code-auditor — claude-sonnet-5-5
    • In: 10
    • Out: 2.0k
    • Cache read: 263.2k
    • Cache create: 14.6k
    • Cost: $0.34
    • Duration: 32s
  • go:implementation — claude-sonnet-5-5
    • In: 6
    • Out: 804
    • Cache read: 124.5k
    • Cache create: 10.1k
    • Cost: $0.25
    • Duration: 12s
  • documentation:docs — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 175.4k
    • Cache create: 10.2k
    • Cost: $0.28
    • Duration: 14s
  • architecture:system — claude-opus-5-5
    • In: 8
    • Out: 3.4k
    • Cache read: 386.9k
    • Cache create: 12.9k
    • Cost: $1.73
    • Duration: 46s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 467
    • Cache read: 105.8k
    • Cache create: 3.4k
    • Cost: $0.22
    • Duration: 7s

@piekstra
piekstra merged commit f6def99 into main Nov 3, 2026
4 checks passed
@piekstra
piekstra deleted the fix/credstore-keychain-read-bytes branch November 3, 2026 12:02
@piekstra

piekstra commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Consumer matrix against merge commit f6def99, each repo at its current main with cli-common pinned to that commit, built and tested with an empty HOME:

Consumer Build Tests
codereview-cli Pass 3933 pass, 34 skip, 3 fail
slack-chat-api Pass 920 pass, 1 skip
spotify-cli Pass 478 pass
newrelic-cli Pass 586 pass, 1 skip
google-cli Pass 1769 pass, 1 skip
granola-cli Pass 75 pass
atlassian-cli (shared, jtk, cfl) Pass 3839 pass, 2 skip

The 3 codereview-cli failures are TestPiRPCReviewerImageProcessingBounds, its output and its deadline subtests, which report tool exited null from a Node child under full-suite load. The test has no credstore dependency, the package passes when run alone at this pin, and the same test failed the same way under full-suite load on a codereview-cli branch still pinned to v0.4.1.

Tagging v0.5.1 on f6def99.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@piekstra-dev piekstra-dev piekstra-dev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@piekstra @piekstra-dev