Skip to content

feat: restore companion skill loading to orch skills and commands - #218

Merged
dean0x merged 7 commits into
mainfrom
feat/restore-companion-skill-loading
May 13, 2026
Merged

dean0x merged 7 commits into
mainfrom
feat/restore-companion-skill-loading

Conversation

@dean0x

@dean0x dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner

Restore companion skill loading mechanism to all orch skills and orchestration commands.

Summary

Re-enable the companion skill loading infrastructure that was previously disabled. This restores the ability for orchestration skills to automatically load specialized companion skills during execution, improving code organization and reducing boilerplate in command implementations.

Changes

  • Add companion skill loading references to all orch skills (debug, implement, plan, release, review)
  • Update all orchestration commands to reference companion skills
  • Update skill catalog documentation to reflect companion skill availability

Testing

  • Verify that orch commands load specialized companion skills as expected
  • Test that skills are available in downstream agent invocations
  • Check that no duplicate or conflicting skill loading occurs

Related Issues

Completes restoration of skill composition patterns after recent refactoring.

Co-Authored-By: Claude noreply@anthropic.com

Dean Sharon added 2 commits May 12, 2026 22:50
Commit d56f391 migrated companion loading from the router into guided
skills but dropped it from ORCHESTRATED depth entirely. This restores
always-on companion skills to all 5 orch skills and 10 commands, matching
the guided skill lists exactly. File-type conditionals are not included
at ORCH depth as agents load their own language skills.

Companion loading is placed before Continuation Detection so no
resume/re-validation/refinement path can bypass it.
… - Add cli-rules KNOWLEDGE.md entry to index - Update index version marker for reliability rule changes Co-Authored-By: Claude <noreply@anthropic.com>
@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

[INLINE #1] Inconsistent Section Ordering in debug:orch (HIGH, 85% confidence)

The "Load Companion Skills" section is placed after Worktree Support (line 25), but in implement:orch, review:orch, and release:orch it appears immediately after Iron Law. This creates two distinct placement patterns instead of one.

For consistency, move the "Load Companion Skills" section to immediately after Iron Law and before any content sections (Worktree Support, etc.), matching the 3-of-5 majority pattern.


Claude Code Attribution: This is a code review finding from automated review analysis.

@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

[INLINE #2] Inconsistent Section Ordering in plan:orch (HIGH, 85% confidence)

The "Load Companion Skills" section is placed after Worktree Support, but in implement:orch, review:orch, and release:orch it appears immediately after Iron Law. This creates two distinct placement patterns instead of one.

For consistency, move the "Load Companion Skills" section to immediately after Iron Law and before any content sections, matching the 3-of-5 majority pattern.


Claude Code Attribution: This is a code review finding from automated review analysis.

@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

[BLOCKING] CLAUDE.md Ambient Mode Description Needs Update (HIGH, 85% confidence)

File: CLAUDE.md, around line 46

Problem: CLAUDE.md states that the router "maps intent + depth to a guided skill (short, focused, loads companion skills) or an orch skill (full agent pipeline)". The parenthetical implies only guided skills load companions, but this PR restores companion skill loading to orch skills too.

Impact: The primary project documentation now contradicts the implemented behavior. The new skill-catalog.md correctly documents ORCHESTRATED companion skills, but CLAUDE.md -- the canonical reference -- is now outdated.

Fix: Update line 46 to:

Router SKILL.md is a pure dispatcher loaded on-demand only for GUIDED/ORCHESTRATED depth -- maps intent + depth to a guided skill (short, focused, loads companion skills) or an orch skill (full agent pipeline, loads companion skills before first phase).

Claude Code Attribution: This is a code review finding from automated review analysis.

@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

Code Review Summary

Issues Identified

BLOCKING ISSUES (3):

  1. debug:orch & plan:orch inconsistent ordering — Two distinct placement patterns for "Load Companion Skills" across 5 orch skills (see inline comments above)
  2. CLAUDE.md needs documentation update — Ambient mode description still implies only guided skills load companions (see inline comment above)
  3. No tests validate companion skill consistency — The companion skill lists across 3 surfaces (skill-catalog.md, orch skills, commands) must stay in sync. Consider adding a regression test to tests/skill-references.test.ts that validates:
    • Orch skill companion lists match skill-catalog ORCHESTRATED table
    • Command companion lists match skill-catalog ORCHESTRATED table
    • Base and teams command variants match

SHOULD FIX ISSUES (2 - Medium confidence, 80-82%):

  1. Inconsistent "Load Companion Skills" placement in commands — In code-review.md it's in Phase 1b (inside phase), in others it's in Phase 1-2 at the top. While locally reasonable, standardization would improve maintainability.
  2. Missing Phase Completion Checklist items in 4 orch skillsdebug:orch added the checklist line, but plan:orch, implement:orch, review:orch, and release:orch are missing this audit step.

Review Scores

Domain Score Status
Architecture 9/10 APPROVED
Complexity 9/10 APPROVED
Consistency 8/10 APPROVED_WITH_CONDITIONS
Documentation 7/10 APPROVED_WITH_CONDITIONS
Security 10/10 APPROVED
Performance 10/10 APPROVED
Regression 10/10 APPROVED
Reliability 9/10 APPROVED
Testing 7/10 APPROVED_WITH_CONDITIONS

Recommendation

APPROVED_WITH_CONDITIONS — Merge after:

  1. Fixing the 3 blocking issues above (HIGH confidence)
  2. Consider adding the regression test for companion skill consistency (HIGH priority)
  3. Addressing the should-fix checklist items in remaining orch skills (MEDIUM priority)

The core feature (restoring companion skill loading) is sound and complete — all 5 orch skills, 10 commands, and the skill catalog are consistent today. The issues are about structural standardization and test coverage to prevent future drift.


Claude Code Attribution: Comprehensive review completed by automated devflow code review system.

…e CLAUDE.md, add consistency test

- Move Load Companion Skills before Worktree Support in debug:orch and
  plan:orch to match the majority pattern (Iron Law → Companions → rest)
- Update CLAUDE.md ambient mode description to reflect orch companion loading
- Add companion skill consistency test validating catalog/orch/command parity
@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

File: tests/skill-references.test.ts (line 1079)

Missing error handling for teams variant file reads

The test calls readFileSync(cmdPath, 'utf-8') for all command files including -teams.md variants without try/catch protection. If a teams variant is removed in the future, the test will throw an unhandled ENOENT error instead of gracefully skipping.

This contrasts with the pattern at line 992-996 which wraps teams file reads in try/catch.

Fix: Wrap the readFileSync in try/catch, consistent with the existing pattern:

for (const cmdRelPath of intentCommandMap[intent]) {
  const cmdPath = path.join(ROOT, cmdRelPath);
  let cmdContent: string;
  try {
    cmdContent = readFileSync(cmdPath, 'utf-8');
  } catch {
    continue; // teams variant may not exist
  }
  const cmdSkills = parseCompanionLine(cmdContent);
  expect(cmdSkills, `${cmdRelPath} companions must match catalog for ${intent}`).toEqual(expectedSkills);
}

@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

File: shared/skills/implement:orch/SKILL.md (line 92)

Incomplete orch skill ordering standardization

The PR's stated goal is to standardize section ordering so "Load Companion Skills" comes before "Worktree Support." This was successfully applied to debug:orch and plan:orch (now lines 21-25), but implement:orch still has "Worktree Support" buried at line 92 (between Phase 3 and Phase 4), far from the top-of-file placement used elsewhere.

The commit message claims "standardize orch skill ordering" but the standardization is incomplete.

Fix: Move the "Worktree Support" section to line 25 (right after "Load Companion Skills"), matching the pattern in debug:orch and plan:orch.

@dean0x

dean0x commented May 12, 2026

Copy link
Copy Markdown
Owner Author

Code Review Summary

Reviewers: Architecture, Consistency, Reliability, Testing
High-Confidence Issues: 3 MEDIUM (≥80% confidence)
Lower-Confidence Suggestions: 4 (60-79% confidence in summary)


Blocking Issues (≥80% confidence)

1. Incomplete Orch Skill Ordering Standardization

Files: implement:orch (line 92), release:orch (line 250), review:orch (missing section)
Confidence: 85% (Architecture + Consistency reviewers)

The PR's stated goal is to standardize section ordering so "Load Companion Skills" comes before "Worktree Support." This was successfully applied to debug:orch and plan:orch, but:

  • implement:orch still has Worktree Support at line 92 (between Phase 3 and Phase 4)
  • release:orch has it at line 250 (near the end)
  • review:orch lacks the section entirely

The new test validates companion skill names but not section ordering, so future reordering drift would go undetected.

Action: Move Worktree Support to immediately after Load Companion Skills in all orch skills for consistency.


2. Missing Error Handling for Teams Variant File Reads

File: tests/skill-references.test.ts (line 1079)
Confidence: 85% (Reliability reviewer)

The test calls readFileSync(cmdPath, 'utf-8') for all command files including -teams.md variants without try/catch protection. If a teams variant is removed in the future, the test will crash with an unhandled ENOENT error instead of gracefully skipping.

This contrasts with the pattern at line 992-996 which wraps teams variant reads in try/catch.

Action: Wrap readFileSync in try/catch block (see inline comment with fix).


3. Test Validates Content But Not Section Ordering

File: tests/skill-references.test.ts (line 1007)
Confidence: 82% (Consistency reviewer)

The new test validates that companion skill names match the catalog across orch skills and commands — good coverage. However, the PR's main change was about section ordering (Load Companion Skills before Worktree Support), and the test does not verify that constraint.

A future change could reorder sections incorrectly and the test would still pass.

Action: Add lightweight ordering assertion for orch skills containing both sections.


Pre-Existing Issues (Informational)

  • Missing Worktree Support in review:orch (80% confidence) — Orch skills with companion skills should have consistent Worktree Support placement
  • Inconsistent placement in explore:orch, research:orch (80% confidence) — These place Worktree Support at the end, not near the top like other orch skills

Lower-Confidence Suggestions (60-79%)

  1. Test regex could miss new intents (62% confidence) — orchTableRegex hardcodes 5 intent names; adding a new companion-enabled intent would silently not be covered
  2. Assertion precision (65% confidence) — Use .toBe(5) instead of .toBeGreaterThanOrEqual(5) in catalog size check
  3. Guided skill companion consistency (70% confidence) — The test covers ORCHESTRATED companions but not GUIDED companions
  4. Regex $ anchor behavior (65% confidence) — The multiline regex flag makes $ match end-of-line rather than end-of-string; low risk but could cause issues if format changes

Summary

Scores: Architecture 8/10 | Consistency 7/10 | Reliability 8/10 | Testing 8/10

The core architectural pattern is sound: companion skill loading sections restored with a catalog as single source of truth, backed by a consistency test. However, the "standardization" of section ordering is incomplete across the five orch skills, and the test doesn't enforce the ordering that this PR is specifically about. Fixing the implement:orch ordering and adding error handling + ordering assertions would resolve all blocking issues.

Recommendation: CHANGES_REQUESTED (3 MEDIUM issues at ≥80% confidence)

Dean Sharon added 4 commits May 13, 2026 11:36
…Wrap command-file readFileSync in try/catch so missing teams variants skip gracefully (matching existing pattern at line 992-996) rather than throwing an uncaught ENOENT - Add ordering assertion: Load Companion Skills must precede Worktree Support in each orch skill that contains both sections, locking the invariant introduced by this PR against future regressions Co-Authored-By: Claude <noreply@anthropic.com>
…ase:orch Move the Worktree Support section in both skills to immediately after Load Companion Skills, matching the pattern established in debug:orch and plan:orch (Iron Law, Load Companion Skills, Worktree Support, phases). Previously implement:orch had Worktree Support buried between Phase 3 and Phase 4, and release:orch had it after Phase 7 near end of file. Co-Authored-By: Claude <noreply@anthropic.com>
…ons The ordering test silently skipped validation when a section was missing via an if-guard. Replace with explicit toBeGreaterThanOrEqual(0) assertions so missing sections produce clear failures instead of passing silently.
…nce The skill catalog is a cross-cutting reference doc, not router-specific. Move it alongside the other reference docs and update the test path.
@dean0x
dean0x merged commit bbdf8ad into main May 13, 2026
4 checks passed
@dean0x
dean0x deleted the feat/restore-companion-skill-loading branch May 13, 2026 09:25
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.

1 participant