Skip to content

feat(cli): inline --oauth for storyboard run + actionable auth hint#766

Merged
bokelley merged 5 commits into
mainfrom
bokelley/storyboard-oauth-debug
Apr 22, 2026
Merged

feat(cli): inline --oauth for storyboard run + actionable auth hint#766
bokelley merged 5 commits into
mainfrom
bokelley/storyboard-oauth-debug

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: storyboard run against an OAuth-protected agent reported Agent unreachable — requires OAuth authorization with no actionable hint. detectAuthRejection's keyword list missed NeedsAuthorizationError's "requires OAuth authorization" phrasing, so the classification fell through to the generic unreachable path and the --save-auth --oauth remediation observation never fired.
  • Fix classification: keyword list now matches OAuth-specific bigrams (oauth authorization, requires oauth, oauthflowhandler, www-authenticate, bearer realm, etc.) instead of the bare substring "authorization" — that was also a security/DX false-match on benign errors like "authorization header missing from upstream" or "failed to connect to oauth proxy". The remediation observation now fires whenever OAuth phrasing is present OR metadata is discoverable.
  • Inline --oauth on storyboard run and storyboard step: opens the browser PKCE flow when the saved alias lacks valid tokens, persists them back, then continues the run. Matches the existing top-level --oauth contract. Raw URL + --oauth stays a warning in human mode and becomes a structured oauth_requires_alias error (exit 2) under --json so CI pipelines fail fast.
  • Shared helper hardening: ensureOAuthTokensForAlias wraps cleanup in try/finally, persists silently-refreshed tokens on the happy path, and carries oauth_code_verifier so a crashed mid-flight PKCE attempt survives.
  • Docs: docs/CLI.md and docs/guides/VALIDATE-YOUR-AGENT.md document both flows (pre-save + inline) and the CI path (save tokens once locally, ship ~/.adcp/agents.json to the runner, drop --oauth).

Test plan

  • Unit: detectAuthRejection classifies NeedsAuthorizationError-style messages as auth, emits the remediation hint even when OAuth metadata isn't discoverable, doesn't mis-classify plain bearer 401s, and — regression guard — doesn't mis-classify benign "authorization header" upstream errors.
  • CLI: storyboard run --oauth with a raw URL prints the save-first warning in human mode and emits the structured oauth_requires_alias JSON error + exit 2 under --json.
  • E2E: cleared stale tokens on the test-agent alias → ran adcp storyboard run test-agent --oauth --storyboards security_baseline → browser opened → completed auth at agenticadvertising.org → tokens persisted → storyboard ran with Auth: oauth (auto-refresh). The new remediation wording renders with fenced issuer.
  • Full suite green: 2321/2325 pass across compliance + storyboard + oauth + cli test files (4 skipped, 0 fail).
  • Prettier clean; no new lint errors; pre-push typecheck passed.

🤖 Generated with Claude Code

bokelley and others added 3 commits April 22, 2026 00:03
When capability discovery fails with an OAuth challenge, `detectAuthRejection`
now classifies via OAuth-specific bigrams (not the bare substring "authorization",
which false-matched benign "authorization header" upstream errors), and the
remediation observation fires whenever the error phrasing signals OAuth even if
metadata discovery can't resolve the well-known chain.

`adcp storyboard run <alias> --oauth` and `storyboard step <alias> --oauth` now
run the browser PKCE flow inline when the saved alias lacks valid tokens and
persist them back to the alias — matching the existing `--oauth` contract on the
top-level command. Raw URL + `--oauth` stays a warning in human mode and becomes
a structured `oauth_requires_alias` error (exit 2) under `--json` so CI pipelines
fail fast instead of hanging.

The shared helper wraps MCP client/transport/provider cleanup in try/finally,
persists silently-refreshed tokens on the happy path, and carries
`oauth_code_verifier` into the temp agent so a crashed mid-flight PKCE attempt
survives.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up asset_type discriminators on inline asset interfaces (image, video,
audio, vast, daast, text, markdown, url, html, css, webhook, javascript, brief,
catalog) and the "no separate account field" clarifications on plan-scoped
governance requests. Pure regeneration — no handwritten source touched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

CI status note — Test & Build and CodeQL failures on this PR aren't introduced by the OAuth work; they reflect upstream state on main:

  1. Test & Build — main's most recent commits (f64007c1, 8f9260bf) also fail the Validate generated files are in sync step. I tried regenerating (npm run sync-schemas && npm run generate-types && npm run generate-wellknown-schemas) in commit e38e526b, which picked up the new asset_type discriminator on inline assets, but that surfaced 4 downstream fixture test failures in test/lib/conformance-seeder.test.js, test/lib/request-validation.test.js, and test/lib/zod-schemas.test.js — fixtures that don't yet include asset_type. Reverted in d23e2b38 so this PR matches main's failure mode rather than adding new ones. The schema-fixture migration is a separate workstream from the OAuth fix.

  2. CodeQL alert fix: improve --save-auth UX with better error messages and auth handling #83 (bin/adcp-json-stdout.js:14, js/clear-text-logging) is pre-existing on main and open against the refs/heads/main ref. My PR added new taint sources (oauthProvider references in the new ensureOAuthTokensForAlias helper), which CodeQL aggregates into the existing alert. No actual token/credential is logged — the sink is captureStdoutLogs, a pure stdout→stderr forwarder used to keep --json output uncorrupted by library-emitted console.log strings. Same alert will remain open after merge until the console forwarding is refactored (separate fix).

Happy to rebase once the schema-fixture migration lands on main.

bokelley and others added 2 commits April 22, 2026 00:16
…xtures

The regenerated AdCP schemas require an asset_type discriminator on every
inline asset (image/video/audio/text/url/html/javascript/css/markdown etc).
Four downstream sites got caught:

- `src/lib/conformance/seeder.ts` ASSET_PLACEHOLDER entries — the seeder
  now emits matching `asset_type` on every synthesized placeholder.
- `test/lib/conformance-seeder.test.js` assertion updated to expect the
  discriminator on the synthesized text asset.
- `test/lib/request-validation.test.js` two sync_creatives fixtures now
  include `asset_type: 'video'`.
- `test/lib/zod-schemas.test.js` GetCreativeFeaturesRequest fixture now
  includes `asset_type: 'image'` + required dimensions.

Pure catch-up work against the latest schemas; no client/runtime changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

Update: Test & Build now passes after adding asset_type discriminators to the conformance seeder (src/lib/conformance/seeder.ts) and four test fixtures. Full suite: 5155 pass / 0 fail.

All required checks green. The standalone CodeQL check at the bottom still shows fail — that's the pre-existing alert #83 on bin/adcp-json-stdout.js:14 (same alert, same line, created 2026-04-21 before this PR, also open against refs/heads/main). My OAuth work added new taint sources that CodeQL aggregates into the existing alert, but no actual token/credential is logged — the sink is a pure stdout→stderr forwarder for keeping --json output uncorrupted. Refactoring that forwarder is out of scope for this PR.

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.

1 participant