Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion docs/norms/ops.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<issueNo>-*` and a body carrying
`Closes #<n>` and `loop-task: #<n>`; the router pairs the ref with the marker and
requires both to agree on `<n>`.

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/<issueNo>-*`, body `Closes #<n>` and
`loop-task: #<n>`.
`loop-task: #<n>`, 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
Expand Down
14 changes: 12 additions & 2 deletions scripts/loop/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ reason a future reader may see the parameter and wonder where it came from.
| branch check | `applyActions` | `loop/<issueNo>-<slug>` 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 #<n>` / `loop-task: #<n>` 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 #<n>` / `loop-task: #<n>` 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). |

Expand Down Expand Up @@ -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/<n>-*` 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

Expand Down
14 changes: 13 additions & 1 deletion scripts/loop/entry.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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') {
Expand All @@ -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 } };
}

Expand Down
31 changes: 30 additions & 1 deletion scripts/loop/shared/agent.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 #<n>',
'loop-task: #<n>',
...PR_CONTRACT_SECTIONS.flatMap((h) => [`## ${h}`, PR_CONTRACT_HINTS[h] ?? '']),
];

/**
* Read-only tool allowlist for the two isolated reviewers (#71): no `edit`/`write`,
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -300,8 +324,13 @@ export function buildPrompt({
L.push(' ```json');
L.push(' "push": { "branch": "loop/<taskId>-<slug>", "base": "main",');
L.push(' "title": "<PR title>",');
L.push(' "body": "Closes #<n>\\n\\nloop-task: #<n>\\n\\n<what and why>" }');
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 #<n>` and');
L.push(' `loop-task: #<n>`). Omit `push` when the stage produced no branch — triage,');
L.push(' sweep, or a `needs-info`/`needs-triage` verdict never pushes anything.');
Expand Down
48 changes: 48 additions & 0 deletions scripts/loop/shared/state.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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/<n>-*` **and**
* a body carrying `loop-task: #<n>`, agreeing on `<n>`. 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();
Expand Down
14 changes: 13 additions & 1 deletion skills/review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down Expand Up @@ -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.
7 changes: 7 additions & 0 deletions skills/verify/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Loading