feat(state): port slck to cli-common state components [MON-5372]#165
Conversation
Adopt cli-common/statedir for the config-dir resolver (native per OS: $XDG_CONFIG_HOME→Linux, ~/Library/Application Support→macOS, %APPDATA%→Windows; relative $XDG_CONFIG_HOME now errors per the §1.1 intentional tightening). Make Save atomic via os.CreateTemp + chmod + rename (perms 0700/0600 already correct). Bump cli-common pin b753d5c → e67b2fc (HEAD; statedir/statedirtest available). Add a macOS/Windows config-relocation gate (copy-leave-old; fail loud on divergence; mutation-free runtime read-fallback via the new LoadForRuntime wrapper used by non-init callers). The gate runs ahead of EnsureMigrated in `slck init` so divergent old/new aborts before any token migration / config write papers over the conflict. Old-only malformed YAML fails loud BEFORE CopyNeeded so a corrupt legacy file never propagates into the new dir. LoadForRuntime soft-degrade contract (MON-5371 lesson): only suppress ErrRelocationConflict when a canonical config was actually read. A malformed canonical under conflict returns (nil, ErrRelocationConflict) so non-init commands hard-fail instead of silently swapping CredentialRef back to DefaultCredentialRef. `slck config clear --all` now removes BOTH the new canonical config.yml AND the pre-MON-5372 hand-rolled path. Runtime old-only fallback would otherwise let a stale old config silently resurrect post-clear. Path-identity dedup (Linux: old == new). Switch internal/testutil to delegate state-dir isolation to cli-common/statedirtest (full 7-var set) — closes a Windows real-dir leak (AppData/USERPROFILE/LocalAppData/XDG_CACHE_HOME/XDG_DATA_HOME were missing). Add testutil.ConfigDir(t) for the new canonical surface and testutil.LegacyCredentialsPath(t) for the keychain migrator's hand-rolled probe path (the two diverge on macOS/Windows; test helpers must not conflate them). §3.2 acceptance matrix coverage: 8 cases against the config surface (old-only / new-only / equal-both / divergent-both / malformed-old / malformed-new / neither / no-real-dir-writes) plus runtime soft-conflict canonical-returned and malformed-canonical-hard-fail. Closes #162 [MON-5372]
|
Findings Major: Runtime §3.2 behavior is tested through a copied test helper, not the production runtime seam. relocate_test.go reimplements Major: The init relocation gate is not covered through the Focused verification: |
Codex r1 caught two majors: - Test helper loadForRuntimeAt was a parallel implementation of Load + LoadForRuntime — production could regress and tests would pass. Extract unexported loadFromNewDir / loadForRuntimeFromNewDir as the testable seams; production Load / LoadForRuntime now route through the same code paths the tests exercise. - Init relocation gate was only unit-tested (Detect + Apply directly), not through runInit. A regression that moved the gate after EnsureMigrated would still pass. Add init-level tests: TestRunInit_RelocationGate_OldOnlyCopied (asserts old→new copy through runInit) and TestRunInit_RelocationGate_DivergentAborts BeforeMutation (skip on Linux where old == new; asserts abort-before-mutation on macOS/Windows). [MON-5372]
Codex r2 catch: the divergent-abort test snapshot-asserted only config.yml bytes — if the gate were moved after keychain.Open, the test would still pass unless a legacy source existed for the migrator to mutate. Seed writeLegacyCreds before runInit and assert the legacy file still exists post-abort: that proves the gate ran BEFORE keychain.Open / migration, not just before SaveConfig. [MON-5372]
|
No findings. The remaining test gap is closed: the divergent init test now seeds the legacy credentials file and proves the relocation gate aborts before Focused verification passes: |
TDD Coverage Assessment: AdequateVerdict: Adequate — every load-bearing behavior introduced by this PR has a test exercising the production seam. What's covered well
Minor nits (not blockers)
Neither rises to "regression could ship green." Ship it. |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a31675a
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 3 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-architecture-reviewer (3 findings)
internal/config/config.go:76
Load() is exported but can return (cfg != nil, ErrRelocationConflict) simultaneously, violating the standard Go error-handling idiom. Any future caller doing
if err != nil { return err }will silently discard a fully-valid config during a relocation conflict. Since all runtime callers have been migrated to LoadForRuntime(), Load() is now an internal stepping stone with an undocumented dual-return contract. Either unexport it or add a prominent doc comment warning callers they must use errors.Is rather than a nil check.
💡 Suggestion - internal/config/relocate.go:185
configsMaterialEqual explicitly enumerates only CredentialRef, Workspace, and Keyring. Any new top-level Config field added in the future will be silently excluded from divergence detection, causing configs that differ only in the new field to be misclassified as equal and relocation to proceed without conflict. Consider using reflect.DeepEqual on the full default-applied Config struct, or adding a compile-time exhaustiveness check, so the function self-heals as Config grows.
💡 Suggestion - internal/config/relocate.go:255
warnReloConflictOnce writes directly to os.Stderr via fmt.Fprintf rather than through the output package. This bypasses --no-color handling and is inconsistent with the rest of the CLI. The warning text also cannot be asserted from external test packages since only same-package tests can reset the sync.Once seam. Routing through output.Warningf (or equivalent) would restore consistency and testability.
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - internal/config/relocate.go:134
detectRelocation returns Kind=relocBothDivergent when old is present but malformed and new is absent — semantically wrong since only one file exists. loadFromNewDir gates its old-dir fallback on
reloc.Kind == relocOldOnly, so behavior is correct today, but a future caller switching on Kind for diagnostics would be misled. Introduce a dedicated relocOldOnlyMalformed kind (or return relocOldOnly with CopyNeeded=false and a separate malformed flag) to eliminate the misleading classification.
💡 Suggestion - internal/config/relocate_test.go:264
reloConflictOnce (a sync.Once) is reset via direct struct assignment
reloConflictOnce = sync.Once{}in the loadForRuntimeAt helper. This is a data race under the Go memory model if any goroutine is concurrently inside warnReloConflictOnce.Do. Tests are currently sequential so there is no observable race, but adding t.Parallel() to any LoadForRuntime test would immediately trigger -race failures. Safer alternatives: inject the warn function via a package-level var, or use sync/atomic to reset the done flag.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - internal/config/relocate.go:220
fileExists treats any non-nil error from os.Stat as file-absent. A permission-denied error on the old config directory would silently make oldPresent=false, collapsing an old-only or both-present case into neither-present and skipping the relocation gate entirely. While unlikely for a user-owned file, a directory with unexpected permissions (e.g. mode 0000) would produce a silent no-op instead of a loud error. Distinguish os.IsNotExist from other errors and propagate non-not-exist errors to the caller.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - internal/config/config.go:104
When loadFromNewDir encounters both a relocation conflict (relErr != nil) and a YAML parse error in the canonical new-dir config, the parse error is silently swallowed and only ErrRelocationConflict is surfaced. A user with both a conflict and a malformed new config.yml receives no indication of the YAML failure. Consider wrapping both errors (e.g.
fmt.Errorf("%w; canonical also malformed: %v", relErr, uerr)) so the parse diagnostic is preserved.
4 info-level observations excluded. Run with --verbose to include.
3 PR discussion threads considered.
Completed in 4m 27s | $1.32 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, 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 27s wall · 12m 04s compute (Reviewers: 3m 02s · Synthesis: 1m 22s) |
| Cost | $1.32 |
| Tokens | 186.2k in / 44.0k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 31.6k | 5.1k | 0 | 31.6k (1h) | $0.20 |
| harness-engineering:harness-architecture-reviewer | sonnet | 31.2k | 9.0k | 2.1k | 29.1k (1h) | $0.27 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 31.2k | 7.6k | 2.1k | 29.1k (1h) | $0.25 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 31.2k | 9.6k | 2.1k | 29.1k (1h) | $0.28 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 30.3k | 7.2k | 2.1k | 28.2k (1h) | $0.24 |
| security:security-code-auditor | haiku | 30.6k | 5.4k | 0 | 30.6k (1h) | $0.09 |
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.
Address 4 of 7 pr-review-daemon findings (3 buggered off in-thread): - Load() now carries a prominent doc warning of the dual-return contract (non-nil cfg + non-nil ErrRelocationConflict simultaneously). Route init's only remaining direct Load() callsite through LoadForRuntime too — post-gate behavior is identical but keeps the read pattern uniform and harder to misuse. - configsMaterialEqual now uses reflect.DeepEqual on the whole default-applied Config struct, so any future top-level Config field is auto-covered as a divergence-trigger. - fileExists distinguishes os.IsNotExist from other errors; an oddly- permissioned dir no longer silently collapses the gate to a no-op. - Load wraps parse-error diagnostics into relErr under conflict (`%w; canonical also malformed: %v`) instead of swallowing them. [MON-5372]
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 424dfb3 | Previous: a31675a (incremental)
Approved with 4 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 2 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (2 findings)
💡 Suggestion - internal/cmd/config/clear.go:66
OldConfigPath() errors are silently swallowed: if os.UserHomeDir() fails, the old config.yml is quietly skipped. After --all, the runtime old-only fallback in Load would then resurrect the stale config — exactly the scenario this code exists to prevent. At minimum surface a warning so the user knows --all was incomplete; ideally propagate the error.
💡 Suggestion - internal/config/relocate.go:213
ApplyConfigRelocation uses fileExists(dst) to gate the idempotency skip, and fileExists now treats non-IsNotExist stat errors as 'file present' (returning nil). If the canonical config dir is somehow unreadable after MkdirAll succeeds, the copy is silently skipped and init returns success with no config written to the new location. The error should be surfaced explicitly rather than collapsed into the 'already present' path.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
💡 Suggestion - internal/config/relocate_test.go:161
In TestRelocate_Divergent_FailLoudNamesBothPaths_MutatesNothing, pre-detect snapshots are taken with
oldBefore, _ := os.ReadFile(...)— errors are silently ignored. If either read fails (e.g. file not created as expected in setup), oldBefore/newBefore will be nil and the subsequent comparison will pass trivially, making the 'mutates nothing' assertion vacuous. Use require.NoError on these reads.
💡 Suggestion - internal/cmd/initcmd/init_test.go:214
TestRunInit_RelocationGate_OldOnlyCopied asserts the new canonical config.yml contains
slack-chat-api/from-old, but this relies on runInit exiting early before cfg.Save() because no tokens are supplied. If the wizard's no-input flow changes (e.g. a new prompt is added or Save is called earlier), the assertion will fail for an unrelated reason or silently pass with overwritten content. The assumed control-flow path should be documented or tested more directly.
4 info-level observations excluded. Run with --verbose to include.
10 PR discussion threads considered.
Completed in 4m 07s | $1.21 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, 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 07s wall · 9m 49s compute (Reviewers: 2m 08s · Synthesis: 1m 33s) |
| Cost | $1.21 |
| Tokens | 223.2k in / 34.0k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 42.6k | 5.1k | 13.6k | 29.0k (1h) | $0.19 |
| harness-engineering:harness-architecture-reviewer | sonnet | 31.9k | 6.7k | 2.1k | 29.7k (1h) | $0.24 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 31.9k | 6.4k | 2.1k | 29.7k (1h) | $0.23 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 31.9k | 4.7k | 2.1k | 29.7k (1h) | $0.20 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 30.9k | 4.9k | 2.1k | 28.8k (1h) | $0.20 |
| security:security-code-auditor | haiku | 31.2k | 5.5k | 0 | 31.2k (1h) | $0.09 |
| discussion-summarizer | — | 22.8k | 709 | 13.6k | 9.1k (1h) | $0.05 |
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.
| newPath, perr := appconfig.Path() | ||
| if perr != nil { | ||
| return perr | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): OldConfigPath() errors are silently swallowed: if os.UserHomeDir() fails, the old config.yml is quietly skipped. After --all, the runtime old-only fallback in Load would then resurrect the stale config — exactly the scenario this code exists to prevent. At minimum surface a warning so the user knows --all was incomplete; ideally propagate the error.
Reply to this thread when addressed.
| _ = os.Remove(tmpPath) | ||
| return err | ||
| } | ||
| if err := os.Rename(tmpPath, dst); err != nil { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): ApplyConfigRelocation uses fileExists(dst) to gate the idempotency skip, and fileExists now treats non-IsNotExist stat errors as 'file present' (returning nil). If the canonical config dir is somehow unreadable after MkdirAll succeeds, the copy is silently skipped and init returns success with no config written to the new location. The error should be surfaced explicitly rather than collapsed into the 'already present' path.
Reply to this thread when addressed.
| if !strings.Contains(err.Error(), oldDir) || !strings.Contains(err.Error(), newDir) { | ||
| t.Errorf("error must name both paths: %v", err) | ||
| } | ||
| oldAfter, _ := os.ReadFile(filepath.Join(oldDir, configFileName)) |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): In TestRelocate_Divergent_FailLoudNamesBothPaths_MutatesNothing, pre-detect snapshots are taken with oldBefore, _ := os.ReadFile(...) — errors are silently ignored. If either read fails (e.g. file not created as expected in setup), oldBefore/newBefore will be nil and the subsequent comparison will pass trivially, making the 'mutates nothing' assertion vacuous. Use require.NoError on these reads.
Reply to this thread when addressed.
| // must have copied old→new by then if they differ. | ||
| err := runInit(&initOptions{stdin: strings.NewReader("\nn\n"), noVerify: true}) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): TestRunInit_RelocationGate_OldOnlyCopied asserts the new canonical config.yml contains slack-chat-api/from-old, but this relies on runInit exiting early before cfg.Save() because no tokens are supplied. If the wizard's no-input flow changes (e.g. a new prompt is added or Save is called earlier), the assertion will fail for an unrelated reason or silently pass with overwritten content. The assumed control-flow path should be documented or tested more directly.
Reply to this thread when addressed.
|
No findings. The final pass lines up with the batch architecture and §3.2 matrix: init gate is first, runtime soft-degrade is guarded by readable canonical config, clear covers old+new paths, equality is full defaulted config, and legacy credentials remain under the per-CLI migrator. Focused verification passes: |
2 participants
Closes #162. Implements working-with-state.md §6.4 unit 4 — slck (single-binary, no cache): statedir resolver, atomic Save, copy-leave-old relocation gate with the MON-5371 hard-fail-on-malformed-canonical contract, statedirtest hermeticity.
§3.2 acceptance matrix (merge gate)
Config policy: copy-leave-old at `slck init`; runtime mutation-free with `LoadForRuntime` soft-degrade ONLY when canonical was readable (malformed canonical under conflict hard-fails — MON-5371 lesson encoded). Divergent old/new fails loud naming both paths. Linux: zero-byte op (old == new).
Single file — `config.yml` only. No companion deployment material, no legacy JSON. The legacy `credentials` k=v secret file is handled by the existing `keychain/migrate.go` `discover()` enumerator (per-CLI legacy probe, §5a-allowed).
Plus runtime: TestLoadForRuntime_SoftConflict_ReturnsCanonical and TestLoadForRuntime_MalformedCanonicalUnderConflict_HardFails.
Surfaces
Out of scope
Verification