Skip to content

feat(init): add audience prompt for admin-provided vs DIY OAuth setup - #124

Merged
rianjs merged 3 commits into
mainfrom
feat/123-init-audience-prompt
May 13, 2026
Merged

feat(init): add audience prompt for admin-provided vs DIY OAuth setup#124
rianjs merged 3 commits into
mainfrom
feat/123-init-audience-prompt

Conversation

@rianjs

@rianjs rianjs commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a top-level SelectAudience prompt to gro init that routes users between two prefatory messages: admin-provisioned (paste/point to credentials.json shared via 1Password etc.) and DIY (the existing Google Cloud Console setup steps, now augmented with a pointer to WORKSPACE_ADMINS.md for admins who'd rather provision once for their whole org).
  • Both paths fall into the same SelectCredSource → save logic. --credentials-file still bypasses the wizard entirely.
  • SelectAudience is asked exactly once per wizard invocation — sticky across credential-validation retries.

Why now

PR #122 merged WORKSPACE_ADMINS.md. Today, every user who runs gro init sees Google Cloud Console setup steps even if their admin already handed them a credentials.json. This PR aligns the wizard with the new doc so neither audience reads past content they don't need.

Tests

Three new tests in init_test.go:

  • TestEnsureCredentialsAudienceAdminSkipsDIYSteps — admin audience does not print the 7-line DIY block but does print the admin one-liner.
  • TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer — DIY audience prints the existing block plus a fully-qualified link to WORKSPACE_ADMINS.md on GitHub.
  • TestEnsureCredentialsAudienceIsAskedOncePerWizard — sticky behavior: clipboard returns invalid JSON on call 1 and valid JSON on call 2; `audience` is called exactly once and `select` exactly twice across the retry.

Two existing bypass tests extended to assert `audience` is NOT called:

  • `TestEnsureCredentialsFlagFile` (--credentials-file bypass)
  • `TestEnsureCredentialsShortCircuitsWhenAlreadyPresent` (existing creds.json bypass)

Test plan

  • `make check` passes (was green locally; 49 tests pass in initcmd)
  • Manual: `rm -f ~/.config/google-readonly/credentials.json && gro config clear` then `gro init` — admin path shows no DIY block, DIY path shows block + admin doc URL.

Closes #123

A new top-level wizard question routes users between two prefatory
messages: admin-provisioned users get a one-liner pointing at the
credentials.json their admin shared; DIY users get the existing
Google Cloud Console setup steps plus a pointer to WORKSPACE_ADMINS.md
for admins who want to provision once for their whole org.

Both paths flow into the same SelectCredSource → save logic. The
--credentials-file flag still bypasses the wizard entirely.

SelectAudience is asked exactly once per wizard invocation; sticky
across credential-validation retries.

Closes #123
@rianjs

rianjs commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Minor

  • internal/cmd/initcmd/init.go:60: cmd.Long still says “see WORKSPACE_ADMINS.md in the repository.” Installed CLI users do not have that file locally. Use the same fully-qualified GitHub URL as the DIY wizard text.
  • internal/cmd/initcmd/init_test.go:452: the DIY pointer test only asserts WORKSPACE_ADMINS.md, so it would pass if the wizard regressed back to a bare filename. Assert the full URL to lock the product requirement.

Nit

  • internal/cmd/initcmd/init.go:434: consider a switch audience with a default error, matching SelectCredSource. The current else silently treats any unexpected value as DIY.

Code Quality Lens: the core architecture is right: audience only changes prefatory text, both branches share credential source/save logic, retries keep audience sticky, and both bypasses are preserved. Remaining issues are small user-facing/help-text consistency and test precision gaps.

- Use fully-qualified WORKSPACE_ADMINS.md URL in cmd.Long (matches
  the wizard text; reachable by installed-CLI users)
- Tighten DIY-pointer test assertion to the full URL so a regression
  to bare filename would fail
- Switch audience branch to switch/default-error pattern, matching
  the SelectCredSource pattern; rejects unexpected prompter values
@rianjs

rianjs commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Findings

None.

The updated PR now matches the intended architecture: SelectAudience only controls prefatory text, both audiences flow into the same SelectCredSource and save path, retries keep the audience decision sticky, and both --credentials-file plus existing-credentials bypasses remain prompt-free.

Code Quality Lens: no scope creep, no unrelated edits, no hidden local-doc assumption, and the important behavior is covered by focused tests.

@rianjs

rianjs commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

TL;DR: Coverage is adequate. Optional nit below; not blocking.

Walked through the diff against init_test.go:

  • Admin branch — covered (TestEnsureCredentialsAudienceAdminSkipsDIYSteps, asserts DIY steps absent + admin one-liner present).
  • DIY branch — covered (TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer, asserts DIY steps + fully-qualified WORKSPACE_ADMINS.md URL).
  • Sticky-across-retries — covered (TestEnsureCredentialsAudienceIsAskedOncePerWizard with a 2-attempt clipboard retry, asserts audience==1 and select==2). This is the test I'd most want to see and it's here.
  • --credentials-file bypass — covered with a negative assertion that SelectAudience is not in stub.calls.
  • Existing-creds short-circuit — covered with the same negative assertion.

Gap: the default: arm of the audience switch (unknown audience: %s) is not exercised. The producer (huhPrompter.SelectAudience) only ever returns "admin" or "diy", so this is purely defensive against future refactors. Pragmatic call — fine to leave as-is. If you want belt-and-suspenders, a 5-line test setting stubPrompter{audience: "bogus"} and asserting the error would do it, but I wouldn't block on it.

Test patterns match the rest of the file (parallel, baseDeps, stubPrompter, output-buffer assertions). Ship it.

t.Errorf("diy audience should show DIY steps; output:\n%s", got)
}
if !strings.Contains(got, "https://github.com/open-cli-collective/google-readonly/blob/main/WORKSPACE_ADMINS.md") {
t.Errorf("diy audience should show the fully-qualified Workspace admin doc URL (so Homebrew/Choco users can reach it); output:\n%s", got)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium (harness-engineering:harness-self-documenting-code-reviewer): TestEnsureCredentialsAudienceIsAskedOncePerWizard accesses reads[idx] without bounds checking. If ClipboardReadAll is called more than twice (e.g., after a refactor raising the retry cap), the test panics with an index-out-of-range instead of failing cleanly. Use a bounds-checked helper or t.Fatal inside the closure.

Reply to this thread when addressed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8a40d4d — the ClipboardReadAll closure now checks idx >= len(reads) and calls t.Fatalf if the wizard makes more than 2 reads, so a future retry-cap bump fails cleanly instead of panicking with index-out-of-range.

calls []string
}

func (s *stubPrompter) SelectAudience() (string, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): stubPrompter.SelectAudience() silently returns "diy" when the audience field is the zero value (empty string). Any existing or future test that constructs a &stubPrompter{} without setting audience will quietly exercise the diy path rather than failing. A sentinel value or required-non-empty check would surface misconfigured stubs earlier.

Reply to this thread when addressed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining — this was a deliberate design choice flagged in the architect review for this PR. The zero-value default to 'diy' was specifically requested to avoid touching ~20 existing test setups that would otherwise need to set audience: "diy" to keep compiling. Existing tests that exercise the wizard exercised the DIY path before this PR; defaulting their zero-value to 'diy' preserves that semantic exactly. New tests that care about audience set it explicitly. The two bypass tests (TestEnsureCredentialsFlagFile, TestEnsureCredentialsShortCircuitsWhenAlreadyPresent) now assert audience is NOT in calls, which catches the misuse case you're worried about: if a wizard test forgot to think about audience and the audience prompt fires unexpectedly, those tests would surface it.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 6821029

Summary

Reviewer Findings
harness-engineering:harness-architecture-reviewer 1
harness-engineering:harness-self-documenting-code-reviewer 2
harness-engineering:harness-architecture-reviewer (1 findings)

💡 Suggestion - internal/cmd/initcmd/init.go:477

The WORKSPACE_ADMINS.md URL is hardcoded in two places: the cobra command's Long description and the runtime Println in the diy case. A single package-level constant would eliminate drift if the repo is renamed or the doc is moved.

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

⚠️ Should Fix - internal/cmd/initcmd/init_test.go:458

TestEnsureCredentialsAudienceIsAskedOncePerWizard accesses reads[idx] without bounds checking. If ClipboardReadAll is called more than twice (e.g., after a refactor raising the retry cap), the test panics with an index-out-of-range instead of failing cleanly. Use a bounds-checked helper or t.Fatal inside the closure.

💡 Suggestion - internal/cmd/initcmd/init_test.go:155

stubPrompter.SelectAudience() silently returns "diy" when the audience field is the zero value (empty string). Any existing or future test that constructs a &stubPrompter{} without setting audience will quietly exercise the diy path rather than failing. A sentinel value or required-non-empty check would surface misconfigured stubs earlier.

3 PR discussion threads considered.


Completed in 2m 30s | $0.71 | sonnet | daemon 0.2.116 | Glorfindel
Field Value
Model sonnet
Reviewers hybrid-synthesis, 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 30s wall · 7m 14s compute (Reviewers: 1m 48s · Synthesis: 40s)
Cost $0.71
Tokens 186.7k in / 28.9k out
Turns 10

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost
hybrid-synthesis sonnet 30.1k 2.0k 13.6k 16.5k (1h) $0.10
harness-engineering:harness-architecture-reviewer sonnet 35.0k 4.5k 19.8k 15.2k (1h) $0.14
harness-engineering:harness-enforcement-reviewer sonnet 36.0k 5.3k 19.8k 16.2k (1h) $0.15
harness-engineering:harness-knowledge-reviewer sonnet 35.2k 4.5k 19.8k 15.4k (1h) $0.14
harness-engineering:harness-self-documenting-code-reviewer sonnet 34.2k 3.5k 19.8k 14.4k (1h) $0.12
security:security-code-auditor haiku 16.2k 9.1k 0 16.2k (1h) $0.07

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.

Note: Posted 2 inline comments. 1 finding remained in the summary because GitHub could not attach them to the current diff.

…e test

- Extract workspaceAdminsURL to a package-level const, referenced from
  both cmd.Long (via fmt.Sprintf) and the runtime wizard. Eliminates
  drift if the repo moves or the doc is renamed.
- Bounds-check ClipboardReadAll closure in
  TestEnsureCredentialsAudienceIsAskedOncePerWizard so a future retry-cap
  bump fails cleanly with t.Fatal instead of an index-out-of-range panic.

Addresses two pr-review-daemon findings on PR #124.
@rianjs

rianjs commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Re. the URL-constant suggestion from harness-architecture-reviewer: addressed in 8a40d4d. Extracted workspaceAdminsURL to a package-level const referenced from both cmd.Long (via fmt.Sprintf) and the runtime wizard.

The bounds-check finding is fixed in the same commit. The audience zero-value default suggestion is being declined with rationale on the inline thread.

@rianjs
rianjs merged commit e906613 into main May 13, 2026
2 checks passed
@rianjs
rianjs deleted the feat/123-init-audience-prompt branch May 13, 2026 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(init): add audience prompt distinguishing admin-provided vs DIY OAuth setup

2 participants