[US-482] feat: skill-local scripts ship with their skill and mirror byte-identically - #483
Conversation
Verdict
PR: #483 · Author: rucka · Reviewer: pair-reviewer 1/1 (independent, clean context) · Date: 2026-09-10 · Story: US-482 · Type: feature Reviewed head Classification matrix — per dimension
Tier = max(assessed) = green. Confirmed, not raised. Custody and evidenceCustody: verified, no breach. Evidence re-run on this exact head — every row reproduced, none taken on trust
The corpus is 6 workflow skills carrying 10 skill-local scripts, all AssessmentsSecurity — Input validationVerdict: green — the only inputs are repo-controlled dataset paths; no untrusted data reaches this gate. Security — Output handlingVerdict: green — output is diagnostic strings to stdout; no encoding surface. Security — Authentication / AuthorizationVerdict: not applicable — a local dev-time filesystem gate, no auth surface. Security — Introduced vulnerabilitiesVerdict: green — 0 introduced, 0 pre-existing. DetailsPath traversal was considered and is handled deliberately: CostVerdict: Architecture (Coupling)Verdict: not assessed — outside the One coupling observation, recorded rather than assessed
Bug fix — Red test before fixVerdict: not applicable — feature PR. (The two in-cycle fixes above the seals, DetailsFindings by severityCritical (must fix before merge) None. Major (must fix before merge)
Minor (must fix before merge — same bar as Major, just lower impact)
Questions (informational, never blocking)
Prior findings — transitions verified against the producer, not against the claim
Definition of Done
What happens next
|
… bare one `collectSkillFiles` returned early on a dir's own SKILL.md marker, so a dir holding BOTH its marker and a SKILL.md-bearing sub-dir yielded only the bare skill — moving the silent drop instead of closing it. The copy pipeline (`datasetSkillDirs`) collects every */SKILL.md, so the dropped nested entrypoint still installs invocable as `pair-<dir>-<sub>`: wholly unchecked (portability, size, links, approval signal) and `skillCount` short by one, which then fails the catalog/KB prose counts somewhere unrelated. `checkEntrypointDepth` cannot be the backstop — `<dir>/<sub>/SKILL.md` sits at exactly ENTRY_DEPTH. The two markers are now independent: collect the bare SKILL.md, then keep walking the sub-dirs. Set parity with the copy pipeline is pinned on the real corpus (55 = 55, difference empty both ways) and per domain row R1-R6. Closes the r1-g1 RED contract for PR #483.
e8194d0 to
b3efa04
Compare
…coordinator, handed on verbatim Canary run 3 (#482, PR #483, 26 agents): three of four groups had their first `red` rejected by hasRedContractReady for an absolute contractPath; the relative retry then did not resolve from the story worktree (the contract lives in the main checkout's run directory) and cost one failed seal; group g4 (test mode) ended the card as failed-fix on the same rejection. The coordinator now accepts an absolute path under /.pair/working/runs/ (no `..`, no shell syntax) and passes it to red-verify and red-seal unchanged; red-spec returns it absolute by contract. ADR-024 §11. Run 3 also proved seal → GREEN → P3 live for the first time (three groups green, no breach). Refs: #479 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ndoned attempt instead of refusing stale Canary run 4 (#482, PR #483, 5 agents): the RED author found HEAD at base but the tree dirty at one test file — the unsealed edit run 3's g4 author left when the coordinator rejected its result — and refused `stale`, correctly under the old rule. New rule (ADR-024 §12): with HEAD at base, a tree dirty ONLY at test artifacts, no $repair and no snapshot for the PR, the author records each path + sha256 as `discarded` in its handoff, restores the tree, and proceeds; a moved head or a dirty production path is still `stale`. Refs: #479 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…un keeps its runId and builds on its own prior attempts Canary run 5 (#482, PR #483, 8 agents): GREEN had appended the cycle log under the MAIN checkout's .pair/working/reviews/ while the probe looked for it in the worktree — no resume was ever a continuation. cycle-comments/green-fix now resolve $reviewLog against the main checkout, like the handoffs (ADR-024 §13). Convergence across runs: a resume passes the SAME runId; red-spec treats the verifier's earlier rejection for its phase (<phase>-red-verify.json in the run directory) as mandatory rows, and a `fresh` review re-validates the previous review's findings before hunting for new ones, so two attempts of one cycle build on each other instead of re-sampling. Repair budget unchanged. Refs: #479 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e merge gate instead of grouping them Canary run 7 (#482, PR #483, 5 agents, severityFloor Major): the review raised a Major on the story CARD (business rule 3 described a layout the gate refuses); the planner had no place for a finding with no repository path, put it in a `structural` group with `allowedPaths: []`, and the plan was rejected twice → failed-plan. PLAN_SCHEMA gains `carried[]` ({ finding, disposition }); every index must be in exactly one group or carried; carried findings are accepted as "Outside the repository — <disposition>" and a plan with no groups converges with them on the record. remediation-plan skill updated; ADR-024 amendment §7. Story #482's rule 3 corrected on the card as the review asked. Incident noted: during run 7 an agent flipped the main checkout's `core.bare` to true (restored by hand); no transcript names the command. Refs: #479 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…h instead of joining it onto the worktree Canary run 8 (#482, PR #483, 8 agents): RED authored and verified, then the sealer refused twice with ENOENT — join(cwd, <absolute contractPath>) produced <worktree>/Users/.../red-contract.json. resolve(cwd, contractPath) keeps an absolute path and resolves a relative one against cwd; the dirty-tree filter compares resolved paths. Regression test on a contract stored outside the worktree; all four copies synced. Refs: #479 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… too `checkInstallableLayout` implemented only the SHALLOW half of the installer's bounded-flatten rule (`validateNoShallowEntryWithSubdir`). The DEEP half (`validateNoDeepEntry`) had no owner in the gate: `checkEntrypointDepth` is marker-BOUND (`basename(file) !== 'SKILL.md'`), so a marker-LESS directory below the entry depth whose ancestor at that depth holds no files tripped the installer and nothing in the gate — the whole corpus installed as NOTHING behind a green gate. Measured at the real boundary, injecting `workflow/shared/lib/util.mjs` (the plausible trigger: `red-snapshot.mjs` is already duplicated across two skills) into the live corpus: - before: `skills:conformance` PASS (exit 0) while the producer REFUSES - after: FAIL — 1 violation, naming `workflow/shared/lib`, the same directory the producer's refusal blames The rule is marker-BLIND on both sides, mirroring the producer: a dir deeper than the entry depth is CONTENT iff its ancestor at EXACTLY that depth holds files of its own. Additive — `checkEntrypointDepth` is untouched. Each rule is its own helper so the lint ceilings (50 lines / complexity 10) still hold. Refs: #483 review round 1, finding 0 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…emental resume, upstream acceptance contract; way-of-working delta - replaces §1/3/4/6/7 and amendment-a §3–7 with the rules in force, stage owners, retired dispatches (rejected at parse time), typed statuses, finding-id policy - freezes the baseline identities (engine 2.0.0 @ 8b8b260, #482/#483, runs 1–8 cost) before optimizing - Task: T-10 — Freeze the delta contract and replace the contradictory rules Refs: #479
c2e5527 to
ad81780
Compare
…Workflow harness refused the script (canary run 11 launch); the NUL delimiter is spelled as an escape, guarded by test Refs: #479
… in the Workflow sandbox and aborted canary run 11 at launch; wall time is read from the harness summary, guarded by test Refs: #479
Pair-RED-Snapshot: pr=0; phase=a0; base=421441fa0a8e2a8a4c02bbf4d00bcd99feb129eb; manifest=.pair/red-snapshots/pr-0-a0.json
…yte-identically checkSkillLocalScripts(skillsDir, installedSkillsDir), wired into runChecks against the real installed root and named in the CLI PASS summary. - linked ./scripts/x and scripts/x resolve inside the skill's own scripts/; delegated to extractLinkTargets + isCheckableTarget + the #fragment strip, so a fenced example, a <placeholder> and an adr-NNN- pattern stay examples - every dataset skill-local script has a byte-identical twin at the bounded flatten's path: <cat>/<name>/scripts/<sub> -> pair-<cat>-<name>/scripts/<sub>, recursive, sub-directories preserved (verified against the real installedArtifactPath); missing / drifted / unreadable reported distinctly - directional like the SKILL.md mirror guard: orphans ignored, drift reported never repaired; absent installed root (dataset-only checkout) skips the twin half - a meta skill owning scripts/ is refused: the flatten maps next/scripts/router.mjs to pair-next-scripts/router.mjs, a separate top-level dir, and the entry walk then loses the skill entirely - own corpus walk, not collectSkillFiles, precisely so that layout stays visible - no import of skill-md-mirror: the gate runs under ts-node with no build and its exit branch is exercised by spawning a copy of this file alone Sealed contract green: 112/112 (18 previously-red rows R1-R13, R15, R19-R22; controls R14, R16, R18 still pass). Refs: #482
… snapshot The seal's record lives in Git history at a3772ab; left in the working tree it is the ONE path failing prettier (format:check) and it blocks the pre-push gate. Custody verify expects it gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Pair-RED-Snapshot: pr=0; phase=a0-rev2; base=a5f4da500a1ff5f511ea335a3acf97e5cf0cab12; manifest=.pair/red-snapshots/pr-0-a0-rev2.json
… leave the skill's own scripts/ r0-1: readdirSync-withFileTypes reports lstat semantics and existsSync follows, so a symlinked script, a symlinked sub-directory, a dangling entry and a dangling `scripts` link were all dropped in silence. Entry type is now resolved by a following statSync inside a try (a dangling entry is `unresolvable` and gets named, never an ENOENT that takes the run down); the walk is cycle-guarded by resolved path. r2-6: a scripts/-prefixed link is refused when its RESOLVED path leaves the skill's own scripts/ directory — the boundary is scripts/, not the skill folder, so <skill>/helper.mjs is refused too, while scripts/lib/../helper.mjs stays legal. Also drops the transient seal manifest, whose record lives in Git history.
Human decision required — round 5 of 5 (budget exhausted)Reviewed head Custody is clean ( The round-5 finding closed, and a new one opened
Proved end to end by running the real install pipeline ( A freshly and correctly installed tree is red, and the remediation the message prescribes cannot clear it — re-running Why this needs a contract revision rather than a fix. Secondary, same root cause: with the walk-global set replaced by an ancestor chain, aliases fan out combinatorially — a 6-level tree with two aliases per level yields 127 entries (measured, 70 ms). Refusing directory symlinks removes this too. Blast radius today: none. The corpus carries zero symlinks ( The decisionThe fix budget is spent (round 5 of
Carried to the merge gate (not actionable inside this contract)
Resolved and verified this cycle: |
|
Human decision on the r3 escalation (maintainer, 2026-09-10): option 1 — extend the fix-round budget by one round for
Technical note (author, 2026-09-10): engine 3.0.9 removed the fix-round budget from the effective-inputs digest (a budget change must not invalidate review evidence). The formula change itself invalidates the pre-3.0.9 review digests once, so the resume performs one migration re-review ( |
Pair-RED-Snapshot: pr=0; phase=a0-rev3; base=9735d2da1d354f60171afbe2c80fe703d8a7f9d3; manifest=.pair/red-snapshots/pr-0-a0-rev3.json
…, in either readdir order The scripts/ walk kept one visited set for the whole descent, keyed on realpathSync. A directory reachable under two names — a real scripts/lib and a sibling symlink aliasing it — really ships under both, so pair update installs a twin at each mirrored path. The shared key dropped whichever name readdirSync yielded second, so a drifted twin of a genuinely shipped path was answered with silence and the corpus gave two answers depending on directory order. Bound the descent by its own ancestor chain instead: a sibling ALIAS is walked once per relative path, a true CYCLE (scripts/lib/loop -> scripts) is still refused and the walk still terminates. The transient seal manifest leaves the tree above the snapshot. Refs: #482
… recorded inside this contract The a0-rev3 fixScope widened allowedPaths to include .pair/adoption/decision-log/ "because the implement process records its decisions there", but kept mode "behavioral", under which the custody check flags any added file (status != M) as behavioral-adds-or-moves-module. A decision-log entry is always a NEW file, so the contract permits the path and forbids the only way to use it. Adding one also forces .pair/llms.txt to be regenerated — the committed index is pinned byte-for-byte by llms-index-conformance — and that path is not in allowedPaths at all. Both are reported as contractGaps rather than patched around. The walk-strategy rationale and its alternatives stay where they are in scope: the doc comment on collectLocalScriptEntries. Refs: #482
|
Human decision on the r5 escalation (maintainer, 2026-09-10): grant
|
Cycle status after
|
Cycle v4 status — failed-preparation (
|
PR Information
PR Title: [US-482] feat: skill-local scripts ship with their skill and mirror byte-identically
Story/Epic: #482 (epic #212 — Supervised automation)
Type: Feature
Priority: P1 (Should-Have)
Assignee: rucka
Labels:
user story,risk:green,pr-state:to-be-reviewedBase:
feature/US-479-delivery-workflow-to-be— STACKED on #479, notmain. That branch's commits are already in this history and are not this story's work.Summary
What Changed
pnpm skills:conformancegains one failure class,checkSkillLocalScripts, wired intorunChecksagainst the real installed root and named in the PASS summary:[…](./scripts/x)/[…](scripts/x)aSKILL.mdlinks must resolve to a file inside that skill's OWNscripts/directory. Delegates toextractLinkTargets/isCheckableTargetand the same#fragmentstripcheckLinksdoes, so a fenced authoring example, a<placeholder>and anadr-NNN-pattern path stay examples instead of becoming phantom missing scripts.<category>/<name>/scripts/<sub-path>→pair-<category>-<name>/scripts/<sub-path>), sub-directories preserved.Why This Change
A skill must be portable as ONE folder. Today a script edited in the dataset but not in
.claude/skills/(or the reverse) is a runtime surprise at the moment an agent runs the stale copy; after this it is a red gate atpnpm skills:conformance.Story Context
User Story: As a pair maintainer shipping skills that carry their own scripts, I want the conformance check to prove every linked script ships beside its
SKILL.mdand every shipped script has a byte-identical installed twin, so a skill is portable as one folder and a one-sided edit is a red gate rather than a runtime surprise.Acceptance Criteria: AC 1 (linked script exists), AC 2 (installed twin byte-identical), AC 3 (PASS on the real corpus, summary names the check) — all three covered by the sealed acceptance contract below.
Changes Made
Decisions
pair updaterepairs it.'ab'vs'ba'is drift.scripts/, not the skill folder.scripts/../helper.mjsresolving to<skill>/helper.mjsis refused even though it is portable with the folder;scripts/lib/../helper.mjsstays legal. Decided on the RESOLVED path, never by banning..in the spelling. Authority: AC 1 ("in the skill'sscripts/directory") and the story's business rule ("files under a skill's ownscripts/directory").statSyncnever escapes atryin this module. It throwsENOENTon a dangling entry, and an unhandled throw there kills the whole conformance run with no report — contradicting the module's own stated principle that an unreadable twin is never assumed identical.scripts/is refused. Both read the same flatten authority (installedArtifactPath,skill-md-mirror.ts) in opposite directions, verified by running it:workflow/alpha/scripts/lib/util.mjs→pair-workflow-alpha/scripts/lib/util.mjs(ships inside the skill ⇒ guard it),next/scripts/router.mjs→pair-next-scripts/router.mjs(a SEPARATE top-level skill dir, and the corpus walk then stops findingnextat all ⇒ refuse it).checkMirroredLocalScripts→compareScriptsTree.Decision added in this revision (finding r3-9) — the walk bounds by its ANCESTOR CHAIN, not a walk-global
visitedsetThe walk kept ONE
visitedset for the whole descent, keyed onrealpathSync(dir). A directory reachable under two names — a realscripts/zzz-lib/and, beside it, a sibling symlink aliasing it — really ships under BOTH, because the bounded flatten installs a twin at each mirrored path. The shared key dropped whichever namereaddirSyncyielded SECOND, so a drifted twin of a genuinely shipped path was answered with silence, and which half went silent was decided by directory order: one corpus, two answers.The fix records the resolved path of the directories currently being descended and pops on exit. A sibling ALIAS is then walked once per relative path (each compared against its own twin); a true CYCLE (
scripts/lib/loop -> scripts, whose target is an ancestor of the descent that reached it) is still refused, so the walk still terminates.Two alternatives were rejected on evidence, not taste:
visitedset — the cheapest way to emit both names, and a true cycle then recurses until the stack dies, taking the whole gate down with no report. Row R32 is the trap that catches it.A third resolution (keep the global set but re-emit an already-visited subtree under its second relative path) is equally admissible and yields an identical entry list; the ancestor chain was chosen because it needs no second bookkeeping structure. The rows assert the ANSWER at each mirrored path, never a walk strategy.
Boundary probe at the real producer (isolated, discarded after the run): a tree with
scripts/lib/util.mjs, two independent aliasesscripts/a1 -> libandscripts/a2 -> lib, and a self-alias cyclescripts/lib/self -> libreturns exactly three errors —scripts/a1/util.mjs,scripts/a2/util.mjs,scripts/lib/util.mjs— in 2 ms, without throwing. Both aliases emitted, the cycle bounded, no blowup.Files Changed
packages/knowledge-hub/src/tools/skills-conformance-check.ts— the whole check (the only production path in the contract'sfixScope.allowedPaths)packages/knowledge-hub/src/tools/skills-conformance-check.test.tsNothing else in the repository is touched:
git diff feature/US-479-delivery-workflow-to-be...HEAD --name-onlyis exactly those two files.Testing
Test Coverage
packages/knowledge-hub/src/tools/skills-conformance-check.test.ts— 124 passed / 124, of which 34 are this story's acceptance rows R1–R34 (witnesses, boundaries, controls, interactions).pnpm skills:conformance(PASS, exit 0); R18 spawns the module copied into a temp ROOT over a violating fixture (FAIL — 1 violation, exit 1).runChecksresolves the REAL installed root, so a drifted script fails the gate end to end.readdirSyncyields first} × {which twin drifted}: R30 and R33 were RED at the base head (eachexpected [] to have a length of 1), R31 and R34 are the already-correct controls that a naive ordering fix would have silently regressed, and R32 is the true-cycle termination control.Test Results
Pre-merge tiering is
disabledin this project's adoption, so the FULL adopted suite was run for CI parity rather than therisk:greenbase subset. No gate was bypassed; the pre-push hook (pnpm quality-gate) ran on the push that produced this head.Testing Strategy
scripts/, absent installed root, orphan installed script, nested sub-directory (both outcomes), non-checkable targets (placeholder / pattern / fragment), fenced-block-only link, equal-length byte swap, a directory aliased by a sibling symlink in either readdir order.scripts/, danglingscriptsentry itself, a true symlink cycle — every one reported by path or bounded, none throwing.Reviewer Guide
Review Focus Areas
collectLocalScriptEntries. The ancestor chain must be popped on exit (that is what makes a sibling alias walkable) while still refusing a directory already open in the current descent (that is what makes a cycle terminate). R30/R33 fail if the pop is missing; R32 hangs if the refusal is.isWithin(<skill>/scripts, resolved). R26 (escapes both boundaries), R29 (inside the folder, outsidescripts/) and R27 (normalizes back inside) are the three rows that separate the two candidate readings; only a resolved-path test against the skill's ownscripts/passes all three.resolvedKind(followingstatSync, in atry) andentryExists(lstatSync). R28 is the trap: astatSyncinside the walk with notrypasses R23/R24/R25 and crashes the gate.installedSkillDirNamemust keep agreeing withskill-md-mirror.ts's real transform.Testing the Changes
Dependencies & Related Work
Blocking Dependencies
feature/US-479-delivery-workflow-to-be; that branch merges first.Follow-up Work
Three gaps fall outside this story's sealed
fixScopeand are deliberately NOT patched around here:scripts/-prefixed link must resolve inside the skill's ownscripts/; a depth-1 meta skill cannot ownscripts/— is stated nowhere in.pair/knowledge/.../skill-conventions/. The check is currently its only statement, so an author meets the rule for the first time as a red gate.red-snapshot.mjs sealwritesJSON.stringify(…, null, 2), whichprettierwould reformat, and no.prettierignorecovers it — so a tree that keeps the seal manifest failsformat:check. Answered here by removing the transient manifest above the snapshot (the workflow's own Step 3.1), but the collision itself lives in the workflow script and is out of scope.a0-rev3fixScopeadded.pair/adoption/decision-log/toallowedPathsprecisely so the implement process could record its decisions there, but keptmode: behavioral, under which the custody check breachesbehavioral-adds-or-moves-moduleon any diff entry whose status is notM. A decision-log entry is always a NEW file, hence always statusA— so the contract permits the path and forbids the only way to use it. Reproduced, not inferred: with the ADL committed,red-snapshot.mjs verifyreturned{"verified":false,"contractBreach":true,"breaches":[{"code":"behavioral-adds-or-moves-module","path":".pair/adoption/decision-log/2026-09-10-…md","status":"A"}]}. Independently, adding any file under.pair/adoption/decision-log/also forces.pair/llms.txtto be regenerated —apps/pair-cli/src/registry/llms-index-conformance.test.tspins the committed index byte-for-byte against the generator over the real tree — and.pair/llms.txtis not inallowedPathsat all, so even amodefix alone would leave the contract unsatisfiable. The walk-strategy rationale therefore lives in the doc comment oncollectLocalScriptEntries(in scope) and in the "Decision added in this revision" section above.