diff --git a/README.md b/README.md index fdedca8..99e7280 100644 --- a/README.md +++ b/README.md @@ -474,8 +474,10 @@ node --test test/build-agent-adapters.test.mjs \ test/build-agent-adapters-template.test.mjs \ test/build-sh-template.test.mjs \ test/install-skills.test.mjs \ - test/journal-config.test.mjs # adapter generator + build.sh + + test/journal-config.test.mjs \ + test/skill-wiring.test.mjs # adapter generator + build.sh + # skill installer + journal binding + # + skill-to-skill wiring ``` The container-based render scripts (`build.sh` itself and @@ -539,7 +541,7 @@ example to pin a digest) with `DOCS_TOOLBOX_IMAGE`. | `generate` | Validate, then generate derived fragments/indexes | `ruby scripts/validate-metamodel.rb --generate` | | `test` | Run all tests (Ruby units, Ruby CLI, JS adapter) | see [Tests](#tests) | | `test-ruby` | Ruby validator/generator unit and CLI tests | `ruby -Itest test/validate_metamodel_test.rb` and `ruby -Itest test/validate_metamodel_cli_test.rb` | -| `test-js` | JS adapter generator + build.sh template tests | `node --test test/build-agent-adapters.test.mjs test/build-agent-adapters-template.test.mjs test/build-sh-template.test.mjs test/install-skills.test.mjs test/journal-config.test.mjs` | +| `test-js` | JS adapter generator + build.sh template tests | `node --test test/build-agent-adapters.test.mjs test/build-agent-adapters-template.test.mjs test/build-sh-template.test.mjs test/install-skills.test.mjs test/journal-config.test.mjs test/skill-wiring.test.mjs` | | `adapters` | Regenerate agent adapters from skills | `node scripts/build-agent-adapters.js` | | `check-adapters` | Fail if the generated adapters are stale | `node scripts/check-agent-adapters.js` | | `build` | Generate fragments and render architecture HTML | see below | diff --git a/build.sh b/build.sh index a5b31a4..2a1a561 100755 --- a/build.sh +++ b/build.sh @@ -87,7 +87,7 @@ run_local_test_ruby() { } run_local_test_js() { - node --test test/build-agent-adapters.test.mjs test/build-agent-adapters-template.test.mjs test/build-sh-template.test.mjs test/install-skills.test.mjs test/journal-config.test.mjs + node --test test/build-agent-adapters.test.mjs test/build-agent-adapters-template.test.mjs test/build-sh-template.test.mjs test/install-skills.test.mjs test/journal-config.test.mjs test/skill-wiring.test.mjs } run_local_test() { diff --git a/features/skill-wiring.feature b/features/skill-wiring.feature new file mode 100644 index 0000000..fe5b9f5 --- /dev/null +++ b/features/skill-wiring.feature @@ -0,0 +1,41 @@ +# Living documentation for the wiring between canonical skills. +# Bridged to: test/skill-wiring.test.mjs (node:test, classic runner, no native +# BDD). Each scenario maps to one test named after the scenario title, with +# Given/When/Then comment anchors inside the test body. Traceability is a +# reviewer-verifiable convention, not a build-enforced link. +# +# Skills are prose contracts, so what an agent *does* with them cannot be +# verified here — that is the conformance and evaluation layer of #65. What can +# be verified is the wiring those contracts depend on: that a skill delegating +# to another one still points at a file that exists, and that a rule delegated +# away has not been copied back. Both fail loudly when someone edits one skill +# and forgets the other. + +Feature: Skill wiring + As a toolkit maintainer + I want delegation between canonical skills to stay intact + So that a skill that defers a rule still reaches the skill that owns it + + Scenario: Every skill-to-skill reference resolves + Given the canonical skills under skills/ + When their relative references to other SKILL.md files are resolved + Then every referenced file exists + + Scenario: The skills that own a gate moment reach the Convergence Check + Given architecture-impact, implement-issue-workflow and pr-review + When each is inspected for the canonical gate + Then all three reference skills/convergence-check/SKILL.md + + Scenario: No caller defines any of the Convergence Check result states + Given the four result states read from the Convergence Check itself + When every other skill is searched for a definition of any of them + Then none defines one + # Naming a state is allowed and often clearer; defining it elsewhere is the + # copy that drifts. All four are checked, not just one. + + Scenario: No caller carries the Convergence Check question structure + Given the seven question titles read from the Convergence Check itself + When every other skill is searched for those titles as headings + Then none carries two or more of them + # The section heading alone would not catch a copy made without it, so the + # question titles themselves are what is checked. diff --git a/skills/architecture-impact/SKILL.md b/skills/architecture-impact/SKILL.md index 28f15af..fd65ecc 100644 --- a/skills/architecture-impact/SKILL.md +++ b/skills/architecture-impact/SKILL.md @@ -82,6 +82,10 @@ source of truth when these rules evolve. 15. Run the relevant validators, generators, render checks, tests, or manual checks. Report any unavailable verification and remaining open human decisions. +16. Before the work is treated as complete, run `../convergence-check/SKILL.md`. + Impact analysis establishes what the change touches; the gate asks whether + the request, the specification, the architecture record, the implementation + and the delivery metadata still agree afterwards. ## Required Reading @@ -197,3 +201,5 @@ Read these files when the feature or review touches the corresponding scope: risks, quality scenarios, PRs, and real sub-issues where supported? - Are validators, generators, tests, and render checks run or explicitly noted as not run? +- Does the change carry a Convergence Check result, with every finding given a + disposition and every blocker its kind, per `../convergence-check/SKILL.md`? diff --git a/skills/convergence-check/SKILL.md b/skills/convergence-check/SKILL.md index 1515272..05996e8 100644 --- a/skills/convergence-check/SKILL.md +++ b/skills/convergence-check/SKILL.md @@ -165,6 +165,26 @@ waivers, or not converged. An unavailable blocker does not resolve by waiting, and reporting it as pending sends someone to wait for something that will not arrive. +### A standing limitation is not this change's blocker + +A blocker is about **this** change: evidence this change owes and cannot +produce. + +A limitation that would appear identically on every change of its kind is not a +blocker. No execution layer for prose contracts, no environment to run a +particular check in, a tool that does not exist yet — these are properties of +the medium or the tooling, not gaps in the change under review. They are +**residual risks**, and question 6 already requires reporting them, together +with the follow-up that would remove them. + +The test is one question: *would this same blocker appear on every change of +this kind?* If yes, blocking on it stops all such work indefinitely while +changing nothing about the change in front of you — and a gate that can never +be passed is not a gate, it is a stop. + +Report the limitation, name the follow-up, and answer the question on the +evidence that **can** be produced here. + ### A waiver is not a way out of a blocker A waiver is a human decision about **proportionality** — "this is too small to diff --git a/skills/implement-issue-workflow/SKILL.md b/skills/implement-issue-workflow/SKILL.md index 3a3e4df..4c7db1d 100644 --- a/skills/implement-issue-workflow/SKILL.md +++ b/skills/implement-issue-workflow/SKILL.md @@ -63,6 +63,12 @@ If useful work was already done on `main`, stash or otherwise preserve only that 3. Push the `issue_` branch to the remote repository. 4. Open a pull request against `main`. 5. Include the issue link, implementation summary, and verification results in the pull request body. +6. Optionally run `../convergence-check/SKILL.md` now and record its result and + finding dispositions in the pull request body. Mark it **provisional**: the + authoritative run happens before integration, because review comments and + later commits change the state it inspects. That skill owns the questions, + the result states, and what counts as evidence; this workflow only says when + to run it. ## Address PR Comments @@ -70,6 +76,9 @@ If useful work was already done on `main`, stash or otherwise preserve only that 2. Group fixes into useful commits by intent. 3. Push the new commits to the existing PR branch. 4. Re-run and report relevant verification. +5. A recorded Convergence Check result is void once new commits land. Re-run + `../convergence-check/SKILL.md` and replace the recorded result rather than + amending it — a result describes one state of the branch, not the branch. ## PR Integration @@ -86,10 +95,18 @@ unless the user explicitly requests squash integration or repository policy requires it. Use merge commits only when the user explicitly requests them or repository policy requires merge commits. +The authoritative Convergence Check runs here, immediately before the +integration decision, against the commit that would be integrated. A result +recorded earlier is evidence about an earlier state: if any commit landed after +it, it is stale, and a stale result is not a result. Re-run +`../convergence-check/SKILL.md` and record the outcome before deciding. + Do not integrate or clean up branches automatically when there are warning signs, such as failed or unknown checks, unresolved merge state, a diverged -local branch, unpushed local commits, uncertain PR state, or unclear branch -ownership. Report the situation and let the developer decide. +local branch, unpushed local commits, uncertain PR state, unclear branch +ownership, a Convergence Check result that is anything other than converged or +converged with recorded waivers, or a result that does not cover the current +head commit. Report the situation and let the developer decide. ## After PR Integration diff --git a/skills/pr-review/SKILL.md b/skills/pr-review/SKILL.md index 4d5a96a..b396e6b 100644 --- a/skills/pr-review/SKILL.md +++ b/skills/pr-review/SKILL.md @@ -33,12 +33,18 @@ review workflow needs a stricter rule. markings stay aligned. 6. Run relevant validators, tests, linters, render checks, or targeted commands when available and reasonable for the changed files. -7. Prioritize defects, behavioral regressions, broken contracts, missing +7. Check the Convergence Check result the pull request reports, per + `../convergence-check/SKILL.md`. A result claimed without the evidence behind + it, a finding left without a disposition, or a blocker reported without its + kind is itself a review finding. When the pull request reports no result and + the change went through a feature or implementation workflow, say so rather + than supplying one from the review. +8. Prioritize defects, behavioral regressions, broken contracts, missing verification, unsafe assumptions, stale links, and traceability problems. -8. Write findings first, ordered by severity. Keep summaries secondary. -9. Prefer precise file and line references for actionable findings. -10. Separate blocking findings from non-blocking suggestions and questions. -11. If no actionable findings are found, say so clearly and still report any +9. Write findings first, ordered by severity. Keep summaries secondary. +10. Prefer precise file and line references for actionable findings. +11. Separate blocking findings from non-blocking suggestions and questions. +12. If no actionable findings are found, say so clearly and still report any residual risk or verification that could not be performed. ## Review Focus @@ -73,6 +79,9 @@ Read these files when the PR touches the corresponding scope: PRs, or refactoring PRs. - `../traceability-review/SKILL.md` before reviewing relation metadata or traceability changes. +- `../convergence-check/SKILL.md` before judging a reported convergence result, + so the review checks it against the canonical definition rather than a + remembered one. - `../adr/SKILL.md`, `../quality-scenario/SKILL.md`, or `../risk/SKILL.md` before reviewing those artifact types. - `../../adapters/github-copilot/README.md` when GitHub Copilot performs the diff --git a/test/skill-wiring.test.mjs b/test/skill-wiring.test.mjs new file mode 100644 index 0000000..ad394b8 --- /dev/null +++ b/test/skill-wiring.test.mjs @@ -0,0 +1,173 @@ +// Behaviour specification for the wiring between canonical skills. +// +// Skills are prose contracts. Whether an agent obeys one is not verifiable from +// this repository — that is the conformance and evaluation layer tracked in #65. +// What is verifiable, and what breaks silently without a guard, is the wiring +// underneath: a skill that defers a rule must still point at the file that owns +// it, and a rule that was deferred must not have been copied back. +// +// Both checks fail on a real regression: deleting a reference, renaming a skill +// directory, or pasting the canonical rules into a caller. +// +// Bridged from: features/skill-wiring.feature + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); +const skillsDir = path.join(repoRoot, "skills"); + +// Every SKILL.md in the tree, at any nesting depth. +function skillFiles(dir = skillsDir) { + return fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return skillFiles(full); + return entry.name === "SKILL.md" ? [full] : []; + }); +} + +function read(file) { + return fs.readFileSync(file, "utf8"); +} + +function rel(file) { + return path.relative(repoRoot, file); +} + +// Relative references to other skills, as they are written in the prose: +// `..//SKILL.md` inside backticks. +function skillReferences(file) { + const text = read(file); + const matches = text.match(/`(\.\.\/[A-Za-z0-9_./-]*SKILL\.md)`/g) || []; + return matches.map((m) => m.slice(1, -1)); +} + +test("Every skill-to-skill reference resolves", () => { + // Given the canonical skills under skills/ + const files = skillFiles(); + assert.ok(files.length > 0, "no skills found"); + + // When their relative references to other SKILL.md files are resolved + const broken = []; + let checked = 0; + for (const file of files) { + for (const reference of skillReferences(file)) { + checked += 1; + const target = path.resolve(path.dirname(file), reference); + if (!fs.existsSync(target)) broken.push(`${rel(file)} -> ${reference}`); + } + } + + // Then every referenced file exists + assert.ok(checked > 0, "no skill-to-skill references found to check"); + assert.deepEqual(broken, []); +}); + +test("The skills that own a gate moment reach the Convergence Check", () => { + // Given architecture-impact, implement-issue-workflow and pr-review + const callers = ["architecture-impact", "implement-issue-workflow", "pr-review"]; + + // When each is inspected for the canonical gate + const missing = callers.filter((name) => { + const file = path.join(skillsDir, name, "SKILL.md"); + return !skillReferences(file).includes("../convergence-check/SKILL.md"); + }); + + // Then all three reference skills/convergence-check/SKILL.md + assert.deepEqual(missing, []); +}); + +// The gate's own rule, applied to the gate: a second copy of a rule inside a +// caller is exactly the drift the gate exists to detect. These two tests are +// what stops that copy from being made quietly. +const canonical = path.join(skillsDir, "convergence-check", "SKILL.md"); + +function skillsContaining(pattern) { + return skillFiles() + .filter((file) => pattern.test(read(file))) + .map(rel); +} + +// Both checks read the canonical file for what to look for, rather than +// hard-coding it. Rename a result state or a question and the guard follows; +// hard-coded copies would quietly stop guarding anything. +function canonicalResultStates() { + const section = read(canonical).split("## Result")[1].split("###")[0]; + return [...section.matchAll(/^\|\s*\*\*([^*]+)\*\*\s*\|/gim)].map((m) => + m[1].trim(), + ); +} + +function canonicalQuestionTitles() { + const section = read(canonical) + .split("## The seven questions")[1] + .split("\n## ")[0]; + return [...section.matchAll(/^###\s+\d+\.\s+(.+?)\s*$/gim)].map((m) => m[1]); +} + +function escapeForRegex(text) { + return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +test("The Convergence Check result states live in exactly one skill", () => { + // Given the canonical skills under skills/, and the four result states read + // from the Convergence Check itself + const states = canonicalResultStates(); + assert.equal(states.length, 4, `expected four result states, got ${states.join(", ")}`); + + // When they are searched for a definition of any of those states + // + // Naming a state elsewhere is allowed and often clearer: implement-issue-workflow + // says which results stop integration, which is its own policy expressed in the + // gate's vocabulary. What must not spread is the *definition* — the table row + // saying what a state means. That is the copy that drifts. + const offenders = states.flatMap((state) => { + const row = new RegExp(`^\\|\\s*\\*\\*${escapeForRegex(state)}\\*\\*\\s*\\|`, "im"); + return skillsContaining(row) + .filter((file) => file !== rel(canonical)) + .map((file) => `${file} defines "${state}"`); + }); + + // Then only the Convergence Check itself defines them + assert.deepEqual(offenders, []); +}); + +test("The Convergence Check questions are not copied into a caller", () => { + // Given the canonical skills under skills/, and the seven question titles read + // from the Convergence Check itself + const titles = canonicalQuestionTitles(); + assert.equal(titles.length, 7, `expected seven questions, got ${titles.length}`); + + // When every other skill is searched for those titles as headings + // + // A caller could copy the questions without the section heading above them, + // so the heading alone is not what is checked. Two or more of the canonical + // titles appearing as headings in one file is the copy; a single shared word + // such as "Traceability" is not. + const offenders = skillFiles() + .filter((file) => file !== canonical) + .map((file) => { + const text = read(file); + const hits = titles.filter((title) => + new RegExp(`^#+\\s+(\\d+\\.\\s+)?${escapeForRegex(title)}\\s*$`, "im").test(text), + ); + return { file: rel(file), hits }; + }) + .filter((entry) => entry.hits.length >= 2) + .map((entry) => `${entry.file} carries ${entry.hits.length} question headings`); + + // Then only the Convergence Check itself carries the structure + assert.deepEqual(offenders, []); + // ... and the canonical file still has all seven, so the guard has something + // to compare against. + const canonicalHits = titles.filter((title) => + new RegExp(`^###\\s+\\d+\\.\\s+${escapeForRegex(title)}\\s*$`, "im").test(read(canonical)), + ); + assert.equal(canonicalHits.length, 7); +});