From 76805cd6cf939895acfc503aa52890506312cad7 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:53:36 +0900 Subject: [PATCH 1/9] fix(review): grade severity by the threatened success criterion Reviews graded findings by the blast radius of the code, so a hole in the integrity mechanism a cycle existed to build was triaged as minor (ROADMAP row 59, retro T7). Reviewing Step 5, the lane pre-delivery gate, and the implementing task-review prompt now grade by the criterion a finding threatens; such a hole is never below P1. Constraint: implementing forbids pre-rating a finding, so the prompt states the grading rule, not a severity Rejected: P2 floor | P2 projects to minor, which implementing defers instead of fixing Confidence: high Scope-risk: narrow Tested: require_review_contract fragments RED then GREEN (actionable_phase_reuse); reviews group Not-tested: no live reviewer run measured the new grading Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-run-artifact-integrity.sh | 3 +++ skills/implementing/SKILL.md | 2 +- skills/reviewing/SKILL.md | 2 ++ skills/reviewing/references/lanes.md | 2 +- 4 files changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/test-run-artifact-integrity.sh b/scripts/test-run-artifact-integrity.sh index 77eaaad..cd5dd0d 100755 --- a/scripts/test-run-artifact-integrity.sh +++ b/scripts/test-run-artifact-integrity.sh @@ -1434,6 +1434,9 @@ def require_review_contract() -> None: (IMPLEMENTING, "Allocate and persist the review event before dispatch"), (IMPLEMENTING, "reviewer body verbatim"), (IMPLEMENTING, "Only the verifying re-review operation writes `fixed`"), + (IMPLEMENTING, "grade each finding by the success criterion it threatens"), + (REVIEWING, "Grade severity by the success criterion the finding threatens"), + (REVIEWING, "never below P1"), (REVIEWING, "cheapest artifact that satisfies every written check"), (REVIEWING, "phase-gate reuse does not allocate another event"), (MERGE_PIPELINE, "review-body and outside-diff"), diff --git a/skills/implementing/SKILL.md b/skills/implementing/SKILL.md index f04ca2f..c473900 100644 --- a/skills/implementing/SKILL.md +++ b/skills/implementing/SKILL.md @@ -136,7 +136,7 @@ Workers never invoke blocking-question tools or address the user directly — re - a mechanism check proving the intended boundary failed rather than fixture setup or unrelated validation. Evidence may reference larger sanitized artifacts in the same unit directory, but it never contains credentials, personal data, or unbounded output. Produce it only with disposable local fixtures, local bare remotes, and stubs; never execute a real outward or production mutation. For a stateless deliverable or unit governed by the plan fallback `No stateful ceremony in the deliverable; no mutation/failure-state matrix required.`, pass that fallback to review and create no irrelevant fixture evidence. -5. **Task review**: generate a diff (`base-before-unit..HEAD`) at `/.tmp/U-diff.tmp`. Publish it to `/reviews/U-diff.txt`. Allocate the unit event through the durable review event protocol, then dispatch. Give the reviewer one approved artifact set: the approved spec, the approved plan including its Mutation/failure-state matrix, the unit evidence directory (or the plan's exact stateless fallback), every applicable committed file discovered under `docs/deviations/` whose Original contract and/or Traceability identifies that approved spec and/or plan as its source, any explicitly handed-off deviation references, the brief path, the report path, the diff path, and Global Constraints copied verbatim from the plan. The link direction is addendum -> approved source: never edit an approved spec or plan to add a backlink just so later review can find the addendum. The reviewer returns **both** verdicts — spec compliance and quality — missing either makes the review incomplete. For each integrity mechanism, the prompt requires the cheapest artifact that satisfies every written check while violating the claimed guarantee. Reviewer-prompt rules: never pre-judge or pre-rate a finding's severity, never tell the reviewer what not to flag; the constraints block is the reviewer's attention lens, copied verbatim, not paraphrased or softened. +5. **Task review**: generate a diff (`base-before-unit..HEAD`) at `/.tmp/U-diff.tmp`. Publish it to `/reviews/U-diff.txt`. Allocate the unit event through the durable review event protocol, then dispatch. Give the reviewer one approved artifact set: the approved spec, the approved plan including its Mutation/failure-state matrix, the unit evidence directory (or the plan's exact stateless fallback), every applicable committed file discovered under `docs/deviations/` whose Original contract and/or Traceability identifies that approved spec and/or plan as its source, any explicitly handed-off deviation references, the brief path, the report path, the diff path, and Global Constraints copied verbatim from the plan. The link direction is addendum -> approved source: never edit an approved spec or plan to add a backlink just so later review can find the addendum. The reviewer returns **both** verdicts — spec compliance and quality — missing either makes the review incomplete. For each integrity mechanism, the prompt requires the cheapest artifact that satisfies every written check while violating the claimed guarantee. The prompt also tells the reviewer to grade each finding by the success criterion it threatens, not by the size or reachability of the code it sits in. Reviewer-prompt rules: never pre-judge or pre-rate a finding's severity, never tell the reviewer what not to flag; the constraints block is the reviewer's attention lens, copied verbatim, not paraphrased or softened. 6. **"Cannot verify from diff"** items are the orchestrator's to resolve, not the reviewer's — they require cross-unit context only you hold. A confirmed gap is a failed spec review: send it back to the implementer. 7. **Plan-mandated conflicts** — a finding that conflicts with what the plan's own text requires is the human's decision, like any plan contradiction: present the finding and the plan text, ask which governs. Never silently dismiss a finding because "the plan says so," and never dispatch a fix that contradicts the plan without asking. 8. **Observable deviation gate** — if review or implementation confirms observable behavior absent from, contradictory to, or materially narrower/broader than the approved artifact set, acceptance is blocked until a separate committed deviation addendum records that behavior. A post-approval change to a Mutation/failure-state matrix row or outcome is such observable behavior and follows this committed addendum rule before acceptance. Do not treat this addendum rule as permission to bypass step 7: when the plan itself mandates the conflicting behavior, the human still decides whether the plan governs or a change is required. Use `docs/solutions/workflow-issues/review-introduced-state-machine-deviation.md` for the observable-behavior definition, required addendum contents, the incomplete-release recovery example that requires an addendum, and the internal-refactor exemption when interfaces, state transitions, persistence, consent boundaries, and terminal behavior stay unchanged. diff --git a/skills/reviewing/SKILL.md b/skills/reviewing/SKILL.md index 2cc12fa..1d9fc93 100644 --- a/skills/reviewing/SKILL.md +++ b/skills/reviewing/SKILL.md @@ -98,6 +98,8 @@ For every full review, including every phase-gate review, dispatch the `test-int Full pipeline in `references/merge-pipeline.md` (fingerprint dedup, cross-lane promotion, mode-aware demotion, confidence gate last with the P0-at-50 exception, atomic artifact writes). Severity is **P0-P3 everywhere** -- the envelope's critical/important/minor `rollup` is a presentation-only projection, never a second severity scale. +Grade severity by the success criterion the finding threatens, not by the size or reachability of the code path it sits in (`CONCEPTS.md`: threatened-criterion severity). A hole in an integrity mechanism the change exists to build, or a defect that defeats a named success criterion, is never below P1, however narrow its current blast radius. + ## Step 6: Verification (dual path) - **Interactive** (default mode, standalone or phase-gate in a chat session): a validator wave -- one independent validator per surviving finding, budget cap 15 (never drop a P0/P1 to fit the cap; raise it instead). Protocol and infra-failure handling: `references/merge-pipeline.md`. diff --git a/skills/reviewing/references/lanes.md b/skills/reviewing/references/lanes.md index c2bedf0..4ae545e 100644 --- a/skills/reviewing/references/lanes.md +++ b/skills/reviewing/references/lanes.md @@ -4,7 +4,7 @@ Every lane -- always-on, conditional, or project-defined -- emits `schemas/lane- **Evidence is not optional** (`enforces: P3`): every finding carries at least one piece of code-grounded evidence (snippet, line reference, or traced pattern) and a `why_it_matters` that names what breaks -- not what is wrong. If you cannot cite code you read, the finding does not exist yet; go read it or drop it. This applies with full force in the single-call degradation tier, where no schema validator will catch an empty evidence field. -**Pre-delivery quality gate** (every lane, every tier): before returning, re-read each of your findings for vagueness (no nameable failure mode), false positives from skimming (did you actually trace it?), severity calibration against the schema's P0-P3 definitions, and line-number accuracy. A finding that fails this re-read gets fixed or dropped, never delivered as-is. +**Pre-delivery quality gate** (every lane, every tier): before returning, re-read each of your findings for vagueness (no nameable failure mode), false positives from skimming (did you actually trace it?), severity calibration against the schema's P0-P3 definitions and against the success criterion the finding threatens (never the blast radius of the code path; rule in `skills/reviewing/SKILL.md` Step 5), and line-number accuracy. A finding that fails this re-read gets fixed or dropped, never delivered as-is. **Scope tiers** (every lane): primary = changed lines, full confidence. Secondary = unchanged code in the same block/function whose bug becomes reachable through the diff -- report it, `pre_existing: false`. Pre-existing = unrelated unchanged code the diff doesn't interact with -- `pre_existing: true`, reported separately, never counted toward the verdict. Test: if you'd flag the same issue on an identical diff without the surrounding file, it's pre-existing. From 19c67683b3213615b9a1bbfa5db78c38aae36053 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:53:37 +0900 Subject: [PATCH 2/9] fix(implementing): keep dispatched worker commits signed Dispatched implementers did not inherit SSH_AUTH_SOCK, so their commits landed unsigned on a signed branch (ROADMAP row 60). Step 2 now passes the agent socket by variable reference and blocks a unit whose commits differ from the base commit in %G? status or %GK signing key. Rejected: require %G? = G | SSH signing without allowed signers reports U on every commit Rejected: compare status only | a foreign key also reports U (external review round 1) Confidence: medium Scope-risk: narrow Tested: portability fragment assertions RED then GREEN; test-plan-consumer-portability.sh 322 passed Not-tested: no dispatched-worker commit was signed through the new step Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-plan-consumer-portability.sh | 2 ++ skills/implementing/SKILL.md | 2 ++ 2 files changed, 4 insertions(+) diff --git a/scripts/test-plan-consumer-portability.sh b/scripts/test-plan-consumer-portability.sh index 8493992..107e93a 100755 --- a/scripts/test-plan-consumer-portability.sh +++ b/scripts/test-plan-consumer-portability.sh @@ -2655,6 +2655,8 @@ for fragment in ( "/briefs/U-brief.md", "/reports/U-report.md", "/reviews/U-diff.txt", + "SSH_AUTH_SOCK", + "--format='%h %G? %GK'", ): assert fragment in implementing, fragment diff --git a/skills/implementing/SKILL.md b/skills/implementing/SKILL.md index c473900..d1e3fa1 100644 --- a/skills/implementing/SKILL.md +++ b/skills/implementing/SKILL.md @@ -115,6 +115,8 @@ For each unit, in dependency order: 1. **Brief**: write the unit's full text to `/.tmp/U-brief.tmp`. Include exact values, signatures, Files, Interfaces, Test scenarios, and the Execution note. Publish it to `/briefs/U-brief.md`. A dispatch prompt describes one unit, not the session's history — never paste prior-unit summaries into a later dispatch. 2. **Dispatch implementer**: one line on where the unit fits, the final brief path introduced as "read this first — your requirements," interfaces/decisions from earlier units the brief cannot know, your resolution of any ambiguity you noticed, and the temporary report path `/.tmp/U-report.tmp`. Code units follow `tdd`'s Execution note; non-code units write → self-review against spec → commit. After the worker returns, publish the temporary report to `/reports/U-report.md`. + + **Signing pass-through**: when the base-before-unit commit is signed (its `git log -1 --format=%G?` is not `N`), a committing worker must sign with the orchestrator's key. Pass `SSH_AUTH_SOCK` (and `GNUPGHOME` when set) to the worker by variable reference, never as a copied literal path. After the worker returns, run `git log --format='%h %G? %GK' ..HEAD`. Any commit whose signature status or signing key differs from the base commit's blocks the unit until that commit is re-signed; a status other than `G` or `U` always blocks. 3. **Status protocol**: | Status | Orchestrator response | From c7390dc23472499c8766a0bbdbba767f65248be5 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:53:38 +0900 Subject: [PATCH 3/9] fix(planning): keep matrix table cells from splitting on pipes A matrix cell with an unescaped | split the Markdown table and persisted invalid shell syntax (ROADMAP row 56). The matrix contract, plan schema, and self-review now require every literal | in a cell to be escaped, or the command to live in a referenced fenced block, and every command to pass bash -n. Constraint: planning ## 14. Self-review is a pinned conformance clause; its digest and source_generation are repinned Rejected: escape commands only | a prose cell with A | B still split the table (external review round 1) Confidence: high Scope-risk: narrow Tested: fixed-root consumer fragments RED then GREEN; release-loop conformance static/fixture/gate Not-tested: no plan was authored under the new rule Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-plan-consumer-portability.sh | 2 +- scripts/test-run-artifact-integrity.sh | 3 +++ skills/planning/SKILL.md | 4 ++-- skills/planning/schemas/plan-schema.md | 2 +- tests/conformance/release-loop/baseline-policy.json | 2 +- tests/conformance/release-loop/corpus.json | 2 +- tests/conformance/release-loop/source-manifest.json | 2 +- 7 files changed, 10 insertions(+), 7 deletions(-) diff --git a/scripts/test-plan-consumer-portability.sh b/scripts/test-plan-consumer-portability.sh index 107e93a..930295e 100755 --- a/scripts/test-plan-consumer-portability.sh +++ b/scripts/test-plan-consumer-portability.sh @@ -2661,7 +2661,7 @@ for fragment in ( assert fragment in implementing, fragment schema = (plugin_root / "skills/planning/schemas/plan-schema.md").read_text(encoding="utf-8") -for fragment in ("/evidence/U/", "executable probe", "exact partial durable state", "compensation owner"): +for fragment in ("/evidence/U/", "executable probe", "exact partial durable state", "compensation owner", "escapes every literal `|` as `\\|`"): assert fragment in schema, fragment plan_filename_stem = "2026-08-23-001-fix-portable-plan" diff --git a/scripts/test-run-artifact-integrity.sh b/scripts/test-run-artifact-integrity.sh index cd5dd0d..943a7bc 100755 --- a/scripts/test-run-artifact-integrity.sh +++ b/scripts/test-run-artifact-integrity.sh @@ -1377,6 +1377,9 @@ def require_phase_consumer_contract(texts: dict[str, str] | None = None) -> None (PLAN_SCHEMA, "executable probe"), (PLAN_SCHEMA, "exact partial durable state"), (PLAN_SCHEMA, "compensation owner"), + (PLAN_SCHEMA, "escapes every literal `|` as `\\|`"), + (selected["planning"], "escapes every literal `|` as `\\|`"), + (selected["planning"], "passes `bash -n`"), ) missing.extend(fragment for text, fragment in required if fragment not in text) if missing: diff --git a/skills/planning/SKILL.md b/skills/planning/SKILL.md index 5c8ebbe..3e65b17 100644 --- a/skills/planning/SKILL.md +++ b/skills/planning/SKILL.md @@ -161,7 +161,7 @@ Use the code or non-code unit template from `schemas/plan-schema.md` as-is — d A **stateful ceremony** is a workflow whose deliverable can cross an observable side-effect boundary. An **outward-publication boundary** — any action that makes an artifact accessible outside the local repository's default branch (pushing to a remote, creating a remote repository, publishing to a registry, creating a GitHub/platform release, changing repository visibility) — constitutes a stateful ceremony. A plan carrying the stateless fallback whose units include any of these transitions has a recognition error. A **durable transition** is a step that changes persisted or externally observable state across invocations. -If the deliverable contains a stateful ceremony, add a **Mutation/failure-state matrix** plan section. Include one row for every durable transition. Each row must name the transition identity, pre-state, action, expected post-state, owning implementation unit, and evidence owner that will produce disposable fixture evidence under `/evidence/U/`. Fill all six outcome classes: success; forced failure; rerun; rollback or compensation; headless; and cancellation or abort. Every forced-failure outcome names an executable probe, a safe injection boundary and isolation approach, the exact partial durable state, and its compensation owner. Irreversible transitions describe compensation or explicit manual recovery rather than fictional rollback. Blank cells are invalid; every not-applicable cell gives a concrete reason tied to the interface or irreversibility boundary. +If the deliverable contains a stateful ceremony, add a **Mutation/failure-state matrix** plan section. Include one row for every durable transition. Each row must name the transition identity, pre-state, action, expected post-state, owning implementation unit, and evidence owner that will produce disposable fixture evidence under `/evidence/U/`. Fill all six outcome classes: success; forced failure; rerun; rollback or compensation; headless; and cancellation or abort. Every forced-failure outcome names an executable probe, a safe injection boundary and isolation approach, the exact partial durable state, and its compensation owner. Irreversible transitions describe compensation or explicit manual recovery rather than fictional rollback. Blank cells are invalid; every not-applicable cell gives a concrete reason tied to the interface or irreversibility boundary. Every matrix cell escapes every literal `|` as `\|`, because a Markdown parser otherwise splits the cell at the pipe. A command, probe, or condition may instead live in a fenced code block outside the table that the cell references. Every such command passes `bash -n` after unescaping. Use `references/stateful-ceremony-matrix-example.md` as the worked example and `docs/solutions/workflow-issues/review-introduced-state-machine-deviation.md` as the deviation authority; link them rather than duplicating their contracts. Changing an approved matrix row or outcome is observable behavior and triggers item 3's deviation-addendum rule before release. @@ -202,7 +202,7 @@ Before finalizing, the author (not a subagent) checks: next step is a plan gap, not an implementation-time unknown. A known value deliberately out of scope goes to Deferred to Follow-Up Work with its reason, and a verdict no unit consumes is itself either a gap or a deliberate Deferred entry. -- **Mutation/failure-state completeness** — when the deliverable contains a stateful ceremony, confirm every durable transition has a row with transition identity, pre-state, action, expected post-state, all six outcome classes, and an implementation-unit/evidence-owner mapping. Confirm irreversible transitions name compensation or manual recovery, every forced failure uses safe isolated injection, and no cell is blank or uses not-applicable without a concrete reason. Otherwise confirm the exact stateless fallback is present. +- **Mutation/failure-state completeness** — when the deliverable contains a stateful ceremony, confirm every durable transition has a row with transition identity, pre-state, action, expected post-state, all six outcome classes, and an implementation-unit/evidence-owner mapping. Confirm irreversible transitions name compensation or manual recovery, every forced failure uses safe isolated injection, and no cell is blank or uses not-applicable without a concrete reason. Confirm no matrix cell holds an unescaped `|`, and every command in a cell or its referenced fenced block passes `bash -n`. Otherwise confirm the exact stateless fallback is present. - **Placeholder scan** — search for step 13's red flags; fix inline. - **Type consistency** — do signatures, names, and types agree across units (a function `clearLayers()` in U2 and `clearFullLayers()` in U5 is a bug)? - **Callers + invariants** — for code units, who calls the functions being changed, and what invariants must still hold afterward? diff --git a/skills/planning/schemas/plan-schema.md b/skills/planning/schemas/plan-schema.md index fb382a9..cf8fdcc 100644 --- a/skills/planning/schemas/plan-schema.md +++ b/skills/planning/schemas/plan-schema.md @@ -240,7 +240,7 @@ The adoption transition is fail-closed at every interruption boundary: 4. **File structure** — files to create/modify, grouped by responsibility (single-responsibility files, colocate what changes together, follow the existing codebase's scale). 5. **Scenario coverage map** — one row per User Scenario (S-ID) in the origin spec: the ordered chain of units that realizes it end to end, and the scenario evidence that walks it — for code plans, integration test scenario(s) tagged `Covers S`; for non-code plans, a named observable verification per scenario (e.g. "a reader following README.md alone completes S1"), since non-code units carry no test field. A scenario with neither a completing unit chain nor walking evidence is a plan gap that blocks approval — either add the missing unit or send the scenario back to `designing` for explicit descoping. When the origin spec has no User Scenarios section (or no spec exists), state that in this section explicitly — never leave it absent. The map is the durable traceability record downstream verification runs against (`enforces: P8`); the fresh verification itself happens in `implementing`'s final branch review and `reviewing`'s tests lane (`enforces: P3` there, not here). 6. **Implementation Units** (see below). -7. **Mutation/failure-state matrix** — conditional on the deliverable containing a stateful ceremony: a workflow whose deliverable can cross an observable side-effect boundary. A durable transition is a step that changes persisted or externally observable state across invocations. Include one row per durable transition with transition identity, pre-state, action, expected post-state, owning implementation unit, and the evidence owner that will produce disposable fixture evidence under `/evidence/U/`, where `artifact_root = dirname(progress_path)` for the selected exact repo-relative progress record. Fill all six outcome classes: success; forced failure; rerun; rollback or compensation; headless; and cancellation or abort. Blank cells are invalid; every not-applicable cell must contain a concrete reason tied to the interface or irreversibility boundary. Every forced-failure outcome names an executable probe, a safe injection boundary and isolation approach, the exact partial durable state, and its compensation owner; irreversible transitions name compensation or explicit manual recovery. Link `skills/planning/references/stateful-ceremony-matrix-example.md` and `docs/solutions/workflow-issues/review-introduced-state-machine-deviation.md` rather than duplicating their contracts. Changing an approved row or outcome is observable behavior and triggers item 3's deviation-addendum rule before release. When the deliverable has no stateful ceremony, write exactly: `No stateful ceremony in the deliverable; no mutation/failure-state matrix required.` +7. **Mutation/failure-state matrix** — conditional on the deliverable containing a stateful ceremony: a workflow whose deliverable can cross an observable side-effect boundary. A durable transition is a step that changes persisted or externally observable state across invocations. Include one row per durable transition with transition identity, pre-state, action, expected post-state, owning implementation unit, and the evidence owner that will produce disposable fixture evidence under `/evidence/U/`, where `artifact_root = dirname(progress_path)` for the selected exact repo-relative progress record. Fill all six outcome classes: success; forced failure; rerun; rollback or compensation; headless; and cancellation or abort. Blank cells are invalid; every not-applicable cell must contain a concrete reason tied to the interface or irreversibility boundary. Every forced-failure outcome names an executable probe, a safe injection boundary and isolation approach, the exact partial durable state, and its compensation owner; irreversible transitions name compensation or explicit manual recovery. Every matrix cell escapes every literal `|` as `\|`; a command, probe, or condition may instead live in a referenced fenced code block outside the table, and every such command passes `bash -n` after unescaping. Link `skills/planning/references/stateful-ceremony-matrix-example.md` and `docs/solutions/workflow-issues/review-introduced-state-machine-deviation.md` rather than duplicating their contracts. Changing an approved row or outcome is observable behavior and triggers item 3's deviation-addendum rule before release. When the deliverable has no stateful ceremony, write exactly: `No stateful ceremony in the deliverable; no mutation/failure-state matrix required.` 8. **Carry-forward trigger audit** — records the planning-time trigger audit: the classification of every open carry-forward tracker row's trigger — edit-based (fires on a named file or section being touched), drift-based (fires on a named record shape deviating), or event-based (fires on a future occurrence) — diffed against the plan's file list and observable record state. Three record shapes, lean by design: one row per fired trigger (tracker row, trigger class, what fired it, disposition with reason — "what fired it" is a deliberate one-field addition to spec R5's enumeration, approved with docs/plans/2026-07-24-001-feat-planning-trigger-audit-plan.md); one row per unobservable drift-based trigger (tracker row, the named record, why it is not observable at planning time); and one always-present attestation line in the fixed template `Audited at : open rows, fired, unobservable.` where tracker state is a commit or equivalent identifier. Fired includes latched rows: a firing already recorded in the tracker or a prior retro counts regardless of current observability. When no trigger fired and nothing is unobservable, the section is the attestation line alone. A fired trigger's disposition is fold-as-unit or a Deferred to Follow-Up Work entry naming the row and the reason; a fired row with neither blocks approval. When the repo has no durable tracker, write exactly: `No durable carry-forward tracker in this repo; no trigger audit possible.` 9. **Deferred to Follow-Up Work** — tangential discoveries and scope creep land here, never in units (`enforces: P4`). 10. **Open unknowns** — split into *planning-time* (must resolve before approving the plan) and *implementation-time* (deferred implementation notes: exact method names, final SQL, runtime-dependent behavior — resolved during execution, listed so they are not mistaken for gaps). diff --git a/tests/conformance/release-loop/baseline-policy.json b/tests/conformance/release-loop/baseline-policy.json index e82a689..76a1ad9 100644 --- a/tests/conformance/release-loop/baseline-policy.json +++ b/tests/conformance/release-loop/baseline-policy.json @@ -3,6 +3,6 @@ "state": "bootstrap", "approved_spec": "docs/specs/2026-08-24-release-loop-conformance-fuzzing-design.md", "approved_spec_sha256": "2cde033379b87d6c8eb92ea32ea3800a82625d86056da496343a91cf0bd8930b", - "source_generation": "7ab46dfd4e2d45fd6e82d6cdb48e6c83ec92d7d6a87a4b8dfa1c25848c92c53f", + "source_generation": "3bc42a7b242f69e29ec6afeffdc63cc40cf5500e7087309640adc76137981fc6", "roadmap_item": "Conformance suite" } diff --git a/tests/conformance/release-loop/corpus.json b/tests/conformance/release-loop/corpus.json index f0e9bb7..60af311 100644 --- a/tests/conformance/release-loop/corpus.json +++ b/tests/conformance/release-loop/corpus.json @@ -1,6 +1,6 @@ { "schema": "release-loop-conformance-corpus/v1", - "source_generation": "7ab46dfd4e2d45fd6e82d6cdb48e6c83ec92d7d6a87a4b8dfa1c25848c92c53f", + "source_generation": "3bc42a7b242f69e29ec6afeffdc63cc40cf5500e7087309640adc76137981fc6", "harnesses": [ "claude", "codex" diff --git a/tests/conformance/release-loop/source-manifest.json b/tests/conformance/release-loop/source-manifest.json index 49b006a..1b8fa8c 100644 --- a/tests/conformance/release-loop/source-manifest.json +++ b/tests/conformance/release-loop/source-manifest.json @@ -28,7 +28,7 @@ "path": "skills/planning/SKILL.md", "heading": "## 14. Self-review", "text": "- **Discrimination check** — for every step that compares two things, run the step's own comparison on two controlled fixture pairs.", - "sha256": "1673a875cde4b2b361083b9cc0e2da35ef6480490bdda653946750f502f75799" + "sha256": "4352c76af3a93f1b4e75a373b6301ea4d7df0e12a2254bfbd5e5243b28bf8e55" }, { "id": "release-loop-pending-gate", From 83cb3b5339088b4fe2c81d89223eb2028c95764e Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:53:39 +0900 Subject: [PATCH 4/9] fix(shipping): split the merge from branch deletion gh pr merge --delete-branch deletes the local branch inside the merge command, so it exited non-zero after a successful remote merge whenever a worktree held the branch (ROADMAP row 62, PRs #22 and #23). Step 7 now merges with gh pr merge --squash [--auto]; Step 8 checks MERGED before any retry and deletes the local, then remote, branch as compare-and-delete steps bound to the merged headRefOid. The shipping invariance case pinned commit 5f3036b and had failed since #41 rewrote a citation in the same paragraph; it now asserts the merge command, ordering, and cleanup commands directly. The unused git_show helper is removed. Constraint: the pending-disposition section is a pinned conformance clause; its digest and source_generation are repinned Rejected: git branch -D after a tip check | the tip can move between check and delete (external review round 2) Rejected: update-ref alone | it ignores worktrees that still hold the branch (external review round 3) Confidence: medium Scope-risk: moderate Directive: never re-add --delete-branch; fork heads are never deleted by this step Tested: shipping cases RED then GREEN; git CAS probe in a scratch repo; conformance static/fixture/gate; validate.sh Not-tested: no real PR has been merged through the new Step 8 sequence Assisted-by: Claude Opus 5.5 with oh-my-pi --- ...uash-merge-commit-message-not-rewritten.md | 3 +- scripts/test-release-loop-conformance.sh | 14 +++--- scripts/test-run-artifact-integrity.sh | 47 ++++++++++--------- skills/shipping/SKILL.md | 15 ++++-- .../release-loop/baseline-policy.json | 2 +- tests/conformance/release-loop/corpus.json | 2 +- .../release-loop/source-manifest.json | 2 +- 7 files changed, 48 insertions(+), 37 deletions(-) diff --git a/docs/solutions/workflow-issues/squash-merge-commit-message-not-rewritten.md b/docs/solutions/workflow-issues/squash-merge-commit-message-not-rewritten.md index 5b7802a..f32cee6 100644 --- a/docs/solutions/workflow-issues/squash-merge-commit-message-not-rewritten.md +++ b/docs/solutions/workflow-issues/squash-merge-commit-message-not-rewritten.md @@ -43,7 +43,8 @@ Net reduction: 165,383 → 149,832 B (−9.4%). All gate/integrity clauses preserved; validate.sh + portability suite green. EOF -gh pr merge --squash --delete-branch --body-file "$BODY_FILE" +# Branch deletion is a separate step (shipping Step 8); never add --delete-branch. +gh pr merge --squash --body-file "$BODY_FILE" ``` Key points: diff --git a/scripts/test-release-loop-conformance.sh b/scripts/test-release-loop-conformance.sh index a1da850..9872b9c 100644 --- a/scripts/test-release-loop-conformance.sh +++ b/scripts/test-release-loop-conformance.sh @@ -532,8 +532,8 @@ def validate_gh_command(argv, fixture_root): if len(argv) == 6 and argv[1:4] == ["pr", "comment", "1"] and argv[4] == "--body" and argv[5]: return True, "fixture-gh-comment" if argv in ( - ["gh", "pr", "merge", "1", "--squash", "--delete-branch"], - ["gh", "pr", "merge", "1", "--squash", "--delete-branch", "--auto"], + ["gh", "pr", "merge", "1", "--squash"], + ["gh", "pr", "merge", "1", "--squash", "--auto"], ): return True, "fixture-gh-merge" if argv in ( @@ -753,8 +753,8 @@ def gh_allowed(): if len(args) == 6 and args[1:4] == ["pr", "comment", "1"] and args[4] == "--body" and args[5]: return True if args in ( - ["gh", "pr", "merge", "1", "--squash", "--delete-branch"], - ["gh", "pr", "merge", "1", "--squash", "--delete-branch", "--auto"], + ["gh", "pr", "merge", "1", "--squash"], + ["gh", "pr", "merge", "1", "--squash", "--auto"], ): return True if args in ( @@ -860,8 +860,6 @@ elif args[:2] == ["pr", "merge"]: git("merge", "--squash", "origin/feat/fuzz-fixture", cwd=merge_repo) git("commit", "-m", "fixture squash merge", cwd=merge_repo) git("push", "origin", "main", cwd=merge_repo) - if "--delete-branch" in args: - git("push", "origin", "--delete", "feat/fuzz-fixture", cwd=merge_repo) pr["merged"] = True pr["merge_sha"] = git("rev-parse", "HEAD", cwd=merge_repo) save() @@ -1251,7 +1249,7 @@ def validate_fixture(): ["gh", "api", "repos/{owner}/{repo}/pulls/1/reviews", "--jq", "length"], ["gh", "api", "repos/{owner}/{repo}/pulls/1/comments", "--jq", "length"], ["gh", "api", "graphql", "-F", "number=1", "-f", "query=fixture"], - ["gh", "pr", "merge", "1", "--squash", "--delete-branch"], + ["gh", "pr", "merge", "1", "--squash"], ["gh", "pr", "view", "1", "--json", "mergeCommit", "--jq", ".mergeCommit.oid // empty"], ) results = [ @@ -1288,7 +1286,7 @@ def validate_fixture(): wrapper_command + ["git", "push", "origin", "--receive-pack=false", "main"], wrapper_command + ["gh", "release", "create", "v1"], wrapper_command + ["gh", "pr", "view", "1", "--repo", "outside/repo"], - wrapper_command + ["gh", "pr", "merge", "1", "--squash", "--delete-branch", "--repo", "outside/repo"], + wrapper_command + ["gh", "pr", "merge", "1", "--squash", "--repo", "outside/repo"], wrapper_command + ["gh", "api", "--hostname", "example.invalid", "user"], ) for command in forbidden: diff --git a/scripts/test-run-artifact-integrity.sh b/scripts/test-run-artifact-integrity.sh index 943a7bc..897df48 100755 --- a/scripts/test-run-artifact-integrity.sh +++ b/scripts/test-run-artifact-integrity.sh @@ -634,16 +634,19 @@ def new_history_repo(tmp: Path, conflict: bool = False, name: str = "repo") -> P return repo -def git_show(source_root: Path, revision: str, path: str) -> str: - result = subprocess.run( - ("git", "show", f"{revision}:{path}"), - cwd=source_root, - check=True, - text=True, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) - return result.stdout +SHIPPING_MERGE_COMMAND = "gh pr merge --squash [--auto]" +SHIPPING_MERGE_ORDERING = ( + "Merge ordering invariant, never reordered: **merge -> verify tests on the merged result -> " + "run every eligible approved-plan pre-removal transition -> remove worktree -> " + "delete local branch -> delete remote branch.**" +) +SHIPPING_CLEANUP_COMMANDS = ( + "gh pr view --json state --jq '.state'", + "gh pr view --json headRefName,headRefOid,isCrossRepository", + "git worktree list --porcelain", + "git update-ref -d refs/heads/ ", + "git push --force-with-lease=: origin --delete ", +) def shipping_command_blocks(text: str) -> dict[str, object]: @@ -3154,21 +3157,21 @@ def run_case(name: str) -> None: event = history.complete_review("standalone:branch:1", git(repo, "rev-parse", "HEAD")) assert history.phase_gate(str(event["id"])) elif name in {"shipping_command_invariance", "shipping_command_changed_axis"}: - baseline = git_show(ROOT, "5f3036b767a5e951aa0ab711832daa24c64915d1", "skills/shipping/SKILL.md") observed = PHASE_CONSUMERS["shipping"] - if name == "shipping_command_changed_axis": - observed = observed.replace( - "gh pr merge --squash --delete-branch [--auto]", - "gh pr merge --merge --delete-branch [--auto]", - 1, - ) - baseline_commands = shipping_command_blocks(baseline) - observed_commands = shipping_command_blocks(observed) + blocks = shipping_command_blocks(observed) if name == "shipping_command_invariance": - assert observed_commands == baseline_commands + assert blocks["merge"] == (SHIPPING_MERGE_COMMAND,), blocks["merge"] + assert "--delete-branch" not in blocks["merge"][0] + assert "--squash --delete-branch" not in observed + assert blocks["cleanup"][0][0].startswith(SHIPPING_MERGE_ORDERING), blocks["cleanup"][0] + missing = set(SHIPPING_CLEANUP_COMMANDS) - set(blocks["cleanup"][1]) + assert not missing, sorted(missing) else: - assert observed_commands != baseline_commands - assert observed_commands["cleanup"] == baseline_commands["cleanup"] + mutated = observed.replace(SHIPPING_MERGE_COMMAND, "gh pr merge --merge [--auto]", 1) + assert mutated != observed + mutated_blocks = shipping_command_blocks(mutated) + assert mutated_blocks["merge"] != blocks["merge"] + assert mutated_blocks["cleanup"] == blocks["cleanup"] assert sent.read_bytes() == before write_history_evidence(name, repo, history, pre_observation, sent, before, command) write_matrix_observation(name, repo, matrix_pre_state, sent, before) diff --git a/skills/shipping/SKILL.md b/skills/shipping/SKILL.md index 4452f4d..a1dbf05 100644 --- a/skills/shipping/SKILL.md +++ b/skills/shipping/SKILL.md @@ -141,7 +141,7 @@ If invoked from `release-loop`, update the supplied exact `progress_path` with r ## Step 7: Merge Gate -**Persist before the gate resolves**: before asking the blocking question or evaluating `--auto` conditions, write the exact merge command (the `gh pr merge --squash --delete-branch` line with literal values) plus the non-authorization marker "preparation evidence -- first-hand consent still required" to the durable record. This holds on every path that reaches this step -- interactive, `--auto`, and dispatched worker; preparation-only never gets here, having terminated at Step 0 with its manual-command file. Re-persist whenever the command changes (e.g. the merge strategy is overridden by repo convention). On every merge path, append a separate `merged-result-verification-command` record containing the exact full-suite Step 1 command with literal values before presenting the merge gate; a missing or ambiguous record blocks the gate. Sink by mode: dispatched worker -> the hand-up packet (the structured payload a dispatched worker returns to its orchestrator when it cannot execute the protected action); `release-loop` -> the supplied exact `progress_path` (legacy compatibility mapping: `release-loop` -> `.release-loop/progress.md` only when that exact legacy ledger was selected); standalone -> the worktree's git-dir state (`$(git rev-parse --git-dir)/shipping-final-action.md`, not a tracked or root-level file). The record is preparation evidence, not authorization. `enforces: P3, P7` +**Persist before the gate resolves**: before asking the blocking question or evaluating `--auto` conditions, write the exact merge command (the `gh pr merge --squash` line with literal values; it never carries `--delete-branch`, see Step 8) plus the non-authorization marker "preparation evidence -- first-hand consent still required" to the durable record. This holds on every path that reaches this step -- interactive, `--auto`, and dispatched worker; preparation-only never gets here, having terminated at Step 0 with its manual-command file. Re-persist whenever the command changes (e.g. the merge strategy is overridden by repo convention). On every merge path, append a separate `merged-result-verification-command` record containing the exact full-suite Step 1 command with literal values before presenting the merge gate; a missing or ambiguous record blocks the gate. Sink by mode: dispatched worker -> the hand-up packet (the structured payload a dispatched worker returns to its orchestrator when it cannot execute the protected action); `release-loop` -> the supplied exact `progress_path` (legacy compatibility mapping: `release-loop` -> `.release-loop/progress.md` only when that exact legacy ledger was selected); standalone -> the worktree's git-dir state (`$(git rev-parse --git-dir)/shipping-final-action.md`, not a tracked or root-level file). The record is preparation evidence, not authorization. `enforces: P3, P7` **Message freshness**: if review rounds (Step 6) produced additional fix commits, regenerate the merge commit message from the current diff against base — the Step 3 draft describes the pre-review artifact and is stale after review changes it. @@ -215,7 +215,7 @@ Default: present the PR (CI status, comments fixed/deferred, any deferred items) For an interactive final disposition invoked by `release-loop`, atomically set `phase_status: waiting-user` and issue `pending_gate.id: ship-approval` before asking. Record a fresh `issued_at` and `expected_answer_class: merge-or-nonmerge-disposition`. After observing the disposition, validate its timestamp and reserved receipt. Then atomically remove `pending_gate` and `gate_answer_receipt`, change `phase_status`, and log the outcome. Standalone and `--auto` paths do not issue these records. ```bash -gh pr merge --squash --delete-branch [--auto] +gh pr merge --squash [--auto] ``` ## Step 8: Cleanup @@ -224,7 +224,7 @@ gh pr merge --squash --delete-branch [--auto] Runs only for the **merge** and **discard** outcomes -- "keep as-is" and "PR open for iteration" always preserve the worktree, since the user needs it alive. -Merge ordering invariant, never reordered: **merge -> verify tests on the merged result -> run every eligible approved-plan pre-removal transition -> remove worktree -> delete branch.** This transition path applies only to a chosen merge outcome. A transition is eligible only when `shipping` is invoked by `release-loop` and the approved, body-sealed plan contains a `## Release-loop Ship-cleanup transition R:` section with an explicit owner, executable acceptance, and matching mutation/failure-state matrix row. Revalidate the plan `body_seal` immediately before running it. Execute eligible transitions in heading order after merged-result verification and before worktree removal; any failure blocks cleanup. A local transition may run headlessly only when its matrix explicitly permits that outcome and its boundary proof makes every outward target unreachable; an outward transition still requires first-hand confirmation at the point of risk. Only remove a worktree this tooling created (path under `.worktrees/` or `worktrees/`); anything else is harness- or user-owned -- leave it. The typed `discard` path executes no approved-plan transition: after `references/question-tools.md` (plugin root) confirmation its separate order is **typed discard confirmation -> remove worktree -> delete branch**, with force-delete (`git branch -D`) only after worktree removal succeeds. +Merge ordering invariant, never reordered: **merge -> verify tests on the merged result -> run every eligible approved-plan pre-removal transition -> remove worktree -> delete local branch -> delete remote branch.** This transition path applies only to a chosen merge outcome. A transition is eligible only when `shipping` is invoked by `release-loop` and the approved, body-sealed plan contains a `## Release-loop Ship-cleanup transition R:` section with an explicit owner, executable acceptance, and matching mutation/failure-state matrix row. Revalidate the plan `body_seal` immediately before running it. Execute eligible transitions in heading order after merged-result verification and before worktree removal; any failure blocks cleanup. A local transition may run headlessly only when its matrix explicitly permits that outcome and its boundary proof makes every outward target unreachable; an outward transition still requires first-hand confirmation at the point of risk. Only remove a worktree this tooling created (path under `.worktrees/` or `worktrees/`); anything else is harness- or user-owned -- leave it. The typed `discard` path executes no approved-plan transition: after `references/question-tools.md` (plugin root) confirmation its separate order is **typed discard confirmation -> remove worktree -> delete branch**, with force-delete (`git branch -D`) only after worktree removal succeeds. For **every merge outcome**, merged-result verification is an executable prerequisite for cleanup: @@ -235,6 +235,15 @@ For **every merge outcome**, merged-result verification is an executable prerequ Every merge path MUST complete steps 1-3 successfully **before cleanup**. Missing SHA, checkout mismatch, absent or ambiguous command evidence, or failed verification blocks cleanup. Step 4 applies only to `release-loop` paths with one or more eligible approved-plan pre-removal transitions. On those paths, the worker MUST also complete step 4 successfully **before persisting any approved-plan transition start** or running that transition. Missing or late success evidence **blocks every pre-removal transition and, on this release-loop path, cleanup**; this prerequisite does not bypass the existing transition-eligibility or user-approval gates. If the success record cannot be appended, treat the evidence as absent and stop. Standalone `shipping` reads the command from its existing git-dir record and has no release-loop `progress.md` recording requirement. The typed `discard` path remains separate and does not run merged-result verification or any approved-plan transition. +**Merge command and branch deletion.** Each operation below is a separate command with its own observed result. The merge command never carries `--delete-branch`: `gh` then deletes the local branch inside the merge command, git refuses while any worktree has the branch checked out, and the command exits non-zero after GitHub has already merged (`docs/solutions/integration-issues/gh-merge-delete-branch-worktree-conflict.md`). + +- **Merge outcome**: when the merge command exits non-zero or its outcome is unknown, read `gh pr view --json state --jq '.state'` before any retry. `MERGED` means the remote merge happened: continue with merged-result verification and never resend the merge. +- **Merged head identity**: before deleting any branch, read `gh pr view --json headRefName,headRefOid,isCrossRepository`. Each deletion below names `headRefOid` as its expected tip, so git refuses it atomically when the branch moved; a refused deletion leaves the branch, which holds commits the merge did not include, and is reported. +- **Local branch**: after worktree removal, confirm `git worktree list --porcelain` shows no `branch refs/heads/` line, because `update-ref` does not check worktrees. Then run `git update-ref -d refs/heads/ `. When a worktree still holds the branch, including one that is not this tooling's to remove, leave the branch and report it. +- **Remote branch**: only when `isCrossRepository` is `false` and `git ls-remote --heads origin ` is not empty, run `git push --force-with-lease=: origin --delete `. An empty result means the repository already deleted the merged head branch. A fork head is not this run's to delete. + +A deletion that git refused because the tip moved is never retried with a newer tip; any other failed deletion is reported and may be retried alone. Neither reverses or repeats the merge. + When the selected ledger is a legacy record (`artifact_root: .release-loop`), permit `.release-loop` removal only after the handoff CLI result records `cleanup_permitted: true` for a complete marker at `.release-loop/.handoff/.json` and an exact base discovery of `.release-loop/progress.md`. Cancellation or any non-complete handoff result preserves both `.release-loop` roots -- source worktree and base checkout -- and blocks local worktree cleanup until it completes. ## Handoff diff --git a/tests/conformance/release-loop/baseline-policy.json b/tests/conformance/release-loop/baseline-policy.json index 76a1ad9..3c5a059 100644 --- a/tests/conformance/release-loop/baseline-policy.json +++ b/tests/conformance/release-loop/baseline-policy.json @@ -3,6 +3,6 @@ "state": "bootstrap", "approved_spec": "docs/specs/2026-08-24-release-loop-conformance-fuzzing-design.md", "approved_spec_sha256": "2cde033379b87d6c8eb92ea32ea3800a82625d86056da496343a91cf0bd8930b", - "source_generation": "3bc42a7b242f69e29ec6afeffdc63cc40cf5500e7087309640adc76137981fc6", + "source_generation": "14b6e61ee6f1be1a60521a6fc29c167d7a5c97305cb6c1a4a21cbd54aa96ffb8", "roadmap_item": "Conformance suite" } diff --git a/tests/conformance/release-loop/corpus.json b/tests/conformance/release-loop/corpus.json index 60af311..ef7206a 100644 --- a/tests/conformance/release-loop/corpus.json +++ b/tests/conformance/release-loop/corpus.json @@ -1,6 +1,6 @@ { "schema": "release-loop-conformance-corpus/v1", - "source_generation": "3bc42a7b242f69e29ec6afeffdc63cc40cf5500e7087309640adc76137981fc6", + "source_generation": "14b6e61ee6f1be1a60521a6fc29c167d7a5c97305cb6c1a4a21cbd54aa96ffb8", "harnesses": [ "claude", "codex" diff --git a/tests/conformance/release-loop/source-manifest.json b/tests/conformance/release-loop/source-manifest.json index 1b8fa8c..81a3e32 100644 --- a/tests/conformance/release-loop/source-manifest.json +++ b/tests/conformance/release-loop/source-manifest.json @@ -63,7 +63,7 @@ "path": "skills/shipping/SKILL.md", "heading": "### Release-loop pending disposition", "text": "For an interactive final disposition invoked by `release-loop`, atomically set `phase_status: waiting-user` and issue `pending_gate.id: ship-approval` before asking.", - "sha256": "189186385075041cb9c38d76922867ea2443fc7f43f412ff4c6bce326df07405" + "sha256": "580810b95c384a4a074dbdcc251e16b45d7fddb1b88cab92619e9e4b85a0d499" }, { "id": "legacy-handoff-persistent-siblings", From b954bdee552ee4d9480ba59501470ca2fb97add3 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:53:39 +0900 Subject: [PATCH 5/9] docs(roadmap): close implemented and stale carry-forward rows Rows 56, 59, 60, and 62 are implemented by this branch. Rows 58, 67, and 68 were already implemented by PR #22 (295909c) and PR #32 (22fcd46) without the tracker being updated. Rows 57 and 64 fired on this branch's edits and are deferred to the unmerged reproducible-evidence branch that implements them. Confidence: high Scope-risk: narrow Tested: git merge-base --is-ancestor and git log -S for each cited commit Assisted-by: Claude Opus 5.5 with oh-my-pi --- ROADMAP.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 158d804..36b1f09 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -53,19 +53,19 @@ Open items registered by retro docs (`docs/retros/`), pushed here per `retrospec | ~~`execution: ops` as a possible third execution mode~~ **Done** carry-forward-clear batch | 2026-07-27 plan-status-terminal-states cycle | P3 | ~~First `execution: ops` instance in this repo~~ resolved: added `ops` to `plan-schema.md` enum and `validate-plan-frontmatter.py` EXECUTIONS set | | ~~Procedural skill text can authorize durable state transitions even when the tracked diff is documentation-only~~ **Done** default-worktree-isolation cycle | 2026-08-03 archive-on-loop-completion retro | P2 | ~~Next planning cycle whose procedural workflow text authorizes durable local mutation~~ resolved: the plan classified runtime effects and included a mutation/failure-state matrix, with Deviation 006 retaining the observed partial branch state | | A success criterion that fires after Retro cannot be measured inside that Retro's Phase 3 pass | 2026-08-03 archive-on-loop-completion retro | P3 | Next design or retrospective cycle declaring a post-Retro terminal criterion — assign proof to the release-loop completion gate and retain the exact evidence path | -| Forced-failure matrices can omit the exact partial durable state and persist invalid shell syntax in Markdown tables | 2026-08-05 default-worktree-isolation retro | P3 | **Fired** in PR #15: plan-specific T0-T7 carried executable probes, expected partial states, rerun behavior, and compensation owners; durable rule remains — next planning-contract change must add the reusable requirement to the matrix contract | -| Facilitator and reviewer output is never persisted, so review claims rest on ledger summaries rather than the reviewer's own words | 2026-08-15 retro-interview-integrity retro | P3 | Next change to the interview protocol's round contract or to `reviewing`'s dispatch steps — write reviewer output verbatim to an artifact at dispatch time | -| Review verifies conformance to the sealed plan instead of attacking the invariant the plan claims to establish | 2026-08-14 retro-interview-integrity retro (T6) | P1 | **Fired** in PR #15: Design, plan, and implementation reviews attacked the claimed invariants; durable rule remains — next change to `reviewing`'s dispatch contract must require construction of the cheapest conforming counterexample | -| Finding severity graded against the blast radius of the code rather than the success criterion the finding threatens | 2026-08-14 retro-interview-integrity retro (T7) | P2 | **Fired** in PR #15: every bounded review graded against the threatened success criterion; durable rule remains — next change to the review triage contract must encode this rule | -| Dispatched agents that commit do not inherit `SSH_AUTH_SOCK`, so their commits land unsigned on an otherwise signed branch | 2026-08-14 retro-interview-integrity retro | P3 | **Fired** in PR #15: U1-U4 were committed by dispatched implementers and all four reported `%G? = G`, but socket pass-through was not retained as evidence; durable rule remains — next dispatched-committer cycle must pass the socket explicitly and verify `%G?` per commit | +| ~~Forced-failure matrices can omit the exact partial durable state and persist invalid shell syntax in Markdown tables~~ **Done** carry-forward bundle A | 2026-08-05 default-worktree-isolation retro | P3 | ~~Next planning-contract change must add the reusable requirement to the matrix contract~~ resolved: partial-state half landed in #22 (`exact partial durable state`); `skills/planning/SKILL.md` matrix contract and self-review plus `plan-schema.md` now require matrix-cell commands to escape literal `\|` or use a referenced fenced block and pass `bash -n` | +| Facilitator and reviewer output is never persisted, so review claims rest on ledger summaries rather than the reviewer's own words | 2026-08-15 retro-interview-integrity retro | P3 | Next change to the interview protocol's round contract or to `reviewing`'s dispatch steps — write reviewer output verbatim to an artifact at dispatch time. **Fired** by carry-forward bundle A (implementing task-review prompt edit); deferred: the reviewer half is implemented on unmerged `orca/reproducible-evidence` (`review-event-integrity.py complete`) and lands with that branch; the facilitator half stays open | +| ~~Review verifies conformance to the sealed plan instead of attacking the invariant the plan claims to establish~~ **Done** | 2026-08-14 retro-interview-integrity retro (T6) | P1 | ~~Next change to `reviewing`'s dispatch contract must require construction of the cheapest conforming counterexample~~ resolved: PR #22 (295909c) added the invariant-attack instruction to `skills/reviewing/SKILL.md` Step 4 and the implementing unit/final review prompts, asserted by `require_review_contract` | +| ~~Finding severity graded against the blast radius of the code rather than the success criterion the finding threatens~~ **Done** carry-forward bundle A | 2026-08-14 retro-interview-integrity retro (T7) | P2 | ~~Next change to the review triage contract must encode this rule~~ resolved: `skills/reviewing/SKILL.md` Step 5, the lane pre-delivery gate, and the implementing task-review prompt grade by threatened criterion; an integrity-mechanism hole is never below P1 | +| ~~Dispatched agents that commit do not inherit `SSH_AUTH_SOCK`, so their commits land unsigned on an otherwise signed branch~~ **Done** carry-forward bundle A | 2026-08-14 retro-interview-integrity retro | P3 | ~~Next dispatched-committer cycle must pass the socket explicitly and verify `%G?` per commit~~ resolved: `skills/implementing/SKILL.md` step 2 signing pass-through passes `SSH_AUTH_SOCK` by reference and blocks a unit whose commits' `%G?` status or `%GK` signing key differs from the base commit | | A retro committed on a feature branch must be merged or pushed in the same action, with the ledger pointer updated then | 2026-08-15 retro-interview-integrity retro | P2 | **Fired** in the run-artifact-integrity recovery — push or merge `retro/run-artifact-integrity` and update the ledger pointer before archive cleanup | -| `gh pr merge --delete-branch` can merge remotely and then exit 1 when local branch cleanup collides with a base branch checked out in another worktree | 2026-08-16 planning-discrimination-and-verdict-coverage retro (T5) | P2 | **Fired again** in PRs #22 and #23 — next shipping merge-command change must split remote merge, merged-result verification, state handoff, local worktree cleanup, local branch deletion, and remote branch deletion | +| ~~`gh pr merge --delete-branch` can merge remotely and then exit 1 when local branch cleanup collides with a base branch checked out in another worktree~~ **Done** carry-forward bundle A | 2026-08-16 planning-discrimination-and-verdict-coverage retro (T5) | P2 | ~~Next shipping merge-command change must split the operations~~ resolved: `skills/shipping/SKILL.md` merges with `gh pr merge --squash [--auto]`, checks `MERGED` before any retry, and deletes the local then remote branch as separate compare-and-delete steps bound to the merged PR's `headRefOid` (`git update-ref -d`, `git push --force-with-lease`; same-repository heads only); conformance fixtures follow the new argv | | Canonical adoption evidence publication rewrites repository records with run-specific timestamps and temporary paths, and per-record replacement can expose mixed generations | 2026-08-16 schema-reference-and-seal-verification retro (T12) | P1 | Next change to `scripts/test-body-seal.sh` evidence publication — keep default validation fixture-local or publish a complete hash-manifested generation through one atomic pointer; consumers fail closed on record/manifest mismatch | -| A final approval can coexist with an unresolved outside-diff finding because thread counts exclude review-body findings | 2026-08-16 schema-reference-and-seal-verification retro (T12, T16) | P1 | **Partially fired again** in PR #22: reviewing now inventories review-body and outside-diff fingerprints with dispositions. Remainder: Shipping still lacks the same per-fingerprint terminal-disposition gate — re-armed for the next shipping or release-loop merge-gate change | +| A final approval can coexist with an unresolved outside-diff finding because thread counts exclude review-body findings | 2026-08-16 schema-reference-and-seal-verification retro (T12, T16) | P1 | **Partially fired again** in PR #22: reviewing now inventories review-body and outside-diff fingerprints with dispositions. Remainder: Shipping still lacks the same per-fingerprint terminal-disposition gate. **Fired** again by carry-forward bundle A (Step 7 merge-command change); deferred: unmerged `orca/reproducible-evidence` implements the gate as `review-event-integrity.py verify-gate` in Shipping Step 7 and lands with that branch | | ~~Legacy selected ledgers have no collision-safe handoff destination contract, so cleanup cannot preserve evidence for mandatory Retro~~ **Done** | 2026-08-25 run-artifact-integrity retro (T4) | P1 | ~~Next change to handoff or Ship cleanup~~ resolved: PR #26 (648f1883) implements crash-recoverable legacy handoff with closed allowlist, v2 marker, collision detection, symlink rejection, `--legacy-destination` opt-in, and acceptance matrix in `transition-hooks.md` and `shipping/SKILL.md` | | Post-merge fix claims retain summarized results but not publisher-owned raw failure and RED/GREEN traces | 2026-08-25 run-artifact-integrity retro (T3) | P2 | Next merged-result failure that creates a follow-up — publish bounded raw output and exact environment observations before citing root cause | -| Two spec-declared test case names (`legacy_handoff_source_changed`, `legacy_handoff_complete_rerun`) were never implemented; add them to close the SC3/SC4 measurement gap | 2026-08-25 legacy-handoff-contract retro (T2) | P2 | Next change to legacy handoff test cases or spec measurement contract | -| Add a mid-directory-copy fault injection (`handoff-after-copy-one-file`) to exercise partial-directory recovery directly | 2026-08-25 legacy-handoff-contract retro (T3) | P3 | Next change to handoff fault injection or recovery code | +| ~~Two spec-declared test case names (`legacy_handoff_source_changed`, `legacy_handoff_complete_rerun`) were never implemented; add them to close the SC3/SC4 measurement gap~~ **Done** | 2026-08-25 legacy-handoff-contract retro (T2) | P2 | ~~Next change to legacy handoff test cases or spec measurement contract~~ resolved: PR #32 (22fcd46) registered both cases in `scripts/test-run-artifact-integrity.sh` | +| ~~Add a mid-directory-copy fault injection (`handoff-after-copy-one-file`) to exercise partial-directory recovery directly~~ **Done** | 2026-08-25 legacy-handoff-contract retro (T3) | P3 | ~~Next change to handoff fault injection or recovery code~~ resolved: PR #32 (22fcd46) added the injection to `run-artifact-integrity.py`, used by `legacy_handoff_v1_partial_directory_rerun` and `legacy_handoff_source_changed` | | ~~Structured review events must be populated when `completeness: exact` — either use the event system or downgrade to `partial`~~ **Done** | 2026-08-25 legacy-handoff-contract retro; resolved 2026-09-20 auto-plan-assumption-tradeoffs retro (T1) | P3 | Resolved: events 1–3 use publisher-owned wrappers, and the ledger explicitly retains `completeness: partial` rather than claiming exact historical coverage | | Review re-review verification must be executable rather than manual ledger editing | 2026-09-19 auto-plan-assumption-tradeoffs retro (T2) | P2 | Next change to reviewing or implementing review-event operations — expose a packaged `verify_re_review` path that validates source/result inventories and writes fixed dispositions atomically | From 49ea8bb4b3d703ffb6b3827fbd204954eaa9625a Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:39:46 +0900 Subject: [PATCH 6/9] fix(implementing): check signatures on fix-subagent commits too The signing pass-through covered only the unit implementer, so an unsigned commit from a step 9 fix subagent could land after the check (CodeRabbit on #43). The rule now applies to every committing worker and runs after each one returns; the portability test asserts the blocking sentence, not just the two command strings. Confidence: high Scope-risk: narrow Tested: test-plan-consumer-portability.sh fragments RED then GREEN, 322 passed Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-plan-consumer-portability.sh | 2 ++ skills/implementing/SKILL.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/test-plan-consumer-portability.sh b/scripts/test-plan-consumer-portability.sh index 930295e..ca62d81 100755 --- a/scripts/test-plan-consumer-portability.sh +++ b/scripts/test-plan-consumer-portability.sh @@ -2657,6 +2657,8 @@ for fragment in ( "/reviews/U-diff.txt", "SSH_AUTH_SOCK", "--format='%h %G? %GK'", + "including step 9 fix subagents", + "blocks the unit until that commit is re-signed", ): assert fragment in implementing, fragment diff --git a/skills/implementing/SKILL.md b/skills/implementing/SKILL.md index d1e3fa1..c35a391 100644 --- a/skills/implementing/SKILL.md +++ b/skills/implementing/SKILL.md @@ -116,7 +116,7 @@ For each unit, in dependency order: 1. **Brief**: write the unit's full text to `/.tmp/U-brief.tmp`. Include exact values, signatures, Files, Interfaces, Test scenarios, and the Execution note. Publish it to `/briefs/U-brief.md`. A dispatch prompt describes one unit, not the session's history — never paste prior-unit summaries into a later dispatch. 2. **Dispatch implementer**: one line on where the unit fits, the final brief path introduced as "read this first — your requirements," interfaces/decisions from earlier units the brief cannot know, your resolution of any ambiguity you noticed, and the temporary report path `/.tmp/U-report.tmp`. Code units follow `tdd`'s Execution note; non-code units write → self-review against spec → commit. After the worker returns, publish the temporary report to `/reports/U-report.md`. - **Signing pass-through**: when the base-before-unit commit is signed (its `git log -1 --format=%G?` is not `N`), a committing worker must sign with the orchestrator's key. Pass `SSH_AUTH_SOCK` (and `GNUPGHOME` when set) to the worker by variable reference, never as a copied literal path. After the worker returns, run `git log --format='%h %G? %GK' ..HEAD`. Any commit whose signature status or signing key differs from the base commit's blocks the unit until that commit is re-signed; a status other than `G` or `U` always blocks. + **Signing pass-through**: when the base-before-unit commit is signed (its `git log -1 --format=%G?` is not `N`), every committing worker, including step 9 fix subagents, must sign with the orchestrator's key. Pass `SSH_AUTH_SOCK` (and `GNUPGHOME` when set) to each such worker by variable reference, never as a copied literal path. After each such worker returns, run `git log --format='%h %G? %GK' ..HEAD`. Any commit whose signature status or signing key differs from the base commit's blocks the unit until that commit is re-signed; a status other than `G` or `U` always blocks. 3. **Status protocol**: | Status | Orchestrator response | From 2662f99fbe937e0ddf9b7250ce53bcbf2f2070a3 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:39:46 +0900 Subject: [PATCH 7/9] fix(shipping): bind branch deletion to the base checkout and PR repository Step 8 did not say where branch deletion runs after the worktree is removed, and the remote deletion trusted origin without checking that its push URL names the PR repository (CodeRabbit on #43). Both deletions now run from the base checkout, and the remote deletion also requires git remote get-url --push origin to match the PR's owner/repo. Rejected: re-check worktrees inside update-ref | git offers no atomic worktree-aware compare-and-delete; the tip CAS still protects commits Confidence: high Scope-risk: narrow Tested: shipping_command_invariance and changed_axis RED then GREEN; validate.sh; history group; conformance static Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-run-artifact-integrity.sh | 1 + skills/shipping/SKILL.md | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/test-run-artifact-integrity.sh b/scripts/test-run-artifact-integrity.sh index 897df48..80850a4 100755 --- a/scripts/test-run-artifact-integrity.sh +++ b/scripts/test-run-artifact-integrity.sh @@ -644,6 +644,7 @@ SHIPPING_CLEANUP_COMMANDS = ( "gh pr view --json state --jq '.state'", "gh pr view --json headRefName,headRefOid,isCrossRepository", "git worktree list --porcelain", + "git remote get-url --push origin", "git update-ref -d refs/heads/ ", "git push --force-with-lease=: origin --delete ", ) diff --git a/skills/shipping/SKILL.md b/skills/shipping/SKILL.md index a1dbf05..a2f0b29 100644 --- a/skills/shipping/SKILL.md +++ b/skills/shipping/SKILL.md @@ -239,8 +239,8 @@ Every merge path MUST complete steps 1-3 successfully **before cleanup**. Missin - **Merge outcome**: when the merge command exits non-zero or its outcome is unknown, read `gh pr view --json state --jq '.state'` before any retry. `MERGED` means the remote merge happened: continue with merged-result verification and never resend the merge. - **Merged head identity**: before deleting any branch, read `gh pr view --json headRefName,headRefOid,isCrossRepository`. Each deletion below names `headRefOid` as its expected tip, so git refuses it atomically when the branch moved; a refused deletion leaves the branch, which holds commits the merge did not include, and is reported. -- **Local branch**: after worktree removal, confirm `git worktree list --porcelain` shows no `branch refs/heads/` line, because `update-ref` does not check worktrees. Then run `git update-ref -d refs/heads/ `. When a worktree still holds the branch, including one that is not this tooling's to remove, leave the branch and report it. -- **Remote branch**: only when `isCrossRepository` is `false` and `git ls-remote --heads origin ` is not empty, run `git push --force-with-lease=: origin --delete `. An empty result means the repository already deleted the merged head branch. A fork head is not this run's to delete. +- **Local branch**: run this and the next step from the base checkout, which survives worktree removal. After worktree removal, confirm `git worktree list --porcelain` shows no `branch refs/heads/` line, because `update-ref` does not check worktrees. Then run `git update-ref -d refs/heads/ `. When a worktree still holds the branch, including one that is not this tooling's to remove, leave the branch and report it. +- **Remote branch**: only when `isCrossRepository` is `false`, `git remote get-url --push origin` names the same `owner/repo` as the PR, and `git ls-remote --heads origin ` is not empty, run `git push --force-with-lease=: origin --delete `. An empty result means the repository already deleted the merged head branch. A fork head or a different push target is not this run's to delete. A deletion that git refused because the tip moved is never retried with a newer tip; any other failed deletion is reported and may be retried alone. Neither reverses or repeats the merge. From b1d7998b4f060398b33a5316f7a696a68c5e3466 Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:41:08 +0900 Subject: [PATCH 8/9] fix(implementing): compare worker signatures with the orchestrator's key Comparing each worker commit's %GK with the base commit blocked correctly signed work whenever another collaborator had signed the base (CodeRabbit round 2 on #43). The expected key is now the orchestrator's own, taken from its latest signed commit in the repository; ROADMAP row 60 states the new rule. Confidence: medium Scope-risk: narrow Tested: portability fragment RED then GREEN, 322 passed; validate.sh Not-tested: no multi-signer repository was exercised Assisted-by: Claude Opus 5.5 with oh-my-pi --- ROADMAP.md | 2 +- scripts/test-plan-consumer-portability.sh | 1 + skills/implementing/SKILL.md | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 36b1f09..81f7ff9 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -57,7 +57,7 @@ Open items registered by retro docs (`docs/retros/`), pushed here per `retrospec | Facilitator and reviewer output is never persisted, so review claims rest on ledger summaries rather than the reviewer's own words | 2026-08-15 retro-interview-integrity retro | P3 | Next change to the interview protocol's round contract or to `reviewing`'s dispatch steps — write reviewer output verbatim to an artifact at dispatch time. **Fired** by carry-forward bundle A (implementing task-review prompt edit); deferred: the reviewer half is implemented on unmerged `orca/reproducible-evidence` (`review-event-integrity.py complete`) and lands with that branch; the facilitator half stays open | | ~~Review verifies conformance to the sealed plan instead of attacking the invariant the plan claims to establish~~ **Done** | 2026-08-14 retro-interview-integrity retro (T6) | P1 | ~~Next change to `reviewing`'s dispatch contract must require construction of the cheapest conforming counterexample~~ resolved: PR #22 (295909c) added the invariant-attack instruction to `skills/reviewing/SKILL.md` Step 4 and the implementing unit/final review prompts, asserted by `require_review_contract` | | ~~Finding severity graded against the blast radius of the code rather than the success criterion the finding threatens~~ **Done** carry-forward bundle A | 2026-08-14 retro-interview-integrity retro (T7) | P2 | ~~Next change to the review triage contract must encode this rule~~ resolved: `skills/reviewing/SKILL.md` Step 5, the lane pre-delivery gate, and the implementing task-review prompt grade by threatened criterion; an integrity-mechanism hole is never below P1 | -| ~~Dispatched agents that commit do not inherit `SSH_AUTH_SOCK`, so their commits land unsigned on an otherwise signed branch~~ **Done** carry-forward bundle A | 2026-08-14 retro-interview-integrity retro | P3 | ~~Next dispatched-committer cycle must pass the socket explicitly and verify `%G?` per commit~~ resolved: `skills/implementing/SKILL.md` step 2 signing pass-through passes `SSH_AUTH_SOCK` by reference and blocks a unit whose commits' `%G?` status or `%GK` signing key differs from the base commit | +| ~~Dispatched agents that commit do not inherit `SSH_AUTH_SOCK`, so their commits land unsigned on an otherwise signed branch~~ **Done** carry-forward bundle A | 2026-08-14 retro-interview-integrity retro | P3 | ~~Next dispatched-committer cycle must pass the socket explicitly and verify `%G?` per commit~~ resolved: `skills/implementing/SKILL.md` step 2 signing pass-through passes `SSH_AUTH_SOCK` by reference to every committing worker, including fix subagents, and blocks a unit whose commits are not `G`/`U` or carry a `%GK` key other than the orchestrator's own | | A retro committed on a feature branch must be merged or pushed in the same action, with the ledger pointer updated then | 2026-08-15 retro-interview-integrity retro | P2 | **Fired** in the run-artifact-integrity recovery — push or merge `retro/run-artifact-integrity` and update the ledger pointer before archive cleanup | | ~~`gh pr merge --delete-branch` can merge remotely and then exit 1 when local branch cleanup collides with a base branch checked out in another worktree~~ **Done** carry-forward bundle A | 2026-08-16 planning-discrimination-and-verdict-coverage retro (T5) | P2 | ~~Next shipping merge-command change must split the operations~~ resolved: `skills/shipping/SKILL.md` merges with `gh pr merge --squash [--auto]`, checks `MERGED` before any retry, and deletes the local then remote branch as separate compare-and-delete steps bound to the merged PR's `headRefOid` (`git update-ref -d`, `git push --force-with-lease`; same-repository heads only); conformance fixtures follow the new argv | | Canonical adoption evidence publication rewrites repository records with run-specific timestamps and temporary paths, and per-record replacement can expose mixed generations | 2026-08-16 schema-reference-and-seal-verification retro (T12) | P1 | Next change to `scripts/test-body-seal.sh` evidence publication — keep default validation fixture-local or publish a complete hash-manifested generation through one atomic pointer; consumers fail closed on record/manifest mismatch | diff --git a/scripts/test-plan-consumer-portability.sh b/scripts/test-plan-consumer-portability.sh index ca62d81..e26efa8 100755 --- a/scripts/test-plan-consumer-portability.sh +++ b/scripts/test-plan-consumer-portability.sh @@ -2659,6 +2659,7 @@ for fragment in ( "--format='%h %G? %GK'", "including step 9 fix subagents", "blocks the unit until that commit is re-signed", + "the orchestrator's own signing key", ): assert fragment in implementing, fragment diff --git a/skills/implementing/SKILL.md b/skills/implementing/SKILL.md index c35a391..75210c5 100644 --- a/skills/implementing/SKILL.md +++ b/skills/implementing/SKILL.md @@ -116,7 +116,7 @@ For each unit, in dependency order: 1. **Brief**: write the unit's full text to `/.tmp/U-brief.tmp`. Include exact values, signatures, Files, Interfaces, Test scenarios, and the Execution note. Publish it to `/briefs/U-brief.md`. A dispatch prompt describes one unit, not the session's history — never paste prior-unit summaries into a later dispatch. 2. **Dispatch implementer**: one line on where the unit fits, the final brief path introduced as "read this first — your requirements," interfaces/decisions from earlier units the brief cannot know, your resolution of any ambiguity you noticed, and the temporary report path `/.tmp/U-report.tmp`. Code units follow `tdd`'s Execution note; non-code units write → self-review against spec → commit. After the worker returns, publish the temporary report to `/reports/U-report.md`. - **Signing pass-through**: when the base-before-unit commit is signed (its `git log -1 --format=%G?` is not `N`), every committing worker, including step 9 fix subagents, must sign with the orchestrator's key. Pass `SSH_AUTH_SOCK` (and `GNUPGHOME` when set) to each such worker by variable reference, never as a copied literal path. After each such worker returns, run `git log --format='%h %G? %GK' ..HEAD`. Any commit whose signature status or signing key differs from the base commit's blocks the unit until that commit is re-signed; a status other than `G` or `U` always blocks. + **Signing pass-through**: when the base-before-unit commit is signed (its `git log -1 --format=%G?` is not `N`), every committing worker, including step 9 fix subagents, must sign with the orchestrator's key. Pass `SSH_AUTH_SOCK` (and `GNUPGHOME` when set) to each such worker by variable reference, never as a copied literal path. Take the expected key from the orchestrator's own signing key: the `%GK` of the latest commit the orchestrator signed in this repository, not the base commit, which another collaborator may have signed. After each such worker returns, run `git log --format='%h %G? %GK' ..HEAD`. Any commit whose status is not `G` or `U`, or whose key differs from the expected key, blocks the unit until that commit is re-signed. 3. **Status protocol**: | Status | Orchestrator response | From 51668314ca9fb49c570db7c63c3e05a87996931a Mon Sep 17 00:00:00 2001 From: Jaehoon You <158752+teslamint@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:41:08 +0900 Subject: [PATCH 9/9] fix(shipping): delete the remote branch by full ref on every push URL check A short branch name could match a same-named tag or another ref in the lookup and the deletion, and git remote get-url --push returned only the first of several push URLs (CodeRabbit round 2 on #43). Step 8 now checks every push URL and uses refs/heads/ for ls-remote, the lease, and the delete. Confidence: high Scope-risk: narrow Tested: shipping_command_invariance and changed_axis RED then GREEN; history group; validate.sh Assisted-by: Claude Opus 5.5 with oh-my-pi --- scripts/test-run-artifact-integrity.sh | 5 +++-- skills/shipping/SKILL.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/test-run-artifact-integrity.sh b/scripts/test-run-artifact-integrity.sh index 80850a4..052e60d 100755 --- a/scripts/test-run-artifact-integrity.sh +++ b/scripts/test-run-artifact-integrity.sh @@ -644,9 +644,10 @@ SHIPPING_CLEANUP_COMMANDS = ( "gh pr view --json state --jq '.state'", "gh pr view --json headRefName,headRefOid,isCrossRepository", "git worktree list --porcelain", - "git remote get-url --push origin", + "git remote get-url --push --all origin", "git update-ref -d refs/heads/ ", - "git push --force-with-lease=: origin --delete ", + "git ls-remote origin refs/heads/", + "git push --force-with-lease=refs/heads/: origin --delete refs/heads/", ) diff --git a/skills/shipping/SKILL.md b/skills/shipping/SKILL.md index a2f0b29..e898c0b 100644 --- a/skills/shipping/SKILL.md +++ b/skills/shipping/SKILL.md @@ -240,7 +240,7 @@ Every merge path MUST complete steps 1-3 successfully **before cleanup**. Missin - **Merge outcome**: when the merge command exits non-zero or its outcome is unknown, read `gh pr view --json state --jq '.state'` before any retry. `MERGED` means the remote merge happened: continue with merged-result verification and never resend the merge. - **Merged head identity**: before deleting any branch, read `gh pr view --json headRefName,headRefOid,isCrossRepository`. Each deletion below names `headRefOid` as its expected tip, so git refuses it atomically when the branch moved; a refused deletion leaves the branch, which holds commits the merge did not include, and is reported. - **Local branch**: run this and the next step from the base checkout, which survives worktree removal. After worktree removal, confirm `git worktree list --porcelain` shows no `branch refs/heads/` line, because `update-ref` does not check worktrees. Then run `git update-ref -d refs/heads/ `. When a worktree still holds the branch, including one that is not this tooling's to remove, leave the branch and report it. -- **Remote branch**: only when `isCrossRepository` is `false`, `git remote get-url --push origin` names the same `owner/repo` as the PR, and `git ls-remote --heads origin ` is not empty, run `git push --force-with-lease=: origin --delete `. An empty result means the repository already deleted the merged head branch. A fork head or a different push target is not this run's to delete. +- **Remote branch**: only when `isCrossRepository` is `false`, every URL from `git remote get-url --push --all origin` names the same `owner/repo` as the PR, and `git ls-remote origin refs/heads/` returns exactly that ref at `headRefOid`, run `git push --force-with-lease=refs/heads/: origin --delete refs/heads/`. Full ref names keep a tag or another ref with the same short name out of the lookup and the deletion. An empty result means the repository already deleted the merged head branch. A fork head or a different push target is not this run's to delete. A deletion that git refused because the tip moved is never retried with a newer tip; any other failed deletion is reported and may be retried alone. Neither reverses or repeats the merge.