Repository navigation
feat(hooks): task-boundary test discovery hook [OMN-7261] - #1504
Conversation
Task-boundary test hook fires on git commit / gh pr create, discovers relevant tests for staged Python files, and runs them with a 120s budget. Blocks on test failure; fails open on timeout/infra errors. Opt-in via TASK_BOUNDARY_TESTS bit in ONEX_HOOKS_MASK (default: disabled until user enables it). Files: - lib/task_boundary_tests.py: test discovery + execution logic - scripts/pre_tool_use_task_boundary_tests.sh: shell hook wrapper - hooks.json: registered under PreToolUse with Bash matcher - lib/hook_bits.sh: regenerated with TASK_BOUNDARY_TESTS bit 58 - tests/hooks/test_task_boundary_tests.py: 37 unit tests
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a PreToolUse Bash hook that runs staged-Python-file test discovery and pytest (120s) before ChangesTask Boundary Tests Hook
Sequence DiagramsequenceDiagram
actor User
participant Bash as "Bash (tool)"
participant HookGate as "PreToolUse Hook Gate"
participant Script as "pre_tool_use_task_boundary_tests.sh"
participant Python as "task_boundary_tests.py"
participant Git as "git"
participant Pytest as "pytest"
User->>Bash: run git commit / gh pr create
Bash->>HookGate: invoke PreToolUse hooks (stdin JSON)
HookGate->>Script: execute pre_tool_use_task_boundary_tests.sh
Script->>Script: parse stdin, check TASK_BOUNDARY_TESTS mask
alt Gate enabled & tool == Bash & trigger command
Script->>Python: run task_boundary_tests.py (stdin payload)
Python->>Git: locate repo root (.git)
Git-->>Python: repo root path
Python->>Git: list staged *.py files
Git-->>Python: staged files
Python->>Python: map changed files → candidate tests under tests/
Python->>Pytest: run discovered tests (timeout 120s)
Pytest-->>Python: results (pass/fail/timeout)
alt Tests pass or no applicable tests
Python-->>Script: exit 0 (allow)
Script-->>HookGate: print original payload and exit 0
else Tests fail
Python-->>Script: print JSON {"decision":"block"} and exit 2
Script-->>HookGate: exit 2 (block)
end
else Gate disabled or non-trigger
Script-->>HookGate: passthrough original payload and exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical/error findings | No |
blocked |
CRITICAL or ERROR findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)
…oundary-test-hooks-local
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/hooks/test_task_boundary_tests.py (1)
46-50: ⚡ Quick winAdd regressions for the missing edge cases.
The suite covers the happy path, but it still misses
gh pr createafter;and the case where a failed run should not start the debounce window. Those are the two regressions most likely to slip through here.Also applies to: 252-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/test_task_boundary_tests.py` around lines 46 - 50, Add two regression tests in tests/hooks/test_task_boundary_tests.py: one that asserts tbt._is_trigger_command returns True for a command with a preceding semicolon (e.g., "...; gh pr create --title 'x'") to cover the semicolon edge-case, and another that verifies a failed run does not start the debounce window by ensuring the task-boundary tracker (use the same test helper that simulates runs in this suite) does not update its last-start/debounce state after a run that is marked failed; refer to the existing test helpers and tbt._is_trigger_command and the test names pattern (e.g., test_gh_pr_create_with_semicolon, test_failed_run_does_not_start_debounce) when adding these cases so they mirror the other tests in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/onex/hooks/lib/hook_bits.sh`:
- Line 7: The default hook bits mask HOOK_BITS_DEFAULT_MASK was widened to
include the TASK_BOUNDARY_TESTS bit which makes that hook opt-out instead of
opt-in; change HOOK_BITS_DEFAULT_MASK so it does not contain the
TASK_BOUNDARY_TESTS bit (restore the previous mask value or explicitly remove
the TASK_BOUNDARY_TESTS bit from the constant) and leave TASK_BOUNDARY_TESTS
only enabled when explicitly added by consumers.
In `@plugins/onex/hooks/lib/task_boundary_tests.py`:
- Around line 31-36: The regex _RE_GH_PR_CREATE used by _is_trigger_command
doesn't treat ';' as a command separator so constructs like "echo hi; gh pr
create" won't match; update the pattern for _RE_GH_PR_CREATE to include ';' (and
other shell separators if desired, e.g., '||') as alternatives (mirror what
_RE_GIT_COMMIT does) so that _is_trigger_command correctly detects "gh pr
create" following a semicolon.
- Around line 166-178: The debounce marker is being touched before running
tests, which starts the cooldown even on failures; move the call to
_touch_debounce(debounce_key) so it only executes after tests pass: call
_run_tests(repo_root, tests), check the returned passed flag, and if passed is
True then call _touch_debounce(debounce_key) (only after this success path and
before any json.dump/sys.exit that indicates success); do not touch the debounce
key on the early exits or when passed is False.
- Around line 112-126: The subprocess.run call in task_boundary_tests.py is
passing the pytest CLI flag "--timeout=120" but the project lacks the
pytest-timeout plugin; add "pytest-timeout" to the dev dependencies in
pyproject.toml (dev dependency group) so pytest recognizes that flag, and mirror
the same addition for any CI or hook scripts that invoke pytest with --timeout
(e.g., the invocation in plugins/onex/hooks/scripts/epic_postaction_gate.sh), or
alternatively remove the "--timeout=120" argument and rely solely on the
subprocess.run timeout controlled by TEST_TIMEOUT_SECONDS.
In `@plugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.sh`:
- Around line 34-57: The script can return early and leave stdin buffered;
ensure you drain stdin before any non-standard exit by adding a stdin drain
(e.g., read/discard all input) both after the failed source of onex-paths.sh
(the block that currently does: source ".../onex-paths.sh" || { echo ...; exit
1; }) and immediately before the early passthrough return triggered by
onex_hook_gate TASK_BOUNDARY_TESTS || exit 0 so the hook preserves the stdin
passthrough contract; update those locations to discard stdin prior to exit and
then exit 0.
---
Nitpick comments:
In `@tests/hooks/test_task_boundary_tests.py`:
- Around line 46-50: Add two regression tests in
tests/hooks/test_task_boundary_tests.py: one that asserts
tbt._is_trigger_command returns True for a command with a preceding semicolon
(e.g., "...; gh pr create --title 'x'") to cover the semicolon edge-case, and
another that verifies a failed run does not start the debounce window by
ensuring the task-boundary tracker (use the same test helper that simulates runs
in this suite) does not update its last-start/debounce state after a run that is
marked failed; refer to the existing test helpers and tbt._is_trigger_command
and the test names pattern (e.g., test_gh_pr_create_with_semicolon,
test_failed_run_does_not_start_debounce) when adding these cases so they mirror
the other tests in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 062de5ba-c00b-468e-ad97-ff94fdb7f591
📒 Files selected for processing (5)
plugins/onex/hooks/hooks.jsonplugins/onex/hooks/lib/hook_bits.shplugins/onex/hooks/lib/task_boundary_tests.pyplugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.shtests/hooks/test_task_boundary_tests.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.sh`:
- Around line 99-104: The grep patterns checking COMMAND in the
pre_tool_use_task_boundary_tests.sh hook are inconsistent: one pattern uses \|\|
(matching only "||") while the other uses \| (matching only "|"), so pipe
contexts like "foo | git commit" or "foo || gh pr create" are missed; update
both regexes that reference COMMAND (the lines that match 'git\s+commit' and
'gh\s+pr\s+create') to accept one or more pipe characters (e.g., use a
quantifier like \|+ or an alternation covering both) in the
'(^|\s|&&|\|\||;|...)' portion so both single and double pipes are matched
consistently. Ensure the anchors remain the same and only the pipe token portion
is changed so COMMAND detection continues to work for both "|" and "||".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c8336d3-da9f-48e6-850f-ecb9df2a1459
📒 Files selected for processing (3)
plugins/onex/hooks/lib/task_boundary_tests.pyplugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.shtests/hooks/test_task_boundary_tests.py
✅ Files skipped from review due to trivial changes (1)
- plugins/onex/hooks/lib/task_boundary_tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hooks/test_task_boundary_tests.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.sh`:
- Line 81: Guard the unprotected source of common.sh so failures don't cause the
script to exit under set -e: before sourcing "${HOOKS_DIR}/scripts/common.sh"
check that the file exists and is readable (e.g., test -f/-r) and only source it
inside that conditional; if the file is missing or not readable, emit a
non-fatal warning message and continue so the hook remains fail-open—do not
allow errors from sourcing to propagate (use a conditional source or append a
no-fail fallback) and update the reference at the existing source line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 842d18d9-5f6d-4cd9-840a-d74f7a45dcb0
📒 Files selected for processing (3)
plugins/onex/hooks/lib/task_boundary_tests.pyplugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.shtests/hooks/test_task_boundary_tests.py
✅ Files skipped from review due to trivial changes (1)
- tests/hooks/test_task_boundary_tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/onex/hooks/lib/task_boundary_tests.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.sh`:
- Around line 46-54: The hook-bit loading/parsing must fail-open: guard sourcing
HOOK_BITS_PATH (check file existence before calling source) so source
"$HOOK_BITS_PATH" cannot cause set -e to abort, and make hook_bits_parse_mask
invoked in a way that returns an empty TASK_BOUNDARY_MASK on error (e.g. capture
its output with a fallback like || true) so TASK_BOUNDARY_MASK is never left as
a failing command; then ensure hook_bits_is_enabled is only called when
TASK_BOUNDARY_BIT and TASK_BOUNDARY_MASK are non-empty (use TASK_BOUNDARY_BIT
from hook_bits_bit_for_name and TASK_BOUNDARY_MASK from hook_bits_parse_mask) so
the script always exits 0 unless you intentionally block.
- Around line 27-30: Wrap the path-bootstrap sequence that sets _SELF,
SCRIPT_DIR and PLUGIN_ROOT with a fail-open guard so any error in the bootstrap
does not exit non-zero: if the realpath/cd resolution fails, drain stdin (e.g.
run cat to consume input) and then exit 0. Concretely, ensure the compound
assignment for _SELF, the SCRIPT_DIR calculation, and PLUGIN_ROOT expansion are
guarded as a unit (referencing _SELF, SCRIPT_DIR, PLUGIN_ROOT) and on failure
run cat >/dev/null (to drain stdin) then exit 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e483dacf-b048-4fed-a6a9-829270088caa
📒 Files selected for processing (2)
plugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.shtests/hooks/test_task_boundary_tests.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hooks/test_task_boundary_tests.py
Summary
pre_tool_use_task_boundary_tests.shfires ongit commit/gh pr createtests/test_<module>.py,tests/<path>/test_*.py)--timeout=120, 120s budgetTASK_BOUNDARY_TESTSbit 58 inONEX_HOOKS_MASK— disabled by defaultFiles changed
plugins/onex/hooks/lib/task_boundary_tests.pyplugins/onex/hooks/scripts/pre_tool_use_task_boundary_tests.shplugins/onex/hooks/hooks.jsonplugins/onex/hooks/lib/hook_bits.shtests/hooks/test_task_boundary_tests.pyVerification
Linked
Evidence-Source: OCC#669
Summary by CodeRabbit
New Features
Chores
Tests
Evidence-Ticket: OMN-7261