Skip to content

Add Auth Platform tester management#888

Closed
IAMSamuelRodda wants to merge 3 commits into
openclaw:mainfrom
IAMSamuelRodda:main
Closed

Add Auth Platform tester management#888
IAMSamuelRodda wants to merge 3 commits into
openclaw:mainfrom
IAMSamuelRodda:main

Conversation

@IAMSamuelRodda

@IAMSamuelRodda IAMSamuelRodda commented Jun 30, 2026

Copy link
Copy Markdown

Adds a narrow Auth Platform operator command for OAuth beta/test users.

Summary:

  • adds gog auth-platform testers list/add/remove with oauth alias
  • implements read-add-write-verify behavior for idempotent tester changes
  • supports stored gog auth, direct access tokens, ADC, and service-account auth paths
  • documents the Google Cloud Console private-backend caveat
  • uses --cloud-project so the shipped global --project JSON-projection alias remains compatible

Maintainer repairs

  • Rebased onto current main; contributor commit and authorship preserved.
  • Reduced generated churn from 712 changed files to 17 (+1087/−4).
  • Kept the existing global --project alias; the new command uses --cloud-project.
  • Made only the exact signed GetTrustedUserList POST readonly-safe; add/remove remain blocked by --readonly.
  • Fixed Cloud Resource Manager projectNumber string decoding and remove success output.
  • Replaced a real third-party test address with example.com fixtures.
  • Added regression coverage for string project lookup, remove wording, and exact read-only POST matching.

Exact-head proof

Candidate: 68b3bfa1b38cd80c70a26dd148f729614f67bf84

  • make ci: pass.
  • Autoreview after two bounded repair cycles: clean, no accepted/actionable findings (confidence 0.74).
  • Built binary: v0.31.1-11-g68b3bfa1, SHA-256 1d12e3e7daada8ef6b36bc46ef524859746e1b97e677e11b22e9ed995981d192.
  • Source-blind behavior validation: 3 PASS, 0 FAIL, 2 BLOCKED. CLI compatibility, readonly mutation blocking, and normal string project lookup pass. Live list and mutation are blocked by the provider contract below; no tester state was changed.

Terminal provider blocker

An authenticated, read-only exact-head list against an owned active Google Cloud project resolves the string project ID correctly, then Google returns 403 SERVICE_DISABLED for Cloud Client Private API (cloudclient-pa.googleapis.com). The original contributor reported the same block independently.

Google's official documentation exposes oauthconfig.testusers.get / oauthconfig.testusers.update IAM permissions and the Console UI, but no supported public REST or Discovery tester-list API. This implementation hard-codes an undocumented Console GraphQL endpoint, operation signatures, and request metadata that can change without notice.

The code is locally coherent, but this is not a reliable public CLI contract and cannot meet gog's live-proof requirement. The PR should be closed until Google publishes a supported tester-management endpoint; the repaired branch remains available for reference.

@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 10:57 PM ET / 02:57 UTC.

Summary
The PR adds gog auth-platform/gog oauth tester list/add/remove commands, docs and regenerated command pages, and removes the global --project JSON projection alias.

Reproducibility: yes. for the PR-introduced blockers: source inspection shows the root flag diff removes the existing --project alias, and --readonly auth-platform testers list would use an unallowlisted POST endpoint. I did not run tests because this review is read-only.

Review metrics: 2 noteworthy metrics.

  • Changed files: 712 files affected. Most churn is generated command docs caused by a root flag alias change, so the compatibility decision touches the whole CLI surface.
  • Global alias removed: 1 root alias removed. Removing --project from the global projection flag is user-visible and can break existing scripts.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted terminal output or logs showing the new CLI behavior after the fix; redact tokens, project internals, IPs, non-public endpoints, and other private details.
  • Preserve or explicitly migrate the existing global --project projection alias.
  • [P1] Add a tested readonly-safe list path while keeping add/remove blocked under --readonly.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR needs redacted terminal output, logs, screenshots, recordings, or linked artifacts proving after-fix real behavior; update the PR body after adding proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Removing the existing global --project alias can break scripts that use JSON projection on any command.
  • [P1] The new Auth Platform command depends on a private Google Cloud Console backend that the PR body says is currently blocked for live use, so CI and unit tests do not prove production viability.
  • [P1] The read-only list command currently routes through a POST endpoint blocked by the existing read-only transport.

Maintainer options:

  1. Preserve and prove before merge (recommended)
    Keep the existing global --project projection alias or provide an explicit migration, fix readonly listing, and require redacted live CLI proof before merge.
  2. Accept the breaking flag change
    Maintainers can intentionally approve removing the global alias, but the PR should document it as a breaking CLI change with clear upgrade guidance.
  3. Pause the private-backend command
    If an undocumented Auth Platform backend is not an acceptable core dependency, pause or close this PR until Google exposes a supported public endpoint or a narrower operator path exists.

Next step before merge

  • [P1] Manual review and contributor proof are needed because the blockers include missing real behavior proof, a new private auth-provider backend, and a root-flag compatibility decision.

Security
Cleared: The diff touches auth-provider behavior, but I found no concrete credential leak, dependency, workflow, or supply-chain regression beyond the compatibility/product risks already called out.

Review findings

  • [P1] Preserve the global --project projection alias — internal/cmd/root.go:48
  • [P2] Keep readonly list requests usable — internal/cmd/auth_platform.go:333
Review details

Best possible solution:

Land only after maintainers approve the private Auth Platform backend surface, preserve or explicitly migrate the global alias, keep readonly listing usable, and add redacted live CLI proof.

Do we have a high-confidence way to reproduce the issue?

Yes for the PR-introduced blockers: source inspection shows the root flag diff removes the existing --project alias, and --readonly auth-platform testers list would use an unallowlisted POST endpoint. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

No: the feature direction may be useful, but this is not the best merge shape until global flag compatibility and readonly safety are fixed or explicitly accepted by maintainers.

Full review comments:

  • [P1] Preserve the global --project projection alias — internal/cmd/root.go:48
    This removes --project from the global --select aliases, so existing scripts like gog ... --json --project id stop parsing across every command. Keep the old alias with a deprecation path or choose a non-conflicting Auth Platform project flag.
    Confidence: 0.93
  • [P2] Keep readonly list requests usable — internal/cmd/auth_platform.go:333
    gog --readonly auth-platform testers list still uses NewHTTPClientForScopes, whose transport rejects unallowlisted POSTs. The list call is a POST to the private batchGraphql endpoint, so readonly mode fails before the request reaches Google; add coverage and a safe read path that does not permit add/remove mutations.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 213ddb60d7d1.

Label changes

Label justifications:

  • P2: This is a normal-priority feature PR with concrete compatibility and auth-provider blockers, not an emergency regression on main.
  • merge-risk: 🚨 compatibility: The diff removes a documented global CLI alias from RootFlags and every generated command page.
  • merge-risk: 🚨 auth-provider: The new command manages OAuth consent-screen tester state through a private Auth Platform backend that CI cannot validate.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR needs redacted terminal output, logs, screenshots, recordings, or linked artifacts proving after-fix real behavior; update the PR body after adding proof so ClawSweeper can re-review automatically, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its PR review-mode guidance applies, so the review used gh pr view/raw diff inspection without branch changes or edits. (AGENTS.md:35, 213ddb60d7d1)
  • PR body and prior discussion: The PR body says it adds Auth Platform tester commands, removes the global --project projection alias, and notes live list is blocked by the private Google backend; the existing ClawSweeper comment also identified missing proof, alias compatibility, and readonly list blockers. (3fcea0b03d75)
  • Current main global alias: Current main exposes --project as an alias for the global --select field projection flag. (internal/cmd/root.go:48, 213ddb60d7d1)
  • PR root alias removal: The PR changes RootFlags.Select aliases from pick,project to pick, removing a user-visible global alias across commands. (internal/cmd/root.go:48, 3fcea0b03d75)
  • PR list operation uses POST: The new Auth Platform client sends all operations, including listTesters, through http.MethodPost to the batchGraphql endpoint. (internal/cmd/auth_platform.go:333, 3fcea0b03d75)
  • Current read-only allowlist: Current main allows GET/HEAD/OPTIONS and only a small allowlist of POST hosts/paths; the PR's cloudconsole-pa/clients6 batchGraphql endpoint is not allowlisted. (internal/googleapi/read_only.go:82, 213ddb60d7d1)

Likely related people:

  • Peter Steinberger: Blame shows the current root flag alias, read-only allowlist, account/auth transport path, and release baseline in the relevant files are recent commits by this author; the runtime read-only mode also appears in commit 4cac149. (role: recent area contributor; confidence: high; commits: 608aa46e7b9c, 4cac149d75a7; files: internal/cmd/root.go, internal/googleapi/read_only.go, internal/googleapi/client.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 30, 2026
@steipete

steipete commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Thank you for the contribution, @IAMSamuelRodda. I rebased and repaired the branch at 68b3bfa1b38cd80c70a26dd148f729614f67bf84: preserved the global --project contract via --cloud-project, reduced the diff from 712 to 17 files, fixed readonly routing, project-number decoding, remove output, fixtures, and added regression coverage.

Code proof is clean: full local CI and autoreview pass, and exact-head hosted CI plus Docker are green.

The feature still cannot satisfy gog's live-provider contract. An authenticated read-only call against an owned active project reaches the repaired provider path, then Google returns 403 SERVICE_DISABLED for Cloud Client Private API (cloudclient-pa.googleapis.com). Google's official OAuthConfig documentation publishes the IAM permissions, but no supported public tester-management REST or Discovery API; this PR hard-codes an undocumented Console GraphQL endpoint and signatures. No tester state was changed.

Closing because an unavailable private Console backend is not a reliable public CLI contract. The repaired head remains available for reference, and we would be glad to revisit this when Google publishes a supported, live-provable tester-management endpoint. Thanks again for exploring the gap and documenting the Console behavior.

@steipete steipete closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants