From 5adc86a3289b08538d3aee103e7949f0c5485e76 Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Mon, 19 Oct 2026 14:33:54 +0100 Subject: [PATCH] feat(stage): add the stage approval setting The mirror config gains a top-level `approval` field: `reviewed`, `two-person`, or absent for off. `venfork stage approval` prints the level; `venfork stage approval ` sets it, where `on` means two-person. Turning it off prints a warning. Any other stored value (a level from a newer venfork, or a non-string) is kept on read and by every other config write, and approvalOf refuses it with a ConfigError that suggests `venfork stage approval two-person`, so an older CLI never reads it as off. Only "off" and "" read as off. The setting lives under stage instead of a new top-level command, to keep the command list short. `approval` becomes a reserved stage word like `issue` and `branch`: `venfork stage approval` no longer stages a branch named approval; use `venfork stage branch approval`. The unknown level and branch-flag usage errors name that form. Stage does not enforce the level yet. --- .../references/features/README.md | 1 + .../references/features/approval.md | 42 ++++++++++ .../references/features/stage.md | 2 +- CLAUDE.md | 2 +- docs/reference/commands.md | 21 ++++- docs/reference/json.md | 3 +- src/commands.ts | 1 + src/commands/approval.ts | 56 ++++++++++++++ src/commands/help.ts | 19 ++++- src/config.ts | 50 ++++++++++++ src/dispatch.ts | 5 +- src/index.ts | 4 + src/stage-args.ts | 76 ++++++++++++++---- tests/expected-counts.json | 4 +- tests/integration/approval.test.ts | 65 ++++++++++++++++ tests/unit/config.test.ts | 77 +++++++++++++++++++ tests/unit/docs-drift.test.ts | 3 +- tests/unit/stage-args.test.ts | 63 +++++++++++++++ 18 files changed, 470 insertions(+), 24 deletions(-) create mode 100644 .agents/skills/verify-venfork/references/features/approval.md create mode 100644 src/commands/approval.ts create mode 100644 tests/integration/approval.test.ts diff --git a/.agents/skills/verify-venfork/references/features/README.md b/.agents/skills/verify-venfork/references/features/README.md index bc29954..3b0e36d 100644 --- a/.agents/skills/verify-venfork/references/features/README.md +++ b/.agents/skills/verify-venfork/references/features/README.md @@ -10,6 +10,7 @@ One file per `src/commands/.ts`. Open only the file for the command you ch | `schedule.md` | `venfork schedule` | no | | `stage.md` | `venfork stage [branch] ` | yes, unless `--no-pr` | | `pull-request.md` | `venfork pull pr` | yes | +| `approval.md` | `venfork stage approval` | no | | `issue.md` | `venfork stage issue`, `venfork pull issue` | yes | | `doctor.md` | `venfork doctor` | no (`transport` skips for a non-GitHub origin; `token`, `last-run`, `cron-age` need a schedule, gh and a GitHub origin) | | `workflows.md` | `venfork workflows` | no | diff --git a/.agents/skills/verify-venfork/references/features/approval.md b/.agents/skills/verify-venfork/references/features/approval.md new file mode 100644 index 0000000..adbe8f6 --- /dev/null +++ b/.agents/skills/verify-venfork/references/features/approval.md @@ -0,0 +1,42 @@ +--- +command: approval +entry: src/commands/approval.ts +shared: [json-output] +config: + read: [approval] + written: [approval] +tests: + unit: [tests/unit/stage-args.test.ts, tests/unit/config.test.ts] + integration: [tests/integration/approval.test.ts] + e2e: [] +--- + +## What exists + +- `venfork stage approval` prints the mirror's `approval` level from venfork-config: `off` (field absent), `reviewed` or `two-person`. `off` also prints the warning `The approval gate is off: ...`. +- `venfork stage approval ` writes the level with a leased config push. `on` is `two-person`. `off` deletes the field and prints the same warning. +- Any other stored value (a level from a newer venfork, or a non-string) is kept on read and by every other config write, and refused by `approvalOf` with `ConfigError` (it suggests `venfork stage approval two-person`), so an older CLI never treats it as `off`. Only `"off"` and `""` read as off. +- Usage errors (exit 2): an unknown level (the message names the explicit form `stage branch approval`), a second level, and any branch flag (`--draft`, `--title`, `--dry-run`, `--pr`, ...); the branch-flag message also names `stage branch approval`. `stage approval` needs no gh. +- `--json`: `result` is `{ level }` after the action. + +## How a user reaches it + +`venfork stage approval [on|reviewed|two-person|off] [--json]`. A branch named `approval` needs the explicit form `stage branch approval` (see `stage.md`). No prompt. + +## How to drive it + +```bash +venfork stage approval --json /dev/null | jq -c .result # {"level":"off"} +venfork stage approval on &1 | grep -o "Approval set to two-person" +git -C "$VF_ORIGIN" show venfork-config:.venfork/config.json | jq -r .approval # two-person +venfork stage approval off &1 | grep -o "The approval gate is off" +venfork stage approval maybe `, `venfork stage branch ` (for a branch named `issue` or `branch`), plus `--no-pr`, `--draft`, `--dry-run`, `--title `, `--base `, `--internal-pr `, `--no-update-existing`, `--yes`/`-y`, `--no-input`, `--json`. `stage issue` is in `issue.md`. +`venfork stage `, `venfork stage branch ` (for a branch named `issue`, `approval` or `branch`), plus `--no-pr`, `--draft`, `--dry-run`, `--title `, `--base `, `--internal-pr `, `--no-update-existing`, `--yes`/`-y`, `--no-input`, `--json`. `stage issue` is in `issue.md`; `stage approval` is in `approval.md`. ## How to drive it diff --git a/CLAUDE.md b/CLAUDE.md index 2385f71..2367eef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -56,7 +56,7 @@ CLI entry `src/index.ts` parses `argv[0]` as the command and dispatches via a sw `src/commands/` holds one file per command. Helpers shared by several commands live in `src/shared/`: `net.ts` (network-op safety), `managed-commit.ts` (managed-commit detection), `mirror-commit.ts` (building the managed commit), `divergence.ts`, `redaction.ts`, `worktree.ts` (temporary detached worktrees), `prompt.ts` (every interactive prompt), `ui.ts` (all human output, on stderr), `json-output.ts` (the one `--json` envelope: a command returns its result, `src/index.ts` writes it, `error.code` comes from `VenforkError.code`, and a failed `git`/`gh` subprocess maps to `git`/`github`), `interrupt.ts` (a command that handles SIGINT/SIGTERM itself claims them, so the `--json` signal envelope waits for its rejection), `constants.ts`. The rest are focused modules: -- **`config.ts`** — the source of truth for persisted state. All cross-run state lives in `VenforkConfig` (`.venfork/config.json`) on an **orphan `venfork-config` branch** in the private mirror — never on a working branch. It tracks `upstreamUrl`, `publicForkUrl`, `mode`, `schedule`, `scheduleAuth` (top level; `'app'` or absent for token auth), workflow allow/block lists, the `preserve` allowlist, and link maps (`shippedBranches`, `pulledPrs`, `shippedIssues`, `pulledIssues`). Mutate via `updateVenforkConfig` with a `VenforkConfigPatch` (shallow-merge; `null` deletes an entry or clears a field). Read with `fetchVenforkConfig` (clones only the config branch to a temp dir) or `readVenforkConfigFromRepo`. +- **`config.ts`** — the source of truth for persisted state. All cross-run state lives in `VenforkConfig` (`.venfork/config.json`) on an **orphan `venfork-config` branch** in the private mirror — never on a working branch. It tracks `upstreamUrl`, `publicForkUrl`, `mode`, `schedule`, `scheduleAuth` (top level; `'app'` or absent for token auth), `approval` (top level; `'reviewed'`, `'two-person'` or absent for off, set by `venfork stage approval`), workflow allow/block lists, the `preserve` allowlist, and link maps (`shippedBranches`, `pulledPrs`, `shippedIssues`, `pulledIssues`). Mutate via `updateVenforkConfig` with a `VenforkConfigPatch` (shallow-merge; `null` deletes an entry or clears a field). Read with `fetchVenforkConfig` (clones only the config branch to a temp dir) or `readVenforkConfigFromRepo`. - **`git.ts`** — thin git/gh wrappers (`checkGhAuth`, `getRemotes`, `getDefaultBranch`, `ghRepoExists`, `ghRepoIsForkOf`, …). - **`utils.ts`** — URL/shorthand parsing (`parseOwner`, `parseRepoName`, `parseRepoPath`, `normalizeGitHubRepoInput`); shorthand `owner/repo` is treated as `git@github.com:owner/repo.git`. - **`workflow.ts`** — generates the deterministic GitHub Actions sync YAML (`.github/workflows/venfork-sync.yml`). diff --git a/docs/reference/commands.md b/docs/reference/commands.md index dd2d1d2..1955d6c 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -165,7 +165,7 @@ venfork sync # Sync default branches with upstream ### `venfork stage [--no-pr] [--draft] [--dry-run] [--title ] [--base ] [--internal-pr ] [--no-update-existing] [--yes] [--no-input] [--json]` -Push a branch to the public fork, making it visible and ready for PR to upstream. `venfork stage branch ` is the explicit form of the same command; use it for a branch named `issue` or `branch`. It also opens the upstream PR for you using your internal review PR's body, unless you pass `--no-pr`. With `--json`, stage never prompts and prints one JSON envelope on stdout (see [JSON output](json.md#json-output)). +Push a branch to the public fork, making it visible and ready for PR to upstream. `venfork stage branch ` is the explicit form of the same command; use it for a branch named `issue`, `approval` or `branch`. It also opens the upstream PR for you using your internal review PR's body, unless you pass `--no-pr`. With `--json`, stage never prompts and prints one JSON envelope on stdout (see [JSON output](json.md#json-output)). **⚠️ Important:** This is when your work becomes visible to the client! @@ -252,6 +252,23 @@ The implementation follows the spec at https://example.com/oauth. A comment that mentions venfork but is not a well-formed marker (for example a typo such as `venfork:intenral`), an unmatched close marker, or any `venfork` left in the text after stripping stops the stage before anything is pushed. An unmatched open marker drops everything after it. If you forget to add markers around other internal context, it is sent upstream, so review the preview prompt before confirming. +### `venfork stage approval [on|reviewed|two-person|off] [--yes] [--no-input] [--json]` + +Show or set how much review a branch needs before `venfork stage` publishes it. The level is stored as `approval` in the mirror's venfork settings, so every clone of the mirror reads the same level. With no argument it prints the current level. `venfork stage branch approval` stages a branch named `approval`. + +- `off` - No review needed. This is the default. +- `reviewed` - The branch's PR on the private mirror needs an approving review on its current head from a person (not a bot) with write access to the mirror who is not the PR author, and every check on that head must pass. +- `two-person` - As `reviewed`, and the approver must also be someone other than the GitHub user who runs `venfork stage`. +- `on` - The same as `two-person`. + +Setting `off` prints a warning. The gate is a guardrail against publishing by accident or by an agent, not enforcement: anyone with push access to the public fork or upstream can publish without venfork. With `--json`, `result` is `{ "level" }` after the action. + +```bash +venfork stage approval # print the level +venfork stage approval on # two-person +venfork stage approval reviewed # a solo vendor reviewing an agent's PR +``` + ### `venfork pull pr [--branch-name ] [--no-push] [--yes] [--no-input] [--json]` Pull a third-party upstream PR into the private mirror so your team can review it internally before it lands. The PR's commits land on a new branch (`upstream-pr/` by default) that's pushed to your mirror. With `--json`, it prints one JSON envelope on stdout (see [JSON output](json.md#json-output)). @@ -299,7 +316,7 @@ Move *issue* context between the private mirror and upstream. The command names - `venfork stage issue ` - read an internal triage issue from the mirror, redact `...` blocks (same convention as `stage `), and open the upstream counterpart via `gh issue create`. - `venfork pull issue ` - read an upstream issue **and its comments**, create a parallel internal issue on the mirror titled `[upstream #N] ` so the team can triage it without leaving the private space. The upstream comment thread is snapshotted into the mirror issue body under an "Upstream comments" section. -A branch named `issue` or `branch` cannot use the bare `venfork stage ` form. Run `venfork stage branch issue` instead. +A branch named `issue`, `approval` or `branch` cannot use the bare `venfork stage ` form. Run `venfork stage branch issue` instead. **Flags:** - `--title ` - Override the destination issue's title. diff --git a/docs/reference/json.md b/docs/reference/json.md index 86dd7d0..3e1e694 100644 --- a/docs/reference/json.md +++ b/docs/reference/json.md @@ -15,7 +15,7 @@ When stderr is a terminal, venfork draws styled output with colors and spinners. ## JSON output -Every command accepts `--json`: `setup`, `clone`, `sync`, `schedule`, `stage` (branch and `stage issue`), `pull pr`, `pull issue`, `doctor`, `workflows` and `keep`, with every action of `schedule`, `workflows` and `keep`. With it, stdout gets exactly one JSON object on one line, followed by a newline, also when the command fails. Progress, warnings and errors still go to stderr as usual. The exit code still follows the [exit codes](headless.md#non-interactive-use) table. +Every command accepts `--json`: `setup`, `clone`, `sync`, `schedule`, `stage` (branch, `stage issue` and `stage approval`), `pull pr`, `pull issue`, `doctor`, `workflows` and `keep`, with every action of `schedule`, `workflows` and `keep`. With it, stdout gets exactly one JSON object on one line, followed by a newline, also when the command fails. Progress, warnings and errors still go to stderr as usual. The exit code still follows the [exit codes](headless.md#non-interactive-use) table. `--json` implies `--no-input`: venfork never prompts, even at a terminal. A confirm without `--yes` gives a `missing-input` error and exit 2, and so does a value that only a prompt could give (for example `setup` without ``, or `clone` of a legacy mirror without `--public-fork`). @@ -61,6 +61,7 @@ Any command line that contains `--json` before any `--` gets the envelope, whate - `pull pr`: `{ "branch", "head", "prNumber", "prUrl", "pushed", "refreshed" }`. `refreshed` is true when this PR's copy already existed (the local branch, the mirror copy or a matching `pulledPrs` record), also when its head did not move. `pushed` is false with `--no-push` or a failed push; a failed push exits 1, with or without `--json`. - `stage `: `{ "branch", "head", "target": "public" | "upstream", "compareUrl", "prAction"?, "prUrl"?, "internalPrUrl"? }`. `head` is the rebuilt commit that was pushed. `prAction` is present unless `--no-pr`: `created`, `updated` (an open PR's body was refreshed), `unchanged` (an open PR was left as is) or `failed` (the push worked, the PR could not be opened or its body could not be updated; the run exits 1). - `stage --dry-run`: `{ "dryRun": true, "branch", "head", "target", "compareUrl", "base", "commits", "files", "removedLater", "prTitle"?, "prBody"?, "existingPrUrl"?, "internalPrUrl"? }`. Nothing was pushed: `head` is the preview rebuild's local commit (a real stage rebuilds again, so its SHA differs but its tree is the same), `commits` are its subjects oldest first, and `prTitle`/`prBody` are the redacted PR text that a real stage would publish (absent with `--no-pr`). `existingPrUrl` is the open upstream PR a real stage would update; with it, `prTitle` is absent (the title stays), and with `--no-update-existing` `prBody` is absent too. +- `stage approval` (with or without a level): `{ "level": "off" | "reviewed" | "two-person" }`, the level after the action. - `stage issue` and `pull issue`: `{ "number", "url", "source": { "number", "url" } }`. `number` and `url` are the issue venfork opened; `source` is the issue it was made from. - `doctor`: `{ "checks": [{ "id", "ok": true | false | "skipped", "detail", "fix"? }], "links": { "shippedBranches", "pulledPrs", "shippedIssues", "pulledIssues" } | null, "healthy" }`. `healthy` is false when any check failed. Doctor that ran is `ok: true` even when a check failed; it then exits 1. - `setup`: `{ "mode": "standard" | "no-public", "mirror": { "repo", "url" }, "publicFork"?: { "repo", "url" }, "upstream": { "repo", "url" }, "directory" }`. `repo` is `owner/name` and `url` its `https://github.com/` page. `publicFork` is absent in no-public mode. `directory` is the absolute path of the local mirror checkout. diff --git a/src/commands.ts b/src/commands.ts index 441eca7..18ad864 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -1,3 +1,4 @@ +export { type ApprovalResult, approvalCommand } from './commands/approval.js'; export { cloneCommand } from './commands/clone.js'; export { type DoctorCheck, diff --git a/src/commands/approval.ts b/src/commands/approval.ts new file mode 100644 index 0000000..d838588 --- /dev/null +++ b/src/commands/approval.ts @@ -0,0 +1,56 @@ +import { + type ApprovalLevel, + approvalOf, + configNotFoundError, + readVenforkConfigFromRepo, + updateVenforkConfig, +} from '../config.js'; +import * as p from '../shared/ui.js'; + +/** The approval level after `venfork stage approval`, reported as the `--json` result. */ +export interface ApprovalResult { + level: ApprovalLevel; +} + +const MEANING: Record = { + off: 'venfork stage publishes a branch without a review on its mirror PR.', + reviewed: + 'venfork stage needs an approving review on the mirror PR head from a person with write access who is not the PR author, and passing checks.', + 'two-person': + 'venfork stage needs an approving review on the mirror PR head from a person with write access who is not the PR author and not the person running stage, and passing checks.', +}; + +/** The warning printed whenever the gate is turned off. */ +export const APPROVAL_OFF_WARNING = + 'The approval gate is off: venfork stage can publish any branch, from any person or agent with access to this clone, without a review. Turn it back on with `venfork stage approval on`.'; + +/** + * `venfork stage approval [level]`: prints the mirror's approval level, or + * sets it in venfork-config. Turning the gate off prints a warning. + * @returns The level after the action, for `--json`. + */ +export async function approvalCommand( + level?: ApprovalLevel +): Promise { + p.intro('🛂 Venfork Stage Approval'); + const repoDir = process.cwd(); + try { + if (level === undefined) { + const config = await readVenforkConfigFromRepo(repoDir); + if (!config) throw configNotFoundError(); + const current = approvalOf(config); + p.note(`${current}: ${MEANING[current]}`, 'Approval'); + if (current === 'off') p.log.warn(APPROVAL_OFF_WARNING); + p.outro('✨ Approval level shown'); + return { level: current }; + } + const updated = await updateVenforkConfig(repoDir, { approval: level }); + const now = approvalOf(updated); + p.note(`${now}: ${MEANING[now]}`, 'Approval'); + if (now === 'off') p.log.warn(APPROVAL_OFF_WARNING); + p.outro(`✨ Approval set to ${now}`); + return { level: now }; + } catch (error) { + throw p.reportFailure(error); + } +} diff --git a/src/commands/help.ts b/src/commands/help.ts index 1d70c59..d1f6d49 100644 --- a/src/commands/help.ts +++ b/src/commands/help.ts @@ -92,8 +92,9 @@ const COMMAND_HELP: ReadonlyArray = [ `venfork stage [--no-pr] [--draft] [--dry-run] [--title ] [--base ] [--internal-pr ] [--no-update-existing] [--yes] [--no-input] [--json] venfork stage branch [same options] venfork stage issue [--title ] [--yes] [--no-input] [--json] +venfork stage approval [on|reviewed|two-person|off] [--yes] [--no-input] [--json] Push a branch (or an issue) from the mirror outward, to the public fork or upstream - Branch names \`issue\` and \`branch\` need the explicit form: venfork stage branch issue + Branch names \`issue\`, \`approval\` and \`branch\` need the explicit form: venfork stage branch issue Branch: push to public fork for PR to upstream Rebuilds the branch as linear history on upstream (merges and venfork's sync commit dropped, new SHAs) and refuses commits that carry mirror-only files or @@ -114,6 +115,8 @@ venfork stage issue [--title ] [--yes] [--no-input] [--jso • --json Print one JSON envelope on stdout; implies --no-input Issue: read the internal mirror issue, strip venfork:internal blocks, open the upstream counterpart, record the link + Approval: show or set the review a branch needs on its mirror PR before stage + publishes it (see venfork stage approval --help) This is when your work becomes visible to the client`, ], [ @@ -149,6 +152,20 @@ venfork pull issue [--title ] [--yes] [--no-input] [--json ]; const SUBCOMMAND_HELP: ReadonlyArray = [ + [ + 'stage approval', + `venfork stage approval [on|reviewed|two-person|off] [--yes] [--no-input] [--json] + Show (no argument) or set the approval the mirror PR of a branch needs + before venfork stage publishes it. Stored in the mirror's venfork settings + • off No review needed (the default) + • reviewed An approving review on the PR head from a person with write + access who is not the PR author, and passing checks + • two-person As reviewed, and the approver is not the person running stage + • on Same as two-person + A guardrail against accidental or agent-driven publishing, not enforcement: + anyone who can push can publish without venfork + --json prints { level } after the action as one JSON envelope on stdout`, + ], [ 'pull pr', `venfork pull pr [--branch-name ] [--no-push] [--yes] [--no-input] [--json] diff --git a/src/config.ts b/src/config.ts index 43ae7ad..bb05e68 100644 --- a/src/config.ts +++ b/src/config.ts @@ -91,6 +91,13 @@ export interface VenforkConfig { * is a mode from a newer venfork, kept as-is so a read does not fail. */ scheduleAuth?: string; + /** + * Approval stage needs on the mirror PR before it publishes a branch: + * `'reviewed'` or `'two-person'`; absent means `'off'`. Any other value + * (a level from a newer venfork, or a corrupt one) is kept as-is so a read + * does not fail and `approvalOf` refuses it. + */ + approval?: unknown; enabledWorkflows?: string[]; disabledWorkflows?: string[]; /** @@ -158,6 +165,35 @@ export function scheduleAuthOf( ); } +/** How much review `venfork stage` needs on the mirror PR before it publishes. */ +export type ApprovalLevel = 'off' | 'reviewed' | 'two-person'; + +/** Every approval level, weakest first. */ +export const APPROVAL_LEVELS: readonly ApprovalLevel[] = [ + 'off', + 'reviewed', + 'two-person', +]; + +/** + * The approval level of `config`; `'off'` when it is unset. + * + * @param config Venfork config, or `null` when the mirror has none. + * @throws ConfigError when the level is one this version does not know, so + * an older venfork never stages past a gate it cannot read. + */ +export function approvalOf( + config: Pick | null | undefined +): ApprovalLevel { + const level = config?.approval; + if (level === undefined) return 'off'; + const known = APPROVAL_LEVELS.find((candidate) => candidate === level); + if (known) return known; + throw new ConfigError( + `approval ${JSON.stringify(level)} is not supported by this venfork version. Upgrade venfork, or run: venfork stage approval two-person` + ); +} + const CONFIG_BRANCH = 'venfork-config'; /** The error for a command that needs `venfork-config` when origin has none. */ @@ -176,6 +212,7 @@ export type VenforkConfigPatch = Omit< Partial, | 'schedule' | 'scheduleAuth' + | 'approval' | 'invalidPreserve' | 'enabledWorkflows' | 'disabledWorkflows' @@ -189,6 +226,8 @@ export type VenforkConfigPatch = Omit< schedule?: VenforkConfig['schedule'] | null; /** `null` (or `'token'`) returns to token auth; omitted keeps the current mode. */ scheduleAuth?: ScheduleAuth | null; + /** `null` (or `'off'`) turns the approval gate off; omitted keeps the level. */ + approval?: ApprovalLevel | null; /** Replaces the whole allowlist; `null` clears it. */ enabledWorkflows?: string[] | null; /** Replaces the whole block list; `null` clears it. */ @@ -579,6 +618,10 @@ function normalizeConfig( delete normalized.scheduleAuth; } + if (normalized.approval === '' || normalized.approval === 'off') { + delete normalized.approval; + } + if (normalized.schedule) { const cron = typeof normalized.schedule.cron === 'string' @@ -919,6 +962,7 @@ export function applyPatchAndNormalize( shippedIssues: _shippedIssuesPatch, pulledIssues: _pulledIssuesPatch, scheduleAuth: scheduleAuthPatch, + approval: approvalPatch, ...basePatch } = patch; @@ -942,6 +986,12 @@ export function applyPatchAndNormalize( merged.scheduleAuth = scheduleAuthPatch; } + if (approvalPatch === null || approvalPatch === 'off') { + delete merged.approval; + } else if (approvalPatch !== undefined) { + merged.approval = approvalPatch; + } + if (patch.enabledWorkflows === null) { delete merged.enabledWorkflows; } else if (patch.enabledWorkflows !== undefined) { diff --git a/src/dispatch.ts b/src/dispatch.ts index 3bd529e..7744acb 100644 --- a/src/dispatch.ts +++ b/src/dispatch.ts @@ -44,7 +44,7 @@ export async function assertInRepository( * fail fast when gh is not authenticated. `pull` needs gh once its arguments parse, so a usage error * is reported first. `stage` needs gh for * `stage issue` and for a branch stage, which opens the upstream PR unless - * `--no-pr`. + * `--no-pr`; `stage approval` reads and writes only venfork-config. */ export function requiresGhAuth(command: string, args: string[]): boolean { if (ALWAYS_GH.has(command)) return true; @@ -60,7 +60,8 @@ export function requiresGhAuth(command: string, args: string[]): boolean { try { const parsed = parseStageCliArgs(args); return ( - parsed.kind === 'issue' || Boolean(parsed.createPr || parsed.draft) + parsed.kind === 'issue' || + (parsed.kind === 'branch' && Boolean(parsed.createPr || parsed.draft)) ); } catch { return false; diff --git a/src/index.ts b/src/index.ts index b8c9889..81dca61 100644 --- a/src/index.ts +++ b/src/index.ts @@ -3,6 +3,7 @@ import { parseCloneCliArgs } from './clone-args.js'; import { commandHelp, helpTopic } from './commands/help.js'; import { + approvalCommand, cloneCommand, doctorCommand, isHealthy, @@ -133,6 +134,9 @@ function parseCommand( } case 'stage': { const parsed = parseStageCliArgs(args); + if (parsed.kind === 'approval') { + return resultOf(() => approvalCommand(parsed.level)); + } if (parsed.kind === 'issue') { return resultOf(() => issueCommand('stage', parsed.ref, { diff --git a/src/stage-args.ts b/src/stage-args.ts index 2c95c12..7026a53 100644 --- a/src/stage-args.ts +++ b/src/stage-args.ts @@ -1,3 +1,4 @@ +import type { ApprovalLevel } from './config.js'; import { UsageError } from './errors.js'; import { consumeValue, @@ -54,7 +55,32 @@ export type ParsedStageIssueArgs = { json: boolean; }; -export type ParsedStageArgs = ParsedStageBranchArgs | ParsedStageIssueArgs; +/** Parsed `venfork stage approval [level]` arguments. */ +export type ParsedStageApprovalArgs = { + kind: 'approval'; + /** The level to set; absent prints the current level. `on` is `two-person`. */ + level?: ApprovalLevel; + /** `--yes` / `--no-input`. */ + prompt: PromptContext; + /** Print the `--json` envelope on stdout. */ + json: boolean; +}; + +export type ParsedStageArgs = + | ParsedStageBranchArgs + | ParsedStageIssueArgs + | ParsedStageApprovalArgs; + +/** Usage line for `venfork stage approval`. */ +export const STAGE_APPROVAL_USAGE = + 'venfork stage approval [on|reviewed|two-person|off] [--yes] [--no-input] [--json]'; + +const APPROVAL_WORDS: Record = { + on: 'two-person', + 'two-person': 'two-person', + reviewed: 'reviewed', + off: 'off', +}; /** Usage line for `venfork stage `. */ export const STAGE_USAGE = @@ -62,9 +88,9 @@ export const STAGE_USAGE = /** * Parse `venfork stage ...` argv after the `stage` token. - * Forms: `stage `, `stage branch `, `stage issue `. - * The first positional selects the form, so a branch named `issue` or - * `branch` must use `stage branch `. + * Forms: `stage `, `stage branch `, `stage issue `, + * `stage approval [level]`. The first positional selects the form, so a + * branch named `issue`, `approval` or `branch` must use `stage branch `. */ export function parseStageCliArgs(stageArgs: string[]): ParsedStageArgs { let prFlag: string | undefined; @@ -124,21 +150,45 @@ export function parseStageCliArgs(stageArgs: string[]): ParsedStageArgs { }); const [first, ...rest] = positional; + const branchFlags = [ + dryRun && '--dry-run', + prFlag, + noPr && '--no-pr', + draft && '--draft', + base !== undefined && '--base', + internalPrNumber !== undefined && '--internal-pr', + noUpdateExisting && '--no-update-existing', + ].filter((flag): flag is string => typeof flag === 'string'); + if (first === 'approval') { + const used = [...branchFlags, ...(titleFlag ? [titleFlag] : [])]; + if (used.length > 0) { + throw new UsageError( + `${used.join(', ')} only applies to stage branch, not stage approval. To stage a branch named 'approval', run \`venfork stage branch approval\`.` + ); + } + if (rest.length > 1) + throw unexpectedArgument(rest[1], STAGE_APPROVAL_USAGE); + const word = rest[0]; + const level = word === undefined ? undefined : APPROVAL_WORDS[word]; + if (word !== undefined && level === undefined) { + throw new UsageError( + `Unknown approval level '${word}'. Usage: ${STAGE_APPROVAL_USAGE}. To stage a branch named 'approval', run \`venfork stage branch approval\`.` + ); + } + return { + kind: 'approval', + ...(level ? { level } : {}), + prompt: promptFlags.prompt, + json: promptFlags.json, + }; + } if (first === 'issue') { if (rest.length === 0) { throw new UsageError( "venfork stage issue requires an issue number or URL. To stage a branch named 'issue', run `venfork stage branch issue`." ); } - const branchOnly = [ - dryRun && '--dry-run', - prFlag, - noPr && '--no-pr', - draft && '--draft', - base !== undefined && '--base', - internalPrNumber !== undefined && '--internal-pr', - noUpdateExisting && '--no-update-existing', - ].filter((flag): flag is string => typeof flag === 'string'); + const branchOnly = branchFlags; if (branchOnly.length > 0) { throw new UsageError( `${branchOnly.join(', ')} only applies to stage branch, not stage issue` diff --git a/tests/expected-counts.json b/tests/expected-counts.json index e9bef39..16991c1 100644 --- a/tests/expected-counts.json +++ b/tests/expected-counts.json @@ -1,4 +1,4 @@ { - "unit": 1260, - "integration": 474 + "unit": 1272, + "integration": 477 } diff --git a/tests/integration/approval.test.ts b/tests/integration/approval.test.ts new file mode 100644 index 0000000..106145a --- /dev/null +++ b/tests/integration/approval.test.ts @@ -0,0 +1,65 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test'; +import * as prompts from '@clack/prompts'; +import { quietPrompts } from '../harness/prompts.js'; + +mock.module('@clack/prompts', quietPrompts); + +import { approvalCommand } from '../../src/commands.js'; +import { + createMirrorFixture, + type MirrorFixture, +} from '../harness/mirror-fixture.js'; + +let fx: MirrorFixture; +const originalCwd = process.cwd(); + +beforeEach(async () => { + fx = await createMirrorFixture(); + process.chdir(fx.work); +}); + +afterEach(async () => { + process.chdir(originalCwd); + await fx.cleanup(); +}); + +async function storedApproval(): Promise { + const raw = await fx.fileAt( + fx.origin, + 'venfork-config', + '.venfork/config.json' + ); + return JSON.parse(raw ?? '{}').approval; +} + +describe('stage approval', () => { + test('a mirror with no approval field reads as off and warns', async () => { + expect(await approvalCommand()).toEqual({ level: 'off' }); + expect(prompts.log.warn).toHaveBeenCalledWith( + expect.stringContaining('The approval gate is off'), + expect.anything() + ); + }); + + test('setting a level writes it to venfork-config on origin', async () => { + expect(await approvalCommand('reviewed')).toEqual({ level: 'reviewed' }); + expect(await storedApproval()).toBe('reviewed'); + + expect(await approvalCommand('two-person')).toEqual({ + level: 'two-person', + }); + expect(await storedApproval()).toBe('two-person'); + expect(await approvalCommand()).toEqual({ level: 'two-person' }); + }); + + test('off removes the field and prints the warning', async () => { + await approvalCommand('two-person'); + + expect(await approvalCommand('off')).toEqual({ level: 'off' }); + expect(await storedApproval()).toBeUndefined(); + expect(prompts.log.warn).toHaveBeenCalledWith( + expect.stringContaining('venfork stage approval on'), + expect.anything() + ); + }); +}); diff --git a/tests/unit/config.test.ts b/tests/unit/config.test.ts index 86cae80..7fa63d1 100644 --- a/tests/unit/config.test.ts +++ b/tests/unit/config.test.ts @@ -100,6 +100,7 @@ import { preserveRemoveHint, readVenforkConfigFromRepo, scheduleAuthOf, + approvalOf, updateVenforkConfig, } from '../../src/config.js'; import { ConfigError } from '../../src/errors.js'; @@ -733,3 +734,79 @@ describe('scheduleAuth', () => { ); }); }); + +describe('approval', () => { + test('approvalOf is off when unset and returns a known level', () => { + expect(approvalOf(null)).toBe('off'); + expect(approvalOf({})).toBe('off'); + expect(approvalOf({ approval: 'reviewed' })).toBe('reviewed'); + expect(approvalOf({ approval: 'two-person' })).toBe('two-person'); + }); + + test('approvalOf refuses a level from a newer venfork instead of reading it as off', () => { + expect(() => approvalOf({ approval: 'three-person' })).toThrow(ConfigError); + expect(() => approvalOf({ approval: 'three-person' })).toThrow( + 'run: venfork stage approval two-person' + ); + }); + + test('approvalOf refuses a stored value that is not a string', () => { + for (const approval of [1, null, true, { level: 'off' }]) { + expect(() => approvalOf({ approval })).toThrow(ConfigError); + } + }); + + test('reading keeps a level, drops off, and keeps an unknown level', async () => { + for (const [stored, read] of [ + ['two-person', 'two-person'], + ['reviewed', 'reviewed'], + ['three-person', 'three-person'], + ['off', undefined], + ['', undefined], + [1, 1], + [null, null], + [{ level: 'off' }, { level: 'off' }], + ] as const) { + mockResponses.set( + 'git show FETCH_HEAD:.venfork/config.json', + mockReadResponse({ ...baseConfig, approval: stored }) + ); + const config = await readVenforkConfigFromRepo('/tmp/repo'); + expect(config?.approval).toEqual(read); + } + }); + + test('an unrelated patch keeps a stored value approvalOf cannot read', async () => { + mockResponses.set( + 'git show FETCH_HEAD:.venfork/config.json', + mockReadResponse({ ...baseConfig, approval: 1 }) + ); + const updated = await updateVenforkConfig('/tmp/repo', { + enabledWorkflows: ['ci.yml'], + }); + expect(updated.approval).toBe(1); + const written = JSON.parse(writeFileCalls.at(-1)?.content ?? '{}'); + expect(written.approval).toBe(1); + expect(() => approvalOf(updated)).toThrow(ConfigError); + }); + + test('a patch sets the level, and off or null removes it', async () => { + mockResponses.set( + 'git show FETCH_HEAD:.venfork/config.json', + mockReadResponse({ ...baseConfig }) + ); + expect( + (await updateVenforkConfig('/tmp/repo', { approval: 'reviewed' })) + .approval + ).toBe('reviewed'); + + mockResponses.set( + 'git show FETCH_HEAD:.venfork/config.json', + mockReadResponse({ ...baseConfig, approval: 'two-person' }) + ); + for (const approval of ['off', null] as const) { + const updated = await updateVenforkConfig('/tmp/repo', { approval }); + expect(updated).not.toHaveProperty('approval'); + } + }); +}); diff --git a/tests/unit/docs-drift.test.ts b/tests/unit/docs-drift.test.ts index 2ce0b50..d153403 100644 --- a/tests/unit/docs-drift.test.ts +++ b/tests/unit/docs-drift.test.ts @@ -254,7 +254,8 @@ describe('the user docs match the CLI contract', () => { /(?:\bvenfork|\bvf_yes|"\$VF_CLI")\s+stage\s+([^\s`;|<>&#)]+)/g )) { const target = m[1] ?? ''; - if (target === 'issue' || target.startsWith('-')) continue; + if (target === 'issue' || target === 'approval') continue; + if (target.startsWith('-')) continue; const rest = line.slice(m.index ?? 0).split(/[;|`]|&&/)[0] ?? ''; if (/<[a-z-]+>|\[/.test(rest)) continue; if (rest.trim() === 'venfork stage branch issue') continue; diff --git a/tests/unit/stage-args.test.ts b/tests/unit/stage-args.test.ts index 4d1ece2..131cc76 100644 --- a/tests/unit/stage-args.test.ts +++ b/tests/unit/stage-args.test.ts @@ -334,3 +334,66 @@ describe('parseStageCliArgs prompt flags', () => { }); }); }); + +describe('parseStageCliArgs: stage approval', () => { + test('bare `stage approval` asks for the current level', () => { + expect(parseStageUnion(['approval'])).toEqual({ + kind: 'approval', + prompt: { yes: false, noInput: false }, + json: false, + }); + }); + + test('`on` means two-person; each level parses as itself', () => { + for (const [word, level] of [ + ['on', 'two-person'], + ['two-person', 'two-person'], + ['reviewed', 'reviewed'], + ['off', 'off'], + ] as const) { + expect(parseStageUnion(['approval', word, '--json'])).toEqual({ + kind: 'approval', + level, + prompt: { yes: false, noInput: true }, + json: true, + }); + } + }); + + test('an unknown level is a usage error naming the explicit branch form', () => { + expect(() => parseStageUnion(['approval', 'maybe'])).toThrow( + "Unknown approval level 'maybe'" + ); + expect(() => parseStageUnion(['approval', 'maybe'])).toThrow( + 'venfork stage branch approval' + ); + }); + + test('branch flags and a second level are usage errors', () => { + expect(() => parseStageUnion(['approval', 'on', '--draft'])).toThrow( + '--draft only applies to stage branch, not stage approval' + ); + expect(() => parseStageUnion(['approval', '--title', 'x'])).toThrow( + '--title only applies to stage branch, not stage approval' + ); + expect(() => parseStageUnion(['approval', 'on', 'off'])).toThrow( + "Unexpected argument 'off'" + ); + }); + + test('a branch flag names the explicit form for a branch named approval', () => { + for (const argv of [ + ['approval', 'on', '--pr'], + ['approval', '--draft'], + ['approval', '--title', 'x'], + ]) { + expect(() => parseStageUnion(argv)).toThrow( + "To stage a branch named 'approval', run `venfork stage branch approval`." + ); + } + }); + + test('`stage branch approval` stages a branch named approval', () => { + expect(parseStageCliArgs(['branch', 'approval']).branch).toBe('approval'); + }); +});