fix: distinguish tamper timeouts and pytest launchers - #2946
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change validates direct ChangesPytest runtime and timeout classification
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Workflow source neededPR #2946 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
There was a problem hiding this comment.
Pull request overview
This PR tightens the deliberate-break checker’s classification and runtime safety checks by (1) reporting tamper-inspection timeouts with a distinct reason code and (2) being more selective about when a bare pytest launcher is considered “managed” (only when its shebang resolves to the active interpreter and does not alter import-context flags). It also mirrors the behavior into the consumer template and adds regression tests to lock in the updated contract.
Changes:
- Report
subprocess.TimeoutExpiredduring the tamper (git diff/assertion) inspection phase astamper-check-timeout(while preservingcommand-timeoutfor the actual command execution path). - Treat a plain
pytest ...invocation as managed only when the resolved shebang launcher matchessys.executableand does not change Python import context. - Add regression tests for the new timeout reason and for managed/unmanaged
pytestshebang behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/check_deliberate_break.py |
Adds stricter managed-runtime detection for bare pytest based on shebang resolution; classifies tamper inspection timeouts as tamper-check-timeout. |
templates/consumer-repo/scripts/check_deliberate_break.py |
Mirrors the same checker logic updates into the consumer template to prevent drift. |
tests/scripts/test_check_deliberate_break.py |
Adds focused regression coverage for tamper timeout reason and for managed/unmanaged pytest shebang variants. |
Automated Status SummaryHead SHA: 0d24f7a
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
Summary
tamper-check-timeoutinstead of command execution timeoutspytestlauncher as safely managed only when its shebang resolves to the active interpreter without import-context flagsValidation
188 passed, 3 skippedacross checker, sync dependency, and template-drift coverage123 passedin the focused checker suite after formattinggit diff --checkpassReview lineage
Addresses exact-head review findings on learning-management-system #513 and trip-planner #1635. Ready #503 was resolved with evidence because
EIPsSalready contains uppercaseP. The consumer fleet remains held pending corrected propagation.Summary by CodeRabbit
Bug Fixes
tamper-check-timeoutstatus.Tests