fix(skills): vendored suites behave in consumer checkouts - #1522
Conversation
Two vendored-content defects surfaced by consumer PR reviews: review-gate's settings-example-sync-controls suite hard-required the catalog-root settings template, which only the kendex source tree has, so any consumer running its vendored tests died with 'fixture source missing' — while the suite under test deliberately counts that absence as a skip. The fixtures are synthetic (every control plants its own drift), so the skill template now stands in as the fixture's root copy and every control runs in both layouts. preflight's docs-cited-paths lane split a repo-qualified citation (kendex:docs/x.md) at the colon on code lines and judged the bare path as this repo's own, firing on a sibling repo's file. The markdown branch already skips the qualified form by charset; the code branch now matches it. Claude-Session: https://claude.ai/code/session_01Jx2yfoaodk4rDpdWDqgoMk
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The qualified-citation gsub and its precision case grow the script by five lines and the suite by one; deliberate, recorded here per the ratchet's contract.
There was a problem hiding this comment.
Pull request overview
Fixes vendored skill checks so they work correctly in consumer repositories.
Changes:
- Falls back to the skill template when the root template is unavailable.
- Ignores repository-qualified documentation citations.
- Adds precision coverage for the citation case.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
skills/review-gate/tests/settings-example-sync-controls.test.sh |
Supports vendored fixture layouts. |
skills/preflight/tests/precision.test.sh |
Adds repository-qualified citation coverage. |
skills/preflight/scripts/preflight |
Filters repository-qualified paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/review-gate/tests/settings-example-sync-controls.test.sh:28
- The new vendored-checkout branch is not exercised by the repository's normal test sweep: in this checkout
ROOT_TEMPLATEalways exists, so this condition stays false. A regression that restores the hard requirement would therefore still pass CI, contrary to the repository rule that every behavior change has a failing regression test. Add a consumer-layout test that copies only the review-gate skill into a temporary checkout, runs this controls suite without a root template, and asserts its successful verdict.
if [ ! -f "$ROOT_SRC" ]; then
ROOT_SRC="$SKILL_TEMPLATE"
Copilot's late findings on the branch: the qualified-citation strip could swallow the md:42 tail of a line-suffixed local citation, hiding a dead path; the qualifier now needs a boundary before it and a slash after the colon, with a regression case. And the consumer-facing suite fixes get their CHANGELOG entry.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/review-gate/tests/settings-example-sync-controls.test.sh:29
- The new vendored-checkout branch is not exercised by automated tests:
.github/workflows/skill-tests.yml:77runs this suite in the source layout, whereROOT_TEMPLATEalways exists. Add a regression test that copies the controls suite, sync suite, and skill template under a temporary.agents/skills/review-gate/tree with no root template, then invokes the copied controls suite; that test would fail on the pre-fix hard requirement and protect this consumer-only path.
if [ ! -f "$ROOT_SRC" ]; then
ROOT_SRC="$SKILL_TEMPLATE"
echo " note root template absent (vendored checkout) — fixtures use the skill template as the root copy"
The comment's own example cited a doc path the lane then judged.
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/kendex/actions/runs/32347550803 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/preflight/scripts/preflight:644
- This
gsubpattern includes the boundary character in the match ((^|[^...])...). When the citation isn’t at the beginning of the line, the preceding delimiter (e.g.,(,=, whitespace) is also replaced with a space, which mutates the surrounding token stream beyond removing just the repo-qualified citation. To avoid unintended tokenization changes, match and replace only the repo-qualified citation itself (i.e., don’t consume the boundary), or use a replacement strategy that preserves the boundary character.
# A repo-qualified citation (`kendex:docs/x.md`) names a sibling repo
# this checkout cannot resolve; splitting it at the colon would judge
# the bare path as this repo. The markdown branch already skips the
# qualified form by charset — the code branch has to match it. The
# qualifier needs a boundary before it and a `/` after the colon, so a
# line-suffixed local citation (`docs/<x>.md:42`) is untouched.
gsub(/(^|[^A-Za-z0-9_.\/-])[A-Za-z][A-Za-z0-9_-]*:[A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]*/, " ")
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/kendex/actions/runs/32373935817 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
|
Merged by admin via the gate-repair lane: the merge queue is wedged repo-wide (every entry ejected ~3 min in with the group runner receiving an external shutdown — 10+ attempts across three PRs, all suites green; ruleset-elimination testing showed it persists with only the queue ruleset active). Verified green before merge: PR-leg checks, review-gate converged, threads resolved, full workspace suites + preflight + review-gate battery locally. Queue wedge filed in Linear for the owner. |
Two vendored-content defects surfaced by Copilot reviews on consumer migration PRs (vg#24, memsira#487):
tests/*.shsweep died with 'fixture source missing', while the suite under test deliberately counts an absent root template as a skip. The fixtures are synthetic (every control plants its own drift), so the skill template now stands in as the fixture's root copy; all controls pass in both layouts (verified with the root template present and deleted).kendex:docs/x.md) at the colon on code lines and judged the bare path as this repo's — firing on a sibling repo's file (hit memsira's check-doc-pointers.py). The markdown branch already skips the qualified form by charset; the code branch now matches it. Precision case added.All preflight and review-gate suites green.
https://claude.ai/code/session_01Jx2yfoaodk4rDpdWDqgoMk