feat: migrate credential storage to cli-common/credstore (Phase B pilot)#158
Conversation
Replace the hand-rolled internal/keychain (macOS `security` shell-out + Linux/Windows plaintext file) with the shared cli-common/credstore library, per the Open CLI Collective Secret-Handling Standard §2.4 + §1.3/§1.4/§1.7/ §1.8/§1.11/§1.12. slck is the first real credstore consumer; this PR's structure is the reusable per-CLI template for gro/atlassian. - internal/keychain is now a credstore adapter (OS keyring only; no shell-out, no plaintext file). Ref is authoritative: ParseRef(config credential_ref) -> Open(parsed-service) against parsed-profile. - New internal/config: config.yml (credential_ref, workspace, keyring.backend). - One-time legacy migration (§1.8/§2.4): old `slck` AND `slack-chat-api` Keychain services + the legacy plaintext credentials file -> the new bundle, with the api_token->bot_token rename. All conflicts detected before any write/delete; conflicts fail loud naming all locations and never print a value (§1.8/§1.12). Idempotent; --overwrite resolution. - _migration JSON signal (object-merge / non-object-wrap, consume-once) + one-line stderr notice. - New `slck set-credential` (stdin / --from-env ingress, §1.5.2). - §1.12 no-leak acceptance suite; all credential tests hermetic (file backend in a temp HOME), fixing the B0-deferred TestRunInit_WrongTokenType non-hermetic flake. BREAKING CHANGE: SLACK_API_TOKEN / SLACK_USER_TOKEN are no longer read at runtime (only as setup ingress via `init --bot-token-from-env`). Positional `config set-token <token>` and `init --bot-token <value>` value-flags are removed (use `slck set-credential --stdin` / `slck init`). Linux/Windows plaintext fallback is replaced by the OS keyring (fail-closed; opt-in encrypted-file backend). Closes #157 [INT-437]
|
Blocker
Major
Minor
|
- go.mod back to go 1.24.0 (pin x/term v0.27, x/sys v0.28 — latest needed go 1.25, breaking B0's 1.24 alignment and CI). - migration: legacy-vs-legacy disagreement is ALWAYS a conflict, even with --overwrite (it can't pick a winner; §1.8). - config clear opens WITHOUT migration (keychain.OpenNoMigrate) so the §1.8 remediation it advertises actually works during a conflict. - fail closed on an unrecognized config keyring.backend (no silent degradation to auto-select; §1.4). - init interactive token prompt is now no-echo (term.ReadPassword) with a non-terminal test seam (§1.12). - _migration block recorded ONLY on JSON runs (no stale block leaking into a later JSON response in a long-lived/test process). - purge residual 'config set-token'/plaintext guidance from search, errors, config test, whoami, README, and the Homebrew release caveats. - §1.12 no-leak suite rewritten as internal/noleak: every command class via its real cobra command, capturing stdout+stderr+writer. - narrow security-delete not-found classification to exit 44 only. - clear --all documents the (currently empty) cache scope explicitly. [INT-437]
|
Reviewed the live branch at Findings
The earlier issues around Go 1.24 compatibility, legacy-vs-legacy conflict behavior, |
- B: legacy macOS Keychain discovery is now independent of the destination backend (§2.4). A macOS user who opts into keyring.backend:file still gets old slck/slack-chat-api Keychain items migrated. Test hermeticity moved to a documented test-only env seam (SLCK_TEST_DISABLE_LEGACY_KEYCHAIN_SCAN, set by testutil) instead of coupling discovery to the backend. - M: no-leak suite now also asserts masked-prefix/suffix canaries and covers config test + representative API command classes (channels list text+json, messages history). - m: init --bot-token-from-env / --user-token-from-env fail closed on an empty/unset env var instead of silently reporting setup success. [INT-437]
|
Findings
The three re-review code fixes themselves look present: discovery is destination-backend independent, env-var ingress now fails closed when empty/unset, and the no-leak suite now includes full/prefix/suffix canaries plus more command classes. Verification: |
- no-leak suite: the JSON API path no longer passes vacuously. -o is a ROOT persistent flag; toggling output.JSON drives the real IsJSON() path instead, and API-class subtests now require a real (non-usage) execution error so a parser failure can't masquerade as a clean no-leak pass. Added root.Command() so the suite/B2/B3 can drive the real top-level command with persistent flags. - root_test.go is now hermetic (testutil.Setup → file backend in temp HOME; no real keyring / security shell-out) and asserts bot-vs-user resolution by the live error instead of a long-removed exact string. - purge remaining old credential guidance: snap/snapcraft.yaml caveat and integration-tests.md (setup, user-token, env-precedence, token-type, restore, error-table) — now teach init / set-credential --stdin and state env vars are not read at runtime. [INT-437]
|
Findings
The three re-review #3 majors look addressed: no-leak JSON now drives the real output path and asserts non-usage execution errors, root token-mode tests are hermetic, and the remaining old credential guidance is purged from the main user-facing paths. Verification: |
TDD / Test-Coverage Assessment (independent)Reviewed the diff and tests independently, ran the suite ( Strengths
Major
Minor
Nit
VerdictMust-fix before merge: none are hard blockers, but Major #1 (untested legacy-vs-legacy conflict) and Major #2 (no test for the headline runtime-env-not-read invariant) are the two I would want addressed since they are the highest-stakes claims in the PR and are currently asserted only by code/comments, not tests. Major #3 and the Minors are reasonable as fast-follows given the hermeticity constraints. |
- Extract the pure §1.8 resolver planMigration() out of migrateLegacyOverwrite (no I/O; injected current-value lookup) so every branch is unit-testable. TestPlanMigration now covers the legacy-vs-legacy disagreement branch (asserting --overwrite still cannot resolve it, and the error names all sources and leaks no value), plus legacy-vs-target / equal-target / identical-duplicate paths. - TestRuntimeEnvNotReadAsCredential locks the headline §1.11 invariant: SLACK_API_TOKEN/SLACK_USER_TOKEN set, empty keyring -> reads still fail missing (env never consulted at runtime). [INT-437]
|
Thanks — addressed both Majors and replying on the noted gaps (commit
|
|
Findings No findings. The Verification:
|
| func NewBotClient() (*Client, error) { | ||
| token, err := keychain.GetAPIToken() | ||
| st, err := keychain.Open() | ||
| if err != nil { |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-architecture-reviewer): keychain.Open() is called inside NewBotClient() and NewUserClient(), which runs the one-time legacy migration on every API call. A migration conflict (ErrMigrationConflict) will surface from arbitrary commands like slck channels list, not just setup commands, producing a confusing error with no clear remediation path visible from the failing command. Migration should either be pre-checked at startup in root's PersistentPreRunE, or these client constructors should use a migration-already-ran sentinel so the side effect is bounded to the first invocation.
Reply to this thread when addressed.
There was a problem hiding this comment.
By design, not changing. The §1.8 one-time migration is idempotent: once legacy originals are gone, discover() finds nothing and Open() is a cheap no-op — it does not "run migration on every API call" in steady state. A legacy/keyring conflict surfacing from channels list is the intended fail-closed behavior (§1.4/§1.8): slck must not operate with ambiguous credentials. The remediation path is visible and now unblocked — slck config show (diagnostic), slck config clear / slck delete-token (both OpenNoMigrate as of ea11ef5), or slck init --overwrite. A startup PersistentPreRunE pre-check would add a second migration entry point for no behavioral gain.
| hasUserToken := keychain.HasStoredUserToken() | ||
| st, err := keychain.Open() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-enforcement-reviewer): runDeleteToken calls keychain.Open() which runs the one-time legacy migration. If a migration conflict exists (ErrMigrationConflict), delete-token is blocked — leaving no escape hatch short of manually editing the keyring. The clear.go command explicitly uses OpenNoMigrate() for this reason. delete-token has the same remediation role and should use OpenNoMigrate() as well.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — delete-token now uses OpenNoMigrate(), matching config clear. It is a §1.8 remediation path ("delete the conflicting key, then re-run"); running migration first would surface ErrMigrationConflict and block the very command meant to resolve it.
| st, err := keychain.OpenRef(opts.ref) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-knowledge-reviewer): OpenRef(opts.ref) is called with runMigration=true. When a non-default --ref is supplied, the one-time legacy migration runs against the custom ref's service/profile rather than the canonical default ref, potentially writing migrated credentials into the wrong service/profile — the opposite of what a user specifying --ref intends. OpenRef should pass runMigration=false, or the --ref flag should be explicitly documented as 'skips migration'.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — OpenRef now passes runMigration=false. The one-time §1.8 migration only ever targets the canonical configured ref; running it against an arbitrary --ref could write the default ref's legacy data under the wrong service/profile. set-credential is pure ingress; migration still runs on the next init / first API call.
| Ref string `json:"credential_ref"` | ||
| Backend string `json:"backend"` | ||
| BackendSource string `json:"backend_source"` | ||
| PassphraseSource string `json:"passphrase_source,omitempty"` |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): runShow calls keychain.Open() which triggers the one-time legacy migration. If a migration conflict exists, the diagnostic command 'config show' fails with ErrMigrationConflict rather than displaying current credential state — giving users no diagnostic path to understand which keys are in which location before remediating. Consider using OpenNoMigrate() in runShow.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — config show now uses OpenNoMigrate(). It is the §1.11 item 3 diagnostic and must remain usable during an unresolved conflict so the user can see which keys are where before remediating.
| switch opts.key { | ||
| case keychain.KeyBotToken: | ||
| err = st.SetBotToken(value) | ||
| case keychain.KeyUserToken: |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): readValue() with --from-env silently returns an empty string when the named env var is unset, then falls through to the generic 'empty secret value rejected' error. Unlike init.go's resolveBot() which reports '--bot-token-from-env %s is empty or unset' (naming the var), the user has no way to know which env var was the problem. Add a targeted error naming the variable before the generic empty guard.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — readValue with --from-env now returns "--from-env is empty or unset" (parity with init resolveBot/resolveUser). The variable name is not secret; the value is never echoed (§1.12).
| } | ||
| return "" | ||
| return openWith(cfg, false, true) | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): OpenRef (used by slck set-credential --ref) calls openWith(..., runMigration=true), so specifying an alternate --ref on a fresh install will trigger migration against the DEFAULT ref's legacy data before writing to the requested ref. If migration surfaces a conflict, set-credential fails with an error that makes no sense in the context of a simple credential-write operation. OpenRef should use runMigration=false.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 (same change as the setcred.go:71 thread) — OpenRef now uses runMigration=false, so an alternate --ref no longer triggers migration against the default ref's legacy data.
| return err | ||
| } | ||
| botToken, err := opts.resolveBot() | ||
| if err != nil { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): In non-interactive mode (--bot-token-from-env, --user-token-from-env, --bot-token-stdin), the overwrite-existing-credentials prompt is skipped and tokens are written with WithOverwrite() without any feedback. A CI pipeline that accidentally re-runs slck init will silently replace existing credentials. Consider emitting an 'Overwriting existing credential at ' notice for non-interactive overwrites.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — non-interactive ingress now emits a stderr notice ("Notice: overwriting existing credential(s) at ") before replacing a stored credential, so an accidental CI re-run of slck init is visible. The ref is not secret; the value is never printed.
| } | ||
| if err := s.cs.Delete(s.profile, key); err != nil && !errors.Is(err, credstore.ErrNotFound) { | ||
| return fmt.Errorf("delete %s at %s: %w", key, s.ref, err) | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): del() uses an Exists pre-check to make Delete idempotent, but this is a TOCTOU pattern: a concurrent writer can add the key after Exists returns false, and a concurrent deleter can remove it after Exists returns true. The Delete call already handles ErrNotFound, so the Exists check is redundant. The idempotency contract is already satisfied by the errors.Is(err, ErrNotFound) guard on Delete alone. Remove the Exists pre-check.
Reply to this thread when addressed.
There was a problem hiding this comment.
Not redundant — keeping the Exists pre-check. The premise ("Delete already handles ErrNotFound") does not hold for the file backend: credstore's file backend returns a raw os "not found" error from Delete, not credstore.ErrNotFound, so errors.Is(err, ErrNotFound) does NOT catch it. This was established by a TDD-driven test (del() was genuinely non-idempotent on the file backend before the Exists guard). The TOCTOU concern is academic for a single-process CLI mutating its own keyring under the active profile. The rationale is documented at the call site (keychain.go:177-179).
|
|
||
| // Path is the config.yml location. | ||
| func Path() string { return filepath.Join(Dir(), configFileName) } | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): home, _ := os.UserHomeDir() silently discards the error. On a misconfigured system where UserHomeDir() fails, home is the empty string and Dir() returns /.config/slack-chat-api, which is outside the user's home and will likely fail with a permission error during Save(). The error should be propagated or produce an explicit error message.
Reply to this thread when addressed.
There was a problem hiding this comment.
Not changing. home, _ := os.UserHomeDir() matches the established convention across this codebase (e.g. migrate.go legacyCredentialsPath); UserHomeDir only fails when neither $HOME nor the OS user db is available, an exotic state in which Save() already fails loudly with a permission error naming the path. Threading an error through Dir()/Path() ripples into many call sites for an unreachable-in-practice case — non-surgical scope creep relative to this ticket (Code Quality Lens #3).
| if botToken == "" && userToken == "" { | ||
| output.Println("No tokens provided. Setup cancelled.") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): workspace = info.TeamID stores the machine-readable team ID (e.g. T04AB1234) in config.yml while verify() prints the human-readable info.Team to the user, so the stored value and the displayed value are different fields. The current intent is ambiguous — if config show is meant to be human-readable, info.Team is the right choice; if used programmatically, TeamID is correct but should be documented.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in ea11ef5 — added a comment documenting the intent: workspace stores info.TeamID (the stable machine identifier, durable across workspace renames) as a non-secret correlation field; verify() prints info.Team only for human feedback. This is deliberate, not an inconsistency.
|
|
||
| go 1.24 | ||
| go 1.24.0 | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): cli-common is pinned at a pseudo-version (v0.0.0-20260516182733-b753d5c62d14) rather than a tagged semver release. This library owns all OS keyring I/O and defines the AllowedKeys allowlist — it is the core security component of this PR. A pseudo-version gives no semantic contract: a breaking change or security fix in the upstream library will not be flagged by go get -u tooling. The library should be tagged before this PR merges, or the go.mod should document why a tag is not yet available.
Reply to this thread when addressed.
There was a problem hiding this comment.
Deferred deliberately, not blocking this PR. The pseudo-version is intentional during active Phase B credstore co-development — cli-common's API is still being shaped by each pilot CLI (slck is the first). Tagging a semver release is tracked at the epic level (INT-310) and cli-common will be pinned to a tag before Phase B closes, after the API has stabilized across gro/atlassian. Pinning to a premature tag now would force a churn of bump PRs across every CLI mid-migration.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 49aef89
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 5 |
| harness-engineering:harness-enforcement-reviewer | 3 |
| harness-engineering:harness-knowledge-reviewer | 3 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-architecture-reviewer (5 findings)
internal/client/client.go:45
keychain.Open() is called inside NewBotClient() and NewUserClient(), which runs the one-time legacy migration on every API call. A migration conflict (ErrMigrationConflict) will surface from arbitrary commands like
slck channels list, not just setup commands, producing a confusing error with no clear remediation path visible from the failing command. Migration should either be pre-checked at startup in root's PersistentPreRunE, or these client constructors should use a migration-already-ran sentinel so the side effect is bounded to the first invocation.
💡 Suggestion - internal/keychain/keychain.go:85
OpenRef (used by
slck set-credential --ref) calls openWith(..., runMigration=true), so specifying an alternate --ref on a fresh install will trigger migration against the DEFAULT ref's legacy data before writing to the requested ref. If migration surfaces a conflict, set-credential fails with an error that makes no sense in the context of a simple credential-write operation. OpenRef should use runMigration=false.
💡 Suggestion - internal/config/config.go:63
home, _ := os.UserHomeDir() silently discards the error. On a misconfigured system where UserHomeDir() fails, home is the empty string and Dir() returns /.config/slack-chat-api, which is outside the user's home and will likely fail with a permission error during Save(). The error should be propagated or produce an explicit error message.
💡 Suggestion - internal/cmd/initcmd/init.go:110
workspace = info.TeamID stores the machine-readable team ID (e.g. T04AB1234) in config.yml while verify() prints the human-readable info.Team to the user, so the stored value and the displayed value are different fields. The current intent is ambiguous — if config show is meant to be human-readable, info.Team is the right choice; if used programmatically, TeamID is correct but should be documented.
💡 Suggestion - go.mod:4
cli-common is pinned at a pseudo-version (v0.0.0-20260516182733-b753d5c62d14) rather than a tagged semver release. This library owns all OS keyring I/O and defines the AllowedKeys allowlist — it is the core security component of this PR. A pseudo-version gives no semantic contract: a breaking change or security fix in the upstream library will not be flagged by
go get -utooling. The library should be tagged before this PR merges, or the go.mod should document why a tag is not yet available.
harness-engineering:harness-enforcement-reviewer (3 findings)
internal/cmd/config/delete_token.go:52
runDeleteToken calls keychain.Open() which runs the one-time legacy migration. If a migration conflict exists (ErrMigrationConflict), delete-token is blocked — leaving no escape hatch short of manually editing the keyring. The clear.go command explicitly uses OpenNoMigrate() for this reason. delete-token has the same remediation role and should use OpenNoMigrate() as well.
💡 Suggestion - internal/cmd/config/show.go:35
runShow calls keychain.Open() which triggers the one-time legacy migration. If a migration conflict exists, the diagnostic command 'config show' fails with ErrMigrationConflict rather than displaying current credential state — giving users no diagnostic path to understand which keys are in which location before remediating. Consider using OpenNoMigrate() in runShow.
💡 Suggestion - internal/cmd/setcred/setcred.go:77
readValue() with --from-env silently returns an empty string when the named env var is unset, then falls through to the generic 'empty secret value rejected' error. Unlike init.go's resolveBot() which reports '--bot-token-from-env %s is empty or unset' (naming the var), the user has no way to know which env var was the problem. Add a targeted error naming the variable before the generic empty guard.
harness-engineering:harness-knowledge-reviewer (3 findings)
internal/cmd/setcred/setcred.go:71
OpenRef(opts.ref) is called with runMigration=true. When a non-default --ref is supplied, the one-time legacy migration runs against the custom ref's service/profile rather than the canonical default ref, potentially writing migrated credentials into the wrong service/profile — the opposite of what a user specifying --ref intends. OpenRef should pass runMigration=false, or the --ref flag should be explicitly documented as 'skips migration'.
internal/cmd/initcmd/init_test.go:1
No test exercises the --overwrite flag path (OpenForMigrationOverwrite). This is the only resolution path for an ErrMigrationConflict that forces a legacy credential over an existing keyring entry. A regression in that code path could cause silent credential corruption with no test catching it.
💡 Suggestion - internal/cmd/initcmd/init.go:100
In non-interactive mode (--bot-token-from-env, --user-token-from-env, --bot-token-stdin), the overwrite-existing-credentials prompt is skipped and tokens are written with WithOverwrite() without any feedback. A CI pipeline that accidentally re-runs slck init will silently replace existing credentials. Consider emitting an 'Overwriting existing credential at ' notice for non-interactive overwrites.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - internal/keychain/keychain.go:185
del() uses an Exists pre-check to make Delete idempotent, but this is a TOCTOU pattern: a concurrent writer can add the key after Exists returns false, and a concurrent deleter can remove it after Exists returns true. The Delete call already handles ErrNotFound, so the Exists check is redundant. The idempotency contract is already satisfied by the errors.Is(err, ErrNotFound) guard on Delete alone. Remove the Exists pre-check.
4 info-level observations excluded. Run with --verbose to include.
7 PR discussion threads considered.
Completed in 4m 21s | $1.79 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 4m 21s wall · 13m 27s compute (Reviewers: 3m 06s · Synthesis: 1m 12s) |
| Cost | $1.79 |
| Tokens | 298.5k in / 52.4k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 33.6k | 3.4k | 0 | 33.6k (1h) | $0.19 |
| documentation:docs-reviewer | haiku | 10.9k | 10.7k | 0 | 10.9k (1h) | $0.07 |
| harness-engineering:harness-architecture-reviewer | sonnet | 52.3k | 6.2k | 2.1k | 50.1k (1h) | $0.33 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 52.3k | 9.9k | 2.1k | 50.1k (1h) | $0.38 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 52.3k | 7.8k | 2.1k | 50.1k (1h) | $0.35 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 48.5k | 8.4k | 2.1k | 46.3k (1h) | $0.34 |
| security:security-code-auditor | haiku | 48.7k | 6.0k | 0 | 48.7k (1h) | $0.13 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
Note: Posted 11 inline comments. 1 finding remained in the summary because GitHub could not attach them to the current diff.
… hatches, ingress) - delete-token / config show now use OpenNoMigrate so they remain usable during an unresolved §1.8 conflict (same remediation role as config clear); config show is the §1.11 item 3 diagnostic and must not be blocked - OpenRef (set-credential --ref) no longer runs migration: the one-time §1.8 migration only ever targets the canonical configured ref, never an arbitrary --ref (which would discover the default ref's legacy data) - set-credential --from-env now names the empty/unset variable (parity with init resolveBot/resolveUser); never echoes a value (§1.12) - init: non-interactive ingress emits a stderr notice before overwriting an existing credential so an accidental CI re-run is visible - document why workspace stores info.TeamID (stable id) not info.Team - add end-to-end init --overwrite tests: migration conflict fails loud without leaking, and --overwrite forces the legacy value [INT-437]
Codex consolidated final review (post-daemon-fix batch)Findings: none. The daemon-fix batch preserves the intended architecture and §1.x boundaries. The declined daemon items are sound: per-call Verification run: A broader initcmd subset hit the known sandbox localhost-bind restriction from |
…nt [INT-446] (#164) ## Problem Released `slck` macOS binaries fail closed on every credential op. `.goreleaser.yaml` set `CGO_ENABLED=0` and the release job ran on `ubuntu-latest`; `99designs/keyring`'s Keychain backend is `//go:build darwin && cgo`, so with cgo off it was never compiled in. On macOS `credstore` auto-selects the Keychain backend then `keyring.Open` returns `ErrNoAvailImpl` → fail-closed. Every Mac user who upgraded since the credstore migration (#158) is broken. Verified **macOS-only** (Windows wincred and Linux godbus secret-service are pure Go, fine under `CGO_ENABLED=0`). ## Fix (pipeline-only — no Go source change) - **`.goreleaser.yaml`**: split builds — `slck-darwin` (`CGO_ENABLED=1`, fully-keyed per-arch `CC=xcrun clang -arch …` overrides for `darwin_amd64_v1` / `darwin_arm64_v8.0`) + `slck-unix-win` (`CGO_ENABLED=0` static). `nfpms.ids: [slck-unix-win]` (not the deprecated `builds`). **Both macOS arches (Intel + Apple Silicon) still produced** — the split is by GOOS, not arch. - **`release.yml`**: goreleaser job → pinned `macos-15` (cgo+darwin can't cross-compile from Linux). Restructured to **verify before publish**: `goreleaser check` → `release --snapshot` (no publish) → pre-publish gate → real `release --clean --release-notes`. The gate parses `dist/artifacts.json` and asserts: arm64 binary functionally reports `backend=keychain backend_source=auto credential_ref=slack-chat-api/default` (isolated HOME/XDG, `-u SLACK_CHAT_API_KEYRING_BACKEND`); amd64 binary links `Security.framework` (necessary cgo signal for the slice the runner can't execute); both darwin archives present exactly once. A backend-less darwin binary can no longer be published silently. - Hardened the `update-homebrew` checksum step (`set -euo pipefail` + non-empty SHA assertions) so a renamed/missing archive can't publish a broken cask. ## Non-goals - No cgo on linux/windows (glibc portability regression, zero benefit — pure-Go backends). - No runtime env credential fallback (would unwind the §1.11 keyring-only invariant). - The pre-existing `workflow_dispatch` `TAG: github.ref_name || inputs.tag` bug is out of scope (only affects the dispatch path; this fix uses the tag-push path) — separate ticket. ## Validation `goreleaser check` passes; both cgo cross-builds succeed locally (arm64 native + amd64 cross, correct Mach-O archs, amd64 links Security.framework); the gate's functional assertion reproduced on real hardware (`backend=keychain/auto`). ## Release mechanics `auto-release.yml` Gate-1 is a path filter (`**.go|go.mod|go.sum`); a pipeline-only diff matches none, so this is a `ci:` commit that does not auto-release. After merge the corrected release is cut by a deliberate annotated tag push (`release.yml` `on: push: tags: v*`). Pilot for the cross-CLI CGO regression (Jira INT-446, child of INT-310); verified on a non-dev Mac before any fan-out to nrq/gro/atlassian. Closes #163
2 participants
What
slck adopts the shared
cli-common/credstorelibrary (Phase A), replacing the hand-rolledinternal/keychain(macOSsecurityshell-out + Linux/Windows plaintext file). Authoritative scope: Secret-Handling Standard §2.4 + cross-cutting §1.3/§1.4/§1.7/§1.8/§1.11/§1.12.slck is the first real credstore consumer — this PR's structure is the reusable per-CLI template B2 (gro) / B3 (atlassian) follow. Plan:
~/.claude/plans/INT-437.md(Codex-architect converged 0/0/0/0 after 1B/3M→0B/1M/1m→clean).Highlights
internal/keychain→ pure credstore adapter (OS keyring only; no shell-out, no plaintext). Ref authoritative:ParseRef(config credential_ref)→Open(parsed-service)against parsed-profile; backend/passphrase env names derived from the parsed service. Nothing hard-coded.internal/config—config.yml(credential_ref,workspace,keyring.backend).slckandslack-chat-apiKeychain services + the legacy plaintext file → new bundle, with theapi_token→bot_tokenrename. All conflicts detected before any write/delete; conflicts fail loud naming every location and never print a value (masked or not). Idempotent;--overwriteresolution path._migrationJSON (object-merge / non-object-wrap, run-scoped consume-once) + one-line stderr notice.slck set-credential(stdin /--from-envonly, §1.5.2).TestRunInit_WrongTokenTypenon-hermetic flake.SLACK_API_TOKEN/SLACK_USER_TOKENno longer read at runtime (§1.11) — only as setup ingress viainit --bot-token-from-env.config set-token <token>hard-deprecated (exits nonzero → points toset-credential);init --bot-token <value>value-flags removed.Verification
go build/go vet/gofmt -lclean ·go test -race ./...all green · pinned golangci-lint v2.0.2 = 0 issues. Docs (README/CLAUDE.md) updated to the new ingress model; CLAUDE.md dep-drift (false slack-go/zalando) corrected.Closes #157
[INT-437]