ci(portability-lint): define and guard the stack/ecosystem token class (stage 3) - #1790
Conversation
…s (stage 3) Stage 3 of #531 owns the stack/ecosystem opinion class: baked stack defaults in agnostic-flagged skills. The operator ratification on #714 fixed the shape — staged per-token rollout, no bulk activation, and per-token activation only after a POSIX-safe rewrite, a green `--all` audit, and a class-scoped guard marker. This lands all three. The previously staged spellings were INERT, not merely dormant. POSIX ERE defines no word boundary, and on gawk as well as mawk/nawk a backslash-b matches a literal backspace byte, so `\bdotnet\b` and `\bClean Arch\b` matched nothing: uncommenting either would have gone green while missing every violation. Both are replaced with POSIX-safe spellings rather than uncommented, and a fixture now fails if a backslash-b ever reaches an ACTIVE token. Clean-Architecture vocabulary is ACTIVATED. It measures 0 hits in 0 files through the gate, and a raw grep finds the phrase nowhere in the scanned corpus, so it carries no migration debt at all — the one member of this class that can be locked in before it arrives, which is the prevention the umbrella issue asks for. A skill whose subject genuinely is that architecture declares portability-scope; a fixture pins that escape so the token cannot strand it. The ecosystem literals stay staged with their residue measured through the gate itself at c7b0af7, so guards, per-site annotations, and scope declarations are already applied to the counts. Their shared blocker is recorded: these are not stray hardcodes but skills that legitimately need to name an ecosystem and have no seam to read it from, which is the userConfig seam work on #405/#406/#421/#428. The class gets its own guard markers, scoped so they never leak between classes: consumer-seam evidence co-located on the line, either a `${user_config.*}` read or a neutral placeholder together with an explicit illustration lead. Both halves of the second form are required — a placeholder alone excuses `run dotnet build in <repo-root>`, which ships the stack and parameterizes only the path, and an illustration lead alone excuses `e.g. run dotnet build`, the bare hardcode wearing a hedge. Class membership is tested with index() on literal fragments, never a regex match against the pattern text. A regex reads the pattern as syntax, so `p ~ /[Dd]otnet/` does not match the pattern STRING `[Dd]otnet` — it silently returns 0 for every member and leaves the class unguarded while the flagging tests still pass. That failure was caught by writing the honored-case fixtures, which is why they are part of the deliverable. Fixtures cover both directions per the acceptance criteria: flagged where nothing declares the stack, honored where a userConfig seam supplies it, and honored under the per-site and whole-file escapes — plus prose about a seam failing to stand in for a seam, each half of the placeholder guard failing alone, and class scoping proven both ways (branch-resolution evidence does not excuse an ecosystem hardcode; a seam read does not excuse a branch default). Refs #714 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
|
Claude finished @kyle-sexton's task in 4m 35s —— View job Code Review
Overall this is a well-engineered stage. The POSIX-safety fix (replacing 1.
|
|
Claude finished @kyle-sexton's task in 3m 5s —— View job Security Review — PR #1790
Scope: No traditional security vulnerabilities were found — no command injection, path traversal, credential exposure, or dangerous GitHub Actions patterns. The AWK patterns from the token file are used as regex operands in Two logic-layer findings worth flagging: SUGGESTION — Confidence: MEDIUM
return index(p, "otnet") || index(p, "NET") || index(p, "Clean Arch") ||
index(p, "csproj") || index(p, "sln") || index(p, "razor") ||
index(p, "lazor") || index(p, "C#") || index(p, ".cs(")
If a future token class adds a pattern whose text contains The design contract is "more token classes will follow," so the collision risk grows. Narrowing to SUGGESTION — Confidence: LOW-MEDIUM
l ~ /<[a-z][a-z0-9-]*>/ && l ~ /(e\.g\.|for example|such as)/
satisfies both conditions — No further security concerns with this diff. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28b1d4df54
ℹ️ 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".
Phrase-delimit the active Clean-Architecture token. `Clean Arch(itecture)?`
had no outer boundaries, so it matched inside a preceding word
(`UnClean Architecture`) and where the optional suffix stopped short
(`Clean Archive directory`), reporting ordinary prose as a violation. Use the
explicit `[^a-zA-Z0-9_]` boundaries this file already uses for `.NET` and
`C#`, and drop the claim that a multi-word phrase is self-delimiting.
Give the architecture token its own guard. It inherited the class-wide
consumer-seam marker, so ANY `${user_config.*}` read on the line excused it
— `Read ${user_config.test_command}, then place handlers per Clean
Architecture` passed the gate. The token file names exactly two escapes for
this token, `portability-scope` and placeholder-plus-illustration, and a
userConfig read is neither; evidence has to parameterize the coupling it
excuses. This is the only ACTIVE token in the class, so it was the one live
false negative.
Narrow the `.NET` class-membership fragment from `NET` to `\.NET`. A bare
`NET` is a substring of `NETRC`, `CONNECT`, and `INTERNET`, so a future token
class carrying that trigram would silently inherit this class's guard.
`assert_staged` pins the pattern spelling verbatim, so the fragment and the
pattern it selects cannot drift apart.
Factor the placeholder-plus-illustration test into one helper shared by both
branches, and record the residual it knowingly admits: inline HTML shares the
placeholder spelling space, and one line of text cannot separate them here
because `<b>`, `<i>`, `<p>`, `<s>`, and `<n>` are all genuine CLI operands in
this corpus. A minimum length rejects real placeholders and a tag denylist
enumerates instances of an open-ended shape, so the per-site portability-ok
remains the remedy — the same posture this file records for the
multi-ecosystem enumeration.
Bind the token list to the membership test: every staged ecosystem literal is
now asserted to be RECOGNIZED by is_ecosystem_pattern, not just present
verbatim. Each is checked twice so a fixture that never matched its pattern
cannot pass vacuously. Cover both remaining illustration leads and the new
architecture boundaries.
|
Claude finished @kyle-sexton's task in 4m 8s —— View job PR Review — Stage 3: Stack/Ecosystem Token Class
This is a two-commit PR. The second commit ( Prior findings — dispositionClaude review finding #1 — Claude review finding #2 — Claude review finding #3 — lowercase-only placeholder undocumented Claude review finding #4 — Codex finding #1 — architecture token inherits userConfig guard Codex finding #2 — architecture token not phrase-delimited
Fresh read — one observation
The STAGED_COVERAGE loop asserts that This is not a blocker — the coverage exists and the test will catch the regression — but it is worth noting for symmetry as the class grows. The cost of closing it explicitly would be a single short block (pattern text → Non-issues worth confirming
No blocking issues. This is a clean, correct, well-documented stage. |
|
Claude finished @kyle-sexton's task in 5m 55s —— View job Security Review — PR #1790
Scope: No GitHub Actions workflows were changed, so zizmor's supply-chain / trigger / injection advisory lane has nothing to assess here. Prior security findings — both resolved in HEADThe first security pass (on commit
Architecture guard isolated from the No injection surfacesAWK patterns from the token file are loaded as strings (
No shell interpolation of token file content occurs. The fail-closed mechanism is confirmed by the test at line 143. SUGGESTION — Confidence: LOW
The in-code comment at lines 242–249 explains why No other security concerns with this diff. |
|
Classifying the six findings from the two automated reviews on
Detail on the rows that did more than apply the suggestion. Row 2. The two failures are genuinely separate, which is what made this worth building: Row 6. The bypass is real and I am not disputing it, but the proposed fix does not survive the corpus. A minimum tag length of two would break genuine placeholders here: I also considered and rejected excluding known HTML tag names. It answers a class of shape with a list of instances ( Inline HTML and placeholders share the same spelling space here, so one line of text cannot separate them. That is the identical situation this token file already records for the multi-ecosystem enumeration — "a co-occurrence test cannot tell it from a line that hardcodes one stack and merely mentions another. It uses the per-site portability-ok escape until someone brings evidence for a tighter rule." Rather than invent a second posture for the same problem, I followed that precedent: the residual is now documented at the guard with the reasoning and the measurements, and Row 3 is folded into that same comment block, since the shape the guard accepts and the residual it knowingly admits are one thing to explain, not two. Verification: suite 79 pass / 0 fail (was 65 / 0); One correction to the affirmations in the code review: the note that the |
1 participant
Stage 3 of #531's portability-lint lane: the stack/ecosystem opinion class — baked stack
defaults in skills that claim to work in any repository.
The operator ratification recorded on this issue fixed the shape: staged per-token rollout, no bulk
activation, and per-token activation only after (1) a POSIX-safe rewrite, (2) a green
--allaudit,and (3) a class-scoped guard marker. This lands all three, plus the fixtures the acceptance criteria
ask for.
The staged patterns were inert, not merely dormant
\bdotnet\band\bClean Arch\bcould not have worked. POSIX ERE defines no word-boundary escape,and on gawk as well as mawk/nawk a backslash-b matches a literal backspace byte — so those
patterns matched nothing. Uncommenting either would have turned the class green while missing every
violation, which is the worst outcome a gate has.
Both are replaced with POSIX-safe spellings rather than uncommented, and a fixture now fails if
a backslash-b ever reaches an ACTIVE token again.
One token activates, on measured evidence
Clean Arch(itecture)?is now ACTIVE. It measures 0 hits / 0 files through the gate, and araw grep finds the phrase nowhere in the scanned corpus. It carries no migration debt whatsoever —
the one member of this class that can be locked in before it arrives, which is exactly the
prevention #531 exists for ("prevention instead of hand-fixes plus indefinite recurrence"). A skill
whose subject genuinely is that architecture declares
portability-scope, and a fixture pins thatescape so the active token cannot strand it.
The ecosystem literals stay staged, with real numbers
Residue measured through the gate itself at
c7b0af78, so guards, per-site annotations, andwhole-file scope declarations are already applied to the counts:
[Dd]otnet(^|[^a-zA-Z0-9_])\.NET([^a-zA-Z0-9_]|$)\.csproj(^|[^a-zA-Z0-9_])C#([^a-zA-Z0-9_]|$)[Bb]lazor\.sln([^a-zA-Z0-9_]|$)\.razor\*\*/\*\.cs([^a-zA-Z0-9_]|$)Their shared blocker is now recorded in the token file rather than left implicit: these are not
stray hardcodes, they are skills that legitimately need to name an ecosystem and have no seam to
read one from. The fix is the
userConfigseam work on #405 / #406 / #421 / #428 — the lint is theforcing function, not the fix — or a
portability-scopedeclaration on genuinely ecosystem-lockedfiles.
The class-scoped guard
The class is excused by consumer-seam evidence co-located on the line — never by the branch
class's evidence:
${user_config.*}read (the seam this class exists to force), anchored to the read syntax soprose about a seam does not count; or
<placeholder>and an explicit illustration lead (e.g.,for example,such as) — the portable idiom already used across this corpus, where the ecosystem tokenillustrates the placeholder rather than being the value the skill ships.
Both halves of (2) are required. A placeholder alone would excuse
run dotnet build in <repo-root>,which ships the stack and parameterizes only the path; an illustration lead alone would excuse
e.g. run dotnet build, the bare hardcode wearing a hedge — precisely the generic hedge the tokenfile's staged-class preamble forbids as a guard.
Deliberately not a marker: "another ecosystem is named on the same line". That multi-ecosystem
enumeration shape is the dominant residual in the corpus and reads as correct, but a co-occurrence
test cannot distinguish it from a line that hardcodes one stack and merely mentions another. It is
recorded in the token file as the shape the next activator must handle, with
portability-okas itsescape.
A bug the fixtures caught, worth calling out
Class membership is tested with
index()on literal fragments, never a regex match against thepattern text. My first revision used
p ~ /[Dd]otnet/— copying the branch class's shape — whichasks for a
D-or-dfollowed byotnetand therefore does not match the pattern string[Dd]otnet, whosedis followed by]otnet. It silently returned 0 for every member, leaving thewhole class unguarded while every flagging test still passed. It surfaced only because the
honored-case fixtures were written, which is why they are part of the deliverable rather than a
nice-to-have.
index()removes the trap instead of re-solving the escaping per pattern.Acceptance criteria
skill-portability-tokens.txt,staged entries carry measured residue and a named blocker,
assert_stagedpins every stagedpattern verbatim so an edit that stops catching its own defect fails loudly.
userConfigseamdeclares it — both directions, plus prose-about-a-seam failing to stand in for a seam, each half
of the placeholder guard failing alone, the per-site and whole-file escapes, and class scoping
proven both ways (branch-resolution evidence does not excuse an ecosystem hardcode; a seam
read does not excuse a branch default).
Verification
bash scripts/check-skill-portability.test.sh— PASS=65 FAIL=0 (was 44; 21 added)bash scripts/check-skill-portability.sh --all— the only hit repo-wide is one pre-existingbranch-class line in
plugins/playbooks/skills/boris/reference/foundations.md; the newly activeClean-Architecture token contributes 0. CI is changed-file scoped, so this never red-lines
main.bash scripts/check-shell-portability.sh --paths <the two changed scripts>— clean. The\bassertion is written as the bracket expression
[\]bso it does not trip the sibling gate thatagrees with it.
bash scripts/check-shell-portability.test.sh— PASS=215 FAIL=0 (sibling gate unaffected)shellcheckon both changed scripts — cleanscripts/check-changelog-parity.sh --check-bump origin/main,scripts/check-changed-skills.sh—pass. No plugin version bump: the diff is repo tooling under
scripts/, not a plugin.typos scripts/exit 0Related
time) was ratified there
portability-scopeexemption rides onuserConfigseam members this class is the forcing function for;advanced, not retired, by this change
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C