fix(init): harden inline reviewer credentials

[rianjs]

Summary

• collect PAT and GitHub App reviewer secrets inline in the reviewer entity editor
• derive new reviewer secret refs from typed labels while preserving existing refs and manual overrides
• harden reviewer credential writes so missing/unavailable refs do not bypass no-overwrite preflight and same-ref auth-mode switches purge stale keys
• fix linear editor description wrapping so explicit status newlines render as separate rows

Verification

• go test -count=1 ./internal/cmd/credentialcmd
• go test -count=1 ./...
• tmux 120x40: label-derived GitHub App ref shows intact status rows, stages masked secrets inline, and preseeded destination fails with credential already exists instead of overwriting
• tmux 120x40: PAT reviewer switched to GitHub App on the same ref commits successfully, with no stale reviewer git_token written
• tmux 120x40: manually typed reviewer ref with unavailable status stages secrets but commit hits no-overwrite conflict when the store item already exists

Follow-up to #347, #348, and #349.

[rianjs]

Findings

Blocker: Profile setup still defers reviewer secrets instead of collecting them inline

internal/cmd/credentialcmd/init_profile_v2.go:353 lets the profile editor choose a new PAT/GitHub App reviewer, and internal/cmd/credentialcmd/init_profile_v2.go:702 applies that selection without any inline secret fields. The flow then falls through to collectInteractiveInitSessionWorkspaceSecrets(..., "reviewer_credentials", ...) at internal/cmd/credentialcmd/credentialcmd.go:1383, where the generic collector still accepts defer at internal/cmd/credentialcmd/credentialcmd.go:5838. That leaves a primary interactive path where required reviewer keys are not captured inline and can be deferred, contrary to the product intent and updated UX contract.

Blocker: Same-ref auth-mode switches do not purge existing stale keys

mergeReviewerCredentialDraftWrites only filters staged in-memory writes for the active auth mode at internal/cmd/credentialcmd/credentialcmd.go:1547; commit then only preflights and writes the provided keys at internal/cmd/credentialcmd/credentialcmd.go:7007 and internal/cmd/credentialcmd/credentialcmd.go:7029. There is no delete/purge of keys already present in the store. So switching codereview/work-reviewer from PAT to GitHub App can leave the old git_token in the same ref after commit. The new test at internal/cmd/credentialcmd/credentialcmd_test.go:15320 only proves stale keys are absent from session.writes, not from the credential store.

Major: Re-entering a staged overwrite can clear the overwrite flag while keeping the staged value

At internal/cmd/credentialcmd/credentialcmd.go:1547, an existing staged write for the same ref is preserved if it is valid for the current reviewer mode. But if the user re-enters the reviewer editor and stages a label-only/no-secret change, draft.ReviewerCredentialOverwrite is false and mergeReviewerCredentialDraftWrites deletes session.overwriteRefs[ref] at internal/cmd/credentialcmd/credentialcmd.go:1558. The staged replacement value remains, so commit later hits the no-overwrite preflight instead of applying the already-staged overwrite. Missing coverage: existing reviewer key replacement, re-enter editor, stage no new secret, then commit.

Minor: The linear editor viewport rewrite is a drive-by behavior change

internal/cmd/credentialcmd/init_linear_editor.go:196 manually handles only page up/down, and internal/cmd/credentialcmd/init_linear_editor.go:218 stops delegating other messages to viewport.Update. That is broader than the explicit-newline wrapping fix and changes untested scrolling/input behavior such as mouse wheel or other viewport-supported keys.

[rianjs]

Blocker
None.

Major

• init_reviewer_entity_editor.go: status unavailable is accepted for any existing reviewer edit because it only checks state.preserveCurrentLocation. If an existing reviewer’s credential ref is changed while backend status is unavailable, staging can succeed with no required secrets; credentialcmd.go then marks the ref keep-existing via satisfiedRefs. Require inline keys unless the ref is actually unchanged.
• credentialcmd.go: write groups keep only one plan entry per ref. If two touched profiles use the same reviewer ref with different auth modes, stale cleanup uses whichever entry wins and can delete keys still required by the other mode, including keys just written. Carry all entries for the ref and skip/error cleanup on mixed active specs.

Minor
None.

Nit
None.

[rianjs]

Blocker
None.

Major

• credentialcmd.go: stale reviewer-key cleanup only runs when plan.writes is non-empty. A same-ref auth-mode switch can be satisfied with existing target keys and no inline writes; satisfiedRefs then turns the overwrite-ref entry into keep-existing, so writeBundles never runs and the old mode key remains. Add a no-write cleanup plan for same-ref mode changes and cover PAT -> GitHub App with existing app keys plus stale git_token.

Minor
None.

Nit
None.

[rianjs]

Findings

Blocker: cleanup can delete keys still required by untouched profiles

buildInteractiveInitSessionPlan only builds credential entries for sortedTouchedProfileNames(session) (internal/cmd/credentialcmd/credentialcmd.go:4370). The stale-key cleanup then decides what is still active from only plan.credentialPlan (internal/cmd/credentialcmd/credentialcmd.go:5757) and deletes any reviewer key not represented there (internal/cmd/credentialcmd/credentialcmd.go:7261).

That breaks shared-ref cases. Example: profile A remains a PAT reviewer on codereview/shared-reviewer, profile B is the only touched profile and switches that same ref to GitHub App. The cleanup group only sees B’s GitHub App entry, treats git_token as stale, and deletes it, leaving profile A configured but unusable. The new lower-level test manually passes both entries, but the integrated session planner won’t include the untouched active profile.

Fix by computing cleanup safety from all active refs in the final config for the same resolved store/ref, or by ensuring every affected shared-ref profile is included in the session credential plan.

Major: inline staged writes are not bound to the destination shown to the user

Reviewer inline secrets are stored by ref string only (session.writes[ref]) and overwrite state is also ref-only (internal/cmd/credentialcmd/credentialcmd.go:1535). Later, secrets-management/profile edits can change the resolved destination without clearing those staged values (internal/cmd/credentialcmd/credentialcmd.go:1658, internal/cmd/credentialcmd/credentialcmd.go:5254). Final commit then groups the same staged values under the current entry.SecretsProfile (internal/cmd/credentialcmd/credentialcmd.go:5728).

So a user can enter reviewer secrets after seeing destination A, change the profile/default secrets store, and commit those same secret values to destination B. Worse, if overwriteRefs[ref] was set from status in A, no-overwrite preflight is skipped for B too (internal/cmd/credentialcmd/credentialcmd.go:5741). The staged write identity should include the resolved store, or store-affecting edits should invalidate/reconfirm staged reviewer secrets.

Minor: linear editor scrolling rewrite is a drive-by UI behavior change

init_linear_editor.go no longer delegates unhandled key messages to viewport.Update; it manually handles a small key set and returns nil for everything else (internal/cmd/credentialcmd/init_linear_editor.go:196). That may be fine, but it changes more than description newline wrapping and drops any viewport behavior not covered by those keys. Please either split it out or add focused coverage for expected scroll inputs so the TUI verification is repeatable.

[rianjs]

Findings

Blocker: writeBundles still performs stale reviewer-key cleanup with only the narrow write-group entries, so the prior shared-ref deletion bug can still happen when there are staged writes. groupInitWritesByStore only copies entries from the current plan into group.Entries (credentialcmd.go), and interactive plans are built from touched profiles only (credentialcmd.go). Then writeBundles deletes stale keys using those narrow entries (credentialcmd.go). A touched profile switching shared ref PAT -> GitHub App with inline writes can still delete git_token required by an untouched PAT profile sharing that ref. The new whole-config cleanup pass runs later, but cannot restore a key already deleted. The added writeBundles test passes both active entries manually, so it misses the real grouping/apply shape.

Blocker: stale-key deletion now happens before config save, which can destroy the still-current credential if saving config fails. Both apply paths call deleteStaleReviewerCredentialKeysForRefs before deps.saveConfig (credentialcmd.go, credentialcmd.go, credentialcmd.go, credentialcmd.go), and writeBundles also deletes immediately after SetBundle but before config save (credentialcmd.go). If an existing PAT reviewer switches to GitHub App on the same ref and config save fails after git_token is deleted, the on-disk config still requires PAT but the token is gone. Stale-key cleanup should be after a successful config write, or otherwise be rollback-safe.

Major: the new resolved-store binding does not flow through the inventory/detail reviewer path. When inventoryRunner is set, EditReviewerEntity takes editReviewerEntityInventory (credentialcmd.go), then detail editing calls editReviewerEntityFieldsLinear without the prompt context (credentialcmd.go, init_reviewer_entity_editor.go). That path builds the draft with initPromptContext{} (init_reviewer_entity_editor.go), so synthesizeReviewerCredentialStatus cannot resolve SecretsProfile (init_reviewer_credential_status.go), and the later store-change filter explicitly ignores the empty binding (credentialcmd.go). Inline writes staged through that path can still be retargeted after a secrets-store change. Pass the prompt context through the detail editor or resolve the write store from the session before merging.

The previous no-write same-ref cleanup concern looks fixed for the expanded cleanup pass, but the write-time cleanup path needs the same whole-final-config protection.

[rianjs]

Findings

Major: The inventory/detail reviewer editor now passes ctx into draft creation, but the visible credential-status text still ignores it. In init_reviewer_entity_editor.go, the initial status is synthesized with initPromptContext{}, and field-change refreshes do the same at init_reviewer_entity_editor.go. The draft path later uses ctx at init_reviewer_entity_editor.go, so the write can target the resolved store while the user was shown an empty/default-context destination/status. That undercuts the “destination shown to the user” guarantee for the inventory/detail path. Use ctx for initial status and refresh as well.

Minor: Post-save cleanup open failures do not use the partial-success/manual-cleanup error shape. applyStaleReviewerCredentialCleanups saves config before running, but if openInitStoreForEntry fails it returns a plain credential error at credentialcmd.go, unlike delete failures which say config was saved and list stale refs. This can make a partially successful init look like a total failure. The open-store error path should include the same “config saved; stale refs need cleanup” context.

Nit: writeBundles still takes entries, and initWriteGroup still carries Entries, but write-time stale cleanup was removed and the parameter is now unused (credentialcmd.go, credentialcmd.go). Removing those leftovers would make the new ownership boundary cleaner.

The prior blockers look addressed: writeBundles no longer deletes stale keys, cleanup runs after config save, and there is shared-ref/save-failure coverage.

[rianjs]

No findings.

The prior issues look resolved: writeBundles is write-only now, stale reviewer cleanup runs after config save using final active config state, cleanup failures report saved-config/manual-cleanup context, and reviewer detail status now carries prompt context through render/refresh/draft handling.

Verification run: go test -count=1 ./internal/cmd/credentialcmd (558 passed).

[rianjs]

The suite is much better than before, but it still does not fully prove the ticket’s highest-risk behavior.

• Major: The “inline secrets are bound to the store shown to the user” claim is still under-tested. The interactive tests stub openStore/openResolvedStore to return the same fake store regardless of the requested profile, and the label-derived conflict case seeds both the old and new refs, so ErrExists is not unique to the correct destination. A regression that writes to the wrong store or wrong ref could still pass. See credentialcmd_test.go:15606, credentialcmd_test.go:15779, and credentialcmd_test.go:7087.

• Major: There is no negative assertion that inline reviewer secrets are actually masked in the rendered TUI. The new tests check labels, status copy, and staged values, but they never verify that the raw PAT/private key strings are absent from model.View()/stderr. A leak in initLinearMaskedSecretValue or field rendering would still pass. See credentialcmd_test.go:6478, credentialcmd_test.go:6516, credentialcmd_test.go:15644, and credentialcmd_test.go:7038.

• Minor: The explicit-newline regression test only inspects layout.Content, not the rendered viewport path that actually changed. If viewport slicing or styling re-collapses those rows, this test can still pass. See credentialcmd_test.go:11704.

[monit-reviewer]

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.

[monit-reviewer]

Automated PR Review

Reviewed commit: a219f16

Manually approved via CLI.

[rianjs]

No findings.

I don’t see architectural drift from the post-daemon/TDD edits. The added coverage stays aligned with the reviewed design: inline reviewer writes still route through the resolved credential plan store, writeBundles remains write-only, stale cleanup stays post-save and final-config based, and the new assertions cover secret masking/leak prevention without changing the production shape.

Verification run: go test -count=1 ./internal/cmd/credentialcmd (559 passed).

[monit-reviewer]

Automated PR Review

Reviewed commit: 2c7488f

Manually approved via CLI.

[rianjs]

Minor

• init_reviewer_entity_editor.go: initReviewerEntityCredentialFieldKey now enumerates unrelated retention, LLM runtime, and secrets-management field IDs just to return "". That is daemon-driven scope creep: the helper should only know about reviewer credential input fields, and default already handles every non-credential ID. Keeping those unrelated constants here creates unnecessary compile-time coupling from the reviewer editor to other init editors without adding exhaustiveness or behavior. Trim the switch back to the four reviewer credential fields plus default.

No blockers or majors found. The credential write-store routing, post-save stale cleanup, and inline secret masking paths still match the reviewed architecture.

Verification run: go test -count=1 ./internal/cmd/credentialcmd (559 passed).

[monit-reviewer]

Automated PR Review

Reviewed commit: 09483f9

Manually approved via CLI.

[rianjs]

No findings.

The prior mapper drift is fixed: initReviewerEntityCredentialFieldKey is scoped to the four reviewer credential value fields, with default handling all non-credential editor IDs. I did not see remaining daemon-driven coupling, untraceable scope creep, or unverifiable behavior changes in the reviewer credential write/store routing, post-save stale cleanup, or linear editor status rendering paths.

Verification run: go test -count=1 ./internal/cmd/credentialcmd (559 passed).