Skip to content

chore(norms): require the four-section loop PR contract - #74

Merged
Alex1990 merged 2 commits into
mainfrom
loop/73-pr-contract
Sep 22, 2026
Merged

Alex1990 merged 2 commits into
mainfrom
loop/73-pr-contract

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Closes #73

loop-task: #73

What/why

Loop PRs were defined by identity only, so an agent's discarded context — intent, proof, risk tier, review focus — never reached the reviewer, who was the first human to read the change. This makes those four sections a requirement, enforced by the host when it creates the PR and by a blocking gate in review.

Proof it works

Gates (all green):

  • pnpm lint — 0 warnings / 0 errors
  • pnpm fmt:check — all files formatted
  • pnpm build — exit 0
  • pnpm check:types — exit 0 (after pnpm build; dist/ is required)
  • pnpm test:azure-oracle / pnpm test:tos-oracle — all checks passed
  • pnpm exec vitest run --browser.headless (with pnpm serve) — 24 files passed, 255 passed / 26 skipped

Harness, 9/9 (missingPrContractSections + planPush + prompts): a full body reports no missing section; removing one reports it by name; bold headings, trailing colons and What / why spacing are accepted; an empty body reports all four in contract order; ensureLoopPrBody repairs only the identity markers; a planPush body keeps all four sections and still passes loopTaskOf; both the maker and reviewer prompts render the headings.

Integration (fake pi engine, temp STATE_DIR, synthetic task, LOOP_WRITE_LEVEL=auto): entry.mjs refused an pr-opened whose body omitted Proof it works — outcome=failed, note pr-opened refused by the host: the PR body is missing the author contract section(s): Proof it works…, task → waiting-human, no push attempted.

Risk tier + AI role

AI-authored end to end (this run's maker is an agent); the owner's review is the checker. The change is norms (docs/norms/ops.md, skills/review/SKILL.md, skills/verify/SKILL.md) plus loop host code (scripts/loop/shared/state.mjs, entry.mjs, shared/agent.mjs, README.md). No src/, signer, dependency or workflow file is touched, so the signer oracles and tree-shaking are unaffected. The main failure mode is a heading parser that rejects a valid body, which would send the task to the human inbox rather than open a non-compliant PR — the safe direction. The parser accepts ## and bold headings plus case, colon and spacing variants to bound that risk.

Review focus

  1. The contract's shape and the refusal-at-creation design: are the four sections right, and is refusing (rather than fabricating placeholder sections) the right enforcement?
  2. missingPrContractSections in scripts/loop/shared/state.mjs — the regex and normalisation — and that PR_CONTRACT_SECTIONS is the single source the maker prompt imports, so the check and the template cannot drift.

Norms-layer proposal: the loop does not merge its own work; the owner approves.

@github-actions
github-actions Bot requested a review from Alex1990 as a code owner September 21, 2026 18:56
@github-actions

Copy link
Copy Markdown
Contributor Author

Coverage

Metric % Covered / Total
Lines 91.73% 1121/1222
Statements 90.42% 1209/1337
Functions 92.76% 218/235
Branches 76.97% 575/747

@github-actions

Copy link
Copy Markdown
Contributor Author

Loop pr-review — request-changes ⚠️ — two isolated reviewers

Reviewer a — correctness & regression (model deepseek/deepseek-v4-flash, session /home/runner/work/_temp/pi-sessions/reviewer-a): request-changes

Reviewer A — Correctness & regression: request-changes

PR-contract gate (step 0): pass — the body carries all four author sections plus both identity markers. Findings below are line-level.

[L2] scripts/loop/shared/agent.mjs:32 (used at :327) — the PR-body example the maker prompt tells agents to copy is invalid JSON — drop or escape the inner quotes.
PR_CONTRACT_HINTS['Proof it works'] holds raw " (not "should work") and is interpolated straight into a JSON string literal. Rendering buildPrompt() and parsing the fragment fails: Expected ',' or '}' after property value .... An agent that copies the example writes a malformed result.json, and readJson (state.mjs:119-126) re-throws non-ENOENT parse errors, so the run dies instead of reporting a clean outcome. Newly introduced (the old example had no quotes).

[L2] scripts/loop/shared/state.mjs:383 — bold headings are only recognised when they occupy the whole line, so a valid common style is refused with all four sections reported missing — accept a trailing : <content>, or document the restriction.
Verified: **What/why**: intent … **Proof it works**: gates … → all four reported missing; the own-line form passes. An author who wrote four readable sections is refused by validatePush before the reviewer's step-0 gate can see it, and the task lands in the human inbox as failed.

[L3] scripts/loop/shared/state.mjs:394-401 — the scan is body-wide, so headings quoted inside a code fence satisfy the gate with no real sections — skip fenced regions, or require content under a heading.
Verified: a body whose only content is a fence containing the four headings returns [] (nothing missing). The maker prompt now embeds exactly those heading lines.

[L3] scripts/loop/shared/state.mjs:375 + agent.mjs:30-41 — the "cannot drift" claim holds for the headings only; the hints are a parallel map that can silently go missing — single-source heading+hint, or assert every section has one.
PR_CONTRACT_HINTS[h] ?? '' renders an empty template line for a new section while the check keeps accepting it.

[L3] "Proof it works" does not cover the changed files — every cited gate is scoped to src/test (lint = oxlint src test, fmt:check = oxfmt --check src test, check:types = test/*.ts, build = src, pnpm test = test/**), and the 9/9 harness was never committed (/tmp/... is absent in a fresh runner) — commit a small harness and say which command runs it.
Finding 1 is the class of defect a committed harness catches. (oxfmt --check scripts also fails for all 10 pre-existing scripts/loop files, so that gap is repo-wide, not introduced here.)

[L3] scripts/loop/shared/agent.mjs:233 (+ docs/norms/ops.md, skills/review/SKILL.md) — the host check is presence-only while the reviewer gate says "missing or empty", and the reviewer prompt drops the "empty" half — align the wording; optionally require content under a heading.
Verified: four headings with no content between them are accepted by missingPrContractSections.

Not findings (checked): normalisation is self-consistent across ##/bold/case/colon/What / why; the non-body validatePush refusal paths are unchanged; agent.mjs→state.mjs is not an import cycle; no host-side fabrication of author content.

Verdict: request-changes — fix the two L2 items (L3 optional), re-run verify, then re-push for the owner's approval.

Reviewer b — standards, safety & maintainability (model deepseek/deepseek-v4-pro, session /home/runner/work/_temp/pi-sessions/reviewer-b): approve

Reviewer B — standards, safety & maintainability

PR-contract gate: passed (all four sections present and substantive). Scope conforms to AGENTS.md hard constraints — no src/, signer, dependency, or workflow file touched. PR_CONTRACT_SECTIONS is the single source both validatePush and the maker prompt derive from, and refusal-at-creation is the safe direction (false negative → failed → human inbox, never a non-compliant PR).

Findings (all L3, non-blocking):

  1. [L3] package.json (gate scope) — proof mismatch — pnpm lint (oxlint src test) and pnpm fmt:check (oxfmt --check src test) do not cover scripts/loop/**, where the executable changes live; the PR lists them first as if they validate the diff. The 9/9 harness + integration are the real coverage. Consider extending the gates to scripts/loop, or noting the scope in the proof section.
  2. [L3] scripts/loop/shared/agent.mjs:30 — duplicated heading literals — PR_CONTRACT_HINTS re-declares the four canonical headings as object keys; a future rename would silently render an empty hint (?? '') instead of failing. Consider deriving hint+heading as pairs from state.mjs or asserting key parity.
  3. [L3] scripts/loop/shared/state.mjs:394 + skills/review/SKILL.md step 0 — presence vs emptiness — the host enforces heading presence; bare headings with no content pass validatePush and only the reviewer's "missing or empty" gate catches them. The layering is coherent, but consider aligning ops.md's "missing any section" wording with the review skill's "missing or empty".

@Alex1990
Alex1990 merged commit cd2eda6 into main Sep 22, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

loop PRs carry no evidence, so the reviewer is the first human to read the change

1 participant