fix(init): collect reviewer secrets in reviewer flow by rianjs · Pull Request #350 · open-cli-collective/codereview-cli

rianjs · 2026-06-18T20:27:21Z

Summary

collect PAT/GitHub App reviewer secrets immediately after reviewer-entity staging while keeping writes draft-local until commit
record draft defer/optional-skip decisions so commit does not re-prompt handled reviewer refs and stale decisions are filtered when reviewer config changes
make reviewer credential prompts auth/key-specific and update init UX docs

Verification

go test ./internal/cmd/credentialcmd -run focused reviewer/secret tests
make test
make build
make lint
make tidy
tmux 120x40 checks:
- GitHub App reviewer set-now prompt appears immediately after staging reviewer settings and before commit
- multi-line PEM-shaped private key accepted via clipboard without pane/config leakage
- commit after set-now writes one credential ref and does not re-prompt reviewer credentials
- GitHub App defer prints required set-credential hints only
- PAT reviewer requests only git_token
- profile Git-account reviewer returns to main menu without reviewer credential prompt
- discard after entering sentinel secrets leaves config unchanged and pane/config free of sentinels

Closes #347

rianjs · 2026-06-18T20:31:44Z

Findings

Minor: initCredentialSecretBundleLabel / initCredentialScopedKeySummary changes prompt copy for non-reviewer credential flows too, including Git PAT and GitHub App Git credentials. That may be fine, but Move reviewer credential secret capture into reviewer-entity init flow #347 is scoped to reviewer credential capture and reviewer key-specific wording. Either constrain the new wording to reviewer_credentials or add a small test/PR note making the shared Git prompt change intentional.
internal/cmd/credentialcmd/credentialcmd.go:3324, internal/cmd/credentialcmd/credentialcmd.go:6468
Minor: The stale-decision regression covers clearing the reviewer back to profile Git identity, but not changing a deferred reviewer to a different separate reviewer ref/auth mode. The keying/filtering design should handle that, but this is the exact lifecycle edge the new decision map owns. Add one test for defer -> change reviewer ref or auth -> commit, verifying no stale suppression/hints for the old ref.
internal/cmd/credentialcmd/credentialcmd.go:6116

Overall, the PR matches the ticket’s product intent: reviewer secrets move into the reviewer-entity flow, writes stay draft-local until commit, defer/optional-skip decisions are modeled, and docs now describe the new contract.

Closes #347

rianjs · 2026-06-18T20:36:06Z

The new tests are useful, but they still leave a couple of important holes for this ticket.

Major: the added coverage is still entirely unit-level and fake-driven in internal/cmd/credentialcmd/credentialcmd_test.go. None of these cases drive the real PTY / huh flow that the ticket explicitly called for, so they can pass even if the actual interactive terminal ordering regresses. In particular, they do not prove the reviewer credential step is rendered correctly in a real session, only that the internal prompt dispatcher was invoked.
Minor: TestInitInteractiveMenuFocusedPATReviewerPromptsForGitTokenOnly is weaker than it looks. It asserts initCredentialRequiredKeys(prompt.Entry) on the intercepted prompt, which is derived from the same internal model as the implementation. That means it can still pass if the PAT prompt wiring is wrong but the helper returns the expected key list. It does not inspect the actual key prompts or prompt text.
Minor: discard coverage in TestInitInteractiveMenuReviewerSetNowDiscardDoesNotWriteConfigOrCredentials only exercises Discard staged changes and exit via initMenuActionExit. The acceptance criteria also call out Back without staging, and there is no direct test for backing out of the reviewer credential subflow without committing any of the staged secret values.

rianjs · 2026-06-18T20:39:07Z

Findings

The new reviewer-flow tests are still too synthetic to prove the actual UX change: TestInitInteractiveMenuFocusedGitHubAppReviewerDeferEmitsReadinessAndHints, ...SetNowPromptsBeforeCommitAndDoesNotRepeat, and ...PATReviewerPromptsForGitTokenOnly mostly assert internal helper state (actionPrompts, sourcePrompts, initCredentialRequiredKeys) rather than the rendered reviewer-specific prompt copy or the real PTY interaction. That means a regression that keeps the generic credential wording, or one that writes reviewer secrets on defer, could still pass these tests. credentialcmd_test.go:13985 credentialcmd_test.go:14093 credentialcmd_test.go:14202
TestInitInteractiveMenuReviewerSetNowDiscardDoesNotWriteConfigOrCredentials only covers Exit, not Back without staging, for the new reviewer-secret step. Since credential capture now happens before the user returns to the main menu, a back-navigation regression that leaks or persists secrets would not be caught by the added tests. credentialcmd_test.go:14661

Closes #347

monit-reviewer

Automated PR Review

Reviewed commit: f6efe1e

Approved with 3 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-knowledge-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	2

harness-engineering:harness-knowledge-reviewer (1 findings)

💡 Suggestion - docs/init-ux-contract.md:144

The updated contract introduces subflow credential collection but doesn't document the session-level invariant that defer/skip-optional decisions are memoized and filtered when reviewer config changes. A short note in the Draft-Local Reuse Rules section stating that interim credential decisions are session-scoped and discarded when their associated credential ref changes would make this invariant explicit for future contributors adding new subflow types.

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

💡 Suggestion - internal/cmd/credentialcmd/credentialcmd.go:5616

recordInitCredentialEntryDecision expresses its inclusion logic as two inverted guard conditions that require the reader to mentally negate both to understand the intent. A named predicate such as decisionAppliesToSpec(kind, spec) bool would state the inclusion intent directly and remove the double-negation overhead.

💡 Suggestion - internal/cmd/credentialcmd/credentialcmd_test.go:14213

actionMenuPromptCounts is a misleading name — the variable records the menu-prompt depth at the moment each credential-action prompt fires, not a count of action-menu prompts. A name like menuDepthAtCredentialAction or menuPromptCountsAtEachActionPrompt would make the assertion self-explanatory.

3 PR discussion threads considered.

Completed in 2m 48s | $2.46 | sonnet | daemon 0.2.132 | 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	2m 48s wall · 2m 44s compute (Reviewers: 2m 06s · Synthesis: 37s)
Cost	$2.46 (estimated)
Tokens	443.4k in / 23.3k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	37.9k	2.3k	13.8k	24.1k (1h)	$0.18
documentation:docs-reviewer	sonnet	35.0k	1.1k	13.8k	21.2k (1h)	$0.15
harness-engineering:harness-architecture-reviewer	sonnet	71.3k	809	13.8k	57.5k (1h)	$0.36
harness-engineering:harness-enforcement-reviewer	sonnet	77.0k	6.1k	13.8k	63.2k (1h)	$0.48
harness-engineering:harness-knowledge-reviewer	sonnet	73.1k	2.4k	13.8k	59.4k (1h)	$0.40
harness-engineering:harness-self-documenting-code-reviewer	sonnet	75.8k	6.0k	13.8k	62.0k (1h)	$0.47
security:security-code-auditor	sonnet	73.3k	4.5k	13.8k	59.5k (1h)	$0.43

_{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.}

monit-reviewer · 2026-06-18T20:50:08Z

@@ -144,19 +144,23 @@ Until the user chooses **Commit staged changes and exit**:



🔵 Low (harness-engineering:harness-knowledge-reviewer): The updated contract introduces subflow credential collection but doesn't document the session-level invariant that defer/skip-optional decisions are memoized and filtered when reviewer config changes. A short note in the Draft-Local Reuse Rules section stating that interim credential decisions are session-scoped and discarded when their associated credential ref changes would make this invariant explicit for future contributors adding new subflow types.

Reply to this thread when addressed.

monit-reviewer · 2026-06-18T20:50:08Z

+		if kind == initCredentialDecisionSkipOptional && spec.Required {
+			continue
+		}
+		if kind == initCredentialDecisionDefer && !spec.Required {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): recordInitCredentialEntryDecision expresses its inclusion logic as two inverted guard conditions that require the reader to mentally negate both to understand the intent. A named predicate such as decisionAppliesToSpec(kind, spec) bool would state the inclusion intent directly and remove the double-negation overhead.

Reply to this thread when addressed.

monit-reviewer · 2026-06-18T20:50:08Z

+		reviewerPrompter: initReviewerEntityPrompterFunc(func(prompt initReviewerEntityPrompt) (initDraft, error) {
+			reviewerPrompterCalls++
+			if reviewerPrompterCalls > 1 {
+				return initDraft{}, errInitNavigateBack


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): actionMenuPromptCounts is a misleading name — the variable records the menu-prompt depth at the moment each credential-action prompt fires, not a count of action-menu prompts. A name like menuDepthAtCredentialAction or menuPromptCountsAtEachActionPrompt would make the assertion self-explanatory.

Reply to this thread when addressed.

Closes #347

monit-reviewer

Automated PR Review

Reviewed commit: 3fbf859 | Previous: f6efe1e (incremental)

Approved with 2 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-self-documenting-code-reviewer	2

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

💡 Suggestion - internal/cmd/credentialcmd/credentialcmd.go:5600

recordInitCredentialEntryDecision still expresses its inclusion logic as two inverted guard conditions, requiring the reader to mentally negate both to understand that defer records only required keys and skip-optional records only optional keys. A named predicate such as decisionAppliesToSpec(kind, spec) bool would state the intent directly. (Unchanged from previous cycle; no author reply on thread.)

💡 Suggestion - internal/cmd/credentialcmd/credentialcmd_test.go:14213

actionMenuPromptCounts still records menu-prompt depth at the moment each credential-action prompt fires, not a count of action-menu prompts. A name like menuDepthAtCredentialAction would make the []int{1} assertion self-explanatory without tracing the onAction callback. (Unchanged from previous cycle; no author reply on thread.)

6 PR discussion threads considered.

Completed in 1m 37s | $6.31 | sonnet | daemon 0.2.132 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
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	1m 37s wall · 1m 34s compute (Reviewers: 47s · Synthesis: 47s)
Cost	$6.31 (estimated)
Tokens	1143.4k in / 31.0k out
Turns	28

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	87.8k	4.6k	28.2k	59.6k (1h)	$0.44
documentation:docs-reviewer	sonnet	78.0k	1.2k	25.0k	53.0k (1h)	$0.34
harness-engineering:harness-architecture-reviewer	sonnet	186.3k	987	25.0k	161.3k (1h)	$0.99
harness-engineering:harness-enforcement-reviewer	sonnet	204.3k	7.2k	25.0k	179.3k (1h)	$1.19
harness-engineering:harness-knowledge-reviewer	sonnet	193.4k	4.1k	25.0k	168.4k (1h)	$1.08
harness-engineering:harness-self-documenting-code-reviewer	sonnet	201.5k	8.2k	25.0k	176.6k (1h)	$1.19
security:security-code-auditor	sonnet	192.1k	4.7k	25.0k	167.1k (1h)	$1.08

_{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.}

monit-reviewer · 2026-06-18T20:56:20Z

+		if kind == initCredentialDecisionSkipOptional && spec.Required {
+			continue
+		}
+		if kind == initCredentialDecisionDefer && !spec.Required {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): recordInitCredentialEntryDecision still expresses its inclusion logic as two inverted guard conditions, requiring the reader to mentally negate both to understand that defer records only required keys and skip-optional records only optional keys. A named predicate such as decisionAppliesToSpec(kind, spec) bool would state the intent directly. (Unchanged from previous cycle; no author reply on thread.)

Reply to this thread when addressed.

monit-reviewer · 2026-06-18T20:56:20Z

+		reviewerPrompter: initReviewerEntityPrompterFunc(func(prompt initReviewerEntityPrompt) (initDraft, error) {
+			reviewerPrompterCalls++
+			if reviewerPrompterCalls > 1 {
+				return initDraft{}, errInitNavigateBack


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): actionMenuPromptCounts still records menu-prompt depth at the moment each credential-action prompt fires, not a count of action-menu prompts. A name like menuDepthAtCredentialAction would make the []int{1} assertion self-explanatory without tracing the onAction callback. (Unchanged from previous cycle; no author reply on thread.)

Reply to this thread when addressed.

fix(init): collect reviewer secrets in reviewer flow

0c21cf4

Closes #347

test(init): cover reviewer credential decision changes

7275b20

Closes #347

test(init): cover reviewer credential prompt and back paths

f6efe1e

Closes #347

monit-reviewer previously approved these changes Jun 18, 2026

View reviewed changes

fix(init): preserve reviewer staging after credential back

3fbf859

Closes #347

rianjs dismissed monit-reviewer’s stale review via 3fbf859 June 18, 2026 20:53

monit-reviewer approved these changes Jun 18, 2026

View reviewed changes

rianjs merged commit 2ab598c into main Jun 18, 2026
10 checks passed

rianjs deleted the feat/347-reviewer-secret-capture branch June 18, 2026 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(init): collect reviewer secrets in reviewer flow#350

fix(init): collect reviewer secrets in reviewer flow#350
rianjs merged 4 commits into
mainfrom
feat/347-reviewer-secret-capture

rianjs commented Jun 18, 2026

Uh oh!

rianjs commented Jun 18, 2026

Uh oh!

rianjs commented Jun 18, 2026

Uh oh!

rianjs commented Jun 18, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Jun 18, 2026

Uh oh!

monit-reviewer Jun 18, 2026

Uh oh!

monit-reviewer Jun 18, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Jun 18, 2026

Uh oh!

monit-reviewer Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -144,19 +144,23 @@ Until the user chooses Commit staged changes and exit:

Uh oh!

Conversation

rianjs commented Jun 18, 2026

Summary

Verification

Uh oh!

rianjs commented Jun 18, 2026

Findings

Uh oh!

rianjs commented Jun 18, 2026

Uh oh!

rianjs commented Jun 18, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants