fix(init): show reviewer credential status#351
Conversation
|
Findings Major: Major: Status only inspects the currently planned reviewer credential entry, not every selectable reviewer entity. Minor: The contract docs omit the implemented |
|
Findings Major: The fallback status path can still report Suggested fix: include current template/current input refs in the backend-inspected status set, or make the fallback query the resolved backend for the current ref. If backend lookup is not possible, render The previous issues around stale status after changing the secret-location field and inactive selectable reviewer entities with explicit refs look addressed. |
|
No findings. The updated diff aligns with #348 and preserves the #347 draft/write boundary: backend access is read-only for status, staged secret values remain draft-local, unknown typed refs no longer falsely claim |
|
The new coverage is solid on the happy path, but I see 3 remaining gaps where a regression could still slip through.
|
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d96f21c
Approved with 4 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 3 |
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - docs/init-ux-contract.md:162
The credential status section enumerates the seven possible key states but does not define their preconditions. The PR discussion established a critical invariant:
missingmust only be reported after consulting the backend (meaning not present in backend AND not staged), andstatus unavailableis the fallback when the backend is unreachable. Without these definitions, a future implementer adding a new status path could regress to falsely claimingmissingwithout a backend check — the exact architectural gap this PR fixes. One sentence per state defining its precondition would make the contract self-contained.
harness-engineering:harness-self-documenting-code-reviewer (3 findings)
💡 Suggestion - internal/cmd/credentialcmd/init_reviewer_credential_status.go:229
The variable
refis reassigned across three potential sources (user-suppliedreviewerSecretLocation→ seed fallback → standard ref) without communicating the resolution intent. A name likeeffectiveCredentialRefwould make the fallback priority chain readable at a glance rather than requiring readers to trace through the conditionals.
💡 Suggestion - internal/cmd/credentialcmd/init_reviewer_credential_status.go:258
if ctx.ReviewerCredentialStatuses != nilencodes a non-obvious domain contract: nil means statuses were never computed (speculate withmissing), while non-nil-but-no-match means the backend was consulted and does not know this ref (reportunavailable). This distinction is load-bearing — it is the exact invariant the PR enforces — but a bare nil check does not surface it. ExtractingbackendStatusesWereComputed := ctx.ReviewerCredentialStatuses != nilwould make the intent explicit.
💡 Suggestion - internal/cmd/credentialcmd/init_reviewer_credential_status.go:328
store := strings.TrimSpace(status.SecretsProfile.DisplayName())assigns a display-name string to a variable namedstore, which throughout this package refers to aninitStoreinstance. A reader scanning this function will expect a credential store object.displayNameorstoreLabelwould eliminate the naming collision and match what the variable actually holds.
4 PR discussion threads considered.
Completed in 4m 09s | $2.40 | 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 | 4m 09s wall · 4m 06s compute (Reviewers: 3m 21s · Synthesis: 45s) |
| Cost | $2.40 (estimated) |
| Tokens | 424.8k in / 26.9k out |
| Turns | 14 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 37.8k | 1.9k | 13.8k | 24.0k (1h) | $0.18 |
| documentation:docs-reviewer | sonnet | 34.6k | 1.5k | 13.8k | 20.8k (1h) | $0.15 |
| harness-engineering:harness-architecture-reviewer | sonnet | 66.6k | 753 | 13.8k | 52.8k (1h) | $0.33 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 72.7k | 6.5k | 13.8k | 58.9k (1h) | $0.46 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 68.0k | 1.9k | 13.8k | 54.2k (1h) | $0.36 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 78.7k | 12.8k | 13.8k | 65.0k (1h) | $0.59 |
| security:security-code-auditor | sonnet | 66.4k | 1.4k | 13.8k | 52.6k (1h) | $0.34 |
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.
| @@ -162,6 +162,25 @@ choosing **Commit staged changes and exit**, any pending secret values remain | |||
| draft-only and the session returns to a no-write state. Until final commit | |||
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): The credential status section enumerates the seven possible key states but does not define their preconditions. The PR discussion established a critical invariant: missing must only be reported after consulting the backend (meaning not present in backend AND not staged), and status unavailable is the fallback when the backend is unreachable. Without these definitions, a future implementer adding a new status path could regress to falsely claiming missing without a backend check — the exact architectural gap this PR fixes. One sentence per state defining its precondition would make the contract self-contained.
Reply to this thread when addressed.
| } | ||
| ref := strings.TrimSpace(reviewerSecretLocation) | ||
| if ref == "" { | ||
| ref = strings.TrimSpace(state.seed.ReviewerCredentialRef) |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): The variable ref is reassigned across three potential sources (user-supplied reviewerSecretLocation → seed fallback → standard ref) without communicating the resolution intent. A name like effectiveCredentialRef would make the fallback priority chain readable at a glance rather than requiring readers to trace through the conditionals.
Reply to this thread when addressed.
| status := initReviewerCredentialStatus{Ref: ref} | ||
| if ctx.ExistingProfile != nil { | ||
| if resolved, err := credentials.ResolveSecretsProfileForProfile(ctx.ExistingConfig, *ctx.ExistingProfile); err == nil { | ||
| status.SecretsProfile = resolved |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): if ctx.ReviewerCredentialStatuses != nil encodes a non-obvious domain contract: nil means statuses were never computed (speculate with missing), while non-nil-but-no-match means the backend was consulted and does not know this ref (report unavailable). This distinction is load-bearing — it is the exact invariant the PR enforces — but a bare nil check does not surface it. Extracting backendStatusesWereComputed := ctx.ReviewerCredentialStatuses != nil would make the intent explicit.
Reply to this thread when addressed.
| return fmt.Sprintf("%s via %s", ref, store) | ||
| case backend != "": | ||
| return fmt.Sprintf("%s via %s", ref, backend) | ||
| default: |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): store := strings.TrimSpace(status.SecretsProfile.DisplayName()) assigns a display-name string to a variable named store, which throughout this package refers to an initStore instance. A reader scanning this function will expect a credential store object. displayName or storeLabel would eliminate the naming collision and match what the variable actually holds.
Reply to this thread when addressed.
|
No findings. The daemon changes are additive and aligned with #348: the docs now clarify each status meaning, tests cover ordering and auth-mode filtering, and the code changes are naming/readability only. I don’t see architectural drift from the approved shape: status remains per key, draft-aware, backend-inspected where safe, unavailable when not inspected, and non-secret. |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 5448c0b | Previous: d96f21c (incremental)
Summary
No issues found.
9 PR discussion threads considered.
Completed in 1m 11s | $5.93 | 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 11s wall · 1m 00s compute (Reviewers: 39s · Synthesis: 22s) |
| Cost | $5.93 (estimated) |
| Tokens | 1095.8k in / 31.8k out |
| Turns | 28 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 86.5k | 2.9k | 28.2k | 58.3k (1h) | $0.40 |
| documentation:docs-reviewer | sonnet | 78.0k | 2.2k | 28.2k | 49.8k (1h) | $0.34 |
| harness-engineering:harness-architecture-reviewer | sonnet | 174.4k | 937 | 28.2k | 146.2k (1h) | $0.90 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 194.0k | 7.9k | 28.2k | 165.8k (1h) | $1.12 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 179.3k | 2.9k | 28.2k | 151.1k (1h) | $0.96 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 210.5k | 13.4k | 28.2k | 182.3k (1h) | $1.30 |
| security:security-code-auditor | sonnet | 173.1k | 1.6k | 28.2k | 144.9k (1h) | $0.90 |
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.
|
No findings. The post-daemon diff is still aligned with #348. The added docs clarify the state contract, the test additions strengthen mode filtering and UI ordering, and the implementation still preserves the intended architecture: per-key non-secret status, backend-inspected |
2 participants
Summary
Verification
github_app_idandgithub_app_private_keyasmissing, optionalgithub_app_installation_idasoptionalstaged,staged, andskipped optional; raw sentinel values were not renderedgit_token: existing; raw token was not renderedmake tidymake lintmake testmake buildCloses #348