Support non-bash hook interpreters for pack hooks - #368
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Interpreter trust tracking, command validation, cleanup ownership, and export discovery contain unresolved security and correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable and inferred hook interpreters across synchronization, validation, export, doctor checks, and documentation.
Changes:
- Adds interpreter resolution, validation, command parsing, and manifest support.
- Updates sync cleanup, export, diagnostics, and pack heuristics.
- Adds unit and lifecycle coverage plus schema documentation.
File summaries
| File | Description |
|---|---|
Sources/mcs/TechPack/HookInterpreter.swift |
Implements interpreter handling. |
Sources/mcs/TechPack/Component.swift |
Composes interpreter-aware hook commands. |
Sources/mcs/Sync/SyncStrategy.swift |
Displays non-default interpreters. |
Sources/mcs/Sync/SyncScope.swift |
Separates hook directories from interpreters. |
Sources/mcs/Sync/ProjectSyncStrategy.swift |
Uses interpreter-aware project commands. |
Sources/mcs/Sync/GlobalSyncStrategy.swift |
Updates global composition and cleanup. |
Sources/mcs/Sync/ConfiguratorSupport.swift |
Composes and reports hook commands. |
Sources/mcs/Sync/Configurator.swift |
Removes interpreter-aware artifacts. |
Sources/mcs/ExternalPack/PackHeuristics.swift |
Adds hook runtime heuristics. |
Sources/mcs/ExternalPack/ExternalPackManifest.swift |
Adds manifest decoding and validation. |
Sources/mcs/ExternalPack/ExternalPackLoader.swift |
Sanitizes values and improves errors. |
Sources/mcs/Export/ManifestBuilder.swift |
Exports interpreter metadata. |
Sources/mcs/Export/ConfigurationDiscovery.swift |
Recovers interpreters from settings. |
Sources/mcs/Doctor/DoctorRunner.swift |
Adds interpreter checks. |
Sources/mcs/Doctor/CoreDoctorChecks.swift |
Verifies interpreter resolution. |
Sources/mcs/Core/ShellRunner.swift |
Returns resolved executable paths. |
Sources/mcs/Core/Constants.swift |
Defines hook command components. |
Tests/MCSTests/PackHeuristicsTests.swift |
Tests runtime heuristics. |
Tests/MCSTests/ManifestBuilderTests.swift |
Tests export round-tripping. |
Tests/MCSTests/LifecycleIntegrationTests.swift |
Tests hook lifecycle behavior. |
Tests/MCSTests/HookInterpreterTests.swift |
Tests interpreter utilities. |
Tests/MCSTests/GlobalSyncTests.swift |
Tests global cleanup. |
Tests/MCSTests/ExternalPackManifestTests.swift |
Tests manifest behavior. |
Tests/MCSTests/CoreDoctorCheckTests.swift |
Tests interpreter diagnostics. |
Tests/MCSTests/ConfigurationDiscoveryTests.swift |
Tests interpreter discovery. |
Tests/MCSTests/ComponentTests.swift |
Tests command composition. |
skills/techpack-creator/SKILL.md |
Documents interpreter authoring. |
skills/techpack-creator/references/techpack-schema.md |
Extends the skill schema. |
skills/techpack-creator/references/stack-detection.md |
Updates hook guidance. |
skills/techpack-creator/references/examples/node-web.yaml |
Adds a JavaScript hook example. |
docs/troubleshooting.md |
Adds runtime troubleshooting. |
docs/techpack-schema.md |
Documents the public schema. |
docs/creating-tech-packs.md |
Updates pack creation guidance. |
docs/architecture.md |
Describes interpreter architecture. |
CLAUDE.md |
Updates repository guidance. |
Review details
- Files reviewed: 35/35 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Resolve a hook's interpreter from `hookInterpreter` or the file extension, defaulting to bash - Recognise managed hook entries by path token so non-bash hooks are cleaned up on removal - Verify interpreter binaries in doctor, and warn at pack-validate on undeclared runtimes
- Reject control characters in `hookInterpreter`, which split away as token whitespace but survive into the command as a shell separator - Bring a non-default interpreter into the pack trust surface, so changing it forces renewed trust even when the script is unchanged - Discover hooks in namespaced subdirectories on export, look through `env` when verifying a binary, and stop two heuristics producing false warnings
a96f674 to
3afccc5
Compare
- Replace the blanket no-amend/no-force-push rules with the actual preference: rebase onto main for linear history, `--force-with-lease` on your own branch - Note that CI takes Homebrew's latest while a local install from another manager can shadow it, and name the stale-install symptom
There was a problem hiding this comment.
🟡 Changes recommended
Trust downgrade handling and several validation/export edge cases remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Sources/mcs/Export/ConfigurationDiscovery.swift:323
- Discarding the recursive relative path makes hook correlation ambiguous:
discoverFileslater usescommand.contains(filename), sogate.tscan pick up the event/interpreter forpre-gate.tsdepending on dictionary order. Settings commands reference the full namespaced path, not merely the basename. Preserve each path relative tohooksDirand match the exact managed path token before flattening the exported destination.
/// A flat listing misses every hook mcs itself placed: `DestinationCollisionResolver` always
/// namespaces hooks, so a synced hook never sits at the top level. Files are returned by
/// basename, which is what a settings command references and what the exported manifest uses
/// as its destination — so a basename appearing twice is reported and skipped rather than
- Files reviewed: 37/37 changed files
- Comments generated: 6
- Review effort level: Balanced
- Give the manifest model one hook-eligibility rule so validation and trust stop reporting hooks sync never registers - Detect an interpreter being dropped back to bash, and key interpreter trust by component ID rather than display name - Correlate exported hooks on the full managed path, and compare asserted interpreters instead of prefix-testing for bash
|
Addressed the suppressed finding too ( Correlation now matches the full managed path token instead of Worth recording what surfaced while fixing it: my first attempt derived the relative path by subtracting |
- Resolve the LifecycleIntegrationTests conflict by keeping both appended suites: HookInterpreterLifecycleTests from #368 and ScopeDuplicationCheckTests - Reword the duplication rationale for the interpreter change: only the hook directory is scope-dependent, so the two entries are still distinct Claude-Session: https://claude.ai/code/session_01MkBJdGBUoZ2aRtchjaKXMN
Summary
Pack hooks could only run under bash. The registered command was hardcoded to
bash <path>, so a pack shipping a.jsor.tshook had to either add a wrapper script thatexecs the real interpreter, or hand-write the settings entry itself and give up doctor verification. Packs can now declare the interpreter, or let the file extension imply it.Changes
hookInterpreter— a bare command, an absolute path, or a command with arguments such asnode --experimental-strip-types --disable-warning=ExperimentalWarning. Left unset, the interpreter follows the file extension (.js→ node,.py→ python3,.shand extensionless → bash). TypeScript infers nothing on purpose, since node's flags,tsx,bunanddenoare all plausible and guessing wrong produces a hook that dies at runtime.bashwere recognised as ours, so a non-bash entry survived unconfigure and kept firing at a deleted script. This also covers entries carrying trailing arguments, which the old prefix match missed.mcs doctorchecks that each pack's interpreter binary resolves, and warns when it resolves only through a version manager — a path that works in your terminal but often not in the environment Claude Code hands its hooks.mcs pack validatewarns when no brew component installs the runtime, when a TypeScript hook declares no interpreter, and when ahookEventExistscheck asserts an interpreter its own component doesn't use.mcs exportcarries a non-default interpreter back into the generated manifest rather than re-emitting the hook under bash. Manifest errors raised during decoding — an unknownhookEvent, or hook metadata withouthookEvent— now surface their actual explanation instead of Foundation's generic "the data couldn't be read".Test plan
swift testpasses locallyswiftformat --lint .andswiftlintpass without violationsmcs sync,mcs doctor)mcs pack validatewas exercised against real scratch packs;mcs syncandmcs doctorwere covered by sandboxed integration tests rather than a live run, becauseNSHomeDirectory()ignores$HOMEand a live run would write the developer's own~/.claudeand~/.mcs. Worth a live pass before merge.To verify by hand:
.tswithhookInterpreter: node --experimental-strip-types, one.jswith no interpreter, one.sh— then runmcs sync→ expect three entries in.claude/settings.local.jsonreadingnode --experimental-strip-types …/gate.ts,node …/fmt.jsandbash …/legacy.sh.mcs doctor→ expect one interpreter check per distinct binary, passing where the runtime is installed.mcs sync→ expect all three entries and their files gone.--globaland confirm the non-bash entries are gone from~/.claude/settings.json; this is the case that previously orphaned..tshook write a marker file and start a Claude Code session → expect the marker. An empty hook log is indistinguishable from success, so a side effect is the only real proof it fires.Checklist for engine changes
fix()implementation does cleanup/migration only — never installs or registers resourcesLifecycleIntegrationTestsorDoctorRunnerIntegrationTests)CLAUDE.md,docs/,techpack.yamlschema inExternalPackManifest.swift)