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

[jbaruch]

Author-Model: claude-opus-4-8

Summary

Implements the Ask in #188: the skill-review action gains an opt-in, fail-safe credit-outage: fail | skip input so consumers stop vendoring the whole composite just to survive a tessl billing 403. jbaruch/nanoclaw-travel had copied action.yml verbatim (pinned b63f13e) only to wrap the review call — exactly the drift owning the logic here is meant to prevent.

This is also the PR that unblocks this repo: #184 merged but its publish failed at the skill-review step with 403 — organization has run out of credits. With credit-outage: skip wired into publish.yml, that outage skips the affected skill (flagged) instead of blocking every skill-changing merge until credits reset.

What changed

• credit-outage: fail | skip input (default fail — preserves today's Mandatory Review gate exactly).
• Composite body extracted from an inline run: block into a real, unit-tested review-skills.sh, invoked via bash "$GITHUB_ACTION_PATH/review-skills.sh" (no exec-bit dependency). Per rules/script-delegation.md — the credit-vs-real-failure classification is deterministic logic that belongs in a tested script, not an untested YAML heredoc.
• Fail-safe, precise detection (the skill-review action: native tolerance for a tessl 'out of credits' (403) outage #188 follow-up fix): skip only when the review output carries the credit phrase AND a 403, both as fixed strings (grep -F). A real failure that merely quotes "run out of credits" in prose, a different 403, or any future tessl wording change all fall through to hard-fail — the safe direction. Everything non-credit (below-threshold score, auth error, tooling bug) still hard-fails in both modes.
• unreviewed-skills output + a $GITHUB_STEP_SUMMARY note naming every skipped skill, so an unreviewed publish is never silent and consumers can gate follow-up steps.
• context-artifacts Credit-Outage Review Carve-Out documenting the sanctioned skip: opt-in, fail-safe, flagged, self-healing — mirrors the ci-safety Publish-Pipeline carve-out and is explicitly distinct from the forbidden review-step bypass.
• run-diagnostics.sh now shellchecks .github/actions/ so the extracted script is held to the repo's zero-findings bar.
• publish.yml opts into credit-outage: skip (dogfooding).

Test plan

• New .github/actions/skill-review/tests/test_review_skills.sh — 25 assertions, mocked tessl: fail+credits hard-fails; skip+credits tolerates and records; skip+below-threshold hard-fails; skip+phrase-without-403 and skip+403-without-phrase both hard-fail (the precision fix); mixed run records only the outage skill; deleted skill never calls tessl; invalid credit-outage exits 2.
• shellcheck clean on review-skills.sh + the test; run-diagnostics.sh (shellcheck + pyright) clean including the new .github/actions/ root.
• Full suite {"suites":18,"passed":18,"failed":0} (was 17 — the new suite is discovered by run-tests.sh).
• action.yml + publish.yml parse as valid YAML; $GITHUB_ACTION_PATH is the correct composite-action sibling-path variable.
• Paired cross-family policy review + Copilot.

Out of scope (deliberately)

The broader direction in #188's first follow-up — making the Mandatory Review gate satisfiable by a credit-free reviewer (promote the gh-aw policy pass, or a Claude-API rubric) to remove the tessl-credit dependency entirely — is a larger design change. This PR delivers the outage tolerance + the detection-precision fix; the credit-free-gate discussion stays open for a separate follow-up. Not using a Closes #188 keyword for that reason.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR updates the repo’s internal skill-review composite GitHub Action to optionally tolerate a tessl “out of credits” (403) billing outage without forcing consumers to vendor the action, while keeping the Mandatory Review gate intact by default and documenting the narrow carve-out.

Changes:

• Add credit-outage: fail | skip input (default fail) and surface skipped skills via a new unreviewed-skills output plus run-summary warnings.
• Extract the composite’s inline review logic into a dedicated, testable script (review-skills.sh) with deterministic “credit outage” classification.
• Extend diagnostics to shellcheck .github/actions/, and dogfood credit-outage: skip in publish.yml.

Reviewed changes

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

Show a summary per file

File	Description
scripts/run-diagnostics.sh	Includes .github/actions/ in shell script discovery so action scripts are held to the same shellcheck standard.
rules/context-artifacts.md	Documents the opt-in, narrow Credit-Outage Review Carve-Out under Mandatory Review.
CHANGELOG.md	Records the new carve-out and the skill-review action’s credit-outage tolerance feature.
.github/workflows/publish.yml	Opts the repo publish workflow into credit-outage: skip (dogfooding).
.github/actions/skill-review/tests/test_review_skills.sh	Adds outcome-based tests validating the fail-safe outage detection and behavior.
.github/actions/skill-review/review-skills.sh	New action implementation script: identifies changed skills, runs reviews, classifies/records credit outages, emits outputs.
.github/actions/skill-review/action.yml	Adds the new input/output and delegates execution to review-skills.sh with step output wiring.

---

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

[jbaruch]

Fixed in 0cf191f — corrected to "names" ('a warning plus a note' takes a singular verb).

[github-actions]

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

All rules pass — no violations found.

Reviewed the declared claude-opus-4-8 author model via the resolver, loaded the full PR-head policy set, and checked the changed action, shell script, test harness, workflow, changelog, diagnostics runner, and context-artifacts rule update against the in-tree rules. No inline findings.

Generated by PR Policy Review (OpenAI) for #190 · 32 AIC · ⌖ 7.27 AIC · ⊞ 18.7K

[github-actions]

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

Requesting changes because the new credit-outage review path is covered by a test harness whose setup can continue after fixture creation failures under set +e, and one of the advertised precision tests does not exercise the exact credit phrase matched by production. These violate rules/error-handling.md setup-failure handling and leave the rules/testing-standards.md coverage for the core fail-safe classifier incomplete.

Generated by PR Policy Review (OpenAI) for #190 · 37.5 AIC · ⌖ 3.18 AIC · ⊞ 15.5K

[github-actions]

rules/error-handling.md Shell Error Handling says: "A setup step whose failure would silently corrupt the run rather than loudly abort it ... carries its own explicit failure check." This harness has already switched to set +e, so if mktemp fails the run can continue with an empty/bad fixture path and the later setup/assertions no longer mean what they report. Please check the setup explicitly, e.g. FIXTURE=$(mktemp -d -t skill-review-test.XXXXXX) || { echo "fatal: could not create fixture dir" >&2; exit 2; }, and do the same for fixture mkdir/file creation or temporarily keep setup under set -e.

[jbaruch]

Fixed in fb226c4 — the fixture mktemp and every mkdir/file-create now hard-exit on failure, so a corrupted fixture aborts loudly instead of running assertions that no longer mean what they report.

[github-actions]

rules/testing-standards.md Coverage says: "Every module gets tests — no untested code ships." This case is meant to prove the credit phrase alone does not trigger the skip, but the mock emits ran out of credits while production is_credit_outage searches for the fixed string run out of credits. If the implementation accidentally stopped requiring 403 and matched only the real credit phrase, this test would still pass. Please make this fixture use the exact matched phrase without 403, for example Your organization has run out of credits..

[jbaruch]

Fixed in fb226c4 — good catch: the mock said "ran out of credits" so it never hit production's fixed-string "run out of credits" and passed for the wrong reason. It now emits the exact phrase without a 403, so dropping the 403 conjunct would flip this case to a wrongful skip and fail the test.

[github-actions]

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

Requesting changes because the PR extracts the Mandatory Review action body into a new script, but the submitted tests cover only the credit-outage review loop and leave the script’s discovery/main paths untested. rules/testing-standards.md requires modules to ship with tests, and these untested paths are the gate-critical logic for changed-skill detection, review-all fallback, unreachable-base failure, and action output emission.

Generated by PR Policy Review (OpenAI) for #190 · 44.6 AIC · ⌖ 9.84 AIC · ⊞ 15.5K

[github-actions]

rules/testing-standards.md says: "Every module gets tests — no untested code ships." This new script is now the module that enforces the Mandatory Review gate, but the added tests only drive run_reviews; they explicitly leave skill discovery (git diff / find) and the main output path unexercised. That leaves the load-bearing paths for "review all" fallback, changed-skill detection, unreachable-base hard-fail, and unreviewed-skills output writes without coverage. Please add focused tests for identify_skills/main around those cases so the extracted action body is covered end to end.

[jbaruch]

Fixed in c3ce90d — added coverage for the gate-critical paths: identify_skills review-all fallback, git-diff changed-skill detection (against a throwaway git tree), unreachable-base hard-fail (exit 2), and main's GITHUB_OUTPUT emission (both a credit-skip and a clean run). Made review-all discovery portable (find|sed, not GNU -printf) so those paths run off the CI runner too. 30 assertions, diagnostics clean.

[github-actions]

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

Requesting changes for two test-harness comment issues under rules/boy-scout.md and rules/language-diagnostics.md: the added file’s overview is stale about discovery coverage, and its ShellCheck suppression carries a false rationale. The implementation path otherwise follows the new credit-outage carve-out shape I checked: opt-in skip, credit phrase plus 403, non-credit failures still hard-fail, warnings/summary/output are emitted for skipped skills, and diagnostics now cover .github/actions/.

Generated by PR Policy Review (OpenAI) for #190 · 46.9 AIC · ⌖ 7.62 AIC · ⊞ 15.5K

[github-actions]

rules/boy-scout.md says the rule applies to "stale comments". This header now says skill discovery "is not exercised here", but the same added test file exercises identify_skills starting below. Please update this overview to describe the current coverage, or remove the stale discovery paragraph.

[jbaruch]

Fixed in 34fcd10 — rewrote the header overview to describe the actual coverage (run_reviews classification, identify_skills discovery/base-failure, main output emission); the stale "discovery not exercised" paragraph is gone.

[github-actions]

rules/language-diagnostics.md requires that "A diagnostic is fixed, or suppressed inline with a stated reason" and that "The cause rides next to the suppression". This suppression's reason names a run() dispatcher, but this harness has no run() function and the test cases are invoked directly. Please remove the unnecessary suppression or replace it with a true, specific reason for the diagnostic being suppressed.

[jbaruch]

Fixed in 34fcd10 — that SC2329 line was copied from another harness that does use a run() dispatcher; this one doesn't. Removed the file-top suppression and put a targeted SC2329 disable on the tessl mock instead, with the real reason: it's invoked only from run_reviews inside the sourced script, which shellcheck can't trace.

[github-actions]

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

Requesting changes for rule-prose self-consistency: the new Credit-Outage Review Carve-Out in rules/context-artifacts.md adds rationale/contrast prose that rules/context-writing-style.md requires rule bodies to cut or move to CHANGELOG.md. The executable action and tests otherwise line up with the declared fail-safe skip contract from the portions reviewed.

Generated by PR Policy Review (OpenAI) for #190 · 43.8 AIC · ⊞ 14.6K

[github-actions]

rules/context-writing-style.md says: "Meta-justification prose explaining why the rule exists — belongs in CHANGELOG" and "Em-dash, colon, and semicolon are not loopholes — using them to attach the same justifying clause is the same violation." This inserted clause is rationale for why the outage is acceptable, not a precondition. Please keep the rule contract here, e.g. Applies when tessl skill review fails with the out-of-credits signature; never a below-threshold score, and leave the motivation in CHANGELOG.md.

[jbaruch]

Fixed in 66d776b — reduced to "Applies when tessl skill review fails with the out-of-credits signature; never a below-threshold score". The "billing state the author cannot fix" motivation now lives only in the CHANGELOG.

[github-actions]

rules/context-writing-style.md says to cut "reinforcement and hedging restatements of conclusions the preconditions already entail" and move explanatory rationale to the changelog. The semicolon contrast (that dodges a verdict; this tolerates...) is explanatory rather than a new enforceable condition. Please rewrite this as a terse reset/constraint, for example: The forbidden review-step bypass in Disagreeing With the Reviewer remains forbidden.

[jbaruch]

Fixed in 66d776b — rewrote as the terse constraint "The forbidden review-step bypass in Disagreeing With the Reviewer remains forbidden." The dodges-a-verdict-vs-tolerates-an-outage contrast is already in the CHANGELOG Rules entry.

[github-actions]

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

Requesting changes for one policy violation in the new test harness: a changed comment contradicts the shell scoping it documents, which falls under rules/boy-scout.md for stale/lying comments. The credit-outage action logic otherwise follows the new carve-out shape I reviewed.

Generated by PR Policy Review (OpenAI) for #190 · 45.3 AIC · ⌖ 3.04 AIC · ⊞ 15.5K

[github-actions]

rules/boy-scout.md says this applies to "stale comments" and "lying type signatures". This comment says the config vars are local, dynamically scoped, and kept out of the global namespace, but the helper below uses export instead; capture_main later does the same while running in the main shell, so those globals do leak. Please either make these variables actually local/dynamically scoped (for example with function-local assignments if shellcheck stays clean) or rewrite the comments to describe the export/subshell behavior without claiming local scope/no global leakage.

[jbaruch]

Fixed in 535320b — the comments were left over from the pre-export version. Rewrote both honestly: they use export (the env contract the action passes, and what keeps shellcheck from reading them as dead); capture_identify's exports stay in its $() subshell, and capture_main's do persist in the current shell but are harmless since those are the final two cases. Kept export over local because local reintroduces the SC2034 the export fixes.

[github-actions]

Policy loaded: 23 rule files from rules/ (PR head).
All rules pass — no violations found.

Reviewed the new credit-outage action input, extracted review-skills.sh, action tests, diagnostics discovery update, publish workflow opt-in, changelog entry, and the new context-artifacts carve-out against the PR-head policy. I found no self-consistency, CI-safety, shell error-handling, testing, secret-handling, or context-artifact violations on the changed lines.

Generated by PR Policy Review (OpenAI) for #190 · 20.4 AIC · ⌖ 8.56 AIC · ⊞ 33.3K

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