fix: make macOS Keychain repair action-based#70
Conversation
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 85bc8fdb14a1
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| automation:ci-release | 1 |
| policies:conventions | 1 |
| structure:repo-health | 0 |
| documentation:docs | 2 |
automation:ci-release (1 finding)
Minor - .github/workflows/ci.yml:68
The
test-scriptsjob runs only onubuntu-latest, butrepair-macos-keychain-credentials.shis a macOS Keychain script. If--self-testdetects a non-macOS host and early-exits (the common pattern for macOS-specific repair scripts), this job validates only that the script is bash-syntactically valid and that--self-testdoesn't crash on Linux — not that any repair logic executes correctly. Thebash -nstep is genuinely cross-platform useful, but for the self-test to provide meaningful automation coverage it needs a macOS runner.Fix: change the runner to
macos-latest, or add a matrix so the job runs on bothubuntu-latest(for the syntax check) andmacos-latest(for the self-test logic). If--self-testis intentionally designed to exercise full logic cross-platform without hitting macOS APIs, add a comment to the workflow step explaining that so the choice is not silently reversed by a future editor.
policies:conventions (1 finding)
Minor - .github/workflows/ci.yml:63
ci.md §1requires aconcurrencygroup keyed on the ref withcancel-in-progress: true(MUST). The workflow has noconcurrencyblock, so rapid pushes or re-pushes on the same ref queue redundant runs rather than cancelling the superseded one. This is pre-existing, but this PR modifiesci.ymland is the natural place to add it.Suggested addition immediately after the
on:block:concurrency: group: ci-${{ github.ref }} cancel-in-progress: true
documentation:docs (2 findings)
Nits - scripts/README.md:1
The README documents
--heal,--cleanup,--rebuild,--apply, and--toolbut omits--keychain PATH, which the script supports and lists in its usage string. A user who needs to target a non-default keychain path has no indication this flag exists. Add a short example such as:scripts/repair-macos-keychain-credentials.sh --keychain ~/Library/Keychains/login.keychain-db
Nits - scripts/README.md:45
The
--toolsection shows example usage withcrandnrqbut omits the list of valid values. The script's usage string documents them as:slck, nrq, gro, jtk, cfl, atlassian, cr, sfdc. Without this list in the README a reader must run--helpor open the script to discover what tools are supported. Add a brief note after the example, e.g.: "Supported values:slck,nrq,gro,jtk,cfl,atlassian,cr,sfdc."
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| automation:ci-release | complete_broad | .github/workflows/ci.yml, Makefile | unavailable | The script under test (scripts/repair-macos-keychain-credentials.sh) was not in the assigned file list; its --self-test implementation could not be inspected directly, so the assessment of what it exercises on Linux is based on context from the PR description. |
| policies:conventions | complete_broad | .github/workflows/ci.yml, Makefile, scripts/repair-macos-keychain-credentials.sh | unavailable | Branch protection settings are not visible in the diff; cannot verify whether the new test-scripts CI job is listed as a required check. |
| structure:repo-health | complete_broad | .github/workflows/ci.yml, Makefile, docs/development.md, scripts/README.md | unavailable | unavailable |
| documentation:docs | complete_broad | docs/development.md, scripts/README.md | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 44s | ~$1.21 (est.) | claude-sonnet-4-6 | cr 0.10.243
| Field | Value |
|---|---|
| Model | claude-sonnet-4-6 |
| Reviewers | automation:ci-release, policies:conventions, structure:repo-health, documentation:docs |
| Engine | claude_cli · claude-sonnet-4-6 |
| Reviewed by | cr · piekstra-dev |
| Duration | 5m 44s wall · 10m 42s compute |
| Cost | ~$1.21 (est.) |
| Tokens | 54 in / 29.2k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-4-6 | 5 | 1.8k | 24.6k | 10.9k | ~$0.08 (est.) | 40s |
| automation:ci-release | claude-sonnet-4-6 | 6 | 3.2k | 52.6k | 14.0k | ~$0.12 (est.) | 1m 04s |
| policies:conventions | claude-sonnet-4-6 | 13 | 8.0k | 317.5k | 47.4k | ~$0.39 (est.) | 3m 03s |
| structure:repo-health | claude-sonnet-4-6 | 13 | 8.9k | 232.9k | 24.3k | ~$0.29 (est.) | 3m 02s |
| documentation:docs | claude-sonnet-4-6 | 12 | 6.9k | 187.0k | 20.9k | ~$0.24 (est.) | 2m 03s |
| orchestrator-rollup | claude-sonnet-4-6 | 5 | 532 | 44.4k | 19.1k | ~$0.09 (est.) | 48s |
| scripts/repair-macos-keychain-credentials.sh --rebuild --tool nrq --apply | ||
| ``` | ||
|
|
||
| Limit discovery to one or more tools: |
There was a problem hiding this comment.
The --tool section shows example usage with cr and nrq but omits the list of valid values. The script's usage string documents them as: slck, nrq, gro, jtk, cfl, atlassian, cr, sfdc. Without this list in the README a reader must run --help or open the script to discover what tools are supported. Add a brief note after the example, e.g.: "Supported values: slck, nrq, gro, jtk, cfl, atlassian, cr, sfdc."
Reply inline to this comment.
| version: v2.12.2 | ||
|
|
||
| test-scripts: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The test-scripts job runs only on ubuntu-latest, but repair-macos-keychain-credentials.sh is a macOS Keychain script. If --self-test detects a non-macOS host and early-exits (the common pattern for macOS-specific repair scripts), this job validates only that the script is bash-syntactically valid and that --self-test doesn't crash on Linux — not that any repair logic executes correctly. The bash -n step is genuinely cross-platform useful, but for the self-test to provide meaningful automation coverage it needs a macOS runner.
Fix: change the runner to macos-latest, or add a matrix so the job runs on both ubuntu-latest (for the syntax check) and macos-latest (for the self-test logic). If --self-test is intentionally designed to exercise full logic cross-platform without hitting macOS APIs, add a comment to the workflow step explaining that so the choice is not silently reversed by a future editor.
Reply inline to this comment.
| @@ -63,3 +63,9 @@ jobs: | |||
| - uses: golangci/golangci-lint-action@v7 | |||
There was a problem hiding this comment.
File-level note: .github/workflows/ci.yml
ci.md §1 requires a concurrency group keyed on the ref with cancel-in-progress: true (MUST). The workflow has no concurrency block, so rapid pushes or re-pushes on the same ref queue redundant runs rather than cancelling the superseded one. This is pre-existing, but this PR modifies ci.yml and is the natural place to add it.
Suggested addition immediately after the on: block:
concurrency:
group: ci-${{ github.ref }}
cancel-in-progress: trueReply inline to this comment.
| @@ -0,0 +1,68 @@ | |||
| # Scripts | |||
There was a problem hiding this comment.
File-level note: scripts/README.md
The README documents --heal, --cleanup, --rebuild, --apply, and --tool but omits --keychain PATH, which the script supports and lists in its usage string. A user who needs to target a non-default keychain path has no indication this flag exists. Add a short example such as:
scripts/repair-macos-keychain-credentials.sh --keychain ~/Library/Keychains/login.keychain-dbReply inline to this comment.
2 participants
Summary
Safety model
Validation