Skip to content

fix(auth): support OpenAI OAuth vision in Peekaboo - #293

Open
scotthuang wants to merge 3 commits into
openclaw:mainfrom
scotthuang:fix/openai-oauth-vision
Open

fix(auth): support OpenAI OAuth vision in Peekaboo#293
scotthuang wants to merge 3 commits into
openclaw:mainfrom
scotthuang:fix/openai-oauth-vision

Conversation

@scotthuang

@scotthuang scotthuang commented Jul 23, 2026

Copy link
Copy Markdown

What problem this solves

Peekaboo already converts captures into provider-neutral image messages, but its OpenAI availability gate only accepted a currently valid OAuth access token. Once that token expired, Peekaboo reported OpenAI as unavailable and stopped before Tachikoma had a chance to use the refresh token.

Previous call chain

expired OpenAI access token + valid refresh token
  -> ConfigurationManager.hasOpenAIAuth()
  -> validOAuthAccessToken() == nil
  -> OpenAI reported unavailable
  -> Tachikoma refresh/vision path never reached

New call chain

expired OpenAI access token + valid refresh token
  -> ConfigurationManager.hasOpenAIAuth() == true
  -> Tachikoma refreshes the OAuth session
     - stored login: atomically persists the rotated chain
     - environment source: caches the rotated chain only in this process
  -> Codex Responses receives text + image
  -> visual answer returned

Implementation

  • treat a refreshable OpenAI OAuth session as available when the current access token has expired
  • keep OAuth access tokens and expiry timestamps paired by credential source, rather than mixing environment and stored values
  • bump Tachikoma to the Codex OAuth Responses vision implementation and its environment-credential boundary fix
  • document OpenAI OAuth support for see --analyze, image --analyze, and agent vision
  • document that environment OAuth refreshes stay process-local while peekaboo config login persists refresh rotation

Evidence

Red-green availability proof

The deterministic setup stores:

OPENAI_ACCESS_TOKEN=<expired token>
OPENAI_ACCESS_EXPIRES=<one hour in the past>
OPENAI_REFRESH_TOKEN=<present>

Red step: the PR test was run with only the new refresh-token clause temporarily removed, reproducing the previous implementation.

return self.validOAuthAccessToken(prefix: "OPENAI") != nil

Observed result:

Expectation failed: manager.hasOpenAIAuth()
Test failed after 0.008 seconds

Green step: restore the PR implementation:

return self.validOAuthAccessToken(prefix: "OPENAI") != nil ||
    self.hasOAuthRefreshToken(prefix: "OPENAI")

Observed result:

Test passed after 0.007 seconds

Only that availability condition changed between the red and green runs. Both repositories were restored to clean state afterward.

Reproduction command:

/usr/bin/arch -arm64 swift test \
  --package-path Core/PeekabooCore \
  -Xswiftc -DPEEKABOO_SKIP_AUTOMATION \
  --filter "OpenAI availability accepts an expired access token with a refresh token"

Real OAuth vision proof from the pinned Tachikoma dependency

openclaw/Tachikoma#40 includes protocol-level red/green evidence using the same valid ChatGPT OAuth token:

Old public endpoint -> HTTP 401, missing api.responses.write scope
New Codex endpoint  -> deterministic 16x16 red PNG -> GPT-5.6 Sol returned "red"
Elapsed             -> 6.252 seconds
SSE completion      -> status=completed, total_tokens=55

The image is embedded in the live integration test; no host desktop was captured.

Environment credential-boundary proof

The pinned Tachikoma commit d4cd49f adds direct tests proving that an environment-supplied OAuth refresh does not write to ~/.peekaboo/credentials:

cache refreshed environment credentials without persisting      passed
continue an environment refresh chain in memory                 passed
environment value changes invalidate the in-memory chain         passed
ignore-environment changes invalidate the in-memory chain        passed
refresh environment account without overwriting stored account  passed

The tests assert an empty credential store for environment-only sessions, unchanged stored values when a separate stored account exists, correct use of rotated refresh tokens, and cache invalidation when the source changes. Stored-login refreshes retain their separate atomic-persistence test.

Dependency

Important

Depends on openclaw/Tachikoma#40. This PR must merge only after that dependency lands; then the Tachikoma gitlink must be repinned to the final upstream merge commit before Peekaboo merges.

User impact

After the Tachikoma dependency lands, users authenticated through peekaboo config login openai can use OpenAI OAuth for image analysis and agent vision without configuring a separate OpenAI API key. Existing explicit API-key configurations retain priority. Hosts that inject OAuth through environment variables keep those credentials ephemeral and must provide fresh values after a process restart.

Validation

  • deterministic availability red/green: failed in 0.008 seconds before the clause, passed in 0.007 seconds with the fix
  • ConfigurationAccessorsOAuthTests — 14 tests passed
  • pinned Tachikoma AuthManagerTests — 19 tests passed
  • pinned Tachikoma OpenAIResponsesProviderTests — 30 tests passed
  • pinned Tachikoma full suite — 842 of 843 tests passed; the only failure is an unrelated existing real-network OpenAI audio test that timed out after 60 seconds
  • previous Peekaboo head passed all five macOS CI jobs; the new submodule-pointer/docs head is tracked in the current CI run
  • pnpm run lint:docs — passed
  • git diff --check

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 27, 2026
@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed July 30, 2026, 3:38 AM ET / 07:38 UTC.

ClawSweeper review

What this changes

This PR lets Peekaboo treat refreshable OpenAI OAuth sessions as available for vision, updates the Tachikoma dependency, adds regression tests, and documents stored versus environment credential refresh behavior.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep this PR open. Its OAuth availability and credential-boundary work has focused tests and credible runtime evidence, but the branch still pins a pre-merge Tachikoma commit even though the PR explicitly requires repinning to the final upstream merge revision before Peekaboo can safely land it.

Priority: P2
Reviewed head: 99254685f7d9b765bb1beca4be5b181f3b62d425

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The evidence is strong and the patch is focused, but a merge-blocking dependency pin remains unresolved.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides focused red/green terminal results for the availability gate and linked live provider evidence for the OAuth vision path; the remaining blocker is dependency finalization, not missing behavior proof.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides focused red/green terminal results for the availability gate and linked live provider evidence for the OAuth vision path; the remaining blocker is dependency finalization, not missing behavior proof.
Evidence reviewed 5 items Unresolved upstream dependency: The PR body states that the Tachikoma OAuth work must merge upstream and that this repository must then repin the gitlink to the final upstream merge commit; the current branch still points to pre-merge commit d4cd49f.
Prior review continuity: The latest completed ClawSweeper review at the same head SHA identified the same unresolved P1: repin Tachikoma to its final merged OAuth revision. The PR head has not changed since that review.
Focused availability coverage: The branch adds a deterministic regression test for an expired OpenAI access token paired with a refresh token, matching the reported gate behavior.
Findings 1 actionable finding [P1] Repin Tachikoma to the final merged OAuth revision
Security Needs attention Do not ship the pre-merge OAuth dependency pin: The gitlink controls the implementation that refreshes OAuth sessions and keeps environment credentials out of persistent storage; a final upstream merge pin is required to ensure the reviewed boundary is what Peekaboo ships.

How this fits together

Peekaboo’s configuration layer decides whether an OpenAI credential can start image analysis or agent vision, then hands the request to Tachikoma’s provider runtime. The changed gate must allow a refreshable OAuth session through without mixing environment credentials with persisted login state.

flowchart LR
  A[OAuth credentials] --> B[Peekaboo configuration]
  B --> C{API key or usable refresh path?}
  C -->|yes| D[Tachikoma OAuth provider]
  C -->|no| E[OpenAI unavailable]
  D --> F[Refresh credential session]
  F --> G[Vision request]
  G --> H[Visual answer]
Loading

Before merge

  • Repin Tachikoma to the final merged OAuth revision (P1) - This gitlink still selects d4cd49f, a pre-merge dependency commit. The PR itself requires a repin after the linked upstream OAuth work merges; otherwise Peekaboo can ship provider and credential-boundary behavior that differs from the final reviewed upstream implementation.
  • Resolve security concern: Do not ship the pre-merge OAuth dependency pin - The gitlink controls the implementation that refreshes OAuth sessions and keeps environment credentials out of persistent storage; a final upstream merge pin is required to ensure the reviewed boundary is what Peekaboo ships.
  • Resolve merge risk (P1) - Merging with the pre-merge Tachikoma gitlink can ship OAuth provider and environment-credential behavior that differs from the finalized upstream implementation.
  • Resolve merge risk (P1) - The change controls whether refreshable OAuth credentials reach a provider that can rotate tokens, so an incorrect dependency revision can affect authentication and the environment-versus-persisted credential boundary.
  • Complete next step (P2) - The next action depends on the external Tachikoma merge and a contributor repin, not a safe standalone ClawSweeper repair.

Findings

  • [P1] Repin Tachikoma to the final merged OAuth revision — Tachikoma:1
  • [medium] Do not ship the pre-merge OAuth dependency pin — Tachikoma:1
Agent review details

Security

Needs attention: The credential-boundary logic is security-sensitive, and the branch must not ship its pre-merge Tachikoma revision as the finalized dependency.

Review metrics

Metric Value Why it matters
Patch scope 5 files affected; 75 added, 16 removed The implementation is focused on the availability gate, credential-source handling, one regression test, OAuth documentation, and the dependency pin.
Dependency state 1 submodule gitlink changed This single pointer determines which OAuth provider and credential-boundary implementation Peekaboo ships.

Merge-risk options

Maintainer options:

  1. Repin after upstream merge (recommended)
    Wait for the linked Tachikoma OAuth work to merge, then update this gitlink to its final merge commit and rerun the focused validation.
  2. Pause this dependent branch
    Keep the PR open but do not merge it until the upstream dependency has a final reviewed revision.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
After Tachikoma OAuth merges, repin the submodule to its final merge commit and rerun the focused OAuth availability and credential-boundary validation.

Technical review

Best possible solution:

After the upstream Tachikoma OAuth PR is merged, repin Tachikoma to that exact final merge commit and rerun the focused Peekaboo OAuth tests plus the upstream provider and credential-boundary checks before merging.

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

Yes. The PR supplies a deterministic current-path test using an expired access token plus a refresh token, with an exact focused Swift test command and red/green results; this read-only review did not execute it locally.

Is this the best way to solve the issue?

Unclear. Allowing a refreshable session through the existing availability gate is a narrow solution, but this branch is not the best landing path until its Tachikoma gitlink is repinned to the final upstream merge revision.

Full review comments:

  • [P1] Repin Tachikoma to the final merged OAuth revision — Tachikoma:1
    This gitlink still selects d4cd49f, a pre-merge dependency commit. The PR itself requires a repin after the linked upstream OAuth work merges; otherwise Peekaboo can ship provider and credential-boundary behavior that differs from the final reviewed upstream implementation.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8448599ab899.

Labels

Label justifications:

  • P2: The PR addresses a bounded OpenAI OAuth vision availability problem without evidence of a currently broad runtime outage.
  • merge-risk: 🚨 compatibility: A submodule revision chosen before its final upstream merge can differ from the reviewed provider behavior delivered to existing OAuth users.
  • merge-risk: 🚨 auth-provider: The changed availability gate and dependency pin directly determine whether OpenAI OAuth credentials can reach the provider refresh path.
  • merge-risk: 🚨 security-boundary: The dependent change governs whether refreshed environment OAuth credentials remain process-local rather than entering persisted credentials.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides focused red/green terminal results for the availability gate and linked live provider evidence for the OAuth vision path; the remaining blocker is dependency finalization, not missing behavior proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides focused red/green terminal results for the availability gate and linked live provider evidence for the OAuth vision path; the remaining blocker is dependency finalization, not missing behavior proof.

Evidence

Security concerns:

  • [medium] Do not ship the pre-merge OAuth dependency pin — Tachikoma:1
    The gitlink controls the implementation that refreshes OAuth sessions and keeps environment credentials out of persistent storage; a final upstream merge pin is required to ensure the reviewed boundary is what Peekaboo ships.
    Confidence: 0.97

What I checked:

  • Unresolved upstream dependency: The PR body states that the Tachikoma OAuth work must merge upstream and that this repository must then repin the gitlink to the final upstream merge commit; the current branch still points to pre-merge commit d4cd49f. (Tachikoma:1, d4cd49f0e8ec)
  • Prior review continuity: The latest completed ClawSweeper review at the same head SHA identified the same unresolved P1: repin Tachikoma to its final merged OAuth revision. The PR head has not changed since that review. (Tachikoma:1, 99254685f7d9)
  • Focused availability coverage: The branch adds a deterministic regression test for an expired OpenAI access token paired with a refresh token, matching the reported gate behavior. (Core/PeekabooCore/Tests/PeekabooTests/ConfigurationAccessorsOAuthTests.swift:81, 99254685f7d9)
  • Credential-source boundary change: The branch separates environment and stored access-token/expiry pairs before checking token validity, which is the intended defense against combining credentials from different sources. (Core/PeekabooCore/Sources/PeekabooAutomation/Configuration/ConfigurationManager+Credentials.swift:89, 99254685f7d9)
  • Repository policy applied: The fully supplied target AGENTS.md directs submodule work to land upstream first and then bump the gitlink in this repository, which supports requiring a final upstream revision before merge. (AGENTS.md:5, 8448599ab899)

Likely related people:

  • scotthuang: Authored the three commits on this PR, including the environment credential-boundary update and the current submodule pin; no stronger current-main ownership trail was available in the supplied review evidence. (role: OAuth change contributor; confidence: medium; commits: e3025248f377, a9f338cccfb4, 99254685f7d9; files: Core/PeekabooCore/Sources/PeekabooAutomation/Configuration/ConfigurationManager+Accessors.swift, Core/PeekabooCore/Sources/PeekabooAutomation/Configuration/ConfigurationManager+Credentials.swift, Tachikoma)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Repin Tachikoma to the final upstream merge commit after the dependency lands.
  • Rerun and post the focused OAuth availability and credential-boundary validation against that final pin.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (6 earlier review cycles)
  • reviewed 2026-07-27T11:34:03.441Z sha a9f338c :: found issues before merge. :: [P1] Keep environment OAuth credentials out of the credentials file | [P1] Pin Tachikoma only after its OAuth change merges upstream
  • reviewed 2026-07-27T11:54:23.315Z sha 9925468 :: needs changes before merge. :: [P1] Pin Tachikoma only after its OAuth change merges upstream
  • reviewed 2026-07-27T12:01:27.758Z sha 9925468 :: found issues before merge. :: [P1] Repin Tachikoma after its OAuth change merges upstream
  • reviewed 2026-07-29T20:23:31.354Z sha 9925468 :: found issues before merge. :: [P1] Repin Tachikoma after its OAuth change merges upstream
  • reviewed 2026-07-29T21:46:07.917Z sha 9925468 :: found issues before merge. :: [P1] Repin Tachikoma to its final merged OAuth revision
  • reviewed 2026-07-29T22:18:27.471Z sha 9925468 :: found issues before merge. :: [P1] Repin Tachikoma to its final merged OAuth revision

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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant