Repository navigation
chore(skills): the Phase 6 skill-quality sweep (REBUILD.md 10.2) - #590
Conversation
Swept all 23 repo-scoped skills against the writing-great-skills rubric, one agent per batch. Applied only changes that improve predictability, reduce context load, or remove duplication and sediment. No trigger, completion criterion, or output contract was changed. Net: 338 insertions, 547 deletions across 24 files. The dominant failure mode was the trailing "Guardrails, do NOT" section: in most skills every bullet was a negated restatement of a rule already stated positively upstream in the same file. A guardrail was deleted only when its meaning survives at another site inside the SAME file, so nothing now depends on a separate file staying loaded. Also fixed, found while sweeping: - FIVE skills had frontmatter that does not parse, so their descriptions never loaded and the model could not auto-invoke them by intent. The cause is an unquoted YAML scalar containing ": ". Affected: audit-code-quality, audit-performance, commit-sweep (argument-hint), deep-research, investigate. All five now use a folded block scalar. - prod-readiness and llm-council each carried a stray unmatched code fence, leaving trailing content inside ambiguous markdown. - commit-sweep credited the deleted parity-nudge hook with catching parity drift per edit; rewritten to describe the CI job that replaced it. - audit-code-quality Phase 0 pointed at "Phase 3's fallback"; it is Phase 2. - Em dashes removed from the six files whose swept lines carried them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #590
Scope: PR #590 in thomasluizon/orbit-ui-mobile (chore/skills-sweep -> main)
Recommendation: APPROVE
Summary
This PR is a prose-only sweep across 24 .claude/skills/*.md files (338 insertions, 547
deletions; no app/API code, no packages/shared types, no i18n strings). It trims
duplicated "Guardrails - do NOT" bullets, fixes five broken skill description: frontmatter
fields, removes two stray unmatched code fences, and corrects a few stale cross-references.
I spot-checked the PR's own stated cut rule (a guardrail is deleted only if its meaning
survives elsewhere in the same file) against audit-security, audit-code-quality,
thermo-nuclear-code-quality-review, and llm-council - every removed bullet I checked has
a surviving equivalent in the same file. No trigger, completion criterion, or output
contract changed, matching the PR's own stated constraint.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
None posted - one prose-accuracy nit was considered (below) but doesn't clear the Signal
gate for concretely-actionable Medium+, so it isn't posted as a finding.
Context only, not a finding: the PR body's defect table attributes "frontmatter that does
not parse" to five skills because of "an unquoted YAML scalar containing ': '". That cause
is verifiable for audit-code-quality ("...(D11): dead/stale code..."),
audit-performance ("...(D11): the perf.yml..."), and commit-sweep's argument-hint
("...<repo-scope: both|ui|api>") - each does contain a literal embedded ": ", a genuine
plain-scalar break per the YAML spec. For deep-research and investigate, their pre-PR
description: lines contain exactly one colon (the description: key separator itself) and
no other embedded colon anywhere in the frontmatter block, so those two skills' frontmatter
did in fact parse before this PR. Their real issue was em dashes in the description prose,
unrelated to YAML parsing (and .claude/ is in the dash-ban tool's own skip list,
tools/check-dashes.mjs's SKIP array, so it was never a gated violation either). The fix
applied (converting all five to description: >- folded block scalars) is harmless
regardless, so nothing in the shipped repo state is wrong - this is a narrative-accuracy
note about the PR description, not a code defect, so it's dropped per the Signal gate.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A - no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A - no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A - no packages/shared/src/types/* / orbit-api DTO changed, single-repo diff |
| security-reviewer | N/A - no orbit-api code changed |
| design-reviewer | N/A - no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A - diff is entirely .claude/skills/*.md; no lint glob covers .claude/ prose |
| Type check | N/A - no .ts/.tsx/.cs files touched |
| Tests | N/A - no test files touched |
| Build (api) | N/A - orbit-api untouched |
Deferred - N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A - no
apps/*UI file in the diff. - Parity (#9): N/A - no
apps/web/**orapps/mobile/**file in the diff. - i18n (#10): N/A - no i18n JSON or user-facing string in the diff.
- Contract drift / backward-compat (#11): N/A - no
packages/shared/src/types/*or
orbit-apiDTO in the diff; single-repo change. - Security (#12, API side) / Backend hard rules (#13): N/A - no
orbit-apicode. - FEATURES.md parity (#14): N/A - no user-facing feature surface changed (internal agent
tooling only). - Every changed file got a verdict. All 24 files were read in full (post-PR content via
git show b95d717c:<path>, since the working tree at review time was checked out to the
pre-PR base commit rather than the PR head):android-generate,audit-code-quality,
audit-performance,audit-security,audit-tests(+ itsrubric.md),bug,
commit-sweep,deep-research,dep-sweep,dev-server,feature,investigate,
lesson,llm-council,make-tool,orchestrate,pr-review,prod-readiness,
profile,rollup,second-opinion,thermo-nuclear-code-quality-review,validate.
Nothing deferred beyond the N/A dimensions above.
What's good
- The stated methodology (cut a guardrail only when its meaning survives elsewhere in the
same file) genuinely holds up under spot-checking across multiple files - a real,
verifiable discipline, not just a claim. - Three genuinely broken
description:frontmatter fields are fixed with evidence I could
independently reproduce by counting embedded colons in the pre-PR text
(audit-code-quality,audit-performance,commit-sweep'sargument-hint). - Both stray unmatched code fences (
llm-council,prod-readiness) are confirmed gone; a
fence-line-count check found no other file in the PR with an unpaired fence introduced or
left behind. - The
commit-sweepfix (crediting the CIguards.ymlCross-Platform Parity job instead of
a deletedparity-nudgehook) is accurate -.claude/hooks/no longer contains any
parity-nudgescript. - The
feature/SKILL.mdfix (dropping the unread.claude/orchestrator.jsonmention) is
accurate -tools/wave-plan.mjsreads that file itself;feature/SKILL.mdnever did. second-opinion/SKILL.md's "Interpreting the verdict" table andpr-review/SKILL.md
Phase 6's description of it stay in lockstep after the edit - no cross-file drift.- The PR body is explicit about what it deliberately left alone (Phase 0 boilerplate
duplication,verification-protocol.md's stale "report" vocabulary, etc.), the right call
for a mechanical sweep since those are separate, larger decisions.
Recommendation
Approve as-is. Nothing here blocks merge. Optional, non-blocking follow-up: when the "Not
changed, needs a decision" backlog in the PR body gets picked up, correct the causal claim
for deep-research and investigate in institutional memory (their pre-PR frontmatter
parsed fine; their issue was em-dash prose, not a YAML break) so a future sweep doesn't
inherit the wrong root cause.
… recur (#593) * fix(agents): security-reviewer never loaded, plus a gate so it cannot recur `/pr-review` names `security-reviewer` as one of the subagents it orchestrates. That agent was never loading, so its security pass has been running as a no-op. Cause: the frontmatter `description` is an unquoted YAML scalar containing ": " (in "security issues: missing [Authorize]"), which breaks the parse. The agent is then dropped with no error. Evidence: all 10 sibling agents appeared in the session's agent list and this one did not. Ruled out the alternatives first: no duplicate `name` in the directory, no agent-disabling key in any settings scope. Fixed by moving the value to a folded block scalar; verified live, the agent appeared in the list immediately after. This is the EIGHTH instance of the same bug found today. Five skills hit it in #590 (audit-code-quality, audit-performance, commit-sweep, deep-research, investigate), where the symptom was a silently missing description, so the model could not auto-invoke them by intent. So it gets a gate rather than a ninth manual fix: - `tools/check-frontmatter.mjs` scans every `.md` under `.claude/skills` and `.claude/agents` and fails on an unquoted frontmatter value containing ": ". `--fix` rewrites offenders to a block scalar. - New `Skill and Agent Frontmatter` job in `guards.yml`. - Proved it catches the real defect: reverting security-reviewer to the broken form makes the gate exit 1 and naming that exact file. `Explore.md` was also normalized. It carries the same pattern but loads correctly, so it is not a live defect; the rule is kept absolute rather than special-cased, because a block scalar is strictly safer and "no unquoted ': ' in frontmatter" is a rule with no judgement in it. Also corrects this repo's CLAUDE.md, which described orbit-landing-page as Astro 6. It is Astro 7.1.3. Follow-up, not in this PR: orbit-api needs the same vendored gate. Its own security-reviewer copy is fixed on the #434 branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc * docs(guards): list the frontmatter job in the workflow header The header enumerates every job; the new one was missing from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes the last open item of REBUILD.md Phase 6.
All 23 repo-scoped skills swept against the
writing-great-skillsrubric, one agent per batch. Applied only changes that improve predictability, reduce context load, or remove duplication and sediment. No trigger, completion criterion, or output contract was changed; every proposal that touched one is flagged for a separate decision, not applied here.Net: 338 insertions, 547 deletions across 24 files.
The dominant pattern
The trailing
## Guardrails, do NOTsection. In most skills every bullet was a negated restatement of a rule already stated positively upstream in the same file, which is both duplication and steering by prohibition. The cut rule used throughout: a guardrail was deleted only when its meaning survives at another site inside the same file, so nothing now depends on a separate file staying loaded. Guardrails that duplicated only againstCLAUDE.mdorcore.mdwere left in place.Defects found while sweeping, fixed here
descriptionnever loaded, so the model could not auto-invoke them by intent. Cause: an unquoted YAML scalar containing": ". Affectedaudit-code-quality,audit-performance,commit-sweep(argument-hint),deep-research,investigate. All five now use a folded block scalar, verified loading.prod-readinessandllm-councileach had a stray unmatched code fencecommit-sweepcredited the deletedparity-nudgehookaudit-code-qualityPhase 0 cited "Phase 3's fallback"featuredeclared an.claude/orchestrator.jsonconstant it never readswave-plan.mjsreads it itselfEm dashes were removed from the six files whose swept lines carried them.
Not changed, needs a decision
Collected across batches, none applied:
audit-code-qualitypoints at "the rubric's Phase 0 note". Consolidating needs one coordinated cross-skill edit._shared/verification-protocol.mdstill speaks the pre-D10 vocabulary of a report throughout. It is upstream of all five audit skills, so it wants one edit there rather than five workarounds.audit-performance/SKILL.mdis its own finders' checklist (audit.mjs:104), unlike the other three kinds which point at a sibling file. So its prose cannot be disclosed out of the file, and every edit silently mutates the Haiku finder prompt.audit-security's headless fallback prescribesExplorefinders whileaudit.mjsusesaudit-readonlyeverywhere, so the fallback path runs a wider tool surface than the primary path..claude/audits/prod-readiness-both.mdstill exists on disk while all three D10-rewritten files promise nothing is persisted there. That leftover is the rotting photograph D10 kills.llm-councilanddeep-research: both attribute a "3-concurrent subagent cap" to the rootCLAUDE.md, which contains no such rule./validatecovers two repos while the workflow is now three, its description carries no trigger phrasing, and itsdotnet buildstep cannot surfaceORBIT0001..0005locally, so a green/validateis not proof the analyzer gates pass in CI./investigatePhase 2 still asks to confirm the Render workspace choice, which contradicts the standing auto-select preference.descriptionis permanent context load, flagged per skill.Full per-skill reports with the withheld proposals and their rationale are in the session scratchpad.
🤖 Generated with Claude Code
https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc