From 6cb59e812c785ee2f9bc4f430aa4302f4deefb9c Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Wed, 15 Jul 2026 15:05:18 +0300 Subject: [PATCH] fix: close post-merge safety gaps --- README.md | 7 +- docs/agentic-dev-kit/workflows/parallel.md | 81 ++- docs/agentic-dev-kit/workflows/pr-watch.md | 53 +- docs/autonomous-session-playbook.md | 14 +- docs/parallel-dev.md | 39 +- docs/parallel-howto.md | 47 +- init.sh | 101 ++++ scripts/archive_plan_sessions.py | 109 +++- scripts/dev_session.sh | 218 +++++-- scripts/lib/repo_root.sh | 36 ++ scripts/pr_watch.py | 288 ++++++++- scripts/reconcile_sessions.sh | 18 +- scripts/tests/test_portability.py | 657 ++++++++++++++++++++- scripts/tests/test_pr_watch.py | 258 ++++++++ 14 files changed, 1744 insertions(+), 182 deletions(-) create mode 100644 scripts/tests/test_pr_watch.py diff --git a/README.md b/README.md index d21b744..da57aaf 100644 --- a/README.md +++ b/README.md @@ -189,9 +189,10 @@ branch, and `DEVKIT_STATE_ROOT` state sandbox. The rule that makes it safe is edited by both (the sandbox prevents *state* collisions, not *source* merge conflicts). The flow: `parallel plan` clusters candidate work by footprint → launch a lane per -disjoint cluster (`scripts/dev_session.sh new`) → each lane works to a draft-green PR → -the cockpit reconciles every lane and merges. Each lane gets an effort tier and a merge -class (self-merge vs operator-merge) assigned at plan time. +disjoint cluster (`scripts/dev_session.sh new … --merge-class self|operator`) → each +lane works to a draft-green PR → the cockpit reconciles every lane and completes the +recorded merge path. A self-merge must go through `scripts/dev_session.sh merge`; an +operator-merge remains an explicit cockpit decision. Full walkthrough — the lane contract, the live board, reconciliation, and a worked example — in **[`docs/parallel-dev.md`](docs/parallel-dev.md)**. For step-by-step diff --git a/docs/agentic-dev-kit/workflows/parallel.md b/docs/agentic-dev-kit/workflows/parallel.md index ff58760..f85af93 100644 --- a/docs/agentic-dev-kit/workflows/parallel.md +++ b/docs/agentic-dev-kit/workflows/parallel.md @@ -102,8 +102,8 @@ deliberately: not at merge time, stops a batch stalling on ad-hoc "can I merge this?" calls and tells each lane whether it may self-merge or must hand back. -1. **Launch each + relay a kickoff.** Run `/dev_session.sh new ` per - chosen ticket (see below) and relay each copy-paste line **with a kickoff prompt** +1. **Launch each + relay a kickoff.** Run `/dev_session.sh new + --merge-class ` per chosen ticket (see below) and relay each copy-paste line **with a kickoff prompt** the operator pastes as the session's first message: > Read tracker ticket `` (+ any recipe in ``). Pre-flight its @@ -156,16 +156,24 @@ decision (and so an autonomous batch knows what it may close itself): | **Mechanical** / **Standard** | **self-merge** *(autonomous/headless batches only)* | the lane (or cockpit), once green-and-clean with one independent review pass | | **High-stakes** | **operator-merge** | always the operator — never self-merged, even when green | -Operator-merge is the floor for anything in your project's high-risk classes: +The class is persisted in the session metadata and headless JSON descriptor; +missing/unknown metadata defaults to **operator**. Operator-merge is the floor for anything in your project's high-risk classes: data-shape / fetcher / config-semantics changes, shared primitives, a **guard / gate / verifier / send-path / kill-path** (see `docs/agentic-dev-kit/safety-critical-changes.md`), security, PII, or anything touching production cron/CI or shared `state/`. Normal **interactive** sessions leave *all* merges to the operator regardless of class — self-merge is an -autonomous-session behavior (see - the configured autonomous-session playbook (`paths.playbook`). +autonomous-session behavior (see the configured autonomous-session playbook at +`paths.playbook`). When unsure, classify **operator-merge**. +For an autonomous self-merge, run `/dev_session.sh merge `. +That deterministic wrapper re-polls `pr-watch` at act time, requires current-head +independent-review evidence, validates the exact repository/branch/base, and pins the +merge to the reviewed head commit. It refuses any lane whose persisted class is not +`self`. Operator-class lanes intentionally cannot use the wrapper; the operator lands +those directly after the required review and sign-off. + **How the tier reaches the lane.** It depends on the launch mechanism: - **Headless lanes — fan out through a multi-agent workflow when it exposes a real @@ -257,7 +265,7 @@ in-agent workflow cannot `cd` the operator into a new worktree and open a fresh terminal there). When asked to prepare one, run: ```bash -/dev_session.sh new +/dev_session.sh new --merge-class ``` substituting a lowercase slug for `` (e.g. `feat-graduation-flow`). Pass @@ -266,9 +274,11 @@ override it for this lane. The script prints a copy-paste line — `cd && export DEVKIT_STATE_ROOT=… && export DEVKIT_ROOT=… && `. **Relay that line to the operator** and tell them to run it in a new terminal; don't try to start the session yourself. Options: -`--base ` (default `main`), `--prefix

` (default `dev` — parallel-session +`--base ` (default `vcs.protected_branch`), `--prefix

` (default +`vcs.dev_branch_prefix` — parallel-session branches get their own namespace to avoid colliding with hand-named feature branches), -`--branch ` to override the whole name. +`--branch ` to override the whole name. Omitting `--merge-class` fails safe to +`operator`. ### Unattended / headless launch — `new --headless` @@ -278,7 +288,7 @@ rule above says *don't start the session yourself*). That's the wrong shape for *sandboxed* lane without a human in the loop. `--headless` is for exactly that: ```bash -/dev_session.sh new --headless +/dev_session.sh new --headless --merge-class ``` It creates the worktree + sandbox exactly as `new` does, but instead of the human @@ -294,10 +304,13 @@ block it: it's unaffected.) 1. **Prints a JSON descriptor to stdout** (diagnostics go to stderr, so stdout is clean JSON): `{"scope","branch","worktree","state_root","repo_root","base", - "prompt_preamble","env","runtime","launcher"}`. `prompt_preamble` is the canonical lane-contract text + "merge_class","prompt_preamble","env","runtime","launcher"}`. `prompt_preamble` is the canonical lane-contract text below — the launcher **MUST** prepend it verbatim to the lane's task prompt. `env` - (currently `{"DEVKIT_REFUSE_UNSANDBOXED_STATE": "1"}`) flips the unsandboxed-write - guard from *warn* to *refuse*, by default, for every headless lane — so a lane + carries lane-specific `DEVKIT_STATE_ROOT`, `DEVKIT_ROOT`, and + `DEVKIT_REFUSE_UNSANDBOXED_STATE=1`. The launcher **MUST replace inherited values + with this map**: the resolver gives an explicit env root precedence over the marker, + so inheriting the cockpit's root would collapse every child lane into one sandbox. + The refusal flag flips the unsandboxed-write guard from *warn* to *refuse* — so a lane whose marker resolution somehow fails (deleted marker, cwd escaped the worktree) hard-errors on a `state/` write instead of silently landing in prod. Interactive `new` and cron/CI never set either field. @@ -324,14 +337,14 @@ every lane prompt**; runtime capability changes how tiers are applied, not wheth the safety contract binds. **Preferred when available — a workflow launcher with a real effort dial.** Run -`new --headless ` once per chosen scope, collect each one's JSON descriptor +`new --headless --merge-class ` once per chosen scope, collect each one's JSON descriptor into a list (attaching the per-lane `effort`/`model` tier from the plan's risk read), then drive the lanes from a *single* fan-out that gives each sub-agent its own `{effort, model}` — the one path on which the tier's `effort` half actually takes effect. Pseudocode: ```js -// args.lanes = [{scope, worktree, branch, ticket, effort, model, prompt_preamble}, …] +// args.lanes = [{scope, worktree, branch, ticket, effort, model, merge_class, prompt_preamble, env}, …] // — one per `new --headless` descriptor (prompt_preamble copied straight off it). // effort ∈ low|medium|high|max (omit ⇒ inherit cockpit effort); model ∈ cheap|default|expensive (omit ⇒ inherit). runInParallel(args.lanes.map(lane => () => @@ -339,30 +352,28 @@ runInParallel(args.lanes.map(lane => () => `${lane.prompt_preamble}\n\n` + `Work in worktree ${lane.worktree} on branch ${lane.branch} (cd there first — its state sandbox is active via the on-disk marker, so your state/ writes isolate automatically). ` + `Read tracker ticket ${lane.ticket}, pre-flight its premise against the live code, implement, draft PR on first push, drive it to green-and-clean, then hand off per the contract above.`, - { label: lane.scope, effort: lane.effort, model: lane.model } // omit effort/model to inherit (default-safe) + { label: lane.scope, effort: lane.effort, model: lane.model, env: lane.env } ) )) ``` -Four things to keep right: **(1)** `lane.prompt_preamble` is prepended verbatim, +Five things to keep right: **(1)** `lane.prompt_preamble` is prepended verbatim, ahead of everything else, on every lane — never abbreviated to "follow the usual contract" (that's exactly the prose-reference that failed to bind a lane before). **(2)** do **not** open a second worktree on top of `--headless` — it already owns the worktree+sandbox, so a second one would have no marker and lose isolation. **(3)** A lane with no assigned tier omits `effort`/`model` and inherits the cockpit's — the -same default-safe fallback as everywhere else. **(4)** Check what compute budget your +same default-safe fallback as everywhere else. **(4)** replace the spawned process's +environment roots with `lane.env`; do not merge them over inherited cockpit roots. +**(5)** Check what compute budget your fan-out mechanism draws from before running a large batch, and monitor it via whatever live-progress view your runtime exposes, plus `list --watch` on the lanes' branches/PRs. -Note on the `env` field: if your fan-out or background-agent tool doesn't expose a -parameter to inject env vars into the spawned lane's process, `DEVKIT_REFUSE_ -UNSANDBOXED_STATE` cannot be force-set through that launch path the way -`prompt_preamble` can be force-injected into the prompt text. In the normal case this -is moot: the marker already resolves the sandbox, so the guard never fires. The `env` -field exists for launchers that DO control the spawned process's environment (a -custom subprocess-based batch driver) — read it and export it there. Treat this as a -documented, conservative gap, not a silent one. +The `env` field is mandatory for unattended launches. If a fan-out/background-agent +tool cannot replace the spawned process's environment, do not use it for a headless +state-writing lane; use an env-capable subprocess/fresh terminal or keep the work +attended. Prompt injection cannot override an inherited `DEVKIT_STATE_ROOT`. **Fallback — a single background sub-agent per lane (model-only).** Parse the descriptor and spawn a background sub-agent whose prompt is **the `prompt_preamble` @@ -392,7 +403,25 @@ interactive paths are unaffected. ## Finishing a session -After its PR has merged: +From the cockpit, drive a lane's review loop through the scope-aware wrapper so its +seen-set and receipt land in the same sandbox the merge gate reads: + +```bash +/dev_session.sh pr-watch --json +/dev_session.sh pr-watch --mark-seen +/dev_session.sh pr-watch --record-review --head +``` + +The receipt command uses the `head` from the exact poll whose diff was independently +reviewed and refuses if a push landed before recording. For a `self` lane, once this +scope-aware `pr-watch` has converged, merge through the deterministic class gate: + +```bash +/dev_session.sh merge +``` + +For an `operator` lane, the wrapper refuses; the operator performs the reviewed, +signed-off merge directly. After its PR has merged: ```bash /dev_session.sh rm diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index fd65e91..e1061a8 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -20,16 +20,23 @@ Engine: `/pr_watch.py` (deterministic — check rollup + comment uni issue/review/inline surfaces, noise-filtered, diffed against a per-PR seen-set). You drive the loop + apply the judgment. +For a lane coordinated from the cockpit, invoke the same engine through +`/dev_session.sh pr-watch ...`. That scope-aware wrapper pins the +repository and stores polls, acknowledgments, and review receipts in the lane sandbox +that `dev_session.sh merge ` re-checks. + ## Loop Repeat until the report says **done**: 1. **Poll.** `uv run /pr_watch.py --json` (omit `` for the current - branch). Read `done`, `checks` (`all_green`, `failing[]`, `pending`), and - `new_comments[]`. + branch). Read `done`, `checks` (`all_green`, `failing[]`, `pending`), + `merge_blockers[]`, `review_evidence`, and `new_comments[]`. -1. **If `done` (checks all green + nothing new):** stop the loop and report — PR #, - the green check count, and "no outstanding review findings." You're finished. +1. **If `done` (checks all green + nothing new + PR open/ready/mergeable with no + requested changes + independent review evidence bound to the current head):** + stop the loop and report — PR #, the green check count, review source, and "no + outstanding review findings." You're finished. 1. **If checks are still `pending` and there are no new comments:** nothing to do yet — wait and re-poll (see Pacing). CI can take 20–30 min; that's expected, keep @@ -42,6 +49,16 @@ Repeat until the report says **done**: 1. **If there are `new_comments`:** handle each with judgment — + - **Reviewer unavailable** (`review_unavailable_reason` is set — rate limit, + skipped review, no credits): run the current runtime's configured + `review.fallback_commands` pass. A blocked bot is an action signal, never + auto-noise or a review waiver. Acknowledge the notice only after the fallback + review has completed and every finding from it is handled. Then record the + pass against the exact `head` from the poll you reviewed with `uv run + /pr_watch.py --record-review "fallback:" --head + `. For a lane, use `/dev_session.sh pr-watch + --record-review "fallback:" --head ` instead. + - **Real finding** (a bug, a missing guard, a correctness/clarity issue): fix it in the code, commit, push. Re-running the local gate first. - **Nitpick you disagree with** (style preference, out-of-scope, already-correct): @@ -62,6 +79,15 @@ Repeat until the report says **done**: prior poll (nothing pending) acks nothing and says so (`note` in the output) — always poll-and-read first. +1. **Record the independent pass:** run `--record-review --head ` + only after the configured bot, human, or fallback reviewer has completed and all + findings are resolved. `` is the `head` field from the exact poll whose + diff was reviewed. Recording fails if the PR head changed in the meantime. The + receipt is persisted with that exact `headRefOid`; any later push invalidates it and + requires another independent pass. A platform `APPROVED` state is still recorded + explicitly so the engine never assumes that an approval predates no later push. + Marking comments seen never creates review evidence. + 1. **Pace the next poll** (see below), then go to step 1. ## Pacing @@ -74,10 +100,17 @@ Self-pace on a bounded cadence — don't busy-wait: is fine. - After you push a fix, expect a fresh CI run + possibly a re-review — keep looping; don't declare done off a stale poll. +- A transient `merge state is UNKNOWN` blocker is expected immediately after GitHub + receives new state; re-poll until it resolves. `BLOCKED`, `DIRTY`, `BEHIND`, a + draft bit, or `CHANGES_REQUESTED` needs action rather than acknowledgment. +- `UNSTABLE` remains blocking unless every real check is green and its only remaining + status contexts are names explicitly classified as informational by the engine. A + current-head independent-review receipt is still required in that case. ## Stop conditions -- **Done** — `done: true` (green + clean). Report and finish. This is the goal. +- **Done** — `done: true` (green + clean + current-head independent review evidence). + Report and finish. This is the goal. - **Stuck / needs a decision** — a check fails for a reason you can't resolve (a flaky-infra failure that won't clear on re-run; an external dependency; a finding that needs an operator product/design call). Stop, report the specific blocker, and @@ -89,10 +122,12 @@ Self-pace on a bounded cadence — don't busy-wait: - The seen-set lives at `state/pr-watch/.json` (gitignored). It's per-PR, so re-running on a different PR starts fresh. -- Known auto-noise from your review bots (billing/usage notices, walkthrough / "no - actionable comments" summaries) is filtered out by the engine — edit the - noise-marker list in `/pr_watch.py` for your own bot mix; those never count - as findings. +- Known auto-noise from your review bots (walkthrough / "no actionable comments" + summaries) is filtered out by the engine. Reviewer-unavailable notices are + deliberately *not* noise: they surface and block `done`; acknowledging one still + leaves the current-head review-evidence blocker until the configured fallback runs + and records its receipt. Edit the marker lists in `/pr_watch.py` for + your own bot mix. - This is interactive-only. A scheduled job that opens its own PRs should be excluded from this loop by your cron/CI runner's job-name signal, so an automated open never silently enters an unattended watch loop. diff --git a/docs/autonomous-session-playbook.md b/docs/autonomous-session-playbook.md index 2ef1491..82c8943 100644 --- a/docs/autonomous-session-playbook.md +++ b/docs/autonomous-session-playbook.md @@ -70,13 +70,21 @@ Follow this top to bottom, per ticket. bot's last review rather than waiting indefinitely for a re-review that may not come. Fix real findings (commit + push → re-watch); reply-with-reason to nitpicks you keep. If every configured bot is unavailable (rate-limited, no credits, skipped the PR), run - the current runtime's `review.fallback_commands` value as the substitute independent pass — a blocked bot is not a - waiver on review. + the current runtime's `review.fallback_commands` value as the substitute independent + pass, handle its findings, then bind that evidence to the current head with + `uv run /pr_watch.py --record-review "fallback:" --head + ` (use the `head` from the exact reviewed poll; a lane uses the + scope-aware `dev_session.sh pr-watch` wrapper) — a blocked bot is not a waiver on + review. ### Merge + close out - Merge only once checks are green, the PR is mergeable, and every finding has been fixed - or explicitly addressed: + or explicitly addressed. A lane created by `dev_session.sh` must honor its persisted + merge class at act time: `self` lanes merge through + `/dev_session.sh merge ` (which re-polls `pr-watch`); missing, + unknown, or `operator` metadata refuses autonomous merge and requires operator + sign-off. For non-lane/operator merges: ```sh gh pr merge --squash --delete-branch git checkout && git pull --ff-only origin diff --git a/docs/parallel-dev.md b/docs/parallel-dev.md index 95b3980..4045878 100644 --- a/docs/parallel-dev.md +++ b/docs/parallel-dev.md @@ -86,14 +86,17 @@ Don't spin up lanes ticket-by-ticket. Compose the batch deliberately: ### 2 · Launch each lane — `dev_session.sh new` ```bash -scripts/dev_session.sh new [--base main] [--prefix dev] [--headless] [--runtime ] [--launcher ] +scripts/dev_session.sh new --merge-class [--base ] [--prefix ] [--headless] [--runtime ] [--launcher ] ``` -Each `new` creates the worktree + branch + sandbox and prints a copy-paste line that -drops you into the lane. `--headless` instead writes the sticky +The configured `vcs.protected_branch` and `vcs.dev_branch_prefix` supply the default +base and prefix. Each `new` creates the worktree + branch + sandbox, persists the +merge class, and prints a copy-paste line that drops you into the lane. Omitting +`--merge-class` fails safe to `operator`. `--headless` instead writes the sticky `/.devkit_state_root` marker so an unattended agent's tool calls resolve the -sandbox from disk (they don't share a shell). Interactive `new` and your CI/cron -runner never set the marker — behavior there is unchanged. +sandbox from disk (they don't share a shell). Its JSON descriptor also contains an +`env` map; a launcher must replace inherited lane-root variables with that map before +starting the process. Interactive `new` and your CI/cron runner never set the marker. ### 3 · Each lane works to a draft-green PR @@ -125,12 +128,23 @@ Before the cockpit writes anything to the shared handoff, drive **every** launch to a terminal state — **merged**, **parked-with-reason**, or **still-open** — with `scripts/reconcile_sessions.sh`. An aggregate "everything's done" is *not* evidence a specific lane actually shipped; reconcile per lane. Then review and merge each per its -pre-assigned merge class, and remove the worktree: +pre-assigned merge class. Self-merging lanes use the deterministic wrapper, which +re-polls CI/review/merge readiness at act time and refuses missing or operator +metadata. Operator lanes are merged only after explicit cockpit sign-off. Remove the +worktree afterward: ```bash +scripts/dev_session.sh pr-watch --json +scripts/dev_session.sh pr-watch --mark-seen +scripts/dev_session.sh pr-watch --record-review --head +scripts/dev_session.sh merge # self class only; refuses otherwise scripts/dev_session.sh rm # tear down the worktree (branch kept if unmerged) ``` +The cockpit uses the scope-aware watcher so review state lives in the same lane +sandbox the merge gate reads. `` is the `head` from the exact independently +reviewed poll; recording refuses if it changed. + ## Model / effort tiering per lane The risk read from planning also sets each lane's reasoning budget (Principle #7): a @@ -154,15 +168,16 @@ Two of them share `auth/` (cluster A) — so you run **one** of them now and def other. You launch three disjoint lanes: ```bash -scripts/dev_session.sh new auth-ratelimit --headless # cluster A · top tier · operator-merge -scripts/dev_session.sh new metrics-rename --headless # cluster B · cheap tier · self-merge -scripts/dev_session.sh new cli-help-typo --headless # cluster C · cheap tier · self-merge +scripts/dev_session.sh new auth-ratelimit --headless --merge-class operator # cluster A · top tier +scripts/dev_session.sh new metrics-rename --headless --merge-class self # cluster B · cheap tier +scripts/dev_session.sh new cli-help-typo --headless --merge-class self # cluster C · cheap tier ``` Each lane works to a draft-green PR while you watch `list --watch` from the cockpit. -The two cheap self-merge lanes land themselves; the auth rate-limit lane (security- -adjacent) hands back for your review. You reconcile all three, merge, and only then -update `docs/handoff.md` with what shipped — from the cockpit, once. +The two cheap self-merge lanes land through `dev_session.sh merge`; the auth rate-limit +lane (security-adjacent) hands back for operator review. You reconcile all three, +merge, and only then update `docs/handoff.md` with what shipped — from the cockpit, +once. ## See also diff --git a/docs/parallel-howto.md b/docs/parallel-howto.md index a094def..7908d7e 100644 --- a/docs/parallel-howto.md +++ b/docs/parallel-howto.md @@ -49,7 +49,8 @@ Keep that three-way split in mind and every verb below makes sense. | `parallel`  or  `… list` | Prints the board of active lanes | No | Yes | | `parallel list --watch` | Same board, auto-refreshing | No | Yes | | `parallel plan` | Proposes a disjoint batch to launch | No | Yes (until you confirm) | -| `dev_session.sh new ` | Creates worktree + branch + sandbox, prints a launch line | No | No — writes to disk, but **not** to your checkout | +| `dev_session.sh new --merge-class ` | Creates worktree + branch + sandbox, persists merge policy, prints a launch line | No | No — writes to disk, but **not** to your checkout | +| `dev_session.sh merge ` | Re-polls and merges an eligible self-merge lane; refuses operator/missing metadata | No | No | | *(you paste the launch line)* | Starts the new session in a **new terminal** | No | — | Even `new` doesn't touch *your* working tree: the worktree it creates is a separate @@ -91,7 +92,7 @@ This is the "I want a fresh session that isn't tied to what I'm working on right case. One command sets it up: ```bash -scripts/dev_session.sh new +scripts/dev_session.sh new --merge-class ``` Substitute a lowercase slug for `` (e.g. `add-rate-limit`, `fix-cli-help`). @@ -102,9 +103,10 @@ What this does: 1. Creates a git **worktree** in the sibling `-sessions/` directory (a linked worktree sharing this repo's objects — not a full clone). -2. Creates a fresh branch **`dev/`**, branched off **`origin/main`** by default - — **not** off your current branch. That's what makes the new session independent: - it starts from a clean base, so it can't inherit or disturb your in-progress work. +2. Creates a fresh branch using `vcs.dev_branch_prefix`, branched off the configured + `origin/` by default — **not** off your current branch. That's + what makes the new session independent: it starts from a clean base, so it can't + inherit or disturb your in-progress work. 3. Sets up an isolated state sandbox (`DEVKIT_STATE_ROOT`) so the new session's scratch-state writes never collide with yours. 4. Prints a **copy-paste line** and stops: @@ -119,11 +121,13 @@ own branch. Useful options: -- `--base ` — branch off something other than `main`. Use this if you +- `--base ` — override `vcs.protected_branch`. Use this if you *deliberately* want the new session to build on another branch (including your current one). -- `--prefix

` — branch namespace (default `dev`, giving `dev/`). +- `--prefix

` — override `vcs.dev_branch_prefix` for the branch namespace. - `--branch ` — override the whole branch name. +- `--merge-class self|operator` — persist who may perform the terminal merge. If + omitted, the lane fails safe to `operator`. - `--runtime ` — select a key from `runtime.launchers` for this lane. - `--launcher ` — override the configured command for this lane; use `--launcher none` to print activation-only guidance. @@ -165,15 +169,16 @@ A headless lane has **no human terminal** to hand a launch line to, so `new` beh differently: ```bash -scripts/dev_session.sh new --headless +scripts/dev_session.sh new --headless --merge-class ``` Instead of printing a copy-paste line, `--headless` writes a sticky `/.devkit_state_root` marker file so the agent's tool calls resolve the sandbox from disk (they don't inherit a shell), and emits a machine-readable descriptor -whose `prompt_preamble` carries the **lane contract** — draft PR, actively poll your -own CI to green, never touch the narrative files, check `git branch --show-current` -before every commit. See the exact contract text with: +whose `prompt_preamble` carries the **lane contract** and whose `env` map contains the +lane-specific roots. The launcher must replace any inherited cockpit root variables +with this map. The contract covers draft PR, active CI polling, narrative ownership, +and branch checks. See the exact contract text with: ```bash scripts/dev_session.sh print-contract @@ -203,6 +208,21 @@ shipped: scripts/reconcile_sessions.sh … # merged / parked / open, per lane ``` +**Merge an eligible self-merge lane** through the safety wrapper. It reads the +persisted class and branch, resolves the open PR, re-polls `pr-watch`, and merges only +when every gate is currently clean. Operator-class or missing metadata is refused: + +```bash +scripts/dev_session.sh pr-watch --json +scripts/dev_session.sh pr-watch --mark-seen +scripts/dev_session.sh pr-watch --record-review --head +scripts/dev_session.sh merge +``` + +Use the `head` field from the exact independently reviewed poll. The scope-aware +wrapper keeps the cockpit's acknowledgments and head-bound receipt in the lane state +sandbox; recording refuses if a push landed between the review and receipt. + **Remove the worktree** once you're done with it: ```bash @@ -223,8 +243,9 @@ No. Every verb either reads state (`list`, `plan`, `path`) or creates a *separat worktree (`new`). Your current session stays on its own branch the whole time. **Does the new session branch off my current branch?** -No — off `origin/main` by default, so it starts clean. Pass `--base ` if you -specifically want it based on something else (including your current branch). +No — off the configured `origin/` by default, so it starts +clean. Pass `--base ` if you specifically want it based on something else +(including your current branch). **Why can't `parallel new` just open the new session for me?** Because an in-agent workflow runs inside your current agent process. It can't move your diff --git a/init.sh b/init.sh index 8ff3021..685d7ae 100644 --- a/init.sh +++ b/init.sh @@ -137,6 +137,105 @@ set_field() { ' "$CONFIG_FILE" > "$tmpfile" && mv "$tmpfile" "$CONFIG_FILE" } +# Insert a block immediately before a top-level section. Used only for schema +# additions introduced after the first template release; existing values are +# preserved rather than guessed over. +insert_before_section() { + anchor="$1" + block="$2" + tmpfile="${CONFIG_FILE}.tmp.$$" + blockfile="${tmpfile}.block" + printf '%s\n' "$block" > "$blockfile" + awk -v anchor="$anchor" -v blockfile="$blockfile" ' + function emit( line) { + while ((getline line < blockfile) > 0) print line + close(blockfile) + } + index($0, anchor) == 1 && $0 ~ /^[A-Za-z_]/ && !inserted { emit(); inserted = 1 } + { print } + END { if (!inserted) emit() } + ' "$CONFIG_FILE" > "$tmpfile" && mv "$tmpfile" "$CONFIG_FILE" + rm -f "$blockfile" +} + +# Append a block to a named top-level section, immediately before the next one. +append_to_section() { + section="$1" + block="$2" + tmpfile="${CONFIG_FILE}.tmp.$$" + blockfile="${tmpfile}.block" + printf '%s\n' "$block" > "$blockfile" + awk -v section="$section" -v blockfile="$blockfile" ' + function emit( line) { + while ((getline line < blockfile) > 0) print line + close(blockfile) + } + $0 == section { inside = 1 } + inside && $0 != section && $0 ~ /^[A-Za-z_][A-Za-z0-9_]*:/ && !inserted { + emit() + inserted = 1 + inside = 0 + } + { print } + END { if (inside && !inserted) emit() } + ' "$CONFIG_FILE" > "$tmpfile" && mv "$tmpfile" "$CONFIG_FILE" + rm -f "$blockfile" +} + +# Migrate the original single-runtime schema in place. Re-running init.sh is +# documented as idempotent; silently leaving an old config without these keys +# would make the new runtime-aware launchers appear bootstrapped while falling +# back to the wrong behavior. +migrate_runtime_schema() { + if ! grep -q '^ engines:' "$CONFIG_FILE"; then + append_to_section "paths:" ' # Directory containing the deterministic kit engines. + engines: scripts' + echo "added paths.engines to config/dev-model.yaml" + fi + + if ! grep -q '^runtime:' "$CONFIG_FILE"; then + insert_before_section "doc_budgets:" 'runtime: + default: claude + launchers: + claude: claude + codex: codex +' + echo "added runtime mappings to config/dev-model.yaml" + fi + + if ! grep -q '^ fallback_commands:' "$CONFIG_FILE"; then + old_fallback=$(get_field "review:" "" "^ fallback_command:") + [ -n "$old_fallback" ] || old_fallback="/code-review" + append_to_section "review:" " fallback_commands: + claude: $old_fallback + codex: \"/review\"" + echo "added runtime review fallbacks to config/dev-model.yaml" + fi + + if ! grep -q '^ tiers:' "$CONFIG_FILE"; then + old_cheap=$(get_field "models:" "" "^ cheap:") + old_default=$(get_field "models:" "" "^ default:") + old_expensive=$(get_field "models:" "" "^ expensive:") + [ -n "$old_cheap" ] || old_cheap="haiku" + [ -n "$old_default" ] || old_default="sonnet" + [ -n "$old_expensive" ] || old_expensive="opus" + append_to_section "models:" " tiers: + cheap: mechanical + default: standard + expensive: judgment + runtime_mappings: + claude: + cheap: $old_cheap + default: $old_default + expensive: $old_expensive + codex: + cheap: low + default: medium + expensive: high" + echo "added runtime model mappings to config/dev-model.yaml" + fi +} + # ask -> prints the answer (default kept on empty input # or when stdin isn't a terminal, e.g. running init.sh from a non-interactive # script). @@ -160,6 +259,8 @@ if [ ! -t 0 ]; then echo "note: no terminal attached — keeping all current config/dev-model.yaml values." >&2 fi +migrate_runtime_schema + # ── prompts ────────────────────────────────────────────────────────────── cur_name=$(get_field "project:" "" "^ name:") diff --git a/scripts/archive_plan_sessions.py b/scripts/archive_plan_sessions.py index 900d404..b56b195 100755 --- a/scripts/archive_plan_sessions.py +++ b/scripts/archive_plan_sessions.py @@ -37,10 +37,13 @@ from __future__ import annotations import argparse +import os import re import sys from pathlib import Path +import yaml + sys.path.insert(0, str(Path(__file__).resolve().parent / "lib")) from devmodel_config import _repo_root, load_config, resolve_path # noqa: E402 @@ -63,14 +66,17 @@ DEFAULT_KEEP = 6 RECENT_SESSIONS_HEADING = "## Recent sessions" HISTORY_SECTION_HEADINGS = ("## Session log", "## Recent sessions (archived)") +_RECENT_SESSION_RE = re.compile(r"^### \d{4}-\d{2}-\d{2}\b") + -POINTER = [ - "> Older session entries (below the live blocks above) live in [`handoff-history.md`](handoff-history.md).\n", - '> Active open items from them are folded into the "Open for next session" lists above.\n', - "\n", - SEP, - "\n", -] +def history_pointer(link: str, label: str) -> list[str]: + return [ + f"> Older session entries (below the live blocks above) live in [`{label}`]({link}).\n", + '> Active open items from them are folded into the "Open for next session" lists above.\n', + "\n", + SEP, + "\n", + ] def configured_paths( @@ -100,7 +106,9 @@ def split_plan(lines: list[str]) -> tuple[list[str], list[str], list[str]]: session blocks plus any inter-block separators and the existing pointer; ``tail`` is the first non-session ``##`` heading (standing sections) onward. """ - sess_start = next((i for i, ln in enumerate(lines) if _is_session_heading(ln)), None) + sess_start = next( + (i for i, ln in enumerate(lines) if _is_session_heading(ln)), None + ) if sess_start is not None: standing = next( ( @@ -115,11 +123,7 @@ def split_plan(lines: list[str]) -> tuple[list[str], list[str], list[str]]: return lines[:sess_start], lines[sess_start:standing], lines[standing:] recent_start = next( - ( - i - for i, ln in enumerate(lines) - if ln.rstrip("\n") == RECENT_SESSIONS_HEADING - ), + (i for i, ln in enumerate(lines) if ln.rstrip("\n") == RECENT_SESSIONS_HEADING), None, ) if recent_start is None: @@ -127,16 +131,16 @@ def split_plan(lines: list[str]) -> tuple[list[str], list[str], list[str]]: "no session blocks or '## Recent sessions' section found in handoff doc" ) standing = next( - ( - i - for i, ln in enumerate(lines) - if i > recent_start and ln.startswith("## ") - ), + (i for i, ln in enumerate(lines) if i > recent_start and ln.startswith("## ")), len(lines), ) # Keep the section heading in the head; parse_blocks handles its ``###`` # entries and rebuild_plan preserves this layout without adding a pointer. - return lines[: recent_start + 1], lines[recent_start + 1 : standing], lines[standing:] + return ( + lines[: recent_start + 1], + lines[recent_start + 1 : standing], + lines[standing:], + ) def parse_blocks(region: list[str]) -> list[list[str]]: @@ -150,7 +154,7 @@ def parse_blocks(region: list[str]) -> list[list[str]]: def is_block_heading(line: str) -> bool: if uses_recent_sections: - return line.startswith("### ") + return bool(_RECENT_SESSION_RE.match(line)) return _is_session_heading(line) cur: list[str] | None = None @@ -164,7 +168,9 @@ def is_block_heading(line: str) -> bool: if cur is not None: blocks.append(cur) for block in blocks: - while block and (block[-1].strip() == "" or _is_sep(block[-1]) or block[-1].startswith(">")): + while block and ( + block[-1].strip() == "" or _is_sep(block[-1]) or block[-1].startswith(">") + ): block.pop() return blocks @@ -202,7 +208,15 @@ def trim_megaline(head: list[str], keep: int) -> list[str]: return out -def rebuild_plan(head: list[str], keep_blocks: list[list[str]], tail: list[str], keep: int) -> list[str]: +def rebuild_plan( + head: list[str], + keep_blocks: list[list[str]], + tail: list[str], + keep: int, + *, + history_link: str = "handoff-history.md", + history_label: str = "handoff-history.md", +) -> list[str]: """Reassemble the handoff doc from the trimmed head, kept blocks, fresh pointer, and tail.""" if head and head[-1].rstrip("\n") == RECENT_SESSIONS_HEADING: body: list[str] = ["\n"] @@ -214,7 +228,7 @@ def rebuild_plan(head: list[str], keep_blocks: list[list[str]], tail: list[str], body: list[str] = [] for block in keep_blocks: body += block + ["\n", SEP, "\n"] - body += POINTER + body += history_pointer(history_link, history_label) return head + body + tail @@ -245,16 +259,43 @@ def main(argv: list[str] | None = None) -> int: Returns 0 on success / no-op / dry-run, 2 on usage error, unparseable handoff structure, or a failed write (the handoff doc is rolled back in that case). """ - default_plan, default_history = configured_paths() parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--keep", type=int, default=DEFAULT_KEEP, help="live blocks to keep") - parser.add_argument("--plan", type=Path, default=default_plan, help="living handoff doc") parser.add_argument( - "--history", type=Path, default=default_history, help="handoff history/archive doc" + "--keep", type=int, default=DEFAULT_KEEP, help="live blocks to keep" + ) + parser.add_argument("--plan", type=Path, default=None, help="living handoff doc") + parser.add_argument( + "--history", type=Path, default=None, help="handoff history/archive doc" + ) + parser.add_argument( + "--dry-run", action="store_true", help="report only, write nothing" ) - parser.add_argument("--dry-run", action="store_true", help="report only, write nothing") args = parser.parse_args(argv) + if args.plan is None or args.history is None: + try: + default_plan, default_history = configured_paths() + except ( + FileNotFoundError, + KeyError, + OSError, + TypeError, + ValueError, + yaml.YAMLError, + ) as exc: + print( + f"error: could not resolve configured handoff paths ({exc})", + file=sys.stderr, + ) + return 2 + args.plan = args.plan or default_plan + args.history = args.history or default_history + + args.plan = args.plan if args.plan.is_absolute() else Path.cwd() / args.plan + args.history = ( + args.history if args.history.is_absolute() else Path.cwd() / args.history + ) + if args.keep < 1: print("error: --keep must be >= 1", file=sys.stderr) return 2 @@ -278,7 +319,17 @@ def main(argv: list[str] | None = None) -> int: return 0 keep_blocks, moved = blocks[: args.keep], blocks[args.keep :] - new_plan = rebuild_plan(head, keep_blocks, tail, args.keep) + history_link = os.path.relpath(args.history, start=args.plan.parent).replace( + os.sep, "/" + ) + new_plan = rebuild_plan( + head, + keep_blocks, + tail, + args.keep, + history_link=history_link, + history_label=args.history.name, + ) try: new_history = insert_into_history(history, moved) except ValueError as exc: diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 4fec220..daf7fd8 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -13,9 +13,11 @@ # runs in zero sessions until something sets it. `new` is what finally sets it. # # Usage: -# scripts/dev_session.sh new [--base main] [--prefix dev] [--branch ] [--force] [--headless] [--runtime ] [--launcher ] +# scripts/dev_session.sh new [--base ] [--prefix ] [--branch ] [--merge-class self|operator] [--force] [--headless] [--runtime ] [--launcher ] # scripts/dev_session.sh list [--watch [interval]] # scripts/dev_session.sh path +# scripts/dev_session.sh pr-watch [pr-watch options] +# scripts/dev_session.sh merge # scripts/dev_session.sh rm [--force] [--keep-branch] # scripts/dev_session.sh print-contract # @@ -35,10 +37,12 @@ # worktree and its state/ writes isolate via the marker, no env export needed. # Diagnostics go to stderr in this mode so stdout is clean JSON. The # descriptor also carries `prompt_preamble` — the canonical lane-contract text -# a launcher MUST prepend verbatim to the lane's prompt — and an `env` map -# (currently `DEVKIT_REFUSE_UNSANDBOXED_STATE: "1"`) the launcher MUST export -# into the lane's process so a marker-resolution failure refuses rather than -# silently falling back to prod `state/`. The same env var is also baked into +# a launcher MUST prepend verbatim to the lane's prompt — a persisted +# `merge_class`, and an `env` map the launcher MUST use to replace inherited +# lane-root values. This prevents an already-sandboxed cockpit's environment +# from overriding the new lane's marker; the fail-closed guard also refuses a +# marker-resolution failure rather than silently falling back to prod `state/`. +# The same guard is baked into # the worktree's `activate` snippet for the interactive fallback (`source # /activate`). Interactive `new` (no `--headless`) and cron are # byte-identical — neither sets this flag. @@ -62,16 +66,29 @@ REPO_ROOT="$(devkit_find_repo_root "$SCRIPT_DIR")" || { exit 1 } ENGINE_DIR_REL="${SCRIPT_DIR#"$REPO_ROOT"/}" +CONFIG_FILE="$REPO_ROOT/config/dev-model.yaml" + +# Configuration owns the integration branch and lane namespace. Environment +# overrides remain available for unusual launchers; missing legacy keys fall +# back to the historical defaults. +CONFIGURED_PROTECTED_BRANCH="$(devkit_config_scalar "$CONFIG_FILE" vcs "" protected_branch || true)" +[[ -n "$CONFIGURED_PROTECTED_BRANCH" ]] || { + echo "[dev-session] error: config must define vcs.protected_branch" >&2 + exit 1 +} +git check-ref-format --branch "$CONFIGURED_PROTECTED_BRANCH" >/dev/null 2>&1 || { + echo "[dev-session] error: invalid vcs.protected_branch '$CONFIGURED_PROTECTED_BRANCH'" >&2 + exit 1 +} +PROTECTED_BRANCH="$CONFIGURED_PROTECTED_BRANCH" +DEFAULT_BASE="${DEV_SESSION_BASE:-$PROTECTED_BRANCH}" +CONFIGURED_PREFIX="$(devkit_config_scalar "$CONFIG_FILE" vcs "" dev_branch_prefix || true)" +DEFAULT_PREFIX="${DEV_SESSION_PREFIX:-${CONFIGURED_PREFIX:-dev}}" # Sessions container (sibling of the repo by default). Each session is one # subdir holding wt/ (worktree) + state/ (sandbox) + activate (env snippet). SESSIONS_DIR="${DEVKIT_SESSIONS_DIR:-$(dirname "$REPO_ROOT")/dev-model-sessions}" -# Parallel-session branches get their own namespace so they can't collide with -# hand-named feature branches. Default "dev" — keep this in sync with -# `vcs.dev_branch_prefix` in config/dev-model.yaml if you change it there. -DEFAULT_PREFIX="${DEV_SESSION_PREFIX:-dev}" - # Sticky sandbox marker written at the worktree root by `new --headless`. MUST # match scripts/lib/state_paths's STATE_ROOT_MARKER — the resolver reads it to # select the sandbox when DEVKIT_STATE_ROOT is unset. @@ -156,35 +173,8 @@ _slug_ok() { [[ "$1" =~ ^[a-z0-9][a-z0-9-]*$ ]] } -# Read one scalar from the kit's deliberately simple, hand-authored YAML config. -# This avoids adding a YAML dependency to the shell launcher. It supports exactly -# the top-level/subsection/scalar shape used by runtime.default and -# runtime.launchers.. _config_scalar() { - local section="$1" subsection="$2" key="$3" - [[ -n "$subsection" ]] && subsection="$subsection:" - awk -v want_section="$section:" -v want_subsection="$subsection" -v want_key="$key:" ' - function trim(s) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", s); return s } - /^[A-Za-z_][A-Za-z0-9_]*:[[:space:]]*$/ { - current_section = trim($0); current_subsection = ""; next - } - /^ [A-Za-z_][A-Za-z0-9_]*:[[:space:]]*$/ { - current_subsection = trim($0); next - } - current_section == want_section && current_subsection == want_subsection { - line = $0 - stripped = line - sub(/^[[:space:]]+/, "", stripped) - if (index(stripped, want_key) == 1) { - value = substr(stripped, length(want_key) + 1) - sub(/[[:space:]]+#.*/, "", value) - value = trim(value) - gsub(/^"|"$/, "", value) - print value - exit - } - } - ' "$REPO_ROOT/config/dev-model.yaml" 2>/dev/null + devkit_config_scalar "$CONFIG_FILE" "$1" "$2" "$3" } _sed_replacement() { @@ -217,11 +207,11 @@ _resolve_launcher() { _is_protected_branch() { local b="$1" base="$2" # allow-hardcoded — main/master are the universal protected mainlines by definition - [[ "$b" == "$base" || "$b" == "main" || "$b" == "master" ]] + [[ "$b" == "$base" || "$b" == "$PROTECTED_BRANCH" || "$b" == "main" || "$b" == "master" ]] } cmd_new() { - local scope="" base="main" prefix="$DEFAULT_PREFIX" branch="" force=0 + local scope="" base="$DEFAULT_BASE" prefix="$DEFAULT_PREFIX" branch="" merge_class="operator" force=0 local requested_runtime="" requested_launcher="" runtime="" launcher="" HEADLESS=0 while [[ $# -gt 0 ]]; do @@ -229,6 +219,7 @@ cmd_new() { --base) [[ $# -ge 2 ]] || _die "--base needs a value"; base="$2"; shift 2 ;; --prefix) [[ $# -ge 2 ]] || _die "--prefix needs a value"; prefix="$2"; shift 2 ;; --branch) [[ $# -ge 2 ]] || _die "--branch needs a value"; branch="$2"; shift 2 ;; + --merge-class) [[ $# -ge 2 ]] || _die "--merge-class needs self or operator"; merge_class="$2"; shift 2 ;; --force) force=1; shift ;; --headless) HEADLESS=1; shift ;; --runtime) [[ $# -ge 2 ]] || _die "--runtime needs a value"; requested_runtime="$2"; shift 2 ;; @@ -237,8 +228,10 @@ cmd_new() { *) [[ -z "$scope" ]] && scope="$1" && shift || _die "unexpected arg: $1" ;; esac done - [[ -n "$scope" ]] || _die "usage: dev_session.sh new [--base main] [--prefix dev] [--branch ] [--force] [--headless] [--runtime ] [--launcher ]" + [[ -n "$scope" ]] || _die "usage: dev_session.sh new [--base ] [--prefix ] [--branch ] [--merge-class self|operator] [--force] [--headless] [--runtime ] [--launcher ]" _slug_ok "$scope" || _die "scope must be a lowercase slug ([a-z0-9-]): got '$scope'" + [[ "$merge_class" == "self" || "$merge_class" == "operator" ]] \ + || _die "merge class must be 'self' or 'operator' (got '$merge_class')" [[ -z "$branch" ]] && branch="${prefix}/${scope}" IFS=$'\t' read -r runtime launcher <<< "$(_resolve_launcher "$requested_runtime" "$requested_launcher")" @@ -331,6 +324,7 @@ ACTIVATE # is never committed. Written for both the interactive and --headless paths. printf '%s\n' "$branch" > "$session_dir/branch" printf '%s\n' "$base" > "$session_dir/base" + printf '%s\n' "$merge_class" > "$session_dir/merge_class" if [[ "$HEADLESS" -eq 1 ]]; then # Resolve to ABSOLUTE paths before writing the marker/descriptor. $sandbox @@ -358,14 +352,16 @@ ACTIVATE # Written to fd 3 (the saved real stdout) so it is the sole thing on # the caller's stdout. `prompt_preamble` is the canonical lane-contract # text the launcher MUST prepend verbatim to the lane's prompt; `env` - # is the map of env vars the launcher MUST export into the lane's - # process — currently just the fail-closed sandbox guard. + # is the map of env vars the launcher MUST REPLACE in the lane process. + # Explicit lane roots are load-bearing: an inherited cockpit + # DEVKIT_STATE_ROOT otherwise outranks this lane's marker and collapses + # multiple headless lanes into one shared state directory. python3 - "$scope" "$branch" "$worktree_abs" "$sandbox_abs" "$repo_root_abs" "$base" \ - "$(_lane_contract)" "$REFUSE_UNSANDBOXED_ENV_VALUE" "$runtime" "$launcher" >&3 <<'PY' + "$merge_class" "$(_lane_contract)" "$REFUSE_UNSANDBOXED_ENV_VALUE" "$runtime" "$launcher" >&3 <<'PY' import json import sys -scope, branch, worktree, state_root, repo_root, base, prompt_preamble, refuse_unsandboxed, runtime, launcher = sys.argv[1:11] +scope, branch, worktree, state_root, repo_root, base, merge_class, prompt_preamble, refuse_unsandboxed, runtime, launcher = sys.argv[1:12] print( json.dumps( { @@ -375,8 +371,13 @@ print( "state_root": state_root, "repo_root": repo_root, "base": base, + "merge_class": merge_class, "prompt_preamble": prompt_preamble, - "env": {"DEVKIT_REFUSE_UNSANDBOXED_STATE": refuse_unsandboxed}, + "env": { + "DEVKIT_STATE_ROOT": state_root, + "DEVKIT_ROOT": repo_root, + "DEVKIT_REFUSE_UNSANDBOXED_STATE": refuse_unsandboxed, + }, "runtime": runtime, "launcher": launcher or None, } @@ -387,7 +388,7 @@ PY fi echo - echo "✓ session '$scope' ready." + echo "✓ session '$scope' ready (merge class: $merge_class)." echo if [[ -n "$launcher" ]]; then echo " Start it with $runtime (copy-paste):" @@ -647,6 +648,121 @@ cmd_print_contract() { _lane_contract } +# Resolve exactly one same-repository open PR for a recorded lane branch/base. +# Prints ` TAB `; every caller then invokes gh/pr-watch in +# that same repository and the lane's own state sandbox. +_resolve_lane_pr() { + local branch="$1" base="$2" + local repo_json repo_nwo repo_owner pr_json pr_meta + local pr listed_base listed_branch listed_head listed_owner + + # GH_REPO overrides local checkout discovery. Unset it for identity + # resolution, then explicitly pin that resolved repository everywhere else. + repo_json="$(cd "$REPO_ROOT" && env -u GH_REPO gh repo view --json nameWithOwner)" \ + || _die "could not resolve the GitHub repository for $REPO_ROOT" + repo_nwo="$(printf '%s' "$repo_json" | python3 -c ' +import json, sys +print(json.load(sys.stdin).get("nameWithOwner") or "") +')" + [[ "$repo_nwo" == */* ]] || _die "GitHub returned an invalid repository identity" + repo_owner="${repo_nwo%%/*}" + + pr_json="$(cd "$REPO_ROOT" && GH_REPO="$repo_nwo" gh pr list --repo "$repo_nwo" --head "$branch" --state open \ + --json number,baseRefName,headRefName,headRefOid,headRepositoryOwner --limit 2)" \ + || _die "could not resolve an open PR for '$branch'" + pr_meta="$(printf '%s' "$pr_json" | python3 -c ' +import json, sys +rows = json.load(sys.stdin) +if len(rows) != 1: + raise SystemExit(2) +row = rows[0] +owner = row.get("headRepositoryOwner") or {} +if isinstance(owner, dict): + owner = owner.get("login") or owner.get("name") or "" +print("\t".join(str(row.get(k) or "") for k in ("number", "baseRefName", "headRefName", "headRefOid")) + "\t" + str(owner)) +')" || _die "expected exactly one open PR for '$branch'" + IFS=$'\t' read -r pr listed_base listed_branch listed_head listed_owner <<< "$pr_meta" + [[ -n "$pr" ]] || _die "open PR metadata for '$branch' has no number" + [[ "$listed_base" == "$base" ]] \ + || _die "PR #$pr targets '$listed_base', not recorded base '$base'" + [[ "$listed_branch" == "$branch" ]] \ + || _die "PR #$pr head '$listed_branch' does not match recorded branch '$branch'" + [[ "$listed_owner" == "$repo_owner" ]] \ + || _die "PR #$pr head owner '$listed_owner' does not match repository owner '$repo_owner'" + [[ -n "$listed_head" ]] || _die "PR #$pr has no head commit" + printf '%s\t%s\n' "$repo_nwo" "$pr" +} + +# Scope-aware front door for the PR watcher. Cockpit review happens outside the +# lane process, so this wrapper is what keeps its polls, acknowledgments, and +# head-bound review receipt in the same lane sandbox that cmd_merge re-checks. +cmd_pr_watch() { + local scope="${1:-}" + [[ -n "$scope" ]] || _die "usage: dev_session.sh pr-watch [pr-watch options]" + shift + _slug_ok "$scope" || _die "scope must be a lowercase slug ([a-z0-9-]): got '$scope'" + local session_dir="$SESSIONS_DIR/$scope" branch="" base="$DEFAULT_BASE" resolved repo_nwo pr + [[ -d "$session_dir" ]] || _die "no session '$scope' at $session_dir" + [[ -s "$session_dir/branch" ]] && branch="$(cat "$session_dir/branch")" + [[ -s "$session_dir/base" ]] && base="$(cat "$session_dir/base")" + [[ -n "$branch" ]] || _die "session '$scope' has no recorded branch" + _is_protected_branch "$branch" "$base" \ + && _die "refusing to watch protected branch '$branch' as a lane" + resolved="$(_resolve_lane_pr "$branch" "$base")" || return + IFS=$'\t' read -r repo_nwo pr <<< "$resolved" + [[ -n "$repo_nwo" && -n "$pr" ]] || _die "could not resolve lane PR identity" + GH_REPO="$repo_nwo" DEVKIT_STATE_ROOT="$session_dir/state" \ + uv run "$SCRIPT_DIR/pr_watch.py" "$pr" "$@" +} + +# Autonomous/headless self-merges must pass through this wrapper. The merge +# class is a deterministic artifact written at lane creation, and pr-watch is +# re-polled at act time so stale green state cannot authorize a merge. +cmd_merge() { + local scope="${1:-}" + [[ -n "$scope" ]] || _die "usage: dev_session.sh merge " + _slug_ok "$scope" || _die "scope must be a lowercase slug ([a-z0-9-]): got '$scope'" + local session_dir="$SESSIONS_DIR/$scope" + [[ -d "$session_dir" ]] || _die "no session '$scope' at $session_dir" + + local merge_class="operator" branch="" base="$DEFAULT_BASE" + local resolved repo_nwo pr + local report done validated_pr validated_base validated_head + [[ -s "$session_dir/merge_class" ]] && merge_class="$(cat "$session_dir/merge_class")" + [[ "$merge_class" == "self" ]] \ + || _die "lane '$scope' is operator-merge (or missing metadata); autonomous merge refused" + [[ -s "$session_dir/branch" ]] && branch="$(cat "$session_dir/branch")" + [[ -s "$session_dir/base" ]] && base="$(cat "$session_dir/base")" + [[ -n "$branch" ]] || _die "session '$scope' has no recorded branch" + if _is_protected_branch "$branch" "$base"; then + _die "refusing to merge protected branch '$branch' as a lane" + fi + + resolved="$(_resolve_lane_pr "$branch" "$base")" || return + IFS=$'\t' read -r repo_nwo pr <<< "$resolved" + [[ -n "$repo_nwo" && -n "$pr" ]] || _die "could not resolve lane PR identity" + + report="$(GH_REPO="$repo_nwo" DEVKIT_STATE_ROOT="$session_dir/state" \ + uv run "$SCRIPT_DIR/pr_watch.py" "$pr" --json)" \ + || _die "pr-watch failed for PR #$pr" + IFS=$'\t' read -r done validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c ' +import json, sys +d = json.load(sys.stdin) +print("\t".join(("true" if d.get("done") is True else "false", str(d.get("pr") or ""), str(d.get("base") or ""), str(d.get("head") or "")))) +')" + [[ "$done" == "true" ]] \ + || _die "PR #$pr is not green, review-clean, and merge-ready; run pr-watch to convergence first" + [[ "$validated_pr" == "$pr" ]] \ + || _die "pr-watch validated PR #$validated_pr, not resolved PR #$pr" + [[ "$validated_base" == "$base" ]] \ + || _die "validated PR #$pr targets '$validated_base', not recorded base '$base'" + [[ -n "$validated_head" ]] || _die "pr-watch returned no validated head for PR #$pr" + + (cd "$REPO_ROOT" && GH_REPO="$repo_nwo" gh pr merge --repo "$repo_nwo" "$pr" --squash --delete-branch \ + --match-head-commit "$validated_head") \ + || _die "GitHub merge failed for PR #$pr" +} + cmd_rm() { local scope="" force=0 keep_branch=0 while [[ $# -gt 0 ]]; do @@ -663,10 +779,10 @@ cmd_rm() { local worktree="$session_dir/wt" [[ -d "$worktree" ]] || _die "no session '$scope' at $worktree" - # Resolve the session's OWN base first (recorded by `new`; default main) so the + # Resolve the session's OWN base first (recorded by `new`; configured default) so the # protected-branch guard below knows the real integration ref. local expected_branch base dirty - base="main" # allow-hardcoded — default base; the session's real base is recorded by `new` + base="$DEFAULT_BASE" [[ -s "$session_dir/base" ]] && base="$(cat "$session_dir/base")" # Resolve the branch THIS session OWNS — the name `new` recorded, else the @@ -752,12 +868,14 @@ main() { new) cmd_new "$@" ;; list|ls) cmd_list "$@" ;; path) cmd_path "$@" ;; + pr-watch) cmd_pr_watch "$@" ;; + merge) cmd_merge "$@" ;; rm|remove) cmd_rm "$@" ;; print-contract) cmd_print_contract "$@" ;; ""|-h|--help|help) sed -n '2,52p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' ;; - *) _die "unknown subcommand '$sub' (try: new | list | path | rm | print-contract)" ;; + *) _die "unknown subcommand '$sub' (try: new | list | path | pr-watch | merge | rm | print-contract)" ;; esac } diff --git a/scripts/lib/repo_root.sh b/scripts/lib/repo_root.sh index 563c47c..ba0c2e0 100644 --- a/scripts/lib/repo_root.sh +++ b/scripts/lib/repo_root.sh @@ -13,3 +13,39 @@ devkit_find_repo_root() { done return 1 } + +# Read one scalar from the kit's deliberately simple, hand-authored YAML config. +# Supports top-level sections, one optional subsection, and scalar values. Keeping +# this here lets every shell engine share the same config semantics without taking +# a YAML runtime dependency. +devkit_config_scalar() { + local config_file="$1" section="$2" subsection="$3" key="$4" + [[ -n "$subsection" ]] && subsection="$subsection:" + awk -v want_section="$section:" -v want_subsection="$subsection" -v want_key="$key:" ' + function trim(s) { gsub(/^[[:space:]]+|[[:space:]]+$/, "", s); return s } + /^[A-Za-z_][A-Za-z0-9_]*:[[:space:]]*$/ { + current_section = trim($0); current_subsection = ""; next + } + /^ [A-Za-z_][A-Za-z0-9_]*:[[:space:]]*$/ { + current_subsection = trim($0); next + } + current_section == want_section && current_subsection == want_subsection { + line = $0 + stripped = line + sub(/^[[:space:]]+/, "", stripped) + if (index(stripped, want_key) == 1) { + value = substr(stripped, length(want_key) + 1) + sub(/[[:space:]]+#.*/, "", value) + value = trim(value) + # Strip one matching layer of the simple single/double quotes + # used by hand-authored scalar config. Leaving single quotes in + # a branch value defeats exact protected-branch comparisons. + if ((value ~ /^".*"$/) || (value ~ /^'"'"'.*'"'"'$/)) { + value = substr(value, 2, length(value) - 2) + } + print value + exit + } + } + ' "$config_file" 2>/dev/null +} diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index e2da28e..7caf289 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -9,14 +9,15 @@ agent instructions) call this once per round: it asks `gh` for the PR's check rollup + every comment surface (issue comments, review submissions, inline review comments), -filters out known auto-noise (a review bot's billing notice, its walkthrough / -pre-merge-check summaries), diffs against a per-PR seen-set so only *new -actionable* comments surface, and reports whether the PR is `done` (all checks -green AND nothing new to address). +filters out known auto-noise (while surfacing reviewer-unavailability notices), +diffs against a per-PR seen-set so only *new actionable* comments surface, and +reports whether the PR is `done` (checks green, nothing new, merge-ready, and +independently reviewed at the current head). The caller loops: run this -> if not done, fix the failures / address or reply to the new comments -> `--mark-seen` -> wait -> run again. `done` flips true -once CI is green and every review-bot finding has been handled. +once CI is green, every finding has been handled, and an explicitly recorded +independent-review receipt covers the current head. `--mark-seen` NEVER re-polls `gh`. Every plain poll (any invocation without `--mark-seen`) persists the exact ``all_seen_keys`` it just reported into a @@ -37,6 +38,7 @@ uv run scripts/pr_watch.py # current branch's PR, human summary uv run scripts/pr_watch.py 916 --json # explicit PR, machine-readable uv run scripts/pr_watch.py --mark-seen # ack exactly what the last poll reported + uv run scripts/pr_watch.py 916 --record-review "fallback:codex" --head uv run scripts/pr_watch.py 916 --assert-draft # correct a drifted draft bit after `gh pr create --draft` uv run scripts/pr_watch.py 916 --assert-ready # correct a drifted draft bit before `gh pr merge` @@ -59,6 +61,7 @@ from __future__ import annotations import argparse +from datetime import datetime, timezone import hashlib import json import os @@ -67,6 +70,7 @@ import time from pathlib import Path + def _find_repo_root(start: Path) -> Path: """Nearest ancestor with a ``.git`` marker (so this keeps working when the kit is vendored under a nested dir, e.g. scripts/devkit/); falls back to the @@ -86,7 +90,11 @@ def _find_repo_root(start: Path) -> Path: # override else repo-root state/ (a relative override falls back rather than # raising — never crash the loop). _STATE_ENV = os.environ.get("DEVKIT_STATE_ROOT") -_STATE_ROOT = Path(_STATE_ENV) if _STATE_ENV and os.path.isabs(_STATE_ENV) else REPO_ROOT / "state" +_STATE_ROOT = ( + Path(_STATE_ENV) + if _STATE_ENV and os.path.isabs(_STATE_ENV) + else REPO_ROOT / "state" +) STATE_DIR = _STATE_ROOT / "pr-watch" # A comment is auto-noise (not a finding to act on) if its body matches any of @@ -105,6 +113,19 @@ def _find_repo_root(start: Path) -> Path: "", # a tracker's auto issue-mirror comment (not a finding) ) +# Review unavailability is actionable even when the surrounding comment also +# carries a generic walkthrough/noise marker. Surfacing it is what triggers the +# configured independent fallback; hiding it would turn a down reviewer into a +# silent review waiver. +_REVIEW_UNAVAILABLE_MARKERS = ( + "bugbot needs on-demand usage enabled", + "review limit reached", + "rate limited by coderabbit", + "couldn't start this review", + "review skipped", + "no review credits", +) + # Status contexts that are advisory only — they must NEVER block "done". A # review-bot status check can sit PENDING indefinitely after a trivial # follow-up commit (it never auto-incrementally-reviews it), which would @@ -124,14 +145,21 @@ def _gh(args: list[str], *, timeout: int = 60) -> str: cmd = ["gh", *args] try: result = subprocess.run( # noqa: S603 - cmd, capture_output=True, text=True, check=False, cwd=str(REPO_ROOT), timeout=timeout + cmd, + capture_output=True, + text=True, + check=False, + cwd=str(REPO_ROOT), + timeout=timeout, ) except subprocess.TimeoutExpired as exc: # A hung gh call must not wedge the watch loop — surface it as an error # the caller already handles (main catches RuntimeError → exit 2). raise RuntimeError(f"gh {' '.join(args)} timed out after {timeout}s") from exc if result.returncode != 0: - raise RuntimeError(f"gh {' '.join(args)} failed (exit {result.returncode}): {result.stderr.strip()}") + raise RuntimeError( + f"gh {' '.join(args)} failed (exit {result.returncode}): {result.stderr.strip()}" + ) return result.stdout @@ -217,14 +245,23 @@ def summarize_checks(rollup: list[dict]) -> dict: (non-informational) check. """ terminal_ok = {"SUCCESS", "NEUTRAL", "SKIPPED"} - bad = {"FAILURE", "ERROR", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED", "STARTUP_FAILURE"} - success = pending = informational = 0 + bad = { + "FAILURE", + "ERROR", + "CANCELLED", + "TIMED_OUT", + "ACTION_REQUIRED", + "STARTUP_FAILURE", + } + success = pending = informational = informational_non_green = 0 failing: list[dict] = [] for c in rollup: status = (c.get("conclusion") or c.get("state") or "").upper() name = c.get("name") or c.get("context") or "check" if name.strip().lower() in _INFORMATIONAL_CHECK_NAMES: informational += 1 + if status not in terminal_ok: + informational_non_green += 1 continue # advisory only — never blocks "done" if status in terminal_ok: success += 1 @@ -238,6 +275,7 @@ def summarize_checks(rollup: list[dict]) -> dict: "success": success, "pending": pending, "informational": informational, + "informational_non_green": informational_non_green, "failing": failing, "all_green": not failing and pending == 0 and blocking_total > 0, } @@ -279,8 +317,17 @@ def _author(raw: dict) -> str: return str(a) +def review_unavailable_reason(body: str) -> str | None: + low = (body or "").lower() + return next( + (marker for marker in _REVIEW_UNAVAILABLE_MARKERS if marker in low), None + ) + + def is_noise(body: str) -> bool: low = (body or "").lower() + if review_unavailable_reason(body) is not None: + return False return any(marker in low for marker in _NOISE_MARKERS) @@ -294,6 +341,7 @@ def collect_comments(view: dict, inline: list[dict]) -> list[dict]: out: list[dict] = [] for raw in view.get("comments") or []: body = raw.get("body") or "" + unavailable = review_unavailable_reason(body) out.append( { "key": _comment_key("issue", raw), @@ -303,6 +351,7 @@ def collect_comments(view: dict, inline: list[dict]) -> list[dict]: "path": None, "line": None, "body": body, + "review_unavailable_reason": unavailable, } ) for raw in view.get("reviews") or []: @@ -318,6 +367,7 @@ def collect_comments(view: dict, inline: list[dict]) -> list[dict]: "path": None, "line": None, "body": body, + "review_unavailable_reason": review_unavailable_reason(body), } ) for raw in inline or []: @@ -331,6 +381,7 @@ def collect_comments(view: dict, inline: list[dict]) -> list[dict]: "path": raw.get("path"), "line": raw.get("line") or raw.get("original_line"), "body": body, + "review_unavailable_reason": review_unavailable_reason(body), } ) return out @@ -344,18 +395,37 @@ def new_actionable(comments: list[dict], seen: set[str]) -> list[dict]: finding under a fresh id (or an edit that bumps ``updated_at`` / re-homes the line) is recognized as already handled instead of read twice. """ - return [c for c in comments if c["key"] not in seen and c["content_key"] not in seen and not is_noise(c["body"])] - - -def decide_done(checks: dict, new_items: list[dict], *, settling: bool = False) -> bool: - """Done = checks all green AND nothing new to act on AND not mid-settle. + return [ + c + for c in comments + if c["key"] not in seen + and c["content_key"] not in seen + and not is_noise(c["body"]) + ] + + +def decide_done( + checks: dict, + new_items: list[dict], + *, + merge_blockers: list[str] | None = None, + review_evidence: bool = False, + settling: bool = False, +) -> bool: + """Done = green, independently reviewed, merge-ready, and not mid-settle. ``settling`` is set right after a push (the PR head SHA moved, or the rollup is smaller than the largest seen for this head — new checks not yet registered), so a poll can't false-settle on the *stale pre-push* rollup (an all-green old commit) before the new commit's CI even starts. """ - return checks["all_green"] and not new_items and not settling + return ( + checks["all_green"] + and not new_items + and not merge_blockers + and review_evidence + and not settling + ) # ------------------------------------------------------------------ state I/O @@ -385,7 +455,9 @@ def load_state(pr: int) -> dict: def save_state(pr: int, state: dict) -> None: STATE_DIR.mkdir(parents=True, exist_ok=True) - _seen_path(pr).write_text(json.dumps(state, indent=1, sort_keys=True), encoding="utf-8") + _seen_path(pr).write_text( + json.dumps(state, indent=1, sort_keys=True), encoding="utf-8" + ) def load_seen(pr: int) -> set[str]: @@ -435,6 +507,40 @@ def mark_seen(pr: int) -> dict: return {"pr": pr, "marked_seen": True, "marked_seen_keys": sorted(pending)} +def record_review(pr: int, source: str, expected_head: str) -> dict: + """Persist independent-review evidence bound to the PR's current head SHA. + + The caller runs this only after an independent reviewer (the configured bot, + or the configured fallback when that bot is unavailable) has completed and + every finding has been handled. A later push changes ``headRefOid`` and + automatically invalidates the receipt. + """ + source = source.strip() + if not source: + raise ValueError("review source must not be empty") + expected_head = expected_head.strip() + if not expected_head: + raise ValueError("expected reviewed head must not be empty") + snapshot = _gh_json(["pr", "view", str(pr), "--json", "number,headRefOid"]) + current_head = snapshot.get("headRefOid") + if not current_head: + raise ValueError("PR has no headRefOid; cannot bind review evidence") + if current_head != expected_head: + raise ValueError( + f"PR head changed during review (expected {expected_head}, current {current_head}); " + "review the new head before recording evidence" + ) + receipt = { + "head": expected_head, + "source": source, + "recorded_at": datetime.now(timezone.utc).isoformat(), + } + state = load_state(pr) + state["review_receipt"] = receipt + save_state(pr, state) + return {"pr": pr, "recorded_review": True, "review_receipt": receipt} + + # ----------------------------------------------------------------------- main @@ -450,12 +556,14 @@ def build_report( *, prior_head: str | None = None, prior_max_total: int = 0, + review_receipt: dict | None = None, ) -> dict: """Assemble the JSON-serializable watch report for one PR snapshot. Returns a dict with: - - ``pr`` / ``url`` / ``is_draft`` / ``merge_state`` — PR identity + state. + - ``pr`` / ``url`` / ``state`` / ``is_draft`` / ``base`` / ``merge_state`` / + ``review_decision`` — PR identity + merge/review state. - ``head`` — the PR head SHA (``headRefOid``); ``head_changed`` — true when it moved since ``prior_head``; ``max_total`` — the largest check count seen for this head (persisted across runs); ``settling`` — true while a just-pushed @@ -472,8 +580,13 @@ def build_report( - ``all_seen_keys`` — the persistence set ``--mark-seen`` writes: BOTH the id key AND the content key of every current comment, so a later re-post under a new id stays handled. - - ``done`` — :func:`decide_done`: all checks green AND no fresh comments AND - not ``settling``. + - ``review_evidence`` — whether a persisted independent-review receipt is + bound to this exact head SHA. + - ``merge_blockers`` — deterministic reasons the PR is not currently safe to + merge (draft, blocked/unknown merge state, requested changes, non-open PR, + or missing current-head review evidence). + - ``done`` — :func:`decide_done`: all checks green, current-head review + evidence, no fresh comments, no merge blockers, and not ``settling``. """ checks = summarize_checks(view.get("statusCheckRollup") or []) comments = collect_comments(view, inline) @@ -489,14 +602,54 @@ def build_report( head_changed = bool(prior_head) and head is not None and head != prior_head # On a head change, reset the baseline to the new commit's current count; # otherwise remember the largest count ever seen for this head. - max_total = checks["total"] if head_changed else max(prior_max_total, checks["total"]) + max_total = ( + checks["total"] if head_changed else max(prior_max_total, checks["total"]) + ) settling = head_changed or checks["total"] < max_total - return { + pr_state = (view.get("state") or "UNKNOWN").upper() + base = view.get("baseRefName") + merge_state = (view.get("mergeStateStatus") or "UNKNOWN").upper() + review_decision = (view.get("reviewDecision") or "").upper() + receipt_head = ( + review_receipt.get("head") if isinstance(review_receipt, dict) else None + ) + review_evidence = { + "valid": bool(head) and receipt_head == head, + "source": ( + review_receipt.get("source") + if isinstance(review_receipt, dict) and receipt_head == head + else None + ), + "head": receipt_head, + } + merge_blockers: list[str] = [] + if pr_state != "OPEN": + merge_blockers.append(f"PR state is {pr_state}") + if bool(view.get("isDraft")): + merge_blockers.append("PR is draft") + informational_only_unstable = ( + merge_state == "UNSTABLE" + and checks["all_green"] + and checks["informational_non_green"] > 0 + ) + if merge_state not in {"CLEAN", "HAS_HOOKS"} and not informational_only_unstable: + merge_blockers.append(f"merge state is {merge_state}") + if review_decision == "CHANGES_REQUESTED": + merge_blockers.append("review decision is CHANGES_REQUESTED") + if not review_evidence["valid"]: + merge_blockers.append("independent review evidence is missing for current head") + + report = { "pr": view.get("number"), "url": view.get("url"), + "state": pr_state, "is_draft": view.get("isDraft"), - "merge_state": view.get("mergeStateStatus"), + "base": base, + "merge_state": merge_state, + "review_decision": review_decision, + "review_evidence": review_evidence, + "merge_blockers": merge_blockers, "head": head, "head_changed": head_changed, "settling": settling, @@ -509,6 +662,7 @@ def build_report( "path": c["path"], "line": c["line"], "excerpt": _excerpt(c["body"]), + "review_unavailable_reason": c.get("review_unavailable_reason"), # Full body too: handling a finding no longer needs a second # `gh api .../pulls/N/comments` fetch for the suggested diff. "body": c["body"], @@ -518,9 +672,18 @@ def build_report( "all_comment_keys": [c["key"] for c in comments], # Persistence set for --mark-seen: BOTH id and content keys, so a later # re-post under a new id is matched on content and stays handled. - "all_seen_keys": sorted({k for c in comments for k in (c["key"], c["content_key"])}), - "done": decide_done(checks, fresh, settling=settling), + "all_seen_keys": sorted( + {k for c in comments for k in (c["key"], c["content_key"])} + ), } + report["done"] = decide_done( + checks, + fresh, + merge_blockers=merge_blockers, + review_evidence=review_evidence["valid"], + settling=settling, + ) + return report def render(report: dict) -> str: @@ -538,19 +701,37 @@ def render(report: dict) -> str: ) for f in ck["failing"]: lines.append(f" ✗ {f['name']} ({f['status']})") + for blocker in report.get("merge_blockers") or []: + lines.append(f" ✗ merge blocker: {blocker}") if report["new_comments"]: lines.append(f"new comments to address ({len(report['new_comments'])}):") for c in report["new_comments"]: loc = f" {c['path']}:{c['line']}" if c["path"] else "" - lines.append(f" • [{c['kind']}] @{c['author']}{loc}: {c['excerpt']}") + if c.get("review_unavailable_reason"): + lines.append( + f" • [review unavailable] @{c['author']}{loc}: " + f"{c['review_unavailable_reason']} — run the configured fallback review" + ) + else: + lines.append(f" • [{c['kind']}] @{c['author']}{loc}: {c['excerpt']}") return "\n".join(lines) +def render_record_review(report: dict) -> str: + receipt = report["review_receipt"] + return ( + f"PR #{report['pr']} — recorded independent review from " + f"{receipt['source']} for head {receipt['head']}" + ) + + def render_mark_seen(report: dict) -> str: pr = report.get("pr") keys = report.get("marked_seen_keys") or [] if keys: - return f"PR #{pr} — acked {len(keys)} comment key(s) from the last reported poll" + return ( + f"PR #{pr} — acked {len(keys)} comment key(s) from the last reported poll" + ) return f"PR #{pr} — {report.get('note', 'nothing to acknowledge')}" @@ -584,14 +765,28 @@ def persist_poll(pr: int, report: dict, seen: set[str]) -> dict: "max_total": report["max_total"], "pending_seen": report["all_seen_keys"], } + previous = load_state(pr) + if isinstance(previous.get("review_receipt"), dict): + new_state["review_receipt"] = previous["review_receipt"] save_state(pr, new_state) return new_state def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("pr", nargs="?", type=int, default=None, help="PR number (default: current branch's PR)") + parser.add_argument( + "pr", + nargs="?", + type=int, + default=None, + help="PR number (default: current branch's PR)", + ) parser.add_argument("--json", action="store_true", help="machine-readable output") + parser.add_argument( + "--head", + metavar="EXPECTED_SHA", + help="exact head SHA reviewed; required with --record-review", + ) mode_group = parser.add_mutually_exclusive_group() mode_group.add_argument( "--mark-seen", @@ -601,6 +796,14 @@ def main(argv: list[str] | None = None) -> int: "what that poll showed (no fresh gh re-poll); call after addressing a round" ), ) + mode_group.add_argument( + "--record-review", + metavar="SOURCE", + help=( + "after an independent review and all fixes, persist a receipt bound to " + "the PR's current head (for example fallback:codex)" + ), + ) mode_group.add_argument( "--assert-draft", action="store_true", @@ -618,6 +821,10 @@ def main(argv: list[str] | None = None) -> int: ), ) args = parser.parse_args(argv) + if args.record_review is not None and not args.head: + parser.error("--record-review requires --head ") + if args.head and args.record_review is None: + parser.error("--head is only valid with --record-review") try: pr = resolve_pr(args.pr) @@ -636,6 +843,18 @@ def main(argv: list[str] | None = None) -> int: print(render_mark_seen(mark_report)) return 0 + if args.record_review is not None: + try: + review_report = record_review(pr, args.record_review, args.head) + except (RuntimeError, KeyError, ValueError) as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 + if args.json: + print(json.dumps(review_report, ensure_ascii=False)) + else: + print(render_record_review(review_report)) + return 0 + if args.assert_draft or args.assert_ready: try: draft_report = assert_draft_state(pr, want_draft=args.assert_draft) @@ -655,10 +874,12 @@ def main(argv: list[str] | None = None) -> int: "view", str(pr), "--json", - "number,title,url,isDraft,mergeStateStatus,headRefOid,statusCheckRollup,reviews,comments", + "number,title,url,state,isDraft,baseRefName,mergeStateStatus,reviewDecision,headRefOid,statusCheckRollup,reviews,comments", ] ) - inline = _gh_json(["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments", "--paginate"]) + inline = _gh_json( + ["api", f"repos/{{owner}}/{{repo}}/pulls/{pr}/comments", "--paginate"] + ) except (RuntimeError, KeyError, ValueError) as exc: print(f"error: {exc}", file=sys.stderr) return 2 @@ -666,7 +887,12 @@ def main(argv: list[str] | None = None) -> int: state = load_state(pr) seen = set(state.get("seen", [])) report = build_report( - view, inline, seen, prior_head=state.get("head"), prior_max_total=int(state.get("max_total") or 0) + view, + inline, + seen, + prior_head=state.get("head"), + prior_max_total=int(state.get("max_total") or 0), + review_receipt=state.get("review_receipt"), ) persist_poll(pr, report, seen) diff --git a/scripts/reconcile_sessions.sh b/scripts/reconcile_sessions.sh index 2129539..60a67fd 100755 --- a/scripts/reconcile_sessions.sh +++ b/scripts/reconcile_sessions.sh @@ -26,7 +26,7 @@ # prints before writing its block. # # Usage: -# scripts/reconcile_sessions.sh [...] [--prefix dev] [--base main] +# scripts/reconcile_sessions.sh [...] [--prefix ] [--base ] # scripts/reconcile_sessions.sh --match '' [--match ''] ... # scripts/reconcile_sessions.sh # discover in-flight lanes # @@ -54,11 +54,23 @@ REPO_ROOT="$(devkit_find_repo_root "$SCRIPT_DIR")" || { echo "[reconcile] error: no .git repository found above $SCRIPT_DIR" >&2 exit 64 } +CONFIG_FILE="$REPO_ROOT/config/dev-model.yaml" +CONFIGURED_PROTECTED_BRANCH="$(devkit_config_scalar "$CONFIG_FILE" vcs "" protected_branch || true)" +[[ -n "$CONFIGURED_PROTECTED_BRANCH" ]] || { + echo "[reconcile] error: config must define vcs.protected_branch" >&2 + exit 1 +} +git check-ref-format --branch "$CONFIGURED_PROTECTED_BRANCH" >/dev/null 2>&1 || { + echo "[reconcile] error: invalid vcs.protected_branch '$CONFIGURED_PROTECTED_BRANCH'" >&2 + exit 1 +} +DEFAULT_BASE="${DEV_SESSION_BASE:-$CONFIGURED_PROTECTED_BRANCH}" +CONFIGURED_PREFIX="$(devkit_config_scalar "$CONFIG_FILE" vcs "" dev_branch_prefix || true)" # Sessions container — mirror dev_session.sh so no-arg discovery lines up with # the sibling that created the sessions. SESSIONS_DIR="${DEVKIT_SESSIONS_DIR:-$(dirname "$REPO_ROOT")/dev-model-sessions}" -DEFAULT_PREFIX="${DEV_SESSION_PREFIX:-dev}" +DEFAULT_PREFIX="${DEV_SESSION_PREFIX:-${CONFIGURED_PREFIX:-dev}}" _die() { echo "[reconcile] error: $*" >&2 @@ -155,7 +167,7 @@ _add_lane() { } cmd_reconcile() { - local prefix="$DEFAULT_PREFIX" base="main" + local prefix="$DEFAULT_PREFIX" base="$DEFAULT_BASE" local scopes=() match_globs=() while [[ $# -gt 0 ]]; do case "$1" in diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index 053d5e4..50fae14 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -1,11 +1,14 @@ from __future__ import annotations import importlib.util +import json +import os import shutil import subprocess from pathlib import Path from types import ModuleType +import pytest import yaml @@ -33,9 +36,7 @@ def _load_module(name: str, path: Path) -> ModuleType: def _install_nested_shell_engines(repo: Path) -> Path: engine_dir = repo / "scripts" / "devkit" (engine_dir / "lib").mkdir(parents=True) - shutil.copy2( - ENGINE_DIR / "dev_session.sh", engine_dir / "dev_session.sh" - ) + shutil.copy2(ENGINE_DIR / "dev_session.sh", engine_dir / "dev_session.sh") shutil.copy2( ENGINE_DIR / "reconcile_sessions.sh", engine_dir / "reconcile_sessions.sh", @@ -63,6 +64,54 @@ def _install_nested_shell_engines(repo: Path) -> Path: return engine_dir +def _git(cwd: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], cwd=cwd, check=True, capture_output=True, text=True + ) + + +def _install_real_trunk_repo(tmp_path: Path) -> tuple[Path, Path, Path]: + remote = tmp_path / "origin.git" + subprocess.run( + ["git", "init", "--bare", str(remote)], + check=True, + capture_output=True, + text=True, + ) + repo = tmp_path / "project" + repo.mkdir() + _git(repo, "init", "-b", "trunk") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "Test") + (repo / "README.md").write_text("seed\n", encoding="utf-8") + (repo / ".gitignore").write_text("state/\n.devkit_state_root\n", encoding="utf-8") + _git(repo, "add", "README.md", ".gitignore") + _git(repo, "commit", "-m", "seed") + _git(repo, "remote", "add", "origin", str(remote)) + _git(repo, "push", "-u", "origin", "trunk") + + engine_dir = repo / "scripts" / "devkit" + shutil.copytree(ENGINE_DIR, engine_dir) + (repo / "config").mkdir() + (repo / "config" / "dev-model.yaml").write_text( + """paths: + handoff: handoff.md + friction_log: friction-log.md +runtime: + default: codex + launchers: + claude: claude + codex: codex +vcs: + protected_branch: trunk + dev_branch_prefix: lane +""", + encoding="utf-8", + ) + sessions = tmp_path / "sessions" + return repo, engine_dir, sessions + + def test_nested_shell_engines_find_the_repository_root(tmp_path: Path) -> None: repo = tmp_path / "project" engine_dir = _install_nested_shell_engines(repo) @@ -110,6 +159,453 @@ def test_nested_lane_contract_uses_configured_paths(tmp_path: Path) -> None: assert "never trunk" in result.stdout +def test_real_headless_lane_uses_configured_base_and_replaces_inherited_state( + tmp_path: Path, +) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + inherited = tmp_path / "cockpit-state" + env = { + **os.environ, + "DEVKIT_SESSIONS_DIR": str(sessions), + "DEVKIT_STATE_ROOT": str(inherited), + } + + result = subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "new", + "probe", + "--headless", + "--merge-class", + "self", + ], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + descriptor = json.loads(result.stdout) + + assert descriptor["base"] == "trunk" + assert descriptor["branch"] == "lane/probe" + assert descriptor["merge_class"] == "self" + assert descriptor["env"]["DEVKIT_STATE_ROOT"] == descriptor["state_root"] + assert descriptor["env"]["DEVKIT_STATE_ROOT"] != str(inherited) + assert descriptor["env"]["DEVKIT_ROOT"] == str(repo) + assert ( + Path(descriptor["worktree"], ".devkit_state_root").read_text().strip() + == descriptor["state_root"] + ) + assert (sessions / "probe" / "merge_class").read_text().strip() == "self" + + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + fake_gh = fake_bin / "gh" + fake_gh.write_text("#!/bin/sh\nprintf '[]\\n'\n", encoding="utf-8") + fake_gh.chmod(0o755) + reconcile = subprocess.run( + ["bash", str(engine_dir / "reconcile_sessions.sh"), "probe"], + cwd=repo, + env={**env, "PATH": f"{fake_bin}:{env['PATH']}"}, + check=False, + capture_output=True, + text=True, + ) + assert "EMPTY — 0 commits, never started" in reconcile.stdout + + subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "rm", + "probe", + "--keep-branch", + ], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + assert not (sessions / "probe").exists() + + +def test_force_recreate_refuses_configured_protected_branch_before_mutation( + tmp_path: Path, +) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + env = {**os.environ, "DEVKIT_SESSIONS_DIR": str(sessions)} + subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + + refused = subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "new", + "probe", + "--base", + "main", + "--branch", + "trunk", + "--force", + ], + cwd=repo, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert refused.returncode != 0 + assert "refusing to use protected branch 'trunk'" in refused.stderr + assert _git(repo, "show-ref", "--verify", "refs/heads/trunk").returncode == 0 + assert (sessions / "probe" / "wt").is_dir() + + +def test_single_quoted_protected_branch_is_still_protected(tmp_path: Path) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + env = {**os.environ, "DEVKIT_SESSIONS_DIR": str(sessions)} + subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + config = repo / "config" / "dev-model.yaml" + config.write_text( + config.read_text(encoding="utf-8").replace( + "protected_branch: trunk", "protected_branch: 'trunk'" + ), + encoding="utf-8", + ) + + refused = subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "new", + "probe", + "--base", + "main", + "--branch", + "trunk", + "--force", + ], + cwd=repo, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert refused.returncode != 0 + assert "refusing to use protected branch 'trunk'" in refused.stderr + assert _git(repo, "show-ref", "--verify", "refs/heads/trunk").returncode == 0 + assert (sessions / "probe" / "wt").is_dir() + + +@pytest.mark.parametrize( + "vcs_block,expected_error", + [ + ("vcs:\n dev_branch_prefix: lane\n", "must define vcs.protected_branch"), + ( + "vcs:\n protected_branch: 'not a branch'\n dev_branch_prefix: lane\n", + "invalid vcs.protected_branch", + ), + ], +) +def test_missing_or_invalid_protected_branch_fails_before_mutation( + tmp_path: Path, vcs_block: str, expected_error: str +) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + env = {**os.environ, "DEVKIT_SESSIONS_DIR": str(sessions)} + subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + config = repo / "config" / "dev-model.yaml" + before_vcs = "vcs:\n protected_branch: trunk\n dev_branch_prefix: lane\n" + config.write_text( + config.read_text(encoding="utf-8").replace(before_vcs, vcs_block), + encoding="utf-8", + ) + + refused = subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "new", + "probe", + "--base", + "main", + "--branch", + "trunk", + "--force", + ], + cwd=repo, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert refused.returncode != 0 + assert expected_error in refused.stderr + assert _git(repo, "show-ref", "--verify", "refs/heads/trunk").returncode == 0 + assert (sessions / "probe" / "wt").is_dir() + + +def test_operator_merge_class_refuses_before_contacting_github(tmp_path: Path) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + env = {**os.environ, "DEVKIT_SESSIONS_DIR": str(sessions)} + subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "new", "probe", "--headless"], + cwd=repo, + env=env, + check=True, + capture_output=True, + text=True, + ) + + result = subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], + cwd=repo, + env={**env, "PATH": "/usr/bin:/bin"}, + check=False, + capture_output=True, + text=True, + ) + + assert result.returncode != 0 + assert "operator-merge" in result.stderr + + +def _prepare_self_merge_session(sessions: Path) -> Path: + session = sessions / "probe" + (session / "state").mkdir(parents=True) + (session / "merge_class").write_text("self\n", encoding="utf-8") + (session / "branch").write_text("lane/probe\n", encoding="utf-8") + (session / "base").write_text("trunk\n", encoding="utf-8") + return session + + +def _install_fake_merge_tools(tmp_path: Path) -> tuple[Path, Path, Path]: + fake_bin = tmp_path / "merge-bin" + fake_bin.mkdir() + call_log = tmp_path / "gh-calls.log" + uv_log = tmp_path / "uv-calls.log" + gh = fake_bin / "gh" + gh.write_text( + """#!/bin/sh +printf '%s|%s|%s\n' "$PWD" "${GH_REPO:-unset}" "$*" >> "$CALL_LOG" +if [ "$1 $2" = "repo view" ]; then + printf '{"nameWithOwner":"%s"}\n' "${GH_REPO:-owner/project}" +elif [ "$1 $2" = "pr list" ]; then + printf '%s\n' "$PR_JSON" +elif [ "$1 $2" = "pr merge" ]; then + exit "${MERGE_EXIT:-0}" +else + exit 91 +fi +""", + encoding="utf-8", + ) + gh.chmod(0o755) + uv = fake_bin / "uv" + uv.write_text( + """#!/bin/sh +printf '%s|%s|%s\n' "$DEVKIT_STATE_ROOT" "${GH_REPO:-unset}" "$*" >> "$UV_LOG" +printf '%s\n' "$REPORT_JSON" +""", + encoding="utf-8", + ) + uv.chmod(0o755) + return fake_bin, call_log, uv_log + + +def test_self_merge_refuses_wrong_base_and_binds_gh_to_repo(tmp_path: Path) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + _prepare_self_merge_session(sessions) + fake_bin, call_log, uv_log = _install_fake_merge_tools(tmp_path) + caller = tmp_path / "unrelated-caller" + caller.mkdir() + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "DEVKIT_SESSIONS_DIR": str(sessions), + "CALL_LOG": str(call_log), + "UV_LOG": str(uv_log), + "PR_JSON": json.dumps( + [ + { + "number": 8, + "baseRefName": "wrong-base", + "headRefName": "lane/probe", + "headRefOid": "listed-head", + "headRepositoryOwner": {"login": "owner"}, + } + ] + ), + "REPORT_JSON": "{}", + "GH_REPO": "attacker/other", + } + + result = subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], + cwd=caller, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert result.returncode != 0 + assert "not recorded base 'trunk'" in result.stderr + assert not uv_log.exists() + assert all( + line.startswith(f"{repo}|") for line in call_log.read_text().splitlines() + ) + calls = call_log.read_text(encoding="utf-8") + assert f"{repo}|unset|repo view" in calls + assert f"{repo}|owner/project|pr list" in calls + + +def test_self_merge_pins_validated_head_so_push_race_is_refused(tmp_path: Path) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + session = _prepare_self_merge_session(sessions) + fake_bin, call_log, uv_log = _install_fake_merge_tools(tmp_path) + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "DEVKIT_SESSIONS_DIR": str(sessions), + "CALL_LOG": str(call_log), + "UV_LOG": str(uv_log), + "PR_JSON": json.dumps( + [ + { + "number": 8, + "baseRefName": "trunk", + "headRefName": "lane/probe", + "headRefOid": "listed-head", + "headRepositoryOwner": {"login": "owner"}, + } + ] + ), + "REPORT_JSON": json.dumps( + {"pr": 8, "base": "trunk", "head": "reviewed-head", "done": True} + ), + # Simulate GitHub rejecting --match-head-commit because a new push won + # the race after the act-time poll. + "MERGE_EXIT": "17", + "GH_REPO": "attacker/other", + } + + result = subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], + cwd=tmp_path, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert result.returncode != 0 + assert "GitHub merge failed" in result.stderr + calls = call_log.read_text(encoding="utf-8") + assert f"{repo}|unset|repo view" in calls + assert f"{repo}|owner/project|pr list" in calls + assert f"{repo}|owner/project|pr merge" in calls + assert ( + "pr merge --repo owner/project 8 --squash --delete-branch --match-head-commit reviewed-head" + in calls + ) + assert uv_log.read_text(encoding="utf-8").startswith(f"{session / 'state'}|") + + +def test_scope_pr_watch_and_merge_share_lane_state_and_pinned_repo( + tmp_path: Path, +) -> None: + repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + session = _prepare_self_merge_session(sessions) + fake_bin, call_log, uv_log = _install_fake_merge_tools(tmp_path) + env = { + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "DEVKIT_SESSIONS_DIR": str(sessions), + "CALL_LOG": str(call_log), + "UV_LOG": str(uv_log), + "PR_JSON": json.dumps( + [ + { + "number": 8, + "baseRefName": "trunk", + "headRefName": "lane/probe", + "headRefOid": "reviewed-head", + "headRepositoryOwner": {"login": "owner"}, + } + ] + ), + "REPORT_JSON": json.dumps( + {"pr": 8, "base": "trunk", "head": "reviewed-head", "done": True} + ), + "GH_REPO": "attacker/other", + } + + record = subprocess.run( + [ + "bash", + str(engine_dir / "dev_session.sh"), + "pr-watch", + "probe", + "--record-review", + "fallback:codex", + "--head", + "reviewed-head", + ], + cwd=tmp_path, + env=env, + check=False, + capture_output=True, + text=True, + ) + merge = subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], + cwd=tmp_path, + env=env, + check=False, + capture_output=True, + text=True, + ) + + assert record.returncode == 0, record.stderr + assert merge.returncode == 0, merge.stderr + uv_calls = uv_log.read_text(encoding="utf-8").splitlines() + expected_prefix = f"{session / 'state'}|owner/project|" + assert len(uv_calls) == 2 + assert all(line.startswith(expected_prefix) for line in uv_calls) + assert "--record-review fallback:codex --head reviewed-head" in uv_calls[0] + assert uv_calls[1].endswith("8 --json") + gh_calls = call_log.read_text(encoding="utf-8") + assert "|attacker/other|" not in gh_calls + assert f"{repo}|unset|repo view" in gh_calls + assert f"{repo}|owner/project|pr merge" in gh_calls + + def test_archive_defaults_follow_configured_paths(tmp_path: Path) -> None: repo = tmp_path / "project" config_path = repo / "config" / "dev-model.yaml" @@ -198,6 +694,161 @@ def test_archive_supports_recent_sessions_layout(tmp_path: Path) -> None: ) +def test_recent_session_nested_h3_stays_inside_its_dated_block(tmp_path: Path) -> None: + archive = _load_module( + "archive_recent_nested_heading", ENGINE_DIR / "archive_plan_sessions.py" + ) + plan = tmp_path / "handoff.md" + history = tmp_path / "handoff-history.md" + plan.write_text( + """# Handoff + +## Recent sessions + +### 2026-07-03 — Newest + +Newest body. + +### 2026-07-02 — Older + +Older body. + +### Validation + +Validation belongs to the older session. + +## Backlog +""", + encoding="utf-8", + ) + history.write_text( + "# History\n\n## Recent sessions (archived)\n", + encoding="utf-8", + ) + + assert ( + archive.main(["--keep", "1", "--plan", str(plan), "--history", str(history)]) + == 0 + ) + archived = history.read_text(encoding="utf-8") + assert "### 2026-07-02 — Older" in archived + assert "### Validation\n\nValidation belongs to the older session." in archived + + +def test_archive_explicit_paths_do_not_require_config( + tmp_path: Path, monkeypatch +) -> None: + archive = _load_module( + "archive_explicit_paths", ENGINE_DIR / "archive_plan_sessions.py" + ) + monkeypatch.setattr( + archive, + "configured_paths", + lambda: (_ for _ in ()).throw(AssertionError("config must not be read")), + ) + plan = tmp_path / "handoff.md" + history = tmp_path / "handoff-history.md" + plan.write_text( + "# Handoff\n\n## Latest session — One\n\nBody.\n", + encoding="utf-8", + ) + history.write_text("# History\n\n## Session log\n", encoding="utf-8") + + assert ( + archive.main(["--plan", str(plan), "--history", str(history), "--dry-run"]) == 0 + ) + with pytest.raises(SystemExit) as exc: + archive.main(["--help"]) + assert exc.value.code == 0 + + +def test_archive_pointer_follows_configured_history_location(tmp_path: Path) -> None: + archive = _load_module( + "archive_dynamic_pointer", ENGINE_DIR / "archive_plan_sessions.py" + ) + plan = tmp_path / "handoff.md" + history = tmp_path / "saved" / "handoff-history.md" + history.parent.mkdir() + plan.write_text( + """# Handoff + +## Latest session — New + +New. + +## Earlier session — Old + +Old. +""", + encoding="utf-8", + ) + history.write_text("# History\n\n## Session log\n", encoding="utf-8") + + assert ( + archive.main(["--keep", "1", "--plan", str(plan), "--history", str(history)]) + == 0 + ) + assert "](saved/handoff-history.md)" in plan.read_text(encoding="utf-8") + + +def test_init_migrates_the_previous_runtime_schema(tmp_path: Path) -> None: + repo = tmp_path / "project" + (repo / "config").mkdir(parents=True) + shutil.copy2(REPO_ROOT / "init.sh", repo / "init.sh") + (repo / "config" / "dev-model.yaml").write_text( + """project: + name: old-project +paths: + handoff: docs/handoff.md + handoff_history: docs/handoff-history.md + friction_log: docs/friction-log.md + friction_log_archive: docs/friction-log-archive.md +doc_budgets: [] +vcs: + protected_branch: trunk +tracker: + backend: none + project_name: "Old" + linear: + team_id: "" + project_id: "" +review: + bots: [] + fallback_command: "/code-review" +notify: + user_key: "" +models: + cheap: tiny + default: normal + expensive: large +state: + dirname: state +""", + encoding="utf-8", + ) + + subprocess.run( + ["sh", "init.sh"], + cwd=repo, + check=True, + capture_output=True, + text=True, + ) + config = yaml.safe_load( + (repo / "config" / "dev-model.yaml").read_text(encoding="utf-8") + ) + + assert config["paths"]["engines"] == "scripts" + assert config["runtime"]["default"] == "claude" + assert config["runtime"]["launchers"]["codex"] == "codex" + assert config["review"]["fallback_commands"]["codex"] == "/review" + assert config["models"]["runtime_mappings"]["claude"] == { + "cheap": "tiny", + "default": "normal", + "expensive": "large", + } + + def test_python_engine_root_walk_supports_namespacing(tmp_path: Path) -> None: repo = tmp_path / "project" nested_script = repo / "scripts" / "devkit" / "pr_watch.py" diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py new file mode 100644 index 0000000..2260859 --- /dev/null +++ b/scripts/tests/test_pr_watch.py @@ -0,0 +1,258 @@ +from __future__ import annotations + +import importlib.util +from pathlib import Path +from types import ModuleType + +import pytest + + +ENGINE_DIR = Path(__file__).resolve().parent.parent + + +def _load_pr_watch() -> ModuleType: + spec = importlib.util.spec_from_file_location( + "pr_watch_under_test", ENGINE_DIR / "pr_watch.py" + ) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _green_view(**overrides): + view = { + "number": 7, + "url": "https://example.test/pr/7", + "state": "OPEN", + "isDraft": False, + "baseRefName": "trunk", + "mergeStateStatus": "CLEAN", + "reviewDecision": "", + "headRefOid": "abc123", + "statusCheckRollup": [ + {"name": "tests", "status": "COMPLETED", "conclusion": "SUCCESS"} + ], + "comments": [], + "reviews": [], + } + view.update(overrides) + return view + + +def test_changes_requested_and_blocked_merge_state_never_settle_done() -> None: + pr_watch = _load_pr_watch() + review = { + "id": "review-1", + "author": {"login": "reviewer"}, + "body": "Please fix the unsafe branch deletion.", + } + view = _green_view( + mergeStateStatus="BLOCKED", + reviewDecision="CHANGES_REQUESTED", + reviews=[review], + ) + comments = pr_watch.collect_comments(view, []) + seen = { + key for comment in comments for key in (comment["key"], comment["content_key"]) + } + + report = pr_watch.build_report(view, [], seen) + + assert report["new_comments"] == [] + assert report["done"] is False + assert "merge state is BLOCKED" in report["merge_blockers"] + assert "review decision is CHANGES_REQUESTED" in report["merge_blockers"] + + +def test_unknown_or_non_open_pr_state_never_settles_done() -> None: + pr_watch = _load_pr_watch() + + unknown = pr_watch.build_report(_green_view(mergeStateStatus="UNKNOWN"), [], set()) + merged = pr_watch.build_report( + _green_view(state="MERGED", mergeStateStatus="UNKNOWN"), [], set() + ) + + assert unknown["done"] is False + assert "merge state is UNKNOWN" in unknown["merge_blockers"] + assert merged["done"] is False + assert "PR state is MERGED" in merged["merge_blockers"] + + +def test_unstable_is_allowed_only_when_remaining_check_is_informational() -> None: + pr_watch = _load_pr_watch() + receipt = {"head": "abc123", "source": "fallback:codex"} + coderabbit_pending = { + "context": "CodeRabbit", + "state": "PENDING", + } + + informational_only = pr_watch.build_report( + _green_view( + mergeStateStatus="UNSTABLE", + statusCheckRollup=[ + {"name": "tests", "conclusion": "SUCCESS"}, + coderabbit_pending, + ], + ), + [], + set(), + review_receipt=receipt, + ) + unexplained_unstable = pr_watch.build_report( + _green_view(mergeStateStatus="UNSTABLE"), + [], + set(), + review_receipt=receipt, + ) + successful_informational = pr_watch.build_report( + _green_view( + mergeStateStatus="UNSTABLE", + statusCheckRollup=[ + {"name": "tests", "conclusion": "SUCCESS"}, + {"context": "CodeRabbit", "state": "SUCCESS"}, + ], + ), + [], + set(), + review_receipt=receipt, + ) + + assert informational_only["done"] is True + assert "merge state is UNSTABLE" not in informational_only["merge_blockers"] + assert unexplained_unstable["done"] is False + assert "merge state is UNSTABLE" in unexplained_unstable["merge_blockers"] + assert successful_informational["done"] is False + assert "merge state is UNSTABLE" in successful_informational["merge_blockers"] + + +def test_review_unavailable_overrides_coderabbit_summary_noise() -> None: + pr_watch = _load_pr_watch() + body = """ +Review limit reached. We couldn't start this review. +""" + view = _green_view( + comments=[{"id": "notice-1", "author": {"login": "coderabbitai"}, "body": body}] + ) + + report = pr_watch.build_report( + view, + [], + set(), + review_receipt={"head": "abc123", "source": "coderabbit"}, + ) + + assert report["done"] is False + assert len(report["new_comments"]) == 1 + assert ( + report["new_comments"][0]["review_unavailable_reason"] == "review limit reached" + ) + + +def test_acknowledged_unavailable_notice_still_needs_review_evidence() -> None: + pr_watch = _load_pr_watch() + body = "Review limit reached. We couldn't start this review." + view = _green_view( + reviewDecision="", + comments=[ + {"id": "notice-1", "author": {"login": "coderabbitai"}, "body": body} + ], + ) + comments = pr_watch.collect_comments(view, []) + seen = { + key for comment in comments for key in (comment["key"], comment["content_key"]) + } + + report = pr_watch.build_report(view, [], seen) + + assert report["new_comments"] == [] + assert report["done"] is False + assert ( + "independent review evidence is missing for current head" + in report["merge_blockers"] + ) + + +def test_review_receipt_must_match_current_head() -> None: + pr_watch = _load_pr_watch() + view = _green_view(reviewDecision="") + + missing = pr_watch.build_report(view, [], set()) + stale = pr_watch.build_report( + view, + [], + set(), + review_receipt={"head": "older", "source": "fallback:codex"}, + ) + current = pr_watch.build_report( + view, + [], + set(), + review_receipt={"head": "abc123", "source": "fallback:codex"}, + ) + + assert missing["done"] is False + assert stale["done"] is False + assert current["done"] is True + assert current["review_evidence"] == { + "valid": True, + "source": "fallback:codex", + "head": "abc123", + } + + +def test_record_review_refuses_push_between_review_and_receipt( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + pr_watch = _load_pr_watch() + monkeypatch.setattr(pr_watch, "STATE_DIR", tmp_path) + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda _args: {"number": 7, "headRefOid": "new-unreviewed-head"}, + ) + + with pytest.raises(ValueError, match="head changed during review"): + pr_watch.record_review(7, "fallback:codex", "reviewed-head") + + assert not (tmp_path / "7.json").exists() + + +def test_record_review_persists_only_expected_current_head( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + pr_watch = _load_pr_watch() + monkeypatch.setattr(pr_watch, "STATE_DIR", tmp_path) + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda _args: {"number": 7, "headRefOid": "reviewed-head"}, + ) + + report = pr_watch.record_review(7, "fallback:codex", "reviewed-head") + + assert report["review_receipt"]["head"] == "reviewed-head" + assert pr_watch.load_state(7)["review_receipt"]["source"] == "fallback:codex" + + +def test_normal_coderabbit_walkthrough_remains_noise() -> None: + pr_watch = _load_pr_watch() + body = """ + +Summary only. +""" + view = _green_view( + comments=[ + {"id": "summary-1", "author": {"login": "coderabbitai"}, "body": body} + ] + ) + + report = pr_watch.build_report( + view, + [], + set(), + review_receipt={"head": "abc123", "source": "coderabbit"}, + ) + + assert report["new_comments"] == [] + assert report["done"] is True