fix(generate): stop docs-sync from losing facts and skipping skills

[Ethan-Arrowood]

Found while reviewing #84. Three defects in the auto-sync pipeline, in descending order of consequence.

1. Nothing prevented a regeneration from deleting a documented fact

validate-generated.mjs asserted only that each must_cover string appears somewhere in the body. That requires a human to have predicted every fact in advance, and it still passes when the term survives but the fact around it is gone. #81 diagnosed this correctly and responded by growing must_cover 76 → 115 anchors — and the very next regeneration (#84) dropped seven facts that main documents, with every anchor green.

This adds checkFactRetention: fail when an inline code span present in both the previously committed body and the current docs source disappears from the regenerated body.

Requiring presence in the current source is what makes it safe to gate on — a fact deleted upstream is correctly dropped and never reported. Only facts the docs still assert, and that the rule used to carry, are enforced. It is derived from the diff rather than enumerated by hand, so it covers facts nobody thought to anchor.

Run against #84's head it catches:

Rule	Dropped fact
automatic-apis	`Sec-WebSocket-Protocol: mqtt`
programmatic-table-requests	`HdbError: <attribute> is not indexed and not combined with any other conditions`
v5-upgrade	`instanceof`, `import()`, `logger` — from the deleted moduleLoader behavior tables

No false positives across all 22 generate rules in both skills with the docs build attached.

Scope, stated honestly: this gates the identifier / header / status-code / error-string class, which is where this corpus keeps its facts. Prose-only losses (for example the by-ref "cluster builds from source, use a payload deploy instead" caveat) are not caught and remain review's job. Deliberate removals are recorded per rule under allow_dropped, so the decision lands in review rather than in a silent diff.

2. harper-mcp could never sync

The commit step hardcoded git add harper-best-practices. harper-mcp was added later (#69), so it was regenerated on every run and then discarded with the runner — both its rules have carried a stale sourceCommit since they were written, and an MCP-only docs change reported "No changes" and opened no PR at all.

Now staged from the SKILLS registry via a new skill-dirs.mjs, keeping lib/manifest.mjs the single place a skill is registered — as docs/plans-archive/docs-driven-skills.md intended ("handles every skill directory it finds — no per-skill plumbing needed").

Consequence worth flagging separately: harper-mcp/rules/custom-mcp-tools.md:50 still tells agents that data access inside a custom MCP tool "still hits per-record allow* predicates". The docs now say the transport "invokes the custom instance method directly" and "does not open a Resource transaction or run an allow* gate automatically". That is live incorrect security guidance on main and needs its own content PR — this change only makes it reachable by automation.

3. Every run re-sampled from main instead of continuing the branch

The job checked out main (no ref:) and force-pushed over auto/docs-sync. Because rules are skipped only when inputHash matches, and main's baselines are stale, nothing was ever skipped: every run re-generated every drifted rule from scratch.

That is the mechanism behind the fact flicker on #84 — between two revisions with byte-identical inputHash values, 409 and the gh CLI scope warning vanished and came back while the MQTT header stayed gone. It also discarded a completed review.

Now: generate on top of the open sync branch when one exists, merge main in, and fail loudly on conflict rather than resolving it by discarding the branch. Push with --force-with-lease. The branch stays rolling — one open sync PR, updated in place.

Verification

• npm run validate green.
• validate-generated.mjs --docs-path <docs> green on a clean tree; fires as tabled above when docs: regenerate rules from documentation@0d151a2 #84's bodies are staged against main's.
• Workflow YAML parses; every run block passes bash -n; the new step is ordered before provenance capture and generation.

Note on merge order

#84 should not merge before this lands — it is a net content regression on seven facts. With retention enforced, re-running generation is cheaper than reconciling #84 by hand.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a new script skill-dirs.mjs to output skill directories and implements a fact-retention validation check (checkFactRetention) in validate-generated.mjs to ensure documented facts are not accidentally dropped during regeneration. The review feedback suggests improving the inlineCodeSpans helper to also strip tilde-based fenced code blocks, preventing potential false positives when extracting inline code spans.

[gemini-code-assist]

The inlineCodeSpans function currently only strips fenced code blocks that use backticks (```). However, the codebase (specifically sources.mjs) also supports tilde-based fences (~~~). If a rule body uses tildes for fenced blocks, any backticks inside them will be incorrectly parsed as inline code spans. Updating the regex to support both fence types ensures consistency and prevents false positives.

Suggested change

	// dropping `Sec-WebSocket-Protocol: mqtt` is not.
	const withoutFences = md.replace(/(\x60{3}|~{3})[\s\S]*?\1/g, '');

[Ethan-Arrowood]

Fixed in 1c650a1, though not with the suggested regex. You are right that sources.mjs supports tilde fences — sliceSection uses /^\s*(`{3,}|~{3,})/ — so the narrower regex was inconsistent with its own module.

The suggested /(\x60{3}|~{3})[\s\S]*?\1/g fixes the tilde case but still mishandles delimiter runs longer than three: the backreference matches exactly the 3 captured characters, so a ```` fence closes on the first 3 of 4 and leaves a stray backtick behind — which can then pair with a later one and manufacture a phantom span. It also matches fences that are not at line start.

Replaced with a line-based stripFencedBlocks in lib/sources.mjs, shared by inlineCodeSpans and stripCode, honouring delimiter character, run length, indent, and info strings per CommonMark. Covered by 13 tests under node --test.

_{sent with Claude Opus 5}

[kriszyp]

Looks like worthwhile fixes, maybe a couple of updates to the scripts to apply.
_{🤖 Reviewed with Codex}

[kriszyp]

This merge can require a merge commit whenever main advances without conflicts, but the bot's user.name and user.email are configured only in the later commit step. A fresh hosted checkout has no author identity, so Git exits with “Committer identity unknown”; the broad error handler then misreports this as a merge conflict and wedges sync. Please configure the App identity before this merge, while retaining the later generated-content commit behavior.

[Ethan-Arrowood]

Confirmed and fixed in 1c650a1 — this was a real wedge, and worse than "can require a merge commit": main advances routinely via semantic-release chore(release) commits, so the non-fast-forward path is the normal one, not an edge case.

Reproduced it locally with an empty HOME to mimic a fresh hosted checkout:

--- merge with NO identity ---
Committer identity unknown
fatal: unable to auto-detect email address (got 'ethan@Mac.(none)')
exit=128
--- unmerged paths: []

So my handler was not just misleading, it was asserting the opposite of the truth — exit 128 with zero unmerged paths, reported as a conflict.

Two changes:

• The App identity is now configured at the top of the branch-checkout step, before the merge. The later commit step still sets it, as you asked — git config is idempotent, and I would rather that step keep working standalone.
• The handler no longer guesses. A real conflict is detected via git ls-files --unmerged and names the files with --diff-filter=U; anything else reports itself as a non-conflict merge failure. Verified both branches: identity set → Merge made by the 'ort' strategy, exit 0; genuine conflict → exit 1 with h.txt named.

_{sent with Claude Opus 5}

[kriszyp]

Remote branch existence is not equivalent to an open rolling PR. Closing a rejected sync PR normally leaves its head branch, so the documented “close … then re-run” recovery checks out the same stale branch again; it can repeat the conflict or carry abandoned generated changes into a new PR. Please use open-PR state to decide whether to preserve the branch, and reset/start from origin/main when no open sync PR owns it (with the lease still protecting concurrent updates).

[Ethan-Arrowood]

Good catch — fixed in 1c650a1. You are right that this defeated the very recovery my own error message documented: gh pr close leaves the head branch, so git ls-remote --heads still succeeded, the run checked out the same stale branch, and it either replayed the conflict or carried abandoned generated content into the next PR.

Now gated on open-PR state rather than branch existence:

PR_STATE=$(gh pr view "$BRANCH" --json state --jq .state 2>/dev/null || echo "")
if [ "$PR_STATE" = "OPEN" ]; then
  git checkout -B "$BRANCH" "origin/$BRANCH"   # preserve review fixes
  ...
else
  git checkout -B "$BRANCH" origin/main        # start clean
fi

--force-with-lease on the push keeps protecting concurrent updates in the reset case: the lease is taken against the origin/auto/docs-sync remote-tracking ref fetched at checkout, so overwriting an abandoned branch succeeds while anything pushed after this job started still fails.

I also corrected the recovery hint, since closing the PR is now sufficient on its own — it no longer tells people to delete the branch.

_{sent with Claude Opus 5}

[kriszyp]

The comment promises fenced blocks are excluded, but this removes only triple-backtick fences. Valid ~~~ fences remain, so backtick-delimited expressions inside such a code example become retained “facts”; removing the example later can falsely block generation whenever the token still occurs in the docs source. Please use a fence-aware scanner or Markdown parser that handles tilde fences and delimiter runs, and cover those cases with focused tests.

[Ethan-Arrowood]

Agreed, and the inconsistency was worse than the comment over-promising: sliceSection in the same module has handled `{3,}`/`~{3,}` all along, so the narrower regex disagreed with the fence logic sitting right next to it.

Fixed in 1c650a1 with a fence-aware scanner rather than a bigger regex — stripFencedBlocks, added to lib/sources.mjs where fence knowledge already lives, and now shared by both inlineCodeSpans and stripCode. It follows CommonMark on the parts that bite: fences close only on the same character with a run at least as long as the opener, at most 3 spaces of indent (4 is an indented code block, not a fence), nothing but whitespace after a closer (an info string is opener-only), and an unterminated fence runs to EOF.

I skipped a Markdown parser deliberately — it would be a new runtime dependency for one helper, and the failure mode here is a false positive that would get the whole check switched off, so I wanted the rule set small enough to read and test exhaustively.

13 focused tests under node --test (no new dependency), wired into npm run validate via a test script, covering exactly the cases the old regex got wrong:

• backtick run inside a ~~~ fence is content, not a boundary
• tilde run inside a ``` fence, likewise
• opens/closes; ``` ``` ``` does not close ; ````` does
• unterminated fence; 3-space indent vs 4-space; trailing whitespace vs info string on a closer

On behaviour: this is provably a no-op for the current corpus — old and new stripping produce identical fact sets across all 33 rule bodies, and there are zero tilde or 4+ backtick fences in the rule bodies or the docs build today. Re-verified end to end against #84 after the refactor, same result as before: Sec-WebSocket-Protocol: mqtt, the HdbError: <attribute> is not indexed... string, and instanceof/import()/logger from the deleted moduleLoader tables, with no false positives on a clean tree.

_{sent with Claude Opus 5}