feat(hook): implement proactive DIY detection and install vetting - #65
feat(hook): implement proactive DIY detection and install vetting#65kavin0x wants to merge 2 commits into
Conversation
Introduces a new PreToolUse hook that detects hand-rolled capability code during Write, Edit, and MultiEdit operations. The hook validates against known libraries and provides migration guidance when DIY patterns are detected. Additionally, it integrates with the `starlog_advise` tool to recommend vetted libraries, enhancing the safety and reliability of package usage. Updates to documentation reflect these changes, emphasizing the importance of vetting packages before adoption.
|
@claude Could you review this please. Give the go ahead to merge this. |
basicScandal
left a comment
There was a problem hiding this comment.
Thanks for this — the direction (proactively steering agents toward vetted libraries at write-time) is genuinely valuable, and the corpus/policy plumbing is nicely factored. Because this hook runs on the user's machine and can gate Write/Edit/MultiEdit, I went through it with a correctness + fail-open lens. There are three issues I'd consider blocking before merge, plus a few mediums.
🔴 Blockers
1. Advisory path emits additionalContext on PreToolUse — which Claude Code ignores
src/hook-output.ts (emitPreToolUse) / src/diy-hook-runner.ts (advisory branch)
The advisory branch emits:
{ "hookSpecificOutput": { "hookEventName": "PreToolUse", "additionalContext": "..." } }Per the current hooks reference, PreToolUse hookSpecificOutput supports only permissionDecision, permissionDecisionReason, and updatedInput. additionalContext is PostToolUse-only (this was requested for PreToolUse in anthropics/claude-code#15664 but isn't shipped there yet).
Impact: the advisory (non-deny) path is 100% of normal invocations and the whole point of the feature — but the agent never sees the guidance. runAdvise runs (with its scan/network cost), the JSON is written, and nothing surfaces.
Fix: surface guidance via permissionDecision: "ask" + permissionDecisionReason (both supported on PreToolUse), and add an end-to-end shell-protocol test asserting the field actually reaches the agent.
2. Org deny policy silently bypassed on any I/O error
src/diy-hook-runner.ts (handleDiyPreToolUse)
The deny branch runs runAdvise(...) + buildAdvisoryMessage(...) (project scan + corpus lookups + optional resolveFactView network call) before it emits the deny decision — all inside the outer try/catch in runDiy() that exits 0 on throw. So when the user is offline, the corpus dir is missing, or a facts lookup rejects, an org that explicitly set diy_category: deny gets silent non-enforcement (exit 0, no output). The deny test mocks runAdvise to always resolve, so this ordering is never exercised.
Fix: when diyPolicy.decision === 'deny', emit the deny decision first (sparse reason ok), then enrich the reason in a separate try/catch so failure degrades to a terse deny — never to silent allow.
3. Project-level Cursor/Copilot hooks are registered as a bare .js path and won't execute
src/install/project-hooks.ts (buildCursorHooksConfig / buildCopilotHooksConfig)
Entries are written as "command": ".cursor/hooks/starlog-pkg-check.js" — a bare path with no node prefix, and atomicWrite doesn't set the +x bit, so the shebang alone won't save it. The Claude global hook is correctly registered as node "${HOOK_PATH}" in init.ts; the project variants should match (absolute, quoted, node-prefixed) or chmod 0755 the script after writing.
🟠 Medium
- Absolute path used as
relPath—extractWritePayloadpassestoolInput.file_path(absolute) intoscoreFileForHook, whose patterns like/auth/i,/redis/,/socket/then match the project directory name. Anyone working in~/auth-service/gets auth advisories on every write. Relativize againstcwdfirst (asscanProjectdoes). ws/WebSocketflagged as DIY —src/patterns/detect.ts:ws(the vetted npm package) and nativeWebSocketare in realtimeimportPatternsbut notknownLibPatterns, so legitimateimport { WebSocketServer } from 'ws'scores as high-confidence DIY. Add them toknownLibPatterns/ narrow/\bws\b/.detectHookPlatformnever returns'claude'—src/hook-output.ts: Claude Code's PascalCasePreToolUsefalls through to the'copilot'branch. Works today only because the JSON shapes coincide; theHookPlatformtype is misleading and fragile to any future divergence.- Latency note —
runAdvise(full project scan + search + optional network) sits on the critical path of every qualifying write. The debounce helps, but the first hit per category per window pays full cost. Worth calling out explicitly as a latency/richness tradeoff.
🟡 Tests
The suite never exercises the fail-open property, and runDiy() (the stdin/exit-0 entrypoint) isn't tested at the subprocess level the way the PostToolUse path is. Suggested additions: (1) runAdvise rejects → assert no output + no throw; (2) runAdvise rejects with a deny policy active → assert the tool is still denied (after fix #2); (3) a runDiy() subprocess test.
Happy to look again once the three blockers are addressed — the corpus/policy work underneath is solid.
Refines the PreToolUse hook to improve detection of hand-rolled capability code during Write, Edit, and MultiEdit operations. The hook now surfaces migration candidates and guidance through a structured output, including `permissionDecision: "ask"` and `permissionDecisionReason`. It ensures that denial of operations adheres to organizational policies without fail-open scenarios. Updates to the documentation and tests reflect these enhancements, ensuring clarity on the new advisory mechanisms and their integration with existing tools.
|
@basicScandal I have made the requested changes, could you please re-review this PR |
Introduces a new PreToolUse hook that detects hand-rolled capability code during Write, Edit, and MultiEdit operations. The hook validates against known libraries and provides migration guidance when DIY patterns are detected. Additionally, it integrates with the
starlog_advisetool to recommend vetted libraries, enhancing the safety and reliability of package usage. Updates to documentation reflect these changes, emphasizing the importance of vetting packages before adoption.