From 8ab6526c781d1033ba84af4885c7fc7c29643f20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Sun, 7 Jun 2026 22:34:45 +0000 Subject: [PATCH 1/7] docs(plan): add pr merge admin bypass plan --- .agents/plans/pr-merge-admin-bypass.md | 162 +++++++++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 .agents/plans/pr-merge-admin-bypass.md diff --git a/.agents/plans/pr-merge-admin-bypass.md b/.agents/plans/pr-merge-admin-bypass.md new file mode 100644 index 00000000..20af71ae --- /dev/null +++ b/.agents/plans/pr-merge-admin-bypass.md @@ -0,0 +1,162 @@ +Activate the /implement-task skill first. + +# Plan: PR Merge Admin Bypass + +## Acceptance Criteria +- [ ] "since i made my repo public, i enabled need a review to merge so people cannot merge stuff to my repo without me knowing." +- [ ] "however since i'm solo, no one can review my prs so i have to bypass and merge PRs - but citadel doesn't support this in the merge button - we need to add support for it." +- [ ] "Run /do-tech-plan and then /implement-task on that plan to implement this please." +- [ ] Normal PR merges continue to omit GitHub's `--admin` flag. +- [ ] When the operator explicitly selects admin bypass in the merge menu, Citadel runs the merge with GitHub's `--admin` flag. +- [ ] Invalid `admin` request values are rejected by the daemon merge endpoint instead of being silently accepted or coerced. +- [ ] `pr.merge` hooks receive the admin-bypass flag so hook-backed merge flows can honor the same operator intent. +- [ ] The UI admin-bypass control is unchecked by default and must be explicitly selected for each merge. +- [ ] The UI resets admin bypass after every merge attempt and whenever the menu closes/reopens or the workspace/PR context changes. + +## Context and problem statement +Citadel already exposes a Merge action in the inspector PR card. The web UI calls `POST /api/workspaces/:workspaceId/pr-merge`, the daemon validates a merge strategy and delegates to `@citadel/providers.mergePr`, and the provider runs `gh pr merge --` without `--delete-branch`. + +For a public GitHub repository with required reviews enabled, a solo owner/admin may need to intentionally bypass unmet repository requirements. The local `gh pr merge --help` confirms the supported flag is `--admin`, described as using administrator privileges to merge a pull request that does not meet requirements. Citadel needs to expose that option explicitly without weakening its existing normal merge path. + +## Spec alignment +Applicable specs: +- `specs/A-shared-definitions.md` for Provider, Operation, and Action terminology. +- `specs/B.4-git-pr-ci-diff.md` for PR merge behavior. +- `specs/B.2-ade-cockpit.md` for cockpit operator action behavior. +- `specs/B.8-ui-performance-quality.md` for UI quality and test expectations. + +Spec update required first: +- Update `specs/B.4-git-pr-ci-diff.md` PR Identity item 13 to state that the Merge action can explicitly pass GitHub's admin bypass flag for unmet repository requirements, while still respecting allowed merge strategies and never deleting the head branch by default. + +No DB schema changes. This does change the PR merge request contract in `@citadel/contracts`. + +## Implementation approach +Add an optional `admin` boolean to the PR merge contract. Keep the default `false` so existing callers keep the same behavior. Reject non-boolean `admin` values. Thread the parsed field through the daemon route, PR merge hook payload, provider helper, and web merge dropdown. + +In the UI, add a `role="menuitemcheckbox"` control inside the existing merge strategy menu labeled "Admin bypass" with clear text that it bypasses unmet repository requirements. The normal strategy buttons still execute normal merges by default; when the operator checks the bypass option, the request body includes `admin: true` and the provider adds `--admin` to the `gh pr merge` invocation. + +Because `packages/providers/src/index.ts` is already at 799 lines, first extract the shared GitHub CLI runner state into an internal provider module, then move the merge helper into a small provider module and re-export it from the package entry. This avoids violating the 800-line source file limit, preserves the existing `@citadel/providers` import path, and avoids a circular import between the package entry and the extracted merge helper. + +## Alternatives considered +Only retry failed merges with `--admin` after a normal merge fails. Rejected because it turns bypassing repository rules into a hidden fallback. The operator should make the bypass explicit before Citadel invokes admin privileges. + +A separate "Bypass and merge" button outside the dropdown. Rejected because the PR card is intentionally constrained to a single action slot, and merge strategy selection already lives in the dropdown. + +## Implementation steps + +### Spec Update +- Update `specs/B.4-git-pr-ci-diff.md` PR Identity item 13 to include explicit admin bypass support for unmet repository requirements. + +### Contract and Provider +- Update `packages/contracts/src/pr-routes.ts` so `PrMergeRequestSchema` accepts optional `admin` with default `false`. +- Confirm the contract rejects non-boolean `admin` values and preserves default `admin: false`. +- Extract the shared GitHub CLI runner and `setGithubCommand` state from `packages/providers/src/index.ts` into a separate internal module. +- Extract `mergePr` from `packages/providers/src/index.ts` into a new small provider module that imports the internal runner directly; re-export public functions from `packages/providers/src/index.ts`. +- Update `mergePr` to append `--admin` only when `input.admin === true`. +- Preserve the existing no-`--delete-branch` behavior. +- File-size gate: keep all edited non-generated files under 800 lines; the extraction should reduce `packages/providers/src/index.ts`. + +### Daemon Route +- Update `apps/daemon/src/pr-routes.ts` to parse `admin`, include it in `pr.merge` hook payloads, and pass it to `mergePr`. +- Keep route status behavior unchanged: 200 on direct merge success, 202 when hooks handle the merge, 409 on merge failure/no PR. +- Provider degradation: existing GitHub CLI health and gh cooldown behavior remains the availability gate; if admin privilege is unavailable, `gh` failure still returns a structured merge failure rather than silently retrying. + +### Web UI +- Update `apps/web/src/pr-card-actions.tsx` to add a compact `role="menuitemcheckbox"` control in the merge menu for "Admin bypass". +- Include `aria-checked`, stop propagation on the toggle, and keep keyboard/click behavior compatible with the existing menu. +- Send `{ strategy, admin: true }` only when the checkbox is checked; omit or send `false` for normal merges. +- Reset admin bypass to `false` on menu close/reopen, workspace or PR-number change, and after every merge attempt settles, including failed attempts. +- Close menu state consistently after successful merge. +- Keep disabled-state reasons and the existing provider-health/mergeability gates unchanged. + +### Hook Templates +- Update PR merge hook coverage so a `pr.merge` template using `{{admin}}` renders the operator-selected flag for hook-backed merge flows. + +### E2E Smoke +- Extend the PR E2E smoke so the deployed daemon rejects an invalid `admin` payload with `400 invalid_merge_request`, proving the deployed route is validating the new field instead of silently stripping unknown keys. +- Optionally also assert `{ strategy: "squash", admin: true }` reaches the existing `no_pr` response for an existing workspace with no PR, but do not rely on that as the only schema proof. + +### Migration Strategy +No DB schema changes. No `schema_migrations` entry is required. + +## QA/Test Strategy + +### Unit (Vitest) +Tests must be updated. + +New/updated tests: +- `packages/contracts/src/index.test.ts`: assert `PrMergeRequestSchema.parse({ strategy: "squash" })` returns `admin: false`, `PrMergeRequestSchema.parse({ strategy: "squash", admin: true })` returns `admin: true`, and invalid non-boolean admin values fail. +- `packages/providers/src/pr-merge.test.ts`: add a focused provider test for the extracted merge module that asserts normal merges do not pass `--admin`, admin merges do pass `--admin`, and neither path passes `--delete-branch`. +- `apps/daemon/src/pr-routes.test.ts`: assert `POST /api/workspaces/:id/pr-merge` passes `admin: true` through to the `gh` invocation, and `pr.merge` hook payloads include the admin flag. +- `packages/operations/src/hooks-runner.test.ts`: assert a `pr.merge` template can render `{{admin}}` from the merge hook payload. +- `apps/web/src/pr-card-actions.test.ts`: assert the merge menu renders a bypass checkbox and that checking it causes a merge strategy click to send an admin merge request payload. +- `apps/web/src/pr-card-actions.test.ts`: assert admin bypass resets to unchecked after close/reopen, after workspace/PR context changes, and after a failed merge attempt so a later normal merge omits `admin: true`. + +Assertions to tighten: +- Keep the existing provider assertion that `--delete-branch` is never present. +- Add a daemon assertion that invalid `admin` payloads are rejected with `invalid_merge_request` rather than silently coerced. +- Add a web assertion that the bypass option is unchecked by default and uses `role="menuitemcheckbox"`/`aria-checked`. +- Add a web assertion that closing/reopening the menu or completing/failing a merge attempt clears the bypass state before the next strategy click. + +Failure modes covered: +- User checks bypass, but Citadel omits `--admin`. +- User performs a normal merge, but Citadel unexpectedly passes `--admin`. +- Web and daemon contract drift on the new field. +- Custom PR merge hooks do not receive enough context to perform their own admin merge. +- A future provider edit accidentally adds `--delete-branch`. + +Remaining gaps: +- Local tests cannot prove the caller's GitHub account has admin permission on a real repository. A live GitHub failure remains surfaced as a structured `gh` merge failure. + +### E2E (Playwright) +Tests must be updated. + +Update `e2e/pr-display.spec.ts` or add a nearby PR merge smoke spec: +- Create/register a local git fixture and workspace. +- POST `{ strategy: "squash", admin: "true" }` to `/api/workspaces/:id/pr-merge`. +- Assert the response is `400 invalid_merge_request`, proving the deployed daemon route validates the new field rather than silently stripping unknown payload. +- Optionally POST `{ strategy: "squash", admin: true }` and assert the response reaches existing PR-state validation, but this is secondary to the invalid-value assertion. + +Full PR merge UI E2E is not required because the harness cannot fabricate a live GitHub PR with required-review state. Vitest covers the UI interaction and provider command formation. + +### Adversarial Thinking +How this could fail in production: +- GitHub rejects `--admin` because the operator lacks privileges; Citadel should surface the `gh` failure detail. +- The UI could make bypass too easy to trigger accidentally; the checkbox makes the admin path explicit and unchecked by default. +- The UI could accidentally carry a checked admin bypass into a later merge; reset-on-close, reset-on-context-change, and reset-after-settle tests cover this. +- The daemon could accept the field but not pass it to hooks or providers; route tests cover both paths. +- Zod could strip unsupported payload keys and make weak route tests pass before the feature exists; invalid-value E2E coverage prevents that false positive. +- Provider helper extraction could accidentally import from `index.ts` and create a circular import; an internal GitHub runner module avoids that. +- Provider extraction could break existing imports from `@citadel/providers`; typecheck and provider tests cover the package export. +- File growth could violate Citadel's size gate; extraction addresses the known 799-line provider file. + +Automated tests that catch these: +- Contract schema tests catch API shape drift. +- Provider command tests catch missing/extra `--admin` and accidental `--delete-branch`. +- Daemon route tests catch route parsing and pass-through regressions. +- Hook runner tests catch template visibility for hook-backed merge flows. +- Web component tests catch UI payload regressions and admin-bypass state leakage. +- E2E smoke catches deployed route registration/schema regressions. + +## Tests +- Update `packages/contracts/src/index.test.ts`. +- Add `packages/providers/src/pr-merge.test.ts`. +- Update `apps/daemon/src/pr-routes.test.ts`. +- Update `packages/operations/src/hooks-runner.test.ts`. +- Update `apps/web/src/pr-card-actions.test.ts`. +- Update `e2e/pr-display.spec.ts` or add a PR merge smoke spec. + +## Schema or contract generation +No generated schema artifacts. `@citadel/contracts` is the schema package; `pnpm typecheck` validates consumers. + +## Verification +- `pnpm vitest run packages/contracts/src/index.test.ts packages/providers/src/index.test.ts packages/providers/src/pr-merge.test.ts apps/daemon/src/pr-routes.test.ts packages/operations/src/hooks-runner.test.ts apps/web/src/pr-card-actions.test.ts` +- `pnpm exec playwright test e2e/pr-display.spec.ts` +- `pnpm check:arch` +- `pnpm check:size` +- `pnpm typecheck` +- `pnpm lint` +- `pnpm coverage` +- `pnpm build` +- `make check` +- `pnpm e2e` From 92a3b4cc43d3dc47466ee5725fe251315e92e47d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Sun, 7 Jun 2026 22:35:11 +0000 Subject: [PATCH 2/7] docs(spec): document pr merge admin bypass --- specs/B.4-git-pr-ci-diff.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/B.4-git-pr-ci-diff.md b/specs/B.4-git-pr-ci-diff.md index 89aac5ca..e0bfbc80 100644 --- a/specs/B.4-git-pr-ci-diff.md +++ b/specs/B.4-git-pr-ci-diff.md @@ -30,7 +30,7 @@ [x] 10. Checkout rows always reserve a PR slot, even when no PR exists. [x] 11. The PR row exposes a copy-branch affordance for the head branch, and uses GitHub's `base ← head` direction. [x] 12. Stacked PRs are modeled through checkout stack relationships and can also be detected by comparing each PR's base ref to other open and recently-merged PRs in the same head repository. When a parent is detected, the checkout row and inspector show an `↑ #` chip linking to that PR; merged parents are rendered with a distinct tone. -[x] 13. The inspector PR card surfaces a single action button at its bottom-right. It renders Merge when the PR is open and the GitHub CLI is healthy — respecting the repository's allowed merge strategies (squash/merge/rebase) and never deleting the head branch by default — and switches to Fix conflicts when `mergeable === "conflicting"` or `mergeStateStatus === "DIRTY"`. Merge and Fix conflicts are mutually exclusive (a conflicting PR cannot be merged); merged or closed PRs render no action. +[x] 13. The inspector PR card surfaces a single action button at its bottom-right. It renders Merge when the PR is open and the GitHub CLI is healthy — respecting the repository's allowed merge strategies (squash/merge/rebase), allowing an explicit admin-bypass option for unmet GitHub repository requirements, and never deleting the head branch by default — and switches to Fix conflicts when `mergeable === "conflicting"` or `mergeStateStatus === "DIRTY"`. Merge and Fix conflicts are mutually exclusive (a conflicting PR cannot be merged); merged or closed PRs render no action. [~] 14. GitHub's `mergeable` and `mergeStateStatus` fields are surfaced through `PullRequestSummary`. `mergeable !== "CONFLICTING"` is required for the `ready-to-merge` readiness state; `mergeStateStatus` informs the workspace-card tone only. These fields are refreshed when (a) the PR's own `headSha` changes, (b) the repo's default-branch SHA moves (detected by the per-repo merge-to-main watcher), or (c) the operator clicks force-refresh — never on every poll. [~] 15. When `mergeable === "CONFLICTING"`, the workspace enters the dedicated `pr-conflicts` readiness state, distinct from the local working-tree `conflicts` state and from `checks-failing`. The workspace-card tone also flips to `conflicting` when `mergeStateStatus === "DIRTY"`, but the readiness state itself remains scoped to `mergeable === "CONFLICTING"`. [ ] 16. Structured checkout gates consume durable checkout PR facts keyed by provider instance/account, repository identity, checkout id, PR identity, and head SHA. Workspace-level PR caches may render UI summaries but cannot satisfy structured readiness on their own. From 9df9f7662bbae4db8166c3d296451c6ba0f35139 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Sun, 7 Jun 2026 22:40:02 +0000 Subject: [PATCH 3/7] feat(pr): support admin merge bypass backend --- apps/daemon/src/pr-routes.test.ts | 67 ++++++++++++++++++- apps/daemon/src/pr-routes.ts | 5 +- packages/contracts/src/index.test.ts | 9 ++- packages/contracts/src/pr-routes.ts | 1 + packages/operations/src/hooks-runner.test.ts | 11 +++- packages/providers/src/gh-runner.ts | 42 ++++++++++++ packages/providers/src/index.ts | 68 ++------------------ packages/providers/src/pr-merge.test.ts | 32 +++++++++ packages/providers/src/pr-merge.ts | 31 +++++++++ 9 files changed, 193 insertions(+), 73 deletions(-) create mode 100644 packages/providers/src/gh-runner.ts create mode 100644 packages/providers/src/pr-merge.test.ts create mode 100644 packages/providers/src/pr-merge.ts diff --git a/apps/daemon/src/pr-routes.test.ts b/apps/daemon/src/pr-routes.test.ts index 7b1fef88..2dd837a0 100644 --- a/apps/daemon/src/pr-routes.test.ts +++ b/apps/daemon/src/pr-routes.test.ts @@ -399,6 +399,59 @@ describe("PR routes", () => { } }); + it("passes admin bypass through to gh merge only when requested", async () => { + const now = new Date().toISOString(); + const gh = fakeGhRecorder(); + setGithubCommand(gh.script); + const providerCache = new Map(); + providerCache.set(`vc:ws_a:${now}`, { expiresAt: Date.now() + 60_000, value: makeVcSummary(now) }); + const { server } = createPrRouteHarness({ + providerCache, + workspaceUpdatedAt: now, + repoUpdatedAt: now, + }); + const baseUrl = await listen(server); + try { + const result = await postJson<{ ok: true }>(`${baseUrl}/api/workspaces/ws_a/pr-merge`, { + strategy: "squash", + admin: true, + }); + + expect(result).toEqual({ ok: true }); + expect(fs.readFileSync(gh.argsFile, "utf8").trim()).toBe("pr merge 42 --squash --admin"); + } finally { + await closeServer(server); + } + }); + + it("rejects invalid admin payloads before merging", async () => { + const now = new Date().toISOString(); + const gh = fakeGhRecorder(); + setGithubCommand(gh.script); + const providerCache = new Map(); + providerCache.set(`vc:ws_a:${now}`, { expiresAt: Date.now() + 60_000, value: makeVcSummary(now) }); + const { server } = createPrRouteHarness({ + providerCache, + workspaceUpdatedAt: now, + repoUpdatedAt: now, + }); + const baseUrl = await listen(server); + try { + const response = await fetch(`${baseUrl}/api/workspaces/ws_a/pr-merge`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ strategy: "squash", admin: "true" }), + }); + const result = (await response.json()) as { error: string }; + + expect(response.status).toBe(400); + expect(result.error).toBe("invalid_merge_request"); + expect(fs.existsSync(gh.argsFile)).toBe(false); + } finally { + await closeServer(server); + } + }); + it("runs pr.merge hooks as the merge handler when present", async () => { const now = new Date().toISOString(); const script = fakeGhScript("strategy-failure"); @@ -420,7 +473,7 @@ describe("PR routes", () => { const response = await fetch(`${baseUrl}/api/workspaces/ws_a/pr-merge`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ strategy: "squash" }), + body: JSON.stringify({ strategy: "squash", admin: true }), }); const result = (await response.json()) as { ok: true }; @@ -428,7 +481,7 @@ describe("PR routes", () => { expect(result).toEqual({ ok: true }); expect(hookCalls).toHaveLength(1); expect(hookCalls[0]?.event).toBe("pr.merge"); - expect(hookCalls[0]?.payload).toMatchObject({ strategy: "squash", pullRequest: { number: 42 } }); + expect(hookCalls[0]?.payload).toMatchObject({ strategy: "squash", admin: true, pullRequest: { number: 42 } }); expect(providerCache.has(globalPrCacheKey("owner/repo", 42))).toBe(false); } finally { await closeServer(server); @@ -637,3 +690,13 @@ function fakeGhScript(mode: "success" | "strategy-failure"): string { fs.chmodSync(script, 0o755); return script; } + +function fakeGhRecorder(): { script: string; argsFile: string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-fake-gh-")); + dirs.push(dir); + const script = path.join(dir, "gh"); + const argsFile = path.join(dir, "args.txt"); + fs.writeFileSync(script, `#!/usr/bin/env bash\nprintf '%s\\n' "$*" > ${JSON.stringify(argsFile)}\nexit 0\n`); + fs.chmodSync(script, 0o755); + return { script, argsFile }; +} diff --git a/apps/daemon/src/pr-routes.ts b/apps/daemon/src/pr-routes.ts index e30d5f4c..0380c83d 100644 --- a/apps/daemon/src/pr-routes.ts +++ b/apps/daemon/src/pr-routes.ts @@ -220,7 +220,7 @@ export function registerPrRoutes(input: { if (!workspace) return res.status(404).json({ error: "workspace_not_found" }); const repo = store.listRepos().find((candidate) => candidate.id === workspace.repoId); if (!repo) return res.status(404).json({ error: "repo_not_found" }); - let parsed: { strategy: "squash" | "merge" | "rebase" }; + let parsed: { strategy: "squash" | "merge" | "rebase"; admin: boolean }; try { parsed = PrMergeRequestSchema.parse(req.body); } catch (error) { @@ -244,6 +244,7 @@ export function registerPrRoutes(input: { operationMessage: `Running PR #${number} merge hooks`, payload: { strategy: parsed.strategy, + admin: parsed.admin, pullRequest: summary.pullRequest, versionControl: summary, }, @@ -260,7 +261,7 @@ export function registerPrRoutes(input: { if (nameWithOwner) bustGlobalPrEntry(providerCache, nameWithOwner, number); return res.status(202).json({ ok: true }); } - const result = await mergePr({ rootPath: workspace.path, number, strategy: parsed.strategy }); + const result = await mergePr({ rootPath: workspace.path, number, strategy: parsed.strategy, admin: parsed.admin }); const nameWithOwner = resolveRepoFullName(repo.id); if (result.ok) { const mergedPr = markPullRequestMerged(pr); diff --git a/packages/contracts/src/index.test.ts b/packages/contracts/src/index.test.ts index d5aa5512..7718ec33 100644 --- a/packages/contracts/src/index.test.ts +++ b/packages/contracts/src/index.test.ts @@ -546,9 +546,14 @@ describe("contract schemas", () => { }); expect(() => PrMergeResponseSchema.parse({ ok: false })).toThrow(); - // PrMergeRequestSchema requires a valid strategy. - expect(PrMergeRequestSchema.parse({ strategy: "rebase" })).toEqual({ strategy: "rebase" }); + // PrMergeRequestSchema requires a valid strategy and defaults admin bypass off. + expect(PrMergeRequestSchema.parse({ strategy: "rebase" })).toEqual({ strategy: "rebase", admin: false }); + expect(PrMergeRequestSchema.parse({ strategy: "squash", admin: true })).toEqual({ + strategy: "squash", + admin: true, + }); expect(() => PrMergeRequestSchema.parse({ strategy: "x" })).toThrow(); + expect(() => PrMergeRequestSchema.parse({ strategy: "squash", admin: "true" })).toThrow(); // Batch request requires at least one workspace id. expect(WorkspaceCockpitSummaryBatchRequestSchema.parse({ ids: ["ws_1"] })).toEqual({ ids: ["ws_1"] }); diff --git a/packages/contracts/src/pr-routes.ts b/packages/contracts/src/pr-routes.ts index e884b831..7d928529 100644 --- a/packages/contracts/src/pr-routes.ts +++ b/packages/contracts/src/pr-routes.ts @@ -58,6 +58,7 @@ export const PrRefreshResponseSchema = z.object({ export const PrMergeRequestSchema = z.object({ strategy: PrMergeStrategySchema, + admin: z.boolean().default(false), }); export const PrMergeResponseSchema = z.discriminatedUnion("ok", [ diff --git a/packages/operations/src/hooks-runner.test.ts b/packages/operations/src/hooks-runner.test.ts index 9fcbd7a1..d5f6845b 100644 --- a/packages/operations/src/hooks-runner.test.ts +++ b/packages/operations/src/hooks-runner.test.ts @@ -131,7 +131,12 @@ describe("runWorkspaceHooks — config + file hook ordering", () => { it("dispatches .prompt hooks with event-specific payload and returns hook count", async () => { const ws = tmpWorkspace(); - writeFileHook(ws, "pr.merge", "notify.prompt", "Merge PR #{{pullRequest.number}} with {{strategy}}\n"); + writeFileHook( + ws, + "pr.merge", + "notify.prompt", + "Merge PR #{{pullRequest.number}} with {{strategy}} admin={{admin}}\n", + ); const dispatchAgentHook = vi.fn().mockResolvedValue({ sessionId: "agent_session_prompt" }); const activity = vi.fn(); @@ -144,13 +149,13 @@ describe("runWorkspaceHooks — config + file hook ordering", () => { repo: baseRepo, workspace: baseWorkspace(ws), operationId: "op_merge", - payload: { strategy: "squash", pullRequest: { number: 42 } }, + payload: { strategy: "squash", admin: true, pullRequest: { number: 42 } }, dispatchAgentHook, }); expect(result.ran).toBe(1); expect(dispatchAgentHook).toHaveBeenCalledTimes(1); - expect(dispatchAgentHook.mock.calls[0]?.[0]?.prompt).toBe("Merge PR #42 with squash\n"); + expect(dispatchAgentHook.mock.calls[0]?.[0]?.prompt).toBe("Merge PR #42 with squash admin=true\n"); }); it("when dispatcher rejects, records hook..failed activity and continues to the next hook", async () => { diff --git a/packages/providers/src/gh-runner.ts b/packages/providers/src/gh-runner.ts new file mode 100644 index 00000000..e86aee95 --- /dev/null +++ b/packages/providers/src/gh-runner.ts @@ -0,0 +1,42 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { + GhRateLimitedError, + getGhCooldownReason, + getGhCooldownUntil, + isRateLimitError, + setGhCooldown, +} from "./gh-cooldown.js"; + +const execFileAsync = promisify(execFile); + +let githubCommandOverride = "gh"; + +export function setGithubCommand(command: string | undefined) { + githubCommandOverride = command?.length ? command : "gh"; +} + +// Global gh rate-limit circuit breaker. The cooldown state lives in +// ./gh-cooldown.ts; every provider path that shells out to gh should use this +// runner so one cooldown gate covers PR view, CI, auth status, merge, etc. +export async function gh(rootPath: string, args: string[]) { + const until = getGhCooldownUntil(); + if (until > Date.now()) { + throw new GhRateLimitedError(until, getGhCooldownReason() ?? "rate limit"); + } + try { + const result = await execFileAsync(githubCommandOverride, args, { + cwd: rootPath, + timeout: 12000, + maxBuffer: 1024 * 1024, + }); + return result.stdout.trim(); + } catch (error) { + const reason = isRateLimitError(error); + if (reason) { + const newUntil = setGhCooldown(reason); + throw new GhRateLimitedError(newUntil, reason); + } + throw error; + } +} diff --git a/packages/providers/src/index.ts b/packages/providers/src/index.ts index f3da50c4..e9b8073f 100644 --- a/packages/providers/src/index.ts +++ b/packages/providers/src/index.ts @@ -15,18 +15,18 @@ import type { VersionControlSummary, } from "@citadel/contracts"; import { PrMergeStateStatusSchema } from "@citadel/contracts"; -import type { ParentPr, PrCommit, PrMergeResponse, PrMergeStrategy } from "@citadel/contracts/pr-routes"; +import type { ParentPr, PrCommit } from "@citadel/contracts/pr-routes"; import { runtimeUsageFetchers } from "@citadel/runtimes"; import { GhRateLimitedError, getGhCooldown, - getGhCooldownReason, - getGhCooldownUntil, isRateLimitError, - setGhCooldown, } from "./gh-cooldown.js"; +import { gh } from "./gh-runner.js"; export { pLimit } from "./p-limit.js"; export * from "./pr-actions.js"; +export { setGithubCommand } from "./gh-runner.js"; +export { mergePr } from "./pr-merge.js"; // Re-export the cooldown surface so existing consumers (apps/daemon, // gh-quota-wiring, tests) keep their import paths. setGhCooldown is @@ -660,38 +660,6 @@ async function gitOptional(rootPath: string, args: string[]) { } } -let githubCommandOverride = "gh"; - -export function setGithubCommand(command: string | undefined) { - githubCommandOverride = command?.length ? command : "gh"; -} - -// Global gh rate-limit circuit breaker. The cooldown state lives in -// ./gh-cooldown.ts (extracted to keep this file under the 800-line gate); -// every gh-touching code path in this module funnels through gh() below, so -// the gate is uniform. -async function gh(rootPath: string, args: string[]) { - const until = getGhCooldownUntil(); - if (until > Date.now()) { - throw new GhRateLimitedError(until, getGhCooldownReason() ?? "rate limit"); - } - try { - const result = await execFileAsync(githubCommandOverride, args, { - cwd: rootPath, - timeout: 12000, - maxBuffer: 1024 * 1024, - }); - return result.stdout.trim(); - } catch (error) { - const reason = isRateLimitError(error); - if (reason) { - const newUntil = setGhCooldown(reason); - throw new GhRateLimitedError(newUntil, reason); - } - throw error; - } -} - // PR display helpers — used by the daemon's PR routes for the always-on // cross-workspace poll, force-refresh, merge button, and stacked-PR detection. @@ -745,34 +713,6 @@ export function detectParentPr( return { number: choice.number, url: choice.url, headRefName: choice.headRefName, state: choice.state }; } -type GhRunner = (args: string[]) => Promise; - -// Internal seam: tests inject a fake runner; production calls gh() in this -// module. NO --delete-branch is ever passed — branch deletion is a separate -// opt-in flow that's intentionally not part of the night's scope. -export async function mergePr( - input: { rootPath: string; number: number; strategy: PrMergeStrategy }, - runner?: GhRunner, -): Promise { - const run: GhRunner = runner ?? ((args) => gh(input.rootPath, args)); - const args = ["pr", "merge", String(input.number), `--${input.strategy}`]; - try { - await run(args); - return { ok: true }; - } catch (error) { - const detail = error instanceof Error ? error.message : String(error); - return { ok: false, reason: classifyMergeFailure(detail), detail }; - } -} - -function classifyMergeFailure(message: string): string { - const lower = message.toLowerCase(); - if (lower.includes("not mergeable") || lower.includes("merge conflict")) return "not_mergeable"; - if (lower.includes("not authorized") || lower.includes("authentication")) return "gh_auth"; - if (lower.includes("not allowed") || lower.includes("disabled")) return "strategy_disallowed"; - return "gh_error"; -} - // isGhAvailable returns whether `gh auth status` succeeded recently for the // given rootPath. Caches the answer for 60s so the merge button doesn't spawn // gh on every render. diff --git a/packages/providers/src/pr-merge.test.ts b/packages/providers/src/pr-merge.test.ts new file mode 100644 index 00000000..b361dd23 --- /dev/null +++ b/packages/providers/src/pr-merge.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, it } from "vitest"; +import { mergePr } from "./index.js"; + +describe("mergePr", () => { + it("runs normal merges without admin bypass or branch deletion", async () => { + const calls: string[][] = []; + const result = await mergePr({ rootPath: "/tmp/x", number: 7, strategy: "squash" }, async (args) => { + calls.push(args); + return ""; + }); + + expect(result).toEqual({ ok: true }); + expect(calls).toEqual([["pr", "merge", "7", "--squash"]]); + expect(calls[0]).not.toContain("--admin"); + expect(calls[0]).not.toContain("--delete-branch"); + }); + + it("adds admin bypass only when requested", async () => { + const calls: string[][] = []; + const result = await mergePr( + { rootPath: "/tmp/x", number: 8, strategy: "merge", admin: true }, + async (args) => { + calls.push(args); + return ""; + }, + ); + + expect(result).toEqual({ ok: true }); + expect(calls).toEqual([["pr", "merge", "8", "--merge", "--admin"]]); + expect(calls[0]).not.toContain("--delete-branch"); + }); +}); diff --git a/packages/providers/src/pr-merge.ts b/packages/providers/src/pr-merge.ts new file mode 100644 index 00000000..d9cf15bb --- /dev/null +++ b/packages/providers/src/pr-merge.ts @@ -0,0 +1,31 @@ +import type { PrMergeResponse, PrMergeStrategy } from "@citadel/contracts/pr-routes"; +import { gh } from "./gh-runner.js"; + +type GhRunner = (args: string[]) => Promise; + +// Internal seam: tests inject a fake runner; production calls the shared gh +// runner. NO --delete-branch is ever passed — branch deletion is a separate +// opt-in flow that's intentionally not part of the merge action. +export async function mergePr( + input: { rootPath: string; number: number; strategy: PrMergeStrategy; admin?: boolean }, + runner?: GhRunner, +): Promise { + const run: GhRunner = runner ?? ((args) => gh(input.rootPath, args)); + const args = ["pr", "merge", String(input.number), `--${input.strategy}`]; + if (input.admin === true) args.push("--admin"); + try { + await run(args); + return { ok: true }; + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + return { ok: false, reason: classifyMergeFailure(detail), detail }; + } +} + +function classifyMergeFailure(message: string): string { + const lower = message.toLowerCase(); + if (lower.includes("not mergeable") || lower.includes("merge conflict")) return "not_mergeable"; + if (lower.includes("not authorized") || lower.includes("authentication")) return "gh_auth"; + if (lower.includes("not allowed") || lower.includes("disabled")) return "strategy_disallowed"; + return "gh_error"; +} From cd4aad2617dd56eb57ca1e2ee643dff924b4d55b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Sun, 7 Jun 2026 22:46:12 +0000 Subject: [PATCH 4/7] feat(web): add pr merge admin bypass control --- apps/web/src/pr-card-actions.css | 39 ++++++- apps/web/src/pr-card-actions.test.ts | 146 +++++++++++++++++++++++++-- apps/web/src/pr-card-actions.tsx | 44 ++++++-- e2e/pr-display.spec.ts | 6 ++ 4 files changed, 221 insertions(+), 14 deletions(-) diff --git a/apps/web/src/pr-card-actions.css b/apps/web/src/pr-card-actions.css index 5b36d550..87ba836f 100644 --- a/apps/web/src/pr-card-actions.css +++ b/apps/web/src/pr-card-actions.css @@ -111,7 +111,44 @@ white-space: nowrap; } -.pr-card-merge-strategy:hover { +.pr-card-merge-admin { + display: flex; + align-items: center; + gap: 7px; + text-align: left; + background: transparent; + border: 0; + border-bottom: 1px solid var(--c-line-2); + padding: 6px 9px 7px; + font-size: 11px; + color: var(--c-fg-2); + cursor: pointer; + border-radius: 4px 4px 0 0; +} + +.pr-card-merge-admin[aria-checked="true"] { + color: oklch(58% 0.18 35); + background: oklch(60% 0.16 35 / 0.12); +} + +.pr-card-merge-admin-text { + display: flex; + flex-direction: column; + gap: 1px; + line-height: 1.1; +} + +.pr-card-merge-admin-text span:first-child { + font-weight: 700; +} + +.pr-card-merge-admin-text span:last-child { + color: var(--c-fg-4); + font-size: 9.5px; +} + +.pr-card-merge-strategy:hover, +.pr-card-merge-admin:hover { background: var(--c-line-2); } diff --git a/apps/web/src/pr-card-actions.test.ts b/apps/web/src/pr-card-actions.test.ts index 49776010..365a1abe 100644 --- a/apps/web/src/pr-card-actions.test.ts +++ b/apps/web/src/pr-card-actions.test.ts @@ -5,15 +5,19 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { createElement } from "react"; import { flushSync } from "react-dom"; import { type Root, createRoot } from "react-dom/client"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { PrCardActionSlot, mergeDisabledReason } from "./pr-card-actions.js"; const roots: Root[] = []; +const originalFetch = globalThis.fetch; afterEach(() => { flushSync(() => { for (const root of roots.splice(0)) root.unmount(); }); + document.body.innerHTML = ""; + globalThis.fetch = originalFetch; + vi.restoreAllMocks(); }); describe("mergeDisabledReason", () => { @@ -94,9 +98,71 @@ describe("PrCardActionSlot", () => { expect(tooltip?.getAttribute("title")).toBe("PR mergeability is unknown; refresh to recheck"); expect(button?.getAttribute("title")).toBe("PR mergeability is unknown; refresh to recheck"); }); + + it("renders admin bypass unchecked and sends admin merge payload only when checked", async () => { + const fetchMock = installMergeFetchMock([{ status: 200, body: { ok: true } }]); + const harness = renderActionHarness({ allowedMergeStrategies: ["squash"] }); + + await openMergeMenu(harness.container); + const bypass = adminBypassItem(harness.container); + expect(bypass.getAttribute("aria-checked")).toBe("false"); + expect(bypass.textContent).toContain("Admin bypass"); + + await flushReact(() => bypass.click()); + expect(adminBypassItem(harness.container).getAttribute("aria-checked")).toBe("true"); + await clickStrategy(harness.container, "Squash & merge"); + + expect(mergeBodies(fetchMock)).toEqual([{ strategy: "squash", admin: true }]); + }); + + it("resets admin bypass when the merge menu closes and reopens", async () => { + installMergeFetchMock([{ status: 200, body: { ok: true } }]); + const harness = renderActionHarness({ allowedMergeStrategies: ["squash"] }); + + await openMergeMenu(harness.container); + await flushReact(() => adminBypassItem(harness.container).click()); + await openMergeMenu(harness.container); + await openMergeMenu(harness.container); + + expect(adminBypassItem(harness.container).getAttribute("aria-checked")).toBe("false"); + }); + + it("resets admin bypass when the PR context changes", async () => { + installMergeFetchMock([{ status: 200, body: { ok: true } }]); + const harness = renderActionHarness({ allowedMergeStrategies: ["squash"], number: 42 }); + + await openMergeMenu(harness.container); + await flushReact(() => adminBypassItem(harness.container).click()); + harness.rerender({ allowedMergeStrategies: ["squash"], number: 43 }); + await openMergeMenu(harness.container); + + expect(adminBypassItem(harness.container).getAttribute("aria-checked")).toBe("false"); + }); + + it("resets admin bypass after a failed merge before the next normal merge", async () => { + const fetchMock = installMergeFetchMock([ + { status: 409, body: { ok: false, reason: "gh_error", detail: "merge rejected" } }, + { status: 200, body: { ok: true } }, + ]); + const harness = renderActionHarness({ allowedMergeStrategies: ["squash"] }); + + await openMergeMenu(harness.container); + await flushReact(() => adminBypassItem(harness.container).click()); + await clickStrategy(harness.container, "Squash & merge"); + expect(adminBypassItem(harness.container).getAttribute("aria-checked")).toBe("false"); + await settle(); + await flushReact(() => undefined); + + await clickStrategy(harness.container, "Squash & merge"); + expect(mergeBodies(fetchMock)).toEqual([{ strategy: "squash", admin: true }, { strategy: "squash" }]); + }); }); function renderAction(prOverrides: Partial) { + return renderActionHarness(prOverrides).container; +} + +function renderActionHarness(prOverrides: Partial, workspaceOverrides: Partial = {}) { const rootElement = document.createElement("div"); document.body.appendChild(rootElement); const root = createRoot(rootElement); @@ -107,21 +173,30 @@ function renderAction(prOverrides: Partial) { }); client.setQueryData(["provider-health"], { providerHealth: [providerHealth({ status: "healthy" })] }); - flushSync(() => { + const render = (nextPr: Partial, nextWorkspace: Partial) => { root.render( createElement( QueryClientProvider, { client }, createElement(PrCardActionSlot, { - workspace: workspace(), - pr: pullRequest(prOverrides), + workspace: workspace(nextWorkspace), + pr: pullRequest(nextPr), prTone: "pending", }), ), ); + }; + + flushSync(() => { + render(prOverrides, workspaceOverrides); }); - return rootElement; + return { + container: rootElement, + rerender(nextPr: Partial, nextWorkspace: Partial = workspaceOverrides) { + flushSync(() => render(nextPr, nextWorkspace)); + }, + }; } function providerHealth(overrides: Partial): ProviderHealth { @@ -159,7 +234,7 @@ function pullRequest(overrides: Partial): PullRequestSummary }; } -function workspace(): Workspace { +function workspace(overrides: Partial = {}): Workspace { return { id: "ws_test", repoId: "repo_test", @@ -182,5 +257,64 @@ function workspace(): Workspace { createdAt: "2026-05-31T00:00:00.000Z", updatedAt: "2026-05-31T00:00:00.000Z", archivedAt: null, + ...overrides, }; } + +async function openMergeMenu(container: HTMLElement) { + const mergeButton = container.querySelector("button.pr-card-btn-merge"); + if (!mergeButton) throw new Error("merge button missing"); + await flushReact(() => mergeButton.click()); +} + +async function clickStrategy(container: HTMLElement, label: string) { + const strategy = Array.from(container.querySelectorAll(".pr-card-merge-strategy")).find((button) => + button.textContent?.includes(label), + ); + if (!strategy) throw new Error(`strategy button missing: ${label}`); + await flushReact(() => strategy.click()); +} + +function adminBypassItem(container: HTMLElement): HTMLButtonElement { + const item = container.querySelector('[role="menuitemcheckbox"]'); + if (!item) throw new Error("admin bypass item missing"); + return item; +} + +type FetchMock = ReturnType>>; + +function installMergeFetchMock( + responses: Array<{ status: number; body: unknown }>, +): FetchMock { + let index = 0; + const fetchMock = vi.fn<[RequestInfo | URL, RequestInit?], Promise>(async () => { + const response = responses[Math.min(index, responses.length - 1)] ?? { status: 200, body: { ok: true } }; + index += 1; + return new Response(JSON.stringify(response.body), { + status: response.status, + headers: { "Content-Type": "application/json" }, + }); + }); + globalThis.fetch = fetchMock as typeof fetch; + return fetchMock; +} + +function mergeBodies(fetchMock: FetchMock): unknown[] { + return fetchMock.mock.calls + .filter(([path]) => String(path).includes("/pr-merge")) + .map(([, init]) => JSON.parse(String(init?.body ?? "{}")) as unknown); +} + +async function settle() { + for (let i = 0; i < 10; i += 1) await Promise.resolve(); + await new Promise((resolve) => setTimeout(resolve, 0)); +} + +async function flushReact(callback: () => void | Promise) { + let result: void | Promise = undefined; + flushSync(() => { + result = callback(); + }); + await result; + await settle(); +} diff --git a/apps/web/src/pr-card-actions.tsx b/apps/web/src/pr-card-actions.tsx index d45f18ed..fe451668 100644 --- a/apps/web/src/pr-card-actions.tsx +++ b/apps/web/src/pr-card-actions.tsx @@ -1,8 +1,8 @@ import type { ProviderHealth, PullRequestSummary, Workspace } from "@citadel/contracts"; import type { PrMergeStrategy } from "@citadel/contracts/pr-routes"; import { useMutation, useQuery } from "@tanstack/react-query"; -import { GitMerge, GitPullRequest, Loader2 } from "lucide-react"; -import { useState } from "react"; +import { GitMerge, GitPullRequest, Loader2, ShieldAlert } from "lucide-react"; +import { useEffect, useState } from "react"; import { api, queryClient } from "./api.js"; import { markWorkspacePrMergedInQueryCache } from "./cockpit-tools.js"; import type { PrTone } from "./workspace-card.js"; @@ -16,7 +16,7 @@ export function PrCardActionSlot(props: { workspace: Workspace; pr: PullRequestS const { workspace, pr, prTone } = props; if (prTone === "merged" || prTone === "missing") return null; if (prTone === "conflicting") return ; - return ; + return ; } // Launches a fresh agent session against the daemon's fix-conflicts endpoint. @@ -65,9 +65,13 @@ function FixConflictsButton(props: { workspaceId: string }) { function MergeButton(props: { workspace: Workspace; pr: PullRequestSummary }) { const { workspace, pr } = props; const [open, setOpen] = useState(false); + const [adminBypass, setAdminBypass] = useState(false); const allowed: PrMergeStrategy[] = pr.allowedMergeStrategies.length ? pr.allowedMergeStrategies : (["squash", "merge", "rebase"] as const).filter(() => false); + useEffect(() => { + setAdminBypass(false); + }, [workspace.id, pr.number]); const providerHealth = useQuery<{ providerHealth: ProviderHealth[] }>({ queryKey: ["provider-health"], queryFn: () => api<{ providerHealth: ProviderHealth[] }>("/api/health"), @@ -83,15 +87,18 @@ function MergeButton(props: { workspace: Workspace; pr: PullRequestSummary }) { queryClient.cancelQueries({ queryKey: ["workspaces-pr-batch"] }), ]); }, - mutationFn: (strategy: PrMergeStrategy) => + mutationFn: (input: { strategy: PrMergeStrategy; admin: boolean }) => api(`/api/workspaces/${workspace.id}/pr-merge`, { method: "POST", - body: JSON.stringify({ strategy }), + body: JSON.stringify(input.admin ? { strategy: input.strategy, admin: true } : { strategy: input.strategy }), }), onSuccess: () => { setOpen(false); markWorkspacePrMergedInQueryCache(queryClient, workspace.id, pr.number); }, + onSettled: () => { + setAdminBypass(false); + }, }); const disabledReason = mergeDisabledReason({ allowedMergeStrategies: allowed, @@ -114,7 +121,11 @@ function MergeButton(props: { workspace: Workspace; pr: PullRequestSummary }) { className="pr-card-btn pr-card-btn-merge" onClick={(event) => { event.stopPropagation(); - if (canMerge) setOpen((value) => !value); + if (canMerge) + setOpen((value) => { + setAdminBypass(false); + return !value; + }); }} disabled={!canMerge || merge.isPending} aria-disabled={!canMerge || merge.isPending} @@ -126,6 +137,23 @@ function MergeButton(props: { workspace: Workspace; pr: PullRequestSummary }) { {open && canMerge ? (
+ {allowed.map((strategy) => (