fix: handle init credential backend switches#21
Conversation
rianjs
changed the title
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: e2445841f97c
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
go:implementation-tests (1 finding)
Major - internal/cmd/initcmd/init.go:131
The implicit overwrite is not limited to the interactive backend-selection flow. A scripted invocation such as
sptfy --backend keychain init --non-interactive ...with persisted config set tofilenow setsOverwritemerely because the backend differs, so an already-present credential in the destination can be replaced without--overwrite. This bypasses the command's destructive-operation guard outside the behavior described by the PR. Restrict the automatic overwrite to an interactive prompt-driven backend change, and add a non-interactive backend-switch regression test asserting thatcredstore.ErrExistsis still returned without--overwrite.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go | unavailable | Review limited to the assigned Go implementation and test files. go test ./internal/cmd/initcmd could not build in this environment because the compiler received a workspace path with spaces as separate arguments. |
| policies:conventions | complete_broad | internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go | unavailable | Focused on the assigned changed files and convention drift; the package test could not build in this environment because the tool wrapper passed a truncated path containing spaces to clang.; Shared cli-common and automation convenience copies were not present, so this review used only repo-local guidance. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 12s | unavailable | gpt-5.6-terra | cr 0.10.267
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 12s wall · 1m 44s compute |
| Cost | unavailable |
| Tokens | 256.7k in / 3.8k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.6-terra | 16.5k | 209 | 11.0k | unavailable | unavailable | 6s |
| go:implementation-tests | gpt-5.6-terra | 121.5k | 2.1k | 99.1k | unavailable | unavailable | 56s |
| policies:conventions | gpt-5.6-terra | 83.5k | 1.2k | 65.5k | unavailable | unavailable | 34s |
| orchestrator-rollup | gpt-5.6-terra | 35.2k | 300 | 22.0k | unavailable | unavailable | 6s |
e244584 to
f6aed67
Compare
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f6aed671ff63
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 1 |
go:implementation-tests (1 finding)
Minor - internal/cmd/initcmd/init_test.go:120
The new regression coverage does not prove the preserved interactive same-backend guard.
TestInitRefusesExistingCredentialBeforeAuthorizationexercisesInteractive: false, while the changed overwrite condition branches oninteractive; an accidental simplification to overwrite every interactive run would still pass both new tests. Add a case withInteractive: true, an existing credential, and a prompt that leaves the configured backend unchanged, assertingcredstore.ErrExists(and ideally that authorization is not called).
policies:conventions (1 finding)
Minor - internal/cmd/initcmd/init.go:132
This changes the documented overwrite contract: README currently directs users to use
sptfy init --overwriteto replace an older credential, but interactive backend switches now replace an existing destination credential implicitly. Update that user-facing guidance to distinguish ordinary replacement from an intentional interactive backend change, so the durable behavior is discoverable.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go | unavailable | Focused review limited to the two assigned changed files; package test execution was blocked by the sandbox toolchain path handling whitespace in the workspace path. |
| policies:conventions | complete_broad | internal/cmd/initcmd/init.go, internal/cmd/initcmd/init_test.go | unavailable | Shared cli-common and .github convention documents were not available locally; review used repo-local development guidance and README behavior documentation.; Targeted Go test could not build because the environment passed a space-containing path incorrectly to clang. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 18s | unavailable | gpt-5.6-terra | cr 0.10.267
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 18s wall · 1m 42s compute |
| Cost | unavailable |
| Tokens | 300.7k in / 4.4k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.6-terra | 56.9k | 441 | 40.2k | unavailable | unavailable | 6s |
| go:implementation-tests | gpt-5.6-terra | 97.9k | 2.1k | 65.5k | unavailable | unavailable | 53s |
| policies:conventions | gpt-5.6-terra | 65.6k | 1.2k | 37.1k | unavailable | unavailable | 35s |
| orchestrator-rollup | gpt-5.6-terra | 80.3k | 596 | 61.4k | unavailable | unavailable | 6s |
| @@ -120,6 +120,43 @@ func TestInteractiveInitUsesPromptedBackendForStoreAndConfig(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
File-level note: internal/cmd/initcmd/init_test.go
The new regression coverage does not prove the preserved interactive same-backend guard. TestInitRefusesExistingCredentialBeforeAuthorization exercises Interactive: false, while the changed overwrite condition branches on interactive; an accidental simplification to overwrite every interactive run would still pass both new tests. Add a case with Interactive: true, an existing credential, and a prompt that leaves the configured backend unchanged, asserting credstore.ErrExists (and ideally that authorization is not called).
Reply inline to this comment.
| result, err := dependencies.Initializer.Initialize(command.Context(), InitializeOptions{ | ||
| Config: configValue, Profile: profile, Backend: runtimeBackend, BackendSet: runtimeBackendSet, | ||
| Overwrite: overwrite, Verify: !noVerify, | ||
| Overwrite: overwrite || (interactive && strings.TrimSpace(setup.Backend) != configuredBackend), Verify: !noVerify, |
There was a problem hiding this comment.
This changes the documented overwrite contract: README currently directs users to use sptfy init --overwrite to replace an older credential, but interactive backend switches now replace an existing destination credential implicitly. Update that user-facing guidance to distinguish ordinary replacement from an intentional interactive backend change, so the durable behavior is discoverable.
Reply inline to this comment.
1 participant
Summary
--overwriteguard when the backend is unchangedValidation
go test ./internal/cmd/initcmdmake checkCloses #20