feat(credstore): migration helpers (stderr line, _migration JSON, conflict error)#14
Conversation
…flict error)
Add the migration surface from the Secret-Handling Standard §2.1
(line 412) + §1.8 — the final shared-library unit before per-CLI
migration:
- EmitMigrationStderr(field, ref) — the one-time human signal
"migrated <field> to keyring at <ref>; this is a one-time
operation"; unexported formatMigrationLine seam
- MigrationJSONEntry / MigrationBlock / MigrationObject (+ New*) —
marshal byte-for-byte to the §1.8 shape
{"_migration":{"version":1,"changes":[...]}}, empty -> "changes":[]
not null; from/to are opaque locations, never the value
- MigrationConflictError + ErrMigrationConflict sentinel — §1.8
line 254 conflict: names both locations, states they differ,
offers the three remediation options; leak-proof by construction
(no value argument)
credstore owns formats + the conflict error only; migration
mechanics and conflict detection remain per-CLI. No new dependencies.
[INT-435]
Closes #13
Codex architect PR review (converged, first pass)No findings. Verified against the agreed architecture:
Residual risk is caller discipline: |
TDD / test-coverage assessment (independent)
What the tests cover well
Gaps and weaknessesHigh
Medium
Low
Summary table
TDD-ASSESSMENT: adequate-with-gaps |
Independent TDD review (adequate-with-gaps), all addressed in tests only (no production change): - conflict error: replace the no-op "BEGIN/xoxb-" sanity check and substring probes with a single exact golden-message assertion — subsumes both-locations + three-options and proves leak-proofness (message fully determined by the four non-value identifiers) - multi-change JSON: exact bytes instead of strings.Index ordering - direct NewMigrationBlock coverage (version + changes==nil guard + "changes":[]) and the documented embedded-field mode (MigrationBlock behind a caller's json:"_migration" field) - MigrationJSONEntry tested in isolation - JSON-escaping adversarial case (quote/backslash/unicode in field and to-location) with round-trip - formatMigrationLine fmt-verb-inertness (inputs are args, not the format string) Declined (with rationale): the os.Stderr pipe-swap is not goroutine-safe — acceptable, no test here uses t.Parallel(). [INT-435]
Response to TDD assessmentAll findings addressed in tests only (production code was already correct; gaps were assertion strength / missing cases):
187 tests pass with |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 73adb5c
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (1 findings)
credstore/migration_test.go:38
TestEmitMigrationStderr swaps os.Stderr and os.Stdout globally. The test passes with -race today (per PR discussion), but the pattern is fragile: any future t.Parallel() call or -parallel execution with another test that touches stderr creates a race window between the pipe swap and the deferred restore. The safer fix is to accept an io.Writer in EmitMigrationStderr (or an unexported testable wrapper) so tests can inject a buffer instead of mutating global file descriptors.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
credstore/migration_test.go:36
os.Pipe() errors are silently swallowed with
_. If pipe creation fails, the test continues with nil file handles and will panic or produce a misleading assertion failure instead of a clear test error. Both return values should be checked and the test should call t.Fatal on error.
💡 Suggestion - credstore/migration_test.go:44
io.ReadAll errors on the stderr pipe are discarded (
gotErr, _ := io.ReadAll(er)). A read failure silently produces an empty byte slice, causing the subsequent assertion to fail with a confusing mismatch rather than surfacing the underlying I/O error. Check the error and call t.Fatal.
4 info-level observations excluded. Run with --verbose to include.
3 PR discussion threads considered.
Completed in 1m 43s | $0.42 | sonnet | daemon 0.2.116 | 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 | 1m 43s wall · 2m 47s compute (Reviewers: 27s · Synthesis: 1m 13s) |
| Cost | $0.42 |
| Tokens | 97.8k in / 9.9k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 27.4k | 4.5k | 13.6k | 13.8k (1h) | $0.13 |
| harness-engineering:harness-architecture-reviewer | sonnet | 14.0k | 781 | 2.1k | 11.9k (1h) | $0.06 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 14.0k | 616 | 2.1k | 11.9k (1h) | $0.06 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 14.0k | 651 | 2.1k | 11.9k (1h) | $0.06 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 14.0k | 780 | 2.1k | 11.9k (1h) | $0.06 |
| security:security-code-auditor | haiku | 14.3k | 2.6k | 0 | 14.3k (1h) | $0.04 |
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.
| os.Stderr, os.Stdout = ew, ow | ||
| t.Cleanup(func() { os.Stderr, os.Stdout = origErr, origOut }) | ||
|
|
||
| EmitMigrationStderr("api_token", "atlassian-cli/default") |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-architecture-reviewer): TestEmitMigrationStderr swaps os.Stderr and os.Stdout globally. The test passes with -race today (per PR discussion), but the pattern is fragile: any future t.Parallel() call or -parallel execution with another test that touches stderr creates a race window between the pipe swap and the deferred restore. The safer fix is to accept an io.Writer in EmitMigrationStderr (or an unexported testable wrapper) so tests can inject a buffer instead of mutating global file descriptors.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed: replaced the global os.Stderr/os.Stdout swap with the package's pure/impure seam convention — added unexported emitMigration(w io.Writer, field, ref); EmitMigrationStderr(field, ref) keeps its exact standard signature and is now a one-line os.Stderr delegate. The test drives emitMigration with a bytes.Buffer — no global fd mutation, parallelism-safe, no pipe. Commit pushed.
| er, ew, _ := os.Pipe() | ||
| or, ow, _ := os.Pipe() | ||
| os.Stderr, os.Stdout = ew, ow | ||
| t.Cleanup(func() { os.Stderr, os.Stdout = origErr, origOut }) |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-self-documenting-code-reviewer): os.Pipe() errors are silently swallowed with _. If pipe creation fails, the test continues with nil file handles and will panic or produce a misleading assertion failure instead of a clear test error. Both return values should be checked and the test should call t.Fatal on error.
Reply to this thread when addressed.
There was a problem hiding this comment.
Resolved structurally: the os.Pipe call is gone entirely (buffer-injection seam), so there are no longer any swallowed pipe-creation errors to check.
| _ = ow.Close() | ||
| gotErr, _ := io.ReadAll(er) | ||
| gotOut, _ := io.ReadAll(or) | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): io.ReadAll errors on the stderr pipe are discarded (gotErr, _ := io.ReadAll(er)). A read failure silently produces an empty byte slice, causing the subsequent assertion to fail with a confusing mismatch rather than surfacing the underlying I/O error. Check the error and call t.Fatal.
Reply to this thread when addressed.
There was a problem hiding this comment.
Resolved structurally: io.ReadAll is gone (the test now asserts directly on a bytes.Buffer), so there is no discarded read error.
Daemon REQUEST_CHANGES — TestEmitMigrationStderr swapped global os.Stderr/os.Stdout (fragile under future t.Parallel) and swallowed os.Pipe / io.ReadAll errors. Fix via the package's own pure/impure seam convention: add unexported emitMigration(w io.Writer, field, ref); EmitMigrationStderr keeps its exact standard signature and is now a trivial os.Stderr delegate. The test drives emitMigration with a bytes.Buffer — no global fd mutation, no pipe, no swallowed I/O errors, parallelism-safe. errcheck: the best-effort Fprintln return is explicitly discarded with rationale (stdlib fmt.Println family parity; no actionable recovery, signature fixed by the standard). [INT-435]
|
PR review failed and the daemon has stopped retrying for this commit. I am removing my pending review request to stop the automated review cycle. Re-request @monit-reviewer to retry, or push a new commit and re-request. |
2 participants
Implements the package-level migration helpers from the Secret-Handling Standard §2.1 (line 412) + §1.8. PR7 — the final shared-library unit; Phase B (per-CLI migration) begins after this.
Surface
EmitMigrationStderr(field, ref string)migrated <field> to keyring at <ref>; this is a one-time operation(exact §1.8 signature; unexportedformatMigrationLineis the tested seam)MigrationJSONEntry(field, from, to string) MigrationChangeMigrationBlock/NewMigrationBlock(...)_migrationvalue (for CLIs embedding it behind their ownjson:"_migration"field);Version=1, non-nilChangesMigrationObject/NewMigrationObject(...){"_migration":{"version":1,"changes":[...]}}MigrationConflictError(cli, field, legacyLocation, ref string) error+ErrMigrationConflictMigrationSchemaVersion/MigrationFieldNameDesign notes
MigrationConflictErrortakes no value argument, so it structurally cannot print either value, masked or unmasked (§1.8 line 254 / §1.12) — which is why, unlike PR6's redaction path, nosafeErrorText-style scrubber is needed here.ErrMigrationConflictreuses the PR6ErrSecretLeaked/leakErrorsentinel pattern (Is(target) bool) so callers get a stableerrors.Issignal independent of message text.NewMigrationObject()with no changes marshals"changes":[](nevernull). Tested byte-for-byte against the §1.8 example.credstore/migration.go(+ test). No changes to existing files, no new deps (all stdlib).Tests
migration_test.go: exact stderr line +os.Pipecapture proving stderr-not-stdout, byte-for-byte §1.8 JSON (populated / empty-[]/ multi-change / version pin), unmarshal round-trip,MigrationConflictError(errors.Issentinel, both locations, three options, structural leak-proofness).Architect-reviewed: converged 0/0/0/0 (Major — missing top-level
_migrationwrapper — and Minor — unexported the formatter — resolved in iteration 2).[INT-435]
Closes #13