fix(clone): recover from GitHub organization SAML auth

[piotrchabros]

Summary

• render GitHub organization SAML failures as a bounded authorization flow instead of an overflowing raw token
• retry the failed checkout once the user returns from browser authorization
• force the validated GitHub HTTPS URL so a global gh SSH preference cannot select an SSH key lacking the OAuth token's organization grant

Regression evidence

Reproduced on a SAML-enforced repository: the refreshed GitHub CLI OAuth token had MAINTAIN access over HTTPS while the configured SSH key remained rejected by the organization. Passing owner/repo to gh repo clone selected the rejected SSH transport; passing the validated HTTPS URL cloned successfully.

Verification

• NODE_ENV=test npx vitest run packages/cezar/src/server/checkout.test.ts (24 tests)
• npm run typecheck -w @open-mercato/cezar
• full web suite (151 files, 3367 tests)
• web typecheck and production build
• real checkout of Bug-Bounty-Switzerland/bbs-frontend succeeded and registered on develop

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-12T17:17:53Z. Other auto-skills will skip this PR until the lock is released.

[pat-lewczuk]

🔍 Code Review: fix(clone): recover from GitHub organization SAML auth

Verdict

✅ approve — the fix is correct on both halves and I reproduced its central premise locally. Only minors and nits below; none of them block the merge. needs-qa stays on: this is a cockpit dialog change whose retry path deserves one manual pass in a real browser.

🎯 Summary

A user cloning from a SAML-enforced GitHub organization got an unrecoverable dialog: an overflowing raw gh error and no way to act on it. This PR makes that failure a bounded authorization flow — a narrowly-matched github.com/orgs/…/sso link, an automatic retry when the user comes back to the tab, and a scrollable dialog — and, server-side, pins the clone to the validated HTTPS URL so a global gh SSH preference cannot select a key the organization rejects.

Verification of the central claim

I did not take the transport claim on trust; I reproduced it against gh 2.96.0:

• With git_protocol: ssh set for github.com (in a throwaway GH_CONFIG_DIR), gh repo clone octocat/Hello-World selects SSH and fails; gh repo clone https://github.com/octocat/Hello-World.git clones over HTTPS. That is exactly the bug ghCloneArgs fixes, and passing owner/repo really does silently pick the rejected transport.
• Forcing HTTPS does not cost private-repo access: GH_DEBUG=1 gh repo clone <https-url> shows gh injecting its own credential helper — git -c credential.https://github.com.helper=!"gh" auth git-credential clone … — so the clone authenticates with the OAuth token, which is precisely the credential that carries the SAML grant. No SSH-user regression.
• strings $(command -v gh) confirms gh emits Authorize in your web browser: %s, so githubSsoUrl has a real source line to match rather than a hypothesised one.

I also spot-checked the regex against realistic gh/git SAML output: it matches both the gh Authorize in your web browser: line and the git ERROR: The 'Org' organization has enabled or enforced SAML SSO … visit … line, and correctly rejects https://evil.example/authorize and the https://github.com.evil.example/orgs/x/sso?… lookalike. The narrow-by-design matcher and its accompanying phishing test are the right call.

Findings

Minor

• packages/web/src/components/clone-project-dialog.tsx:52 — the module docstring still states that errors are shown verbatim and that "paraphrasing them into 'could not clone' is exactly the silent-spinner failure this dialog exists to avoid". The new SSO branch replaces the server's message entirely; the raw gh text — the only thing a user can paste into a bug report — is now unrecoverable from the UI. Either keep it visible (a second muted line, or a <details> under the link) or record the deliberate exception in the docstring, so the comment and the code stop contradicting each other. Per CODE_REVIEW.md ("Comments cite the spec or issue that motivated the code"), the stale half is the part that matters here.

• packages/web/src/components/clone-project-dialog.tsx:131 — the visibilitychange retry path is untested. The new test drives only fireEvent.focus(window), so neither retryWhenVisible's visibilityState === 'visible' guard nor the double-fire ssoRetryArmed guard that the comment specifically advertises ("The ref prevents browsers that emit both events from cloning twice") is exercised. A case that hides, then shows, then also fires focus, asserting exactly one additional POST, would pin the behaviour the comment promises.

• packages/web/src/components/clone-project-dialog.tsx:251 — "waiting for you to return; cezar will retry automatically" is a promise the code cannot always keep. A ctrl/cmd-click still fires React's onClick, so awaitingSso latches and the copy switches — but the tab opens in the background, the window never blurs and visibility never changes, so neither listener ever fires. The user is left staring at an automatic retry that will not happen; only the manual "Retry clone" button rescues it. Consider arming on the first blur/hide rather than on the click, or softening the copy to keep mentioning the button.

Nit

• packages/web/src/components/clone-project-dialog.tsx:108 — the useCallback around clone never actually memoizes. TanStack Query v5 (5.101.2, useMutation.js:40) returns { ...result, mutate, mutateAsync } — a fresh object on every render — so checkout changes identity every render, clone with it, and the awaitingSso effect tears down and re-registers both listeners on every render. Behaviourally harmless, but it advertises a stability it does not have; depending on checkout.mutate / checkout.isPending instead of the whole object would make it real.

• packages/cezar/src/server/checkout.test.ts:94 — the ghCloneArgs case lives inside describe('checkout — repo reference parsing'), but it asserts clone-argument construction, not parsing. Its own describe would keep the suite's map honest.

• packages/web/src/components/clone-project-dialog.tsx:30 — githubSsoUrl is exported, but nothing imports it (searched packages/**/*.{ts,tsx} excluding node_modules at 44186c69; the only reference is line 72 in the same file). Either drop the export or have the test import it directly and cover the matcher cases at the unit level.

🧪 Validation Gate

Command	Status	Evidence or limitation
npm run typecheck	PASS	exit 0 — contract, client, server and web projects all clean
npm test	PASS	332/332 files, 6395/6395 tests on a clean run (see the flake note below)
npm run test:unit	PASS	36/36 node:test cases, 0 failures
npm run build	PASS	server + web built; check:pack ok — 481 files, 85 under web/dist (shell + assets present)
npm run test:package	PASS	16/16 — tarball packed, installed, built CLI exercised

All five gates were executed at 44186c69, in order. Two environment caveats, neither a finding against this PR:

• TMPDIR artifact. This sandbox sets TMPDIR inside the repository, so the first run failed 8 tests across 8 files that create a temp dir and assert it is not a git repo (e.g. projects-api.test.ts:753, expect(body.repo).toBeNull() // tmp dir — not a git repo, which instead resolved up to the real checkout). Re-running with TMPDIR=/tmp is what the table reports.
• One nondeterministic flake, not attributable to this diff. A single run hit an unhandled rejection in packages/cezar/src/workflows/run.test.ts — RunManager.rescueStalledQueue → RunStore.appendEvent writing an .ndjson after the temp dir was torn down. It passes in isolation (94/94) and the very next full-suite run was clean (6395/6395), so it is a teardown race under parallelism. The diff touches nothing in runs/ or workflows/, so there is no causal path from this change. Flagging it as a pre-existing flake worth a separate issue rather than holding this PR.

⚠️ CI has not run on this PR

Worth a maintainer's attention before merge: the repository's own CI workflow is at completed/action_required for head 44186c69 — the standard GitHub hold on a fork PR, waiting for a maintainer to click Approve and run workflows. gh pr checks 968 therefore reports only license/cla (SUCCESS), and .github/workflows/ci.yml (typecheck → unit → suites → build → E2E → package) has never executed against this branch.

main is not branch-protected (the required-status-checks API answers 404), so nothing mechanically blocks a merge on that. The gate table above is this review's own local evidence and is not a substitute for the repo's CI — please approve the workflow run before merging. mergeable is MERGEABLE; no conflicts against main.

💥 Breaking Changes

None. RepoRef gains a required cloneUrl, which would be breaking for an external constructor — but RepoRef is not on the package's export surface (@open-mercato/cezar exports only . → dist/index.js and ./app-type), and the only construction site in the repo is parseRepoRef itself, which always populates it. slug is retained for messages and dry-run output, so nothing that read it changes. No route, event name, persisted field or CLI flag is touched, so nothing in BACKWARD_COMPATIBILITY.md applies. CEZ_DRY_RUN=1 still works — dryRunCloneRunner never consults cloneUrl.

🧪 Test Coverage

Good, and proportionate to the change. The server side pins the SAML-safe transport without spawning gh (ghCloneArgs kept pure — the right seam), and parseRepoRef's existing table-driven case was extended to assert cloneUrl for every accepted URL spelling, including the git@github.com: form that motivated the fix. The web side covers both the happy SSO path (compact copy, link target/rel, no leaked authorization_request=, "Retry clone", retry-on-return, dialog close) and the phishing negative. The one gap worth filling is the visibilitychange branch, filed as a minor above.

[pat-lewczuk]

🤖 om-auto-review-pr — 🏷️ label rationale

• ✅ merge-queue — the re-review at 90738b47 approves: all eight findings from the previous pass are fixed, the major one verifiably so (the persisted !gh auth git-credential helper was exercised, not just read), and the full local gate plus repository CI are green.
• 🧪 needs-qa — retained: the SAML link, the collapsible Error details, the auto-retry on return and the manual Retry clone fallback are cockpit UI behaviours a human has to click through in a real browser; qaGate is on, so this holds the merge until a QA reviewer adds qa-approved.
• 🐛 bug — unchanged: this repairs a broken flow (cloning from a SAML-enforced organization was unrecoverable from the dialog), it does not add capability.
• 🔹 priority-medium — unchanged: an ordinary bug fix per SDLC.md. It unblocks a real class of users (anyone in a SAML-enforced org) but is neither a release-blocking regression nor a security fix.
• 🟡 risk-medium — unchanged: the blast radius is wider than the dialog — every clone now gets an HTTPS origin plus a repo-local credential helper, and raw git push runs with prompts disabled — but the change ships tests for each of those seams and the full gate is green.

[pat-lewczuk]

🧪 Manual QA instructions (needs-qa)

Exercise the GitHub clone dialog's SAML recovery path, the forced-HTTPS transport, and — new at 90738b47 — that a repo cloned this way can still push. Review: #968 (review) (approved at 90738b47; local gate and repository CI both green).

P0-a needs a real SAML-enforced organization. If you have no access to one, say so and run the rest — the SSO branch can still be driven end to end by stubbing the server error (P1-a), which covers the UI contract but not the real gh transport.

Priority	Setup and action	Expected result / boundary
P0-a	On a machine with gh config set -h github.com git_protocol ssh and an SSH key not authorized for a SAML-enforced org, clone a repo from that org via Add project → Clone from GitHub. Follow the Authorize this GitHub organization link, approve in the browser, return to the cockpit tab.	The first attempt fails with the SAML panel (not a wall of raw gh output); on return the clone retries by itself and the project appears in the sidebar. Boundary: the error panel must stay inside the dialog — no horizontal overflow, dialog scrolls vertically.
P0-b	The regression this revision adds. Take any project cloned by the dialog (P0-a's, or any repo — the transport is forced to HTTPS for everyone now). Confirm git -C <project> config --local --get-all 'credential.https://github.com.helper' prints an empty line followed by !gh auth git-credential. Then start a task on it, let it commit, and use the cockpit's Push.	The push completes without a credential prompt and without stalling. Boundary: it must not hang — if it sits for ~60s and then reports a timeout, the persisted helper is not being picked up by the task worktree.
P0-c	With a project cloned as in P0-b, run gh auth logout, then push again from the cockpit, then gh auth login and retry.	The push fails with a legible one-line error reasonably quickly, and succeeds again after re-login. Boundary: a push that never returns at all is the failure being watched for here.
P1-a	Force a SAML failure (real org, or stub the checkout route's error with a message containing https://github.com/orgs/<org>/sso?authorization_request=<token>). Inspect the error panel.	Link text reads "Authorize this GitHub organization", opens in a new tab, and an Error details disclosure is present and collapsed. Expanding it shows the complete original gh message.
P1-b	From the P1-a panel, ctrl/cmd-click the link (background tab — the window never blurs), then press Retry clone.	The copy reads "…or choose Retry clone" — it must not promise an automatic retry that cannot happen — and the button triggers exactly one new clone attempt.
P1-c	From the P1-a panel, click the link normally, switch to another tab/window, then switch back.	Exactly one additional clone attempt fires on return, even in a browser that emits both focus and visibilitychange. Boundary: two POSTs is a failure.
P1-d	Trigger a non-SAML clone failure: clone owner/does-not-exist-xyz, and separately a repo whose target folder already exists.	The raw server message is shown verbatim with no SSO link and no Error details wrapper — the SAML panel must not capture ordinary failures.
P1-e	Feed the dialog a malformed SAML URL — a token containing %2F or trailing == (stub the error), and separately a phishing shape such as https://github.com.evil.example/orgs/x/sso?authorization_request=abc.	No link is rendered in any of these cases; the raw error is shown instead. A rendered link to anything other than https://github.com/orgs/… is a hard fail.
P2	Cold-load the cockpit, open the clone dialog at a narrow width (~380px) and in both light and dark themes, with a long error expanded under Error details.	Long tokens wrap rather than overflow, the dialog scrolls, the disclosure is readable and keyboard-reachable in both themes.

QA reviewer: move merge-queue → qa when starting. All checks pass → replace qa with merge-queue and qa-approved. A failure → replace qa with qa-failed and report the failed action, expected result, and observation. qaGate is on in this repository, so QA approval still gates the merge.

[pat-lewczuk]

🤖 om-auto-review-pr completed: APPROVED. Lock released.

No blockers or majors. Three minors and three nits are listed in the review for the author to pick up — none of them gate the merge. The full local validation gate (typecheck, npm test, test:unit, build, test:package) passed at 44186c69; note that the repository's own CI is still at action_required on this fork PR and needs a maintainer to approve the workflow run before merge.

Labels: merge-queue + needs-qa — with qaGate on, the QA-approval gate holds the merge until a QA reviewer adds qa-approved. Manual QA instructions are posted above.

autofix: skipped (not my PR — re-run with --autofix to fix it here)

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-14T10:26:56Z. Other auto-skills will skip this PR until the lock is released.

[pat-lewczuk]

🔁 Re-review: fix(clone): recover from GitHub organization SAML auth

Verdict

❌ request changes — one major finding that the previous pass missed. No new commits since the approved review at 44186c69; this re-review re-ran the full gate and went one step further down the transport change, and that step is what turned the verdict.

The delta in one line: forcing the clone onto HTTPS fixes the clone, but gh's credential injection is one-shot — the checkout it leaves behind has a bare HTTPS origin and no credential path, so cezar's own git push can no longer authenticate for exactly the users this PR targets.

Good news alongside it: CI is now green. The last pass reported .github/workflows/ci.yml stuck at action_required on this fork PR; a maintainer has since approved it and Unit, build, E2E, and package passed in 4m36s.

Disposition of the earlier findings

No commits landed since the previous review, so all six earlier findings still stand unchanged and are restated below. Nothing was regressed, nothing was fixed.

🎯 Summary

A user cloning from a SAML-enforced GitHub organization got an unrecoverable dialog: an overflowing raw gh error and no way to act on it. This PR makes that failure a bounded authorization flow — a narrowly-matched github.com/orgs/…/sso link, an automatic retry when the user returns to the tab, a scrollable dialog — and server-side pins the clone to the validated HTTPS URL so a global gh SSH preference cannot select a key the organization rejects.

Both halves are well-built. The UI half is careful about the things that matter (see the security section — I tried to break the matcher and could not). The server half is correct about the clone and incomplete about what happens after it.

Findings

Major

• packages/cezar/src/server/checkout.ts:110 (and :185) — forcing HTTPS fixes the clone but strands every later git operation on the resulting checkout.

  cloneUrl now overrides the user's configured transport for every clone, not just SAML-failing ones. gh makes that clone work by injecting a credential helper for the clone command only — it does not persist. I verified this against gh 2.100.0:

  $ gh repo clone https://github.com/octocat/Hello-World.git hw-https
  $ git -C hw-https config --local --get-regexp credential
  (none in local repo config)
  $ cat hw-https/.git/config
  [remote "origin"]
      url = https://github.com/octocat/Hello-World.git
  

  So the checkout keeps a bare HTTPS origin, and every subsequent git operation falls back on a global credential.https://github.com.helper. A user who ran gh auth login and chose SSH never gets that helper installed — and gh config set -h github.com git_protocol ssh is precisely the configuration this PR's own regression evidence describes. With no helper:

  $ git -c credential.helper= -c "credential.https://github.com.helper=" \
        ls-remote https://github.com/<private>.git HEAD
  fatal: could not read Username for 'https://github.com': terminal prompts disabled
  

  That matters because cezar pushes task branches with raw git, not gh — packages/cezar/src/server/forge/github.ts:2471:

  const push = await execTool(['push', '-u', 'origin', branch], worktree, 'git', PUSH_TIMEOUT_MS);

  and execTool (github.ts:2545) inherits the ambient environment without GIT_TERMINAL_PROMPT=0 — unlike ghCloneRunner, which sets it deliberately on line 206 of the file this PR edits. So the failure mode is not a fast, legible error: the push blocks on a credential prompt until PUSH_TIMEOUT_MS (60s) and then surfaces as a generic timeout. Task worktrees share the parent repo's config, so every task on that project inherits it.

  Net effect for an SSH-protocol user: before this PR they cloned over SSH and pushed with their key; after it the clone succeeds and the first Create PR hangs a minute and fails. The SAML user the PR is written for is in this population too — the fix gets them a working clone and leaves them unable to push from it.

  Concrete fix, cheapest first:

  1. Persist what the clone borrowed — after a successful clone, git -C <target> config credential.https://github.com.helper '!gh auth git-credential' (this is exactly what gh auth setup-git writes globally for HTTPS users), or
  2. narrow the blast radius — keep ref.slug as the default and fall back to cloneUrl only after a first attempt fails with a SAML-shaped error, so users whose transport already works are untouched.

  Either way, a test in checkout.test.ts asserting the post-clone remote/credential state would pin it.

Minor

• packages/web/src/components/clone-project-dialog.tsx:30 — a truncated match is indistinguishable from a good one, and the raw URL is gone. The token charset is [A-Za-z0-9._~-]+, and the match is unanchored, so a value containing anything else is silently cut short rather than rejected. Probing the actual regex:

  input	result
  …/sso?authorization_request=ABCDEFGH123456==	matches …=ABCDEFGH123456 — padding dropped
  …/sso?authorization_request=ABC%2FDEF	matches …=ABC — badly truncated

  Because a non-null ssoUrl makes the dialog replace the server message entirely, the user gets a link that dead-ends on GitHub and no way to see the real URL to open by hand. Anchor the match (require the token to run to a non-URL boundary) or, better, keep the raw text reachable — which also fixes the docstring finding below. The variants with no URL at all (the gh GraphQL message without an Authorize line, the SSH-flavoured SAML error pointing at docs.github.com) correctly fall through to the raw error, which is the right degradation.

• packages/web/src/components/clone-project-dialog.tsx:52 — the module docstring now contradicts the code. It still states errors are shown verbatim and that "paraphrasing them into 'could not clone' is exactly the silent-spinner failure this dialog exists to avoid". The SSO branch replaces the server's message entirely; the raw gh text — the only thing a user can paste into a bug report — is unrecoverable from the UI. Either keep it visible (a second muted line, or a <details> under the link) or record the deliberate exception in the docstring. Per CODE_REVIEW.md ("Comments cite the spec or issue that motivated the code"), the stale half is the part that matters.

• packages/web/src/components/clone-project-dialog.tsx:131 — the visibilitychange retry path is untested. The new test drives only fireEvent.focus(window), so neither retryWhenVisible's visibilityState === 'visible' guard nor the double-fire ssoRetryArmed guard the comment specifically advertises ("The ref prevents browsers that emit both events from cloning twice") is exercised. A case that hides, then shows, then also fires focus, asserting exactly one additional POST, would pin the behaviour the comment promises.

• packages/web/src/components/clone-project-dialog.tsx:251 — "cezar will retry automatically" is a promise the code cannot always keep. A ctrl/cmd-click still fires React's onClick, so awaitingSso latches and the copy switches — but the tab opens in the background, the window never blurs and visibility never changes, so neither listener fires. The user waits for a retry that will not happen; only the manual "Retry clone" button rescues it. Arm on the first blur/hide rather than on the click, or soften the copy to keep mentioning the button.

Nit

• packages/web/src/components/clone-project-dialog.tsx:108 — the useCallback around clone never memoizes. TanStack Query v5 (5.101.2, useMutation.js:40) returns { ...result, mutate, mutateAsync }, a fresh object every render, so checkout changes identity every render and clone with it; the awaitingSso effect tears down and re-registers both listeners on every render. Harmless (React flushes cleanup and setup in the same commit, so no event can slip through the gap), but it advertises a stability it does not have. Depending on checkout.mutate / checkout.isPending instead of the whole object would make it real.

• packages/cezar/src/server/checkout.test.ts:94 — the ghCloneArgs case sits in describe('checkout — repo reference parsing') but asserts clone-argument construction, not parsing. Its own describe would keep the suite's map honest.

• packages/web/src/components/clone-project-dialog.tsx:30 — githubSsoUrl is exported but nothing imports it. Searched packages/**/*.{ts,tsx} excluding node_modules at 44186c69; the only reference is line 72 in the same file, and the test does not import it either. Either drop the export or have the test import it and cover the matcher cases at the unit level — which would be the natural home for the truncation minor above.

🔒 Security review of the matcher — it holds up

I tried to break githubSsoUrl rather than take the "narrow by design" comment on trust. Running the actual regex against hostile inputs:

input	result
https://github.com.evil.example/orgs/x/sso?authorization_request=abc	rejected
remote: visit https://evil.example/authorize	rejected
https://evil.example@github.com/orgs/Acme/sso?authorization_request=ZZZ (userinfo trick)	rejected
http://github.com/orgs/Acme/sso?authorization_request=ZZZ (scheme downgrade)	rejected
https://evil.example/r?next=https://github.com/orgs/Acme/sso?authorization_request=ZZZ	matches the inner, genuine github.com URL — the attacker's wrapper is stripped, so the user still lands on real GitHub

Pinning the literal https://github.com/orgs/ prefix is what makes all of this work, and matching on the URL shape rather than on gh's English ("Authorize in your web browser:") is the right call — it is the part of the output GitHub is least likely to reword.

On credential hygiene, no leak. The authorization_request value is a session-scoped nonce, not a bearer credential — redeeming it requires being signed in to GitHub as that user. It is kept out of the rendered text (the test asserts not.toContain('authorization_request=')), it reaches the DOM only as the link's href, and rel="noreferrer" keeps it out of the outbound Referer. checkout-progress events carry gh's raw stderr but go to the in-memory workspace bus only (server.ts:2521) — nothing under .ai/cezar/ is written, so CODE_REVIEW.md's "no secrets in state files" rule is satisfied.

On looping, no loop. ssoRetryArmed (a ref, so it survives the re-render) plus the awaitingSso gate allow exactly one automatic retry per link click: retry() disarms before calling clone(), and clone() disarms again on entry. A second SAML failure does not re-arm, so the user must click the link again. Server-side the retry is safe too — checkoutRepo runs cleanupCheckout before answering (checkout.ts:353), so the retry cannot trip over its own half-written directory and get a 409.

🧪 Validation Gate

All five commands from .ai/agentic.config.json, in order, at 44186c69 in an isolated worktree.

Command	Status	Evidence
npm run typecheck	✅ PASS	exit 0 — @open-mercato/cezar (tsc --noEmit -p tsconfig.test.json) and @open-mercato/cezar-web (tsc --noEmit) both clean
npm test	✅ PASS	Test Files 332 passed (332) · Tests 6395 passed (6395) · 55.21s
npm run test:unit	✅ PASS	tests 36 · pass 35 · fail 0 · skipped 1 · 3563ms
npm run build	✅ PASS	server + web built; check:pack ok — 481 files, 85 under web/dist (shell + assets present)
npm run test:package	✅ PASS	tests 16 · pass 16 · fail 0 · 24133ms

One environment caveat, not a finding against this PR: this sandbox sets TMPDIR inside the repository, which false-fails the handful of tests that create a temp dir and assert it is not a git repo. The table above was produced with TMPDIR=/tmp. The flake in workflows/run.test.ts reported by the previous pass did not reproduce this time — 6395/6395 on a single clean run.

✅ CI status

Now green, and this is a change since the last review: Unit, build, E2E, and package SUCCESS (4m36s, run 34284143386), license/cla SUCCESS, Publish npm snapshot skipped. No pending checks. mergeable: MERGEABLE, mergeStateStatus: CLEAN — no conflicts against main.

CI passing does not clear the major above: the broken path is a git push from a user's own machine against their own git config, which CI has no way to exercise.

💥 Breaking Changes

None on a published surface. RepoRef gains a required cloneUrl, which would break an external constructor — but RepoRef is not exported from the package (@open-mercato/cezar exposes only . → dist/index.js, ./app-type and package.json), and the sole construction site is parseRepoRef, which always populates it. slug is retained for messages and dry-run output. POST /api/v1/projects/checkout and the checkout-progress event payload — both protected by BACKWARD_COMPATIBILITY.md §2 — are untouched, and the route-inventory drift guard passes. CEZ_DRY_RUN=1 still works; dryRunCloneRunner never reads cloneUrl.

The behavioural change in the major finding is not a contract break, which is why it is filed as major rather than blocker — but it is a silent change to what a user's checkout looks like afterwards, and it deserves a line in the PR description whichever fix you choose.

🧪 Test Coverage

Proportionate and well-placed. Keeping ghCloneArgs pure is the right seam — the transport choice is pinned without spawning a real gh. parseRepoRef's table-driven case was extended to assert cloneUrl for every accepted spelling, including the git@github.com: form that motivated the fix. The web side covers the happy SSO path (compact copy, link target/rel, no leaked authorization_request=, "Retry clone", retry-on-return, dialog close) and the phishing negative.

Three gaps, all filed above: the visibilitychange branch, the matcher's truncation behaviour, and — the one that matters — no test asserts what the clone leaves behind, which is why the push regression got through two passes.

---

🤖 Generated with Claude Code

[pat-lewczuk]

@piotrchabros, the clone half of this fix is right, but forcing every clone onto HTTPS leaves the resulting checkout unable to push: gh injects its credential helper for the clone command only, so the repo keeps a bare HTTPS origin and later git operations fall back on a global credential.https://github.com.helper that an SSH-protocol gh auth login never installs — and cezar pushes task branches with raw git (forge/github.ts:2471), which then blocks on a credential prompt until the 60s timeout. That hits exactly the git_protocol ssh users your regression evidence describes.

Two ways out, both cheap: persist what the clone borrowed (git -C <target> config credential.https://github.com.helper '!gh auth git-credential' after a successful clone), or fall back to cloneUrl only after a first attempt fails with a SAML-shaped error so working transports stay untouched. A test asserting the post-clone remote/credential state would keep it from regressing.

Four minors and three nits are in the review too — the pick of them is that a truncated SSO match is indistinguishable from a good one while the raw URL is discarded, which leaves the user at a dead link with no recourse.

Full review: #968 (review) — the validation gate (typecheck, npm test, test:unit, build, test:package) is green at 44186c69, and so is CI now. Push the update and re-request review.

[pat-lewczuk]

🤖 om-auto-review-pr completed: CHANGES REQUESTED. Lock released.

Re-review at 44186c69 (no new commits since the previous pass — re-review was explicitly requested). 1 major, 4 minors, 3 nits, 0 blockers. The major is new and is what flipped the previous approve: forcing the clone onto HTTPS fixes the clone but leaves the checkout with a bare HTTPS origin and no credential path, so cezar's own git push breaks for the git_protocol ssh users this PR targets. Verified against gh 2.100.0 — the credential helper gh injects is scoped to the clone command and does not persist into the repo.

The full local validation gate passed at 44186c69 in an isolated worktree: npm run typecheck (exit 0), npm test (332 files / 6395 tests), npm run test:unit (36 tests, 0 fail), npm run build (check:pack ok — 481 files), npm run test:package (16/16). Repository CI is now green too — Unit, build, E2E, and package passed, which is a change since the previous pass reported it stuck at action_required. No pending checks and no conflicts against main.

Labels: changes-requested replaces merge-queue; needs-qa, bug, priority-medium and risk-medium are preserved. Assigned back to @piotrchabros with the handoff above.

autofix: skipped (not my PR — re-run with --autofix to fix it here)

[piotrchabros]

@pat-lewczuk addressed your review in the update ending at 90738b4:

• Successful HTTPS clones now persist the GitHub CLI credential helper in repository-local config, resetting inherited helpers first. Task worktrees inherit it, so later raw git push operations can use the same OAuth grant. A failed config write produces an explicit checkout error instead of silently leaving a checkout without credentials.
• The push subprocess disables terminal credential prompts.
• The SSO matcher rejects truncated tokens; the original server error remains accessible under Error details. Retry copy includes the manual button for background-tab clicks, and the callback depends on stable mutation members.
• Added coverage for persisted credentials in the checkout and a task worktree, noninteractive pushes, matcher boundaries, and focus/visibility/manual retry paths, including visibility plus focus firing only one retry. Moved the clone-argument case into its own transport group.

Validation: typecheck, 51 focused tests, 36 additional unit checks, production build, and 16 package tests pass. Nine new regression cases fail against the previous PR head (44186c69). The full suite has 6,409 passing tests and one skip, but exits nonzero on five background workflow cleanup ENOENT rejections; the unchanged PR head reproduces the same five errors. Tests were run with inherited Cezar settings removed and root's permission bypass disabled to avoid environment-dependent failures.

Live browser QA against a SAML-enforced organization was not performed, so the existing QA gate still applies. Ready for re-review.

[pat-lewczuk]

🤖 om-auto-review-pr started by @pat-lewczuk at 2026-09-14T19:25:02Z. Other auto-skills will skip this PR until the lock is released.

Re-review requested by the author at 90738b47; taking the PR back from the changes-requested handoff.

[pat-lewczuk]

🔁 Re-review: fix(clone): recover from GitHub organization SAML auth

Verdict

✅ approve — all eight findings from the previous pass are fixed, and the major one is fixed correctly: I reproduced the credential mechanism end to end rather than reading it. No blockers, no majors. Three minors and three nits below; none of them gate the merge. needs-qa stays on — the dialog's three retry paths are now unit-tested, but the real-browser pass is still worth one human.

The delta in one line: the previous pass's major was "the forced-HTTPS clone leaves a checkout that cannot push". persistGhCredentialHelper now writes gh's credential helper into the clone's local config, execTool disables the terminal prompt, and tests pin both — including that a task worktree inherits the helper, which is the specific reason github.ts:2471 was broken.

Disposition of the earlier findings

Severity	Previous finding	Status
Major	checkout.ts — forced HTTPS strands every later git operation	✅ fixed (verified below)
Minor	dialog :30 — a truncated match is indistinguishable from a good one	✅ fixed — end-anchored, plus a githubSsoUrl unit suite
Minor	dialog :52 — module docstring contradicts the code	✅ fixed — docstring amended and the raw error made reachable
Minor	dialog :131 — visibilitychange retry path untested	✅ fixed — it.each(['focus','visibility','manual'])
Minor	dialog :251 — "cezar will retry automatically" over-promises on ctrl-click	✅ fixed — copy now names the button; ctrl-click is a tested case
Nit	dialog :108 — useCallback never memoizes	✅ fixed — depends on mutate / isPending
Nit	checkout.test.ts:94 — ghCloneArgs sat in the parsing describe	✅ fixed — own describe
Nit	dialog :30 — githubSsoUrl exported but unimported	✅ fixed — the test imports it

Nothing regressed.

🔬 The credential fix, verified rather than read

The previous review's major rested on a claim about gh; this fix rests on a claim about git. Both deserve the same treatment, so I ran the exact configuration the code writes:

$ git config --local --replace-all 'credential.https://github.com.helper' ''
$ git config --local --add         'credential.https://github.com.helper' '!gh auth git-credential'
$ cat .git/config
[credential "https://github.com"]
        helper =
        helper = !gh auth git-credential

Then the part that actually decides it — does that local pair override whatever the user already has, and does it produce a credential?

$ printf 'protocol=https\nhost=github.com\n\n' \
  | git -c 'credential.helper=!echo GLOBALHELPER_SHOULD_NOT_RUN >&2; exit 1' credential fill
protocol=https
host=github.com
username=<redacted>
password=<redacted>

GLOBALHELPER_SHOULD_NOT_RUN never fired and a real credential came back. The empty helper = entry resets the inherited list exactly as gh auth setup-git does, and !gh auth git-credential answers. That is the whole fix, and it holds.

Two details worth calling out as good judgement rather than luck:

• The failure path is safe. persistGhCredentialHelper returning false takes checkoutRepo's !outcome.ok branch (checkout.ts:375), which runs cleanupCheckout before answering — so a retry gets a clean mkdir rather than the 409 a leftover directory would produce.
• The new test substitutes only gh (a two-line shim) and lets real git do the configuring, then asserts git worktree add inherits the helper. That is the right seam: it pins the mechanism the major turned on, not a mock of it.

The GIT_TERMINAL_PROMPT=0 half is pinned honestly too — draft-pr-autosave.test.ts sets the variable to 1 in the environment and asserts a pre-push hook observes 0, so the test fails if the env option is ever dropped.

I also re-probed the anchored matcher against the cases that motivated it:

input	result
…=ABCDEFGH123456==	rejected (was: silently truncated)
…=ABC%2FDEF	rejected (was: truncated to ABC)
…=ABC + newline / quote / EOS	accepted
https://github.com.evil.example/…, http://github.com/…, https://evil.example@github.com/…	rejected

Refusing outright is the right call now that the raw message is always reachable under Error details — the user can still open the URL by hand. No catastrophic backtracking: the lookahead adds one linear rescan, with no nested quantifier.

Findings

Minor

1. packages/cezar/src/server/git-changes.ts:31 — the sibling raw-git push has neither guard, and no timeout at all.

Not a regression from this PR — flagging it because it is the one remaining place that does exactly what execTool was just fixed for, and because the fix is the line you already wrote.

POST /runs/:id/git/push (server.ts:4187) → pushCurrentBranch (git-changes.ts:680) → git(dir, ['push']) at :695. That helper's execFile options set no env — so no GIT_TERMINAL_PROMPT=0 — and no timeout, and execFile's default is 0, meaning unbounded. I confirmed git really does reach for the terminal on a github-shaped HTTPS URL with no usable helper:

$ script -qec 'printf "protocol=https\nhost=example.invalid\n\n" | git -c credential.helper= credential fill' /dev/null
Username for 'https://example.invalid':

It only returned here because script closed the pipe; against a live tty it blocks on that read. So when gh auth git-credential cannot answer — logged out, expired token, or a project cloned before this change — that route never returns at all, which is worse than the 60s stall you just removed from execTool. skills-remote.ts:52 already hardens this way; git-changes.ts is the last one out. Either fold env: { ...process.env, GIT_TERMINAL_PROMPT: '0' } plus a timeout into that helper, or file it as a follow-up.

2. packages/cezar/src/server/checkout.ts:194 — the persist failure paraphrases git's error away, and then discards a clone that worked.

When either git config exits non-zero, persistGhCredentialHelper drops stdout and stderr on the floor and the runner answers a fixed string — "Could not configure GitHub credentials for the checkout. Check directory permissions and retry." — after which cleanupCheckout deletes a clone that genuinely succeeded, possibly ten minutes of a large repo.

Two problems with that. The guess at "directory permissions" can simply be wrong: a git missing from PATH produces the identical message. And this module argues the opposite policy eighty lines down at :281 — "The tail of gh/git's own output IS the error message … paraphrasing them would only lose detail." Capture stderr from the failing git config and append it, the way the clone-failure branch already does.

3. packages/cezar/src/server/checkout.ts:276 — the destructive half of the new branch is untested.

The success path is pinned well. The branch that throws away a successful clone and returns 500 is not exercised anywhere, and it is the one with a destructive side effect. A case that makes git config fail and asserts both the error text and that the target directory was removed would pin it — and would have caught finding 2 as a by-product.

Nit

1. checkout.ts:276 floats the promise with no .catch(). finish is only ever called from inside the .then, so a rejection would leave the clone promise unsettled and checkoutRepo's await run(...) hanging the request permanently. persistGhCredentialHelper cannot realistically reject today — every execFile error arrives through the callback — so this is insurance, not a bug. It is just insurance on the one path with no timeout above it.

2. clone-project-dialog.tsx:263 — the nonce is now in the DOM as text, and the old expect(...).not.toContain('authorization_request=') assertion went with it. That is the direct, correct consequence of making the raw message reachable, and the value is a session-scoped nonce that already travels in the link's href, so nothing is newly exposed. Worth one sentence in the docstring's security note so the next reader does not re-litigate it.

3. clone-project-dialog.tsx:263 — this is the only native <details> in packages/web. Every other disclosure in the cockpit is a shadcn component; an unstyled <summary> inside a text-danger block will not carry the dialog's type scale or focus ring. Cosmetic, and only visible on an error path.

🧪 Validation Gate

All five commands from .ai/agentic.config.json, in order, at 90738b47 in an isolated worktree on a clean npm ci.

Command	Status	Evidence
npm run typecheck	✅ PASS	exit 0 — @open-mercato/cezar and @open-mercato/cezar-web both clean
npm test	✅ PASS	Test Files 332 passed (332) · Tests 6410 passed (6410) · 116.59s (+15 over the previous pass's 6395)
npm run test:unit	✅ PASS	tests 36 · pass 36 · fail 0
npm run build	✅ PASS	server + web built; check:pack ok — 481 files, 85 under web/dist
npm run test:package	✅ PASS	tests 16 · pass 16 · fail 0

The PR's own three files, run on their own: checkout.test.ts + draft-pr-autosave.test.ts → 30/30; clone-project-dialog.test.tsx → 21/21.

Same environment caveat as last time, not a finding against this PR: this sandbox points TMPDIR inside the repository, which false-fails the handful of tests that create a temp dir and assert it is not a git repo. The table was produced with TMPDIR=/tmp. No flakes on a single clean run.

✅ CI status

Green, and no longer pending: Unit, build, E2E, and package SUCCESS (run 34861711389), license/cla SUCCESS, Publish npm snapshot skipped. mergeable: MERGEABLE, no conflicts against main. mergeStateStatus is BLOCKED only because of the previous changes-requested review, which this one supersedes.

💥 Breaking changes

None on a protected surface. persistGhCredentialHelper is not exported, RepoRef is not reachable from the package's exports map, and POST /api/v1/projects/checkout plus the checkout-progress payload — both under BACKWARD_COMPATIBILITY.md §2 — are untouched. CEZ_DRY_RUN=1 still works: dryRunCloneRunner never reaches the new code.

One behavioural note that deserves a line in the PR body and the CHANGELOG, since it is a file cezar did not previously write: cloning now adds a credential.https://github.com.helper entry to the cloned project's local .git/config, which supersedes a user's global helper for github.com in that repo. That is what gh auth setup-git does globally, so it is the conventional choice and strictly narrower, but a user with a corporate credential helper should not have to discover it from a diff.

🧪 Test coverage

Proportionate, and the new cases target the right things rather than the easy things. The credential test uses real git against a fake gh and follows through to a task worktree — that is the assertion whose absence let the push regression through two passes. The it.each(['focus','visibility','manual']) rewrite covers the visibilityState guard, the double-fire ssoRetryArmed ref, and the ctrl-click path in one table, each asserting exactly one additional POST. The githubSsoUrl suite finally gives the matcher a unit-level home, truncation cases included.

One gap remains, filed as minor 3: the credential-persist failure branch.

---

🤖 Generated with Claude Code

[pat-lewczuk]

🤖 om-auto-review-pr completed: APPROVED. Lock released.

Re-review at 90738b47 (two new commits since the changes-requested pass at 44186c69). 0 blockers, 0 majors, 3 minors, 3 nits. All eight findings from the previous pass are fixed, and the major one is fixed correctly — I exercised the persisted !gh auth git-credential helper rather than reading it: the empty-reset entry does override an inherited helper, the helper does answer for github.com, and the shipped test follows it through into a task worktree, which is the exact seam that broke git push.

The three minors are follow-up material, not merge blockers: the sibling push helper in git-changes.ts still lacks the prompt guard and a timeout (pre-existing, and the one-line fix is the one this PR already wrote next door); the credential-persist failure paraphrases git's own error away before deleting a clone that succeeded; and that failure branch has no test.

Full local gate green at 90738b47 in an isolated worktree on a clean npm ci: npm run typecheck (exit 0), npm test (332 files / 6410 tests, +15 on this revision), npm run test:unit (36/36), npm run build (check:pack ok — 481 files), npm run test:package (16/16). Repository CI is green too — Unit, build, E2E, and package SUCCESS, license/cla SUCCESS — so no CI follow-up is outstanding and no ci-monitoring label was applied.

Labels: merge-queue + needs-qa. With qaGate on, the QA-approval gate holds the merge until a QA reviewer adds qa-approved; the manual QA instructions above were refreshed for this revision (they now cover the push-after-clone path).

autofix: skipped (not my PR — re-run with --autofix to fix it here)