From dde77648e1523a36581bd3d50b774420d1d20c49 Mon Sep 17 00:00:00 2001 From: usirin Date: Sun, 19 Jul 2026 17:12:35 -0700 Subject: [PATCH 1/2] =?UTF-8?q?refactor(pipeline):=20adopt=20`verdict=20re?= =?UTF-8?q?ad`=20in=20ship-it=20/=20write-code=20/=20heal-ci=20=E2=80=94?= =?UTF-8?q?=20delete=20the=203=20inline=20resolvers=20(#3686)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the divergent inline SHA-bound verdict-marker resolvers in the ship-it, write-code and heal-ci skills with `pipeline-cli verdict read`, each citing the verb in this same landing commit (the #3254 same-commit-adoption rule), and draw the `verdict read` grandfathered exemptions to zero. The verb owns the (PR, gate) resolution: the ADR-0055 write+ author-gate, the latest-wins pick, and the ADR-0058 SHA-staleness refusal folded into one exit code. Its existing unit tests remain the contract — no verdict-resolution behavior is changed here. Behavior that is MORE than a single (PR, gate) resolution is preserved, not folded into the verb: - write-code keeps the multi-PR pre-pick scan and the N=3 FAIL-round count (the verb resolves the latest verdict; it does not count rounds). - heal-ci keeps its N=3 cap count and the native CHANGES_REQUESTED fold. - ship-it keeps the native APPROVED/CHANGES_REQUESTED review fold and the §CP advisory resolution (an advisory is SHA-less by design, so the verb resolves it `none` — its author-gated pick stays ship-it's). - `is_current` survives only for the two signals the verb does not resolve: a native review's commit_id and a §CP advisory's body Reviewed-head SHA. write-code Step R2 now reads the resolving FAIL body via the comment id the verb returns, rather than re-deriving the latest-wins pick. Fixes #3686 --- .../kampus-pipeline/skills/heal-ci/SKILL.md | 100 +++++----- .../kampus-pipeline/skills/ship-it/SKILL.md | 130 +++++++------ .../skills/write-code/SKILL.md | 179 ++++++++---------- .../src/tools/adoption-lint/manifest.ts | 26 +-- 4 files changed, 205 insertions(+), 230 deletions(-) diff --git a/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md b/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md index b339d061d..05329a2d1 100644 --- a/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md @@ -207,28 +207,53 @@ Step R1). That contract is the floor here: the guard may suppress the twin **onl write-code does, or it would skip the defect on a FAIL write-code will no-op, dropping the failure on the floor. An active repair is an **open PR** whose **latest** gate verdict in *either* namespace is a **FAIL bound to the PR's current head** (`review-code: FAIL @ ` / -`review-doc: FAIL @ `, latest-wins per namespace), still within the N=3 repair cap. Three -parts, each lifted from write-code's scan: - -- **Author-gated against the repo ACL** — a marker counts as a verdict only from a `write+` - collaborator, so a forged `review-(code|doc): FAIL` can't be read as an active repair (ADR - [0055](https://github.com/kamp-us/phoenix/blob/main/.decisions/0055-acl-sourced-review-authz.md), the same trust root `ship-it` - Step 2 and `write-code`'s scan use). A native `CHANGES_REQUESTED` review folds into the code - namespace and needs **no** ACL gate — GitHub author-attributes reviews, so that path is - unforgeable. -- **SHA-bound staleness test** (ADR [0058](https://github.com/kamp-us/phoenix/blob/main/.decisions/0058-sha-bound-verdict-contract.md), - issue #258) — a FAIL whose `@ ` is **not** the PR's current head, or that carries **no** - `@ ` (a pre-0058 legacy marker), is **stale**: it judges code that has since changed, so - `write-code` no-ops on it — therefore it is **not** an active repair here either, and the - defect must fall through and file. This is the load-bearing reconciliation: without the - staleness test the guard would suppress a twin for a FAIL nobody is fixing. -- **Emphasis-tolerant + SHA-capturing matcher** — the canonical shape both sides cite (leading - `\**` absorbs review-code's bolding; the `@\s*([0-9a-f]{7,40})` tail captures the bound head), - per [`../gh-issue-intake-formats.md`](../gh-issue-intake-formats.md) §5. +`review-doc: FAIL @ `, latest-wins per namespace), still within the N=3 repair cap. + +That per-(PR, gate) FAIL-bound-to-head resolution is exactly what +`pipeline-cli verdict read --gate --expect FAIL` owns — the ADR-0055 write+ author-gate, the +latest-wins pick, and the ADR-0058 SHA-staleness test folded into one exit code (its unit tests +are the contract, #2102). So `heal-ci` reads each namespace **through the verb** rather than +re-deriving the resolver write-code once hand-copied, and keeps only the two things the verb does +**not** do — genuinely more than a single (PR, gate) resolution, so they stay here: + +- **The native `CHANGES_REQUESTED` review that folds into the code namespace.** The verb reads + marker comments; a native review is a *different* record type. GitHub author-attributes reviews, + so this path needs **no** ACL gate — `commit_id` IS its bound SHA, and a decisive review whose + `commit_id` prefix-matches the current head is a code FAIL exactly as a marker one is. +- **The N=3 FAIL-round count.** `verdict read` resolves the latest verdict; it does not count + rounds. A PR already at 3 FAIL rounds is escalated to a human, **not** an active repair, so the + guard counts the rounds itself (author-gated to write+ collaborators, clustered by >120s gap — + the same round identity write-code uses) and treats a capped PR as fall-through-and-file. ```bash -# is a write-code repair already in flight on this PR? (PR runs only) — mirror write-code Step R1 -# build THIS PR's authorized set from its marker authors holding write+ on the repo (ADR 0055) +# is a write-code repair already in flight on this PR? (PR runs only) — resolve the verdict the +# EXACT way write-code Step R1 does, by delegating each (PR, gate) FAIL-bound-to-head resolution to +# `pipeline-cli verdict read` (ACL author-gate + latest-wins + SHA-staleness, ADR 0055/0058). Resolve +# the CLI once — in-repo-first, published-fallback (ADR 0062/0064; epic #994). +if [ -f packages/pipeline-cli/src/bin.ts ]; then + VERDICT="node packages/pipeline-cli/src/bin.ts verdict" # phoenix-local: the in-repo consolidated bin +else + VERDICT="pnpm dlx @kampus/pipeline-cli@0.2.0 verdict" # foreign install: the published CLI +fi + +# a namespace is an active-repair FAIL iff its latest authorized verdict is FAIL bound to the current +# head — exit 0 from `verdict read … --expect FAIL`. A stale / SHA-less / PASS / none verdict exits +# non-zero, so it is correctly NOT an active repair, matching write-code's no-op on it. +$VERDICT read --pr "$PR" --gate code --expect FAIL >/dev/null 2>&1 && CODE_FAIL=1 || CODE_FAIL=0 +$VERDICT read --pr "$PR" --gate doc --expect FAIL >/dev/null 2>&1 && DOC_FAIL=1 || DOC_FAIL=0 + +# the native CHANGES_REQUESTED review folds into the code namespace (the verb reads only marker +# comments): a decisive review whose commit_id prefix-matches the current head is a code FAIL too. +CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" +REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ + --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] + | sort_by(.submitted_at) | last | {state, sha: .commit_id}') +RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") +[ -n "$RSHA" ] && [ "$RSTATE" = "CHANGES_REQUESTED" ] && case "$CURRENT_HEAD" in "$RSHA"*) CODE_FAIL=1 ;; esac + +# the N=3 repair cap `verdict read` does NOT count: a PR already at 3 FAIL rounds is escalated to a +# human, NOT an active repair. Author-gate the FAIL markers to write+ collaborators (ADR 0055) and +# cluster by >120s gap — the same round identity write-code uses. comments_file=$(mktemp) gh api "repos/$REPO/issues/$PR/comments?per_page=100" > "$comments_file" markerAuthors=$(jq -r '[.[] @@ -242,27 +267,6 @@ while IFS= read -r a; do admin|maintain|write) authorized=$(jq -c --arg a "$a" '. + [$a]' <<<"$authorized") ;; esac done <<<"$markerAuthors" -# the head every verdict must be bound to (ADR 0058) — a FAIL not bound to this is stale -CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" -# latest verdict per namespace, capturing the bound @ (sha=null for a SHA-less legacy marker) -CODE=$(jq -c --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body: (.body // ""), - sha: ((.body // "") | (capture("(?i)^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file") -DOC=$(jq -c --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body: (.body // ""), - sha: ((.body // "") | (capture("(?i)^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file") -# latest decisive native review folds into the code namespace (commit_id IS its bound SHA, no ACL gate) -REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ - --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}') -# repair cap: a PR already at N=3 FAIL rounds is escalated to a human, NOT an active repair — -# route it to report like any defect (cluster FAILs by >120s gap, same identity write-code uses) ROUNDS=$(jq --argjson authorized "$authorized" \ '[.[] | select(.user.login | IN($authorized[])) | select(.body | test("^\\s*\\**\\s*review-(code|doc):\\s*FAIL"; "i")) @@ -274,14 +278,12 @@ ROUNDS=$(jq --argjson authorized "$authorized" \ | .n' "$comments_file") ``` -An active repair is in flight **iff** `ROUNDS < 3` **and** a namespace's latest verdict is a -**FAIL bound to `$CURRENT_HEAD`** — exactly what write-code Step R1 acts on (latest-wins per -namespace; a newer FAIL wins over an older PASS, but its `@ ` must prefix-match the current -head). Concretely: the latest `review-code` marker is FAIL **and** its captured `sha` is a -non-empty prefix of `$CURRENT_HEAD` (or the latest native review is `CHANGES_REQUESTED` with a -`commit_id` that prefix-matches); **or** likewise for the latest `review-doc` marker. A FAIL -whose `sha` is empty/null (legacy, SHA-less) or does not match the current head is **stale** — -write-code no-ops on it, so it is **not** an active repair. When an active repair is in flight, +An active repair is in flight **iff** `ROUNDS < 3` **and** a namespace is a current-head FAIL — +`CODE_FAIL=1` (the latest `review-code` marker is a current-head FAIL per `verdict read`, or the +latest native review is `CHANGES_REQUESTED` at the current head) **or** `DOC_FAIL=1` — exactly +what write-code Step R1 acts on. A FAIL that is **stale** (SHA-less, or bound to an old head) +exits `verdict read` non-zero, so it is correctly **not** an active repair — write-code no-ops on +it, so the defect falls through and files. When an active repair is in flight, **do not file a defect.** Drop a one-line comment on the PR pointing at the red run — consistent with the `Filed #N` comment the no-repair path posts, but routed to the in-flight repair instead of a fresh issue — and stop. That comment *is* your one routed action for this invocation: diff --git a/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md b/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md index c34c283cc..b9629b28d 100644 --- a/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md @@ -695,12 +695,18 @@ author cannot widen it via a file in their own diff. The solo operator `usirin` ADR 0048) holds `admin` and passes; any future operator or review-bot earns standing by being a `write+` collaborator, with no edit to this skill. +For the **marker namespaces** that author-gate is applied *inside* `pipeline-cli verdict read` +(Step 2) — it is the verb's own ADR-0055 trust root, not re-derived here. The set below is still +resolved explicitly because the **§CP advisory** resolution (Step 2.§CP) needs it: an advisory is +SHA-less in its first line by design (ADR 0111), so `verdict read` resolves that namespace to +`none` and cannot author-gate it — the advisory's own latest-wins/author-gated pick is ship-it's. + Resolve the authorized-author set from the ACL — every distinct marker author whose repo permission is `write` / `maintain` / `admin`. This fails closed: a lookup error or a `read`/`triage` author never enters the set, so their marker is ignored exactly as an off-list author was under 0051. When *no* author clears the bar, `authorized` stays `[]` -and `IN($authorized[])` below matches nothing — every namespace resolves to `null`, i.e. -`unverified` → refuse — so the empty set is the safe terminal state, not an open door. +and no advisory resolves — `unverified` → refuse — so the empty set is the safe terminal +state, not an open door (the same fail-closed terminal `verdict read` applies to markers). ```bash comments_file=$(mktemp) @@ -722,58 +728,58 @@ while IFS= read -r a; do done <<<"$markerAuthors" ``` -Read the latest of each form (sorted by timestamp, newest last — don't lean on the API's -return order for a merge decision). The author gate (`IN($authorized[])`) runs *before* -`sort_by | last`, so a forged newer marker from an unauthorized author can't shadow a real -older verdict: +Resolve the latest current-head marker verdict per namespace **through `pipeline-cli verdict read`**: +the verb folds the ADR-0055 write+ author-gate (a forged newer marker from an unauthorized author +can't shadow a real verdict), the latest-wins pick, and the ADR-0058 SHA-staleness refusal (Step 2b) +into one exit code — its unit tests are the contract (#2102), the same resolution `write-code` reads. +The native decisive review folds into the code namespace separately (the verb reads marker comments, +not reviews); Step 2b keeps `is_current` for it and for the §CP advisory body-SHA: ```bash # the PR's CURRENT head SHA — the head every verdict must be bound to (ADR 0058) CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" # latest decisive native review (APPROVED / CHANGES_REQUESTED) — the review-code path only. -# GitHub author-attributes reviews, so this path is unforgeable and needs no ACL check. -# Carry .commit_id: it IS the SHA the reviewer approved, so Step 2b applies the same staleness -# test to a native review as to a marker's @ . -gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ +# GitHub author-attributes reviews, so this path is unforgeable and needs no ACL check. commit_id IS +# the SHA the reviewer approved, so the same staleness test applies to it as to a marker's @ . +REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}' - -# latest review-code marker comment (code namespace) — author-gated, anchored, never matches review-doc. -# Capture the bound head SHA from the @ tail; a SHA-less legacy marker yields sha=null → Step 2b refuses. -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" - -# latest review-doc marker comment (doc namespace) — author-gated, anchored, never matches review-code/review-skill -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" - -# latest review-skill marker comment (skill namespace) — author-gated, anchored, never matches review-code/review-doc -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-skill:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-skill:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" - -# latest review-design marker comment (design namespace) — author-gated, anchored, never matches review-code/review-doc/review-skill -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-design:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-design:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" + | sort_by(.submitted_at) | last | {state, sha: .commit_id}') + +# resolve the verdict CLI once — in-repo-first, published-fallback (ADR 0062/0064; epic #994) +if [ -f packages/pipeline-cli/src/bin.ts ]; then + VERDICT="node packages/pipeline-cli/src/bin.ts verdict" # phoenix-local: the in-repo consolidated bin +else + VERDICT="pnpm dlx @kampus/pipeline-cli@0.2.0 verdict" # foreign install: the published CLI +fi + +# per present namespace (Step 0), resolve the marker verdict against the current head via the verb: +# _PASS=1 iff a current-head PASS marker in that gate; _FAIL=1 iff a current-head FAIL (the +# veto). A stale / SHA-less / none verdict exits non-zero on BOTH, so it is neither a PASS nor a FAIL +# — Step 2b's `unverified (verdict not bound to current head)` refusal, now owned by the verb. (A §CP +# advisory namespace is SHA-less by design and resolves `none` here — Step 2.§CP handles it from the +# body's Reviewed-head instead.) +$VERDICT read --pr "$PR" --gate code --expect PASS >/dev/null 2>&1 && CODE_PASS=1 || CODE_PASS=0 +$VERDICT read --pr "$PR" --gate code --expect FAIL >/dev/null 2>&1 && CODE_FAIL=1 || CODE_FAIL=0 +$VERDICT read --pr "$PR" --gate doc --expect PASS >/dev/null 2>&1 && DOC_PASS=1 || DOC_PASS=0 +$VERDICT read --pr "$PR" --gate doc --expect FAIL >/dev/null 2>&1 && DOC_FAIL=1 || DOC_FAIL=0 +$VERDICT read --pr "$PR" --gate skill --expect PASS >/dev/null 2>&1 && SKILL_PASS=1 || SKILL_PASS=0 +$VERDICT read --pr "$PR" --gate skill --expect FAIL >/dev/null 2>&1 && SKILL_FAIL=1 || SKILL_FAIL=0 +$VERDICT read --pr "$PR" --gate design --expect PASS >/dev/null 2>&1 && DESIGN_PASS=1 || DESIGN_PASS=0 +$VERDICT read --pr "$PR" --gate design --expect FAIL >/dev/null 2>&1 && DESIGN_FAIL=1 || DESIGN_FAIL=0 + +# fold the native decisive review into the code namespace (the verb reads only marker comments). Only +# a review bound to the current head counts (same ADR-0058 staleness as a marker's @ ). A +# current-head CHANGES_REQUESTED is a code FAIL (veto); a current-head APPROVED is a code PASS-equiv. +RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") +if [ -n "$RSHA" ]; then case "$CURRENT_HEAD" in "$RSHA"*) + [ "$RSTATE" = "CHANGES_REQUESTED" ] && { CODE_FAIL=1; CODE_PASS=0; } + [ "$RSTATE" = "APPROVED" ] && [ "$CODE_FAIL" = 0 ] && CODE_PASS=1 +;; esac; fi ``` -Now resolve **per namespace**, latest-wins by timestamp: +Now resolve **per namespace** (the marker verdict from the verb above, the native review + §CP +advisory folded in): - **review-code namespace** — the verdict is the **newest of {latest decisive review, latest review-code marker comment}** by timestamp (review `submitted_at` vs comment `created_at`). @@ -829,31 +835,33 @@ Now resolve **per namespace**, latest-wins by timestamp: Each resolved verdict carries a bound SHA. A verdict authorizes a merge **only if it is bound to the PR's current head** — this is what closes the masking race (a slower PASS bound to an older head can never outrank a real FAIL on the live head) and the head-moved race (a PASS -bound to `X1` can never be consumed against `X2`). For each namespace's resolved verdict: +bound to `X1` can never be consumed against `X2`). The rule, per namespace: -- **No bound SHA** (`sha == null` — a pre-0058 SHA-less marker) → `unverified (verdict not - bound to current head)` → refuse. +- **No bound SHA** (a pre-0058 SHA-less marker) → `unverified (verdict not bound to current + head)` → refuse. - **Bound SHA ≠ current head** (neither is a prefix of the other — either may be abbreviated, so compare by prefix-match against `$CURRENT_HEAD`) → `unverified (verdict not bound to current head)` → refuse. - **Bound SHA prefix-matches `$CURRENT_HEAD`** → the verdict is current; its polarity decides in the guard below. +**For the marker namespaces this refusal is now enforced *inside* `pipeline-cli verdict read`** +(Step 2): a marker that is SHA-less or bound to an older head resolves `sha-less`/`stale`, so the +verb exits non-zero on **both** `--expect PASS` and `--expect FAIL` — leaving `_PASS=0` **and** +`_FAIL=0`, which is exactly `unverified (verdict not bound to current head)` → refuse. There is +no separate marker staleness test to run here, and none to keep in sync (#2102). + +Two signals `verdict read` does **not** resolve still need the test applied explicitly, so the +helper stays: the **native review**'s `commit_id` (folded into the code namespace in Step 2) and the +**§CP advisory**'s body `Reviewed-head` SHA (Step 2.§CP). + ```bash -# is verdict SHA $vsha bound to the current head? (prefix-match, either side may be abbreviated) -# Empty/absent $vsha MUST short-circuit to refuse FIRST: a jq `sha: null` reaches the shell as -# an empty string, and an unguarded `case "$CURRENT_HEAD" in ""*)` reduces to the glob `*` — which -# matches any head and would falsely report a legacy SHA-less marker as current (ADR 0058 rule 3). +# is a bound SHA $1 the current head? (prefix-match, either side may be abbreviated) — applied to the +# native review's commit_id and the §CP advisory's body Reviewed-head SHA; the marker namespaces are +# already head-bound by `verdict read`. Empty/absent $1 MUST short-circuit to refuse FIRST: an +# unguarded `case "$CURRENT_HEAD" in ""*)` reduces to the glob `*` — which matches any head and would +# falsely report a SHA-less marker as current (ADR 0058 rule 3). is_current () { [ -n "$1" ] || return 1; case "$CURRENT_HEAD" in "$1"*) return 0;; esac; case "$1" in "$CURRENT_HEAD"*) return 0;; esac; return 1; } - -# Extract each resolved verdict's bound SHA into a shell var — the load-bearing normalization: -# `// empty` renders a jq `sha: null` (a pre-0058 SHA-less / absent marker) as "" (NOT the literal -# "null"), so is_current's `[ -n "$1" ] || return 1` short-circuits to refuse exactly as designed. -# $verdict is the per-namespace resolved object emitted above ({state|body, at, sha}). -vsha="$(jq -r '.sha // empty' <<<"$verdict")" -is_current "$vsha" || echo "unverified (verdict not bound to current head) → refuse" -# null/empty $vsha → not current (legacy marker) → refuse. A jq `sha: null` must reach this helper -# as an empty string (or be short-circuited to refuse before the call) — never as the literal "null". ``` diff --git a/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md b/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md index aa1a24dae..232044a30 100644 --- a/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md @@ -162,19 +162,26 @@ namespaces) is an unaddressed FAIL: ```bash ME=$(gh api user --jq '.login') +# resolve the verdict CLI once — in-repo-first, published-fallback (ADR 0062/0064; epic #994). +# Each per-(PR, gate) FAIL-bound-to-head resolution below delegates to `pipeline-cli verdict read` +# (ACL author-gate + latest-wins + SHA-staleness, ADR 0055/0058; its unit tests are the contract). +if [ -f packages/pipeline-cli/src/bin.ts ]; then + VERDICT="node packages/pipeline-cli/src/bin.ts verdict" # phoenix-local: the in-repo consolidated bin +else + VERDICT="pnpm dlx @kampus/pipeline-cli@0.2.0 verdict" # foreign install: the published CLI +fi # open PRs you authored; print each one whose latest verdict in EITHER namespace is FAIL, # UNLESS it has already hit the N=3 repair cap (then it's a human's, not yours to re-pick) gh api "repos/$REPO/pulls?state=open&per_page=100" \ --jq ".[] | select(.user.login==\"$ME\") | .number" | while read PR; do - # whose markers count as a verdict — GitHub's repo ACL, the same trust root ship-it Step 2 - # uses (ADR 0055, supersedes 0051): build THIS PR's authorized set from its marker authors - # holding write+ on the repo, so a forged review-(code|doc|skill): FAIL from a non-reviewer can't - # trigger spurious repair. Empty set ⇒ IN($authorized[]) matches nothing ⇒ no verdict - # resolves ⇒ the scan safely finds nothing — fail-closed. + # The N=3 FAIL-round count is the one thing `verdict read` does NOT do (it resolves the latest + # verdict, it does not count rounds) — genuinely more than a single (PR, gate) resolution, so it + # stays inline. Author-gate the FAIL markers to write+ collaborators (ADR 0055, supersedes 0051) + # so a forged review-(code|doc|skill): FAIL can't inflate the count; an empty authorized set counts + # zero rounds — fail-closed. Cluster by timestamp gap (>120s = new round), per fix-round not per + # marker (a both-namespace round counts once), the same identity the Bounding count uses. comments_file=$(mktemp) gh api "repos/$REPO/issues/$PR/comments?per_page=100" > "$comments_file" - # every marker test below is emphasis-tolerant (leading \** absorbs review-code's bolding) - # per gh-issue-intake-formats.md §5 — the canonical matcher contract markerAuthors=$(jq -r '[.[] | select(.body | test("^\\s*\\**\\s*review-(code|doc|skill):\\s*(PASS|FAIL)"; "i")) | .user.login] | unique | .[]' "$comments_file") @@ -186,8 +193,6 @@ gh api "repos/$REPO/pulls?state=open&per_page=100" \ admin|maintain|write) authorized=$(jq -c --arg a "$a" '. + [$a]' <<<"$authorized") ;; esac done <<<"$markerAuthors" - # FAIL rounds already accrued — per fix-round, not per marker (a both-namespace round counts once); - # cluster by timestamp gap (>120s = new round), same identity as the Bounding count, never a minute bucket ROUNDS=$(jq --argjson authorized "$authorized" \ '[.[] | select(.user.login | IN($authorized[])) | select(.body | test("^\\s*\\**\\s*review-(code|doc|skill):\\s*FAIL"; "i")) @@ -198,31 +203,21 @@ gh api "repos/$REPO/pulls?state=open&per_page=100" \ then {n:(.n+1), prev:$t} else {n:.n, prev:$t} end) | .n' "$comments_file") [ "$ROUNDS" -ge 3 ] && continue # at the cap → already escalated to a human, excluded from the scan - CODE=$(jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last | .body // ""' "$comments_file") - DOC=$(jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last | .body // ""' "$comments_file") - SKILL=$(jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-skill:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last | .body // ""' "$comments_file") - echo "$CODE" | grep -qiE '^\s*\**\s*review-code:\s*FAIL' && echo "#$PR review-code FAIL" - echo "$DOC" | grep -qiE '^\s*\**\s*review-doc:\s*FAIL' && echo "#$PR review-doc FAIL" - echo "$SKILL" | grep -qiE '^\s*\**\s*review-skill:\s*FAIL' && echo "#$PR review-skill FAIL" + # resolve each namespace's latest current-head verdict through the shared verb — exit 0 iff HEAD + # carries a current FAIL in that gate (a stale / SHA-less / PASS / none verdict exits non-zero). + $VERDICT read --pr "$PR" --gate code --expect FAIL >/dev/null 2>&1 && echo "#$PR review-code FAIL" + $VERDICT read --pr "$PR" --gate doc --expect FAIL >/dev/null 2>&1 && echo "#$PR review-doc FAIL" + $VERDICT read --pr "$PR" --gate skill --expect FAIL >/dev/null 2>&1 && echo "#$PR review-skill FAIL" done ``` If such a PR exists, **repair it instead of picking new work** — go to [Repair mode](#repair-mode--consume-a-gate-fail-verdict-fix-and-resubmit) with that PR number. Only once you have **no** PR with an unaddressed latest FAIL do you fall through to -the normal pick below. (This scan is a coarse *signal* that deliberately matches the SHA-less -prefix; repair mode Step R1 re-resolves the verdict authoritatively per namespace **and -applies the SHA-staleness test** (ADR 0058) before acting, so a PR that flipped to PASS, or -whose FAIL is bound to a now-stale head, between the scan and the repair is a clean no-op.) +the normal pick below. (This scan resolves each (PR, gate) verdict through the **same +authoritative SHA-bound `pipeline-cli verdict read`** repair mode Step R1 uses; R1 still +re-resolves at repair time because the head may move between this scan and the repair — a PR +that flipped to PASS, or whose FAIL went stale in that window, is then a clean no-op.) Two properties make this scan terminate rather than starve: @@ -1695,22 +1690,32 @@ fixing its own PR and an independent gate re-judging it is the firewall, intact. ### Step R1 — Resolve the latest verdict per namespace (mirror `ship-it` Step 2) Do **not** act on the presence of any FAIL that ever existed. Resolve `review-code`, -`review-doc`, and `review-skill` in **separate namespaces** — three anchored regexes that never -cross-match — and take the **latest by timestamp** in each. This mirrors `ship-it` Step 2's -resolution exactly (the reading side of the same contract), **including its ACL author-gate**: -a marker comment counts as a verdict only from a `write+` repo collaborator, so a -self-authored or forged `review-(code|doc|skill): FAIL` is invisible (ADR -[0055](https://github.com/kamp-us/phoenix/blob/main/.decisions/0055-acl-sourced-review-authz.md)). The native-review path needs -no ACL gate — GitHub author-attributes reviews, so it is unforgeable. +`review-doc`, and `review-skill` in **separate namespaces** and take the **latest current-head +verdict** in each — the exact resolution `ship-it` Step 2 reads. That resolution (the ADR-0055 +write+ author-gate so a self-authored or forged `review-(code|doc|skill): FAIL` is invisible, the +latest-wins pick, and the ADR-0058 SHA-staleness refusal) is owned by `pipeline-cli verdict read`, +so R1 delegates to the verb rather than re-deriving it (#2102) — its unit tests are the contract. +The native `CHANGES_REQUESTED` review that folds into the code namespace is the one thing the verb +does not resolve (it reads only marker comments); it needs no ACL gate — GitHub author-attributes +reviews, so that path is unforgeable — so R1 keeps just that fold inline. ```bash PR= -# whose markers count as a verdict — GitHub's repo ACL, the same trust root ship-it Step 2 uses -# (ADR 0055): build the authorized set from THIS PR's marker authors holding write+ on the repo. +# resolve the verdict CLI once — in-repo-first, published-fallback (ADR 0062/0064; epic #994). +# The per-(PR, gate) FAIL-bound-to-head resolution delegates to `pipeline-cli verdict read`: the +# ADR-0055 write+ author-gate, the latest-wins pick, and the ADR-0058 SHA-staleness test folded into +# one exit code (its unit tests are the contract, #2102) — the same resolution ship-it Step 2 reads. +if [ -f packages/pipeline-cli/src/bin.ts ]; then + VERDICT="node packages/pipeline-cli/src/bin.ts verdict" # phoenix-local: the in-repo consolidated bin +else + VERDICT="pnpm dlx @kampus/pipeline-cli@0.2.0 verdict" # foreign install: the published CLI +fi + +# The write+ author-set (ADR 0055) — `verdict read` computes it internally for the marker resolution, +# but two DOWNSTREAM steps that are genuinely more than a single (PR, gate) resolution reuse it: the +# inline-review-comment gate (Step R2) and the repair-mode N=3 FAIL-round count. Build it once here. comments_file=$(mktemp) gh api "repos/$REPO/issues/$PR/comments?per_page=100" > "$comments_file" -# every marker test below is emphasis-tolerant (leading \** absorbs review-code's bolding) -# per gh-issue-intake-formats.md §5 — the canonical matcher contract markerAuthors=$(jq -r '[.[] | select(.body | test("^\\s*\\**\\s*review-(code|doc|skill):\\s*(PASS|FAIL)"; "i")) | .user.login] | unique | .[]' "$comments_file") @@ -1723,65 +1728,36 @@ while IFS= read -r a; do esac done <<<"$markerAuthors" -# the PR's CURRENT head SHA — the head every verdict must be bound to (ADR 0058) +# a namespace is a repairable FAIL iff its latest authorized verdict is FAIL bound to the current head +# — exit 0 from `verdict read … --expect FAIL`. The verb's JSON (stdout) carries the resolving comment +# id, which Step R2 uses to read the FAIL body. A stale / SHA-less / PASS / none verdict exits non-zero, +# so it is correctly NOT repaired (idempotent no-op), needing no separate staleness test here. +CODE_FAIL_JSON="$($VERDICT read --pr "$PR" --gate code --expect FAIL 2>/dev/null)" && CODE_FAIL=1 || CODE_FAIL=0 +DOC_FAIL_JSON="$($VERDICT read --pr "$PR" --gate doc --expect FAIL 2>/dev/null)" && DOC_FAIL=1 || DOC_FAIL=0 +SKILL_FAIL_JSON="$($VERDICT read --pr "$PR" --gate skill --expect FAIL 2>/dev/null)" && SKILL_FAIL=1 || SKILL_FAIL=0 + +# the native CHANGES_REQUESTED review folds into the code namespace (the verb reads only marker +# comments): GitHub author-attributes reviews, so this path needs no ACL gate — commit_id IS its bound +# SHA, and a decisive review that is CHANGES_REQUESTED and prefix-matches the current head is a code FAIL too. CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" - -# latest review-code marker (code namespace) — author-gated, anchored, never matches review-doc. -# Capture the bound head SHA from the @ tail (sha=null for a pre-0058 SHA-less marker). -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-code:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" - -# latest decisive native review (APPROVED / CHANGES_REQUESTED) — folds into the code namespace -# (no ACL gate: GitHub author-attributes reviews, so this path is unforgeable). commit_id IS its bound SHA. -gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ +REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}' - -# latest review-doc marker (doc namespace) — author-gated, anchored, never matches review-code/review-skill -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-doc:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" - -# latest review-skill marker (skill namespace) — author-gated, anchored, never matches review-code/review-doc -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-skill:\\s*(PASS|FAIL)"; "i"))] - | sort_by(.created_at) | last - | {body, at: .created_at, - sha: (.body // "" | (capture("(?i)^\\s*\\**\\s*review-skill:\\s*(PASS|FAIL)\\s*@\\s*(?[0-9a-f]{7,40})") // {s:null}).s)}' "$comments_file" + | sort_by(.submitted_at) | last | {state, sha: .commit_id}') +RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") +[ -n "$RSHA" ] && [ "$RSTATE" = "CHANGES_REQUESTED" ] && case "$CURRENT_HEAD" in "$RSHA"*) CODE_FAIL=1 ;; esac ``` -Resolve per namespace, latest-wins by timestamp, **then apply the SHA-staleness test** (ADR -[0058](https://github.com/kamp-us/phoenix/blob/main/.decisions/0058-sha-bound-verdict-contract.md), mirroring `ship-it` Step 2b): - -- **review-code namespace** — the verdict is the **newest of {latest decisive review, - latest review-code marker}**; its bound SHA is the marker's `@ ` (or the review's - `commit_id`). `CHANGES_REQUESTED` or `review-code: FAIL` is FAIL; `APPROVED` or - `review-code: PASS` is PASS. -- **review-doc namespace** — the verdict is the **latest `review-doc` marker** by - `created_at` (review-doc is comment-only — no native review). `review-doc: FAIL` is FAIL. -- **review-skill namespace** — the verdict is the **latest `review-skill` marker** by - `created_at` (review-skill is comment-only — no native review). `review-skill: FAIL` is FAIL. - A `review-skill: advisory` line (a blocking-set skill PR) carries no `@ ` and is **not** - a FAIL — it judges nothing to repair, so it's a clean no-op like a PASS. - -**Act only when a namespace's latest verdict is FAIL *bound to the current head*.** A newer -FAIL is acted on even if an older PASS exists — but a FAIL whose `@ ` is **not** the PR's -current head (`$CURRENT_HEAD`, by prefix-match either way), or that carries **no** `@ ` -(a pre-0058 legacy marker), is **stale**: it judges code that has since changed, so do **not** -repair on it — report `nothing to repair (latest FAIL not bound to current head)` and stop. -A PR whose latest current-head verdict is PASS — or that has no current-head FAIL at all — is -**not repaired**. This keeps repair mode **idempotent**: re-running it on an already-fixed/PASS -PR, a no-FAIL PR, or a stale-FAIL PR is a clean no-op. If **more than one** namespace's latest -current-head verdict is FAIL (a mixed PR — e.g. code+doc, or skill+code), address **all** of -them in this round. +**Act only when a namespace's latest verdict is FAIL *bound to the current head*** — i.e. `CODE_FAIL=1` +(a current-head `review-code: FAIL` marker per `verdict read`, or a current-head `CHANGES_REQUESTED` +review), `DOC_FAIL=1`, or `SKILL_FAIL=1`. `verdict read` already encapsulates latest-wins **and** the +SHA-staleness refusal: a newer FAIL is acted on even if an older PASS exists, but a FAIL whose `@ ` +is not the current head (or carries no `@ ` — a pre-0058 legacy marker) resolves `stale`/`sha-less`, +exits non-zero, and is **not** repaired — report `nothing to repair (latest FAIL not bound to current +head)` and stop. A `review-skill: advisory` line (a blocking-set skill PR) is not a FAIL — `verdict read` +resolves it `none` (no PASS/FAIL polarity), a clean no-op like a PASS. This keeps repair mode +**idempotent**: re-running it on an already-fixed/PASS PR, a no-FAIL PR, or a stale-FAIL PR is a clean +no-op. If **more than one** namespace resolves FAIL (a mixed PR — e.g. code+doc, or skill+code), address +**all** of them in this round. R1 resolves the **AC gate** — the marker (and the decisive native review folded into the code namespace) is what decides whether there's anything to repair, and its `[FAIL]` table @@ -1800,13 +1776,14 @@ enumerated findings as the AC work list** — fix exactly what they name (the in below are additive to this list, not a substitute for it): ```bash -# the full body of the latest FAILing review-code marker (swap review-code→review-doc/review-skill per namespace) -# author-gated against the ACL-derived $authorized set R1 already built — only a real reviewer's findings are your work list -# marker test stays emphasis-tolerant (leading \** absorbs review-code's bolding) per gh-issue-intake-formats.md §5 -jq --argjson authorized "$authorized" \ - '[.[] | select(.user.login | IN($authorized[])) - | select(.body | test("^\\s*\\**\\s*review-code:\\s*FAIL"; "i"))] - | sort_by(.created_at) | last | .body' "$comments_file" +# the full body of the resolving FAIL marker — its comment id came from R1's `verdict read` JSON +# (swap CODE_FAIL_JSON→DOC_FAIL_JSON/SKILL_FAIL_JSON per namespace). `verdict read` already +# author-gated and latest-wins-resolved that exact comment, so this is a plain body fetch of it — +# no re-derivation of the resolver. (When the code namespace was resolved via a native +# CHANGES_REQUESTED review rather than a marker, read the review body from +# `repos/$REPO/pulls/$PR/reviews` instead — a native review carries no issue-comment id.) +CID=$(jq -r '.commentId // empty' <<<"$CODE_FAIL_JSON") +[ -n "$CID" ] && gh api "repos/$REPO/issues/comments/$CID" --jq '.body' ``` #### A review-appended AC is an ordinary `[FAIL]` row — no special parser (ADR 0079) diff --git a/packages/pipeline-cli/src/tools/adoption-lint/manifest.ts b/packages/pipeline-cli/src/tools/adoption-lint/manifest.ts index 377d7b330..15c740eef 100644 --- a/packages/pipeline-cli/src/tools/adoption-lint/manifest.ts +++ b/packages/pipeline-cli/src/tools/adoption-lint/manifest.ts @@ -188,11 +188,13 @@ export const DECISIONS: ReadonlyArray = [ * * `drive-issue.js` is the sole legitimate mirror: the orchestrator is a runtime * `.js` surface that cannot import the TS verb core, so it must inline what a - * skill would cite. `heal-ci` and `write-code` are the two current `verdict read` - * re-derivers named in #3254 — their conversion is a non-drop-in rewrite (both scans - * do more than a single (PR, gate) resolution: write-code also counts FAIL rounds for - * the N=3 repair cap), so #3265 deferred it to its own reviewable PR, tracked by #3619; - * they stay grandfathered here until that migration lands and draws them down. + * skill would cite. The `heal-ci` / `write-code` `verdict read` grandfathers named + * in #3254 are gone: #3686 migrated ship-it / write-code / heal-ci to cite + * `pipeline-cli verdict read` same-commit (each scan keeps only the residue that is + * genuinely more than a single (PR, gate) resolution — write-code's N=3 FAIL-round + * count, the multi-PR pre-pick scan, ship-it's native-review fold + §CP advisory), + * drawing the `verdict read` grandfather pile to zero. The lint now forbids re-growing + * it. The mirror is the only surviving exemption. */ export const EXEMPTIONS: ReadonlyArray = [ { @@ -201,18 +203,4 @@ export const EXEMPTIONS: ReadonlyArray = [ reason: "the orchestrator is a runtime .js surface that cannot import the TS verb core — it mirrors by necessity (#3254; the sole legitimate mirror)", }, - { - kind: "grandfathered", - path: "skills/heal-ci/SKILL.md", - verb: "verdict read", - reason: - "existing `verdict read` re-derivation — migrate to `pipeline-cli verdict read` (deferred by #3265 to its own PR, #3619)", - }, - { - kind: "grandfathered", - path: "skills/write-code/SKILL.md", - verb: "verdict read", - reason: - "existing `verdict read` re-derivation — migrate to `pipeline-cli verdict read` (deferred by #3265 to its own PR, #3619)", - }, ]; From c0c55216f5941d13289920a9b3be103d83663c0c Mon Sep 17 00:00:00 2001 From: usirin Date: Sun, 19 Jul 2026 18:17:57 -0700 Subject: [PATCH 2/2] fix(pipeline): restore newest-wins verdict resolution in the native-review fold (#3686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `verdict read` adoption refactor replaced the code-namespace fold with FAIL-precedence (`CHANGES_REQUESTED` unconditionally vetoing, `APPROVED` only counting when no FAIL stood) and dropped the timestamps newest-wins needs — `REVIEW`'s `at: .submitted_at` and the marker objects' `created_at`. That contradicted the unchanged prose in the same sections (ship-it Step 2's "the newest of {latest decisive review, latest review-code marker comment} by timestamp" and "an old FAIL behind a newer PASS does not block") and #3686's "no behavioral change to verdict resolution" AC. It is also a live wedge: a PR FAIL'd, repaired, and re-PASSed at the same head would stay permanently blocked by the superseded FAIL, breaking the repair -> re-review -> ship loop. The prose is the source of truth, so the code now matches it: - ship-it Step 2, write-code R1, heal-ci: `REVIEW` carries `at: .submitted_at` again, and the fold compares it against the resolving marker's `created_at` (resolved from the comment id `verdict read` prints, since the verb's outcome carries no timestamp). Newest event wins; a newer APPROVED clears an older FAIL, a newer marker stands over an older review. - write-code R1 / heal-ci additionally set CODE_FAIL back to 0 on a newer APPROVED, instead of only ever raising it. Also narrows the transient-5xx fail-open the reviewer raised as non-blocking: `verdict read` prints its outcome JSON on both exit paths, so absent JSON means the namespace never resolved (a transport error, not a verdict). write-code R1 and heal-ci now flag that as VERDICT_UNKNOWN and defer rather than silently read it as "no FAIL" and skip a real repair. ship-it needs no change there — an unresolved namespace already leaves both PASS and FAIL at 0, i.e. `unverified` -> refuse. Everything else the refactor achieved is preserved: the three inline verdict resolvers stay deleted and cite `pipeline-cli verdict read`, the grandfathered exemptions stay at zero, the N=3 repair caps and multi-PR scan stay, and ship-it's §CP advisory resolution is untouched. --- .../kampus-pipeline/skills/heal-ci/SKILL.md | 47 +++++++++++++---- .../kampus-pipeline/skills/ship-it/SKILL.md | 33 +++++++++--- .../skills/write-code/SKILL.md | 52 ++++++++++++++++--- 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md b/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md index 05329a2d1..7b12228be 100644 --- a/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/heal-ci/SKILL.md @@ -216,10 +216,12 @@ are the contract, #2102). So `heal-ci` reads each namespace **through the verb** re-deriving the resolver write-code once hand-copied, and keeps only the two things the verb does **not** do — genuinely more than a single (PR, gate) resolution, so they stay here: -- **The native `CHANGES_REQUESTED` review that folds into the code namespace.** The verb reads - marker comments; a native review is a *different* record type. GitHub author-attributes reviews, - so this path needs **no** ACL gate — `commit_id` IS its bound SHA, and a decisive review whose - `commit_id` prefix-matches the current head is a code FAIL exactly as a marker one is. +- **The native decisive review that folds into the code namespace.** The verb reads marker comments; + a native review is a *different* record type. GitHub author-attributes reviews, so this path needs + **no** ACL gate — `commit_id` IS its bound SHA. The fold is **newest-wins by timestamp** (as in + `ship-it` Step 2 / write-code R1): the code verdict is the newest of {latest decisive review, + latest `review-code` marker}, so a current-head `CHANGES_REQUESTED` is a code FAIL *unless* a newer + current-head marker already PASS'd. - **The N=3 FAIL-round count.** `verdict read` resolves the latest verdict; it does not count rounds. A PR already at 3 FAIL rounds is escalated to a human, **not** an active repair, so the guard counts the rounds itself (author-gated to write+ collaborators, clustered by >120s gap — @@ -239,17 +241,36 @@ fi # a namespace is an active-repair FAIL iff its latest authorized verdict is FAIL bound to the current # head — exit 0 from `verdict read … --expect FAIL`. A stale / SHA-less / PASS / none verdict exits # non-zero, so it is correctly NOT an active repair, matching write-code's no-op on it. -$VERDICT read --pr "$PR" --gate code --expect FAIL >/dev/null 2>&1 && CODE_FAIL=1 || CODE_FAIL=0 -$VERDICT read --pr "$PR" --gate doc --expect FAIL >/dev/null 2>&1 && DOC_FAIL=1 || DOC_FAIL=0 - -# the native CHANGES_REQUESTED review folds into the code namespace (the verb reads only marker -# comments): a decisive review whose commit_id prefix-matches the current head is a code FAIL too. +CODE_FAIL_JSON="$($VERDICT read --pr "$PR" --gate code --expect FAIL 2>/dev/null)" && CODE_FAIL=1 || CODE_FAIL=0 +DOC_FAIL_JSON="$($VERDICT read --pr "$PR" --gate doc --expect FAIL 2>/dev/null)" && DOC_FAIL=1 || DOC_FAIL=0 + +# UNRESOLVED ≠ "no FAIL". The verb prints its outcome JSON on BOTH exit paths, so absent JSON means the +# namespace never resolved (a transport/5xx failure). Reading that as "no repair in flight" would file +# a twin defect against a live repair — so treat it as UNKNOWN and defer this invocation. +VERDICT_UNKNOWN=0 +for J in "$CODE_FAIL_JSON" "$DOC_FAIL_JSON"; do jq -e . >/dev/null 2>&1 <<<"$J" || VERDICT_UNKNOWN=1; done + +# the native decisive review folds into the code namespace (the verb reads only marker comments), by +# NEWEST-WINS timestamp — the same fold ship-it Step 2 / write-code R1 run. `at: .submitted_at` is what +# the compare reads; a bare "CHANGES_REQUESTED ⇒ CODE_FAIL=1" would report a repair in flight on a PR +# whose newer marker already PASS'd at the same head, suppressing a defect that should be filed. CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id}') + | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}') RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") -[ -n "$RSHA" ] && [ "$RSTATE" = "CHANGES_REQUESTED" ] && case "$CURRENT_HEAD" in "$RSHA"*) CODE_FAIL=1 ;; esac +RAT=$(jq -r '.at // ""' <<<"$REVIEW") +# `_tag == "current"` is exactly "a current-head marker verdict stands"; its comment id yields the +# created_at the compare needs (the verb's outcome carries no timestamp). +MARKER_AT="" +[ "$(jq -r '._tag // ""' <<<"$CODE_FAIL_JSON" 2>/dev/null)" = "current" ] && + MARKER_AT=$(gh api "repos/$REPO/issues/comments/$(jq -r .commentId <<<"$CODE_FAIL_JSON")" --jq .created_at 2>/dev/null) +if [ -n "$RSHA" ] && [ -n "$RAT" ]; then case "$CURRENT_HEAD" in "$RSHA"*) + # ISO-8601-UTC sorts lexically, so `>` IS the chronological compare. + if [ -z "$MARKER_AT" ] || [ "$RAT" \> "$MARKER_AT" ]; then + [ "$RSTATE" = "CHANGES_REQUESTED" ] && CODE_FAIL=1 || CODE_FAIL=0 + fi +;; esac; fi # the N=3 repair cap `verdict read` does NOT count: a PR already at 3 FAIL rounds is escalated to a # human, NOT an active repair. Author-gate the FAIL markers to write+ collaborators (ADR 0055) and @@ -278,6 +299,10 @@ ROUNDS=$(jq --argjson authorized "$authorized" \ | .n' "$comments_file") ``` +If `VERDICT_UNKNOWN=1` the verdict never resolved (a GitHub read failure, not a verdict) — **defer +this invocation** rather than assume no repair is in flight; filing a twin against a live repair is +the exact harm this check guards. + An active repair is in flight **iff** `ROUNDS < 3` **and** a namespace is a current-head FAIL — `CODE_FAIL=1` (the latest `review-code` marker is a current-head FAIL per `verdict read`, or the latest native review is `CHANGES_REQUESTED` at the current head) **or** `DOC_FAIL=1` — exactly diff --git a/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md b/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md index b9629b28d..4044d0951 100644 --- a/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/ship-it/SKILL.md @@ -742,9 +742,11 @@ CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" # latest decisive native review (APPROVED / CHANGES_REQUESTED) — the review-code path only. # GitHub author-attributes reviews, so this path is unforgeable and needs no ACL check. commit_id IS # the SHA the reviewer approved, so the same staleness test applies to it as to a marker's @ . +# `at: .submitted_at` is load-bearing, not decoration: it is the timestamp the newest-wins fold below +# compares against the marker's created_at. Drop it and the fold degenerates to a precedence rule. REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id}') + | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}') # resolve the verdict CLI once — in-repo-first, published-fallback (ADR 0062/0064; epic #994) if [ -f packages/pipeline-cli/src/bin.ts ]; then @@ -759,7 +761,9 @@ fi # — Step 2b's `unverified (verdict not bound to current head)` refusal, now owned by the verb. (A §CP # advisory namespace is SHA-less by design and resolves `none` here — Step 2.§CP handles it from the # body's Reviewed-head instead.) -$VERDICT read --pr "$PR" --gate code --expect PASS >/dev/null 2>&1 && CODE_PASS=1 || CODE_PASS=0 +# the code namespace keeps the verb's stdout JSON: it names the RESOLVING comment id, which the +# newest-wins fold below turns into the marker's created_at (the verb's outcome carries no timestamp). +CODE_JSON="$($VERDICT read --pr "$PR" --gate code --expect PASS 2>/dev/null)" && CODE_PASS=1 || CODE_PASS=0 $VERDICT read --pr "$PR" --gate code --expect FAIL >/dev/null 2>&1 && CODE_FAIL=1 || CODE_FAIL=0 $VERDICT read --pr "$PR" --gate doc --expect PASS >/dev/null 2>&1 && DOC_PASS=1 || DOC_PASS=0 $VERDICT read --pr "$PR" --gate doc --expect FAIL >/dev/null 2>&1 && DOC_FAIL=1 || DOC_FAIL=0 @@ -769,12 +773,29 @@ $VERDICT read --pr "$PR" --gate design --expect PASS >/dev/null 2>&1 && DESIGN_P $VERDICT read --pr "$PR" --gate design --expect FAIL >/dev/null 2>&1 && DESIGN_FAIL=1 || DESIGN_FAIL=0 # fold the native decisive review into the code namespace (the verb reads only marker comments). Only -# a review bound to the current head counts (same ADR-0058 staleness as a marker's @ ). A -# current-head CHANGES_REQUESTED is a code FAIL (veto); a current-head APPROVED is a code PASS-equiv. +# a review bound to the current head counts (same ADR-0058 staleness as a marker's @ ). +# +# The fold is NEWEST-WINS by timestamp, never FAIL-precedence — the resolution the prose below states. +# FAIL-precedence would be a live wedge, not a conservative default: a PR FAIL'd, repaired, and +# re-PASSed at the SAME head would stay permanently blocked by the superseded FAIL, breaking the +# repair → re-review → ship loop write-code drives. RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") +RAT=$(jq -r '.at // ""' <<<"$REVIEW") if [ -n "$RSHA" ]; then case "$CURRENT_HEAD" in "$RSHA"*) - [ "$RSTATE" = "CHANGES_REQUESTED" ] && { CODE_FAIL=1; CODE_PASS=0; } - [ "$RSTATE" = "APPROVED" ] && [ "$CODE_FAIL" = 0 ] && CODE_PASS=1 + # the marker's created_at: the verb's outcome carries no timestamp, so resolve it from the comment + # id its JSON names. An empty MARKER_AT means no current-head marker verdict stands (`verdict read` + # already dropped a none/sha-less/stale one) ⇒ the review is the only event ⇒ it decides alone. + MARKER_AT="" + MARKER_ID=$(jq -r '.commentId // empty' <<<"$CODE_JSON" 2>/dev/null) + [ -n "$MARKER_ID" ] && [ "$((CODE_PASS + CODE_FAIL))" -gt 0 ] && + MARKER_AT=$(gh api "repos/$REPO/issues/comments/$MARKER_ID" --jq .created_at 2>/dev/null) + # ISO-8601-UTC sorts lexically, so `>` IS the chronological compare. + if [ -z "$MARKER_AT" ] || [ "$RAT" \> "$MARKER_AT" ]; then + case "$RSTATE" in + CHANGES_REQUESTED) CODE_FAIL=1; CODE_PASS=0 ;; + APPROVED) CODE_PASS=1; CODE_FAIL=0 ;; + esac + fi # else the marker is newer and already stands — leave CODE_PASS/CODE_FAIL as the verb resolved them ;; esac; fi ``` diff --git a/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md b/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md index 232044a30..febb78bf6 100644 --- a/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md +++ b/claude-plugins/kampus-pipeline/skills/write-code/SKILL.md @@ -1695,9 +1695,12 @@ verdict** in each — the exact resolution `ship-it` Step 2 reads. That resoluti write+ author-gate so a self-authored or forged `review-(code|doc|skill): FAIL` is invisible, the latest-wins pick, and the ADR-0058 SHA-staleness refusal) is owned by `pipeline-cli verdict read`, so R1 delegates to the verb rather than re-deriving it (#2102) — its unit tests are the contract. -The native `CHANGES_REQUESTED` review that folds into the code namespace is the one thing the verb -does not resolve (it reads only marker comments); it needs no ACL gate — GitHub author-attributes -reviews, so that path is unforgeable — so R1 keeps just that fold inline. +The native decisive review that folds into the code namespace is the one thing the verb does not +resolve (it reads only marker comments); it needs no ACL gate — GitHub author-attributes reviews, so +that path is unforgeable — so R1 keeps just that fold inline. The fold is **newest-wins by +timestamp**, matching `ship-it` Step 2: the code verdict is the newest of {latest decisive review, +latest `review-code` marker}, so a newer `APPROVED` clears an older FAIL rather than the FAIL +standing forever. ```bash PR= @@ -1736,17 +1739,50 @@ CODE_FAIL_JSON="$($VERDICT read --pr "$PR" --gate code --expect FAIL 2>/dev/nul DOC_FAIL_JSON="$($VERDICT read --pr "$PR" --gate doc --expect FAIL 2>/dev/null)" && DOC_FAIL=1 || DOC_FAIL=0 SKILL_FAIL_JSON="$($VERDICT read --pr "$PR" --gate skill --expect FAIL 2>/dev/null)" && SKILL_FAIL=1 || SKILL_FAIL=0 -# the native CHANGES_REQUESTED review folds into the code namespace (the verb reads only marker -# comments): GitHub author-attributes reviews, so this path needs no ACL gate — commit_id IS its bound -# SHA, and a decisive review that is CHANGES_REQUESTED and prefix-matches the current head is a code FAIL too. +# UNRESOLVED ≠ "no FAIL". `verdict read` prints its outcome JSON on BOTH exit paths, so absent JSON +# means the namespace never resolved at all (a transport/5xx error, not a verdict). Under a flaky +# GitHub that silently reads as "nothing to repair" and SKIPS a real repair — so treat it as UNKNOWN +# and defer the run instead (a deferred repair is retried; a skipped one is lost). +VERDICT_UNKNOWN=0 +for J in "$CODE_FAIL_JSON" "$DOC_FAIL_JSON" "$SKILL_FAIL_JSON"; do + jq -e . >/dev/null 2>&1 <<<"$J" || VERDICT_UNKNOWN=1 +done + +# the native decisive review folds into the code namespace (the verb reads only marker comments): +# GitHub author-attributes reviews, so this path needs no ACL gate — commit_id IS its bound SHA, and +# only a review prefix-matching the current head participates (ADR 0058). +# +# The fold is NEWEST-WINS by timestamp — the resolution ship-it Step 2 states and this step mirrors. +# `at: .submitted_at` is load-bearing, not decoration: it is what the compare reads. A bare +# "CHANGES_REQUESTED ⇒ CODE_FAIL=1" would be FAIL-precedence, re-entering repair on a PR whose newer +# marker already PASS'd at the same head — a spurious repair loop, not a safe default. CURRENT_HEAD="$(gh api repos/$REPO/pulls/$PR --jq .head.sha)" REVIEW=$(gh api "repos/$REPO/pulls/$PR/reviews?per_page=100" \ --jq '[.[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] - | sort_by(.submitted_at) | last | {state, sha: .commit_id}') + | sort_by(.submitted_at) | last | {state, sha: .commit_id, at: .submitted_at}') RSTATE=$(jq -r '.state // ""' <<<"$REVIEW"); RSHA=$(jq -r '.sha // empty' <<<"$REVIEW") -[ -n "$RSHA" ] && [ "$RSTATE" = "CHANGES_REQUESTED" ] && case "$CURRENT_HEAD" in "$RSHA"*) CODE_FAIL=1 ;; esac +RAT=$(jq -r '.at // ""' <<<"$REVIEW") +# marker side: `_tag == "current"` is exactly "a current-head marker verdict stands" (a none/sha-less/ +# stale one does not participate); its comment id yields the created_at newest-wins compares against. +MARKER_AT="" +[ "$(jq -r '._tag // ""' <<<"$CODE_FAIL_JSON" 2>/dev/null)" = "current" ] && + MARKER_AT=$(gh api "repos/$REPO/issues/comments/$(jq -r .commentId <<<"$CODE_FAIL_JSON")" --jq .created_at 2>/dev/null) +if [ -n "$RSHA" ] && [ -n "$RAT" ]; then case "$CURRENT_HEAD" in "$RSHA"*) + # ISO-8601-UTC sorts lexically, so `>` IS the chronological compare. Empty MARKER_AT ⇒ the review is + # the only current-head event ⇒ it decides alone. A newer APPROVED clears an older FAIL — that is the + # point: without it a repaired-and-re-approved PR would re-enter repair forever. + if [ -z "$MARKER_AT" ] || [ "$RAT" \> "$MARKER_AT" ]; then + [ "$RSTATE" = "CHANGES_REQUESTED" ] && CODE_FAIL=1 || CODE_FAIL=0 + fi +;; esac; fi ``` +**A namespace that did not resolve at all is UNKNOWN, not "no FAIL" — defer, don't skip.** If +`VERDICT_UNKNOWN=1` (a `verdict read` that printed no outcome JSON — a transport/5xx failure rather +than a verdict), stop and report `verdict unresolved (GitHub read failed) — deferring, not skipping`. +Reading an unresolvable namespace as "nothing to repair" would silently drop a real repair; a +deferred run is retried, a skipped one is lost. + **Act only when a namespace's latest verdict is FAIL *bound to the current head*** — i.e. `CODE_FAIL=1` (a current-head `review-code: FAIL` marker per `verdict read`, or a current-head `CHANGES_REQUESTED` review), `DOC_FAIL=1`, or `SKILL_FAIL=1`. `verdict read` already encapsulates latest-wins **and** the