Audit sweep: make CI actually run, and actually check the serve/API + optimizer layers#1
Open
jarmstrong158 wants to merge 5 commits into
Open
Audit sweep: make CI actually run, and actually check the serve/API + optimizer layers#1jarmstrong158 wants to merge 5 commits into
jarmstrong158 wants to merge 5 commits into
Conversation
The workflow gated push/pull_request on `branches: [master]`. This repo's default branch has always been `main` and no `master` ref has ever been pushed, so this workflow has never run once since it was added. Its own header comment says "Passing tests must not depend on local discipline" — which is precisely what a workflow that cannot trigger leaves you with. Point both gates at `main`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI installed only `dev = ["pytest", "httpx"]`, but four test modules opened with pytest.importorskip on fastapi / httpx / ortools. In a clean CI environment every one of those guards resolved to "skip", so the entire serve/API layer and the CP-SAT optimizer were never executed — 503 passed / 6 skipped read as green while ~27 tests silently did not run. Locally they passed only because the ambient env happened to have the packages, which is exactly the local-discipline dependence the CI workflow was written to eliminate. Two changes, both needed — installing the deps without removing the guards would still leave the skip mechanism in place for the next dep. 1. pyproject: `dev` now pulls in `clark[serve,optimizer]`, so anything added to those extras is automatically armed in CI. New `optimizer` extra declares ortools (see the sibling commit for the runtime gap). 2. tests: the importorskip guards are removed. importorskip is right for a genuinely optional dependency and wrong for a required one, because a skip is indistinguishable from a pass in a CI summary. A missing dep now fails. tests/test_optional_deps.py is the loud named tripwire: it asserts each optional dep imports and that the CP-SAT native solver actually solves, so an incomplete install produces one clear message naming the missing extra rather than a wall of collection errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clark/inference/optimizer.py imports ortools inside plan_day, but the package was declared nowhere in pyproject.toml. On a clean install the CP-SAT completion bound the README and ENGINEERING_NOTES §10 both advertise died on a bare ImportError telling you to `pip install ortools` with no indication that it was meant to be an extra. - README: the reproduction line now separates the heuristic comparison (`clark eval --baseline heuristic ...`) from the CP-SAT bound, which needs `pip install -e ".[optimizer]"` and `evaluate_bound`. - README Tests section: states that `dev` deliberately pulls in serve+optimizer, and that a narrower install fails rather than skips. - ENGINEERING_NOTES §10: names the extra where the module is introduced. - optimizer.py: the ImportError now names the extra, not a bare package. Also widens the .gitignore venv rule from `.venv/` to `.venv*/`. A 736MB `.venv-ci/` (a clean-install CI mirror, the environment that exposed the silent-skip gap) sat untracked and unignored because the exact-name rule missed it. Deliberately not adding `env/` — it would shadow the tracked `clark/env/` source package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Same root cause as the CI gate: this repo has only ever had `main` (`git branch -r` shows origin/main and origin/cambium, never a master). The published model card linked LICENSE, CHANGELOG.md and ENGINEERING_NOTES.md through `/blob/master/`, so every one of those is a 404 for anyone arriving from Hugging Face — including the license link in the YAML front-matter, which is the one a reuser is most likely to click. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment I added in 9f9facc claimed no `master` ref had ever existed and that this workflow had never triggered once. Opening this PR proved that wrong: GitHub's run history shows CI ran successfully on `master` via push events, most recently on 2026-06-07 (run 27101863487, commit 1d6f015). The actual defect is narrower and worth stating precisely. The gate was correct when written. The default branch was later renamed `master` -> `main`, and nothing updates a workflow's branch filter on rename, so the workflow silently stopped firing — no error, no warning, no failed run, just an absence. Commits landed on `main` with no CI at all until this PR, which is the first run since the rename. I'd rather leave the wrong claim in history with a correction on top than quietly force-push over it. The fix itself (gate on `main`) was right for the wrong stated reason; only the comment changes here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four fixes, all variations on one theme: signals that looked green while checking nothing.
1. CI stopped running when the branch was renamed
.github/workflows/ci.ymlgatedpushandpull_requestonbranches: [master].Correction to my first commit message: I initially claimed no
masterref had ever existed and that this workflow had never run. Opening this PR disproved that — GitHub's run history shows CI ran successfully onmaster, most recently on 2026-06-07 (run27101863487, commit1d6f015). Commitc36991acorrects the comment I wrote in9f9facc.The real defect is narrower and worse-behaved than a typo. The gate was correct when written. The default branch was later renamed
master→main, and nothing updates a workflow's branch filter on rename — so the workflow silently stopped firing. No error, no warning, no failed run. Just an absence, which is the one failure mode a dashboard cannot show you. Everything that landed onmainafter the rename went in with no CI at all, and the checks on this PR are the first run since.Its own header comment reads "Passing tests must not depend on local discipline." A rename quietly switching CI off is exactly that. Both gates now point at
main.2. CI was set up to silently skip ~27 tests
devwas["pytest>=8", "httpx>=0.27"], but four test modules opened withpytest.importorskiponfastapi/httpx/ortools. In a clean environment every one of those guards resolved to skip, so the entire serve/API layer and the CP-SAT optimizer were never executed and the run still reported green.Measured, not assumed:
devextra (CI mirror)That 27-test gap is the serve API, the per-facility agent loader,
/compare+/calendar_check, and the CP-SAT bound. It passed locally only because the ambient interpreter happened to have the packages installed — precisely the local-discipline dependence the workflow exists to eliminate.Both halves were necessary:
devnow pulls inclark[serve,optimizer], so anything added to those extras is automatically armed in CI rather than needing a second manual edit.importorskipguards are deleted.importorskipis the right tool for a dependency that is genuinely optional to the thing under test, and the wrong tool for one the test requires — in a CI summary a skip is indistinguishable from a pass.tests/test_optional_deps.pyis the loud, named tripwire: it asserts each optional dep imports and that the CP-SAT native solver actually solves a model, so an incomplete install yields one clear message naming the missing extra instead of a wall of collection errors. (These 5 tests are the entire difference between 530 and 535.)3.
ortoolswas undeclaredclark/inference/optimizer.pyimportsortoolsinsideplan_day, but the package appeared nowhere inpyproject.toml. The CP-SAT bound that the README and ENGINEERING_NOTES §10 both advertise died on a clean install with a bareImportError.New
optimizerextra declares it. The README reproduction line now separates the heuristic comparison from the CP-SAT bound and gives the real command; ENGINEERING_NOTES §10 names the extra where the module is introduced; and theImportErroritself now points atpip install -e ".[optimizer]"rather than a bare package name.4. Housekeeping
.gitignore:.venv/→.venv*/. A 736MB.venv-ci/(the clean-install mirror that exposed the skip gap) sat untracked and unignored because the exact-name rule missed it. Deliberately not addingenv/— it would shadow the trackedclark/env/source package./blob/master/links, dead since the same rename. Includes the license link in the YAML front-matter — the one a reuser is most likely to click.Verification
Full suite run locally against the final state of this branch: 535 passed, 4 skipped, zero failures. The remaining 4 skips are intentional and self-describing (
CLARK_RUN_SLOWgates and facility-shape guards), not dependency skips.pip install -e ".[dev]"was dry-run verified to resolve the self-referential extra — setuptools emitsRequires-Dist: clark[optimizer,serve]; extra == "dev"and pip pullsortools,fastapi, anduvicornin transitively.Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
🤖 Generated with Claude Code