feat(ci): add portability-lint gate for ecosystem/forge-agnostic skills (#531)

[kyle-sexton]

Summary

Skills declared ecosystem/forge/tracker-agnostic ship bare hardcoded
stack/forge/branch/tracker defaults because agnosticism was asserted in prose and
never enforced mechanically — the dominant review-churn class, re-caught by the
external reviewer PR after PR (branch/remote hardcodes, seam-conditional eval,
.NET-flavored examples). This adds a fail-closed CI lane that mechanically detects
that coupling instead of paying for it at review time again and again.

Detection only — this PR builds the gate. Fixing the existing violations stays
with the member issues below.

Fix

A new portability-lint lane (.github/workflows/ci.yml) wired into the required
ci-status aggregate, plus a repo-local detector:

• scripts/check-skill-portability.sh — scans skill files for coupling tokens.
  On a PR it scans only the skill files the change touches (mirroring the
  skill-quality-gate changed-diff pattern), so enabling a token class prevents
  new coupling without red-lining pre-existing violations — main's push event
  scans nothing (self-test is the push path), and existing hits wait for their
  owning follow-up fix or the file's next edit.
• scripts/skill-portability-tokens.txt — the token list as external,
  extensible data (not logic buried in bash), so a reviewer re-catch is a
  one-line data edit. Seeded with one active class — the branch/default-branch
  hardcode (origin/main / origin/master) — per the ratified one-token-class-at-
  a-time rollout; further classes (dotnet/Clean Arch, raw.githubusercontent,
  bare gh tracker calls) are staged as commented entries with enable-triggers.
• Never-skip shape: the job is unconditional; only the PR-diff step is
  event-gated, and a self-test step runs first so a broken detector can never mask
  a real violation behind a green gate.

Design decisions (resolved per the ratified plan, not invented):

• How a skill declares agnosticism scope — no new frontmatter field. A skill
  is agnostic by default (the Design boundary already binds every plugin), so the
  gated set is the files a change touches. A hit is excused three ways, all
  reviewer-visible comments (reusing the silent-skip gate's annotated-exemption
  shape): an auto-recognized detection-first / presence-gated use; a per-site
  portability-ok: <reason>; or a whole-file portability-scope: <reason>
  declaring an inherent narrower boundary (the forge-locked-under-a-neutral-name
  case). This distinguishes guarded refs (fine) from bare ones, as the
  guarded-forward-ref disposition requires.
• docs/PLUGIN-PHILOSOPHY.md gains one doctrine sentence extending the existing
  declared-narrower-boundary allowance from OS platform to the
  forge/ecosystem/tracker axis.

No plugins/<name>/ directory is touched, so no version bump / CHANGELOG entry is
needed (matching the CI-gate precedent).

Verification

• Self-test suite (scripts/check-skill-portability.test.sh, runs in CI):
  PASS=12 FAIL=0 — covers bare-token FAIL with file:line, detection-ladder pass,
  same-line and comment-block-above portability-ok, annotation non-leak past
  intervening code, whole-file portability-scope, staged tokens staying inactive
  under the shipped list, fail-closed exit 2 on missing token list / invalid base
  ref, the vendor/evals/*.test.sh exclusion set, and empty scope.

• Catches a real violation, passes a real legit use (the bare-vs-guarded
  discrimination that drives this class), on live corpus files:

  $ scripts/check-skill-portability.sh --paths plugins/work-items/skills/track/actions/start.md
  COUPLING: plugins/work-items/skills/track/actions/start.md:61: origin/(main|master) -> ... git checkout -b <type>/<N>-<slug> origin/main ...
  COUPLING: plugins/work-items/skills/track/actions/start.md:65: origin/(main|master) -> ... git checkout -b <type>/<N>-<slug> origin/main ...
  (exit 1)
  
  $ scripts/check-skill-portability.sh --paths plugins/review/skills/fanout/SKILL.md
  No unexcused coupling tokens in 1 skill file(s).   # detection-first origin/HEAD ladder → guarded
  (exit 0)
  

• Calibrated against the full corpus (--all): the active branch class flags
  exactly 3 genuine bare hardcodes in 2 files (owned by member issues; changed-file
  scoping keeps them off main) and correctly passes the detection-ladder uses in
  review/* and excludes evals/test fixtures.

• Changed-file CI path proven against a real base ref (16 changed skills gated,
  all clean). Local checks green: shellcheck (repo .shellcheckrc), shfmt,
  actionlint, comment-residue detector (T1=T2=T3=0), typos.

Closes #620

Part of #531 (umbrella stays open — stages remain; child #620 carries this PR's shipped scope per the #603 pattern).

Related

• Member coupling instances (this is prevention; it does not block them): implementation: Clean-Architecture layer vocabulary baked as universal execution order #404
  implementation: .NET-only marketplace-skill references in every mode context (origin coupling, low severity) #405 implementation: TDD-by-default fires when consumer CLAUDE.md is silent — no userConfig toggle #406 discovery: ecosystem-discovery table is a closed set with no consumer-extension seam #408 toolchain: publisher org/repo/branch URL propagates into consumer-tracked .claude/ecosystems/*.yaml headers #410 toolchain: dotnet.md references dotnet-msbuild:* plugin skills that do not exist in this marketplace #412 planning: branch-name grammar hardcoded to Conventional Commits types, no consumer seam #415 planning: wayfind hardcodes GitHub Issues/gh as the only tracker backend, bypassing the work-item-tracker seam #416 planning: plan close-out and tracker-write phases emit gh-hardcoded tracker writes; close-out hardcodes docs/adr + docs/specs #418 verification: .NET/Blazor globs shipped as "portable defaults" — silent-skip for non-.NET consumers #421 verification: bare unguarded cross-plugin references to dotnet-*/cloudflare plugins in measure contexts #422 verification: consumer override seams gestured ("when the project documents…") but no declared location exists #423 testing: evals grade against the private source monorepo's layout and house test stack (+ ghost testing.md ref) #428 testing: C#/.NET idioms are the fallback defaults and only worked examples in ecosystem-agnostic skills #429 session-flow: topic-docs placement contract resolved via hardcoded raw.githubusercontent URL instead of bundled copy #432 source-control: babysit worktree pruner hard-depends on ghq (undeclared, author-personal repo-layout tool) #438 source-control: PR-body "Generated with Claude Code" attribution is unconditional — no trailer_policy-equivalent seam #439
  source-control: pull-request + babysit-prs are gh/GitHub-locked with no forge seam, under a forge-neutral plugin name #441 source-control: remote name origin hardcoded in pull-request create flow (low severity) #442
• CI-gate backlog: automatable conformance checks surfaced by the #313 fleet audit #445 — sibling CI-gate backlog (mechanical conformance only; explicitly not
  coupling)
• toolchain: dotnet.md references dotnet-msbuild:* plugin skills that do not exist in this marketplace #412 — guarded-forward-ref disposition the gate honors (guarded ≠ bare)
• source-control: pull-request + babysit-prs are gh/GitHub-locked with no forge seam, under a forge-neutral plugin name #441 — declared narrower-scope exemption the portability-scope mechanism serves
• fix(toolchain): resolve default branch by detection, not assumption #467 — the branch/remote review-churn the seed active class targets
• fix(implementation): defer branch-name grammar to consumer convention #453 — seam-conditional eval re-catch of this class
• fix(testing): guard unguarded dotnet cross-plugin ref, frame .NET enrichment lists as stack-specific #491 fix(verification): neutralize .NET/PowerShell-flavored examples in agnostic skills #492 — .NET-flavored-example hand-fixes a future staged class would obviate
• source-control(ci):portability-lint: pending_annot carry-forward doesn't distinguish inline vs block HTML comments #611 — post-green review finding (annotation-carry pending_annot doesn't distinguish inline vs block HTML comments), deferred out of this PR's scope

🤖 Generated with Claude Code

https://claude.ai/code/session_01KyLj6oaFVTE6xFuoYCC2KC

[claude]

Claude finished @kyle-sexton's task in 4m 27s —— View job

---

Code Review — feat(ci): add portability-lint gate (#531)

• Gather context and understand the request
• Read changed files
• Review CI workflow changes
• Review the portability lint script
• Review the test harness
• Review the token list
• Review docs changes
• Compile and post findings

---

Summary: Solid gate design. Fail-closed, self-testing, data-driven, changed-file-scoped — all the right shapes. One medium finding (silent awk failure creates a false-negative path), three low/cosmetic notes. No blockers; the medium issue is addressable in a follow-up unless you want it fixed before merge.

---

Medium — Silent awk failure is a false negative

scripts/check-skill-portability.sh:183

out="$(scan_file "$file")"
if [[ -n "$out" ]]; then

If the awk command inside scan_file exits non-zero and produces no output (e.g., malformed ERE pattern in the token list, awk internal error), $out is empty and the file is silently treated as clean — a false negative that the "fail-closed" design is supposed to prevent. The awk error would appear in CI logs but wouldn't fail the gate.

The caller checks [[ -n "$out" ]] rather than the exit code of scan_file. Since set -uo pipefail is in effect but -e is not, the non-zero return doesn't abort.

Fix: propagate the exit code:

out="$(scan_file "$file")" || { printf 'Error: gate scanner failed on %s\n' "$file" >&2; exit 2; }

There's a self-test that exercises the gate end-to-end, so a pattern syntax error would be caught at the self-test step before the PR-diff step — which reduces the practical risk here. But a runtime awk failure (e.g., disk I/O error mid-scan) would still produce a false negative. Worth closing. Fix this →

---

Low — Test for annotation non-leak only asserts :4: present, not :2: absent

scripts/check-skill-portability.test.sh:100

The fixture at lines 94–105 puts origin/main on both line 2 (should be excused by the annotation) and line 4 (should be flagged). The test only checks grep -q ":4:". If a regression caused line 2 to also be flagged, the test would still pass — the assert is necessary but not sufficient. Adding ! echo "$out" | grep -q ":2:" would close the gap.

---

Low — Staged gh token uses GNU-only \b word boundary

scripts/skill-portability-tokens.txt:57

# \bgh (issue|api|pr|label)

\b is a GNU ERE extension not available in BSD awk (macOS). Since CI runs on ubuntu-24.04 (GNU awk) this won't break CI, and the pattern is staged/inactive today. But a developer auditing with --all on macOS would get a silent mismatch when this class is activated. Worth noting in the enable-trigger comment or replacing with a POSIX boundary like (^|[^[:alnum:]_])gh when activation arrives.

---

Low — portability-ok inside a multi-line HTML comment body is not recognized

scripts/check-skill-portability.sh:139

function is_comment(l) { return l ~ /^[[:space:]]*#/ || l ~ /
origin/main here

…the portability-ok: line won't be recognized as a comment (it doesn't start with # or contain `` on a single line or # portability-ok: reason, which both work correctly. This is an edge case worth calling out in the script header's exemption docs rather than fixing in awk, since both working forms are already natural Markdown/shell comment styles.

---

Positive notes

• Never-skip shape is correct. The self-test step is unconditional; only the PR-diff step is event-gated. A broken detector can't hide a real violation behind a green gate.
• CI integration is clean. Same checkout SHA (9c091bb... / v7.0.0) and fetch-depth: 0 as existing jobs. portability-lint correctly added to ci-status needs.
• Injection-safe BASE_REF handling. Using env: BASE_REF: ${{ github.base_ref }} and then "origin/$BASE_REF" in shell properly prevents YAML expression injection.
• --diff-filter=d is the right choice. Excludes deleted-source sides of renames without excluding the added-target side — a rename that introduces a coupling token is correctly caught.
• Token list as external data. Separating the patterns from the detection logic means a reviewer re-catch is a one-line data edit, not a gate-logic PR.
• Detection-ladder guard markers cover the real pattern. origin/HEAD, symbolic-ref, merge-base, baseRefName, fallback — these match the recommended idiom and correctly distinguish guarded from bare refs.

[claude]

Claude finished @kyle-sexton's task in 3m 31s —— View job

---

PR Review

• Gather context and understand the request
• Read changed files
• Review CI workflow changes
• Review the portability lint script
• Review the test harness
• Review the token list
• Review docs changes
• Compile and post findings

---

This is a solid, well-designed gate. The architecture is sound and the implementation is clean. Three things worth flagging before merge: one is a real concern about future-class breakage, one is a correctness edge case in the guard-marker logic, and one is a low-priority note on staged patterns.

---

\b in staged patterns is non-POSIX ERE

File: scripts/skill-portability-tokens.txt L46–57

# dotnet
# \bClean Arch(itecture)?\b
# \bgh (issue|api|pr|label)

Both staged patterns use \b (word boundary). In POSIX ERE — which is what awk's ~ operator uses — \b is not a word boundary; it matches a literal backspace character (0x08). On gawk (the default on ubuntu-24.04), \b happens to work as a word boundary as a gawk extension, but the script doesn't pin to gawk (#!/usr/bin/awk or gawk invocation). If CI ever runs on an image where the default awk is mawk or nawk, these patterns will silently match backspace characters rather than word boundaries — meaning they'd never fire, not error, making the gate appear green when it isn't.

This isn't a blocker today (both patterns are commented out and their enable-triggers require corpus audits first), but document it in the comment above each staged pattern so whoever enables them knows to either test the awk version or rewrite to POSIX-safe word boundary equivalents (e.g., (^|[^a-zA-Z0-9_])dotnet([^a-zA-Z0-9_]|$) or just [Dd]otnet). Fix this →

---

Generic guard markers will misfire when new token classes are added

File: scripts/check-skill-portability.sh L143–147

function is_guarded(l) {
  return l ~ /origin\/HEAD/ || l ~ /symbolic-ref/ || l ~ /merge-base/ ||
    l ~ /baseRefName/ || l ~ /[Ff]allback/ || l ~ /[Ff]alling back/ ||
    l ~ /-> *origin\// || l ~ /if installed/ || l ~ /if using/ ||
    l ~ /when installed/ || l ~ /when present/
}

The last four markers (if installed, if using, when installed, when present) are presence-guard signals for cross-plugin optional-dependency patterns, not branch-resolution signals. When the dotnet or gh tracker token classes are enabled, a line like "useful if using .NET — requires dotnet CLI" would be marked as guarded even though it's a bare ecosystem hardcode. The script header does note these markers are for the seed class only and would be "revisited here" per class, but the revisit step isn't enforced by the token-class enable-trigger comment. Consider adding a note to the staged-class comment blocks reminding enablers to audit is_guarded against the new class before activating.

---

is_comment matches " is both is_comment() true and is_annotated() true, meaning a subsequent violation on the next line gets pending_annot = 1 from this line. In practice this is fine — inline annotations on the same line are already handled by the is_annotated(line) check at the top of the violation guard — but it means a content line with an inline HTML comment also acts as an "annotated comment" for pending_annot carry-forward purposes. Low likelihood in real skill files, and the self-test doesn't cover it. Worth anchoring to l ~ /^[[:space:]]*<!--/ if you want the carry-forward to require a block-level comment, not an inline annotation on a content line.

---

Everything else looks correct

• Never-skip shape: Job is unconditional, self-test runs first, only the PR-diff step is event-gated. This correctly mirrors the skill-quality-gate pattern. ✓
• ci-status wiring: portability-lint is in the needs list at line 438. ✓
• fetch-depth: 0: Required for git diff ... "$base" in the PR-diff step. Present. ✓
• Fail-closed environment errors: Missing token list → exit 2; invalid base ref → exit 2. Both covered by tests. ✓
• --diff-filter=d: Excludes deleted files from the changed-file list; the [[ -f "$f" ]] check on line 118 is belt-and-suspenders. ✓
• Token list as external data: The SKILL_PORTABILITY_TOKENS override is correctly threaded through all modes. ✓
• Test suite scope: 12 tests cover all claimed scenarios including the staged-inactive case, exclusion set, empty scope, and both fail-closed paths. The real-corpus cases (bare vs. guarded discrimination) are manual verification documented in the PR body rather than automated, which is appropriate for live-corpus tests. ✓
• portability-scope whole-file escape: grep -qE 'portability-scope:' runs before the awk scan and short-circuits the whole file — correct. ✓
• pending_annot annotation semantics: Carries through contiguous comment lines, resets at non-comment lines. The annotation-leak test at line 93–105 confirms the intended behavior. ✓
• Docs change: The 8-line addition to PLUGIN-PHILOSOPHY.md is crisp and correctly generalizes the declared-narrower-boundary allowance to the forge/ecosystem/tracker axis. ✓

[kyle-sexton]

Review classification (work-2 lane, post-green pass)

#	Finding	Classification	Disposition
1	\b in staged (inactive) token patterns is non-POSIX ERE (gawk extension); silent no-op on mawk/nawk if enabled without testing	VALID	Fixing inline — warning comment added above the staged patterns
2	Generic is_guarded() markers (if using, when installed, etc.) will over-match once dotnet/gh-tracker staged classes activate, masking real hardcodes	VALID	Fixing inline — reminder comment added to the staged-class blocks
3	is_comment() matches inline <!-- --> on a content line, so pending_annot can carry forward from a non-block-comment line	VALID, deferred	Filed #611 — low-likelihood edge case, not exercised by current active token class or self-test; fixing here would add script-logic + test scope beyond this PR's ship boundary

Findings 1–2: fix in progress on this branch (comment-only additions, no behavior change). Finding 3: tracked in #611, cited in ## Related.

[claude]

Claude finished @kyle-sexton's task in 2m 3s —— View job

---

PR Review — docs(ci): warn about POSIX-ERE and guard-marker gaps (follow-up commit)

• Gather context and understand the request
• Read the latest diff to understand what changed in the new commit
• Verify the fixes address findings 1 and 2 from prior review
• Check for any new issues introduced
• Post findings

---

The latest commit (72ca25f) is comment-only — 16 lines added to the STAGED section preamble of scripts/skill-portability-tokens.txt, no behavior or test changes. It addresses the two prior review findings that kyle-sexton classified as VALID and committed to fixing inline.

Finding 1 (generic is_guarded() markers over-match on new classes) — Addressed ✓

The new preamble directly names the problematic markers:

"the generic presence signals (if using, when installed, when present) would spuriously guard a bare ecosystem hardcode like 'useful if using .NET — requires dotnet CLI' once the dotnet or gh class is live — a hardcode, not a legitimately guarded use"

This is exactly the right framing and will catch any future maintainer trying to activate a staged class without auditing is_guarded() first. The instruction ("Audit and tighten is_guarded() in check-skill-portability.sh against the class's real false-guard risk") is specific and actionable.

Finding 2 (\b non-POSIX ERE) — Addressed ✓

The new preamble covers the \b issue accurately:

"POSIX ERE defines no word-boundary escape, and on common awks (gawk AND mawk/nawk) \b matches a literal backspace byte"

One minor technical nuance: gawk's default mode does extend \b as a word boundary (it's a gawk extension, not POSIX), so the claim that \b matches a literal backspace byte on gawk may be slightly imprecise — gawk uses \y for word boundaries when in POSIX compatibility mode, but supports \b as a boundary extension otherwise. However, the author notes this was verified on GNU Awk 5.4.0, and the bottom line advice ("Rewrite to a POSIX-safe boundary before activating") is the correct and portable guidance regardless of the exact mechanism. The suggested POSIX-safe alternative (^|[^a-zA-Z0-9_])dotnet([^a-zA-Z0-9_]|$) is correct.

This nuance doesn't affect the PR's correctness — the warning is directionally right and the recommended fix is sound.

No new issues introduced

The commit is purely additive prose in the data file. No logic, patterns, or tests were changed. The self-test still passes 12/12 per the commit message.

Overall

The two prior VALID findings are now addressed. Finding 3 (inline HTML comment pending_annot edge case) was correctly deferred to #611 per the review classification — it's low-likelihood and out of scope for this gate's active token class. The full PR is ready to merge.

[claude]

Claude finished @kyle-sexton's task in 2m 37s —— View job

---

PR Review — feat(ci): add portability-lint gate (#531)

• Gather context and understand the request
• Read changed files
• Review CI workflow changes
• Review the portability lint script
• Review the test harness
• Review the token list and follow-up commit
• Compile and post findings

---

This is a well-constructed gate. Fail-closed, self-testing before the PR-diff step, data-driven, changed-file-scoped — all the right shapes. The follow-up commit (72ca25f) correctly addressed the \b non-POSIX-ERE risk and the is_guarded() over-match risk for future staged classes. Two earlier findings remain open and are worth tracking before merge: one medium (false-negative path in the scanner loop) and one low (incomplete assertion in the annotation-non-leak test).

---

Medium — Silent scan_file failure produces a false negative

scripts/check-skill-portability.sh:183

out="$(scan_file "$file")"
if [[ -n "$out" ]]; then

set -uo pipefail is in effect but set -e (errexit) is not. When scan_file exits non-zero and produces no stdout — e.g., due to a disk I/O error mid-scan, or an internal awk fault — $out is empty and the file is silently treated as clean. The caller only checks [[ -n "$out" ]], not the return code.

The self-test step running before the PR-diff step mitigates the malformed-pattern variant of this (a broken token pattern would surface in CI at the self-test stage), but a runtime awk failure during an actual PR scan would still yield a green gate on a file that was never actually checked — exactly what the fail-closed design is supposed to prevent.

Fix:

out="$(scan_file "$file")" || { printf 'Error: gate scanner failed on %s\n' "$file" >&2; exit 2; }

This finding was flagged in the first review pass and not included in the author's three-item classification table (which addressed findings from the second review). It remains open. Fix this →

---

Low — Annotation-non-leak test only asserts line 4 is flagged, not that line 2 is clean

scripts/check-skill-portability.test.sh:100

The fixture places origin/main on both line 2 (annotated, should pass) and line 4 (unannotated, should fail). The test asserts :4: appears in the output but does not assert :2: is absent. A regression that caused line 2 to also be flagged would not be caught:

elif echo "$out" | grep -q ":4:"; then
  ok "annotation does not leak past intervening code"

Adding && ! echo "$out" | grep -q ":2:" to the condition closes the gap. This finding was also flagged in the first review and remains unaddressed.

---

Addressed findings confirmed ✓

The follow-up commit (72ca25f) correctly addresses both VALID findings from the second review pass:

• \b non-POSIX ERE: The new preamble in scripts/skill-portability-tokens.txt accurately explains the risk (backspace-byte match on mawk/nawk, gawk's \y vs. \b extension), names the concrete test result (\bdotnet\b matches nothing on gawk 5.4.0), and provides the correct POSIX-safe rewrite (^|[^a-zA-Z0-9_])dotnet([^a-zA-Z0-9_]|$).

• is_guarded() generic presence signals: The preamble explicitly calls out if using, when installed, when present as markers that would spuriously guard a bare ecosystem hardcode, and directs anyone enabling a staged class to audit and tighten is_guarded() first. Specific, actionable, visible to maintainers at the right moment.

The is_comment() / inline HTML pending_annot edge case (second review finding 3) was correctly classified as low-likelihood and deferred to #611.

---

Everything else looks correct

• Never-skip shape: portability-lint job is unconditional; self-test step is unconditional; only the PR-diff step is event-gated (if: github.event_name == 'pull_request'). A broken detector cannot hide behind a green gate. ✓
• ci-status wiring: portability-lint is in the needs list at the aggregate job. ✓
• Injection-safe BASE_REF: Passed via env: BASE_REF: ${{ github.base_ref }} then expanded as "origin/$BASE_REF" in shell — correct. ✓
• fetch-depth: 0: Required for the git diff "$base" changed-file path. Present with the same pinned checkout SHA as existing jobs. ✓
• --diff-filter=d + [[ -f "$f" ]] belt-and-suspenders: deleted files correctly excluded from scan. ✓
• Fail-closed environment errors: Missing token list → exit 2; invalid base ref → exit 2; both covered by tests. ✓
• Staged tokens inactive: Test case confirms dotnet and github.com/ghraw don't fire under the shipped token list. ✓
• portability-scope whole-file escape: grep -qE runs before awk and short-circuits the entire file. ✓
• Docs change: The 8-line addition to PLUGIN-PHILOSOPHY.md correctly generalizes the declared-narrower-boundary allowance from OS platform to the forge/ecosystem/tracker axis and names the mechanical gate as the enforcement mechanism. ✓
• Token list as data: External, extensible, reviewer-editable — a re-catch is a one-line data change. ✓

---

Summary: Two prior findings remain open — the medium awk-exit false-negative path (line 183 of the script) and the incomplete test assertion (line 100 of the test). Both are straightforward one-liners to fix. The overall design is sound and the 72ca25f follow-up is clean. Recommend addressing the medium finding before merge; the low test assertion can land as a follow-up if preferred.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 72ca25ffa6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 50s —— View job

---

PR Review — fix(ci): fail closed on scanner fault and restrict branch guards (#531)

• Gather context and understand the request
• Read the latest commit diff (c755ec6)
• Verify fix for silent scan_file failure (medium finding)
• Verify fix for is_guarded() over-matching generic markers
• Verify annotation-non-leak test assertion (low finding)
• Check for new issues introduced
• Compile and post findings

---

The latest commit (c755ec6) closes all three remaining open findings from prior review passes. No new issues introduced. Here's the verification of each.

---

Finding 1 (medium) — Silent scan_file failure → false negative: Fixed ✓

scripts/check-skill-portability.sh:190–193

out="$(scan_file "$file")" || {
    printf 'Error: gate scanner failed on %s — failing closed\n' "$file" >&2
    exit 2
}

Correct. scan_file ends with awk, so an awk fault (e.g., malformed ERE) propagates directly as the function's return code. The || branch fires, prints the right diagnostic, and exits 2. The fix handles all three fault modes: malformed token in the token list, runtime I/O error mid-scan, and awk internal error.

The regression test at scripts/check-skill-portability.test.sh:85–95 writes origin/(main (unmatched paren — invalid ERE) as the active token, calls the gate with --paths, and asserts exit 2. The test drives the SKILL_PORTABILITY_TOKENS override path and verifies the fail-closed contract end-to-end.

---

Finding 2 (Codex P2) — Generic presence markers if using / when present guard bare branch defaults: Fixed ✓

scripts/check-skill-portability.sh:147–151

function is_guarded(l) {
  return l ~ /origin\/HEAD/ || l ~ /symbolic-ref/ || l ~ /merge-base/ ||
    l ~ /baseRefName/ || l ~ /[Ff]allback/ || l ~ /[Ff]alling back/ ||
    l ~ /-> *origin\//
}

The four presence-prose markers (if installed, if using, when installed, when present) are removed. All remaining markers are branch-detection evidence: origin/HEAD, symbolic-ref, merge-base, baseRefName, and the fallback / arrow forms that document a last-resort resolution. The removal is exactly scoped — no collateral damage to the branch-detection markers.

The regression test at scripts/check-skill-portability.test.sh:74–83 exercises 'If using a rebase workflow, diff origin/main first.' and asserts it now produces COUPLING: ${f}:1: (exit 1). The test verifies both the exit code AND the flagged line number, which is the right shape.

The skill-portability-tokens.txt preamble is updated from "Audit and tighten" to "Audit and EXTEND", accurately reflecting that is_guarded() is already scoped and a new class adds markers rather than narrowing them.

---

Finding 3 (low) — Annotation non-leak test only asserted line 4 present, not line 2 absent: Fixed ✓

scripts/check-skill-portability.test.sh:123

elif echo "$out" | grep -q ":4:" && ! echo "$out" | grep -q ":2:"; then
  ok "annotation covers line 2 only and does not leak past intervening code"

Both sides of the assertion are now present: line 4 must appear (the unannotated violation is flagged) and line 2 must not appear (the annotated violation is excused). A regression that caused the annotated line to also be flagged would now fail the test.

---

Suite count

The commit message states 14/14. Two new tests (generic-presence-prose and malformed-active-token) were added to the prior 12 — consistent with the count.

---

No new issues

• The $? capture pattern in the malformed-token test (run command, then if [[ "$?" -eq 2 ]] on the next line) is correct: nothing else runs between the subprocess and the if, so $? captures the right exit status.
• The scan_file function's control flow is sound in all four cases: portability-scope declared (early return 0), awk exits 0 with output (violations returned), awk exits 0 with no output (clean), awk exits non-zero (now caught by ||).
• The is_comment() / inline-HTML pending_annot edge case (finding 3 from the second review pass) remains deferred to source-control(ci):portability-lint: pending_annot carry-forward doesn't distinguish inline vs block HTML comments #611 as classified — not touched here, not a regression.

---

All prior open findings are resolved. The gate now fails closed on scanner faults, restricts branch guards to detection evidence only, and the test assertions are sufficient on both the positive and negative sides. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c755ec642e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 33s —— View job

---

PR Review — fix(ci): drop bare-word "fallback" branch guard (8caf57a)

• Gather context and understand the request
• Read the latest commit diff (8caf57a)
• Verify fix for bare-word fallback guard (Codex P2 finding)
• Verify existing guarded cases still pass after marker removal
• Check for new issues introduced
• Compile and post findings

---

The latest commit (8caf57a) addresses the final open Codex P2 finding — bare-word fallback / falling back as a guard marker suppressing real violations when used in unrelated prose. Everything else carries forward from the prior passing review.

Codex P2 — Bare-word fallback guard: Fixed ✓

scripts/check-skill-portability.sh:155–157

function is_guarded(l) {
  return l ~ /origin\/HEAD/ || l ~ /symbolic-ref/ || l ~ /merge-base/ ||
    l ~ /baseRefName/ || l ~ /-> *origin\//
}

[Ff]allback and [Ff]alling back are removed. The five remaining markers are all genuine branch-resolution evidence — commands that mechanically resolve the branch at runtime rather than prose that merely signals intent. The new comment block explains the principle clearly: the resolution command is the evidence; prose alone is not; a legitimately split-across-lines ladder uses the per-site portability-ok escape.

The new regression test at scripts/check-skill-portability.test.sh:85–94 exercises exactly the Codex scenario (As a fallback for a network timeout, run git diff origin/main.) and asserts COUPLING: ${f}:1:. Correct shape — checks both exit code and the specific flagged line.

Existing guarded cases unaffected ✓

The plugins/review/skills/fanout/SKILL.md detection-ladder case — the "guarded" reference cited in the PR body verification — still passes after the removal:

• Line 14 has origin/HEAD co-located with origin/main on the same line. origin\/HEAD guard still present. ✓
• Line 27 has merge-base and baseRefName co-located with origin/main. Both guards still present. ✓

The previous review's regression test for "generic presence prose" (if using a rebase workflow, diff origin/main) also confirms the test suite was already exercising the non-prose discipline; the fallback removal is consistent with that existing guard.

Suite count

15 tests: the prior 14, plus "standalone 'fallback' prose does not guard the active branch token" added here. Commit message states PASS=15 FAIL=0 — consistent.

No new issues introduced

• The comment documentation update in the script header (lines 34–39) correctly generalizes the escape rule: "the resolution command is the evidence, not the surrounding prose." This matches the implementation.
• The error output change (line 211: "co-locate the branch-resolution command on the line (detection-first)") gives consumers actionable guidance aligned with the new, stricter contract.
• The design is now more principled than the prior version: co-location means the reviewer and the scanner see the same evidence on the same line. A legitimately multi-line ladder uses portability-ok:, which is reviewer-visible — exactly the right escape hatch.

All prior findings resolved

Finding	Source	Status
Silent scan_file failure → false negative	First review (medium)	Fixed in c755ec6 ✓
is_guarded() generic presence markers over-match	Codex P2 / second review	Fixed in c755ec6 ✓
Annotation non-leak test missing negative assertion	First/third review (low)	Fixed in c755ec6 ✓
\b non-POSIX ERE in staged patterns	Second review / Codex	Documented in 72ca25f ✓
is_comment() inline HTML pending_annot carry-over	Second review (deferred)	Tracked in #611 ✓
Bare-word fallback guards unrelated prose	Codex P2 (c755ec6 round)	Fixed here in 8caf57a ✓

Ready to merge. The gate is fail-closed, self-testing before the PR-diff step, data-driven, changed-file-scoped, and now guards against all three reviewer-identified false-negative paths (scanner fault, generic-presence over-match, bare-word prose guard). The one deferred edge case (#611) is correctly out of scope for the initial gate's active token class.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8caf57a1b6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 1s —— View job

---

PR Review — fix(ci): read changed paths NUL-delimited so Git-quoted names aren't dropped (15d8399)

• Gather context and understand the request
• Read the latest diff to understand what changed in the new commit (15d8399)
• Verify the fix addresses the Codex P2 finding (NUL-delimited git path reading)
• Verify existing behavior is preserved
• Check for any new issues introduced
• Post findings

---

The latest commit (15d8399) closes the final open Codex P2 finding — Git-quoted pathnames (non-ASCII bytes, literal backslashes, literal double-quotes) silently excluded from the PR-diff gate. Everything else carries forward from the prior passing review.

Codex P2 — Git-quoted path silently dropped: Fixed ✓

scripts/check-skill-portability.sh:119–127

while IFS= read -r -d '' f; do
  case "$f" in
  plugins/*/skills/*) ;;
  *) continue ;;
  esac
  is_scannable "$f" || continue
  [[ -f "$f" ]] || continue
  files+=("$f")
done < <(git diff --name-only --diff-filter=d -z "$base" -- 'plugins/' | sort -z -u)

Three changes, all correct and consistent:

• --name-only → --name-only -z: Git emits paths verbatim, NUL-terminated, bypassing the C-style quoting that wrapped non-ASCII names in double quotes and broke the glob.
• sort -u → sort -z -u: NUL-aware deduplication; the stream stays NUL-delimited through the pipe.
• read -r f → read -r -d '' f: Reads until NUL instead of newline; -r still prevents backslash interpretation.

This is the canonical POSIX-safe pattern for reading arbitrary filenames from git output. All three mutation sites are coordinated — mixing -z output with newline read (or vice versa) would break silently, and the author got all three right.

The --all and --paths modes are correctly left untouched: find outputs filenames verbatim (no C-quoting), and --paths takes explicit arguments, neither of which has the quoting issue.

Test adequacy ✓

scripts/check-skill-portability.test.sh:209–245

The regression test is the first coverage of the changed-file (base-ref) mode's file-collection path. It:

• Creates a scratch git repo with one ASCII-named and one non-ASCII-named skill file (both containing the active coupling token)
• Diffs against the empty base commit
• Asserts exit non-zero and plain.md:1: present and grep -c 'COUPLING:' == 2

The triple-condition assertion is the right shape: the ASCII count proves -z didn't break the common case; the count of 2 proves the non-ASCII path was read, not skipped. The commit message notes this was confirmed red before the fix ("quoted file dropped, one COUPLING line") — the test is causally verified, not just written against the fixed code.

The non-ASCII filename is constructed with printf 'caf\303\251.md' (UTF-8 octal escapes for café.md), keeping the test source pure ASCII. This is the correct approach — byte-literal construction is more portable than depending on the test runner's locale.

One minor observation on the test structure: cd "$fx" && inside the compound command ensures git operations and the script run in the fixture directory. The script's cd "$(dirname "${BASH_SOURCE[0]}")/.." resolves to $fx/ (since the script was copied to $fx/scripts/), so the git diff also targets the fixture repo. The SKILL_PORTABILITY_TOKENS is an absolute path from the outer test setup — works correctly regardless of the cd. All fixture cleanup via rm -rf "$fx" is present. No leaks.

Suite count: 16/16, consistent with the prior 15 plus this new test.

No new issues introduced

• sort -z portability: This is a GNU coreutils extension, not POSIX, but CI runs on ubuntu-24.04 (GNU sort). Consistent with the rest of the script's existing GNU-reliance (e.g., bash builtins, shellcheck).
• Empty output case: When git diff -z produces no output, the while read -d '' loop simply doesn't execute and files stays empty — correctly handled by the ((${#files[@]} == 0)) guard at line 131.
• --diff-filter=d + -z interaction: These flags are orthogonal and compose correctly; --diff-filter=d is applied before the NUL-delimited output is emitted.

All prior findings resolved ✓

Finding	Source	Status
Silent scan_file failure → false negative	First review (medium)	Fixed in c755ec6 ✓
is_guarded() generic presence markers over-match	Codex P2 / second review	Fixed in c755ec6 ✓
Annotation non-leak test missing negative assertion	First/third review (low)	Fixed in c755ec6 ✓
\b non-POSIX ERE in staged patterns	Second review / Codex	Documented in 72ca25f ✓
Bare-word fallback guards unrelated prose	Codex P2	Fixed in 8caf57a ✓
Git-quoted pathnames silently dropped from PR diff	Codex P2	Fixed here in 15d8399 ✓
is_comment() inline HTML pending_annot carry-over	Second review (deferred)	Tracked in #611 ✓

Ready to merge. The gate is fail-closed, self-testing before the PR-diff step, data-driven, changed-file-scoped, and now correctly gates skill files regardless of pathname encoding. The one deferred edge case (#611) is correctly out of scope for the initial gate's active token class.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 15d8399be1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 42s —— View job

---

PR Review — test(ci): rename quoting-fixture path (6a78358)

• Gather context and read the latest commit diff
• Review test fixture rename
• Check open Codex P2 finding (portability-scope: anywhere in file)
• Compile and post findings

---

The latest commit (6a78358) is a two-line test-fixture rename only. One open Codex P2 finding from the prior review round remains unaddressed — flagged below.

Commit 6a78358 — fixture rename is correct ✓

scripts/check-skill-portability.test.sh:222

-quoted_name="$(printf 'caf\303\251.md')" # café.md in UTF-8 — non-ASCII, triggers Git quoting
+quoted_name="$(printf 'quoted-\303\251.md')" # trailing U+00E9 byte — non-ASCII, triggers Git quoting

The rename is functionally neutral: the trailing byte sequence \303\251 (U+00E9 é, encoded in UTF-8) is identical — it still triggers Git's C-style quoting under the default core.quotePath=true. The ASCII prefix changes from caf (flagged by typos as a misspelling of "calf") to quoted (clean). The comment is updated to match. The test still exercises the same code path. No behavior change; the fix is the right call. Suite count 16/16 per commit message.

---

Open — portability-scope: whole-file escape matches prose, not just comment declarations

scripts/check-skill-portability.sh:141

if grep -qE 'portability-scope:' -- "$file"; then
  return 0
fi

This is the open Codex P2 finding from the 15d8399 review round (posted at 05:59 UTC). It has not been addressed or formally deferred in the comments yet.

The gap: grep -qE 'portability-scope:' matches the string anywhere in the file — including in prose documentation. A skill that describes its own annotation system (e.g., Use portability-scope: forge=github to declare...) would have all its coupling violations silently suppressed, with no reviewer signal that the escape is in effect. This contradicts the fail-closed and reviewer-visible design principles the gate was built on.

Practical risk today: Low for the active origin/(main|master) class — the string portability-scope: appearing naturally in skill prose (not as an annotation) while the same file also has a bare origin/main is unlikely in the current corpus. But it's an active code path, and the fix is a one-liner.

Fix — restrict the match to lines starting with a comment marker:

if grep -qE '^[[:space:]]*(#|`) and rejects a bare prose occurrence. The existing test at line 141–148 of the test file passes under the narrowed pattern without modification, so no test additions are needed.

**Recommended disposition**: Address before merge — the fix is a one-character change to a single line, it doesn't affect the current test suite, and closing the prose-match gap is consistent with the gate's documented "reviewer-visible escape" design. If deferred to a follow-up, track in #611 (or a new issue) and note it as an active rather than staged-class gap.

[Fix this →](https://claude.ai/code?q=In%20scripts%2Fcheck-skill-portability.sh%20on%20branch%20feat%2F531-portability-lint-gate%2C%20line%20141%20reads%20%60if%20grep%20-qE%20'portability-scope%3A'%20--%20%22%24file%22%3B%20then%60%2C%20which%20matches%20portability-scope%3A%20anywhere%20in%20the%20file%20including%20prose.%20Change%20it%20to%20restrict%20the%20match%20to%20comment%20lines%3A%20%60if%20grep%20-qE%20'%5E%5B%5B%3Aspace%3A%5D%5D*(#%7C%3C!--).*portability-scope%3A'%20--%20%22%24file%22%3B%20then%60.%20The%20existing%20test%20at%20line%20141-148%20of%20check-skill-portability.test.sh%20(which%20uses%20an%20HTML%20comment%20for%20the%20fixture)%20passes%20unchanged.&repo=melodic-software/claude-code-plugins)

---

### Prior findings status

| Finding | Source | Status |
|---|---|---|
| Silent `scan_file` failure → false negative | First review (medium) | Fixed in `c755ec6` ✓ |
| `is_guarded()` generic presence markers over-match | Codex P2 / second review | Fixed in `c755ec6` ✓ |
| Annotation non-leak test missing negative assertion | First/third review (low) | Fixed in `c755ec6` ✓ |
| `\b` non-POSIX ERE in staged patterns | Second review / Codex | Documented in `72ca25f` ✓ |
| Bare-word `fallback` guards unrelated prose | Codex P2 | Fixed in `8caf57a` ✓ |
| Git-quoted pathnames silently dropped from PR diff | Codex P2 | Fixed in `15d8399` ✓ |
| `is_comment()` inline HTML `pending_annot` carry-over | Second review (deferred) | Tracked in #611 ✓ |
| **`portability-scope:` escape matches prose, not just comments** | **Codex P2 (15d8399 round)** | **Open — not addressed or deferred** |

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a783584d9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[kyle-sexton]

Reconciliation flag — concurrent-lane collision on the escape-grammar findings.

A prior lane resolved the two open Codex P2 threads (portability-scope: standalone-comment, line 142; portability-ok: non-empty reason, line 145) as deferred to #624, keeping this PR's diff scoped to the quoted-path finding. Concurrently — before that deferral was visible — I pushed 6c7fe98, which fixes both inline plus their #611 sibling (is_comment anchoring so an inline <!-- ... --> tail on a prose line no longer carries an annotation down):

• portability-scope: honored only on a standalone comment line with a non-empty reason
• is_annotated requires residual non-whitespace reason text (rejects <!-- portability-ok: -->)
• 4 regression tests added; suite 20/20, shellcheck clean; --all corpus unchanged

These are active (not staged-class) code paths in scan_file, so the findings are live bypasses in the shipped gate, not future classes.

This leaves a code-vs-disposition inconsistency: the resolved threads read "deferred to #624 / not fixed inline," but head now contains the fix. Flagging for human / control-tower reconciliation before merge — either accept the inline fix at 6c7fe98 and rescope #624 (and the #611 tracking item), or revert 6c7fe98 to honor the scoped-diff deferral. I have not touched the resolved threads and am not merging (human-gated).