Skip to content

Fix MCP OAuth sign-in deadlock, add Google option - #2

Merged
serge-ivo merged 1 commit into
mainfrom
fix/mcp-oauth-signin
Jun 23, 2026
Merged

Fix MCP OAuth sign-in deadlock, add Google option#2
serge-ivo merged 1 commit into
mainfrom
fix/mcp-oauth-signin

Conversation

@serge-ivo

Copy link
Copy Markdown
Contributor

Problem

Browser sign-in to the ProAgentStore MCP server (mcp.proagentstore.online) failed repeatedly with "invalid or expired nonce" and "sign-in is already in progress in another tab", never showing a usable sign-in screen.

Root causes

  1. In-flight cookie deadlock. /authorize set a pags_mcp_oauth_inflight cookie (Max-Age 120s) and short-circuited any subsequent /authorize to a dead-end "already in progress" page. The cookie was only cleared on a successful callback — so any failed/abandoned flow (expired nonce, wrong tab) blocked all retries in the same browser for two minutes. MCP clients retry fast, so this was hit constantly. PKCE + per-request nonce already isolate flows, so the cookie provided no security; removed.

  2. GitHub-only sign-in. The consent page hardcoded a single "Continue with GitHub" button — no Google option, unlike the rest of the platform.

Changes (workers/mcp/src/oauth-provider.ts)

  • Removed the in-flight cookie mechanism (authAlreadyInProgress, cookieValue, set/clear headers, the block in authorize).
  • Consent page now offers both GitHub and Google buttons.
  • /authorize/continue takes a provider param and routes to /v1/auth/{github,google}/start; defaults to GitHub. Both FAS endpoints already accept identical app_id/return_to/response_mode and return fas_session the same way.

No FAS-side change needed — mcp.proagentstore.online is already on the return_to allowlist (isAllowedHost) for both providers.

Tests

workers/mcp/src/oauth-provider.test.ts updated: asserts no cookie, both provider buttons render, and github/google/default routing. 11 tests pass; tsc --noEmit clean.

🤖 Generated with Claude Code

Two issues made the MCP browser sign-in fail repeatedly:

1. In-flight cookie deadlock. /authorize set a `pags_mcp_oauth_inflight`
   cookie (Max-Age 120s) and short-circuited any new /authorize to a
   dead-end "already in progress in another tab" page. The cookie was
   only cleared on a *successful* callback, so any failed or abandoned
   flow blocked all retries in the same browser for two minutes and
   never showed a sign-in screen. PKCE + per-request nonce already
   isolate flows, so the cookie added no security — removed it entirely.

2. GitHub-only sign-in. The consent page hardcoded a single "Continue
   with GitHub" button. Now it offers both GitHub and Google, matching
   the rest of the platform. /authorize/continue takes a `provider`
   param and routes to the matching FAS start endpoint
   (/v1/auth/{github,google}/start); both already accept the same
   app_id/return_to/response_mode and return fas_session identically.
   Defaults to GitHub when provider is omitted.

The MCP callback host (mcp.proagentstore.online) is already on the FAS
return_to allowlist for both providers, so no FAS-side change is needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@serge-ivo
serge-ivo merged commit 0ff0d00 into main Jun 23, 2026
1 check passed
@serge-ivo
serge-ivo deleted the fix/mcp-oauth-signin branch June 23, 2026 06:34
serge-ivo pushed a commit that referenced this pull request Jun 25, 2026
…honesty

#1 Deterministic & honest apply: a Board 'Apply to a job' panel posts to /apply and
shows the REAL returned task id (or the real error) — no chatbot that can claim
success without a task. resume_path is now optional (uses the uploaded résumé); the
apply tool's prompt forbids inventing task ids / claiming success on error.
#2 Setup checklist: Runner · Résumé · Profile status above the board, gating the
Apply button on runner+résumé so you can't start a doomed application.
serge-ivo pushed a commit that referenced this pull request Aug 2, 2026
…an't back (#66)

lintAgentClaims(description, capabilities): heuristic + overridable lint that
flags catalog copy promising a runtime capability (browser / posting /
headless / local runner / cron / scheduled) when capabilities declare no
runtime AND no workflow. Wired into POST /agents as a non-blocking 'warnings'
field so publishing overstated copy is loud. +5 tests incl. a deliberately-
mismatched fixture (Creator OS shape).

Note: fixing the LIVE mismatched descriptions (Creator OS, QA Automation) is a
prod-data change via MCP update_agent, not code — tracked separately. This
delivers the lint (acceptance #2).

Closes #66. tsc + tests green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
serge-ivo pushed a commit that referenced this pull request Aug 13, 2026
…spending the subrequest ceiling it was never counting (#523)

Three runs died at 120, 122 and 122 minutes with `Too many API requests by
single Worker invocation` and were reported to their owner as `outcome: failed`
beside a Wrangler docs link. One had closed ten issues and pushed to `main`
fifteen times. #546 already fixed the SENTENCE — `platform_ceiling` classifies
and reports as an interruption rather than as the objective failing, and this
change deliberately adds no second word for that idea. What was left is the
cause underneath it.

ONE DOCUMENTED FACT DECIDES THE DESIGN, AND IT IS NOT THE ONE THE ISSUE ASSUMED.
Cloudflare applies this ceiling PER WORKFLOW INSTANCE, not per invocation:
"Maximum number of subrequests per Workflow instance — 10,000/request (default),
configurable up to 10 million" (workflows/reference/limits, read 2026-08-13).
So neither a `step.do` boundary nor a `step.sleep` resets it, and the issue's
fix #2 — break `waitIdle` into chunks separated by `step.sleep` — would have
bought NOTHING while adding a durable step per chunk. It is not implemented, and
this is why.

THE ARITHMETIC IS THE FINDING. `capture()` costs four subrequests, counted from
the source rather than estimated: one `RELAY` DO `stub.fetch`
(`runner-client.ts:267`), `SELECT status FROM coding_sessions`,
`isCancelRequested`, and `touchSessionActivity` — whose 60-second throttle is a
WHERE clause, so the subrequest was spent on every poll to move a row one time
in thirty. At a flat 2-second poll that is 2 subrequests per second of Engine
work, so 10,000 buys 83 minutes of waiting. Modelled over the worst run's own
measured shape (26 Pilot steps across 122 minutes = 4.7 minutes of Engine per
step): 26 × 142 polls × 4 = 14,768 against a 10,000 ceiling. It is not the step
count — 26 steps and 37 steps both died, and the 99-minute survivor ran 34.

THE POLL BACKS OFF, WHICH THE ISSUE REJECTED ON TWO GROUNDS THAT ARE BOTH FALSE
OF THIS POLL. "2s is what makes the terminal feel live" — it is not; the live
terminal is the CONSOLE's own 3-second `GET …/sessions/:sid/capture` on its own
rate-limit bucket, and this poll runs inside a durable Workflow with no browser
attached. "touchSessionActivity is throttled against it" — it is throttled
against 60 seconds (`ACTIVITY_TOUCH_MS`), which any interval under a minute
satisfies identically. 2s for the first 30 seconds where most turns finish, then
5s, then 10s past two minutes. Same run: 26 × 51 × 3 = 3,978, which fits under
the ceiling that killed it with 60% to spare. The price is up to 10 seconds of
extra latency at the end of a turn already measured in minutes — 3.5% on a
2-hour run, against a run that used to not finish at all.

THE BOUNDARY IS EXACTLY WHERE IT WAS, which is the regression the issue names.
The old bound was a poll COUNT (240 × 2s) sized against `idleRetry`'s 10-minute
`step.do` timeout. Carried over to a backed-off schedule it would have stretched
the window to 40 minutes and moved the failure from "the wait gave up" to "the
durable step timed out", which reads as a crash. `IDLE_WAIT_MAX_MS` is elapsed
SLEEPING time, asserted equal to `240 * 2_000`.

THE CEILING IS ALSO RAISED, THIRD AND NOT INSTEAD. `[limits] subrequests` is
real — wrangler 4.98's own config schema carries it, and `wrangler deploy
--dry-run` accepts the block with no warning — so the docs link the owner was
shown was actionable advice pointed at the wrong person. 100,000 is 1% of the
documented maximum: ~25 hours of Engine waiting at the new poll cost, still small
enough to stop a genuine runaway. Raising it ALONE would only have let the next
run of this shape go further unsupervised before it died, which is the issue's
own argument and it stands.

Extracted to `lib/coding-idle-poll.ts` rather than edited in place: "how long
may a run wait, and what does waiting cost" is a rule, and a rule inside a
Workflow can only be tested by running one — the same reason `coding-pause.ts`
exists. `workflows/coding-session.ts` is 796 after, one line above the 795 it
started at and under its 800 ratchet; the pin is unchanged.

21 tests, stating the sizes they measured (ADR 0002): the 14,768-against-10,000
reproduction, the 3,978 it becomes, 51 polls against 142 for one turn, 70 against
241 for the full window, and the survivor's shape held inside the ceiling. Every
assertion proven by watching it fail (G4): flat 2s poll → 7 red; the 240-poll
count carried onto the new schedule → 3 red; no activity throttle → 1 red; no
settle sleep → 2 red; ignoring `cancelled`/`alive` in the wait → 2 red.

Not done here, and left open on the issue rather than silently dropped: the
issue's item 4, checkpointing so a cut-off run resumes from issue 9 rather than
from the objective. The Pilot's progress lives in the run's in-memory
`actionLog`/`transcript` and making it durable is its own change. With the spend
now 3.7× lower and the ceiling 10× higher the run this was filed for no longer
reaches either, so the clean pre-emptive stop the comment asked for is a backstop
for a case that should no longer arrive — worth having, not worth folding in
here unmeasured.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serge-ivo pushed a commit that referenced this pull request Aug 29, 2026
…closed (#760)

A coding session on a managed-clone repo record that carries no `github_repo`
(GitHub coordinates absent) and no `workdir` (not a local-path checkout) was
allowed to start with `ghScope: undefined`. The runner treated absent scope as
"older cloud / not said" and installed NO gh guard — the Engine then ran with
unrestricted `gh` write access to any repository it could reach from the machine.

`gh-guard.ts` comment: "No scope is not 'allow nothing' — it is 'the platform
did not say'." That reasoning is correct for a legacy cloud, but wrong for a
record that is inherently incapable of producing a scope (stuck in `cloning`
with coordinates that were never resolved).

Fix: in `startSessionOnRunner`, after the connection check and before `callRunner`,
refuse the session when all three conditions hold:
  1. `repo.provider === "github"` — a `gh`-using remote
  2. `!repo.githubRepo`          — no coordinates to scope the guard with
  3. `!repo.workdir`             — not a local-path repo (which is exempt)

The concrete case: a duplicate `ProAgentStore/platform` record stuck in `cloning`
had a session attached to it. `coding_diagnostics` showed "1 of 2 tracked sessions
guarded"; the unguarded one was running on this broken record. Fixes ask #1 from
the issue.

Asks #2#4 (surface unguarded count as error, detect duplicate records, remove the
stuck data row) are owner/data-cleanup actions, not code changes.

Closes #760 (code guard half only — the stuck record `repo_2b2657fb` on the
operator account requires separate data cleanup).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

2 participants