feat(rules): encode shell error handling; convert the 16 suppression sites by jbaruch · Pull Request #184 · jbaruch/coding-policy

jbaruch · 2026-07-16T20:59:00Z

Author-Model: claude-opus-4-8

Summary

rules/error-handling.md was entirely silent on shell. No set -euo pipefail, no suppression ban, nothing. That discipline lived only in the maintainer's private ~/.claude/CLAUDE.md, so every consumer installing this plugin inherited none of it — while the repo itself practises it in 18 of 18 non-test scripts. A rule that doesn't describe what's committed erodes every other rule (rules/context-artifacts.md Post-Edit Rule Audit), so the section and the drift fix land together rather than the rule landing with an IOU.

The three patterns, and what `|| true` was actually hiding

This looked like 35 mechanical substitutions. It was 16 real sites in three patterns, and the interesting part is that || true wasn't hiding nothing — it was hiding the difference between "no result" and "the tool broke":

pattern	sites	what was masked
`grep ... \|\| true`	5	`grep` exits 1 on no-match and 2 on error. Collapsed, an unreadable tree reports "no residual files" (`migrate.sh` declares clean a tree it never scanned) and a grep fault would manufacture the blocking no-`Author-Model` verdict (`adopt.sh`, per `rules/author-model-declaration.md`)
`jq ... 2>/dev/null \|\| true`	3	`verify-moderation-cleared.sh` exited 2 on a non-JSON body — correct code, wrong message: "the registry response shape may have changed; update this script's jq paths." It sends the operator to rewrite a working parse against a registry returning 502s. `rules/error-handling.md` Actionable Messages: naming the wrong action is worse than terse, because it's confidently wrong
best-effort cleanup	6	a runner without `perl` produced a subtly different file than one with it, and nothing said why. The consumer met it later as a reviewer flagging gh-aw drift on their first PR

Plus two one-offs (preflight.sh git rev-parse, adopt.sh ref restore).

Best-effort sites keep their leniency

scaffold.sh:452 argues its || true deliberately and correctly: sanitization runs after the compile-success rollback boundary, so hard-failing would abandon a scaffold that already succeeded. That reasoning survives intact.

The resolution is that "fail visibly" does not mean "exit non-zero." A missing perl now warns to stderr and continues. Same lenient behaviour, minus the silence.

Ten sites were never violations

2>/dev/null paired with an explicit handler is the form the rule prescribes — the failure is handled, only the diagnostic is silenced, usually to replace it with a better one:

repo_root=$(git rev-parse --show-toplevel 2>/dev/null) || { ... }   # push.sh:25
if ref=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null); then  # branch.sh:55

The rule now says so explicitly, so this doesn't get re-litigated every review.

Test plan

t_non-JSON body / t_truncated JSON body — fail against the pre-fix script, which reported "response shape may have changed — update the jq paths" for an HTML 502. Now exit 2 naming the real fault.
Full suite {"suites":17,"passed":17,"failed":0}; existing verify-publish-landed (17), verify-moderation-cleared (16), scaffold_env_example (15), scaffold_gitattributes (7) all still green.
grep -rn "|| true" --include="*.sh" skills/ scripts/ | grep -v /tests/ → no matches.
shellcheck clean on all six changed scripts; tessl plugin lint valid.
Paired cross-family policy review + Copilot

Scope notes

Test harnesses keep set -uo pipefail and source "$SCRIPT" || true — a failure-counting harness that dies on its first red assertion can't report the rest. The rule states this rather than leaving it as folklore.
Stacks cleanly with fix(ci): run the Python suites; harden the release baseline capture #183 (run-tests.sh + baseline capture); the only shared file is CHANGELOG.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

Copilot

Pull request overview

This PR codifies shell-specific error-handling requirements in rules/error-handling.md and updates the repository’s production shell scripts to replace blanket suppressions (|| true, bare 2>/dev/null) with explicit exit-code branching and actionable diagnostics, with targeted test additions to lock in the new behavior.

Changes:

Add a “Shell Error Handling” section to rules/error-handling.md (and document the “stderr-silenced but explicitly handled” pattern).
Convert multiple production suppression sites across skills to explicit rc handling / validation (notably grep and jq cases) while preserving intentional best-effort cleanup semantics with warnings.
Extend release moderation tests to cover non-JSON and truncated-JSON registry bodies.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
skills/release/verify-publish-landed.sh	Distinguishes grep parse-miss vs grep/tool failure and improves diagnostics around version parsing.
skills/release/verify-moderation-cleared.sh	Validates JSON once before extraction; removes suppression that masked non-JSON bodies and improves messaging.
skills/release/tests/test_verify_moderation_cleared.sh	Adds regression tests for non-JSON and truncated JSON payloads.
skills/migrate-to-plugin/migrate.sh	Branches on grep rc to distinguish clean “no matches” from scan failures.
skills/install-reviewer/scaffold.sh	Adds explicit grep rc handling helpers; replaces silent best-effort cleanup with warnings; refactors rollback directory cleanup logic.
skills/install-reviewer/preflight.sh	Removes blanket suppression and distinguishes expected “not a repo” exit codes from real git failures; improves gh-aw version parsing error handling.
skills/adopt-fork-pr/adopt.sh	Makes ref-restore best-effort but non-silent; distinguishes grep “no match” from grep failure when extracting Author-Model.
rules/error-handling.md	Adds a shell-specific error-handling policy section aligned with repo practices.
CHANGELOG.md	Documents the new rule section and the suppression-to-explicit-handling conversions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jbaruch · 2026-07-17T02:09:30Z

+        local aw_dir; aw_dir="$(dirname "$ACTIONS_LOCK")"
+        if [[ -d "$aw_dir" ]] && [[ -z "$(ls -A "$aw_dir")" ]]; then
+          rmdir "$aw_dir"
+        fi


Fixed in b338b5d — correct on both counts. $(ls -A ...) runs on the error path under set -e, so an unreadable directory aborts the rollback before the compile-failure diagnostic on the next line — losing the message the operator actually needs to a cleanup detail. And it warned about nothing.

Now probes with find "$aw_dir" -mindepth 1 -maxdepth 1 -print -quit, branches on its exit code, and warns without aborting: an unreadable directory is left in place with a note to remove it by hand; a failing rmdir on a genuinely empty directory warns too. The rollback always reaches its diagnostic.

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes for two rule-enforced issues: the new shell policy contradicts itself on set -euo pipefail versus failure-counting test harnesses, and the updated preflight.sh version probe still collapses a gh aw --version tool failure into an unparseable-output path. Both fall under the PR's own rules/error-handling.md contract and the context-artifact consistency audit.

Generated by PR Policy Review (OpenAI) for #184 · 37.5 AIC · ⌖ 5.81 AIC · ⊞ 14.6K

github-actions · 2026-07-16T21:04:33Z

+- Silencing a tool's diagnostic while explicitly handling its failure is not suppression: `cmd 2>/dev/null || { echo "<actionable message>" >&2; exit 1; }` replaces a worse message with a better one
+- Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing
+- An `EXIT` trap's final command status becomes the script's exit status — end cleanup handlers with `return 0` so cleanup never rewrites the outcome
+- Test harnesses that count failures use `set -uo pipefail` — a harness that dies on its first red assertion cannot report the rest


rules/context-artifacts.md requires rule edits to pass the consistency audit: "New rules don't contradict existing ones." This line conflicts with the new mandatory bullet above it: line 33 says every shell script opens with set -euo pipefail, while this says failure-counting test harnesses use set -uo pipefail. Fix by either scoping the first bullet to production/non-harness scripts or rewriting this as an explicit carve-out (per rules/context-writing-style.md: "Carve-outs: lead with Narrow exception for X.") so the rule is self-consistent.

Fixed in b338b5d — you caught the rule contradicting itself, four bullets apart, both mine, in the same section. "Every shell script opens with set -euo pipefail" (line 33) against "test harnesses use set -uo pipefail" (line 40). I wrote a rule whose consistency check is context-artifacts.md's "New rules don't contradict existing ones" and broke it against itself.

Took your second option — a proper carve-out per context-writing-style.md: Narrow exception for dropping set -e in a failure-counting test harness, numbered preconditions, reset line. Scoping the first bullet to "production scripts" would have left "production" undefined and invited exactly this argument again.

The carve-out shape earned its keep immediately: writing the preconditions surfaced a fourth one the contradiction had hidden — without set -e, a harness's own setup steps (temp dirs, fixtures) need explicit failure checks. Copilot independently found that exact bug in test_capture_registry_baseline.sh on #183, where an unchecked mktemp -d would have cascaded into confusing assertion failures.

github-actions · 2026-07-16T21:04:33Z

+  # cause collapsed into one "could not parse" message pointing at a
+  # re-install that may not be the problem.
+  local raw_rc=0
+  raw=$(gh aw --version 2>&1 | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -n1) || raw_rc=$?


rules/error-handling.md says to "Distinguish an expected non-result from a tool failure" and rules/error-handling.md Actionable Messages requires errors to tell the user what to do. This pipeline still collapses a gh aw --version tool failure into the parse-miss path: with set -o pipefail, if gh fails and grep sees no version text, the pipeline status is the rightmost non-zero command (grep exit 1), so raw_rc is not > 1 and the user gets the reinstall-extension message on lines 165-167. Fix by capturing gh aw --version into a variable first, checking that command's exit code, then running the version grep against the captured output so command failure and unparseable output stay separate.

Fixed in b338b5d — and you were right that my fix didn't work. I verified it before changing anything:

raw='' raw_rc=1 --> falls through to 'could not parse, reinstall gh-aw' (WRONG)

Exactly your reasoning: pipefail reports the rightmost non-zero, so a missing gh (127) piped into a no-match grep (1) yields 1 — under my > 1 threshold, straight to the reinstall-the-extension message. I'd replaced || true with a branch that couldn't fire for the case it existed to catch, then wrote a CHANGELOG entry claiming it did.

Now: capture gh aw --version into a variable, check that command's code, then grep the captured output. Command failure and unparseable output stay separate, and grep's own exit 2 gets its own diagnostic naming preflight.sh as the fault rather than blaming gh-aw.

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requested changes: the new Shell Error Handling rule is not self-consistent with the PR head audit required by rules/context-artifacts.md. Production EXIT traps still lack the neutral cleanup status required by the new clause, and scripts/run-diagnostics.sh still drops -e while falling outside the new failure-counting test-harness exception. Please fix the remaining drift, or narrow the rule text with explicit preconditions and audit every matching script against that contract. Also re-check the existing set -uo pipefail test harnesses for the new setup-step precondition before re-review.

Generated by PR Policy Review (OpenAI) for #184 · 66.4 AIC · ⌖ 12.1 AIC · ⊞ 15.5K

github-actions · 2026-07-17T02:11:59Z

+- Distinguish an expected non-result from a tool failure — `grep` exits 1 on no-match and 2 on error, and `|| true` collapses both, so an unreadable file or a bad regex reads as "nothing found"
+- Silencing a tool's diagnostic while explicitly handling its failure is not suppression: `cmd 2>/dev/null || { echo "<actionable message>" >&2; exit 1; }` replaces a worse message with a better one
+- Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing
+- An `EXIT` trap's final command status becomes the script's exit status — end cleanup handlers with `return 0` so cleanup never rewrites the outcome


rules/context-artifacts.md says: "After editing a rule, audit the repo itself against the new rule text and fix any drift in the same PR." This new clause says: "An EXIT trapsfinalcommandstatusbecomesthescripts exit status — end cleanup handlers with return 0 so cleanup never rewrites the outcome." PR head still has production traps like trap 'rm -f "$err_file"' EXIT in skills/release/verify-publish-landed.sh and skills/release/verify-moderation-cleared.sh, so the repo does not yet satisfy the rule it is adding. Please update those cleanup traps to use a handler that ends with return 0 (or adjust the rule if that is not the intended contract) before landing the rule.

Fixed in 7f0ab3c — you're right, and I took the first option rather than softening the rule.

verify-publish-landed.sh and verify-moderation-cleared.sh both used trap 'rm -f "$err_file"' EXIT, whose final command is an rm. Rare, but real: an unwritable TMPDIR makes rm non-zero, and that status rewrites the script's verdict into a bare 1 — a publish-landed or moderation result silently becoming a generic failure. Exactly the defect I hit while writing capture-registry-baseline.sh (a [[ -n ]] guard rewrote a deliberate exit 2 into exit 1), which is what prompted the clause in the first place. I encoded the lesson and left two instances of it in the tree.

Both now use named handlers ending return 0. Audited the tree: no inline EXIT traps remain in production scripts. adopt.sh's trap restore_orig_ref EXIT already conformed — that handler ends return 0 as of this PR's earlier commit.

github-actions · 2026-07-17T02:11:59Z


+## Shell Error Handling
+
+- Every shell script opens with `set -euo pipefail`


rules/context-artifacts.md says: "After editing a rule, audit the repo itself against the new rule text and fix any drift in the same PR." This new clause says: "Every shell script opens with set -euo pipefail", with only the later failure-counting test-harness exception. PR head still has scripts/run-diagnostics.sh opening with set -uo pipefail; it is a production diagnostics gate, not a test harness that reports assertion counts. Please either update that script to keep -e while preserving its explicit rc aggregation, or add a narrow exception with preconditions and audit every matching script against it.

Fixed in 7f0ab3c — and you found the real defect in the carve-out, not just a missing case.

scripts/run-diagnostics.sh isn't a test harness, so my carve-out couldn't reach it. But its own header says "Both engines always run (explicit rc capture, not error suppression per rules/error-handling.md)" — identical shape, different vocabulary. I'd scoped the exception to the example rather than the principle. What justifies dropping -e isn't "being a test harness"; it's running independent checks and reporting an aggregate, where aborting on the first failure hides every later finding.

Widened to that, and verified rather than assumed — run-diagnostics.sh meets all four preconditions: shellcheck and pyright are independent, each rc is captured explicitly and aggregated into a non-zero exit, only -e is dropped, and the setup steps -e would have caught (base-dir presence at :31, engine presence at :37) carry their own checks. It now cites the carve-out in its header so nobody re-derives this.

Audited the whole tree against the clause as written: run-diagnostics.sh was the only script outside the base rule.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

jbaruch · 2026-07-17T16:22:44Z

@@ -44,14 +44,21 @@ set -euo pipefail
 # `\b` GNU extension) and `-i` so capitalized "Tile" is not missed. Emits a
 # JSON array of matching file paths (relative, sorted) on stdout. Always 0.


Fixed in 86c0a90 — the scan_residual_files header comment (was "Always 0"), the migrate.sh Exit contract, and skills/migrate-to-plugin/SKILL.md Step 1 all now document the residual-scan exit 2 (grep rc>1 on an unreadable tree). gh-aw's latest review on the current head confirms no remaining violations. This thread predates that fix.

jbaruch · 2026-07-17T16:22:45Z

+    echo "migrate.sh: residual-file scan failed (grep rc=${grep_rc}) — the tree could not be fully read, so a 'no residual files' result cannot be trusted; check directory permissions, then re-run" >&2
+    return 2


Fixed in 86c0a90 — the scan_residual_files header comment (was "Always 0"), the migrate.sh Exit contract, and skills/migrate-to-plugin/SKILL.md Step 1 all now document the residual-scan exit 2 (grep rc>1 on an unreadable tree). gh-aw's latest review on the current head confirms no remaining violations. This thread predates that fix.

jbaruch · 2026-07-17T16:31:48Z


+## Shell Error Handling
+
+- Every shell script opens with `set -euo pipefail`


Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

jbaruch · 2026-07-17T16:22:46Z

@@ -44,14 +44,21 @@ set -euo pipefail
 # `\b` GNU extension) and `-i` so capitalized "Tile" is not missed. Emits a
 # JSON array of matching file paths (relative, sorted) on stdout. Always 0.


Fixed in 86c0a90 — the scan_residual_files header comment (was "Always 0"), the migrate.sh Exit contract, and skills/migrate-to-plugin/SKILL.md Step 1 all now document the residual-scan exit 2 (grep rc>1 on an unreadable tree). gh-aw's latest review on the current head confirms no remaining violations. This thread predates that fix.

jbaruch · 2026-07-17T16:31:50Z


+## Shell Error Handling
+
+- Every shell script opens with `set -euo pipefail`


Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

jbaruch · 2026-07-17T16:31:51Z

+    *)
+      echo "adopt.sh: grep failed (rc=${rc}) scanning the PR body for an Author-Model declaration — cannot distinguish 'absent' from 'unreadable'; report this with the PR body that triggered it" >&2
+      return 2
+      ;;
+  esac


Resolved — extract_author_model_line's grep-fault path and adopt.sh's documented exit codes were reconciled earlier in this PR (the here-string grep -m1 fix removed the SIGPIPE). gh-aw confirms clean on the current head.

jbaruch · 2026-07-17T16:31:53Z

+    # `// empty` covers an absent field; the body is known-valid JSON by
+    # now, so a jq failure here is a real fault and propagates under
+    # `set -e` rather than being read as an empty field.
    local status passed mod_error
-    status=$(printf '%s' "$body" | jq -r '.data.attributes.moderationStatus // empty' 2>/dev/null || true)
-    passed=$(printf '%s' "$body" | jq -r '.data.attributes.moderationPassed // empty' 2>/dev/null || true)
-    mod_error=$(printf '%s' "$body" | jq -r '.data.attributes.moderationError // empty' 2>/dev/null || true)
+    status=$(printf '%s' "$body" | jq -r '.data.attributes.moderationStatus // empty')
+    passed=$(printf '%s' "$body" | jq -r '.data.attributes.moderationPassed // empty')
+    mod_error=$(printf '%s' "$body" | jq -r '.data.attributes.moderationError // empty')


Resolved on the current head — the trap ends return 0 with a checked rm, JSON is validated with jq empty (not jq -e), and field access uses .attributes? to survive wrong-shape JSON. gh-aw confirms clean; this thread predates those fixes.

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes because the new shell-error-handling rule does not yet satisfy the repo’s own policy-authoring rules: it leaves existing shell test harness || true sites unaccounted for despite the new absolute ban, and the new carve-out prose still carries rationale/connective text that rules/context-writing-style.md says to move out of rule bodies. The inline comments identify the affected clauses and concrete fixes.

Generated by PR Policy Review (OpenAI) for #184 · 67.6 AIC · ⌖ 17.2 AIC · ⊞ 19.8K

github-actions · 2026-07-17T02:21:36Z

+## Shell Error Handling
+
+- Every shell script opens with `set -euo pipefail`
+- Never suppress a failure — no `|| true`, no `|| :`, no `2>/dev/null` standing in for a handler


rules/context-artifacts.md requires: "After editing a rule, audit the repo itself against the new rule text and fix any drift in the same PR." This new absolute ban leaves the PR head inconsistent with itself: rg '\|\| true' skills scripts -g '*.sh' still finds shell test harness uses such as skills/install-reviewer/tests/test_scaffold_env_example.sh:28, skills/migrate-to-plugin/tests/test_migrate.sh:18, and skills/release/tests/test_verify_publish_landed.sh:26 (source "$SCRIPT" || true). If those harness suppressions are legitimate, add a narrow exception with preconditions here; otherwise convert those tests too.

Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

github-actions · 2026-07-17T02:21:36Z

+- Narrow exception for dropping `set -e` in a script whose job is running independent checks and reporting an aggregate — aborting on the first failure hides every later finding, the opposite of what the script exists to do (test harnesses, multi-engine diagnostics gates)
+- Preconditions (all required):
+  1. Each check is independent — a later one's result never depends on an earlier one having passed
+  2. The script captures each check's exit code explicitly and exits non-zero when any check failed
+  3. It keeps `set -uo pipefail` — only `-e` is dropped
+  4. Setup steps the checks depend on (temp dirs, fixtures, tool presence) carry their own explicit failure check, since `set -e` is no longer catching them


rules/context-writing-style.md says: "Logical connectives are why-content detectors: therefore, however, because, since..." and "Carve-outs: lead with Narrow exception for X., then numbered preconditions, then a one-line reset." This carve-out still includes rationale in the rule body (aborting on the first failure...) and uses since in precondition 4. Move the explanation to CHANGELOG.md and keep the rule to the exception label plus directive preconditions, e.g. make precondition 4 state the required explicit setup checks without the since clause.

Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

…on sites error-handling.md said nothing about shell — no set -euo pipefail, no suppression ban. That discipline lived only in the maintainer's private config, so consumers of the published plugin inherited none of it while the repo itself practised it in 18 of 18 non-test scripts. A rule that doesn't describe the repo erodes every rule. Land the section and the drift fix together, per context-artifacts.md Post-Edit Rule Audit. The interesting half is what `|| true` was actually hiding. grep exits 1 on no-match and 2 on error; `|| true` collapses both, so an unreadable tree reported "no residual files" (migrate.sh) and a grep fault would have manufactured the blocking no-Author-Model verdict (adopt.sh). verify-moderation-cleared exited 2 on a non-JSON body but told the operator the registry's shape had changed and to rewrite its jq paths — confidently wrong, against a registry returning 502s. Best-effort sites keep their leniency. "Fail visibly" does not mean exit non-zero: a missing perl now warns instead of silently producing a different file than a runner that has it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

…broke gh-aw caught the new rule contradicting itself: "every shell script opens with set -euo pipefail" sat four bullets above "test harnesses use set -uo pipefail". Both mine, same section. Now a proper carve-out per context-writing-style — narrow exception, numbered preconditions, reset line — with a fourth precondition the contradiction had hidden: without set -e, a harness's own setup steps need explicit failure checks. preflight.sh: the fix didn't work. `gh aw --version | grep | head` under pipefail reports the RIGHTMOST non-zero, so a missing gh (127) plus a no-match grep (1) yields 1 — below the `> 1` tool-failure branch, so the user still got "could not parse, re-install the gh-aw extension" when the real problem was gh not being on PATH. Verified before fixing. Capture the command, check its code, then parse the captured output. scaffold.sh: `[[ -z "$(ls -A "$aw_dir")" ]]` runs on the error path under set -e, so an unreadable directory aborts the rollback before it emits the compile-failure diagnostic — losing the message the operator needs to a cleanup detail. Probe with find, warn, leave the directory alone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

gh-aw ran the Post-Edit Rule Audit against both new clauses and found the repo failing each. A rule that doesn't describe what's committed erodes every rule, so the rule and the repo move together or the rule waits. `set -euo pipefail` clause: scripts/run-diagnostics.sh opens with `set -uo pipefail` and is not a test harness, so the carve-out as written didn't reach it. The carve-out was scoped to the example rather than the principle — what justifies dropping -e is not "being a test harness", it is running independent checks and reporting an aggregate, where aborting on the first failure hides every later finding. Widened to that, and run-diagnostics.sh now cites it and its four preconditions (verified: the engines are independent, each rc is captured and aggregated, only -e is dropped, and its setup steps carry their own checks). `EXIT trap return 0` clause: verify-publish-landed.sh and verify-moderation-cleared.sh both used inline `trap 'rm -f "$err_file"'`, whose final status is an `rm` — so an unwritable TMPDIR would rewrite a publish or moderation verdict into a bare 1. Both now use named handlers ending `return 0`. Audited the tree: no inline EXIT traps remain in production scripts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes: the new shell error-handling rule must satisfy the existing context-writing-style rule, and the scripts updated to comply with the new cleanup/trap clauses still contain unhandled cleanup failures under set -e. The affected paths need explicit warning-and-continue handlers so best-effort cleanup and EXIT traps cannot rewrite the primary outcome.

Generated by PR Policy Review (OpenAI) for #184 · 61.5 AIC · ⌖ 2.74 AIC · ⊞ 14.6K

github-actions · 2026-07-17T02:36:05Z

+- An `EXIT` trap's final command status becomes the script's exit status — end cleanup handlers with `return 0` so cleanup never rewrites the outcome
+- Narrow exception for dropping `set -e` in a script whose job is running independent checks and reporting an aggregate — aborting on the first failure hides every later finding, the opposite of what the script exists to do (test harnesses, multi-engine diagnostics gates)
+- Preconditions (all required):
+  1. Each check is independent — a later one's result never depends on an earlier one having passed


rules/context-writing-style.md says: "Logical connectives are why-content detectors: therefore, however, because, since, thus, consequently, moreover, although, despite, as a result, in order to" and "When you write one of these, strip the entire clause it introduces and move the explanation to CHANGELOG." This new rule line uses since in since set -e is no longer catching them. Drop that justification from the rule body, e.g. keep the directive as Setup steps the checks depend on carry their own explicit failure check, and leave the rationale in the changelog/PR body.

Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

github-actions · 2026-07-17T02:36:05Z

+sanitize_lock() {
+  local l="$1"
+  if sed -i.bak -E 's/[[:space:]]+$//' "$l" 2>/dev/null; then
+    rm -f "${l}.bak"


rules/error-handling.md says: "Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing." This function is explicitly best-effort after compile success, but rm -f "${l}.bak" is still an unhandled command under set -e; if deleting the backup fails, scaffold aborts instead of warning and continuing. Handle it the same way as the sed/perl cleanup paths, e.g. if ! rm -f "${l}.bak"; then echo "scaffold.sh: warning: ..." >&2; fi.

Resolved — sanitize_lock's backup rm is checked now (if ! rm; then warn), so best-effort no longer means a bare rm that could hard-fail under set -e. gh-aw confirms clean on the current head.

github-actions · 2026-07-17T02:36:05Z

+# script's verdict (rules/error-handling.md Shell Error Handling).
+cleanup_err_file() {
+  if [[ -n "${err_file:-}" ]]; then
+    rm -f "$err_file"


rules/error-handling.md says: "An EXIT trap's final command status becomes the script's exit status — end cleanup handlers with return 0 so cleanup never rewrites the outcome." With set -e active, a failing rm -f "$err_file" exits this handler before it reaches the return 0, so the trap can still rewrite the moderation verdict. Make the cleanup failure explicitly handled, e.g. if ! rm -f "$err_file"; then echo "verify-moderation-cleared.sh: warning: ..." >&2; fi; return 0.

Resolved on the current head — the trap ends return 0 with a checked rm, JSON is validated with jq empty (not jq -e), and field access uses .attributes? to survive wrong-shape JSON. gh-aw confirms clean; this thread predates those fixes.

github-actions · 2026-07-17T02:36:05Z

+# script's verdict (rules/error-handling.md Shell Error Handling).
+cleanup_err_file() {
+  if [[ -n "${err_file:-}" ]]; then
+    rm -f "$err_file"


rules/error-handling.md says: "An EXIT trap's final command status becomes the script's exit status — end cleanup handlers with return 0 so cleanup never rewrites the outcome." This handler has the same remaining failure mode the comment is trying to remove: under set -e, rm -f "$err_file" can abort the handler before return 0 executes. Wrap the cleanup in an explicit if ! rm -f ...; then echo warning >&2; fi block, then return 0 unconditionally.

Resolved — the EXIT trap uses a named handler ending return 0 with a checked rm, and the Latest-Version parse uses grep -m1 via here-string. gh-aw confirms clean on the current head.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

jbaruch · 2026-07-17T16:32:06Z

+    *)
+      echo "adopt.sh: grep failed (rc=${rc}) scanning the PR body for an Author-Model declaration — cannot distinguish 'absent' from 'unreadable'; report this with the PR body that triggered it" >&2
+      return 2
+      ;;


Resolved — extract_author_model_line's grep-fault path and adopt.sh's documented exit codes were reconciled earlier in this PR (the here-string grep -m1 fix removed the SIGPIPE). gh-aw confirms clean on the current head.

jbaruch · 2026-07-17T16:32:07Z

+    # it sends the operator to rewrite a parse that works, against a
+    # registry that is returning 502s (rules/error-handling.md Actionable
+    # Messages).
+    if ! printf '%s' "$body" | jq -e . >/dev/null 2>&1; then


Resolved on the current head — the trap ends return 0 with a checked rm, JSON is validated with jq empty (not jq -e), and field access uses .attributes? to survive wrong-shape JSON. gh-aw confirms clean; this thread predates those fixes.

jbaruch · 2026-07-17T16:32:09Z

+if [[ $git_rc -ne 0 && $git_rc -ne 128 ]]; then
+  echo "preflight.sh: 'git rev-parse --show-toplevel' failed (rc=${git_rc}) — not the ordinary not-a-repo case; verify git is installed and the repository is readable, then re-run" >&2
+  exit 2
+fi


Resolved on the current head — the git-usable and tmpdir-writable failures emit JSON envelopes and exit 1 (not stderr-only exit 2), the git-128 classification uses git's stable sentinel via a mktemp'd capture with a checked rm, and push_failure builds JSON via jq. gh-aw confirms clean.

jbaruch · 2026-07-17T16:32:11Z


+## Shell Error Handling
+
+- Every shell script opens with `set -euo pipefail`


Resolved on the current head — the Shell Error Handling section and its carve-outs were reworked across this PR's commits (the aggregate-checks carve-out, the source-under-test carve-out, connectives cut, the setup-check precondition narrowed to silent-corruption). gh-aw's latest review confirms no remaining violations; this thread predates the current wording.

…sion gh-aw found the cleanup handlers still broken. Under `set -e` a failing `rm -f` aborts the handler BEFORE `return 0` runs — reintroducing the exact rewrite the handler exists to prevent. The fix for the bug had the bug. Now `if ! rm -f ...; then warn; fi; return 0`, where the `if` condition suspends `set -e`. Also fixes placement: the handler had been inserted between version_gt's doc comment and version_gt itself, leaving the comment describing the wrong function. The `|| true` ban vs. test harnesses: I claimed those were decorative and removed 10 of them, which broke 8 suites. They are load-bearing. Sourcing a script under test executes ITS `set -euo pipefail` in the harness's shell; the entry-point guard then returns non-zero under the -e it just enabled and kills the harness. The `set +e` on the next line — present at all 10 sites — is the tell. Reverted, and added the narrow exception with preconditions gh-aw's comment offered as the alternative to converting. Verified all 10 sites satisfy it. Style: dropped `since` from precondition 4 and the rationale clause from the carve-out lead (context-writing-style bans connectives and sends why-content to CHANGELOG); the absolute `set -euo pipefail` bullet now carries a forward pointer to the carve-out, matching how Protected Branches points at its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes: the shell-error-handling rule applies to the modified shell scripts, and two changed paths still violate the new contract. scaffold.sh documents a cleanup as best-effort but can exit non-zero under set -e when backup removal fails, and migrate.sh adds a return 2 scan failure path without updating the command-substitution call sites to preserve that diagnostic/exit contract.

Generated by PR Policy Review (OpenAI) for #184 · 49.9 AIC · ⌖ 2.76 AIC · ⊞ 18.5K

github-actions · 2026-07-17T02:48:34Z

+sanitize_lock() {
+  local l="$1"
+  if sed -i.bak -E 's/[[:space:]]+$//' "$l" 2>/dev/null; then
+    rm -f "${l}.bak"


rules/error-handling.md says: "Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing." This cleanup is documented just above as best-effort, but under set -euo pipefail a failing rm -f "${l}.bak" exits scaffold.sh before sanitize_lock reaches return 0. Handle the backup removal like the surrounding best-effort cleanup, e.g. if ! rm -f "${l}.bak"; then echo "scaffold.sh: warning: ..." >&2; fi, so a cleanup failure warns without failing the already-successful scaffold.

Resolved — sanitize_lock's backup rm is checked now (if ! rm; then warn), so best-effort no longer means a bare rm that could hard-fail under set -e. gh-aw confirms clean on the current head.

github-actions · 2026-07-17T02:48:34Z

+    --exclude='CHANGELOG.md' 2>/dev/null) || grep_rc=$?
+  if [[ $grep_rc -gt 1 ]]; then
+    echo "migrate.sh: residual-file scan failed (grep rc=${grep_rc}) — the tree could not be fully read, so a 'no residual files' result cannot be trusted; check directory permissions, then re-run" >&2
+    return 2


rules/error-handling.md says: "A command that can legitimately fail gets an explicit if or case on its exit code, never blanket suppression" and rules/file-hygiene.md requires scripts to use meaningful non-zero exits. Returning 2 here is not enough because both callers use $(scan_residual_files) directly inside emit (lines 112 and 154); a non-zero command substitution there is not branched on, so the intended tool/precondition failure can degrade into an empty/non-JSON --argjson failure instead of this diagnostic and exit contract. Capture the scan first and branch on its status, e.g. residual=$(scan_residual_files) || exit 2; emit ... "$residual", and update the stale "Always 0" function comment.

Fixed in 79abf0c — you're right, and this is the third pass over the same line.

| head -n1 killed grep. I fixed it with grep -m1, which killed printf instead — grep -m1 exits at the first match and closes the pipe on a still-writing printf, so printf carries the 141 while grep exits 0, and pipefail reports the rightmost NON-ZERO. Reproduced:

PIPE form: raw='1.2.3' rc=141 <-- FALSE TOOL FAULT here-string: raw='1.2.3' rc=0 ok

Both times I treated the symptom — moved which process dies rather than removing the thing that kills it. A pipeline under pipefail lets any process in it speak for the parse. So: no pipeline. Here-strings, one process, grep's own rc is the only status the case can read.

Swept instead of fixing just the site you named, and found two more of the same shape: adopt.sh's Author-Model parse (printf | grep -m1) and poll-pr-reviews.sh's no-checks probe (echo | grep -qi — -q exits at the first match, same race, and a false negative there routes a valid no-checks state into the error exit). Both converted; no early-exit grep pipelines remain in production.

jbaruch

Summary

This PR does the right thing in the large: it encodes shell discipline the repo already mostly practiced, then converts the 16 production || true sites into explicit exit-code branches. The grep 1-vs-2 splits in adopt, migrate, verify-publish-landed, and first_match_line are correct; the jq validate-once path in verify-moderation-cleared fixes a real misdiagnosis; EXIT cleanup handlers in the verify scripts use if ! rm + return 0 properly; run-diagnostics documents the independent-checks carve-out with all four preconditions. Two concrete conversion bugs remain in the new code: preflight.sh reintroduces a grep | head pipeline under pipefail (the exact SIGPIPE footgun resolve-publish-run.sh was written to avoid), and sanitize_lock's unprotected rm can still hard-fail the scaffold after a successful compile — the outcome the conversion claims to prevent. Residual whole-repo gaps (e.g. push.sh's || echo "" probe, bare if ! grep sites that still collapse exit 2 into "absent") sit outside the converted set and are noted for follow-up rather than blocking the rule itself.

Issue counts by severity

bugs: 2
suggestions: 0
nits: 0

jbaruch · 2026-07-17T02:50:21Z

+  # Parse the captured output. grep's exit 1 here means the command ran and
+  # printed no version — a real preflight finding, reported below.
+  local grep_rc=0
+  raw=$(printf '%s\n' "$gh_out" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -n1) || grep_rc=$?


[bug] The version parse still pipelines through head -n1 under set -o pipefail: printf ... | grep -oE '...' | head -n1. Pipefail reports the rightmost non-zero status. When grep emits a match and head closes the pipe after one line, grep can die with SIGPIPE (rc 141). That is > 1, so the next branch reports "Version parse failed (grep rc=141)" even though raw already holds a valid version — a false preflight failure on a successful parse. This is the same SIGPIPE-on-head failure mode that skills/release/resolve-publish-run.sh explicitly documents and avoids. The earlier fix correctly split gh aw --version from the parse; the remaining grep | head undoes that discipline for the parse step itself.

Suggestion: Drop the head pipeline. Use grep -m1 -oE '[0-9]+\.[0-9]+\.[0-9]+' (or capture full matches then take the first line from the already-captured string, as first_match_line does). Then branch on grep's own rc only: 0 = version found, 1 = no match, ≥2 = tool fault.

Fixed in 599350e — reproduced before fixing, and you're right about the irony: I split gh aw --version from the parse to kill exactly this class, then rebuilt it one line lower.

raw='1.2.3' pipeline_rc=141 (141 = SIGPIPE) --> REPRODUCED: false 'parse failed (rc=141)' despite raw='1.2.3'

(200k matches — small inputs don't trigger it, which is why my own testing missed it.)

Took grep -m1: grep stops itself, nothing closes the pipe on a still-writing grep, and grep's own rc is the only status.

Then swept for the pattern instead of just the site you flagged, and found it again in first_match_line — printf | head -1 | cut returns printf's 141 under pipefail, and the caller assigns that under set -e, aborting the scaffold on a successful lookup. Now grep -n -m1 plus parameter expansion, no pipeline at all.

Your second bug (summary, not inline) is also fixed: sanitize_lock's rm -f "${l}.bak" was bare under set -e inside a function that is best-effort by design — a failed backup cleanup would hard-fail the scaffold after a successful compile, precisely the outcome the leniency exists to prevent.

jbaruch · 2026-07-17T02:50:21Z

+sanitize_lock() {
+  local l="$1"
+  if sed -i.bak -E 's/[[:space:]]+$//' "$l" 2>/dev/null; then
+    rm -f "${l}.bak"


[bug] sanitize_lock is documented as best-effort ("does not exit non-zero") and ends with return 0, but the sed-success path runs a bare rm -f "${l}.bak". Under set -e, only the if condition suspends errexit — the then-body does not. A failing rm (permissions, sticky bit, unwritable dir) aborts the function before return 0, propagates out of the post-compile for l in "${locks[@]}"; do sanitize_lock "$l"; done loop, and hard-fails the scaffold after compile already succeeded — the exact worse outcome the surrounding comments say this conversion is avoiding. The verify scripts in this same PR correctly use if ! rm ...; then warn; fi; return 0 for this reason; sanitize_lock does not.

Suggestion: Mirror the verify-script pattern:

if sed -i.bak -E 's/[[:space:]]+$//' "$l" 2>/dev/null; then if ! rm -f "${l}.bak"; then echo "scaffold.sh: warning: could not remove ${l}.bak — remove it by hand" >&2 fi else echo "scaffold.sh: warning: sed failed on ${l} — ..." >&2 fi

Keep the final return 0.

Resolved — sanitize_lock's backup rm is checked now (if ! rm; then warn), so best-effort no longer means a bare rm that could hard-fail under set -e. gh-aw confirms clean on the current head.

Grok caught preflight.sh reintroducing `grep -oE ... | head -n1` under pipefail. Reproduced at 200k matches: head closes the pipe, grep dies with SIGPIPE (141), pipefail reports it, and 141 > 1 fires the tool-fault branch — "Version parse failed (grep rc=141)" with a valid version already sitting in $raw. I split the command from the parse to kill exactly this class, then rebuilt it inside the parse step. resolve-publish-run.sh documents the same footgun. `grep -m1` removes the pipe: grep stops itself, nothing closes on it, and grep's own rc is the only status. Swept for the pattern rather than fixing only the reported site, and found first_match_line carrying it too: `printf | head -1 | cut` returns printf's 141 under pipefail, and the caller assigns that under `set -e` — aborting the scaffold on a successful lookup. Now `grep -n -m1` plus parameter expansion, no pipeline. sanitize_lock's `rm -f "${l}.bak"` was bare under `set -e` inside a function that is best-effort by design (it runs past the compile-success rollback boundary) — a failed backup cleanup would hard-fail the scaffold, the exact outcome the leniency exists to prevent. Third instance of that class today. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes: the new shell-error-handling rule applies to the converted shell sites, and two changed parsers still branch on a pipeline status where grep -m1 can succeed while the upstream printf fails with SIGPIPE under pipefail. That preserves the same class of false tool-fault the PR is trying to remove; switch those parses to a non-pipeline form so the explicit case reads the tool status it intends to read.

Generated by PR Policy Review (OpenAI) for #184 · 12.4 AIC · ⌖ 6.16 AIC · ⊞ 54.5K

github-actions · 2026-07-17T03:02:02Z

+  # and grep's own rc is the only status. Same footgun
+  # skills/release/resolve-publish-run.sh documents and avoids.
+  local grep_rc=0
+  raw=$(printf '%s\n' "$gh_out" | grep -m1 -oE '[0-9]+\.[0-9]+\.[0-9]+') || grep_rc=$?


rules/error-handling.md says: "Distinguish an expected non-result from a tool failure" and "A command that can legitimately fail gets an explicit if or case on its exit code, never blanket suppression." This replacement still runs an early-exiting grep -m1 in a pipeline under set -o pipefail: after grep finds the first version and exits, the upstream printf can receive SIGPIPE, making the pipeline return 141 and driving the "Version parse failed" branch even though a valid version was found. Use a single-command parse such as grep -m1 ... <<<"$gh_out", or another non-pipeline extraction, so the status you branch on is grep's status only.

Resolved on the current head — the git-usable and tmpdir-writable failures emit JSON envelopes and exit 1 (not stderr-only exit 2), the git-128 classification uses git's stable sentinel via a mktemp'd capture with a checked rm, and push_failure builds JSON via jq. gh-aw confirms clean.

github-actions · 2026-07-17T03:02:02Z

+  # declaration block the PR, so a grep fault would silently manufacture
+  # the blocking verdict instead of reporting itself.
+  local line rc=0
+  line=$(printf '%s\n' "$1" | grep -m1 -E '^[[:space:]]*\*{0,2}Author-Model:') || rc=$?


rules/error-handling.md says: "Distinguish an expected non-result from a tool failure" and requires explicit exit-code handling for commands that legitimately return non-zero. This has the same remaining pipefail/SIGPIPE race as the preflight parse: grep -m1 can find the Author-Model line and exit before printf finishes writing a long PR body, so the pipeline status can become 141 and this function reports a grep fault instead of returning the declaration. Parse without a pipeline, for example grep -m1 ... <<<"$1", then keep the existing case on grep's status.

Resolved — extract_author_model_line's grep-fault path and adopt.sh's documented exit codes were reconciled earlier in this PR (the here-string grep -m1 fix removed the SIGPIPE). gh-aw confirms clean on the current head.

Grok: the 125 sentinel is a soft invariant. run_suite propagates the suite's own status unchanged on the happy path, so a suite that exits 125 is misclassified as a dispatcher fault — the run stops, JSON says suites=0, and a real red suite reports as a broken runner. The comment claimed 125 is outside what suites produce; true of today's harnesses, enforced by nothing. That was the second attempt at the same mistake. 2 collided with harness fatals, so I reached for a rarer number instead of a different mechanism. No exit code can carry this: any value a suite might return is a value a suite might return. The fault now travels by side channel (DISPATCH_FAULT_SUITE). Every status run_suite returns is the suite's own verdict, 2 and 125 alike. Regression test pins the 125 case per Grok's suggestion. Also fixes capture-registry-baseline.sh's cleanup, which had the bug gh-aw found in verify-publish-landed.sh on #184: `return 0` does not save a handler whose bare `rm` aborts it under `set -e`. Reproduced — an unwritable TMPDIR turned a deliberate `exit 2` into `exit 1` and the handler never reached its last line. Grok reviewed this file and passed it; neither reviewer sees across PRs, so the same bug was fixed in one branch and left live in another. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

gh-aw: `grep -m1` moved the SIGPIPE one process upstream instead of removing it. grep -m1 exits at the first match and closes the pipe on a still-writing printf, so printf carries 141 while grep exits 0 — and pipefail reports the rightmost NON-ZERO, so the pipeline is 141 and the tool-fault branch fires on a successful parse. Reproduced at 200k matches: PIPE form: raw='1.2.3' rc=141 <-- FALSE TOOL FAULT here-string: raw='1.2.3' rc=0 ok Third pass over this same line. `| head -n1` killed grep; `grep -m1` killed printf; the fix both times treated the symptom. A pipeline under pipefail lets any process in it speak for the parse, so the answer is no pipeline: here-strings, one process, grep's own rc. Swept rather than fixing the reported site: adopt.sh's Author-Model parse had the same printf|grep -m1 shape, and poll-pr-reviews.sh's no-checks probe had it as echo|grep -qi (-q exits at first match, same race). Both converted. No early-exit grep pipelines remain in production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

Copilot

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes: the PR adds shell exit-code discipline to rules/error-handling.md, so rules/context-artifacts.md Post-Edit Rule Audit applies. The changed rule is not yet self-consistent with the repository because several production shell probes still collapse expected non-results with tool failures; finish converting those remaining sites before this rule can ship as the repo policy.

Generated by PR Policy Review (OpenAI) for #184 · 172.5 AIC · ⌖ 4.45 AIC · ⊞ 18.3K

github-actions · 2026-07-17T16:55:41Z

+
+- Every shell script opens with `set -euo pipefail` (except under the carve-out below)
+- Never suppress a failure — no `|| true`, no `|| :`, no `2>/dev/null` standing in for a handler
+- A command that can legitimately fail gets an explicit `if` or `case` on its exit code, never blanket suppression


This new rule is not yet consistent with the repo it governs. rules/context-artifacts.md says: “After editing a rule, audit the repo itself against the new rule text and fix any drift in the same PR,” and the new clause here says commands that can legitimately fail need explicit exit-code handling.

There are still production probes that collapse expected absence with tool failure, for example skills/install-reviewer/branch.sh still treats git symbolic-ref refs/remotes/origin/HEAD failure as “fall back to main/master,” and skills/install-reviewer/preflight.sh still treats git show-ref / git ls-remote --exit-code non-zero as simple absence in the install-mode branch checks. Please finish the post-edit audit by branching on those commands’ exit codes (show-ref: 0 present, 1 absent, >1 fault; ls-remote --exit-code: 0 present, 2 absent, other fault; symbolic-ref: expected absence vs fault) and surfacing real tool failures instead of falling through.

Fixed in c6c05da — swept all git boolean-probes (show-ref, ls-remote, symbolic-ref) rather than the two you named, so this class stops surfacing one per round. branch.sh gets a ref_exists helper; preflight's two checks branch on the codes.

The symbolic-ref one exposed a real subtlety worth recording: WITHOUT --quiet, git returns 128 for BOTH 'HEAD symref not set' (expected) AND a real fault — indistinguishable. My first attempt treated the common no-symref case as a fault and broke the master-fallback test; the test catching it is what surfaced the nuance. WITH --quiet, absent is rc 1 and only a genuine fault stays 128. That distinction is the fix.

Also fixed a fixture bug the || true removal exposed: the master-fallback setup pushed-then-deleted from the bare repo, but deleting the bare repo's current branch is prohibited (the || true had swallowed that). resolve_default_branch reads the LOCAL remote-tracking refs, so the fixture now shapes those directly.

gh-aw (Post-Edit Rule Audit): production git probes still collapse expected absence with a tool fault, against the new shell clause. Swept all show-ref / ls-remote / symbolic-ref boolean probes rather than the two named, so gh-aw stops finding them one per round. branch.sh: a `ref_exists` helper (show-ref 0 present / 1 absent / >1 fault => exit 2) routes the three heads probes; resolve_default_branch surfaces a symbolic-ref fault instead of guessing main/master. The symbolic-ref case exposed a real subtlety: WITHOUT `--quiet`, git returns 128 for BOTH "HEAD symref not set" (expected) and a real fault, so they're indistinguishable — my first attempt treated the common no-symref case as a fault and broke the fallback test. WITH `--quiet`, absent is rc 1 and only a genuine fault is 128. That distinction is the whole fix. preflight.sh: check_branch_not_local / check_branch_not_remote branch on show-ref (0/1/>1) and ls-remote --exit-code (0/2/other), pushing a distinct failure for a real git fault instead of reading it as "branch absent". test_branch fixture: the master-fallback setup pushed to then deleted from the bare repo, but deleting the bare repo's current branch is prohibited (the old `|| true` swallowed it). resolve_default_branch reads the LOCAL remote-tracking refs, so the fixture now shapes those directly (`update-ref -d refs/remotes/origin/main`), no bare-repo delete. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JgdWmv7KBj91nxbVjMsj7X

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

skills/release/verify-publish-landed.sh:149

cleanup_err_file is invoked by an EXIT trap after main returns, but err_file is declared local inside main(). In bash, local variables go out of scope when the function returns, so the trap handler won’t see err_file and the temp file won’t be removed (and any cleanup warning becomes ineffective). Make err_file script-global (do not declare it local) so the trap can reliably clean it up.

  local conclusion err_file
  err_file=$(mktemp) || { echo "error: mktemp failed — cannot run verify-publish-landed.sh without writable TMPDIR" >&2; exit 2; }
  # Named handler ending `return 0`, not an inline `rm -f`: the EXIT trap's
  # final command status becomes the script's exit status, so an `rm` that
  # fails (unwritable TMPDIR) would rewrite this script's verdict into a
  # bare 1 — a publish-landed conjunction silently reported as a generic
  # failure. Cleanup reports on cleanup, never on the outcome.
  trap cleanup_err_file EXIT

skills/release/verify-moderation-cleared.sh:132

cleanup_err_file runs from an EXIT trap after main returns, but err_file is declared local inside main(). That means the trap handler won’t see the variable and won’t remove the tempfile. Make err_file script-global (remove local) so the cleanup trap can do its job.

  local endpoint="v1/tiles/${workspace}/${tile}/versions/${version}"
  local delay="$BASE_DELAY_SEC" elapsed=0 attempts=0
  local err_file; err_file=$(mktemp) || { echo "error: mktemp failed — cannot run verify-moderation-cleared.sh without writable TMPDIR" >&2; exit 2; }
  # Named handler ending `return 0` — the EXIT trap's final command status
  # becomes the script's exit status, so a failing `rm` would rewrite the
  # moderation verdict into a bare 1 (rules/error-handling.md).
  trap cleanup_err_file EXIT

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

Requesting changes because the PR edits rules/error-handling.md, so rules/context-artifacts.md requires the repository to satisfy the new rule text in the same PR. The new shell clause is useful, but the added carve-outs need to follow rules/context-writing-style.md's required carve-out structure, and the post-edit audit still leaves production cleanup sites that can fail silently or rewrite/abort an error path under the new best-effort-warning rule. I left inline comments on the changed rule lines with examples and proposed fixes.

Checked: PR metadata/author-family gate via resolver (review), all 23 rule files loaded from PR head, relevant touched SKILL.md contracts loaded, changed-file allowlist built from the PR file list, and bash -n passed for the changed shell scripts.

Generated by PR Policy Review (OpenAI) for #184 · 141.3 AIC · ⌖ 11.5 AIC · ⊞ 14.6K

github-actions · 2026-07-17T22:38:45Z

+- Silencing a tool's diagnostic while explicitly handling its failure is not suppression: `cmd 2>/dev/null || { echo "<actionable message>" >&2; exit 1; }` replaces a worse message with a better one
+- Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing
+- An `EXIT` trap's final command status becomes the script's exit status — end cleanup handlers with `return 0` so cleanup never rewrites the outcome
+- Narrow exception for dropping `set -e` in a script that runs independent checks and reports an aggregate (test harnesses, multi-engine diagnostics gates)


rules/context-writing-style.md says: "Carve-outs: lead with "Narrow exception for X.", then numbered preconditions, then a one-line reset stating every other case follows the rule." This new carve-out lead folds the scope and example list into the lead instead of using that required shape; the same pattern appears again on the || true carve-out below. Please rewrite these as a short Narrow exception for ... lead plus any Applies when ... constraint before the preconditions, matching the established carve-out format.

Fixed in bfa464a. Split the folded scope/example clause out of both carve-out leads into their own Applies when ... bullet, matching the Outer-Boundary carve-out template in the same file: Narrow exception for aggregate-reporting scripts that drop set -e / Applies when a script runs independent checks and reports an aggregate (...), and likewise for the || true-on-source carve-out.

github-actions · 2026-07-17T22:38:45Z

+- A command that can legitimately fail gets an explicit `if` or `case` on its exit code, never blanket suppression
+- Distinguish an expected non-result from a tool failure — `grep` exits 1 on no-match and 2 on error, and `|| true` collapses both, so an unreadable file or a bad regex reads as "nothing found"
+- Silencing a tool's diagnostic while explicitly handling its failure is not suppression: `cmd 2>/dev/null || { echo "<actionable message>" >&2; exit 1; }` replaces a worse message with a better one
+- Fail visibly does not require exit non-zero — best-effort work that legitimately continues past a failure emits a warning to stderr, never nothing


rules/context-artifacts.md Post-Edit Rule Audit says: "After editing a rule, audit the repo itself against the new rule text and fix any drift in the same PR." This new clause requires best-effort failures to warn, but the PR still leaves production cleanup sites that can fail with no warning or abort the wrong path, e.g. scripts/run-diagnostics.sh:65,74, scripts/run-tests.sh:76,88, and skills/install-reviewer/scaffold.sh:265,450,452,457,491,493. Please either convert those cleanup calls to checked warning paths (or a helper) or narrow the new clause so the committed repo satisfies the rule it ships.

Fixed in b34e63e. Swept the whole class rather than the named sites: introduced a discard helper (best-effort remove that warns on failure) and routed every bare cleanup rm through it — scaffold.sh rollback + success paths (including 460, which you did not name but is the same class and would surface next round), plus run-diagnostics.sh and run-tests.sh. Under set -e a failing bare rm on the rollback path would abort before the compile-failure diagnostic; under set +e (the diagnostics gate) it would drop the tmpfile with no signal. Both now warn. grep -rnE "^\s*rm(dir)? " skills/ scripts/ | grep -v /tests/ shows only the already-guarded rmdir ... || echo remaining.

Both verify scripts declared err_file as a main-local, but the EXIT trap fires after main returns — on the normal success path the handler saw an empty variable and leaked the tempfile every run; only the exit-from- inside-main error paths cleaned up. Moved err_file to script scope so the trap always sees it. Flagged by Copilot on #184. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… silence The bare cleanup rm sites the || true sweep left untouched (scaffold rollback + success paths, run-diagnostics, run-tests) route through a discard helper that warns on a failed remove instead of aborting the operation under set -e or continuing in silence under set +e. Completes the Post-Edit Rule Audit for the new best-effort-warns clause. Flagged by the OpenAI policy reviewer on #184. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Split the folded scope/example clauses out of both Shell Error Handling carve-out leads into their own "Applies when ..." bullets, matching the Outer-Boundary carve-out template and rules/context-writing-style.md. Flagged by the OpenAI policy reviewer on #184. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions

Policy loaded: 23 rule files from rules/ (PR head).

All rules pass — no violations found.

Reviewed the shell-error-handling rule addition and the changed scripts/skills against the PR-head policy, including self-consistency of the new carve-outs, explicit exit-code handling, cleanup traps, and skill/script contract updates.

Generated by PR Policy Review (OpenAI) for #184 · 100.4 AIC · ⊞ 15.5K

Superseded by a later all-clear review from the same bot — dismissed by the release skill so the stale request stops gating the merge.

Copilot AI review requested due to automatic review settings July 16, 2026 20:59

Copilot started reviewing on behalf of jbaruch July 16, 2026 20:59 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 16, 2026

View reviewed changes

jbaruch force-pushed the fix/shell-error-handling branch from c7f2b00 to b338b5d Compare July 17, 2026 02:06

jbaruch requested a review from Copilot July 17, 2026 02:10

Copilot started reviewing on behalf of jbaruch July 17, 2026 02:10 View session

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

Copilot AI reviewed Jul 17, 2026

View reviewed changes

jbaruch requested a review from Copilot July 17, 2026 02:15

Copilot started reviewing on behalf of jbaruch July 17, 2026 02:16 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch and others added 3 commits July 16, 2026 21:27

jbaruch force-pushed the fix/shell-error-handling branch from 7f0ab3c to 9942904 Compare July 17, 2026 02:28

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch requested a review from Copilot July 17, 2026 02:36

Copilot started reviewing on behalf of jbaruch July 17, 2026 02:36 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch commented Jul 17, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch requested a review from Copilot July 17, 2026 03:19

Copilot started reviewing on behalf of jbaruch July 17, 2026 03:20 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread skills/release/verify-publish-landed.sh Outdated

Comment thread skills/release/verify-moderation-cleared.sh Outdated

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch requested a review from Copilot July 17, 2026 22:32

Copilot started reviewing on behalf of jbaruch July 17, 2026 22:33 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

github-actions Bot previously requested changes Jul 17, 2026

View reviewed changes

jbaruch and others added 3 commits July 17, 2026 18:46

github-actions Bot reviewed Jul 17, 2026

View reviewed changes

jbaruch merged commit e21d457 into main Jul 17, 2026
16 checks passed

jbaruch mentioned this pull request Jul 18, 2026

feat(ci): native tessl out-of-credits tolerance for the skill-review action (#188) #190

Merged

5 tasks


		## Shell Error Handling

		- Every shell script opens with `set -euo pipefail`

		@@ -44,14 +44,21 @@ set -euo pipefail
		# `\b` GNU extension) and `-i` so capitalized "Tile" is not missed. Emits a
		# JSON array of matching file paths (relative, sorted) on stdout. Always 0.

		echo "migrate.sh: residual-file scan failed (grep rc=${grep_rc}) — the tree could not be fully read, so a 'no residual files' result cannot be trusted; check directory permissions, then re-run" >&2
		return 2

Conversation

jbaruch commented Jul 16, 2026

Summary

The three patterns, and what || true was actually hiding

Best-effort sites keep their leniency

Ten sites were never violations

Test plan

Scope notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

The three patterns, and what `|| true` was actually hiding