fix(implementation): defer branch-name grammar to consumer convention#453
Conversation
The branch-name grammar `<type>/<description>` was presented as the mandated form in the implement skill's Step 1 prerequisite check and its gotchas, with no deferral to the consumer's branch-naming convention. Target environments using Jira-key branches (e.g. `SW2-12345-desc`) diverged, breaking agnosticism. Mirror the same file's commit-message convention treatment: defer to the consuming project's CLAUDE.md / rules and present `<type>/<description>` as a common default. The branch-check eval is reframed to accept any convention- compliant branch name rather than asserting a single hardcoded grammar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 1m 40s —— View job PR Review: fix(implementation): defer branch-name grammar to consumer convention
SummaryClean, targeted fix. All five changed files are consistent with each other and with the plugin's stated design principles. No correctness issues found. File-by-file findingsThe updated bullet now reads:
This is structurally identical to the commit-message convention treatment at line 86:
The symmetry is exactly right. ✓ The em-dash sentence structure is slightly more complex than the old one-liner, but it's grammatically correct and the meaning is unambiguous. No issue. ✓ The new expectation is well-formed and directly testable: The
Coverage checkA repo-wide grep for Alignment with CLAUDE.md design rules
VerdictApprove. The change is minimal, precise, and consistent across all touch points. No issues found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb8c61c692
ℹ️ 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".
Step 1's branch-naming convention lookup checked only CLAUDE.md and rules. A consuming repo declaring its branch grammar in AGENTS.md would be missed before the <type>/<description> fallback. Match the adjacent source-control convention resolution, which already reads AGENTS.md.
|
Claude finished @kyle-sexton's task in 1m 31s —— View job PR Review: fix(implementation): defer branch-name grammar to consumer convention
SummaryTwo-commit fix. Both are clean, minimal, and correct. The second commit ( File-by-file findingsFinal form after both commits:
Resolution order is now Minor note (non-blocking, pre-existing): The adjacent commit-message convention at SKILL.md line 86 still reads The gotcha is a compact user-facing tip that explicitly references Step 1 as the authoritative gate. It doesn't need to enumerate the full resolution stack — it already says "following the project's branch-naming convention" and the reader is directed back to the step that specifies where to look. ✓
The 0.7.1 entry was correctly updated in
Codex finding resolution
Alignment with CLAUDE.md design rules
VerdictApprove. Both commits are minimal, precise, and internally consistent. The AGENTS.md addition in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e197dce2c
ℹ️ 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 finished @kyle-sexton's task in 1m 23s —— View job PR Review: fix(implementation): defer branch-name grammar to consumer convention
SummaryThree-commit arc is clean and complete. Every Codex finding has been addressed; all five changed files are internally consistent. Commit-by-commit findings
SKILL.md:56, gotchas.md:29, evals.json eval #2, plugin.json 0.7.1, CHANGELOG entry. The deferral to consumer convention is correct;
SKILL.md:56 now reads
This commit directly resolves the Codex finding about the unconditional framing. The final eval state: "Does NOT mandate a specific branch-name grammar — defers to the consuming project's declared branch-naming convention, treating `<type>/<description>` only as the fallback default when no convention is declared"The key change is conditional: File-by-file assessment
Non-blocking follow-up (pre-existing, out of scope)
Alignment with CLAUDE.md design rules
VerdictApprove. All Codex findings addressed across all three commits. The fix is minimal, precise, and consistent across SKILL.md, gotchas.md, evals.json, CHANGELOG.md, and plugin.json. |
|
…ls (#531) (#609) ## 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): #404 #405 #406 #408 #410 #412 #415 #416 #418 #421 #422 #423 #428 #429 #432 #438 #439 #441 #442 - #445 — sibling CI-gate backlog (mechanical conformance only; explicitly not coupling) - #412 — guarded-forward-ref disposition the gate honors (guarded ≠ bare) - #441 — declared narrower-scope exemption the `portability-scope` mechanism serves - #467 — the branch/remote review-churn the seed active class targets - #453 — seam-conditional eval re-catch of this class - #491 #492 — .NET-flavored-example hand-fixes a future staged class would obviate - #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.com/claude-code) https://claude.ai/code/session_01KyLj6oaFVTE6xFuoYCC2KC --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
The branch-name grammar
<type>/<description>was presented as the form in theimplementationplugin'simplementskill, with no deferral to the consumer's branch-naming convention. Target environments using Jira-key branches (SW2-12345-desc,feature/SW2-12345) diverge, breaking agnosticism (extensibility seam 3 + the convention-resolution ladder: read the declared value before defaulting).The fix mirrors the same file's commit-message convention treatment (SKILL.md, Step 2 commit discipline), which already defers to the consumer's
CLAUDE.md/ rules and frames Conventional Commits as "a common default".Changes
skills/implement/SKILL.md(Step 1 prerequisite check): branch creation now defers to the consuming project's branch-naming convention (itsCLAUDE.md/ rules) and presents<type>/<description>as a common default rather than the required form. The/source-control:worktreepointer is unchanged.skills/implement/context/gotchas.md(branch-check gotcha): the<type>/<description>example now reads as a common default following the project's convention, keeping the "takes 2 seconds" point.skills/implement/evals/evals.json(evalbranch-check-stops-on-default-branch): reframed as a grammar-agnostic acceptance guard. Note: the file did not literally contain<type>/<description>— the eval already accepted convention-deferring behavior — so the edit makes that acceptance explicit (expected_outputno longer implies a baregit checkout -bas the mechanism; a new expectation asserts no specific grammar is mandated) rather than removing a hardcode that was not there.0.7.0→0.7.1; matching## [0.7.1]### FixedCHANGELOG entry.Verification
npx markdownlint-cli2on all changed.md— 0 issues.jq -e .onevals.json— valid JSON; edits stay within schema-allowed fields.jq -e .versiononplugin.json—"0.7.1".Closes #403
Related
docs/PLUGIN-PHILOSOPHY.md+docs/MIGRATION-PLAYBOOK.md) as sibling agnosticism findings implementation: Clean-Architecture layer vocabulary baked as universal execution order #404, discovery: setup skill cites marketplace-repo ADR by bare path — dead reference in installed plugin form #407, discovery: live absolute project-root injected into skill context can bleed into EXPLORE.md artifacts #409.🤖 Generated with Claude Code