chore(skills): the Phase 6 skill sweep + fix the pre-push guard rejecting every push

[thomasluizon]

Paired with orbit-ui-mobile#590 (the same sweep, that repo's 23 skills).

1. The skill sweep (REBUILD.md 10.2)

Both skills here swept against the writing-great-skills rubric. No trigger, completion criterion, phase number, or output contract changed.

Both carry a hand-maintained lockstep contract with their orbit-ui-mobile twins, so only api-only text was edited, plus one provably dead pointer: .claude/plans/completed/ has never existed in this repo and is gone from orbit-ui-mobile. Everything else with a good rubric case is flagged for a paired reconciliation rather than applied one-sidedly, since a one-sided edit creates the drift the lockstep contract exists to prevent.

2. The pre-push guard fix

The protect-main guard added in #433 rejected every push, not just a push to main.

It used a multi-line shell script in lefthook's inline run: field. On Windows that script does not survive lefthook's shell invocation: it dies with -c: line 9: syntax error: unexpected end of file before reading a single ref, and lefthook treats the non-zero exit as a hook failure.

Reproduced deterministically on a plain feature branch, which must pass:

printf 'refs/heads/chore/x abc refs/heads/chore/x def\n' | lefthook run pre-push

Moved into tools/check-push-target.mjs, invoked as a single command. tools/*.mjs is already pinned to LF in .gitattributes, so it is also immune to line-ending drift on a fresh Windows clone.

Verified both directions after the change:

stdin remote ref	result
refs/heads/chore/x	protect-main passes
refs/heads/main	BLOCKED, exit status 1

orbit-ui-mobile carries the identical inline guard and gets the same fix in a separate PR.

3. Cross-repo drift found, reported not fixed

Each is a behaviour change, so none is applied here:

• The ui copy's blocking fan-out contract is missing from the api copy. A stranded api review posts nothing, and orbit-api's claude-review.yml has the same single-execution property.
• .github/workflows/claude-review.yml cites stale phase numbers. It says skip "Phase 6 (dotnet build / test)" and "Phase 7 posting", but validate is Phase 7 and posting is Phase 8. Followed literally, CI skips the adversarial verification pass and runs the validation it was told to skip.
• rubric.md dimension 8 is stale against the post-#539 DESIGN.md in BOTH repos. It still sanctions the gradient header and asks for "violet-glow character", both deleted by the design freeze.
• The api rubric.md preamble claims /audit-code-quality shares it and "there is no second copy". Neither is true in this repo.

🤖 Generated with Claude Code

https://claude.ai/code/session_018M2vH4pDvNnxUPecWWvwBc

[claude]

Review Complete

Scope: PR #434 in thomasluizon/orbit-api — "chore(skills): the Phase 6 skill sweep + fix the pre-push guard rejecting every push"
Recommendation: APPROVE

Prior reviews/comments check: gh pr view 434 --json reviews,comments returned empty for both — no prior bot review or resolved threads exist on this PR, so the full diff was reviewed (nothing to diff-since-last-review against).

Files reviewed (4/4, all verdicted clean)

File	Verdict
.claude/skills/pr-review/SKILL.md	Clean — editorial trim only
.claude/skills/second-opinion/SKILL.md	Clean — editorial trim only
lefthook.yml	Clean — minimal, correct change
tools/check-push-target.mjs (new)	Clean — logic verified correct

Analysis

The actual bug fix (tools/check-push-target.mjs + lefthook.yml): replaces a multi-line inline shell script in lefthook's run: field with a single node tools/check-push-target.mjs invocation. The new script's logic matches the removed shell script's intent: it reads stdin (use_stdin: true), splits into lines, extracts field index 2 (remote_ref) from each <local_ref> <local_sha> <remote_ref> <remote_sha> line, filters against {refs/heads/main, refs/heads/master}, and exits 1 with a BLOCKED: message if any match — including the empty-stdin edge case (trim → '' → split → [''] → index 2 undefined → filtered out safely, no crash). .gitattributes already pins tools/*.mjs to text eol=lf, so the new file is covered without an update. No test exists for this or the sibling check-dashes.mjs tooling script — consistent with existing precedent (lefthook tooling scripts, not application features subject to the xUnit rule). The file-level /** ... */ header comment matches the existing convention in tools/arch-map.mjs; ORBIT0001 and the ui-mobile no-comments.cjs don't gate .mjs tooling in this repo.

The SKILL.md edits: pure prose trimming (dead-pointer removal of .claude/plans/completed/, confirmed not to exist in this repo, plus de-duplication of explanations still stated verbatim elsewhere, e.g. the "opencode absent in CI → UNAVAILABLE" behavior is preserved in Phase 6's own body and the second-opinion skill's "Operating rules" section). No trigger, phase number, gate, or output contract changed, matching the PR body's own claim.

Note on the PR body's disclosed-but-unfixed items (stale phase numbers in .github/workflows/claude-review.yml, missing blocking fan-out contract, stale rubric.md #8 DESIGN.md references, stale rubric preamble claim about /audit-code-quality): none of these files are in this diff, the author explicitly reported them as known drift for separate follow-up PRs, and fixing them here would itself be an undisclosed behavior change per the PR's own stated policy. Correctly out of scope — not re-raised as findings.

Severity	Count
Critical	0
High	0
Medium	0
Low / Info	0

Subagents

Agent	Verdict
security-reviewer	N/A — gate not met (no src/ code changed)
contract-aligner	N/A — gate not met (no DTO/Controller/packages/shared surface changed); also not verifiable in CI (sibling repo not checked out)

What's good

• The pre-push guard fix is a genuine root-cause fix (moves out of a shell-quoting-fragile inline YAML field into a portable, testable Node script) with documented before/after repro steps in the PR body.
• The SKILL.md trims correctly identify and remove a verifiably dead pointer instead of leaving stale doc rot.
• The author proactively surfaced adjacent drift found during the sweep instead of scope-creeping fixes into this PR — good discipline for a chore PR.

Recommendation

Approve — no Critical/High findings, diff is small, well-scoped, and each change is verified correct against the stated intent.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #434

Recommendation: APPROVE

Summary

Doc-only sweep of two skill files plus a real bug fix: the protect-main pre-push guard added in #433 rejected every push (not just pushes to main) because its multi-line inline shell run: block fails to parse on Windows. The fix moves the guard's logic into tools/check-push-target.mjs, invoked as a single command. The skill/agent-doc edits (.claude/agents/security-reviewer.md, .claude/skills/pr-review/SKILL.md, .claude/skills/second-opinion/SKILL.md) are prose trims with no trigger/phase/output-contract change. No src/ code, DTO, or contract surface is touched.

Findings

Critical: None.
High: None.
Medium: None.

Low / Info:

• tools/check-push-target.mjs was traced by hand: it correctly passes non-protected remote refs, blocks refs/heads/main/refs/heads/master, and handles empty stdin / multi-space or tab-separated fields. No test harness exists for tools/*.mjs in this repo (same as the pre-existing check-dashes.mjs/arch-map.mjs), so this isn't a new gap.
• .claude/agents/security-reviewer.md's description: field changed from a plain YAML scalar to a folded block scalar (>-) — semantically identical (single line, no folding needed), cosmetic only.
• Removing the .claude/plans/completed/ bullet from pr-review/SKILL.md Phase 2 is a correct dead-pointer cleanup — confirmed .claude/plans/ does not exist anywhere in this repo's tree.

Subagents

Agent	Verdict
security-reviewer	N/A — gate not met (no src/ code changed)
contract-aligner	N/A — gate not met (no DTO/Controller/packages/shared change)

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptation (Build/Unit Tests run as separate required checks)
Tests (dotnet)	N/A — skipped per CI adaptation

Deferred

• Dimensions 3, 6, 7, 8, 9, 10, 11, 12, 13, 14 (SOLID, type safety, console.log, DESIGN.md, parity, i18n, contract drift, security, backend hard rules, FEATURES.md): N/A — diff touches no application/frontend/backend-source/contract surface.
• Cross-repo drift the PR body itself flags as found, not fixed (stale Phase 6/7 references and the missing blocking-fan-out contract in .github/workflows/claude-review.yml, stale gradient/violet-glow language in rubric dimension 8, the stale "no second copy" claim in the rubric preamble): confirmed real on inspection — pre-existing, outside this diff's changed files, and explicitly deferred by the author to avoid a one-sided cross-repo edit. Not re-flagged as blocking here.
• Paired orbit-ui-mobile#590 is out of scope for this review (different repo).

What's good

• Root-caused the pre-push break (identified the exact Windows shell-parsing failure) instead of patching around it, and moved the logic into a .mjs script already covered by the repo's LF .gitattributes pinning.
• Verified both directions (non-main passes, main blocked) per the PR body's repro table.
• Declined to one-sidedly fix the cross-repo lockstep drift it found, consistent with the lockstep-pairing rule these skills document for themselves.

Recommendation

Merge as-is.