Ten skills into six: ship the report renderer as code, cut the skills to judgment (LLP 0196/0197) - #668
Open
bgmcmullen wants to merge 21 commits into
Open
Ten skills into six: ship the report renderer as code, cut the skills to judgment (LLP 0196/0197)#668bgmcmullen wants to merge 21 commits into
bgmcmullen wants to merge 21 commits into
Conversation
…tree (LLP 0193/0194 T1) build.sh lived only in ~/hypaware-reports, untracked. Because it was not versioned with the skill that drove it, hypaware-report-to-html/SKILL.md could not call it and trust it, so ~110 of its 399 lines were a prose copy of the script plus repair instructions for copies predating a feature. Prose was the fallback for code we failed to ship. src/core/reports/ is now the canonical source: build.sh plus the five assets every built page carries. The assets came from the bundled skill copies (claude and codex were byte-identical); build.sh came from the live tree, the only place it existed. Behaviour-preserving, verified by building two copies of the reports tree with the original and vendored scripts and diffing the output: identical across all five reports. The only script edits are four em dashes (CLAUDE.md forbids them in-repo) and a header marking the file canonical and warning against features T3 would have to port twice. test/core/report-assets-canonical.test.js holds all three copies of each asset byte-identical, verified to fail on injected drift. This is the check that makes "canonical" mean something: the live tree's style.css had already drifted, and the skill's "is this an older sheet?" heuristic cannot tell a customization from rot. tsconfig excludes src/core/reports/assets: copy-md.js is a browser script and the Node-targeted typecheck has no DOM lib. Follows the existing exclude for plugins-workspace/*/skills, which carries the same file for the same reason. LLP 0193 (rfc) argues the general case: skills encode procedures where they should encode constraints, which makes them long, brittle, and rigid. LLP 0194 (plan) sequences the work so the skill merge, which is the actual complaint, comes last: merging four 25 KB skills before removing their mechanical content just yields one 60 KB skill. Note: npm test is red on master already, at test/core/usage-policy-fold.test.js :387, unrelated to this change and confirmed against a clean tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t (LLP 0194 T2 part 1)
T2 was scoped on a false premise. It called the two trees' divergence "drift"
and asked someone to "decide the correct side" of each. Reading all six diffs
shows there is no wrong side: essentially every diverging line is legitimate
host-specific content.
- disable-model-invocation exists only on Claude Code (LLP 0142), and the
terse-vs-trigger-rich descriptions follow from it: a gated skill is picked
by a human from a menu, an ungated one has to be routed to by a model.
- mcp__hypaware__* naming, AskUserQuestion, /hypaware-ignore cross-refs, and
`claude --fork-session` vs `codex fork` are all host facts.
- 88 codex-only lines in hypaware-privacy exist because Codex has no
CLAUDE_CODE_SESSION_ID, so the skill resolves the session CONTAINER out of
~/.codex/sessions rollouts with refusal-on-ambiguity semantics. It cites
issue #453 and is guarded by its own 132-line test.
So the codex tree is not derivable from the claude tree; it holds knowledge the
claude tree does not. A generator emitting one from the other would delete
tested privacy logic. LLP 0194 T2 is rewritten to say so, and split into three
parts so the part that is correct under every design lands first and alone.
That part is this commit: a guard that records the host-specific surface rather
than removing it. Per skill, the count and hash of lines unique to each side,
checked against a committed fixture. Adding host-specific content now fails the
test and has to be re-recorded, which shows up in review as a count change;
mirroring an edit correctly to both hosts changes nothing. Verified in both
directions: an unmirrored edit fails with an actionable message, and an
identical edit to both copies leaves the record untouched.
Line-set rather than positional comparison, so reflows and reordering do not
produce noise, which is not the failure mode worth catching.
Also records in both hypaware-privacy copies that their fork is deliberate, so
the next reader does not "fix" it.
Parts 2 and 3 (dedup the two byte-identical skills; leave privacy forked) need
a design decision about where any shared source lives and whether generated
output is committed or built at pack time. Not started.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…P 0193 #gate-moves-to-the-command) Maintainer decision: reports should be asked for in the user's own words, not by invoking a skill by name, and a skill the model cannot reach cannot serve that. disable-model-invocation is removed from all three report skills, which supersedes LLP 0142 #user-invoked-only for that paragraph only (forward-ref appended there; the rest of 0142, including why hypaware-ignore was never in the set, stands). One precondition found while implementing it. LLP 0193 argued the gate could move because each skill "already confirms". Checked, and that was true for publishing (an explicit yes, never as a side effect of generating) and for applying (per-change approval). It was NOT true for rendering: hypaware-report-to-html confirms before PUSHING, but its step 3 rewrites the report .md files in place with no confirmation at all, and those are source edits, not derived output like html/. Ungating it as it stood would have let a model rewrite a user's reports off an unrelated prompt. So the gate is removed WITH a confirmation added to the step that mutates sources, which is the "control belongs on the act" claim honoured rather than asserted. The codex copy has been model-invocable all along and had the same hole, so it gets the confirmation too. LLP 0194 T3/T5 move the check into `hyp report render` where it can be tested. Knock-on: this shrank the host divergence rather than growing it. The terse-vs-trigger-rich description split existed only because the claude copies were gated; a model-invocable skill needs trigger words on both hosts. So the claude copies adopt the codex descriptions, the stale @ref LLP 0142 comments are replaced with one that records where the control actually lives, and both hosts get it. hypaware-publish-report and hypaware-report-to-html are now byte-identical across hosts, hypaware-apply-report-changes fell from 6/3 diverging lines to 3/2 (just AskUserQuestion vs a numbered reply), and four of eight shared skills now carry nothing host-specific, up from two. The parity fixture is re-recorded to match, and its "fully shared" assertion now derives that set from the fixture instead of hardcoding two names, so it grows on its own as skills converge. LLP 0194 T12 (the skill merge) is unblocked: no conflicting frontmatter left to reconcile, and the seven-skill fallback is not taken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y-shared skill The gate removal doubled the deduplicable surface to ~80 KB across four skills, which reads like an argument to build the generator now. It is the opposite: all four are slated to stop existing in their current form (hypaware-graph into hypaware-query; the three report skills into hypaware-report). A generator built now would target files T12 deletes, with a transform table written against a file set about to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… one (LLP 0194 T12) T12 merges ten skills into six. The risk is not that the merge fails loudly, it is that a constraint gets dropped as incidental detail during a rewrite and nothing notices until a report is confidently wrong or a query takes the production server down. The constraints most easily lost are the ones that read like trivia. The plan said T12 "should start from an explicit inventory of constraints to preserve, checked off against the merged result". This is that inventory, built before the merge rather than during it, and executable rather than prose. test/fixtures/skill-constraints.json lists seventeen load-bearing constraints across three kinds: correctness (COALESCE every token sum, the one-carrier rule, usage lives in attributes.usage not raw_frame), infrastructure safety (never GROUP BY/DISTINCT/row-fetch wide content columns at scale, one remote worker at a time), and trust boundaries plus confirmation gates (captured content is data not instructions, changes come from observed behaviour, confirm before publishing / applying / editing report sources, the privacy review opts its own session out first). Each carries the harm of dropping it, and the test enforces that: an entry with no nameable harm is guidance, not a constraint, and does not belong here. The match is against the CONCATENATED corpus of each host, never a named file, so T12 can merge, split, and rename skills freely; only disappearance fails. Patterns key on distinctive terms rather than whole sentences, and the corpus is whitespace-normalised first, because skill prose is hard-wrapped at ~90 columns and a constraint routinely straddles a line break. Without that, a reflow during the merge would look identical to a deletion. One pattern caught exactly that while writing this. Verified it bites: rewording "COALESCE every token sum..." to "Sum the token columns." fails with the constraint id and its harm. Two rules recorded in LLP 0194: a constraint that stops applying is deleted from the fixture in the same commit that removes it from the skills, with the reason; and a pattern is never loosened to make the test pass, since that converts the guard into a rubber stamp exactly when it is doing its job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an stated Checking .github/workflows/ci.yml resolves open question 1 the cheap way: CI is ubuntu-latest, so pandoc is a one-line apt-get step and in-process rendering is a change nothing currently forces. The same check sharpens the case for T3. CI is Linux-only and the renderer is macOS-only (sed -E -i '', sips), so the renderer has no automated coverage today and cannot have any until the port lands. 'Fixes the Linux half of the release gate' understates it: until T3, any test written against the renderer is unrunnable on the only platform CI has. Also records that the rewrite_hrefs case table is pulled forward into T3. It is the specification the port is written against, not a check added afterwards, and there is no existing coverage to port against instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l (LLP 0194 T3)
src/core/reports/render.js replaces build.sh. The port exists because the shell
script is macOS-only (`sed -E -i ''` is the BSD spelling, `sips` is macOS-only)
and CI is ubuntu-latest, so the renderer had no automated coverage and could not
have any. It now has 23 tests.
The case table came first, deliberately. rewrite_hrefs is four regexes over
emitted HTML covering six link shapes across two page kinds, and its failure
mode is silent: a missed case ships a dead link that renders fine and breaks
only when clicked. test/core/report-render-hrefs.test.js pins all 18 cases,
including the two exclusions that matter as much as the rewrites (absolute URLs
are left alone via the `:` exclusion; data-src is not href, so the copy buttons
keep pointing at the raw .md sidecars). The port was written against that table.
Accepted by A/B on the real reports tree: build.sh and renderReports run against
identical copies of ~/hypaware-reports produce byte-identical html/ across five
reports with their section pages.
Two divergences the plan had not named, found by the port:
- build.sh copies four assets beside each page, not five. head.html is inlined
into <head> by pandoc's -H and was never a page asset; shipping it would be
dead weight in every output dir. Split into TREE_ASSETS and PAGE_ASSETS.
- build.sh interpolated $slug into a regex unescaped. render.js escapes it.
sips is gone: favicon.png ships prebuilt. pandoc stays (LLP 0193 open question
1, resolved), and CI installs it, so the render tests run there rather than
skipping. Without that step they would skip silently and leave the renderer
uncovered on the only platform CI has.
test/core/report-render.test.js promotes the skill's step-6 grep checklist to
executable contract: no leftover .md hrefs, a copy action on every page, a
full.md per report, back-links, no stale output after a report is deleted, and
a user theme.css surviving a rebuild (ahead of T7).
build.sh is frozen in place, not deleted: the skill still calls it by name until
T5 wires `hyp report render`. Its header and the directory README now say so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cript (LLP 0194 T5) runReportRender joins the report group, registered beside publish/list/get/ delete. The skill's prose renderer is gone: 28,405 -> 22,739 bytes, 414 -> 336 lines, zero build.sh references left. That was the point of T1/T3/T5 as a sequence, and it is what makes the T12 merge cheap rather than cosmetic. The group asymmetry, left open in the plan, is resolved by joining and fixing the help. LLP 0155 built `report` around "there is no local reports plane; --remote selects a server", and render is the first member that is not a REST call: local, credential-free, no --remote. It joins anyway because the workflow is render-then-publish, and splitting that across two command namespaces would serve the implementation rather than the reader. The group help now separates the local build step from the four plane operations, so 0155's claim stays true of the commands it was written about. Skill steps 2, 4, and 6 collapse to "the command does this". Step 6 keeps only what a command cannot check: are the findings carded, does the landing page carry stats. The structural contract (pages built, no leftover .md links, copy action and back-link present, full.md per report) is the renderer's own tests now, so it runs every time rather than when the model remembers the greps. render refuses before wiping html/ when a tree holds no reports, verified to create zero files in that case. An empty tree usually means the reports were just archived, and rebuilding would replace a good site with an empty one. Missing directory exits 2, missing pandoc exits 1 and names both install commands. build.sh is deleted rather than left frozen: the skill no longer calls it, it was macOS-only and untested, and leaving it invites someone to edit the copy CI cannot run. A test asserts it stays gone, and git history keeps it (the README names the commit for an A/B comparison). Both host copies edited identically: the divergence fixture is unchanged and the two files remain byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
index.html was the least reproducible file in the reports tree and the easiest
to lose. build.sh never generated it, so the skill transcribed a 50-line
template out of components.md on every run: two runs over unchanged reports
produced different HTML, and a deleted landing page was simply gone, since
nothing rebuilt derived output that behaved like a source file. That is what
happened to this tree today.
src/core/reports/landing.js derives it from the reports themselves. One card per
report newest-first, each carrying that report's headline numbers hoisted from
its metric-grid with values and crit/warn/good judgments kept exactly, plus a
companion card for any report with a proposed-changes.md page. Verified against
the real tree: 10 cards (matching the hand-written page), all 5 reports listed,
every link resolves, and byte-identical across consecutive runs.
It must not invent, and that changed the output. The hand-written page carried
stats on the proposed-changes companion cards, but those pages have no
metric-grid and no rec cards at all, so those figures came from a model reading
prose. A card whose page has no metrics now gets no stat row. Labels are the
report's own metric labels verbatim, because compressing a label to "2-4 plain
words" is editorial judgment and a renderer doing it would be writing copy
rather than deriving it. Same constraint the report skills carry, applied to the
command.
Two bugs found by running it, not by reading it:
- a lazy regex over `metric` blocks stopped at the nested <div class="value">
and produced one stat per card instead of four
- extractTitle ran the heading through the tag-stripper, truncating a literal
`Tokens & <Costs>` to `Tokens &`. Headings are Markdown, so they are
whitespace-collapsed and escaped at render, never stripped. Labels and
kickers still strip, because those come from raw-HTML blocks where inline
tags are decoration a card should not inherit.
Values keep their authored markup (`20<small>days</small>`) rather than being
flattened, because the unit's spacing is CSS. style.css gains one rule mirroring
`.metric .value small` so a hoisted stat spaces its unit the same way; the two
skill asset copies are re-synced and the drift guard passes.
theme.css is linked only when the tree has one, so no dangling stylesheet
request on a tree without a theme.
Skill step 5 is deleted (24 more lines; the file is now 21,080 bytes, down from
28,405 before T5). components.md's landing template is marked superseded and
kept only as a reference for what the renderer emits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…where (LLP 0194 T7) T4 left this half-done. theme.css was copied beside every built page but linked only from the landing page, so a user theme would have styled the index and nothing else. head.html now carries the link, which puts it on every page pandoc builds, after the base sheet so the cascade resolves it. The command creates assets/theme.css once, with a commented stub naming the custom properties worth overriding (--accent, --fg, the judgment colours, the chart ramp, the type stacks, --max), and never writes it again. It exists even when empty so the stylesheet link never dangles, and so the extension point is discoverable: a user finds the file rather than having to know overrides are supported. That split is the point of the task. The skill used to guess whether a modified style.css was a customization or a stale sheet, by looking for a webfont import or a missing rule, which misclassified the most obvious customization (wanting a webfont) as rot. Now the base is unambiguously the command's and always refreshed, and the theme is unambiguously the user's and never touched. Tests assert all of it: the stub is created, a user edit survives a rebuild, every page links the theme, the theme loads after the base sheet, the theme ships beside each page that links it, and a hand-edited base sheet IS replaced. Skill step 2 now tells users to restyle via theme.css and warns that edits to style.css are lost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the renderer, landing page, and structural verification all in code, what
was left was the enrichment contract plus a layer of description around it. This
removes the description.
Kept, because it is what a command cannot do: the confirmation before editing
report sources, the Phase A inventory (read the whole report, write down the
headline numbers with judgments, each finding's strongest stats, one visual per
section, every item quoting a number that literally appears), the Phase B design
rules, the design bar, the contamination warning about the example file, and the
hard rules (numbers trace to the report's own text, judgment attaches to
patterns and never to a person, ready-to-apply artifacts render verbatim).
Cut: the version-dated layout trivia ("usage review 2026-07-16+", "2026-07-15
predates the split", "legacy standalone improvement review"). That is per-report
shape, it belongs in authoring.md, and it was being loaded on every render by
every reader who did not need it. Also cut the duplicated visual-system prose
that components.md already carries in full, and notes that described renderer
behaviour rather than author judgment (pandoc dialect, flat-vs-sectioned).
The plan estimated "under 8 KB". That was optimistic: the judgment content is
genuinely ~13 KB and cutting further would start deleting design rules rather
than mechanics. Recorded in LLP 0194 rather than trimmed to hit a number.
skill-constraints-survive.test.js passed unchanged across the rewrite, which is
the check it was built for: it does not care which file a constraint lives in,
only that it still exists somewhere in the corpus.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The complaint that started this: too many skills, each too long. The surface is
now hypaware-ignore, hypaware-privacy, hypaware-query, hypaware-reference,
hypaware-report, hypaware-unignore.
The four report skills become one hypaware-report: a short router SKILL.md
naming the four stages (Review, Render, Publish, Apply) and carrying the rules
that hold across all of them, with the detail in reviewing.md, rendering.md,
publishing.md and applying.md, loaded on entry. This only works because T3/T4/T5
and T10 came first: rendering alone went 28.4 KB -> 13.0 KB before it was merged,
so this combines short skills rather than long ones.
hypaware-graph was NOT merged into hypaware-query, and the plan was wrong to say
it should be. @hypaware/context-graph already ships that skill; the claude and
codex copies were byte-identical duplicates carried unconditionally. Folding it
into hypaware-query would have created a fourth copy AND shipped graph guidance
to installs with no graph plugin. Deleting the two duplicates and letting the
plugin that provides the feature own the skill that documents it is the right
shape: the skill appears exactly when the feature does. Six skills without the
graph plugin, seven with it.
Both guards fired, and both were reading too narrow a corpus:
- skill-constraints-survive reported eight constraints dropped. Every one had
moved from a SKILL.md into a stage file. A false alarm of the worst kind: it
trains people to loosen patterns during exactly the refactor the guard
exists for.
- skill-host-parity reported hypaware-report as perfectly in sync while
applying.md diverged underneath it.
A skill is its entry file plus the reference files it loads, so both now read
every .md in a skill directory. With that fixed, all 36 constraint assertions
pass unchanged across the merge and the divergence is correctly reported as 3/2
(applying.md's AskUserQuestion line).
Cross-references throughout the corpus are repointed from dead skill names to
stages, and the three tests that addressed skills by directory name now address
the shipped Markdown by path.
Left open, and it matters: installed copies of the four removed skills will
linger on machines that already have them, so a user could see both the old and
new surfaces at once. That is T13, and it should land before this ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Maintainer decision: actively removing stale skills from machines is out of scope for the merge PR. The plan said T13 should land before T12; it does not, so this records the gap rather than leaving the plan claiming a precondition that was not met. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eclared (LLP 0194 T12) The merge was broken and nothing caught it. A client plugin declares its skills three times: contributes.skills in hypaware.plugin.json, the list activate() passes to ctx.skills.register, and the directories under skills/. T12 replaced five skill directories with one and updated only the third. Both plugins would have tried to install five skills that no longer exist and would never have installed hypaware-report, so the merge would have shipped as pure breakage. Fixed in both plugins: the manifest and the registration list now name hypaware-report instead of the five removed skills, and both manifest descriptions stop advertising skills they no longer ship. test/plugins/client-skill-manifest-agreement.test.js prevents recurrence, with four assertions per client: manifest and registration agree, every registered skill exists on disk with a SKILL.md (and its sourceDir resolves), every skill on disk is registered (an unregistered one is dead weight in the package), and the manifest description names no skill the plugin no longer ships. It captures registrations by running activate() against a stub ctx rather than parsing the source, because the register call is what installs a skill. ctx is stubbed explicitly so a new ctx dependency surfaces as a clear failure; the gateway capability alone gets a permissive proxy, since its surface is broad and entirely beside the point here, and enumerating it would make this test fail whenever the gateway grows a method. Verified it bites: restoring one stale manifest entry fails two of the four. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…marked Active README said Claude Code and Codex 'additionally get a hypaware-graph skill'. They did, as duplicates; the skill ships with the context-graph plugin and the duplicates are gone, so the note now says it ships wherever the graph is enabled. LLP 0155 gains an Extended-by forward-ref for `hyp report render`: the group's 'there is no local mode' paragraph stays true of the four plane operations it was written about, and render is explicitly not one. Recorded on the decision rather than left as an inconsistency between the doc and the group help. LLP 0194 Draft -> Active: most of its task graph has shipped, so Draft misdescribes it. LLP 0193 stays Draft deliberately, since accepting an RFC is a maintainer act and not something to do on the author's behalf. Every @ref added on this branch resolves (10 anchors, checked against both the <a id> and {#anchor} forms the corpus uses). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
T13 (#660), T8 (#662), T11 (#663), and T2 part 2 (#664) each have an issue, so the plan is not the only record of what is outstanding. Also notes #661, the usage-policy fold test that has never passed since it was written, which is the one failure npm test reports throughout this work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows the corpus pattern set by LLP 0128, the other install-scale RFC: the rfc is Accepted (the proposal was agreed) while execution state lives in the design and plan documents (0135 Active, 0136 Implemented). Here that is 0194, which is Active and becomes Implemented when T8/T11/T13/T2-part-2 land. The contested part, removing disable-model-invocation, was decided by the maintainer on 2026-08-06 and is recorded in #gate-moves-to-the-command. The remaining tasks are scheduled work tracked in 0194 and in issues #660, #662, #663 and #664, not unresolved proposals. Accepted also makes it immutable, which is right for a document whose anchors are now @ref'd from shipped code: changing what it settled needs a new LLP, not an edit that would silently drift those references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundled report skill shipped style.css, copy-md.js, head.html and both favicons in each host tree, 64 KB total. Nothing reads them. They existed because the skill used to copy the stylesheet into the reports tree itself (`cp "$SKILL_DIR/assets/style.css" assets/style.css`). T7 replaced that with the command owning the base sheet, and T10 deleted the copy step from the prose. `hyp report render` installs assets from src/core/reports/assets/, which is its own directory, so the skill never touches them. Verified by wiping a reports tree's assets/ entirely and rendering: all five assets plus theme.css were rebuilt. Every remaining `assets/...` mention in the skill refers to a path inside the reports tree or inside a published report on the server, never to the skill's own copy. report-assets-canonical.test.js loses its reason to exist in its old form: it held three copies byte-identical, and there is now one. It keeps the checks that still mean something (the canonical set is complete, render.js exists, build.sh stays gone) and gains one that replaces the sync assertions: no bundled skill ships a copy of a renderer asset. Verified it bites by restoring style.css to the skill. This is the drift problem solved by construction rather than by policing. Three copies of the same bytes is the shape that drifts, and this exact set did: the canonical stylesheet and the one in a live reports tree diverged, and the skill's "is this an older sheet?" heuristic could not tell a customization from rot. One copy makes the question unanswerable. Package drops from 896 to 892 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bgmcmullen
added
the
neutral:adopt
philcunliffe
added
the
neutral:adopted
…his branch was open master merged llp/0193-openclaw-backfill-cli-denylist.decision.md and llp/0194-per-message-provider.decision.md during this work, so both numbers were claimed twice. LLP numbers are globally unique and never reused, so this branch moves rather than the merged docs. 0195 is taken by PR #667, hence 0196/0197. Renaming an LLP is a mechanical edit (LLP 0156), and it has to be exhaustive or the @refs in shipped code point at someone else's decision. Every rewrite here is anchored to my slug or to an anchor only my documents define, so the openclaw refs (LLP 0193#decision, LLP 0194#decision) are untouched, as are LLP 0161/0172/ 0173 and docs/ACCEPTANCE.md. Updated: both filenames and titles, the forward-refs on LLP 0142 and LLP 0155, all 9 @ref annotations across src/ and test/, prose references in the skills, src/core/reports/README.md, and the CI workflow comment. Verified every renumbered anchor resolves, and test/core/llp-ref-hygiene.test.js (which arrived from master in the same merge) passes. Also merges origin/master, which brings the openclaw backfill work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bgmcmullen
changed the title
Contributor
Neutral review round 1 of 2 (adopted PR,
|
… and add renderer unit tests Four space-form `LLP 0193`/`0194` references (not caught by the @ref scan, which only matches `@ref LLP NNNN#anchor`) still pointed at master's merged 0193/0194 docs instead of this PR's own now-renumbered 0196/0197. A sweep grep confirms no other stale references remain; all other 019[34] hits are legitimate references to master's openclaw-denylist (0193) and per-message-provider (0194) decisions. Also: - rendering.md (claude + codex copies) claimed the renderer enforces the structural contract at runtime and fails if it breaks. render.js's only throw is the pandoc-missing check; the contract is covered by tests over a synthetic fixture, not the user's real output. Corrected the claim and restored the deleted step-6 greps against the user's actual built html/. - codex/src/index.js's activate() JSDoc still listed hypaware-graph, the AI usage report skill, and the report-to-html renderer as contributed skills; none are registered any more. Corrected to the four skills actually registered (hypaware-query, hypaware-reference, hypaware-privacy, hypaware-report). - components.md and example-enrichment.md (both trees) still pointed at `build.sh`, which this PR deletes and report-assets-canonical.test.js asserts must never return. Repointed to `hyp report render`. - reports/README.md cited `git show 0f3dce1^:...build.sh`; that sha does not survive a squash merge. Replaced with a stable description plus a PR reference. - Added unit tests for docLabel, pageTitle, masthead, and discoverSections (previously zero test references), and extended the existing HTML-escaping test to also cover a stat label rather than only the card title. Verified each closes the mutant it was written against (date-slice off-by-one, dropped .trim(), dropped escapeHtml(stat.label)). Left components.md's landing-page per-entry rules alone (pre-existing, deliberate narrowing that needs contributor judgment) and claude/src/index.js untouched (pre-existing inaccuracy, out of scope). Co-Authored-By: Claude <noreply@anthropic.com>
philcunliffe
pushed a commit
that referenced
this pull request
Aug 6, 2026
- Renumber llp/0195-setup-ends-on-a-question -> 0198: 0195 collides with PR #658's fix/issue-653 doc, 0196/0197 are taken by PR #668. Updates all 28 @ref sites plus the LLP 0135 Extended-by block. - ask.js askableClients: a successful status probe reporting zero attached clients is evidence of detachment, not grounds to fall back to every launchable client on PATH. Only a thrown probe now falls through. - ask.js --list: launchable claim now reflects actual resolvability instead of being hardcoded true. - ask.js docblock/comment: four suggested prompts, not five. - Add test/core/commands/ask.test.js covering askableClients' probe success/throw branches and the no-launcher/--list exit-code contract. - Add two firstLookHadRows cases (slow, absent/error) to wizard/index.test.js pinning the two mutants the review found. - Add a no-preauth assertion to client-launch-manifest.test.js: bundled launch args never carry --allowedTools or --dangerously-skip-permissions. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Neutral review round 2 of 2 (adopted PR) -
|
| Location | Now cites | Anchor |
|---|---|---|
src/core/reports/types.d.ts:6 |
LLP 0196 #theme-layer |
<a id="theme-layer"> at 0196:246 |
test/fixtures/skill-constraints.json:3 |
LLP 0197 T12 |
real task #t12-constraint-inventory, 0197:328 |
test/fixtures/skill-constraints.json:62 |
LLP 0196 #gate-moves-to-the-command |
0196:289 |
llp/0197-…plan.md:309 |
"Accept 0196 … Superseded-by: LLP 0196" |
self-consistent |
The T14 line got a second look because it reads self-referential. It is correct: T14's job
is to accept the RFC and point LLP 0142 at that same RFC. It matches 0196:382 and the
forward-ref actually landed at llp/0142:71.
Nine unresolved @refs exist in the changed files; all nine confirmed pre-existing on
master (0011#interactive-walkthrough, 0016, 0027, 0037, 0099, 0100#skill, 0138#one-command,
0157#adapter-rework, 0157#requirements). None introduced here.
Over-correction sweep - clean
This was the real risk of neutral's fix, since master's 0193/0194 are heavily cited. Every
surviving mention is OpenClaw and correctly points at the merged docs: backfill.js (x8),
message_projector.js:708, openclaw-backfill.test.js (x7),
ai-gateway-backfill-materializer.test.js:113, docs/ACCEPTANCE.md (x3),
hypaware-plugin-kernel-types.d.ts:1925, and the Extended-by: lines on LLP 0161/0172/0173.
Neutral touched none of them. The sweep in the other direction is clean too.
rendering.md - correct, with one incompleteness (neutral's, low)
The new wording is true of the code: render.js's only throw is the pandoc check at
:188, and the four contract items map exactly onto report-render.test.js's four
integration tests over fixtureTree() - a synthetic fixture, as the wording now says.
The restored greps were executed against a synthetic tree matching buildReport's real
output shape: each produced no output on a healthy tree, ls html/*/full.md listed one per
report, and the first grep correctly caught an injected href="trends.md". Both tree copies
are byte-identical (md5 7550d682…); the only file differing across the skill is
applying.md, which is the deliberate AskUserQuestion-vs-numbered-reply fork recorded in
skill-host-divergence.json and pinned by skill-host-parity.test.js.
Incompleteness worth your one-line fix: the prose enumerates four contract items but the
restored block checks three. "every page built" has no command - master's original had
ls html/ # one dir per report and neutral did not restore it. Master's two-line comment
explaining why --include='*.html' matters (the raw index.md/full.md sidecars keep their
.md links on purpose) was also dropped, which invites a future "simplification" that would
false-positive. Adding both back closes it.
New tests - all three claimed mutants killed by hand
docLabel slice(0,10)→slice(0,9), pageTitle dropped .trim(), and renderCard
dropped escapeHtml(stat.label) each fail only their intended test. Three extra mutants also
killed (discoverSections .endsWith('.md') filter, masthead brand-mark span, masthead
topnav class). One survivor - discoverSections dropping .sort() - survives only because
readdirSync happened to return sorted order on this filesystem; the fixture does create
files out of order, so that is environment luck, not a test-design flaw.
Other doc fixes - correct
codex/src/index.js:49-50 now names exactly the four skills registered at :230-234, and
client-skill-manifest-agreement.test.js was confirmed to kill injected breakage (a bogus
skill in the loop fails 2 tests). No build.sh reference survives outside LLP historical
records and the guard itself. The README's git-sha citation is now squash-durable.
Left for you (all low)
claude/src/index.js:60says "the three Claude-targeted helper skills"; six are registered
at:264-271. Pre-existing on master, so not introduced here, but it is the identical
drift neutral just fixed in the codex sibling.{claude,codex}/skills/hypaware-report/publishing.md:57still says "A report-to-html
folder"; line 52 was renamed tohypaware-reportand 57 was missed by the same pass.components.md:198-266- round 1's item, still open. TheSupersedednote now frames the
template as "a reference for what the renderer emits", which makes the inaccuracies actively
misleading:style="padding-left:1.4rem"(renderer emits none), notheme.csslink
(renderer emits one,landing.js:187-189), and the companion card's ranked-change kicker
and "reccard figures" (extractStatsreads onlymetric-grid,landing.js:91).landing.js:164interpolatescard.hrefunescaped whiletitle,kicker, and
stat.labelare escaped.hrefderives from a filename, so purely theoretical.- PR body still stale: it claims "3671 pass, 1 fail (
usage-policy-fold.test.js:387,
usage-policy fold test has never passed:fold_tightenedemitted when the spelling did not change the verdict #661)". At this head it is 3677 / 0 fail; that test passes. Flagged in round 1 too.
Verification
node scripts/run-tests.js→ exit 0, 3677 pass / 0 fail / 6 skipped (five
pandoc not installed, one ZSTD-unavailable - matches the expected shape exactly)npx tsc -p tsconfig.json --noEmit→ exit 0npm run build:types→ exit 0. Checked because the assets exclude was new:
tsconfig.build.jsoninherits it socopy-md.jsstays out of the declaration build, and
the emittedtypes/core/reports/{render,landing}.d.tsresolve their root-anchored
specifier correctly from the generated tree. The icebird convention holds.- CI green on
4f6ace0:test (22),test (24),typecheck (22),typecheck (24)
across both workflow runs, plusllp-check / duplicate-numbers. PRMERGEABLE. - Zero em dashes in added lines. No semicolons, JSDoc-only types,
interfaces in.d.ts.
philcunliffe
added
the
neutral:approved
Contributor
Verdict: approved - held for a maintainerRound 2 clean at This was strong work. The renderer-as-code move is well factored, the four new guards are Five low items are left for you, listed in the round-2 record. The two worth doing before
The rest ( One informational note for the maintainer, not a defect: removing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bundled skill surface had grown to 18
SKILL.mdfiles across two hand-synced trees, ~290 KB of prose, with the two largest at 25 KB and 27 KB. This reduces it to six skills, and does it by moving the mechanical half into tested code rather than by concatenating files.The skill surface is now
hypaware-ignore,hypaware-privacy,hypaware-query,hypaware-reference,hypaware-report,hypaware-unignore.The cause, and why the merge came last
The skills encoded procedures rather than constraints.
hypaware-report-to-htmlwas the clearest case: roughly 110 of its 399 lines were a prose reconstruction of a shell script, explaining how to write amasthead()function, whichsipscall regenerates a favicon, and whywriteTextloses Safari's clipboard permission across anawait.The reason was structural.
build.shlived only in a user's~/hypaware-reports, untracked. Not versioned with the skill, it could not be called and trusted, so the skill carried its logic as prose plus repair instructions for older copies (grep -q masthead build.sh). Prose was the fallback for code we failed to ship.So the mechanics moved first and the merge came last. Merging four 25 KB skills before removing their mechanical content would have produced one 60 KB skill: the same problem with fewer files.
What landed
hyp report render(LLP 0197 T1/T3/T4/T5). The renderer is nowsrc/core/reports/render.js, ported off BSD-onlysed -E -i ${QUOTE}${QUOTE}and macOS-onlysips. That portability was not cosmetic: CI isubuntu-latest, so the renderer previously had no automated coverage and could not have any. The port was accepted by A/B against the shell original on a real reports tree, byte-identical across five reports. It also absorbed the landing page, which nothing generated before, soindex.htmlwas transcribed by a model from a template on every run and was neither reproducible nor recoverable if deleted.The theme layer (T7).
assets/style.cssis the command's and refreshed every run;assets/theme.cssis the user's, created once with a commented stub and never touched, linked after the base sheet on every page. This removes an undecidable call: the skill used to guess whether a modified stylesheet was a customization or stale, by looking for a webfont import, which misclassified the most obvious customization as rot.The gate moved onto the act (LLP 0196 §5, maintainer decision).
disable-model-invocationis gone from the report skills, so reports can be asked for in the user's own words. Verified the precondition rather than assuming it: publishing and applying already confirmed, rendering did not — its enrichment step rewrote report sources with no confirmation at all, so that confirmation was added before the gate came off. The Codex copies had been model-invocable all along with the same hole.The skills cut to judgment (T10, T12). Rendering went 28.4 KB → 13.0 KB. The four report skills became one
hypaware-report: a 4.5 KB router naming the four stages and carrying the rules that hold across all of them, with detail inreviewing.md/rendering.md/publishing.md/applying.md, loaded on entry.Four new guards, each for a failure that already happened
report-assets-canonicalskill-host-parityhypaware-privacy)skill-constraints-surviveclient-skill-manifest-agreementEach was verified to fail on injected breakage, not just to pass.
The last one caught a real bug in this PR: a skill set is declared in three places, and T12 updated only the directories. Both plugins would have installed five skills that no longer exist and never installed
hypaware-report. The merge would have shipped as pure breakage with 3600 tests green.The constraint guard earned its keep too. It flagged eight constraints as dropped during T12; every one had moved from a
SKILL.mdinto a stage file. A skill is its entry file plus the reference files it loads, so both it and the parity guard now read every.mdin a skill directory. A false alarm during exactly the refactor a guard exists for is how people learn to loosen it.Two places the plan was wrong
hypaware-graphwas not merged intohypaware-query. The plan assumed claude and codex owned it. They did not:@hypaware/context-graphships it and those were byte-identical duplicates carried unconditionally. Folding it in would have made a fourth copy and shipped graph guidance to installs with no graph plugin. The duplicates are deleted and the plugin that provides the feature owns the skill documenting it.T2 was scoped as "decide the correct side of each divergence". Reading all six diffs, there is no wrong side: nearly every diverging line is legitimate host-specific content.
hypaware-privacyin particular carries 88 codex-only lines because Codex has noCLAUDE_CODE_SESSION_IDand must resolve the session container off disk with refusal-on-ambiguity semantics, guarded by its own 132-line test. A generator emitting one tree from the other would delete tested privacy logic. It stays deliberately forked, with a note in both copies.Not in this PR
hypaware-reportwithout losing the four it replaces.Testing
npm test: 3671 pass, 1 fail. That failure isusage-policy-fold.test.js:387, which has never passed — verified by checking outf457009, the commit that introduced both the feature and the test. Filed as #661; unrelated to this work.npm run typecheckclean. CI gains a one-lineapt-get install pandocso the render tests run rather than skip.Design rationale: LLP 0196 (Accepted), LLP 0197 (Active).
🤖 Generated with Claude Code