Skip to content

fix(skills): handle in-attempt scope amendments without user approval - #87

Merged
patextreme merged 4 commits into
mainfrom
fix/pr-review-repair-scope-amendment
Oct 22, 2026
Merged

patextreme merged 4 commits into
mainfrom
fix/pr-review-repair-scope-amendment

Conversation

@patextreme

Copy link
Copy Markdown
Owner

Summary

  • Allow a repair-worthy finding discovered during the active attempt's own required validation to join that attempt through a verified scope-amendment record, without consuming another attempt or pausing for user approval.
  • Escalate only for genuine product, design, security, high-risk, or ambiguous-accounting decisions.
  • Compress pr-review-repair with progressive disclosure:
    • move detailed repair thresholds and in-attempt amendment conditions to references/policy.md;
    • move the setup/reconciliation checklist to references/prepare.md;
    • keep the stage workflow inline.
  • Extend the factory-skills gate tests to cover the new references and contract.

Why

A test-only finding discovered during validation should not freeze the repair loop for a decision the orchestrator already owns. The skill now records an append-only amendment, keeps the active reservation and attempt count, and continues.

Verification

  • npm run lint
  • npm run typecheck
  • npm test — 411 passed, 1 skipped

Keep an in-scope finding discovered during the active attempt's required validation inside the same reservation through a verified scope-amendment record. Escalate only genuine product, design, security, high-risk, or accounting decisions.

Compress the main skill with progressive disclosure: move repair thresholds and amendment conditions to references/policy.md, and the setup checklist to references/prepare.md. Keep the stage workflow inline and extend the gate tests to cover the new references.

Signed-off-by: Pat Losoponkul <[email protected]>
@patextreme

Copy link
Copy Markdown
Owner Author

PR review/repair — repair start

  • PR / run / round: fix(skills): handle in-attempt scope amendments without user approval #87 / 665e9184c2273162cae5442bd602edc14cb79ade / 1
  • Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 / 3da0efee1e28bad8ec1fdef87781371fcc71d335
  • Expected local and remote head: 665e9184c2273162cae5442bd602edc14cb79ade
  • Repairs used: 1/10
  • Authorized actionable IDs: F-001 (blocker), F-002, F-003, F-004, F-005, F-006 (significant bugs)
  • Intended scope: Repair the six authorized findings from round 1 judgment: restore dropped normative threshold clauses in references/policy.md; restore the security/custody boundary and correct the amendment-rule reference in SKILL.md and references/report-contracts.md; align the control-flow source of truth so scope amendments are reachable on validation discovery and blocked recovery still escalates; remove the unannounced retry broadening in SKILL.md; replace the vacuous gate assertion in tests/gates/factory-skills.test.mjs. Text/test alignment only; no new product intent, public contract, security policy, or design change.
  • State: repair reserved; no delivery confirmed

- Add security/custody policy to human-decision and escalation boundaries.
- Restore threshold clauses dropped when extracting references/policy.md.
- Make scope amendments reachable on validation discovery and restore the
  blocked-recovery escalation catch-all.
- Remove the unannounced retry of the reserved attempt.
- Replace the vacuous gate assertion with real boundary assertions.

Tests: node tests/run.mjs (411 passed, 1 skipped).
@patextreme

Copy link
Copy Markdown
Owner Author

Repairs delivered — final review pending

Resolved

  • F-001 (blocker) — Added security/custody policy to the human-decision list and the record-contract escalation trigger, and pointed the amendment-rule reference at references/policy.md · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · gate now asserts the boundary.
  • F-002 — Restored the four threshold clauses dropped during the policy extraction: documented architecture-boundary rule, missing mandatory acceptance criteria, anti-downgrade guard, and qualifying-refactor allowance · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · full gate suite green.
  • F-003 — Made scope amendments reachable whenever validation discovers an eligible in-scope finding, not only when validation is already blocked · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · pseudocode control flow re-read.
  • F-004 — Removed the unannounced “retry of the reserved attempt” broadening from orchestrator-owned recovery · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · aligns with the unchanged delivery-escalation rule.
  • F-005 — Restored the catch-all escalation for blocked or unavailable recovery in the resume path · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · pseudocode re-read.
  • F-006 — Replaced the vacuous gate assertion with assertions on the real amendment boundary and the security pause-list item · e53caa8098ed028f79d08022b50c61dc4a73a0a5 · mutation-checked and independently re-inspected.

Findings: 0 open blockers · 0 unresolved investigations · 8 report-only (recorded in the audit).

Validation: node tests/run.mjs at the repaired source: 412 tests, 411 passed, 1 skipped, 0 failed; git diff --check clean. CI passed at the reviewed head 665e918; CI at the delivered head is pending and is not counted as passing yet.

Scope: The requirement source is self-referential — there is no linked issue, and the PR edits its own capability spec. No runtime adherence test exists, so impacts are contract-reading findings. Full nix flake check was skipped because the change touches only Markdown and one .mjs gate test.

Reviewed: 665e918 · Delivered: e53caa8098ed028f79d08022b50c61dc4a73a0a5 · Attempts: 1/10 · Human input: none

Full review reports, ledger, and audit details

Standards report

Standards axis — PR review/repair scope-amendment change (base 3da0efe → head 665e918)

Verified: node --check clean; node --test tests/gates/factory-skills.test.mjs → 21 pass, 1 skipped (the PI_FACTORY_SKILLS_OUTPUT-gated notice test). No files modified.

HARD (documented standard / internal contradiction)

  1. Silent policy narrowing when extracting references/policy.md. The moved text drops normative clauses from the old SKILL.md repair threshold: "…and missing mandatory acceptance criteria also qualify", "a documented architecture-boundary violation" (now just "an architecture-boundary violation"), "High-risk corrections require human input rather than downgrading the finding", and "allow only refactoring needed for a qualifying correction". policy.md:7-11 now says only "PR-caused build/test/delivery failures also qualify" and "Cheapness alone does not justify repair." A refactor is a move, not a retune; the PR body claims only to "move detailed repair thresholds". The gate test asserts the headings exist, not that the clauses survived (tests/gates/factory-skills.test.mjs:481-487).

  2. Main spec edited directly with no change delta. openspec/specs/pr-review-repair-orchestration/spec.md is hand-edited; the repo routes spec modification through openspec/changes/<id>/specs/... plus archive (openspec/changes/ holds only deltas). AGENTS.md states the convention explicitly for the analogous case: "retire it with a ## REMOVED Requirements delta … rather than deleting the spec by hand."

  3. Dangling cross-reference. references/report-contracts.md:65: "A scope amendment adds only findings that satisfy SKILL.md's in-attempt amendment rules." SKILL.md:21 contains no amendment rules — only "In-attempt scope amendments keep the active reservation. The attempt count stays unchanged." The rules (authorized-intent test, non-high-risk, unambiguous accounting) now live in references/policy.md:16.

  4. Stale control-flow header. references/pseudocode.md:3: "SKILL.md defines preparation, shared guards, thresholds, and stage contracts" — this diff moved preparation to prepare.md and thresholds to policy.md, and SKILL.md:15 now directs readers to those files "before setup" / "before judgment". The declared source-of-truth file still points at the old locations.

JUDGEMENT CALLS (baseline smells)

  • Mysterious Name / contradiction: SKILL.md:17 "Do not pause for a mechanical in-scope scope amendment", pseudocode.md:79 "DO NOT escalate a mechanical scope amendment", spec.md:57 "Mechanical in-scope scope amendments". "Mechanical" is defined nowhere, and policy.md:16 makes each amendment a judgement ("stays within authorized intent … is not high-risk … accounting is unambiguous") that report-contracts.md:52-60 requires an In-scope rationale: for. "in-scope scope amendment" is also a tautology — now pinned verbatim by the gate test (factory-skills.test.mjs:462).
  • Duplicated Code / Shotgun Surgery: the amendment boundary is restated in six files (SKILL.md:17,21,39,47, policy.md:16, pseudocode.md:52-73, report-contracts.md:52-65, spec.md:31,38,42, README.md:120) and already drifts: "consequential architecture/public-contract choices" (SKILL.md:17) vs "consequential design change" (spec.md:31) vs "consequential design choice" (policy.md:16); the spec requirement says "security/custody policy" while both new scenarios say "security-policy change", and the scenarios omit the requirement's "product behavior".
  • Duplicated sentences inside one hunk: SKILL.md:21 "keep the active reservation. The attempt count stays unchanged."
  • Vacuous gate assertion: factory-skills.test.mjs:466-468 asserts prReviewSkill.includes("new requirements") && prReviewSkill.includes("high-risk") for "amendment boundary must exclude new intent and high-risk corrections" — both substrings come from pre-existing unrelated sentences in SKILL.md:39 and :21. The assertion cannot fail when the boundary is deleted.
  • Divergent Change in the new test: "in-attempt scope amendments keep their reservation..." also asserts preparationContract content (sticky contract, paginated comments, unpublished commits) about an unrelated file.
  • Glossary gap: docs/agents/domain.md ("Use glossary terms when naming domain concepts … note the gap") — "scope amendment" is now a first-class concept across skill, spec, README, and tests but has no GLOSSARY.md entry, and "mechanical" has no definition anywhere.
  • SKILL vs pseudocode divergence: SKILL.md:39 applies an amendment whenever "validation finds an additional in-scope finding"; pseudocode.md:52-56 only offers SCOPE-AMENDMENT inside IF repair or validation is blocked/incomplete.

No misdirected links found: ./references/prepare.md and ./references/policy.md resolve, both new files are registered in factorySkills (factory-skills.test.mjs:37-38), and both are packaged via the ./skills directory in package.json.

Spec report

Spec review — base 3da0efee → head 665e918 (8 files, +173/−50, one commit)

Verdict: core requirement implemented and coherent, but with one wrong cross-reference, one missing requirement clause, and two real spec/skill contradictions. Gate tests pass (21 pass, 1 skipped) but under-cover the contract.

** (a) Missing / partial requirements**

  1. Amendment conditions lost in compression. Base SKILL.md significant-bug rule: "PR-caused build/test/delivery failures and missing mandatory acceptance criteria also qualify"; blocker qualifier "a documented architecture-boundary violation can qualify"; "High-risk corrections require human input rather than downgrading the finding"; report-only "allow only refactoring needed for a qualifying correction." None of these strings survives in references/policy.md (verified by grep across skills/). The PR body claims only to "move detailed repair thresholds … to references/policy.md" — dropping clauses is not a move.
  2. Security omitted from the human-decision list. Spec scenario: "a security-policy change or a high-risk correction" → escalate. SKILL.md:17 enumerates "new product intent, consequential architecture/public-contract choices, high-risk corrections, ambiguous identity/history/accounting, or missing prerequisites" — no security/custody item. policy.md has it; the inline list does not.
  3. Accounting ambiguity not in the normative sentence. Spec: "…and is not high-risk; otherwise human input SHALL be required" — accounting ambiguity appears only in the scenario and in the trailing "ambiguous history" clause. policy.md correctly requires it; spec prose does not.
  4. "Mechanical" is never defined. Spec: "Mechanical in-scope scope amendments and other orchestrator-owned accounting actions SHALL be recorded and continued." No skill/reference defines mechanical vs. non-mechanical; policy.md treats every in-scope amendment as non-escalating, so the qualifier is unenforceable.

** (b) Scope creep**

SKILL.md:17/:33 add "another orchestrator-owned action", "another authorized accounting action", and the spec's "other orchestrator-owned accounting actions" — an unbounded open-ended category well past the PR body's closed list ("genuine product, design, security, high-risk, or ambiguous-accounting decisions").

** (c) Implemented but wrong / contradictory**

  1. Stale pointer (checklist item). report-contracts.md:65: "A scope amendment adds only findings that satisfy SKILL.md's in-attempt amendment rules." The amendment rules now live in references/policy.md; SKILL.md:21 retains only "In-attempt scope amendments keep the active reservation. The attempt count stays unchanged." Wrong target.
  2. Retry contradiction. SKILL.md:33: "Orchestrator-owned recovery—… a retry of the reserved attempt … record and continue. Escalate only the actual human decision", while unchanged pseudocode IF delivery is incomplete: ESCALATE and the spec scenario "a later continuation reconciles its remaining operations" disagree.
  3. Resume escalation narrowing. Pseudocode replaced IF recovery is blocked: ESCALATE with "IF recovery needs product/design/security/high-risk input or accounting is ambiguous: ESCALATE", silently dropping agent failure/access blockers that spec still requires: "Blocked operations, stalled progress or exhaustion SHALL preserve safe work and escalate."

** (d) Reference agreement**

Count preservation is consistent across spec ("without incrementing the count"), SKILL.md:21, policy.md:16 ("The attempt count stays unchanged"), pseudocode (KEEPS repairs used unchanged), and report-contracts ("never increments repairs used"). Append-only posted-and-verified record, fresh-review-next-cycle ("A finding from fresh review or judgment belongs to the next repair cycle"), and the marker kind=<repair-start|scope-amendment|summary|escalation> all agree. Only drift: spec "security-policy change" vs policy "security/custody policy".

Gate coverage

tests/gates/factory-skills.test.mjs asserts report-contract fields, marker, pseudocode strings, policy headings, and prepare clauses, but the "amendment boundary must exclude new intent and high-risk corrections" assertion is vacuous — prReviewSkill.includes("new requirements")/"high-risk" are satisfied by SKILL.md:39 archived-change text and :17, not by amendment rules. Nothing guards findings 5–7 or the dropped threshold phrases.

Coverage limitation

No issue and no OpenSpec change directory exist; requirements are self-referential (PR body + post-change capability spec authored alongside the code), so this review cannot distinguish an unmet intent from an intent never recorded.

Judge ledger

ID Provenance Category Evidence Impact Correction Disposition Resolution evidence
F-001 Both (Standards HARD; Spec (a)2) Blocker — security/custody boundary omission SKILL.md:17 exhaustive list omitted security; report-contracts.md:65 escalation triggers omitted security; present at policy.md:16, pseudocode.md:15,68, spec.md:31,38,42; PR body names "security" Validation discovers a correction changing security/custody policy → the exhaustive list plus "another orchestrator-owned action; record and continue" permits folding it into the attempt → violates spec.md:31 "otherwise human input SHALL be required" and scenario :42 Add security/custody to SKILL.md:17 and report-contracts.md:65; point amendment-rules reference at references/policy.md repaired-and-verified SKILL.md:17 and report-contracts.md:65 now include "security/custody policy"; gate asserts the pause-list item; validation PASS at e53caa8098ed028f79d08022b50c61dc4a73a0a5
F-002 Both (Standards HARD 1; Spec (a)1) Significant bug — normative clause loss Base 3da0efe:SKILL.md threshold bullets vs head policy.md:7-10; all four phrases absent repo-wide; PR body says only "move" Judge loses "missing mandatory acceptance criteria", the anti-downgrade guard, "documented" boundary, and the qualifying-refactor allowance → future misclassification/dismissal or mis-scoped repair Restore the clauses in policy.md repaired-and-verified policy.md:7-10 restored; full gate suite green at e53caa8098ed028f79d08022b50c61dc4a73a0a5
F-003 Both (Standards judgement; Spec (c)) Significant bug — control-flow gap pseudocode.md:52-56 gated amendment on blocked/incomplete; contrast SKILL.md:39, policy.md:16, spec.md:31,38-39 Validation surfaces an extra in-scope finding on an otherwise clean attempt → no pseudocode path → escalate, deliver against "no repair-worthy findings", or burn a new attempt — the behavior the PR exists to remove Make SCOPE-AMENDMENT reachable on validation discovery; keep blocked/incomplete escalation after it repaired-and-verified pseudocode.md:54-58 restructured; validation PASS
F-004 Spec (c)6 Significant bug — internal contradiction SKILL.md:33 ("a retry of the reserved attempt … record and continue") vs pseudocode.md:61 ("IF delivery is incomplete: ESCALATE") and spec.md:57; base escalated on failed required operation/agent failure Transient push/check failure → silent retry without escalation record (history gap, duplicate-commit risk) or spurious escalation; not in PR intent Remove the retry broadening from the recovery list repaired-and-verified SKILL.md:33 no longer lists retry; resume paragraph still governs finishing an authorized incomplete attempt; validation PASS
F-005 Spec (c)7 Significant bug — escalation narrowing Base pseudocode.md "IF recovery is blocked: ESCALATE" → head enumerated only product/design/security/high-risk/accounting; spec.md:57 and SKILL.md:33 still require escalation for blocked operations/agent failure Resume with lost access or persistently failing agent → no escalation instruction; attempt left unreconciled or bare pause with no escalation record Restore a catch-all after the enumerated recovery cases repaired-and-verified pseudocode.md:15-18 adds "blocked or unavailable for any other reason: ESCALATE"; validation PASS
F-006 Both (Standards HARD-adjacent; Spec gate coverage) Significant bug — test defect factory-skills.test.mjs:465-469 asserted substrings already present at SKILL.md:39 and :21; the policy.md boundary was never asserted Future edit deletes/narrows the boundary → gates stay green (already demonstrated by F-001) Replace the vacuous assertion with policy boundary terms and the security pause-list item repaired-and-verified factory-skills.test.mjs:465-482 asserts 7 boundary terms plus security/custody policy; mutation check and independent re-inspection PASS
F-007 Both Report-only — undefined term spec.md:57, SKILL.md:17, pseudocode.md:79; no definition in-skill Ambiguous "mechanical"; conservative direction is over-escalation; unsafe read is blocked by the policy eligibility conditions Define or replace later report-only Not repaired
F-008 Both (Standards HARD 3; Spec (c)5) Report-only — stale cross-reference report-contracts.md:65 pointed at SKILL.md amendment rules that live in policy.md Record writer follows pointer, finds no eligibility rules → weaker gate on posting amendments Point at references/policy.md report-only; corrected incidentally in F-001's same sentence report-contracts.md:65 link now resolves to ./policy.md
F-009 Standards (HARD 4) Report-only — stale doc header pseudocode.md:3 says SKILL.md defines preparation/thresholds; they moved to prepare.md/policy.md Descriptive only, no normative effect Update later report-only Not repaired
F-010 Standards (judgement call) Report-only — wording drift spec.md:38,42 "security-policy change" and omit "product behavior" vs spec.md:31; SKILL.md:17 "architecture/public-contract" vs policy.md:16 "design choice" Scenario-level acceptance can be read narrower than the requirement; requirement text still governs Align later report-only Not repaired
F-011 Standards (HARD 2) Report-only — process inference Main spec hand-edited with no change delta; openspec/config.yaml has no rules; no gate/CLI provenance check; prior direct spec edits 45c7f19, c0d964e, 7c3c80f; AGENTS.md delta rule concerns capability retirement No mandatory convention found → not a breach; reviewability hygiene only Optionally route through openspec/changes later report-only (refutes convention violation) openspec/config.yaml, spec history inspected
F-012 Standards (judgement call) Report-only — redundancy SKILL.md:21 None material Merge later report-only Not repaired
F-013 Standards (judgement call) Report-only — divergent test factory-skills.test.mjs:489-495 also asserts prepare.md content Mixed concerns weaken diagnosis only Split later report-only Not repaired
F-014 Standards (judgement call) Report-only — glossary gap GLOSSARY.md lacks "scope amendment"/"mechanical"; docs/agents/domain.md asks for domain terms Concept drift risk only Add entries when domain modeling next runs report-only Not repaired
R-001 Spec (a)3 Refuted spec.md:31 "ambiguous history SHALL require human input"; scenario :42; policy.md:16; pseudocode.md:65 Coverage already present — refuted Text read
R-002 Spec (b) Refuted spec.md:57 "orchestrator-owned accounting actions"; closed lists at spec.md:31,42, SKILL.md:17 Bounded category; concrete hazard is F-001 — refuted Text read

Prior-ID reconciliation: none — first review/judgment for this run; no prior comments, reviews, or repair starts on PR 87.

Audit record

Run identity: PR #87 · run 665e9184c2273162cae5442bd602edc14cb79ade · round 1 · repairs used 1/10.
Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 (both).
Reviewed head: 665e9184c2273162cae5442bd602edc14cb79ade (local and remote aligned).
Delivered head: e53caa8098ed028f79d08022b50c61dc4a73a0a5 on patextreme/agent-mod:fix/pr-review-repair-scope-amendment.
Repair-start record: issue comment 6331991208.
Scope amendments: none; the authorized queue was unchanged.

Repair evidence. Five files changed: skills/pr-review-repair/SKILL.md, skills/pr-review-repair/references/{policy,pseudocode,report-contracts}.md, tests/gates/factory-skills.test.mjs; 30 insertions, 15 deletions. No spec, README, prepare, TypeScript, Nix, or package files changed. Report-only findings were left untouched.

Validation. node --test tests/gates/factory-skills.test.mjs → 22 tests, 21 passed, 1 skipped, 0 failed. node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed. git diff --check clean. The gate hardening was mutation-checked: deleting security/custody policy from policy.md or the pause-list item from SKILL.md fails the new assertions. An independent validation agent re-read every hunk and confirmed the per-finding corrections with no unauthorized changes. nix flake check was skipped: the diff touches no TypeScript, Nix, or package*.json files, so its package/type/lint gates are unaffected; biome and typecheck were likewise not run for Markdown/test-only changes. CI passed at the reviewed head 665e918; CI at the delivered head is pending and not counted as passing. No runtime adherence test exists for the prose contract.

Delivery receipts. Signed commit e53caa8098ed028f79d08022b50c61dc4a73a0a5 contains only the five authorized files; pushed without force to the exact PR head branch; remote head confirmed. This comment is the round summary record. Worktree left clean.

Coverage limitation. Requirements are self-referential: no linked issue and no openspec/changes/** for this work; the PR both writes and satisfies its own capability spec. Findings were therefore anchored to unchanged spec text, surviving policy clauses, the PR body, and base-vs-head regressions. Runtime orchestrator adherence was not exercised.

@patextreme

Copy link
Copy Markdown
Owner Author

PR review/repair — repair start

  • PR / run / round: fix(skills): handle in-attempt scope amendments without user approval #87 / 665e9184c2273162cae5442bd602edc14cb79ade / 2
  • Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 / 3da0efee1e28bad8ec1fdef87781371fcc71d335
  • Expected local and remote head: e53caa8098ed028f79d08022b50c61dc4a73a0a5
  • Repairs used: 2/10
  • Authorized actionable IDs: F-015 (significant bug)
  • Intended scope: Align the human-input and amendment boundary across skills/pr-review-repair/SKILL.md, references/policy.md, references/pseudocode.md, references/report-contracts.md, openspec/specs/pr-review-repair-orchestration/spec.md, and the factory-skills gate test so consequential architecture changes remain human-input-gated as unchanged spec.md:24 requires. Restore "new requirements", "product behavior", and "security/custody policy" in the amendment scenarios, and add architecture to the gate boundary assertions. No new product intent, public contract, security policy, or design change.
  • State: repair reserved; no delivery confirmed

Align the scope-amendment and escalation boundaries with the unchanged
material-repair threshold so consequential architecture changes still
require human input. Restore new-requirements/product-behavior and
security/custody wording in the spec scenarios and harden the gate.

Tests: node tests/run.mjs (411 passed, 1 skipped).
@patextreme

Copy link
Copy Markdown
Owner Author

Repairs delivered — final review pending

Resolved

  • F-015 (significant bug) — Restored architecture to the human-input and amendment boundaries so the extracted policy, pseudocode, report contracts, spec scenarios, and pause list all match the unchanged material-repair requirement; added gate assertions for the architecture boundary · 77dd9a8c2f3a1c478a700ccafd716ad0d9712231 · gate mutation-checked and independently validated.

Findings: 0 open blockers · 0 unresolved investigations · 10 report-only (recorded in the audit; F-008 and the actionable part of F-010 were closed earlier).

Validation: node tests/run.mjs at the repaired source: 412 tests, 411 passed, 1 skipped, 0 failed; factory-skills gate 22 tests, 21 passed, 1 skipped; openspec validate valid with two pre-existing long-requirement warnings; git diff --check clean. CI passed at the reviewed head e53caa8; CI at the delivered head is pending and is not counted as passing yet.

Scope: Requirements remain self-referential — no linked issue and no OpenSpec change; the PR edits its own capability spec. No runtime adherence test exists. nix flake check and biome were not run because the change touches only Markdown, the spec, and one .mjs gate test.

Reviewed: e53caa8 · Delivered: 77dd9a8c2f3a1c478a700ccafd716ad0d9712231 · Attempts: 2/10 · Human input: none

Round-2 reports, full ledger, and audit details

Round-1 complete reports, ledger, and delivery receipts: summary comment 6332039771. Round-1 findings F-001–F-006 remain repaired-and-verified; F-008 was corrected incidentally under F-001; the architecture part of F-010 is now F-015 and has been repaired.

Round-2 Standards report

Standards-axis review (read-only; HEAD verified = e53caa8).

  1. openspec/specs/pr-review-repair-orchestration/spec.md:31,57 — Hard violation: main specs edited directly, with no openspec/changes/* delta carrying this capability. GLOSSARY.md defines Spec synchronization as "Applying a change's delta requirements to the main specs" and Change finalization as sync-then-archive; AGENTS.md routes capability changes through deltas. Fix: move both edits into a change delta and finalize/archive.

  2. skills/pr-review-repair/references/pseudocode.md:3 — Hard violation (dangling cross-reference): "SKILL.md defines preparation, shared guards, thresholds, and stage contracts", but SKILL.md:15,25 now delegates preparation to prepare.md and thresholds to policy.md. The control-flow source of truth points at content that no longer lives there. Fix: name prepare.md/policy.md in that sentence.

  3. skills/pr-review-repair/SKILL.md:21 — Judgement — Duplicated Code: "In-attempt scope amendments keep the active reservation. The attempt count stays unchanged." restates one rule twice, and re-summarises policy.md:5-11 thresholds (now two editable sources). prepare.md is likewise linked twice (SKILL:15, SKILL:25). Fix: one pointer sentence to policy.md; link prepare.md once.

  4. Terminology drift — Judgement — Mysterious Name/consistency (docs/agents/domain.md: "Use glossary terms … Respect explicitly avoided synonyms"): SKILL.md:17 "consequential architecture/public-contract choices" vs policy.md:16 "consequential design choice" vs spec.md:44 "consequential design or public-contract change" vs GLOSSARY "consequential human decision". New domain concept scope amendment is absent from GLOSSARY.md. Fix: pick one phrase and add a glossary entry.

  5. SKILL.md:17, spec.md:57, tests/gates/factory-skills.test.mjs:492 — Judgement — Mysterious Name: "mechanical in-scope scope amendment" doubles "scope", and the gate asserts that exact awkward phrase, freezing it. Fix: "in-scope scope amendment" → "scope amendment".

  6. tests/gates/factory-skills.test.mjs:461-463,480,498-503 — Judgement — loose substring gates: includes("The attempt count stays unchanged") passes even if every amendment condition is deleted from SKILL.md; includes("product behavior") passes while policy.md:16 reads "(no new requirements, product behavior, …)"; nothing asserts SKILL's human-decision list matches the policy boundary. Fix: assert the whole boundary sentence and require SKILL→policy amendment link.

  7. spec.md:31 — Judgement: the reservation requirement grew 381→783 chars and now trips openspec validate's >500-char warning (new). Fix: push conditions into the new scenario.

Tally: 2 hard violations, 5 judgement calls.

Round-2 Spec report

Coverage limitation (stated, not a defect). Requirement source is circular: the PR body and the spec delta in the same diff both describe the behaviour, and there is no issue or openspec/changes/<change> delta. The spec is edited directly as a main spec, which matches house practice (PRs #75, #83 did the same), so it is not a defect — but it leaves only the spec, PR body, and PR comments as intent evidence, so spec-vs-implementation agreement cannot independently validate intent. No finding below is "no issue existed".

  1. Critical — spec contradicts itself, forbidding what the new rule permits. "Each repair SHALL require a posted and verified start record reserving one attempt before edits." (spec:31) now sits in the same paragraph as "An in-scope finding … MAY be added to that attempt … without incrementing the count." An amendment is a repair with no reserving start record. Fix: scope the first sentence to "Each repair cycle/attempt SHALL require…".

  2. Major — escalation boundaries are not identical across the five artifacts. "Consequential product, architecture/public-contract or high-risk corrections SHALL require human input." (spec:24) — yet the amendment clauses drop architecture (spec:31, policy:16, pseudocode:69-70) and report-contracts:65 drops both architecture and "no new requirements, product behavior". SKILL:17 keeps architecture. Net effect: a consequential architecture-only change is permitted by policy/report-contracts and forbidden by spec:24/SKILL:17. Fix: paste one verbatim boundary sentence into all five.

  3. Major — the amendment path is unbounded, defeating the budget. "At most ten attempts SHALL be reserved per recovered run…" (spec:31) vs pseudocode's uncapped "REPEAT repair/validation for the amended scope", with no stall/progress test inside the repeat (it exists only before reserving). Attempt ten can absorb unlimited added scope. Fix: cap amendments per attempt, or re-apply the stall test inside the repeat.

  4. Minor — scenario drifts from its requirement. "needs no new product intent, public-contract or consequential design change, security-policy change or high-risk decision" (spec:38) omits the requirement's "new requirements, product behavior" and renames "security/custody policy" to "security-policy". Fix: mirror spec:31 verbatim.

  5. Minor — the before-edits gate for amendments is only outside the spec. spec:31 attaches "before edits" only to the start record; policy:16/pseudocode/report-contracts:65 require the amendment before editing. Fix: add "before the added edits".

  6. Minor — SKILL Stage 3 leaves amendment recording unassigned. "Apply an in-attempt scope amendment when validation finds an additional in-scope finding." (SKILL:39) neither delegates posting/verification as a record (unlike repair-start and summary) nor restates the record-before-edits gate. Fix: mirror the repair-start wording.

  7. Minor — undefined exemption added beyond the ask. "Mechanical in-scope scope amendments and other orchestrator-owned accounting actions SHALL be recorded and continued rather than escalated as human decisions." (spec:57) — "other accounting actions" is open-ended. Fix: enumerate the permitted mechanical actions.

README, PR body, and SKILL agree on the amendment rule (d); the spec is the outlier per 2, 4, 5.

Round-2 judge ledger

ID Provenance Category Evidence (reviewed head) Disposition Resolution evidence
F-001 R1 Standards+Spec blocker SKILL:17, report-contracts:65, policy:16, pseudocode:70 include security/custody repaired-and-verified Holds at e53caa8 and the delivered head; gate asserts it
F-002 R1 Spec significant policy:7-12 restored clauses repaired-and-verified Holds at e53caa8 and the delivered head
F-003 R1 Spec significant pseudocode:54-57, SKILL:39, resume branch repaired-and-verified Holds at e53caa8 and the delivered head
F-004 R1 Spec significant SKILL:33 no retry broadening repaired-and-verified Holds at e53caa8 and the delivered head
F-005 R1 Spec significant pseudocode:15-18 escalation catch-all repaired-and-verified Holds at e53caa8 and the delivered head
F-006 R1 Standards significant factory-skills.test.mjs policy-boundary assertions repaired-and-verified Holds at e53caa8 and the delivered head
F-007 R1 Standards report-only mechanical undefined; "in-scope scope amendment" tautology open Wording only
F-008 R1 Standards report-only report-contracts:65 pointer closed Corrected incidentally under F-001
F-009 R1 Standards report-only pseudocode:3 stale header open Carried
F-010 R1 Standards+Spec report-only boundary/scenario drift closed by F-015 Scenario and architecture drift corrected; remaining wording only
F-011 R1 Standards report-only direct main-spec edit, no change delta open No mandatory rule; house practice and AGENTS.md retirement-only scope
F-012 R1 Standards report-only SKILL:21 duplicated reservation/count sentence open Wording only
F-013 R1 Standards report-only amendment test also asserts prepare.md content open Test organization only
F-014 R1 Standards report-only GLOSSARY.md lacks new terms open Domain-modeling gap
F-015 R2 Standards 4 + Spec 2, elevated from F-010 significant bug Base SKILL.md required human input for "consequential architecture/public-contract choices"; policy.md:12,16, pseudocode, spec:31 and report-contracts had narrowed this to "design" while unchanged spec.md:24 still requires architecture repaired-and-verified All six artifacts now include architecture; gate mutation-checked; independent validation PASS at the delivered head
F-016 R2 Spec 3 report-only pseudocode:54-57 amendment loop has no explicit stall test open Bounded by new-finding/in-scope/repair-worthy requirements and SKILL:33 stall guard; hardening only
F-017 R2 Standards 6 residue report-only gate remains token-presence style open F-006's repair is real; further robustness only
F-018 R2 Standards 7 report-only two openspec validate >500-char warnings open Advisory, pre-existing, not gated
F-019 R2 Spec 6 report-only SKILL:39 does not restate amendment post/verify wording open Derivable from required references; terseness
R-001 R1 refuted accounting ambiguity covered by spec:31/42/policy refuted Verified
R-002 R1 refuted "other orchestrator-owned actions" bounded refuted Verified
R-003 R2 refuted spec:31 self-contradiction claim refuted An amendment's edits occur under the active attempt's already-posted/verified start record; one active reservation is required
R-004 R2 refuted direct main-spec edit as hard violation refuted No mandatory delta rule; same as F-011
R-005 R2 refuted amendment path defeats ten-attempt budget refuted No spec cap violated; retained as F-016 hardening
R-006 R2 refuted gate passes if every amendment condition is deleted refuted Policy boundary tokens now asserted; deleting them fails the gate
R-007 R2 refuted Stage 3 fails to assign amendment recording refuted as material Retained as F-019 terseness

Prior-ID reconciliation: every round-1 actionable ID (F-001–F-006) re-verified as repaired-and-verified; every report-only ID accounted for; F-008 and F-010 closed; F-015 repaired in this attempt. No prior claim was accepted without current code/check evidence.

Audit record

Run identity: PR #87 · run 665e9184c2273162cae5442bd602edc14cb79ade · round 2 · repairs used 2/10.
Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 (both).
Reviewed head: e53caa8098ed028f79d08022b50c61dc4a73a0a5 (local and remote aligned; CI success, run 39717629104).
Delivered head: 77dd9a8c2f3a1c478a700ccafd716ad0d9712231 on patextreme/agent-mod:fix/pr-review-repair-scope-amendment.
Repair-start records: round 1, 1/10 · round 2, 2/10.
Scope amendments: none; the authorized queue was F-015 only.

Repair evidence. Six files changed: skills/pr-review-repair/SKILL.md, skills/pr-review-repair/references/{policy,pseudocode,report-contracts}.md, openspec/specs/pr-review-repair-orchestration/spec.md, tests/gates/factory-skills.test.mjs; 18 insertions, 9 deletions. Architecture is explicit in the human-decision list, policy threshold and amendment boundary, pseudocode amendment boundary, report-contract escalation trigger, spec requirement, and both amendment scenarios. No new requirements were added and no report-only item was changed.

Validation. node --test tests/gates/factory-skills.test.mjs → 22 tests, 21 passed, 1 skipped, 0 failed. node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed. openspec validate pr-review-repair-orchestration --type spec → valid with two pre-existing >500-character requirement warnings (not introduced by this attempt). git diff --check clean. Mutation checks: removing architecture from policy.md or SKILL.md fails the gate; removing it from report-contracts.md also fails. An independent validation agent re-read every hunk, confirmed no unauthorized changes, and independently reproduced the gate mutation behavior. This diff touches no TypeScript, Nix, or package*.json files, so nix flake check, biome, and typecheck were not run; no runtime adherence test exists for the prose contract.

Delivery receipts. Signed commit 77dd9a8c2f3a1c478a700ccafd716ad0d9712231 contains only the six authorized files; pushed without force to the exact PR head branch; remote head confirmed. This comment is the round-2 summary record. Worktree left clean.

Coverage limitation. Requirements remain self-referential: no linked issue and no openspec/changes/** for this work; the PR both writes and satisfies its own capability spec. Findings were anchored to unchanged spec text, the PR body, recorded decisions, and base-vs-head regressions. No live orchestrator run was performed.

@patextreme

Copy link
Copy Markdown
Owner Author

PR review/repair — repair start

  • PR / run / round: fix(skills): handle in-attempt scope amendments without user approval #87 / 665e9184c2273162cae5442bd602edc14cb79ade / 3
  • Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 / 3da0efee1e28bad8ec1fdef87781371fcc71d335
  • Expected local and remote head: 77dd9a8c2f3a1c478a700ccafd716ad0d9712231
  • Repairs used: 3/10
  • Authorized actionable IDs: F-020 (blocker, required CI failure), F-021 (significant bug, public-contract amendment boundary mismatch)
  • Intended scope: Apply biome's required formatting to tests/gates/factory-skills.test.mjs so checks.x86_64-linux.biome-check passes; add the missing escalation catch-all for corrections that fail the in-attempt amendment conditions to skills/pr-review-repair/SKILL.md, references/report-contracts.md, and the spec.md human-input scenario, with a gate assertion. No behavior change beyond closing the public-contract boundary gap; no new product intent, public contract, security policy, or design decision.
  • State: repair reserved; no delivery confirmed

Apply biome formatting to the gate assertion and add a catch-all
escalation for corrections that fail the in-attempt amendment
conditions, so non-consequential public-contract changes cannot slip
past both the amendment boundary and the human-input list.

Tests: nix flake check -L; node tests/run.mjs (411 passed, 1 skipped).
@patextreme

Copy link
Copy Markdown
Owner Author

Repairs delivered — final review pending

Resolved

  • F-020 (blocker) — Applied biome’s required formatting to the gate assertion so the required CI / biome-check derivation passes; no assertion logic changed · c08717f7e805c666ddc0d590cf00023c7f9a2101 · nix flake check -L passes.
  • F-021 (significant bug) — Closed the public-contract amendment-boundary gap with a catch-all escalation for any correction that fails the in-attempt amendment conditions, without narrowing the boundary; added a non-vacuous gate assertion · c08717f7e805c666ddc0d590cf00023c7f9a2101 · mutation-checked and independently validated.

Findings: 0 open blockers · 0 unresolved investigations · 16 report-only (recorded in the audit; no report-only item was edited).

Validation: at the repaired source, nix flake check -L → all checks passed (biome-check, tsc-check, node-tests, and package builds); node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed; npm run typecheck → exit 0; git diff --check clean. The reviewed head 77dd9a8 had a PR-caused biome-check failure; it is fixed in the delivered commit. CI at the delivered head is pending and is not counted as passing yet.

Scope: Requirements remain self-referential — no linked issue and no OpenSpec change; the PR edits its own capability spec. No runtime adherence test exists. The full Nix gate was run locally; live-model semantic suites were not run.

Reviewed: 77dd9a8 · Delivered: c08717f7e805c666ddc0d590cf00023c7f9a2101 · Attempts: 3/10 · Human input: none

Round-3 reports, full ledger, and audit details

Prior complete records:

Round-3 Standards report

Hard violations:

  1. Format gate fails — tests/gates/factory-skills.test.mjs:480: prReviewSkill.includes("consequential design, architecture or public-contract changes"), exceeds biome's default 80-column printWidth (biome.json sets only indentation). CI biome-check fails on it, so AGENTS.md "Quality gates: format → lint → typecheck → test" and "run nix flake check before committing" are unmet at head.

  2. Main spec edited outside the OpenSpec flow — openspec/specs/pr-review-repair-orchestration/spec.md changes Requirement + scenarios with no openspec/changes/** delta on the branch. Repo workflow: .pi/skills/openspec-sync-specs/SKILL.md:15,71 (main specs receive delta specs), and AGENTS.md's "retire it with a ## REMOVED Requirements delta" convention. Direct spec edits bypass validate/sync/archive.

Contract consistency:

  1. Contradictory escalation boundary. Amendment boundary (policy.md:16, pseudocode.md:70, spec.md:31) is no public-contract change and no security/custody-policy change, period. The human-input list (SKILL.md:17, spec.md:42, report-contracts.md:65) qualifies only design/architecture/public-contract as "consequential". Read literally, SKILL.md:17 says a non-consequential public-contract change is not a pause — yet it may never be amended, so it must escalate. Two rules, one decision.

  2. Progressive disclosure drops the gate. SKILL.md:21 — "In-attempt scope amendments keep the active reservation. The attempt count stays unchanged." — states the amendment unconditionally, while policy.md:16 requires authorized intent, not-high-risk, and unambiguous accounting with "Uncertain: escalate." An orchestrator acting from SKILL.md alone (which the skill says it loads first) can amend anything. The conditions belong in SKILL.md's one-liner or the sentence must be marked conditional.

  3. Duplicated normative list (Duplicated Code, smell). The boundary is restated five times (spec requirement, spec scenario, policy.md:16, pseudocode.md:66-71, report-contracts.md:65, SKILL.md:17) and has already drifted in wording ("consequential design or architecture change" vs "consequential design, architecture or public-contract change"). Keep one source; have the rest link.

  4. Middle Man / duplication: SKILL.md ## Prepare paraphrase restates prepare.md's requirement list; the pointer in "Dependencies and control" already covers it.

  5. Gate test copies prose. assert.ok(reportContracts.includes("architecture")) is vacuous — any unrelated use of the word satisfies it; the test also pins seven policy.md strings and two SKILL.md phrases that encode no conditions, and never asserts pseudocode.md's "IF any condition is uncertain: ESCALATE". The file header endorses text-contract tests, so the pattern is repo-sanctioned; the vacuous single-word assertion and the missing escalation fallback are not.

  6. GLOSSARY gap: "scope amendment" is now a domain concept used in specs, policy, and tests but absent from GLOSSARY.md; docs/agents/domain.md "Vocabulary" requires glossary terms for new named concepts or a noted gap.

  7. SKILL.md:17 also duplicates README.md:120's new claim; consistent today, drift-prone.

Round-3 Spec report

(a) Missing or partial

  • No bound on amendments per attempt. Pseudocode REPEAT repair/validation for the amended scope lets a single reservation absorb unlimited successive amendments, so the spec's "At most ten attempts SHALL be reserved per recovered run" no longer bounds total work. No scenario covers repeated or chained amendments.
  • No withdrawal/supersede path. report-contracts defines only an additive template ("A scope amendment adds only findings..."). If, after the record is posted and verified, the repair reveals the correction does need new product intent, no record or handling is defined — every record is append-only and "never ... replaces the active repair-start".
  • Resume-path amendment has no spec scenario. SKILL ("A repair-worthy blocker discovered by that attempt's own validation is part of finishing it") and pseudocode add amendment during recovery of an incomplete attempt; the spec only covers "required validation of the active attempt".

(b) Not asked for

All four PR-body bullets are implemented. Beyond them: the spec's completion clause generalizes to "Mechanical in-scope scope amendments and other orchestrator-owned accounting actions SHALL be recorded and continued rather than escalated" — "other orchestrator-owned accounting actions" is an unnamed, unbounded exemption class absent from the PR body. Also unsourced: policy.md's "is not high-risk (gate-blocking alone does not make it high-risk)".

(c) Implemented but wrong / self-contradictory

  • "Own required validation" vs delegated inspection. Spec: "An in-scope finding discovered by the active attempt's own required validation MAY be added". SKILL stage 3: "Delegate remaining validation and independent inspection of actual changes before delivery" and "a finding from fresh review or judgment starts the next repair cycle". A blocker found by that delegated independent inspector plausibly reads as fresh-review origin (new attempt) rather than own validation (amendment) — the split is undefined at exactly the seam the change targets.
  • MAY vs mandatory. Spec: finding "MAY be added"; SKILL: "Apply an in-attempt scope amendment when validation finds an additional in-scope finding"; completion clause makes declining an escalation. Normative strength disagrees.
  • Uncertainty trigger. policy: "Uncertain: escalate"; pseudocode: "IF any condition is uncertain: ESCALATE instead" — against the spec's enumerated triggers and "SHALL be recorded and continued rather than escalated as human decisions". An orchestrator unsure whether a finding is "mechanical" gets contradictory instructions.

(d) Spec ↔ skill agreement

Escalation boundaries, attempt counting ("keeps the same attempt count" / "KEEPS repairs used unchanged"), the ten-attempt cap, resume identity, and record-before-edits ordering ("posts and verifies an append-only scope amendment" / "POST and VERIFY ... before editing") agree. README ("may be added by a verified scope amendment without consuming another attempt") agrees but drops the pre-edit ordering. Divergences are the three above.

Round-3 judge ledger

ID Provenance Category Evidence / impact Disposition
F-001 R1 both blocker security/custody boundary present in SKILL:17, policy:16, pseudocode:70, report-contracts:65 repaired-and-verified e53caa8
F-002 R1 both significant four threshold clauses present in policy:7-10 repaired-and-verified e53caa8
F-003 R1 spec significant amendment reachability in spec:31 + scenario :37-39, pseudocode:54-57 repaired-and-verified e53caa8
F-004 R1 both significant retry broadening removed from SKILL:33; delivery escalation intact repaired-and-verified e53caa8
F-005 R1 spec significant resume catch-all in pseudocode:15-18 repaired-and-verified e53caa8
F-006 R1 std significant policy-boundary assertions replace the vacuous check repaired-and-verified e53caa8
F-007 R1 std+spec report-only "mechanical"/uncertainty wording; amendment gate governs open
F-008 R1 std report-only stale pointer closed via F-001
F-009 R1 std report-only pseudocode:3 header omits prepare.md/policy.md open
F-010 R1 both report-only boundary/scenario drift closed by F-015/F-021
F-011 R1 std report-only direct main-spec edit; no mandatory delta rule; precedent 7c3c80f open, investigated
F-012 R1 std report-only repeated reservation/count sentence open
F-013 R1 std report-only prepare.md assertion inside amendment test open
F-014 R1 std report-only GLOSSARY lacks "scope amendment"; domain.md allows a noted gap open
F-015 R2 std+spec significant architecture restored in all six artifacts repaired-and-verified 77dd9a8
F-016 R2 spec report-only no explicit amendment-loop stall test; stall escalation covers the risk open
F-017 R2 std report-only gate token-level robustness; long-phrase assertion is the strong pin open
F-018 R2 std report-only two pre-existing openspec validate >500-char warnings open
F-019 R2 spec report-only Stage 3 does not restate amendment post/verify wording open
F-020 R3 std blocker required CI red at 77dd9a8; biome-check reformats factory-skills.test.mjs:480, a line added in 77dd9a8; AGENTS.md gates unmet repaired-and-verified at c08717f7e805c666ddc0d590cf00023c7f9a2101; nix flake check -L passes
F-021 R3 std significant bug amendment boundary forbids any public-contract change (spec:31, policy:16, pseudocode:70) while human-input/escalation lists only pause for consequential public-contract changes (SKILL:17, spec:42, report-contracts:65); non-consequential public-contract changes were neither amendable nor escalated repaired-and-verified at c08717f7e805c666ddc0d590cf00023c7f9a2101; catch-all added to SKILL:17, report-contracts:65, spec:42; gate mutation-checked
F-022 R3 spec report-only no withdrawal/supersede record path; SKILL:33 escalation and marker rules bound the damage open
F-023 R3 spec report-only no spec scenario for resume-path amendments; spec:31 general wording covers it open
F-024 R3 spec report-only own-validation vs delegated-inspection seam; stage is the discriminator; misread is conservative open
F-025 R3 spec report-only MAY vs apply-when vs SHALL; no divergent action open
F-026 R3 std report-only boundary duplication/drift pattern; semantic half is F-021 open
F-027 R3 spec report-only README omits pre-edit verification ordering; skill governs open
R-001–R-007 R1/R2 refuted accounting ambiguity; unbounded orchestrator actions; spec:31 self-contradiction; direct-spec-edit hard violation; budget defeat; gate deletion; Stage 3 recording gap refuted, no recurrence

Prior-ID reconciliation: every round-1 actionable ID (F-001–F-006) and round-2 F-015 re-verified in current code; F-008 and F-010 closed; F-020 and F-021 repaired in this attempt. All report-only IDs accounted for and left untouched.

Audit record

Run identity: PR #87 · run 665e9184c2273162cae5442bd602edc14cb79ade · round 3 · repairs used 3/10.
Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 (both).
Reviewed head: 77dd9a8c2f3a1c478a700ccafd716ad0d9712231; the required CI check failed at this head on biome-check.
Delivered head: c08717f7e805c666ddc0d590cf00023c7f9a2101 on patextreme/agent-mod:fix/pr-review-repair-scope-amendment.
Repair-start records: round 1, 1/10 · round 2, 2/10 · round 3, 3/10.
Scope amendments: none.

Repair evidence. Four files changed: skills/pr-review-repair/SKILL.md, skills/pr-review-repair/references/report-contracts.md, openspec/specs/pr-review-repair-orchestration/spec.md, tests/gates/factory-skills.test.mjs; 11 insertions, 4 deletions. The gate assertion was biome-formatted, and a catch-all was added to the human-decision list, the escalation triggers, and the spec’s human-input scenario. The public-contract amendment boundary was not narrowed. policy.md and pseudocode.md were unchanged. No report-only item was edited.

Validation. nix develop -c biome check . → exit 0, 46 files checked, no fixes applied. nix flake check -L → exit 0, all checks passed (biome-check, tsc-check, node-tests, pi-acp, pi-codex-alias, pi-notify-me, pi-ollama-usage, pi-permission, pi-prompts, pi-skills). node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed. npm run typecheck → exit 0. git diff --check clean. Independent validation force-rebuilt biome-check and node-tests from source with no fixes; mutation checks confirmed the new catch-all assertion fails when removed from either SKILL.md or report-contracts.md. The full Nix gate was executed for this attempt. CI at the delivered head is pending and not counted as passing. No live orchestrator adherence run was performed.

Delivery receipts. Signed commit c08717f7e805c666ddc0d590cf00023c7f9a2101 contains only the four authorized files; pushed without force to the exact PR head branch; remote head confirmed. This comment is the round-3 summary record. Worktree left clean.

Coverage limitation. Requirements remain self-referential: no linked issue and no openspec/changes/**; the PR both writes and satisfies its own capability spec. Findings were anchored to unchanged spec text, the PR body, recorded decisions, and check evidence. Runtime behavior of the prose skill was not exercised.

@patextreme

Copy link
Copy Markdown
Owner Author

Review complete — verified

No repair-worthy findings remain. Both axes reviewed the full PR diff at c08717f, every prior actionable ID is repaired and verified, and the required CI check passes at that head.

Findings: 0 blockers · 0 unresolved investigations · 23 report-only (recorded in the audit; F-008 and F-010 were closed earlier).

Repairs: 3/10 attempts used — F-001–F-006 (attempt 1), F-015 (attempt 2), F-020–F-021 (attempt 3). No repair was needed this round.

Validation: CI (nix flake check -L) success at c08717f (run 39719265594), covering biome, typecheck, node tests, and package builds. Local node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed. openspec validate --specs → 9 passed, 0 failed, 2 advisory long-requirement warnings. Worktree clean; local and remote heads aligned.

Scope: Requirements are self-referential — no linked issue and no OpenSpec change; the PR edits its own capability spec. Review coverage is complete over the diff, but intent cannot be independently validated, and no live orchestrator adherence run was performed. The openspec warnings are advisory and shared by other specs.

Reviewed: c08717f · Delivered: c08717f · Attempts: 3/10 · Human input: none

Round-4 reports, full ledger, and audit details

Prior complete records:

Round-4 Standards report

Standards axis sub-agent report (fresh, read-only; verified HEAD c08717f, clean worktree, all 8 files reviewed; node tests/run.mjs passes 411/1 skip, so nothing here is tooling-enforced).

Documented-standard issues

  1. New domain concepts lack GLOSSARY entries. docs/agents/domain.md — "Use glossary terms when naming domain concepts in issues, proposals, code, and tests… If a needed concept is missing, reconsider the terminology or note the gap." The diff introduces scope amendment, active repair-start, repair attempt/reservation as named concepts with a spec requirement, PR-record artifact, and test assertions, yet GLOSSARY.md (which already carries sibling terms like Issue attempt, Escalation, Gate test) gains nothing. Hard-ish; at minimum a noted gap.

  2. Main spec edited with no OpenSpec change delta. openspec/specs/pr-review-repair-orchestration/spec.md gains a whole Requirement/Scenario set directly; openspec/changes/ has no matching delta (AGENTS.md models spec change through ## REMOVED Requirements deltas, and this repo's other spec edits arrive via archived changes). No doc forbids it outright → judgement call, but unreviewable by openspec validate.

Baseline smells (judgement calls)

  1. Duplicated Code / Shotgun Surgery — one boundary rule now lives in 6 files plus the gate: README.md:120, spec.md (requirement + 2 scenarios), SKILL.md:17,21, policy.md:12,16, pseudocode.md SCOPE-AMENDMENT, report-contracts.md, and tests/gates/factory-skills.test.mjs:465-478. Changing one condition ("public contract", "high-risk") forces a coordinated 7-file edit — the three repair commits (665e918, 77dd9a8, c08717f) are literally that.

  2. Redundant prose locked by a test — SKILL.md:21: "In-attempt scope amendments keep the active reservation. The attempt count stays unchanged." Same fact twice; factory-skills.test.mjs:462 asserts the second wording, freezing the redundancy. The ## Repair policy section likewise restates references/policy.md in shorthand, inviting drift.

  3. Contradictory/repeated escalation logic — pseudocode.md:

IF recovery needs product/design/security/high-risk input or accounting is ambiguous: ESCALATE
IF recovery is blocked or unavailable for any other reason: ESCALATE

The second subsumes the first, and "recovery" never says whether an in-attempt amendment counts as recovery, so line 2 can contradict DO NOT escalate a mechanical scope amendment.

  1. Weak/duplicated assertions — factory-skills.test.mjs:486 reportContracts.includes("architecture") passes on any incidental word; :491 re-asserts a boundary already covered at :465. One 85-line test mixes accounting, boundary, and disclosure concerns.

  2. Naming drift — "mechanical in-scope scope amendment" (SKILL.md:17, asserted at test :509); references/prepare.md (verb) breaks the sibling noun convention (finalization.md, records.md, report-contracts.md).

Positives: all new links resolve (prepare.md, policy.md); the reference split matches sibling skills; README/spec/skill were updated in the same commits.

Round-4 Spec report

Spec axis sub-agent report (fresh, read-only; verified HEAD c08717f, clean worktree, all four commits inspected; npm test reproduces "411 passed, 1 skipped").

(a) Missing or partial

  1. Spec requirement text drops two conditions the skill files make mandatory. Spec line 31: "without incrementing the count, provided the correction introduces no new requirements, product behavior, public contract, security/custody policy or consequential design or architecture change and is not high-risk". It omits new product intent (present in its own Scenario line 42) and the accounting precondition that policy.md requires: "accounting is unambiguous (one active verified repair-start, same run/round, no conflicting or duplicate reservation)". Only the scenario carries it; the normative MAY is under-constrained.
  2. Inconsistent escalation enumeration inside the same spec. "Material repair threshold" still reads "Consequential product, architecture/public-contract or high-risk corrections SHALL require human input" — no security/custody policy, which the new requirement, both new scenarios, and every skill file escalate on.
  3. Resume-time amendment is unspecified. SKILL.md: "A repair-worthy blocker discovered by that attempt's own validation is part of finishing it and may be amended into the reservation"; pseudocode mirrors it. The spec's amendment clause covers only "the active attempt's own required validation"; no scenario covers resumed incomplete attempts.
  4. No spec validation in Verification. Only lint/typecheck/npm test are claimed. openspec validate --specs shows pr-review-repair-orchestration requirement 3 grew and still trips "Requirement text is very long (>500 characters) … split it into separate requirements that each state one behavior" — new behavior was appended to an existing requirement rather than added as its own.

(b) Not asked for

  1. Unsanctioned normative narrowing: policy.md adds "is not high-risk (gate-blocking alone does not make it high-risk)" — a new interpretation absent from the spec, PR body, and every other file.
  2. Frontmatter rewrite: description changed from "Commit, push, and comment on qualifying repairs; escalate after ten repair attempts" to "Authorizes qualifying commits, pushes, and comments; ten-attempt budget" — not in the PR body's compression list; the escalation hint is lost from the discovery text.
  3. Pseudocode resume branch duplicates escalation: "IF recovery needs …: ESCALATE / IF recovery is blocked or unavailable for any other reason: ESCALATE".

(c) Implemented but questionable

  1. Accounting bound is partial. Count never increments, so ≤10 holds, but nothing bounds amendment repeats: "REPEAT repair/validation for the amended scope" can loop, and the stall gate ("IF repairs used >= 10 OR … progress stalled: ESCALATE") sits before new reservations only. Attempt ten can absorb unlimited amendments despite "cannot authorize attempt eleven".
  2. Archived-intent vs. amendment boundary conflict: Stage 3 permits "behavior-changing repairs" (update documents, rerun validation) without authorization, while the new boundary escalates "a consequential design, architecture or public-contract change" and "any correction that fails the in-attempt amendment conditions". A validation-discovered public-contract change satisfies both; the diff does not resolve it.
  3. Goal "without user approval" is logically met (pseudocode: "DO NOT escalate a mechanical scope amendment"), but remains gated on comment write access ("posted and verified scope-amendment record"); identical to reservations, so not a new defect.

Coverage limitation

Requirement source is self-referential: PR body and spec were authored by this diff; no issue/OpenSpec change exists, so "missing requirement" judgment is bounded by the PR body's Summary. Gate tests assert phrase presence, not cross-file symmetry — commits 77dd9a8/c08717f exist because restated rules drifted, which the new tests still cannot detect (they would pass with the pseudocode's missing "product intent" boundary). biome is absent here, so the lint/typecheck claims are unverified.

Round-4 judge ledger

ID Provenance Category Evidence / impact Disposition
F-001 R1 both blocker security/custody boundary present in spec:31, scenario:42, SKILL:17, policy:16, pseudocode:70, report-contracts repaired-and-verified e53caa8
F-002 R1 both significant threshold clauses present in policy:7-12 repaired-and-verified e53caa8
F-003 R1 spec significant amendment reachability via SKILL:15,39, pseudocode:54-57 repaired-and-verified e53caa8
F-004 R1 both significant retry broadening removed; delivery escalation intact repaired-and-verified e53caa8
F-005 R1 spec significant resume catch-all in pseudocode:15-18 repaired-and-verified e53caa8
F-006 R1 std significant concrete policy-boundary assertions replace the vacuous check repaired-and-verified e53caa8
F-007 R1 std+spec report-only "mechanical"/uncertainty wording; amendment gate governs open
F-008 R1 std report-only stale pointer closed via F-001
F-009 R1 std report-only pseudocode header density/cosmetic open
F-010 R1 both report-only boundary/scenario drift closed by F-015/F-021
F-011 R1 std report-only direct main-spec edit; no mandatory delta rule; precedent 7c3c80f open, investigated
F-012 R1 std report-only repeated reservation/count fact, frozen by test open
F-013 R1 std report-only preparation assertions inside amendment test open
F-014 R1 std report-only GLOSSARY gap; domain.md permits a noted gap open
F-015 R2 std+spec significant architecture restored in all six artifacts repaired-and-verified 77dd9a8
F-016 R2 spec report-only no explicit amendment-loop stall test; stall guard and pseudocode:58 bound it open
F-017 R2 std report-only token-level gate assertions; strong phrase assertion present open
F-018 R2 std report-only two long-requirement warnings; validate passes; four other specs share the class open
F-019 R2 spec report-only Stage 3 post/verify wording derivable from required references open
F-020 R3 std blocker required biome-check failure at 77dd9a8 repaired-and-verified c08717f; CI green
F-021 R3 std significant public-contract amendment-boundary mismatch repaired-and-verified c08717f; catch-all added and gate-tested
F-022 R3 spec report-only no withdrawal/supersede path; no evidenced failure mode open
F-023 R3 spec report-only no dedicated resume-amendment scenario; spec:31 general wording covers it open
F-024 R3 spec report-only own-validation vs delegated-inspection seam; no contradictory behavior established open
F-025 R3 spec report-only MAY vs apply-when vs SHALL; consistent in effect open
F-026 R3 std report-only boundary duplication/drift; maintenance risk only open
F-027 R3 spec report-only README omits pre-edit ordering; skill governs open
F-028 R4 std/spec report-only duplicate ESCALATE branches in pseudocode; both escalate; amendment is not blocked recovery open
F-029 R4 spec report-only spec:31 MAY omits "new product intent" and accounting precondition; no realistic unsafe amendment the skill forbids open
F-030 R4 spec report-only spec:24 threshold omits security/custody wording; covered by spec:31/scenario:42/SKILL:17 open
F-031 R4 spec report-only policy parenthetical "gate-blocking alone does not make it high-risk"; interpretation, not an unsafe narrowing open
F-032 R4 spec report-only frontmatter description rewrite; discovery text only, body retains rule open
F-033 R4 spec report-only archived-intent vs amendment boundary; layered, no autonomous public-contract path open
F-034 R4 spec report-only PR body's change list omits spec/README/frontmatter edits; disclosure nit open
R-001–R-007 R1/R2 refuted accounting ambiguity; unbounded orchestrator actions; spec:31 self-contradiction; direct-spec-edit hard violation; budget defeat; gate deletion; Stage 3 recording gap refuted, no recurrence

Prior-ID reconciliation: every prior actionable ID — F-001–F-006, F-015, F-020, F-021 — is repaired-and-verified with current-code and current-check evidence at c08717f. F-008 and F-010 are closed. All report-only IDs are accounted for and were intentionally left untouched. No prior claim was accepted without current evidence.

Audit record

Run identity: PR #87 · run 665e9184c2273162cae5442bd602edc14cb79ade · round 4 · repairs used 3/10.
Pinned base / merge-base: 3da0efee1e28bad8ec1fdef87781371fcc71d335 (both).
Reviewed head: c08717f7e805c666ddc0d590cf00023c7f9a2101 (local and remote aligned; worktree clean).
Delivered head: c08717f7e805c666ddc0d590cf00023c7f9a2101 — no new commit in this round.
Repair-start records: round 1, 1/10 · round 2, 2/10 · round 3, 3/10.
Scope amendments: none.

Final-round evidence. Round 4 performed a fresh two-axis review over the full diff 3da0efe...c08717f (8 files, +203/−49) with no prior reports supplied to the reviewer. The judge deduplicated and classified every finding, reconciled every prior ID against current code/checks, and found no repair-worthy blocker or significant bug. All remaining items are wording, duplication, test-organization, glossary, advisory-warning, or disclosure preferences, which the repair policy keeps report-only.

Validation. Required CI (nix flake check -L) success at c08717f, run 39719265594, job 119628696479, 59s; this covers biome-check, tsc-check, node-tests, and package builds. Local node tests/run.mjs → 412 tests, 411 passed, 1 skipped, 0 failed. openspec validate --specs → 9 passed, 0 failed, 2 advisory warnings (requirements[3] and [5] exceed 500 characters; other specs share the warning class). git status --porcelain empty; local HEAD equals the remote PR head. No live orchestrator adherence run and no semantic suites were executed.

Delivery state. Round 4 adds no commit or push. This comment is the final PR decision record. The PR is open and ready for the user's merge decision; merging remains with the user.

Coverage limitation. Requirements are self-referential: no linked issue and no openspec/changes/**; the PR both writes and satisfies its own capability spec. Review coverage is complete over the diff and all contract surfaces, but intent cannot be independently validated, and the prose contract has no runtime adherence test.

@patextreme
patextreme merged commit 9ee039c into main Oct 22, 2026
1 check passed
@patextreme
patextreme deleted the fix/pr-review-repair-scope-amendment branch October 22, 2026 03:58
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.

1 participant