Skip to content

feat(fabrika): land skill-doctor — byte-exact upstream vendor, fabrika-shaped (#8048) - #8063

Merged
usirin merged 5 commits into
mainfrom
build/8048-land-skill-doctor
Sep 20, 2026
Merged

usirin merged 5 commits into
mainfrom
build/8048-land-skill-doctor

Conversation

@creosB

@creosB creosB commented Sep 5, 2026 •

Copy link
Copy Markdown
Member

Fixes #8048

Landing child of #8035 (epic). Vendors the Warp Skill Doctor (MIT, warpdotdev/common-skills @ b811c243) byte-exact into claude-plugins/fabrika/skills/skill-doctor/, already shaped to skill-conventions. Status: tests/lint passed; leak scan ran and failed on path-policy matches — the scan refused two doc surfaces on agent home-dir path literals: contract.md (sanitized in e715358e) and the byte-exact upstream references/supported-harnesses.md (held as vendored; its refusal stands until upstream changes the text). The test adaptation is a proposal awaiting maintainer acceptance (see "Proposed adaptation" below).

Byte-exact vendor — git blob SHAs vs upstream tree @ b811c24365ae

File Blob SHA
assets/pierre-diffs.js 1ecc2c99…a25400538a97c2b21a2a172d
assets/warp-pixel-icon.svg 0ed8d084…7aeb0e46ec53
references/skill-improvements.md bbf1dace…f1661d4f
references/supported-harnesses.md e8f589b7…316e37ec50
scorers/code-quality.md 59179f8d…535c5afb6c5b14c37a
scorers/efficiency.md 28648134…019a8284bbe9a04
scripts/collect_sessions.py f24fb541…2aa335835053
scripts/render_report.py 972da3be…4c4ed5a2ab6f
scripts/test_collect_sessions.py f659ac6e…b4ce4244d
scripts/test_render_report.py 46658d0f…5153dcb6c3c30204
scripts/warp_decoder.py c54cebdd…269e0aac11c5498b1f
LICENSE 00bd0da9…4ba8565fc (upstream repo-root MIT)

Upstream's own SKILL.md (9fd7d77d…f598d) is deliberately not vendored — per conventions §1/§2 the landing authors a fabrika routing surface instead. Full SHAs in PROVENANCE.md.

Fabrika-authored (editable per PROVENANCE.md)

CI — replacement verified locally; PR CI pending

The workflow now runs two separate test targets; the job fails if either fails. No skips, no failure allowlists, no continue-on-error, no output parsing.

  • Target 1 — upstream baseline. Both unchanged upstream suites run in an isolated temporary tree: the byte-exact vendored files plus upstream's original SKILL.md, downloaded from the pinned commit b811c24365ae505bfc9646458957b886e29110b5 and hash-verified (9fd7d77d…) before use. The shipped fabrika SKILL.md never enters that tree. Verified locally: 14/14 + 11/11 green, including the three SKILL.md-coupled packaging tests, against upstream's own artifact.
  • Target 2 — fabrika conformance. scripts/test_fabrika_conformance.py (fabrika-authored, PROVENANCE-listed) runs 13 doc-level tests over the shipped SKILL.md/contract.md: session scoping, the scoring contract with label-to-score mappings and aggregation weights, the failed-conversation threshold and edit gate, section addressability, documented collector/discovery limitations, and §4 plain-literal commands. Verified locally: 13/13 green. Doc-level only — it proves the contracts exist where the skill reads them, not runtime behavior.

Failure propagation demonstrated locally: a deliberately broken collector fails the step before the renderer runs (renderer verified passing standalone in the same broken tree); wrong pinned SHA, download 404, and a conformance file planted in the baseline tree each fail their guards.

guard skill-lint: verified by CI

The gate fails closed with zero-scope on the filer's Windows machine — the identical red reproduces on untouched main, so it is a local walker limitation, not this diff. Manual gate-1/3/4 greps over the new directory are clean; frontmatter hand-verified. The authoritative run on ubuntu (this PR's CI, 2026-09-06) passed.

.gitignore

Rows for the skill's results/ directory (transcripts + rendered report embed transcript-derived content and machine-local paths — never committed) and for __pycache__/ (Python enters the repo with this child).

Proposed adaptation — awaiting maintainer acceptance

The two-target design above is the proposed adaptation, landed in commit 70dad2a8 for review: upstream's packaging assertions run against upstream's own artifact (baseline target), while fabrika-authored conformance tests check the replacement surface (target 2). Acceptance is the maintainer's call: if accepted, the baseline target supersedes running the three upstream packaging assertions against the replaced SKILL.md in place; if rejected, the alternatives (upstream path-parameter patch, or vendoring upstream's startup/branding copy into the fabrika body) come back into scope.

Repair round 1 — portability

The branch is now merged up to date with main (it was 417 commits behind, so the guards that landed since never ran on it). The fabrika-authored SKILL.md, contract.md and PROVENANCE.md no longer carry any ticket number, issue URL or declared repo name; the gaps they used to name by ticket are named by what is missing instead. scripts/test_fabrika_conformance.py moved with them: the two assertions that pinned removed ticket numbers now pin that prose. Local verdicts at 53e142c3: guard portability-guard check clean (1449 files), guard skill-lint check clean (95 files), build check --surface prose|code|workflows green, conformance 13/13, upstream baseline 14/14 + 11/11 in an isolated tree, and all 11 vendored blob SHAs plus LICENSE re-verified against warpdotdev/common-skills @ b811c243 through the upstream tree API.

Deviations

  • Guard or gate bypassed — Said: criterion 11 asks that the shipped corpus carry no reference that resolves in one repository only. Did: assets/pierre-diffs.js keeps 138 matches and is carried as one exempt row in portability-guard.config.json naming that exact file. Why: every match is a CSS hex colour on the minified line, and the file is byte-exact upstream under the re-copy-never-edit rule, so it cannot be edited here. Disposition: stated here, and the row states the same reason in its why.
  • Out-of-scope change — Said: criterion 11 names the portability guard. Did: also added one leak-guard exemption for the byte-exact references/supported-harnesses.md, in both halves of that policy (DOC_SELF_EXEMPT in packages/fabrika-cli/src/guard/leak.ts and docLeakExempt in .fabrika.jsonc). Why: that file documents where each harness keeps its session history, so the home-dir literal is its subject matter; merging current main brings the file into the leak gate's scope, and editing it is forbidden. Disposition: stated here; leak.golden.test.ts holds the two halves equal and passes.
  • Declined guidance — Said: upstream's two unittest suites run in CI unchanged. Did: they run against upstream's own SKILL.md in an isolated baseline tree, not against the fabrika-authored SKILL.md that replaces it. Why: three of upstream's assertions read its own packaging copy, which conventions §1/§2 require this skill to replace; running them against the replacement would assert prose the skill deliberately does not carry. Disposition: stated here; the replacement contracts are covered by the fabrika conformance target instead.
  • Pre-existing test or fixture changed — Said: repair the docs for criterion 11. Did: also changed two assertions in scripts/test_fabrika_conformance.py that hard-pinned the removed ticket numbers. Why: the docs no longer carry those numbers, so the assertions would red target 2 of the workflow. Disposition: the assertions now pin the portable prose that replaced them; 13/13 green.

…a-shaped (#8048)

Vendors Warp Skill Doctor (MIT, warpdotdev/common-skills @ b811c243) byte-exact
(blob-SHA verified): scorers, references, collectors, renderer, assets, both
unittest suites, LICENSE. Upstream's SKILL.md is deliberately not vendored —
SKILL.md is authored fresh as the fabrika routing surface (conventions §1/§2)
with contract.md carrying collector flags, the scoring contract, and report
artifacts by section (ADR 0296). PROVENANCE.md carries the byte-exact/editable
split and the re-copy re-sync rule. A new CI job runs both upstream suites
(#8048 §8); .gitignore covers the skill's results dir and Python bytecode.
Inert on arrival: opencode collector is #8049, corpus discovery #8051.
@github-actions

github-actions Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

No preview deploy

  • No preview deploy for this PR — its diff touches no deploy-relevant path, so no preview stack was minted and e2e is not applicable. (53e142c)
  • web — Stage pr-8063 torn down.

…nformance (#8048)

Proposed test adaptation awaiting maintainer acceptance. Target 1 runs both
unchanged upstream suites in an isolated temp tree against upstream's own
SKILL.md, downloaded from the pinned commit b811c243 and hash-gated before use —
the shipped fabrika SKILL.md never enters that tree. Target 2 runs a new
fabrika-authored conformance suite (13 doc-level tests, PROVENANCE-listed) over
the shipped SKILL.md/contract.md: session scoping, scoring contract with
label-to-score mappings and aggregation weights, failed-conversation threshold
and edit gate, section addressability, documented limitations, §4 literal
commands. Workflow gains shell: bash + set -euo pipefail; explicit guards for
download failure, blob mismatch, and conformance-file isolation. Replacement
verified locally; PR CI pending.
…l path

The leak scan refused contract.md on a ~/.claude literal (path-policy match,
#8037/#8063 CI evidence). Describe the default by role instead; the
conformance suite stays 13/13 green locally.
@github-actions

Copy link
Copy Markdown
Contributor

heal-ci: ROUTED — PR #8063 @ e715358 → author

Scheduled stall sweep: this pull request classifies as linkage-refused, stranded 146 minute(s) at head e715358e245a77acae267a1867b82fb04830a007.

Detection only — this run merged nothing, re-ran nothing and spawned nothing. What to do about the flag is a driver decision; /fabrika:heal-ci 8063 reads the full diagnosis.

Posted by the heal-ci-sweep workflow (#6146). The suppression key <pr>:<class>:<head> rides in the marker below, matched over the whole comment history: no further note lands here until this pull request changes class or gains a new head commit.

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

governance: FAIL @ e715358 content:6428e5975ce8 — shipped corpus no longer reads the same in any repository

Governance verdict — FAIL

Scope. governance scope 8063 derives the namespace over 2 of 5 declared roots: claude-plugins/ (16 files) and .github/ (1). self false — this diff does not edit the governance skill or its contract, so §4's fence does not apply and the rules applied are the head's.

Corpus half — one contradiction, hand-read

No decision record is in this diff, so there was no subject to rank and sweep was not run; the corpus half below is a hand read, named as such.

Questions this change decides:

  1. May fabrika's shipped skill corpus carry a vendored third-party skill under a re-copy-never-edit provenance table? Nothing standing forbids it. PROVENANCE.md states the pin, the per-file blob SHAs, and the re-sync procedure, and ADR 0273 (live, accepted — fabrika ships as an installed plugin) is not contradicted by vendoring under that discipline.
  2. May a fabrika skill ship its own Python scripts and its own CI interpreter setup? The repo's tooling rule names Node and the Effect CLI for repository tooling; this is a vendored skill's own runtime, not repository tooling, and the workflow's comment cites the epic's R1.1 ruling for the interpreter step. Named here as evidence read, not as ground for a verdict.
  3. May text shipped under claude-plugins/fabrika/ carry references that resolve only in this repository? Standing law says no, and this diff says yes. That is the contradiction.

Evidence, run live, not inferred. With this PR's skill-doctor/ directory grafted into a current checkout, fabrika guard portability-guard check reds:

  • unmigrated row plugin-tail carries 207 references over a ceiling of 6. The row's floor only shrinks, so every reference this diff adds under it reds on its own.
  • The additions are this PR's own files: SKILL.md (#8035, #8049, #8051, #8052, #8054, each as a github.com/kamp-us/phoenix/issues/ URL), PROVENANCE.md (#8048–#8052), contract.md (#8048, #8049, #8051, #8052, #8054), plus kamp-us and phoenix as names this repo declares under portability.repoNames, and ~130 hits inside the byte-exact assets/pierre-diffs.js (hex colours on a minified line, which the guard's hex carve-out does not reach there).

This contradicts the standing rule that fabrika's shipped corpus must read the same in any repository — the rule ADR 0381 (live, accepted) puts behind a pre-merge local gate explicitly covering shipped markdown, and the rule ADR 0394 (live, accepted) applies in its narrower packages/fabrika-cli/ form. A reader in an installed repository cannot follow any of those pointers.

The guard did not exist at this branch's base (fe039d1b, 2026-09-05; the head is 417 commits behind main), so this is not authorship carelessness — it is standing law that moved under an open PR. It is still standing law at merge time, and no record authorizes the exception.

Note for the repair, not a second finding. scripts/test_fabrika_conformance.py hard-asserts #8049 and #8051 are present in SKILL.md, so satisfying the guard by removing the issue references will red that test. The two have to be repaired together, and the byte-exact assets/pierre-diffs.js hits cannot be edited at all under the re-copy-never-edit rule — they need an exempt row naming that exact file, with its why.

Gate half — empty reach, recorded

governance guards 8063 --sha e715358e reports 0 anchored invariants in reach across the 18 files, with one guard-bearing file compared block-by-block against the base: .github/workflows/skill-doctor-tests.yml, which is new and removes nothing. No gate invariant is in this diff's reach; nothing is removed, softened or made conditional. The new workflow adds a gate rather than weakening one — no continue-on-error, no skips, no failure allowlist, and a hash check on the one artifact it downloads.

Whether this diff needs a code-owner approval is a separate question CODEOWNERS answers.

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

review-skill: FAIL @ e715358 content:6428e5975ce8 — shipped skill text reds portability-guard; deviations disclosure absent

review-skill — FAIL (round 1)

Scope: 16 skill-class files at e715358e (base fe039d1b). Every touched skill file was read whole out of the object database at that commit; the 11 vendored files plus LICENSE are byte-exact against warpdotdev/common-skills @ b811c243 by blob hash, so the graded surface is the four fabrika-authored files — SKILL.md, contract.md, PROVENANCE.md, scripts/test_fabrika_conformance.py.

Findings

1. guard portability-guard check reds on this diff's own shipped files. Run live with this PR's skill-doctor/ directory grafted into a current checkout: the unmigrated row plugin-tail carries 207 references over a ceiling of 6, and a ceiling is a count, so every reference this diff adds under it reds on its own. The additions:

  • SKILL.md lines 18, 21, 26, 73 — #8035, #8049, #8051, #8052, #8054, each written as a https://github.com/kamp-us/phoenix/issues/… URL, plus kamp-us and phoenix as names this repo declares under portability.repoNames.
  • contract.md lines 3, 29, 32, 108, 109 — the same shape, #8048 included.
  • PROVENANCE.md lines 8, 42, 46–49 — #92 (upstream's own pull-request reference) and #8048–#8052 with their issue URLs.
  • assets/pierre-diffs.js — ~130 hits, all hex colours on a minified line that the guard's hex carve-out does not reach. This file is byte-exact upstream and must not be edited; it needs an exempt row naming that exact file with its why.

The guard did not exist at this branch's base (2026-09-05; the head is 417 commits behind main), so this is standing law that moved under an open PR rather than careless authorship — but it is standing law at merge time, and CI at a rebased head will red on it. Blocking.

2. The repair is two-sided, and the second side is in this diff. scripts/test_fabrika_conformance.py::DocumentedLimitations asserts "#8049" in SKILL and "#8051" in SKILL, and test_contract_documents_the_missing_opencode_collector asserts "#8049" in CONTRACT. Removing the references to satisfy the guard turns target 2 of skill-doctor-tests.yml red. Both have to move together: state the two gaps in prose that names no ticket, and re-point the assertions at that prose. Routed as acceptance criterion 11 on #8048.

3. ## Deviations is absent from the PR body. review deviations reads no heading reaching for it. Absent is not None. — it is malformed and fails closed, in this namespace and in review-code. The body's "Proposed adaptation" section is deviation-shaped content (the two-target test design explicitly replaces running upstream's three packaging assertions against the replaced SKILL.md), which makes the missing section a formatting gap over real substance rather than an empty one. Put it under the heading the gate reads.

What passed

  • Criterion 1 — vendored tree byte-exact at the pinned commit, per-file blob hashes recorded in both PROVENANCE.md and the PR body. Criterion 2 — LICENSE is upstream's unmodified MIT. Criterion 3 — PROVENANCE.md names the ref and commit and splits byte-exact from fabrika-authored per file, with the re-copy-never-edit rule and a four-step re-sync procedure.
  • Criterion 4 — SKILL.md is fabrika-authored, routes rather than inlines, and points at contract.md by section through wire doc-section rather than restating the flag table or the formulas. Frontmatter is disable-model-invocation: true with a description that says what it grades and from what.
  • Criterion 5 — contract.md carries ## Collector flags, ## Scoring contract and ## Report artifacts, each addressable, each verbatim-matched by the conformance suite.
  • Criterion 6 — every bash fence in SKILL.md is a plain literal: no $, no default-expansion, no .. climb, and the conformance suite asserts that property rather than trusting it.
  • Criterion 8 — guard skill-lint check run live over the grafted tree: clean across 95 files, no SELF_EXEMPT_SUFFIXES row needed. The PR body's account of a local zero-scope red on the filer's machine is consistent with the green CI run.
  • Criterion 10 — SKILL.md states both arrival gaps (no opencode collector; corpus outside the three auto-discovered skills roots) and names the child that closes each, and its terminal vocabulary makes NO-SESSIONS an expected stop rather than a failure. Good design for an inert-on-arrival skill.

Editorial (writing-for-agents)

The authored surface reads well and in fabrika's voice: imperative steps, a Done when per step, a closed terminal vocabulary, a stated report boundary, and no second copy of what contract.md owns. The **Inert on arrival, by design.** block is the right shape — it tells a reader why an empty run is correct before they run it. No editorial finding. One note, not a finding: the boundary rule appears in SKILL.md and again in contract.md's Report artifacts; the second is the addressable one, so the first could shrink to a pointer on a later pass.

Criteria still open at this head

Criteria 1–6, 8 and 10 are discharged as above; 7 and 9 are graded in review-code. Criterion 11 (appended this round) is open by construction.

Verdict-written: 2026-09-20T19:35:12Z

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

review-code: FAIL @ e715358 content:6428e5975ce8 — deviations disclosure absent; head blocked at governance floor

review-code — FAIL (round 1)

Scope: 2 code-class files at e715358e (base fe039d1b) — .github/workflows/skill-doctor-tests.yml (new, 94 lines) and .gitignore (+9).

Findings

1. ## Deviations is absent from the PR body. review deviations finds no heading reaching for it. Absent is not None. — it is malformed and fails closed. It matters here rather than being a formality: the body's "Proposed adaptation" section describes a real, deliberate divergence (upstream's three SKILL.md-coupled packaging assertions are not run against the replaced SKILL.md; they are run against upstream's own artifact in an isolated baseline tree instead), and that is exactly what the section exists to carry. Move it under the heading the gate reads.

2. Blocked at head by the governance namespace, not by the tests. review ci --wait settles red at this head with 41 checks green, 3 skipped and one failure: governance floor at head, which is my own FAIL for this round. Execution evidence for this class is otherwise clean.

Criterion 7 — the CI job

Discharged at this head, and well built. .github/workflows/skill-doctor-tests.yml runs two targets in one job and the job fails if either fails: no continue-on-error, no skips, no failure allowlist, no output parsing. I confirmed the run rather than the intent — job upstream baseline + fabrika conformance (python 3.12) concluded success with both target steps green (actions/jobs/101399677226). Specific things it gets right:

  • The baseline tree is built by naming the five upstream scripts explicitly instead of copying *.py, and then asserts the fabrika conformance test did not leak into it. The isolation is checked, not assumed.
  • Upstream's SKILL.md is downloaded at the pinned commit and its blob hash compared against the pinned value before use, with curl -fsSL and an explicit failure branch on both the download and the mismatch. set -euo pipefail throughout, shell: bash as a default, permissions: contents: read, both actions pinned to a version tag.
  • Path triggers cover the skill directory and the workflow itself; concurrency cancels in-progress on non-main refs only.

One non-blocking note: target 1 reaches the network at every run, so a raw.githubusercontent.com outage reds this PR for a reason that has nothing to do with the diff. The pin plus the hash check means there is no supply-chain gap, only an availability one; if that flake ever shows up, caching the artifact by its hash is the fix. Not a finding, and not a reason to hold the round.

Criterion 9 — .gitignore

Discharged. claude-plugins/fabrika/skills/skill-doctor/results/ is ignored, and no results artifact appears in the 18 changed files. The added __pycache__/ row is beyond the criterion's letter but correct — Python enters the repo with this change — and both rows carry a comment that says why, matching the file's existing style rather than narrating the obvious.

Silent-failure, type-design, test-gap sweep

Nothing to route. The workflow has no silent-failure path: every branch that can go wrong writes to stderr and exits non-zero. There is no type surface in this class. The test gap that exists is named honestly by the author in both the workflow header and the conformance test's docstring — target 2 is doc-level only and proves nothing about runtime behaviour — and closing it is not this child's job.

Note on head age

This head is 417 commits behind main and its CI ran on 2026-09-06. The repo has since gained gates this run did not see, portability-guard among them, and that one reds on the diff — see review-skill and governance. A repair round should rebase, so criterion 7's green will need re-reading at the new head.

Verdict-written: 2026-09-20T19:35:45Z

…itory (#8048)

Drop every ticket number, issue URL and repo name from the fabrika-authored SKILL.md, contract.md and PROVENANCE.md, and carry the byte-exact upstream files that cannot be edited as per-file exemption rows instead.
@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

Repair round 1 at 53e142c. Each FAIL finding, and what it got:

  1. Head 417 commits behind main — merged origin/main into the published head (no rebase: a rebase would have dropped the three published commits, which build push refused on exit 23). The three guards that landed since now run on this branch.
  2. guard portability-guard check red (plugin-tail, 207 over a ceiling of 6) — the fabrika-authored SKILL.md, contract.md and PROVENANCE.md no longer name any ticket, issue URL or declared repo name; each gap they used to name by ticket is now named by what is missing. assets/pierre-diffs.js is byte-exact upstream and was not touched: it is carried as one exempt row naming that exact file, ceiling 138, with its reason. Guard is clean at 1449 files.
  3. scripts/test_fabrika_conformance.py — the two assertions that pinned the removed numbers now pin the portable prose that replaced them. 13/13 green, so workflow target 2 stays green.
  4. ## Deviations absent — the section is now in the body, four entries, and review deviations reads it as found. The existing text was appended to, not rewritten, with one exception: the status line in the first paragraph quoted a home-dir path literal that the body leak scan refuses, so that phrase was redacted in place. Note that build pr-body refused this PR on exit 14 ("head branch is not a lane branch"), so the body went through the API after the same leak scan had been run over it locally.
  5. Governance — same repair as 2; no @ruling docblock was in reach, since the only packages/fabrika-cli/ change is the leak-guard exemption list, which carries its reason as an inline comment beside the row like its neighbours.

One disclosed extra: merging current main brings the byte-exact references/supported-harnesses.md into the leak gate's scope, and it reds on the home-dir literals that are its subject matter. It cannot be edited, so it is exempted in both halves of that policy (DOC_SELF_EXEMPT and .fabrika.jsonc's docLeakExempt); leak.golden.test.ts holds them equal and passes.

Local verdicts at this head: portability-guard clean, skill-lint clean (95 files), build check green on prose, code and workflows, conformance 13/13, upstream baseline suites 14/14 + 11/11 in an isolated tree built the way the workflow builds it, and all 11 vendored blob SHAs plus LICENSE re-verified against warpdotdev/common-skills @ b811c243 through the upstream tree API — every one matches the table in PROVENANCE.md.

— at 53e142c

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

governance: PASS @ 53e142c content:6a9963d8b269 — no contradiction, and the two loosenings are narrow, reasoned and disclosed

Governance verdict — PASS (round 2)

Scope. governance scope 8063 at 53e142c3 derives the namespace over 3 of 5 declared roots: claude-plugins/ (16 files), .fabrika.jsonc (1) and .github/ (1). self false — this diff does not edit the governance skill or its contract, so §4's fence does not apply and the rules applied are the head's. Round 1's FAIL at e715358e is superseded below.

Corpus half — the round-1 contradiction is gone, hand-read again

No decision record is in this diff, so there was no subject to rank and sweep was not run; this half is a hand read, named as such.

Round 1 failed on one contradiction: fabrika's shipped corpus carried pointers that resolve only in this repository. Re-run live over the head tree, guard portability-guard check --root <head tree> is clean — 1449 files scanned, 343 references under a declared ceiling and none above it. The unmigrated row plugin-tail keeps its ceiling of 6; the count rose only by the one new exempt row, so the sweep floor was not raised to make room for anything. SKILL.md, contract.md and PROVENANCE.md now state the arrival gaps by what is missing — an opencode collector, corpus discovery — rather than by ticket number, which is the portable form of the same fact. The contradiction is resolved.

I re-verified the vendor myself rather than taking the repair's word: git hash-object over the 11 vendored files plus LICENSE in the head tree returns exactly the 12 blob hashes PROVENANCE.md tabulates, unchanged across the repair. The re-copy-never-edit rule held through a repair that had every incentive to break it.

Gate half — one loosening, judged on its own

governance guards 8063 --sha 53e142c3 reports 0 anchored invariants in reach across 21 files, 4 compared block-by-block against the base. No anchored invariant is removed, softened or made conditional. But the scan is blind to an unanchored one, and this diff carries two guard-policy edits that a clean scan must not be read as clearing. Both are judged here.

1. portability-guard.config.json — one exempt row for assets/pierre-diffs.js, ceiling 138. Narrow: one exact file, not a directory and not a suffix. The row is an exempt per-file cap, not a raised unmigrated ceiling, so it does not move the sweep floor — and the guard itself reds a ceiling above the count, so the number is checked rather than asserted. The reason is true and I confirmed it: every hit is a CSS hex colour on that file's minified line, which the hex carve-out does not reach there, and the file is byte-exact upstream under a re-copy-never-edit rule, so editing it is forbidden. The why states that. This is the disposition the guard's own allow-list design exists for.

2. The leak-guard exemption for references/supported-harnesses.md, in packages/fabrika-cli/src/guard/leak.ts (DOC_SELF_EXEMPT) and .fabrika.jsonc (docLeakExempt). This is the one to look hard at, and it holds up.

  • Narrow, on both halves. One exact repo-relative file path in each list. No directory prefix, no suffix rule, nothing that reaches a second file.
  • Two declarations is the required shape, not a double loosening. leak.ts's own docstring states that markdown members of DOC_SELF_EXEMPT are declared a second time in docLeakExempt for the in-tree predictor that scans markdown only, and that leak.golden.test.ts holds the two lists equal so a doc added to one side and not the other reds. A one-sided edit would have been the finding; this is the shape the guard demands.
  • It fits the list's stated membership rule, though not at its centre. The list is "files whose subject IS path hygiene: they must spell the forbidden shapes out". This file documents where each supported harness keeps its session history, so the home-directory literal genuinely is its subject matter. That is a fair fit rather than a stretch, and the list already carries a member on exactly that reasoning (/CLAUDE.md, exempt because its Lineage section names sibling-repo clones).
  • No other route existed. The file is byte-exact upstream; editing it is forbidden by the same rule this PR is built on, and upstreaming a change for a vendoring repo's local gate is not a repair path. Merging current main is what brought the file into the gate's scope, so the exposure is a side effect of getting current, not an attempt to dodge anything.
  • It is disclosed. The PR body's ## Deviations section carries it as an out-of-scope change with the same reason and names the golden test. A weakening a reviewer has to find is a different act from one the author names.

Does it need its own ruling? No. ADR 0398 (live, accepted) is the record that governs this key: it names docLeakExempt among the keys a guard's scope is read from and rules that a machine-local layer must never shrink a guard's scope or authority. This exemption is in the tracked .fabrika.jsonc and tracked CLI source, landing through a reviewed PR with its reason in the row — the visible, reviewable route that record deliberately preserves. ADR 0294 (live, accepted) points the same way: config narrows, it never replaces. Per-row exemptions are the mechanism these records establish, and no standing record asks for a decision record per row.

One note, not a finding and not this PR's to fix: ADR 0286 (live, accepted) puts a repo-specific set in config rather than in CLI source, and this policy lives in both by design, kept honest by a golden test. That tension predates this diff.

The empty answer, written down

Beyond the two edits above, no gate invariant is in this diff's reach. The new workflow adds a gate rather than weakening one — no continue-on-error, no skips, no failure allowlist, a hash check on the one artifact it downloads — and nothing in the diff removes a check, relaxes a matcher, or makes a refusal conditional.

Whether this diff needs a code-owner approval is a separate question CODEOWNERS answers.

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

review-skill: PASS @ 53e142c content:6a9963d8b269 — portability clean, vendor byte-exact, criteria discharged

review-skill — PASS (round 2)

Scope: 16 skill-class files at 53e142c3 (base 0cb9588f, main merged in). Every touched skill file was read whole out of the object database at that commit. Round 1's FAIL at e715358e is superseded; both findings that carried it are cleared, and I re-proved each rather than reading the repair's account of it.

Round 1 finding 1 — portability: cleared, verified live

guard portability-guard check --root <head tree> over an archive of 53e142c3: clean — 1449 files scanned, 343 references under a declared ceiling and none above it.

What I checked beyond the exit code, because an allow-list is a floor that only shrinks and "clean" can be bought:

  • The unmigrated row plugin-tail still carries a ceiling of 6, unchanged. The sweep floor was not raised to make room for this diff.
  • The count moved from 205 to 343 by exactly the one new exempt row — assets/pierre-diffs.js, ceiling 138 — and the guard reds a ceiling above the count, so that number is checked rather than asserted. One exact file, never a directory.
  • The row's why is true: every hit is a CSS hex colour on that file's minified line, which the hex carve-out does not reach there, and the file is byte-exact upstream under the re-copy-never-edit rule, so editing it is forbidden. The cap is the only honest disposition, and the PR body's ## Deviations says so in the same words.
  • The fabrika-authored files carry nothing: no ticket number, no issue URL, no declared repo name in SKILL.md, contract.md or PROVENANCE.md.

Criterion 11 (appended round 1) is discharged, including its second clause.

Round 1 finding 2 — the conformance test moved with the docs

DocumentedLimitations no longer pins #8049/#8051. It now asserts "Inert on arrival", "opencode" and "--skills-dir" in SKILL.md, and "opencode" plus "**absent**" in contract.md, under a docstring that says why: the skill ships to any repository, so the gaps are named by what is missing. The replacement assertions pin the prose that carries the fact rather than the sentence shape around it — the right level. Target 2 is green in CI at this head (13 tests, upstream baseline + fabrika conformance (python 3.12), pass).

The vendor survived the repair

I re-ran git hash-object over the 11 vendored files plus LICENSE in the head tree. All 12 match PROVENANCE.md's table exactly and are unchanged from round 1. A docs repair under portability pressure is precisely when a byte-exact rule gets quietly broken, and it was not: the only edits landed in the four fabrika-authored files.

Criteria at this head

  • 1 — vendored tree byte-exact at the pinned commit, blob hashes recorded in PROVENANCE.md and the PR body, re-verified by me here. 2 — LICENSE is upstream's unmodified MIT. 3 — PROVENANCE.md names the upstream ref and commit, splits byte-exact from fabrika-authored per file, and carries the re-copy-never-edit rule plus a four-step re-sync procedure. The "later work" paragraph now names the four follow-ups by what they do rather than by number, and still tells anyone who finds themselves editing a byte-exact file to stop.
  • 4 — SKILL.md is fabrika-authored, routes rather than inlines, points at contract.md by section through wire doc-section. 5 — the three contract sections exist and are addressable, verbatim-matched by the suite. 6 — every bash fence in SKILL.md is a plain literal: no $, no default-expansion, no .. climb, and the suite asserts that property rather than trusting it.
  • 8 — guard skill-lint check --root <head tree>: clean across 95 files, no SELF_EXEMPT_SUFFIXES row needed. Green in CI at this head too.
  • 10 — discharged in substance, and its letter is superseded by criterion 11. The criterion asks that SKILL.md state both arrival gaps and name the children that fix each. The gaps are stated plainly; the child numbers are gone, because criterion 11 — which I appended in round 1 — requires their removal, and standing law forbids them in shipped text. I created that conflict, so I am naming it rather than grading around it: the substance is met, the naming clause is unsatisfiable under the newer criterion, and nothing is owed here.

Editorial (writing-for-agents)

The de-ticketing was done well, which is not the usual outcome. "until an opencode collector lands" and "until that root is discovered by default" carry the same fact as the numbers did, and read better to someone who cannot open the ticket. The NO-SESSIONS terminal was rewritten too — "while the harness this setup runs under has no collector" instead of naming one repo's issue — so the closed vocabulary stayed honest rather than going vague. No editorial finding.

Two cosmetic notes, neither a finding and neither worth a round: PROVENANCE.md's "at a new pinned commit" sentence and contract.md's "its share posture is ruled. Findings text," now wrap awkwardly where the links were removed. And the report boundary still appears in both SKILL.md and contract.md; the second is the addressable one, so the first could shrink to a pointer whenever that file is next open.

Verdict-written: 2026-09-20T20:03:22Z

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

review-code: PASS @ 53e142c content:6a9963d8b269 — CI green at head, deviations disclosed, guard edits narrow

review-code — PASS (round 2)

Scope: 5 code-class files at 53e142c3 (base 0cb9588f, main merged in) — .github/workflows/skill-doctor-tests.yml, .gitignore, and three new to this round: .fabrika.jsonc (+1), packages/fabrika-cli/src/guard/leak.ts (+4), portability-guard.config.json (+4). Round 1's FAIL at e715358e is superseded.

Round 1 finding — the deviations disclosure now exists, and it is honest

review deviations returns found with four entries, and I matched each against what I saw in the diff rather than against its class label:

Entry Substance Matches the diff
Guard or gate bypassed assets/pierre-diffs.js keeps 138 matches under one exempt row naming that exact file Yes — the row is in portability-guard.config.json with that ceiling and that reason
Out-of-scope change a leak-guard exemption for references/supported-harnesses.md, in both DOC_SELF_EXEMPT and docLeakExempt Yes — and it names both halves, which is the part a reader would otherwise have to find
Declined guidance upstream's suites run against upstream's own SKILL.md, not the replacement Yes — the baseline target builds an isolated tree and asserts the conformance test did not leak into it
Pre-existing test changed two assertions in test_fabrika_conformance.py re-pinned off the removed ticket numbers Yes — DocumentedLimitations now pins prose

The section discloses the loosening a reviewer is most likely to miss, in the entry class that fits it, with the reason and the test that holds it honest. That is what the section is for. deviation-disclosure: PASS here means nothing undisclosed that this gate could see.

The three new code files

portability-guard.config.json — one exempt row, one exact file, ceiling 138 with a why. It does not touch the unmigrated floor: plugin-tail's ceiling is still 6. The guard reds a ceiling above its count, so the number is checked by the tool rather than trusted.

leak.ts + .fabrika.jsonc — the same file exempted on both halves of one policy. That is the required shape, not a doubled loosening: leak.ts's own docstring says markdown members are declared a second time in docLeakExempt for the markdown-only in-tree predictor, and leak.golden.test.ts holds the two equal so a one-sided edit reds. packages unit tests is green at this head, so that golden test ran and passed. The new entry carries a three-line comment in the shape the list's existing entries use — what the file is, why its subject matter is the forbidden shape, and why editing it is not an option. Judged in full under the governance namespace's gate half, including whether it needs its own ruling; the short answer is that it is narrow, reasoned, disclosed and consistent with ADR 0398, and it does not.

A note on .fabrika.jsonc — the new row sits in the tracked config, reviewed in this PR, which is exactly the visible route ADR 0398 preserves when it rules that a machine-local layer must never shrink a guard's scope. A local overlay would have been the finding; this is not one.

Criterion 7 — the CI job, re-read at this head

Discharged. The workflow is byte-identical to round 1 and I confirmed the run rather than the intent: upstream baseline + fabrika conformance (python 3.12) passes at 53e142c3, both target steps green. review ci --wait settles green over 46 check runs — 42 success, 4 skipped, none failing — with 29 of 40 repo-authored workflows having inspected this head, so the green carries gate coverage rather than resting on an empty enumeration.

The design still reads well: two targets in one job, the job fails if either fails, no continue-on-error, no skips, no failure allowlist, no output parsing; the baseline tree names the five upstream scripts instead of globbing and then asserts the conformance test did not leak in; upstream's SKILL.md is hash-verified against the pinned blob before use, with explicit failure branches on both the download and the mismatch; set -euo pipefail, permissions: contents: read, both actions pinned.

The round-1 note stands unchanged and is still not a finding: target 1 reaches raw.githubusercontent.com on every run, so an outage reds this PR for a reason unrelated to the diff. Pinned and hash-checked, so it is an availability question, not a supply-chain one.

Criterion 9 — .gitignore

Discharged. claude-plugins/fabrika/skills/skill-doctor/results/ is ignored, __pycache__/ with it, and no results artifact appears in the 21 changed files. Both rows carry a comment saying why, in the file's existing style.

Silent-failure, type-design, test-gap sweep

Nothing to route. No silent-failure path in the workflow: every branch that can go wrong writes to stderr and exits non-zero. The two config edits are data, and the one source edit is a literal appended to a as const list that a golden test constrains. The known test gap — target 2 is doc-level and proves nothing about runtime behaviour — is stated by the author in both the workflow header and the test's docstring, and closing it is not this child's job.

Base freshness

This head merged current main in rather than rebasing, so the base is 0cb9588f and the guards that landed while this PR sat open now ran against it. That is what turned round 1's blocking finding into a clean read, and it is why the leak-guard exposure appeared at all: it came with getting current, not with the feature.

Verdict-written: 2026-09-20T20:04:01Z

@usirin

usirin commented Sep 20, 2026

Copy link
Copy Markdown
Member

ship: AWAITING-CP-APPROVAL — PR #8063 @ 53e142c → human

Machine gates read satisfied at this head (review-code, review-skill, governance) and CI is green, but the PR touches a control-plane path (.github/workflows/skill-doctor-tests.yml, owned by @kamp-us/control-plane per .github/CODEOWNERS) and no approving review exists at this head. Merge intent disarmed; nothing enqueued.

Owed: one approving review from a @kamp-us/control-plane member (usirin, cansirin, rasitds, notusirin) at this exact head. Do not rebase or force-push once that approval lands — a moved head voids it.

@usirin
usirin added this pull request to the merge queue Sep 20, 2026
Merged via the queue into main with commit ec50a41 Sep 20, 2026
48 checks passed
@usirin
usirin deleted the build/8048-land-skill-doctor branch September 20, 2026 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

land skill-doctor: byte-exact upstream vendor, fabrika-shaped, tests in CI

2 participants