Skip to content

Fix #28: Relax hook policy for normal delivery workflows - #29

Merged
tizerluo merged 5 commits into
mainfrom
codex/issue-28-relax-hook-policy
Jun 22, 2026
Merged

Fix #28: Relax hook policy for normal delivery workflows#29
tizerluo merged 5 commits into
mainfrom
codex/issue-28-relax-hook-policy

Conversation

@tizerluo

Copy link
Copy Markdown
Owner

Fixes #28. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Summary: relax hook policy for normal delivery workflows while tightening shell expansion, repo scope, reviewer dispatch, curl, npm, pnpm, git, and gh safety boundaries. Verification: pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts passed; pnpm lint passed; pnpm test passed 34 files and 395 tests; gitnexus detect_changes passed with expected HIGH hook-runtime impact. Internal tester PASS. Internal reviewer PASS after P0/P1/P2 fixes. Browser validation not required because no dashboard UI changes.

@tizerluo

Copy link
Copy Markdown
Owner Author

Delivery owner report for #28. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Hook doctor: router installed and current binding active; branch creation used recorded carryover fallback because old hook blocked git switch. Changes: quote-aware shell scanner, structured argv rawKind preserved, tiered allowlist for normal delivery, safe git gh npm pnpm agent-loop reviewer dispatch and localhost smoke commands, tightened destructive and data-exfiltration boundaries. Verification: pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts passed 18 tests; pnpm lint passed; pnpm test passed 34 files and 395 tests; git diff --check passed; gitnexus detect_changes passed with expected HIGH hook-runtime impact. Internal tester PASS. Internal reviewer PASS after all P0 P1 P2 findings were fixed. Browser validation not required because this PR does not touch dashboard UI.

@tizerluo

Copy link
Copy Markdown
Owner Author

Internal tester report. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Reviewer: internal_tester. Role: tester. Model: GPT-5.3-Codex-Spark. Conclusion: PASS. Coverage: checked hook-policy diff and ran focused hook-policy tests, full pnpm test, and pnpm lint. Findings: P0 none, P1 none, P2 none, P3 one tsx path traversal risk. Resolution: fixed in this PR by normalizing repo-owned tsx entrypoints and rejecting parent traversal, with regression coverage.

@tizerluo

Copy link
Copy Markdown
Owner Author

Internal reviewer report. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Reviewer: internal_reviewer. Role: code and security review. Model: GPT-5.5. Conclusion: PASS after fixes. Initial findings included shell command substitution bypass, spoofable reviewer dispatch path, gh write and GraphQL over-broad access, local path exfiltration, npm pnpm git and curl over-broad allowlists. Resolution: all P0 P1 P2 findings fixed in this PR with focused regression tests. Final review result: P0 none, P1 none, P2 none, P3 none.

@tizerluo

Copy link
Copy Markdown
Owner Author

Update after Claude ACP fixes. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Added stricter scope guards for curl allowlist, gh repo and hostname flags, git global options, env scope variables, npm and pnpm registry flags, package install specs, and reviewer dispatch path portability. Re-ran pnpm exec vitest run plugins/autonomous-pr-loop/tests/hook-policy.test.ts, pnpm lint, pnpm test, git diff --check, and gitnexus detect_changes. All passed. Browser validation remains not required because no UI changed.

@tizerluo

Copy link
Copy Markdown
Owner Author

Claude ACP review report. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. Reviewer: claude_acp. Sessions: e95413f9-0dac-4b3c-83b0-77c455eb9196, 1cca9cbc-15de-4153-9901-e0eced16f86b, bcded41e-8ccb-4d31-98c2-81823d3781a0. Initial conclusion: BLOCK with P1 findings for curl SSRF and file IO bypasses, gh repo/hostname scope, git global scope, env scope, npm registry scope. Fixes pushed in follow-up commits f4bdc0c and 33e4a89. Final narrow re-review conclusion: PASS. Remaining findings: P0 none, P1 none, P2 none, P3 none. Verification cited by reviewer: focused hook-policy test passed 18 of 18 and dist/pre-tool-use.js is synchronized with source.

@tizerluo

Copy link
Copy Markdown
Owner Author

CI follow-up. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. GitHub Actions failed because the hook-policy test used a local mac-mini HOME path for trusted skill wrappers. Fixed the test to derive trusted skill paths from process.env.HOME, preserving the same policy behavior while making CI portable. Local verification after this fix: focused hook-policy tests passed 18 of 18, pnpm lint passed, pnpm test passed 34 files and 395 tests, git diff --check passed, gitnexus detect_changes passed with LOW test-only impact.

@tizerluo

Copy link
Copy Markdown
Owner Author

Final hook-policy follow-up. Run id: 5da148c5-6e29-4121-87dd-ce74fc6542c4. While attempting the merge, local policy correctly required an explicit scoped gh merge command, but the merge-specific allowlist did not skip the already-validated repo flag. Fixed matchesGhPrMergeAllowlist to allow --repo, -R, --repo=, -R=, and attached -R forms only after matchesGhRepoScope has validated the current repo. Local verification passed: focused hook-policy tests, pnpm lint, pnpm test, git diff --check, and gitnexus detect_changes.

@tizerluo
tizerluo merged commit a552196 into main Jun 22, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

High priority: relax hook policy for normal delivery workflows

2 participants