Repository navigation
fix(reorg): unbreak main — flake_path pin, workflow entrypoints, fail-closed review-gate - #154
Conversation
…, close the review-gate fail-open bb1b968 kept #152 reorg-only but carried four unlanded breakages. This lands them. 1. school-loop.yml invoked five moved modules by their PRE-MOVE root paths, so the workflow was broken silently (it does not run on PRs): :149 from school_mail import ... -> from src.tools.school_mail import ... :374 python orca_precondition.py -> python src/crew/orca_precondition.py :405 python gateway_preflight.py -> python src/tools/gateway_preflight.py :424 python worktree_bloat_guard.py -> python src/tools/worktree_bloat_guard.py :495 python crew_ledger_reconcile.py -> python src/crew/crew_ledger_reconcile.py Verified: all five targets exist on disk; no bare root-path invocation remains. 2. ci.yml:293 imported school_mail by its pre-move path -> src.tools.school_mail. 3. tests/test_review_build_gate.py:170 asserted the PRE-REORG value (Path(director.__file__).resolve().parent). director.py now lives at src/core/director.py, so its package dir is src/core, which has no flake.nix -- the verify gate would silently skip its flake check. Rewritten to pin the repo root: asserts (flake_dir / "flake.nix").exists() and that flake_dir is NOT the module's own package dir. Verified: 7 passed in 0.28s (was RED on bb1b968). 4. review-gate fail-open (the PR's only REQUIRED check). ci.yml passed --head "origin/${{ github.event.pull_request.head.sha }}" -- not a ref name. git diff failed on stderr, the error was discarded, and the gate read stdout only, so it reported 0 changed files and PASSed on every PR since #117. - scripts/review_gate.py: git() now raises SystemExit on non-zero exit; new resolve_ref() verifies each ref with rev-parse before diffing; an empty diff is now a FAILURE, not a pass. - ci.yml: both invocations now pass --head "HEAD". Verified RED->GREEN: unresolvable ref exit 1 (was exit 0 + silent PASS); empty diff exit 1 [empty-diff]; real head exit 0; --head HEAD exit 0. tests/test_review_gate.py: +4 regression tests, 15 passed in 7.00s. NOT included here (deliberately): the behaviour batch -- conductor.py, issue_bridge.py, tests/test_conductor_issue_path.py, tests/test_issue_bridge.py -- which belongs on school/1nzr-behaviour per bb1b968's split. Also NOT fixed: ci.yml's ruff CORE= list still names onboard.py and a2a_bridge.py, which no longer exist. Harmless -- ruff warns and exits 0 -- but a silent scope shrink; tracked separately.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Reviewer's GuideThis PR repairs the post-reorganization CI and workflow breakages by correcting moved-module paths, pinning build verification to the repository root, and replacing the review gate’s fail-open ref/diff handling with explicit validation and failure behavior. Sequence diagram for the fail-closed review gatesequenceDiagram
participant CI
participant Gate as review_gate.py
participant Git
CI->>Gate: main(--base origin/main, --head HEAD)
Gate->>Git: rev-parse --verify base^{commit}
Git-->>Gate: base commit
Gate->>Git: rev-parse --verify head^{commit}
Git-->>Gate: head commit
Gate->>Git: diff base...head
Git-->>Gate: changed files and added lines
Gate-->>CI: PASS or mechanical-check failures
alt ref cannot be resolved
Git-->>Gate: non-zero exit
Gate-->>CI: FAIL with git/ref error
else no changed files
Gate-->>CI: FAIL with empty-diff error
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| - name: Populate issue cache from GitHub | ||
| run: .venv/bin/python populate_board_cache.py | ||
| run: .venv/bin/python src/tools/populate_board_cache.py |
There was a problem hiding this comment.
Both cache steps run python src/tools/populate_board_cache.py, which puts src/tools, not the checkout root, on Python's import path. The script then fails at from src.tools.repo_default import default_repo with ModuleNotFoundError: No module named 'src'. Neither job installs the project or sets PYTHONPATH, so later steps that execute issues or publish the board are skipped.
Use python -m src.tools.populate_board_cache in both steps, keeping .venv/bin/python in this job.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/school-loop.yml
Line: 462
Comment:
**Cache refresh still fails**
Both cache steps run `python src/tools/populate_board_cache.py`, which puts `src/tools`, not the checkout root, on Python's import path. The script then fails at `from src.tools.repo_default import default_repo` with `ModuleNotFoundError: No module named 'src'`. Neither job installs the project or sets `PYTHONPATH`, so later steps that execute issues or publish the board are skipped.
Use `python -m src.tools.populate_board_cache` in both steps, keeping `.venv/bin/python` in this job.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| assert (flake_dir / "flake.nix").exists(), ( | ||
| f"flake_path {flake_dir} does not contain flake.nix — " | ||
| f"the verify gate would silently skip its flake check" | ||
| ) | ||
| # And it must NOT be the module's own package directory (cwd-dependence). | ||
| assert flake_dir != Path(director.__file__).resolve().parent |
There was a problem hiding this comment.
The replacement assertions no longer prove that flake_path points to the checkout containing director.py. A regression to Path.cwd() passes when tests run from the repository root: that directory contains flake.nix and differs from src/core. This leaves the original working-directory bug unguarded.
Keep an exact comparison with the root derived from director.__file__, alongside the existence check. Changing to an unrelated directory during the test would also exercise the behavior its docstring promises.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_review_build_gate.py
Line: 175-180
Comment:
**Checkout pin loses coverage**
The replacement assertions no longer prove that `flake_path` points to the checkout containing `director.py`. A regression to `Path.cwd()` passes when tests run from the repository root: that directory contains `flake.nix` and differs from `src/core`. This leaves the original working-directory bug unguarded.
Keep an exact comparison with the root derived from `director.__file__`, alongside the existence check. Changing to an unrelated directory during the test would also exercise the behavior its docstring promises.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
luna-whymage
left a comment
luna-whymage
left a comment
There was a problem hiding this comment.
APPROVE.
All three pytest legs are green (py3.9 4m21s, py3.11 3m56s, py3.12 4m15s) — including tests/test_review_build_gate.py::test_build_verify_pins_flake_to_director_checkout, which is RED on main and on school/1nzr-behaviour. This PR is the fix for both.
Verified locally on top of main (5a1fdb3):
tests/test_review_build_gate.py— 7 passed in 0.28stests/test_review_gate.py— 15 passed in 4.36sreview_gate.py --base origin/main --head HEAD— 5 changed files, 118 added lines, PASS. The same invocation reported0 changed file(s)and PASSed before the fail-closed fix, which is whymainwent red without the required check noticing.
Scope check: this PR touches exactly the four reorg-fallout items — the flake_path repo-root pin, the five school-loop.yml entrypoint repoints, the ci.yml school_mail import, and the review-gate fail-closed. The behaviour batch is correctly excluded; it belongs on school/1nzr-behaviour.
Non-blocking note: ci.yml's ruff CORE= list still names onboard.py and a2a_bridge.py, which no longer exist. Harmless — ruff warns and exits 0 — but a silent scope shrink, tracked separately.
2 participants
What
Follow-up to #152 (merged as
166b490). The reorg left four breakages sitting uncommitted in the working tree, so they never made it into the merge.mainis red because of one of them.Closes the red CI on
mainand onschool/1nzr-behaviour(both fail on the same test).The four breakages
1.
mainis RED — one failure class, all three legsThe test asserts the pre-reorg value,
Path(director.__file__).resolve().parent.director.pynow lives atsrc/core/director.py, so its package dir issrc/core— which has noflake.nix. The verify gate would silently skip its flake check.Rewritten to pin the repo root: asserts
(flake_dir / "flake.nix").exists()and thatflake_diris not the module's own package dir (the cwd-dependence the test was originally guarding against).2.
school-loop.ymlinvoked five moved modules by pre-move root pathsThe workflow was broken silently — it does not run on PRs, so nothing surfaced it.
from school_mail import ...from src.tools.school_mail import ...python orca_precondition.pypython src/crew/orca_precondition.pypython gateway_preflight.pypython src/tools/gateway_preflight.pypython worktree_bloat_guard.pypython src/tools/worktree_bloat_guard.pypython crew_ledger_reconcile.pypython src/crew/crew_ledger_reconcile.py3.
ci.yml:293importedschool_mailby its pre-move pathfrom school_mail import notify_build_failure→from src.tools.school_mail import notify_build_failure.4. The review-gate fail-open — the PR's only REQUIRED check
ci.ymlpassed--head "origin/${{ github.event.pull_request.head.sha }}", which is not a ref name.git difffailed on stderr, the error was discarded, and the gate read stdout only — so it reported0 changed filesand PASSed on every PR since #117.scripts/review_gate.py:git()now raises on non-zero exit; newresolve_ref()verifies each ref withrev-parsebefore diffing; an empty diff is now a failure, not a pass.ci.yml: both invocations now pass--head "HEAD".Verification
tests/test_review_build_gate.py(RED onmain)tests/test_review_gate.py(+4 regression tests)review_gate.py --base origin/main --head HEADThe 4 new gate tests were RED-verified against the original gate: the failure text is the #117 bug verbatim —
review-gate: 0 changed file(s), 0 added line(s)/PASS - all mechanical checks clean.Why
mainwent red without anyone noticingreview-gateis the only required check, and it was fail-open. A greenreview-gateonmainwas not evidence the tree was sound — it was evidence the gate could not see the tree. This PR closes that.Not included
ci.yml's ruffCORE=list still namesonboard.pyanda2a_bridge.py, which no longer exist. Harmless (ruff warns and exits 0) but a silent scope shrink — separate cleanup.conductor.py,issue_bridge.py,tests/test_conductor_issue_path.py,tests/test_issue_bridge.py) belongs onschool/1nzr-behaviourperbb1b968's split.Summary by Sourcery
Restore post-reorganization workflow paths and make the required review gate fail closed instead of silently passing invalid or empty diffs.
Bug Fixes:
Enhancements:
CI:
Tests:
Fix the cache commands before merging; both workflow jobs still fail at the cache refresh.
Findings
Fix with agent prompt
Summary
This PR fixes workflow paths after the reorganization and makes the review gate reject Git failures and empty diffs.
flake.nix.Reviews (1) · Last reviewed commit: "fix(reorg): repoint workflow entrypoints..." · Reviewed by Greptile