Fix #35: Relax hook allowlist for reviewer and release smoke gaps

[tizerluo]

Summary

Fixes #35.

This PR fills the HOLO PreToolUse allowlist gaps found while preparing the v0.1.3 release PR #34. It is intentionally kept separate from #34 and should stay open until the real PreToolUse hook is reattached and validated against this branch build/dist.

Changes

• Allows trusted skill read-only inspection under $HOME/.codex/skills/** and $HOME/.agents/skills/** for normal skill protocol use.
• Allows limited reviewer discovery/help: which / command -v for known tools, claude --help, claude acp --help, and trusted wrapper --help.
• Keeps actual Claude/AGY dispatch constrained to trusted wrappers and read-only/review-safe flags.
• Allows agent-loop ... --help / pnpm agent-loop ... --help.
• Allows local release tarball smoke with non-dry-run npm pack only into safe system temp holo-* dirs and safe temp read checks.
• Adds explicit npm pack/install flag allowlists and rejects script-enabling/config/path flags.
• Keeps dangerous commands blocked: pipelines, sed -i, find -exec/-delete, non-skill HOME reads, npm publish, unsafe Claude/AGY wrapper args.

Validation

• pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts: passed, 18 tests.
• pnpm build:hooks: passed; pre-tool-use dist updated.
• pnpm lint: passed.
• pnpm test: passed, 36 files and 417 tests.
• npx gitnexus detect_changes --repo HOLO-Codex: completed, HIGH risk expected for hook policy flow; changed files are scoped to hook policy, pre-tool-use dist, and hook-policy tests.
• git diff --check: clean.

Internal review

• Internal tester: final PASS, no remaining P0/P1/P2/P3.
• Internal security reviewer: PASS, no remaining P0/P1/P2. One non-blocking P3 noted: trusted skill root still uses prefix trust; future hardening could restrict reads to SKILL.md, references/**, and scripts/*.mjs if needed.

Hold before merge

Do not merge yet. Next gate is real hook validation after the user reattaches the real PreToolUse hook:

1. pnpm agent-loop install-hooks --repo /Users/mac-mini/projects/HOLO-Codex --json
2. pnpm agent-loop hooks doctor --json
3. Positive smoke: read dispatch-claude-acp/SKILL.md, claude --help, claude acp --help, trusted Claude ACP dispatch, non-dry-run tarball pack/install smoke.
4. Negative smoke: pipeline, sed -i, non-skill HOME read, npm publish, unsafe Claude command.
5. Then run required Claude ACP review for this PR.

Run id: 6a10cd46-45ed-4b79-92d5-6b0dafcad15d.

[tizerluo]

Internal tester report

Result: PASS

P0/P1/P2/P3: none remaining.

Confirmed:

• Trusted skill reads are allowed; non-skill HOME reads are denied.
• which / command -v are limited to known tools.
• claude --help and claude acp --help are allowed; unsafe Claude commands are denied.
• Trusted Claude/AGY wrapper safe forms are allowed; agentCommand, agyCommand, unsafe transport, runDir/log/path overrides are denied.
• npm pack/install safe release smoke forms are allowed; --no-ignore-scripts=true, --cache, --script-shell, unsafe destinations, and package positionals are denied.
• rg -f, rg --file, jq -f, and similar file-loading bypasses are denied.

Validation seen:

• Focused hook-policy test passed, 18 tests.
• Full test passed, 36 files / 417 tests.

No blocking findings remain.

[tizerluo]

Owner delivery note

Run id: 6a10cd46-45ed-4b79-92d5-6b0dafcad15d

Scope:

• Fill HOLO PreToolUse allowlist gaps for trusted skill reads, reviewer discovery/help, safe reviewer dispatch, agent-loop help, and release tarball smoke.
• Keep destructive commands, arbitrary HOME reads, unsafe npm config/publish/token actions, and unsafe Claude/AGY dispatch blocked.

Hook doctor:

• Router installed and points to expected local HOLO dist.
• No legacy or unexpected router commands.
• Global agent-loop binary still points outside this package path; non-blocking for this PR.

Validation:

• pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts: passed, 18 tests.
• pnpm build:hooks: passed.
• pnpm lint: passed.
• pnpm test: passed, 36 files / 417 tests.
• npx gitnexus detect_changes --repo HOLO-Codex: completed, HIGH expected for hook policy flow; diff scoped to 3 files.

Hold:

• Do not merge until the user reattaches the real PreToolUse hook and we validate this branch build/dist against live hook behavior.

[tizerluo]

Internal security reviewer report

Result: PASS

P0/P1/P2: none remaining.

Resolved findings:

• Wrapper jump risk: fixed by explicit Claude/AGY wrapper flag allowlists.
• npm script bypass: fixed by exact --ignore-scripts and rejection of --ignore-scripts=false / --no-ignore-scripts*.
• npm config/path side effects: fixed by explicit npm pack/install flag allowlists.
• Temp read scope: narrowed to real /tmp/holo-* and macOS /var/folders/*/*/T/holo-* prefixes.
• rg / jq file-loading bypasses: denied.

P3 residual:

• Trusted skill reads still use prefix trust, so symlink escape is theoretically possible inside a trusted local skill root. Non-blocking for this PR; future hardening could restrict to SKILL.md, references/**, and scripts/*.mjs.

No unresolved P0/P1/P2 remains.

[tizerluo]

Live hook validation update

Run: 6a10cd46-45ed-4b79-92d5-6b0dafcad15d
Branch: codex/issue-35-hook-allowlist-review-release-gaps
Latest commit: 34c2638

Result

• ✅ Reinstalled hooks and confirmed doctor router points to this PR branch dist.
• ✅ Allowed positive paths: trusted skill read, claude --help, claude acp --help, agent-loop ... --help, safe temp mkdir -p /tmp/holo-*, npm pack --ignore-scripts --json --pack-destination /tmp/holo-*, and npm install --prefix /tmp/holo-* --ignore-scripts <tarball>.
• ✅ Blocked negative paths: shell pipeline, sed -i, non-skill HOME read, npm publish --dry-run, and unsafe Claude wrapper args.
• ✅ Live validation found one real reviewer-dispatch gap: --resume-session was blocked after a 529. Fixed in 34c2638 by allowing only UUID-valued resume sessions for the trusted Claude ACP wrapper, with tests and rebuilt hook dist.

Verification after follow-up

• ✅ pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts passed: 18 tests.
• ✅ pnpm build:hooks passed.
• ✅ pnpm lint passed.
• ✅ pnpm test passed: 36 files / 417 tests.
• ✅ npx gitnexus detect_changes --repo HOLO-Codex passed with expected hook-policy impact.

Claude ACP review status

• ⚠️ Required Claude ACP review was attempted via trusted wrapper and the hook allowed it.
• ACP session da0b684f-eab4-412d-89c6-ed33fe30cbac was created, but Claude returned no text before timeout and then hit provider error 529.
• Resume dispatch is now allowed and was verified, but the provider still produced no review output before timeout.

Current state: hook validation gate passed; Claude ACP review remains incomplete due provider availability, not hook policy.

[tizerluo]

Claude ACP code/security review

Reviewer: Claude ACP
Session: 12532a21-d3f6-4255-93b6-f55ecbe2717d
Model: sonnet
Conclusion: PASS

P0/P1/P2 findings

None.

P3 / follow-up

None identified.

Evidence checked

• Reviewed hook policy changes in plugins/autonomous-pr-loop/core/hook-policy.ts.
• Reviewed rebuilt plugins/autonomous-pr-loop/hooks/dist/pre-tool-use.js.
• Reviewed hook policy test coverage in plugins/autonomous-pr-loop/tests/hook-policy.test.ts.
• Confirmed scoped allowances for trusted skill reads, reviewer dispatch, safe temp mkdir, release smoke pack/install, and ACP resume UUID.
• Confirmed negative coverage for unsafe paths, shell control, npm publish, unsafe Claude wrapper args, and traversal/bypass cases.
• Cross-checked local verification: focused tests, pnpm build:hooks, pnpm lint, pnpm test, GitNexus detect, CI, and live hook validation.

Commander next step

No blockers. Proceed to merge when merge policy is satisfied.

[tizerluo]

Claude ACP re-review for latest delta

Reviewer: Claude ACP
Session: 6e6f4ff7-d6e0-4488-be05-035e9ee35555
Model: sonnet
Scope: latest commit e837aa4 only, gated PR merge cleanup allowlist.

Conclusion: PASS

P0/P1/P2 findings

None.

Notes

• Reviewed allowing long-form --delete-branch for gh pr merge while keeping lifecycle merge gate in place.
• Confirmed --admin, --auto, and short -d remain rejected.
• Classified this as a safe post-merge cleanup allowance because it only works after merge readiness / maintainer override policy is satisfied.

Latest CI: Node 22 and Node 24 passed.