fix(init): show credential destination context by rianjs · Pull Request #352 · open-cli-collective/codereview-cli

rianjs · 2026-06-18T22:13:20Z

Closes #349

Summary

show non-secret destination summaries in shared credential collection prompts
enrich reviewer credential status destinations with backend and 1Password routing context
document the credential-bearing flow vs reusable secrets-management workflow

Verification

go test ./internal/cmd/credentialcmd
make tidy
make lint
make test
make build
tmux capture: default GitHub App reviewer shows Automatic OS default (macOS Keychain)
tmux capture: named 1Password PAT reviewer shows Team Vault, 1Password service account, vault Engineering, item title prefix cr-, item tag code-review, and item field title credential without raw secret values
tmux capture: main menu still exposes Configure secrets management

Closes #349

rianjs · 2026-06-18T22:15:39Z

Findings

Major: Explicit runtime backend summaries use the formatted CLI hint instead of the raw backend value in the shared credential collector. collectInteractiveInitSecrets passes plan.backendArg into initCredentialDestinationDescription, but plan.backendArg is produced by interactiveInitBackendArg as a command suffix like " --backend memory", not "memory". credentials.BackendMetadata expects the raw backend value, so credential prompts under an explicit runtime backend can render credential destination unavailable instead of the resolved backend. Use opts.Backend or store a separate raw backend value for destination formatting, and add a regression for --backend memory/backendFlagSet=true on a non-named credential entry. See credentialcmd.go:5767, credentialcmd.go:5590, and init_credential_destination.go:71.

Minor: The PR verification text does not explicitly cover the required tmux capture for a 1Password profile configured with item tag, field title, and title prefix. The unit test covers these labels, but the ticket requires empirical PTY evidence. Please either add that capture or make the PR verification clear that the named 1Password PAT tmux run included those configured labels.

Minor: The destination helper renders 1Password desktop account id, but there is no destination-summary unit test for the op-desktop path. Since the agreed plan called out service/connect/desktop metadata, add a small desktop-profile case that asserts the 1Password desktop app label and desktop account id are shown without secret leakage.

rianjs · 2026-06-18T22:17:29Z

No findings.

The updated diff resolves the runtime-backend bug by passing the raw opts.Backend into destination formatting while keeping the existing formatted backendArg for command/hint contexts. The new regression covers that path, the desktop 1Password destination metadata is now tested, and the PR verification explicitly includes the required 1Password routing tmux capture. Product scope still looks clean: non-secret destination context is added where credentials are collected, without changing write timing or removing the top-level secrets-management workflow.

rianjs · 2026-06-18T22:19:55Z

The new tests cover the string-building helpers well, but they do not fully prove the user-facing prompt branches that this PR changed.

Findings

Major: the reviewer-entity path still isn’t proven end-to-end. TestInitReviewerCredentialStatusShowsExistingPATAndSecretsProfileDestination only asserts the old destination summary shape, so it would still pass if buildInteractiveInitReviewerCredentialStatuses never populated status.Destination and initReviewerCredentialStatusDescription kept falling back to the legacy formatter. That leaves the named-profile reviewer flow, which is the main ticket goal, effectively untested. See internal/cmd/credentialcmd/credentialcmd_test.go and internal/cmd/credentialcmd/init_reviewer_credential_status.go.
Major: the shared credential prompt coverage is too indirect. TestCollectInteractiveInitSecretsPassesDestinationToSharedCredentialPrompts only verifies that a destination string is threaded into the prompt struct, and TestHuhInitSecretPrompterAccessibleShowsCredentialDestination only exercises ChooseCredentialAction. There is no test for ChooseSecretSource or PasteSecret, and no test for the GitHub App private-key branch where PasteSecret replaces the description. Those are exactly the branches that could drop the destination note and still leave every new test green. See internal/cmd/credentialcmd/credentialcmd_test.go and internal/cmd/credentialcmd/credentialcmd.go.
Minor: the legacy/default backend coverage is helper-level only. TestCollectInteractiveInitSecretsDestinationUsesRawRuntimeBackend checks an explicit --backend memory override, not the actual inferred default path that the ticket calls out. The direct helper test for auto/default copy helps, but there is still no end-to-end assertion that collectInteractiveInitSecrets surfaces the platform-specific default when no backend is explicitly set. See internal/cmd/credentialcmd/credentialcmd_test.go and internal/cmd/credentialcmd/init_credential_destination.go.

monit-reviewer

Automated PR Review

Reviewed commit: 5dbef42

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

Summary

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

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

💡 Suggestion - internal/cmd/credentialcmd/init_credential_destination.go:111

Both ServiceTokenEnv and ConnectTokenEnv emit the identical label "1Password backend auth env var: ", making the two entries indistinguishable in the rendered destination summary. Service account and Connect are distinct authentication mechanisms. Using "1Password service account token env var: " and "1Password Connect token env var: " respectively would let the output self-document which integration is active.

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

In initSecretPasteDescription, the variable description changes semantic role mid-function: it starts as the hidden-input notice text and ends as the full composed display string. Assigning the notice to a named variable like hiddenInputNotice and composing the return value explicitly (e.g. return destination + "\n" + hiddenInputNotice) would make the combination unambiguous and easier to follow.

💡 Suggestion - internal/cmd/credentialcmd/init_credential_destination.go:56

In initNamedCredentialDestinationDetails, strings.TrimSpace(resolved.ID) and strings.TrimSpace(string(backendKind)) are each evaluated twice — once in the condition and once in the append. The rest of the file consistently uses if value := strings.TrimSpace(...); value != "" to avoid the duplication. Using the same scoped-assignment pattern here would match the surrounding style and eliminate the need for a reader to verify both evaluations are identical.

1 info-level observations excluded. Run with --verbose to include.

3 PR discussion threads considered.

Completed in 3m 19s | $2.01 | 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	3m 19s wall · 3m 16s compute (Reviewers: 2m 35s · Synthesis: 41s)
Cost	$2.01 (estimated)
Tokens	375.9k in / 20.4k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	37.7k	2.1k	13.8k	23.9k (1h)	$0.18
documentation:docs-reviewer	sonnet	33.8k	1.0k	13.8k	20.0k (1h)	$0.14
harness-engineering:harness-architecture-reviewer	sonnet	58.3k	821	13.8k	44.5k (1h)	$0.28
harness-engineering:harness-enforcement-reviewer	sonnet	60.7k	2.8k	13.8k	46.9k (1h)	$0.33
harness-engineering:harness-knowledge-reviewer	sonnet	59.7k	2.0k	13.8k	45.9k (1h)	$0.31
harness-engineering:harness-self-documenting-code-reviewer	sonnet	67.6k	10.2k	13.8k	53.8k (1h)	$0.48
security:security-code-auditor	sonnet	57.9k	1.4k	13.8k	44.1k (1h)	$0.29

_{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-18T22:25:05Z

+	}
+	if value := strings.TrimSpace(onePassword.ServiceTokenEnv); value != "" {
+		lines = append(lines, "1Password backend auth env var: "+value)
+	}


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): Both ServiceTokenEnv and ConnectTokenEnv emit the identical label "1Password backend auth env var: ", making the two entries indistinguishable in the rendered destination summary. Service account and Connect are distinct authentication mechanisms. Using "1Password service account token env var: " and "1Password Connect token env var: " respectively would let the output self-document which integration is active.

Reply to this thread when addressed.

monit-reviewer · 2026-06-18T22:25:05Z

@@ -6811,6 +6836,17 @@ func readSecretIngress(r io.Reader, stdin bool, envVar, stdinFlag, envFlag strin
 	return value, nil
 }


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): In initSecretPasteDescription, the variable description changes semantic role mid-function: it starts as the hidden-input notice text and ends as the full composed display string. Assigning the notice to a named variable like hiddenInputNotice and composing the return value explicitly (e.g. return destination + "\n" + hiddenInputNotice) would make the combination unambiguous and easier to follow.

Reply to this thread when addressed.

monit-reviewer · 2026-06-18T22:25:05Z

+	}
+	backendLabel := initSecretsBackendDisplayLabel(backendKind)
+	lines := []string{}
+	if strings.TrimSpace(resolved.ID) != "" {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): In initNamedCredentialDestinationDetails, strings.TrimSpace(resolved.ID) and strings.TrimSpace(string(backendKind)) are each evaluated twice — once in the condition and once in the append. The rest of the file consistently uses if value := strings.TrimSpace(...); value != "" to avoid the duplication. Using the same scoped-assignment pattern here would match the surrounding style and eliminate the need for a reader to verify both evaluations are identical.

Reply to this thread when addressed.

monit-reviewer

Automated PR Review

Reviewed commit: 8afb1c3 | Previous: 5dbef42 (incremental)

Summary

No issues found.

6 PR discussion threads considered.

Completed in 1m 15s | $4.92 | 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 15s wall · 1m 12s compute (Reviewers: 43s · Synthesis: 29s)
Cost	$4.92 (estimated)
Tokens	944.8k in / 25.4k out
Turns	28

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	86.0k	3.5k	28.2k	57.8k (1h)	$0.41
documentation:docs-reviewer	sonnet	74.2k	1.1k	28.2k	46.0k (1h)	$0.30
harness-engineering:harness-architecture-reviewer	sonnet	148.8k	1.0k	28.2k	120.6k (1h)	$0.75
harness-engineering:harness-enforcement-reviewer	sonnet	156.9k	3.9k	28.2k	128.7k (1h)	$0.84
harness-engineering:harness-knowledge-reviewer	sonnet	153.0k	2.2k	28.2k	124.8k (1h)	$0.79
harness-engineering:harness-self-documenting-code-reviewer	sonnet	178.3k	12.0k	28.2k	150.1k (1h)	$1.09
security:security-code-auditor	sonnet	147.6k	1.6k	28.2k	119.4k (1h)	$0.75

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

rianjs · 2026-06-18T22:29:01Z

No findings.

The daemon edits stay aligned with the ticket intent. They tightened the prompt-description path, added useful regressions for inferred default backend copy, secret-source rendering, private-key paste copy, and reviewer status destination enrichment, without changing the architectural boundary: credential-bearing flows show non-secret destination context, secrets-management remains the reusable backend configuration path, and raw token values remain out of UI output.

fix(init): show credential destination context

2f611a0

Closes #349

fix(init): use raw backend for destination summaries

a3b6296

test(init): cover credential destination prompt paths

5dbef42

monit-reviewer previously approved these changes Jun 18, 2026

View reviewed changes

chore(init): clarify destination summary labels

8afb1c3

rianjs dismissed monit-reviewer’s stale review via 8afb1c3 June 18, 2026 22:26

monit-reviewer approved these changes Jun 18, 2026

View reviewed changes

rianjs merged commit 6d0c816 into main Jun 18, 2026
10 checks passed

rianjs deleted the feat/349-credential-destination-context branch June 18, 2026 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(init): show credential destination context#352

fix(init): show credential destination context#352
rianjs merged 4 commits into
mainfrom
feat/349-credential-destination-context

rianjs commented Jun 18, 2026 •

edited

Loading

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!

rianjs commented Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -6811,6 +6836,17 @@ func readSecretIngress(r io.Reader, stdin bool, envVar, stdinFlag, envFlag strin
		return value, nil
		}

Uh oh!

Conversation

rianjs commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

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

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!

rianjs commented Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rianjs commented Jun 18, 2026 •

edited

Loading