From 2e86a2f4414ebb0841f3c84fb3d8d7d57fa81e77 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:55:29 +0000 Subject: [PATCH 1/2] chore(norms): require the four-section loop PR contract (#73) --- docs/norms/ops.md | 26 +++++++++++++++++++++++++- skills/review/SKILL.md | 14 +++++++++++++- skills/verify/SKILL.md | 7 +++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/docs/norms/ops.md b/docs/norms/ops.md index ece192f..ceaa3c2 100644 --- a/docs/norms/ops.md +++ b/docs/norms/ops.md @@ -87,10 +87,34 @@ Never do work outside the declared stage; never modify norms-layer files events (sweep reconciles drift with `sweep-corrected`). You never write them; you only record runs, checkpoints, and task state. +## The loop PR contract + +A loop PR is identified by its head branch `loop/-*` and a body carrying +`Closes #` and `loop-task: #`; the router pairs the ref with the marker and +requires both to agree on ``. + +Identity is not enough. An agent's reasoning is discarded with its context, so the +PR body is the only place the author's intent survives — and the reviewer must judge +the change against it, not reconstruct it from the diff. Every loop PR body therefore +carries four sections, under these headings: + +1. **What/why** — what changed and why, in one or two sentences. +2. **Proof it works** — the actual evidence: gate/test output, manual steps, logs. A + claim that it should work is not proof. +3. **Risk tier + AI role** — which parts the agent produced, and what breaks if the + change is wrong. +4. **Review focus** — the one or two places human judgment is actually needed. + +The host enforces this at PR creation: a `pr-opened` whose body is missing any section +is refused and downgraded to `failed` (`entry.mjs` `validatePush`), so a newly opened +loop PR always carries all four. The same evidence is collected in the run record +(`skills/verify/SKILL.md`). A reviewer that receives a loop PR missing a section +returns `request-changes` before any line-level review (`skills/review/SKILL.md`). + ## Quick rules - Loop PR = head branch `loop/-*`, body `Closes #` and - `loop-task: #`. + `loop-task: #`, plus the four contract sections above. - Don't claim a task whose lock is live; don't fight a live lock. - No self-merge; no norms-layer edits without a PR; no npm publish by the loop. - Host changes (`scripts/loop/**`) may arrive as loop PRs like any other change: the diff --git a/skills/review/SKILL.md b/skills/review/SKILL.md index 43428d7..4d105a0 100644 --- a/skills/review/SKILL.md +++ b/skills/review/SKILL.md @@ -19,7 +19,17 @@ present because a reviewer needs `git`/`gh` and the check gates, and it can stil through the shell. A reviewer writes its report and result file with a `bash` heredoc and keeps every other command read-only. -Review the diff, not the intent. +## 0. PR-contract gate (both lenses, before line-level review) + +Read the PR body and confirm it carries the four author sections named in +`docs/norms/ops.md` ("The loop PR contract"): **What/why**, **Proof it works**, +**Risk tier + AI role**, **Review focus**. If any is missing or empty, the verdict is +`request-changes`, recorded as an L1 problem naming the missing sections — do not +reconstruct the intent from the diff and do not start line-level review. The author +owes the reviewer intent, proof, risk and review focus; the reviewer's job is to judge +whether they hold, not to guess them. + +Then review the diff against the author's stated intent and proof. ## Reviewer A — Correctness & regression - Does the change do what the task state says it should? @@ -54,6 +64,8 @@ Review the diff, not the intent. must be recorded with a reason in the PR body. ## Done when +- The PR-contract gate passed: the body carried all four author sections (a missing + one was a `request-changes` finding, never skipped in favour of line-level review). - Both reviewer runs issued a verdict into their own session and result file; all L1/L2 problems resolved or explicitly declined with reasons; `verify` gates green; verdict recorded in the run log. diff --git a/skills/verify/SKILL.md b/skills/verify/SKILL.md index bab65d0..f642186 100644 --- a/skills/verify/SKILL.md +++ b/skills/verify/SKILL.md @@ -23,7 +23,14 @@ description: Run the repo's full check gates (lint, format, types, tests, build) skipping). 3. Re-run all gates until green. Record outputs (trimmed) in the run log. 4. Never claim verification success without the final command outputs. +5. Collect the PR-contract evidence while the run is fresh (`docs/norms/ops.md`, + "The loop PR contract"): the gate outputs become the PR's **Proof it works**, and + note the **Risk tier + AI role** and **Review focus** alongside them. A product + stage that opens a PR carries all four in the PR body; the host refuses a + `pr-opened` whose body is missing any of them. ## Done when - All five gates pass with recorded output, or a gate failure is reported as a blocker with evidence. +- For a product stage, the four contract sections exist for the PR body, with the + gate output as proof — or the run reports why it produced no PR. From b132bfd337968eb2572c86d4c64f3a42c216a7a3 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:55:30 +0000 Subject: [PATCH 2/2] chore(loop): enforce the PR contract at PR creation and in the maker prompt (#73) --- scripts/loop/README.md | 14 ++++++++-- scripts/loop/entry.mjs | 14 +++++++++- scripts/loop/shared/agent.mjs | 31 +++++++++++++++++++++- scripts/loop/shared/state.mjs | 48 +++++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 4 deletions(-) diff --git a/scripts/loop/README.md b/scripts/loop/README.md index 0446c74..64b5ccc 100644 --- a/scripts/loop/README.md +++ b/scripts/loop/README.md @@ -261,8 +261,8 @@ reason a future reader may see the parameter and wonder where it came from. | branch check | `applyActions` | `loop/-` only — no other ref is pushed, whatever the branch field says. | | dirty-tree check | `applyActions` | A dirty tree is refused: the commit would silently stay behind and the PR would ship an empty diff. | | workflow-file recognition | `applyActions` | A push rejected for touching `.github/workflows/**` is explained (see the cost above) instead of retried. | -| `validatePush` | `entry.mjs` | `pr-opened` without a valid proposal (wrong branch, empty title, another task's number, or `writeLevel=report`) is downgraded to `failed`, so a task can never sit in `waiting-merge` waiting for a PR nobody opened. | -| `ensureLoopPrBody` | `planActions` | Repairs the PR body's `Closes #` / `loop-task: #` markers. The router requires both, and a missing marker would make the loop's own PR look external — no acceptance row, and its merge would close the task by the wrong path. | +| `validatePush` | `entry.mjs` | `pr-opened` without a valid proposal (wrong branch, empty title, another task's number, missing PR-contract section, or `writeLevel=report`) is downgraded to `failed`, so a task can never sit in `waiting-merge` waiting for a PR nobody opened, and no loop PR reaches a reviewer without intent, proof, risk and review focus. | +| `ensureLoopPrBody` | `planActions` | Repairs the PR body's `Closes #` / `loop-task: #` markers. The router requires both, and a missing marker would make the loop's own PR look external — no acceptance row, and its merge would close the task by the wrong path. It only repairs those host-owned markers; the four author sections are validated by `validatePush` and never fabricated. | | PR number recorded | `finishRun` | The number comes back from `gh pr create` and lands in `task.prs` + a `pr-created` timeline row. | | git identity | `agentEnv` | `GIT_AUTHOR_*`/`GIT_COMMITTER_*` are injected under `auto` as GitHub Actions' own bot, `github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>` — the same identity the host's PR comes from. A fresh runner has no global git config, and the commit itself is local (no credential involved). The earlier `loop@users.noreply.github.com` belonged to the real, unrelated user `@loop`, so every loop commit was falsely attributed to them (#52, #56). | @@ -897,6 +897,16 @@ the first `auto` run is a verification step rather than a milestone. sub-agents. Demonstrated with a fake-engine harness: two invocations, distinct session dirs and models, both carrying the allowlist, and the accept/reject/ inconclusive merge paths. +- [x] D36 (#73) A loop PR was defined by identity only (`loop/-*` plus the + `Closes`/`loop-task` markers), so nothing an agent's discarded context had + held — intent, proof, risk tier, review focus — reached the reviewer, who was + the first human to read the change. The contract now requires four author + sections in the PR body (`docs/norms/ops.md` "The loop PR contract"): + `validatePush` refuses a `pr-opened` whose body is missing any of them, the + maker prompt renders the exact template, and `skills/review/SKILL.md` makes + absence a `request-changes` gate before line-level review. The four headings + live in `PR_CONTRACT_SECTIONS` (`shared/state.mjs`) and the maker prompt imports + them, so the check and the template cannot drift. ## Environment facts diff --git a/scripts/loop/entry.mjs b/scripts/loop/entry.mjs index 85f6cab..e96ffaf 100644 --- a/scripts/loop/entry.mjs +++ b/scripts/loop/entry.mjs @@ -33,6 +33,7 @@ import { appendAcceptance, markTaskTerminal, GATE_TRANSITIONS, renderStateSummary, beginRun, finishRun, lockExpired, nowIso, outcomeAllowed, allowedOutcomes, describeActions, LOOP_BRANCH_RE, + missingPrContractSections, } from './shared/state.mjs'; import { route, eventKey, stageForIssue } from './shared/route.mjs'; import { buildPrompt, runPi, parseEvents, summarize, classify } from './shared/agent.mjs'; @@ -211,7 +212,10 @@ async function handleTaskTerminal({ decision }) { * * The branch shape is enforced here as well as in `applyActions` (which re-checks it * against the credential it is about to spend) because a mismatch is an agent error - * worth reporting as `failed`, not a silent no-op. + * worth reporting as `failed`, not a silent no-op. The PR body is checked the same + * way (#73): the four author sections are the only place intent, proof, risk tier and + * review focus survive an agent's discarded context, so a body missing any of them is + * refused before the PR exists rather than handed to a reviewer to reconstruct. */ function validatePush({ writeLevel, stage, taskId, push }) { if (writeLevel !== 'auto') { @@ -234,6 +238,14 @@ function validatePush({ writeLevel, stage, taskId, push }) { + `with the body marker, so it must start with loop/${taskId}-` }; } if (!String(push.title ?? '').trim()) return { ok: false, reason: 'the PR title is empty' }; + const missingSections = missingPrContractSections(push.body); + if (missingSections.length) { + return { ok: false, reason: + `the PR body is missing the author contract section(s): ${missingSections.join(', ')}. ` + + 'Every loop PR must carry all four (docs/norms/ops.md "The loop PR contract") so ' + + 'the reviewer judges the change against stated intent, proof, risk and review ' + + 'focus instead of reconstructing them from the diff' }; + } return { ok: true, push: { ...push, branch } }; } diff --git a/scripts/loop/shared/agent.mjs b/scripts/loop/shared/agent.mjs index 90706c7..d7ed9ec 100644 --- a/scripts/loop/shared/agent.mjs +++ b/scripts/loop/shared/agent.mjs @@ -20,6 +20,26 @@ */ import { spawn } from 'node:child_process'; +import { PR_CONTRACT_SECTIONS } from './state.mjs'; + +/** + * What each PR-contract section must contain (#73), keyed by the canonical heading + * exported from `state.mjs`. Keeping the headings there and the hints here means the + * maker prompt and the host's `validatePush` check cannot drift. + */ +const PR_CONTRACT_HINTS = { + 'What/why': 'intent in one or two sentences', + 'Proof it works': 'the actual gate output / manual steps / logs — not "should work"', + 'Risk tier + AI role': 'which parts the agent produced; what breaks if the change is wrong', + 'Review focus': 'the one or two places human judgment is actually needed', +}; + +/** Render the contract as the PR-body lines an author should produce. */ +const prContractBodyLines = () => [ + 'Closes #', + 'loop-task: #', + ...PR_CONTRACT_SECTIONS.flatMap((h) => [`## ${h}`, PR_CONTRACT_HINTS[h] ?? '']), +]; /** * Read-only tool allowlist for the two isolated reviewers (#71): no `edit`/`write`, @@ -210,6 +230,10 @@ export function buildPrompt({ L.push('other lens, do not spawn sub-agents, and do not read the other reviewer\'s files or'); L.push('session. The host merges your two verdicts after both runs finish.'); L.push(''); + L.push('Both lenses share a PR-contract gate (step 0 in the skill): a loop PR body'); + L.push(`missing any of ${PR_CONTRACT_SECTIONS.map((h) => `**${h}**`).join(', ')}`); + L.push('is `request-changes` before any line-level review.'); + L.push(''); L.push('You hold a **read-only tool set**: the allowlist is `read,grep,find,ls,bash` —'); L.push('there is no `edit` or `write` tool. Write your report and result file with a `bash`'); L.push('heredoc, and keep every other command read-only. Read-only here means no'); @@ -300,8 +324,13 @@ export function buildPrompt({ L.push(' ```json'); L.push(' "push": { "branch": "loop/-", "base": "main",'); L.push(' "title": "",'); - L.push(' "body": "Closes #\\n\\nloop-task: #\\n\\n" }'); + L.push(` "body": "${prContractBodyLines().join('\\n\\n')}" }`); L.push(' ```'); + L.push(' The PR body is part of the deliverable, not paperwork (docs/norms/ops.md'); + L.push(' "The loop PR contract"). Besides the two identity markers it must carry all'); + L.push(` four author sections — ${PR_CONTRACT_SECTIONS.map((h) => `**${h}**`).join(', ')}.`); + L.push(' The host refuses a `pr-opened` whose body is missing any of them, so fill each'); + L.push(' with what you actually did, not a placeholder.'); L.push(' The body must pass the loop-PR test in `docs/norms/ops.md` (`Closes #` and'); L.push(' `loop-task: #`). Omit `push` when the stage produced no branch — triage,'); L.push(' sweep, or a `needs-info`/`needs-triage` verdict never pushes anything.'); diff --git a/scripts/loop/shared/state.mjs b/scripts/loop/shared/state.mjs index 95e4c52..3ec9fce 100644 --- a/scripts/loop/shared/state.mjs +++ b/scripts/loop/shared/state.mjs @@ -363,12 +363,60 @@ export function ghRepoOf(url) { */ export const LOOP_BRANCH_RE = /^loop\/\d+-[a-z0-9][a-z0-9._-]*$/; +/** + * The four sections every loop PR body must carry (#73). The identity markers + * (`Closes` / `loop-task`) say *which* task a PR belongs to; these say *what the + * author owes the reviewer*: an agent's reasoning is discarded with its context, so + * the reviewer must not have to reconstruct intent, proof, risk tier and review + * focus from the diff. Fixed headings on purpose — the host's check, the maker + * prompt and the reviewer's gate all find them mechanically, so the requirement is + * enforceable rather than aspirational. + */ +export const PR_CONTRACT_SECTIONS = [ + 'What/why', + 'Proof it works', + 'Risk tier + AI role', + 'Review focus', +]; + +/** Markdown section heading — ATX (`## X`) or a bold-only line (`**X**`). */ +const PR_SECTION_HEADING_RE = /^(?:#{1,6}\s+(.+?)|(?:\*\*|__)(.+?)(?:\*\*|__))\s*:?\s*$/; + +/** Normalise a heading for comparison: case, markup and `/`-`+`-`&` spacing. */ +const normalizePrSection = (s) => String(s ?? '') + .toLowerCase() + .replace(/\s*([/+&])\s*/g, ' $1 ') + .replace(/[^a-z0-9 /+&-]/g, '') + .replace(/\s+/g, ' ') + .trim(); + +/** Section names present in a PR body (markdown headings or bold lines). */ +export function prContractSections(body) { + const found = new Set(); + for (const line of String(body ?? '').split('\n')) { + const m = PR_SECTION_HEADING_RE.exec(line.trim()); + const text = (m?.[1] ?? m?.[2] ?? '').trim(); + if (text) found.add(normalizePrSection(text)); + } + return found; +} + +/** The required sections a PR body is missing, in contract order. */ +export function missingPrContractSections(body) { + const present = prContractSections(body); + return PR_CONTRACT_SECTIONS.filter((s) => !present.has(normalizePrSection(s))); +} + /** * The loop-PR test lives in the router (`loopTaskOf`): head ref `loop/-*` **and** * a body carrying `loop-task: #`, agreeing on ``. A PR missing the marker is * treated as a non-loop PR — it never enters the acceptance rate and its merge * closes the task by the wrong path. That is a structural contract, so the host * enforces it rather than trusting the model to remember it. + * + * This function only ever *repairs* the identity markers: they are bookkeeping the + * host owns. The four author sections are content the host cannot fabricate, so a + * missing one is a refusal (`entry.mjs` `validatePush`), never a placeholder. */ export function ensureLoopPrBody(body, id) { const b = String(body ?? '').trim();