fix: decode HTML entities in skill-stderr so an approved grant matches

[WeiYiAcc]

Summary

A skill substitution command that a human approves is stored with the HTML escaping it
arrived with, so the grant never matches the rule recomputed from SKILL.md and the
command stays refused. Fixes #346.

The defect

The blocked command is captured from <local-command-stderr>, where the SDK escapes the
payload — a declaration of !`sh x 2>/dev/null` arrives as sh x 2>/dev/null.
#tryHandleSkillBlock keys the persisted grant on that string verbatim, while
skillCommandAllowRules recomputes the rule from the unescaped SKILL.md. The two are
compared by exact string equality, so the filter at #resolveSkillGrants returns []
and allowedTools is built without the grant.

The failure is quiet in the worst way: the approval UI reports success and the daemon
logs skill command approved: …, but the retry is refused again. The anti-loop guard
reads the same escaped key, concludes the command is already granted, and suppresses
the re-park — so the phase dies with phase turn ended in "error" instead of recovering.

The fix

Decode entities at the stderr convergence point — the one string every downstream key
(the persisted grant, the guard, and allowedTools) derives from, so nothing else
changes:

state.lastLocalCommandStderr = decodeStderrEntities(
  content.replace(/<\/?local-command-stderr>/g, "").trim(),
);

& is decoded last, so a doubly-escaped > resolves to the literal >
rather than over-decoding into >. < > " ' & cover what the SDK
emits here; a ~10-line local helper, no dependency (per CONTRIBUTING).

Test plan

• decodeStderrEntities unit tests, including the &-last ordering that prevents
  over-decoding.
• A table test asserting the decoded stderr key equals the rule skillCommandAllowRules
  recomputes for a >-bearing command.
• A provider-level test driving a >-bearing command through park → approve → rebuild
  → retry, asserting the stored key, the rebuilt allowedTools, and a non-error
  turn_done. This test fails without the decode (verified by reverting just the
  decode: 114 pass / 1 fail) and passes with it.

bun run typecheck (all three projects) is clean, biome check is clean, and the
src/tests + src/daemon suites pass. Three failures in
src/integration/conductor-zeroid.test.ts are unrelated — they hit a live ZeroID and
fail identically on unmodified main.

Note on severity

The key mismatch is static and deterministic, but it only kills the phase when the
blocked expansion leaves the turn at num_turns: 0. #346 documents a second run where
other tools ran in the same turn and the phase survived. So a headless pipeline run,
where a skill phase is often the only thing in the turn, hits it reliably; a busy
interactive session may not. The grant is broken either way.

#346 also records three further, independent defects found alongside (skill approval has
no timeoutMs, so it hangs forever with no ui.dialogs client attached; codeoid approve
sends approvalId: "" against a min(1) schema). Those are out of scope here and left
for a decision.

🤖 Generated with Claude Code

[highflame-oracle]

🔮 Oracle Review

🎯 Start Here

src/daemon/providers/claude/index.ts (~15 min) — Logic changes in index.ts

---

📋 PR Summary

What this PR does: Adds HTML entity decoding at the skill-stderr convergence point so that persisted grant keys match the unescaped rules recomputed from SKILL.md, fixing a silent mismatch that caused approved skill commands to be refused.

Key changes:

• Introduce decodeStderrEntities helper that decodes lt, gt, quot, apos, and amp (amp last to prevent over-decoding of doubly-escaped entities like >)
• Apply decode at the single convergence point where state.lastLocalCommandStderr is assigned, aligning all downstream keys (persisted grant, anti-loop guard, allowedTools)

Areas affected: skill command approval/grant matching, local-command-stderr processing

Testing notes: Unit tests for decodeStderrEntities (including amp-last ordering), table test asserting decoded key equals recomputed rule for >-bearing commands, and provider-level park→approve→rebuild→retry integration test that fails without the decode (verified 114 pass / 1 fail on revert)

---

🔍 Code Review

This is a precisely scoped, well-reasoned fix for a subtle but impactful bug where HTML entity escaping caused a deterministic key mismatch that silently broke skill grant matching. The decode helper is minimal, dependency-free, and correctly orders the ampersand decode last to prevent over-decoding — applied at exactly the right convergence point to fix all downstream consumers without broader refactoring.

What's good:

• ✨ Exceptionally thorough PR description that traces the full causal chain from SDK escaping through the anti-loop guard to the silent phase death, making the fix rationale unambiguous
• ✨ Decode applied at the single convergence point rather than patching multiple downstream consumers — minimal blast radius, no behavioral change elsewhere
• ✨ Ampersand-decoded-last ordering prevents over-decoding edge cases (e.g., > → > not >), with test coverage verifying this
• ✨ No external dependency introduced for a 10-line helper, respecting CONTRIBUTING guidelines

Review Stats:

---

Generated by Oracle - Highflame's AI Code Reviewer

[highflame-oracle]

Consider defensive handling of ' entity

The entity set (< > " ' &) correctly covers what the SDK currently emits. However, if the SDK or a future upstream change emits ' (the XML-named equivalent of '), it would silently pass through undecoded, reintroducing the same class of mismatch. Adding ' → ' before the & replacement would be a low-cost defensive measure with no downside for the current input domain:

.replace(/'/g, "'")
.replace(/'/g, "'")

That said, ' is not valid HTML4 and most encoders (including what the SDK uses) prefer ', so this is purely a maintenance guard — not a current bug.

Suggested fix:

Suggested change

	}
	export function decodeStderrEntities(s: string): string {
	return s
	.replace(/&lt;/g, "<")
	.replace(/&gt;/g, ">")
	.replace(/&quot;/g, '"')
	.replace(/&apos;/g, "'")
	.replace(/&#39;/g, "'")
	.replace(/&amp;/g, "&");
	}

[WeiYiAcc]

Good call — adopted in 736236b. ' is now decoded before &, next to ' (same value, so order between those two is irrelevant; only & must stay last). Added a regression test asserting both spellings of the apostrophe map to '.

Agreed it is a guard rather than a live bug: the channel only ever shows the numeric form today. The cost is one .replace, so it is worth closing the door before an encoder swap reopens #346 silently.

[highflame-oracle]

🔮 Oracle Review

🎯 Start Here

src/daemon/providers/claude/index.ts (~15 min) — Logic changes in index.ts

---

📋 PR Summary

What this PR does: Decodes HTML entities in skill-stderr at the capture point so that persisted grant keys match the rules recomputed from SKILL.md, fixing a bug where approved skill commands were silently refused on retry.

Key changes:

• Added decodeStderrEntities helper that decodes lt, gt, quot, apos, and amp (amp last to prevent over-decoding of doubly-escaped sequences)
• Applied decode at the single convergence point where state.lastLocalCommandStderr is assigned, ensuring all downstream consumers (persisted grant, anti-loop guard, allowedTools) use the same unescaped key

Areas affected: skill command approval/grant resolution, stderr processing in claude provider

Testing notes: Unit tests for decodeStderrEntities (including amp-last ordering), table test verifying decoded key equals recomputed rule for >-bearing commands, and provider-level park→approve→rebuild→retry test that fails without the fix (verified by revert: 114 pass / 1 fail)

---

🔍 Code Review

This is an exemplary targeted fix — it identifies the exact convergence point where the mismatch originates and corrects it with minimal blast radius, preserving all downstream semantics without refactoring. The amp-last decoding order and defensive handling of ' demonstrate careful reasoning about edge cases, and the provider-level test that demonstrably fails on revert provides strong correctness evidence.

What's good:

• ✨ Fixing at the single convergence point rather than patching multiple comparison sites is excellent architecture-aware reasoning — it guarantees consistency without scattering decode logic
• ✨ The amp-last decoding order to prevent over-decoding of doubly-escaped sequences shows thorough understanding of the escaping semantics
• ✨ The PR description itself is outstanding — the defect explanation, failure mode analysis (quiet failure with anti-loop guard interaction), and severity note provide exceptional context for reviewers and future readers

Review Stats:

---

Generated by Oracle - Highflame's AI Code Reviewer

[highflame-oracle]

Consider documenting the boundary of the entity set

The named entity set (< > " ' & + ') covers what the SDK emits today, and the JSDoc explains this. However, if the SDK ever emits numeric decimal entities like < (for <) or > (for >), they'd pass through undecoded and silently reintroduce the same class of mismatch. Consider adding a brief inline comment or a runtime assert/dev-mode check that logs a warning if any &#\d+; or &#x[0-9a-fA-F]+; patterns remain after decoding — not to handle them now, but to surface a future SDK change before it becomes a silent mismatch again.

[saucam]

@WeiYiAcc thank you for this fix !