fix(ambient): remove allowed-tools restriction so skills actually load

[dean0x]

Summary

• Root cause: ambient-router skill had allowed-tools: Read, Grep, Glob which prevented Claude from invoking the Skill tool — skills were classified but never loaded
• Removed allowed-tools from ambient-router (unrestricted access as main-session orchestrator)
• Added override note in Step 4 <IMPORTANT> block so loaded skills' allowed-tools don't restrict main session capabilities
• Strengthened hook preamble with explicit Skill tool mention for GUIDED/ORCHESTRATED
• Added hasSkillLoading() and extractLoadedSkills() integration test helpers + 2 verification tests

Test plan

• npm run build — 35 skills distributed across 17 plugins
• npm test — 250/250 pass
• npm run test:integration — verify GUIDED/ORCHESTRATED prompts show "Loading:" marker (requires claude CLI)
• Manual: new session → GUIDED prompt → confirm Skill tool invocations in output

[dean0x]

Issue: Flaky Integration Tests (85% confidence)

This test asserts on non-deterministic LLM classification in -p mode. The file acknowledges classification may not appear, yet the test uses hard expect(...).toBe(true) without conditional fallback.

Fix: Use it.skip() or conditional assertions:

if (hasClassification(output)) {
  expect(hasSkillLoading(output)).toBe(true);
}

---

Claude Code Review

[dean0x]

Issue: Flaky Integration Tests (85% confidence)

Same as GUIDED test above — this assertion may fail in -p mode due to unreliable classification.

Fix: Skip or use conditional assertions (see line 73 comment).

---

Claude Code Review

[dean0x]

Issue: Duplicated Magic String (82% confidence)

The AMBIENT_PREAMBLE is defined identically here and in scripts/hooks/ambient-prompt:42. These can drift if one is updated without the other.

Fix: Add a test that verifies the preamble substring exists in the hook script:

it('preamble matches ambient-prompt hook script', () => {
  const hookScript = readFileSync(
    path.join(__dirname, '../../scripts/hooks/ambient-prompt'),
    'utf-8',
  );
  expect(hookScript).toContain(AMBIENT_PREAMBLE);
});

---

Claude Code Review

[dean0x]

Issue: Sequential I/O Performance (82% confidence)

The two fs.access checks for .docs/ and .memory/ are awaited sequentially. These are independent filesystem operations that could run in parallel.

Fix: Use Promise.allSettled:

const [docsResult, memoryResult] = await Promise.allSettled([
  fs.access(docsDir),
  fs.access(memoryDir),
]);
if (docsResult.status === 'fulfilled') extras.push('.docs/');
if (memoryResult.status === 'fulfilled') extras.push('.memory/');

---

Claude Code Review

[dean0x]

Issue: Untested Deduplication Logic (82% confidence)

The formatDryRunPlan function explicitly deduplicates inputs with [...new Set(assets.skills)], but no test verifies this behavior. If the dedup were removed, no test would catch it.

Fix: Add test case:

it('deduplicates repeated asset names', () => {
  const plan = formatDryRunPlan({
    skills: ['core-patterns', 'core-patterns', 'typescript'],
    agents: ['coder', 'coder'],
    commands: ['/implement'],
  });
  expect(plan).toContain('Skills (2)');
  expect(plan).toContain('Agents (1)');
});

---

Claude Code Review

[dean0x]

Code Review Summary

Branch: fix/ambient-skill-loading → main
Commits: 3 | Changed Files: 9
Review Date: 2026-03-20

Inline Comments Created (6)

1. Flaky Integration Tests (lines 73, 82) — GUIDED/ORCHESTRATED tests assert on unreliable -p mode classification. Use it.skip() or conditional assertions.
2. Duplicated Preamble (line 18, helpers.ts) — String duplicated with ambient-prompt hook. Add test to verify sync.
3. Sequential I/O (line 206, uninstall.ts) — Two fs.access checks should use Promise.allSettled for parallelism.
4. Missing Test (line 68, uninstall-logic.test.ts) — formatDryRunPlan deduplication untested.

Additional Findings (60-79% confidence, see details below)

Documentation Issues:

• README missing --dry-run, --plugin, --verbose flags in Uninstall Options table
• CHANGELOG [Unreleased] empty — needs entries for new features/fixes
• CLAUDE.md wording could clarify that ambient-router omits allowed-tools entirely

Architecture/Design:

• uninstall.ts is a 610-line monolith (pre-existing, not introduced here)
• formatDryRunPlan could extract to utility module but is well-tested
• Unit test imports from tests/integration/helpers.js (boundary violation, pre-existing import)

Test Coverage:

• Missing tests for extractIntent with EXPLORE and CHAT classifications
• Integration test comment is verbose but well-documented

Performance/Robustness:

• Pre-existing: Shell hook invokes jq three times per prompt (optimization opportunity)
• Pre-existing: execSync uses string interpolation (should use execFileSync)
• Suggestion: Dedup in computeAssetsToRemove rather than formatDryRunPlan

Security (Defense-in-Depth):

• ambient-router has no allowed-tools restriction (intentional, documented, but defense-in-depth concern)

Recommendation

APPROVED WITH CONDITIONS

Merge after addressing:

1. HIGH PRIORITY: Fix flaky integration tests (skip or condition assertions)
2. MEDIUM PRIORITY: Verify preamble sync or add test
3. OPTIONAL: Add CHANGELOG entries, update README docs

All three commits are atomic and well-tested. No blocking regressions. The ambient skill-loading fix is correct and necessary.

---

Detailed Review Reports: .docs/reviews/fix-ambient-skill-loading/