fix: prevent nested skill invocations from orphaning skill-active-state

[DdangJin]

Summary

• Add nesting guard in writeSkillActiveState(): if another skill is already active in the same session, skip writing — the parent's protection already covers the session
• Add explicit skill-active-state.json cleanup in setup-progress.sh complete to ensure no stale skill state survives after setup finishes
• Make PostToolUse clear nesting-aware: only clear state if the completing skill owns it (prevents child completion from deleting parent's state)
• Sync SKILL_PROTECTION_MAP between MJS and TS (7+ entries including ralplan)
• Add _meta envelope to MJS state writes, session ID validation, stale-only cleanup

Fixes #2024

Root Cause

During omc-setup, Phase 3 invokes mcp-setup via the Skill tool. writeSkillActiveState() unconditionally overwrites the parent (omc-setup) state with the child (mcp-setup) state. When both skills complete, the child's active: true state persists with no completion signal, causing the stop hook to block 5 times with "[SKILL ACTIVE: mcp-setup] still executing".

Additionally, processPostToolUse unconditionally called clearSkillActiveState() on any Skill completion, meaning even with the nesting guard protecting writes, the child's PostToolUse would delete the parent's state.

Reproduced during omc-setup update from 4.9.1 to 4.9.3.

Changes

src/hooks/bridge.ts

• PostToolUse now reads current state and only clears if the completing skill owns it

src/hooks/skill-state/index.ts

• Nesting guard in writeSkillActiveState(): skip write if a different skill is already active

scripts/pre-tool-enforcer.mjs

• Matching nesting guard + TOCTOU comments
• Synced SKILL_PROTECTION_MAP with canonical TS version
• Added _meta envelope to state writes
• Use validated safeSessionId for session_id field

scripts/setup-progress.sh

• Session-scoped cleanup with regex-validated session ID
• Fallback find scoped to stale files only (-mmin +30)

src/hooks/skill-state/__tests__/skill-state.test.ts

• 5 new tests including end-to-end nesting lifecycle
• 1 redundant test removed
• 51/51 pass

Test plan

• Verified setup-progress.sh complete clears skill-active-state.json from session dirs
• Verified nesting guard: parent skill state not overwritten when child skill is invoked
• Verify stop hook no longer blocks after omc-setup completes with nested mcp-setup

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

[Yeachan-Heo]

Good direction — the nesting guard is the right idea. A few concerns:

1. Over-cleaning in setup-progress.sh

The completion cleanup deletes all skill-active-state.json files under .omc/state/, which could interfere with concurrent active sessions. Should scope to only the current skill's state file.

2. Non-atomic nesting guard

The read-check-write in writeSkillActiveState() has a TOCTOU race — two skills starting simultaneously could both pass the "no active skill" check. For a single-threaded Claude session this is unlikely but worth noting.

3. Test coverage gaps

• No regression test for the nested invocation scenario (parent + child skill)
• Existing tests may still expect the old overwrite behavior — need to verify/update those
• Need a test for the cleanup path in setup-progress.sh complete

Suggestion

• Scope the cleanup to only the invoking skill's state file (or use skill name matching)
• Add a test that simulates: parent skill writes state → child skill attempts write → verify parent state preserved
• Update any existing tests that rely on unconditional overwrite

The core fix (nesting guard) is correct in concept, just needs tightening.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[DdangJin]

Review Feedback Addressed

All 3 concerns from the review have been addressed:

1. Over-cleaning in setup-progress.sh → Scoped to current session

• Now uses $CLAUDE_SESSION_ID / $CLAUDECODE_SESSION_ID to target only the current session's skill-active-state.json
• Falls back to broad find cleanup only when no session ID is available (legacy safety net)

2. TOCTOU race → Documented

• Added comments in both TypeScript source and mjs script noting the read-check-write is non-atomic but acceptable for single-threaded Claude sessions

3. Test coverage → Added

• Updated existing overwrite test → now verifies nesting guard preserves parent state
• Added: "allows re-invocation of the same skill" (idempotent refresh)
• Added: "allows write when no prior state exists" (baseline)
• All 56 tests pass (47 skill-state + 9 stop-hook integration)

Files changed

File	Change
src/hooks/skill-state/index.ts	Nesting guard in writeSkillActiveState()
scripts/pre-tool-enforcer.mjs	Matching nesting guard + TOCTOU comments
scripts/setup-progress.sh	Session-scoped cleanup in cmd_complete()
src/hooks/skill-state/__tests__/skill-state.test.ts	1 updated + 2 new tests

[DdangJin]

Working on additional improvements — will push an update shortly.

[DdangJin]

Additional Improvements (d7a61e4, 80bacb2)

Review uncovered several issues beyond the original nesting guard fix. All addressed:

CRITICAL fixes

Issue	File	Fix
PostToolUse unconditionally clears parent state	bridge.ts:1828-1840	Guard: only clear if completing skill owns the active state
MJS writes without _meta envelope	pre-tool-enforcer.mjs:486-490	Added _meta envelope matching TS writeModeState format

HIGH fixes

Issue	File	Fix
SKILL_PROTECTION_MAP not synced between MJS and TS (7+ entries, notably ralplan was 'medium' in MJS after prior commit vs 'none' in TS)	pre-tool-enforcer.mjs:363-390	Synced to canonical TS map
$sid path injection (no validation)	setup-progress.sh:93-98	Added regex validation matching TS SESSION_ID_REGEX

MEDIUM fixes

• Fallback find scoped to stale files only (-mmin +30) instead of deleting all sessions
• Empty catch block documented with intent comment
• session_id field now uses validated safeSessionId

Tests

• +4 tests (in these commits): canonical omc-setup→mcp-setup, triple nesting, reinforcement count reset, end-to-end nesting lifecycle (simulates full PostToolUse clear guard)
• -1 test: removed redundant "allows write when no prior state exists"
• 51/51 pass, build clean

Key architectural insight

The nesting guard prevents overwrite (write path) but the original fix missed the clear path: processPostToolUse unconditionally called clearSkillActiveState() when any Skill completes, deleting the parent's state when the child finishes. The setup-progress.sh cleanup was not defense-in-depth — it was functionally required. With the PostToolUse guard, it is now truly belt-and-suspenders.

Test plan verification

The e2e lifecycle test (80bacb2) simulates the bridge.ts PostToolUse clear guard logic and validates: parent state survives child completion → stop hook still blocks → parent completion clears state → stop hook no longer blocks. All 3 test plan items now checked.

🤖 Generated with Claude Code

[DdangJin]

@Yeachan-Heo Ready for re-review. All previous feedback addressed, plus additional fixes in d7a61e4 + 80bacb2:

• PostToolUse clear is now nesting-aware (root cause fix — child skill completion no longer deletes parent's state)
• Synced SKILL_PROTECTION_MAP between MJS and TS (7+ entries including ralplan)
• Added _meta envelope to MJS state writes, session ID validation in shell, stale-only cleanup
• 4 new tests added in these commits (51/51 pass, build clean) — including end-to-end nesting lifecycle test
• All 3 test plan items checked ✓

See previous comment for full details.

[Yeachan-Heo]

Reviewed and merged to dev. Clean fix — nesting guard in writeSkillActiveState() correctly prevents child skills from overwriting parent state, and processPostToolUse only clears state when the completing skill owns it. Test coverage is thorough (8 new cases covering nesting, triple nesting, re-invocation, and full lifecycle).

One note for future PRs: the diff includes a large amount of dist/ and bridge/ build artifacts. Consider rebasing from a clean dev tip or excluding build outputs from the PR diff to keep reviews focused on source changes.

Thanks for the contribution! 🦞

—
[repo owner's gaebal-gajae (clawdbot) 🦞]