Run tests locally and keep hosted CI lightweight - #274
Merged
Merged
Conversation
Summary: - Remove Python, Playwright and Flutter test execution from Actions. - Retain formatting, lint, type checks, PostgreSQL migration validation and AI review. - Expand make test-all to all seven Python tiers and add local Flutter test target. Changed files: - Workflows: hosted static checks without test jobs. - Makefile: local web, contract, Playwright and Flutter targets. - Agent instructions and docs: require local test evidence for pushed revision. - Unit policy guards: verify local coverage and hosted behavior. Validation: - Expanded make test-all: 1468 passed, 21 skipped including 33 Playwright tests. - make test-mobile: 64 passed; Flutter analysis passed with info warnings. - Black, Ruff, actionlint and diff whitespace checks passed. - Local strict mypy reports 90 existing errors in unchanged api/models.py and api/security.py; hosted gate unchanged. Follow-ups: - GitHub does not attest local test execution. Include results in every PR. - Keep merge blocked until hosted checks and AI review pass.
AI review (Ollama: glm-5.3-flash){
"verdict": "NEEDS DISCUSSION",
"summary": "Mechanically, this diff is coherent: ci.yml keeps black/ruff, an advisory mypy step and the PostgreSQL alembic upgrade smoke; mobile-ci keeps flutter analyze; Makefile's test-all grows to seven tiers (unit/api/cli/integration/web/contract/e2e) plus test-web/test-contract/test-e2e/test-mobile; docs, agent rules and three guard tests are updated consistently, and new policy tests pin the split. No application code changed, so no tenant-isolation or code-level security issues. However, the change removes the only independent, publicly verifiable test signal: with the e2e job deleted and (per the description) no branch protection and empty rulesets, green CI is now compatible with a completely failing suite, and merge assurance rests on self-reported local runs recorded in PR text. That may be an intentional owner decision, but it cannot be confirmed from untrusted PR metadata, and two internal contradictions need resolution: (1) the description claims a hosted strict mypy gate 'must pass before merge', yet the only mypy step visible in ci.yml sets continue-on-error: true, and local strict mypy reportedly reports 90 errors in unchanged files — if a real strict gate exists elsewhere it should currently be red, and if not, the documented guarantee is false; (2) job renames/deletions ('Lint, type-check, and test' → 'Lint and type-check', e2e job removed, 'Flutter analyze + test' → 'Flutter analyze') can break anything referencing old check names. Confirm policy acceptance, the strict-mypy story, and a sweep for stale references to hosted test lanes before merging; the mechanical implementation itself looks sound.",
"findings": [
{
"priority": "P2",
"path": ".github/workflows/ci.yml",
"line": 15,
"detail": "Removal of all hosted test execution (six pytest steps plus the entire blocking e2e job) leaves no independent verification that tests pass; combined with no branch protection/rulesets, green CI now proves only formatting, advisory mypy and migration smoke. Obtain explicit owner sign-off for this policy and consider compensating controls (e.g., one hosted smoke tier, required reviewer attestation of recorded local results for the pushed SHA)."
},
{
"priority": "P2",
"path": ".github/workflows/ci.yml",
"line": 74,
"detail": "Contradiction to resolve: description says 'Hosted strict gate is unchanged and must pass before merge', but the visible mypy step has continue-on-error: true (advisory only), and local strict mypy reportedly has 90 errors in unchanged api/models.py and api/security.py. Clarify where the strict gate lives, whether it currently passes on base, and reconcile docs; otherwise remove the 'must pass' claim or the continue-on-error flag."
},
{
"priority": "P2",
"path": "AGENTS.md",
"line": 85,
"detail": "Jobs were renamed/deleted ('Lint, type-check, and test'→'Lint and type-check'; 'End-to-end (Playwright)' job deleted; 'Flutter analyze + test'→'Flutter analyze'). Any merge scripts, dashboards, docs/ai-pr-review.md references, or future branch-protection configs keyed to the old check names will fail or silently lose meaning. Verify and update all references; also the mobile-ci header still says the lane 'gates' the Flutter app though it now only analyzes."
},
{
"priority": "P3",
"path": "tests/unit/test_ci_test_policy.py",
"line": 27,
"detail": "test_local_all_runs_each_python_tier_in_a_separate_process shells out to 'make -n'; this requires make in every unit-test environment and still evaluates $(shell ...) during parsing. The exact-count==7 assertion is an intentional guard but couples the test to test-all's precise structure; a comment noting the make/POSIX assumption would help."
},
{
"priority": "P3",
"path": "README.md",
"line": 427,
"detail": "Playwright is installed via 'poetry run pip install playwright==1.60.0' outside poetry.lock; a later 'poetry install' can drop it and the hard-coded pin can drift from the e2e harness expectations. Consider adding it to a Poetry dev dependency group or documenting reinstallation after dependency sync."
},
{
"priority": "P3",
"path": "Makefile",
"line": 513,
"detail": "Help text now lists test-e2e and test-mobile but not the newly added test-web and test-contract targets; add echoes for completeness. Also sweep remaining docs for stale claims that CI executes tests/e2e (only three guard tests were updated)."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Summary: - Document owner-requested local-only testing and retained blocking static checks. - Fix nullable optional stats result rejected by the hosted Flutter analyzer. Changed files: - CI comments, Make help, READMEs and review guide: clarify policy and check names. - Flutter provider and regression test: preserve solution when stats request fails. Validation: - Regression reproduced Future.then type error before fix and passes after fix. - Flutter: 65 tests pass; analyze passes with info warnings. - Backend/web/contract: 1435 passed, 21 skipped. - Browser final full rerun: 33 passed; prior run had one time-off removal timeout (32 passed). - Black, Ruff, actionlint and diff whitespace checks pass. - Hosted strict type and migration job passed on prior refactor commit; revalidate new head. Follow-ups: - Track intermittent time-off browser timing failure; do not conceal initial failure. - Await current-head AI review and hosted static checks before merging.
Owner
Author
|
Addressed review discussion in a0977e4:
Final local evidence for the pushed source: 1435 backend/web/contract passed, 21 skipped; Flutter 65 passed and analyze passed; full Playwright rerun 33 passed. The preceding full run had one time-off removal timeout and 32 browser passes; that intermittent timing risk is recorded, not hidden. Black, Ruff and actionlint pass. Await fresh hosted checks and AI verdict. |
AI review (Ollama: glm-5.3-flash){
"verdict": "NEEDS FIX",
"summary": "The diff is internally consistent and candidly documented: guard tests, docs, and retired check names were all updated to the local-only policy, and the mobile solver_provider.dart change is a genuine bug fix (previously, when the optional stats call failed, onError returned null through a non-nullable .then(), which raises a runtime type error; the new typed .then<SolutionStatsResponse?> plus the added Dio-mock regression test fixes that). However, the core change deletes every hosted test gate for a multi-tenant app: the unit/API/CLI/contract/web/integration steps and the entire blocking Playwright e2e job are removed from Actions, leaving static analysis, an Alembic smoke run, and AI review as the only machine-verified signals. The PR itself reports branch protection returned 404 and rulesets are empty, so after this change nothing automated detects regressions — including the auth/RBAC/tenant-isolation suites — and 'green CI' becomes actively misleading to anyone following AGENTS.md rule 3 ('Merge only when CI is green'). The owner-authorization quote exists only inside untrusted PR content and cannot be verified from this diff. Before merging, require verified owner sign-off recorded outside PR data plus at least one compensating automated control: restore a cheap hosted lane (unit + OpenAPI contract) or add a scheduled full-suite workflow, and configure branch protection with required checks. The remaining code changes (Makefile targets, guard tests, Dart fix/test) reviewed clean; no security or tenant-isolation defects found in the code itself.",
"findings": [
{
"priority": "P1",
"path": ".github/workflows/ci.yml",
"line": 76,
"detail": "All hosted test execution is removed (unit/api/cli/contract/web/integration steps plus the whole e2e job that was explicitly a blocking lane). Hosted CI now verifies only black/ruff, scoped strict mypy, advisory mypy, and an Alembic upgrade — no test runs. For a multi-tenant codebase the auth/RBAC/tenant-isolation API and integration suites lose their only machine-enforced execution point, and since branch protection is reportedly absent (404/empty rulesets), nothing enforces the new local-only policy either. The justification quote lives only in untrusted PR content. Fix: retain at least the fast unit + tests/contract lanes in CI, or add a scheduled (nightly) workflow running the full suite; configure branch protection to require the hosted checks; document verified owner approval outside PR data."
},
{
"priority": "P2",
"path": ".github/workflows/mobile-ci.yml",
"line": 40,
"detail": "'flutter test' removed from the hosted lane; only 'flutter analyze --no-fatal-infos' remains. Regressions in the mobile API-client layer (already flagged as a known gap in mobile/README.md, issue #191) now depend entirely on developers voluntarily running 'make test-mobile'. Consider keeping the cheap flutter test step or a scheduled run."
},
{
"priority": "P2",
"path": "AGENTS.md",
"line": 109,
"detail": "Rule 3 ('Merge only when CI is green') is retained verbatim while green no longer includes any tests; the same wording persists in CLAUDE.md and .github/copilot-instructions.md. Future agents/humans can treat green CI as sufficient. Fix: amend rule 3 to require CI green AND recorded local results bound to the pushed head SHA (e.g., 'make test-all on <sha>: N passed') present in the PR."
},
{
"priority": "P2",
"path": "docs/ai-pr-review.md",
"line": 91,
"detail": "Local test evidence is purely procedural and unbound: nothing ties reported results to the pushed revision and nothing detects skipped runs. Recommend requiring the head SHA in recorded evidence and a PR-template evidence section. Also note the embedded owner-request quote is untrusted PR data; confirm the policy change through an owner-controlled channel."
},
{
"priority": "P3",
"path": "tests/unit/test_ci_test_policy.py",
"line": 7,
"detail": "Imports yaml (PyYAML) with no visible declaration as a test dependency; if it is only transitive, clean environments fail at collection and break the required local suite. Add pyyaml to the Poetry dev group, or parse the workflow with the plain-text pattern used by the sibling guard tests."
},
{
"priority": "P3",
"path": "tests/unit/test_ci_test_policy.py",
"line": 35,
"detail": "test_local_all_runs_each_python_tier_in_a_separate_process invokes 'make -n test-all' and depends on GNU make echoing \u0040-prefixed recipe lines and on make being installed; make -n also still evaluates parse-time $(shell ...) functions. More robust: assert on the Makefile text directly, matching the pattern used in test_ci_has_web_lane.py and test_e2e_lane.py."
},
{
"priority": "P3",
"path": "Makefile",
"line": 227,
"detail": "New test-web/test-contract/test-e2e targets depend only on check-poetry, not ensure-test-env, unlike test-all. If the web/e2e tiers rely on environment prepared by ensure-test-env (TEST_APP_URL, fixture flags), standalone targets may behave differently than within test-all. Verify standalone execution or add ensure-test-env as a prerequisite."
},
{
"priority": "P3",
"path": "README.md",
"line": 428,
"detail": "Playwright is installed ad hoc via 'poetry run pip install \"playwright==1.60.0\"' outside the Poetry lockfile; environments can drift (or lose it on 'poetry install' sync, as the README itself warns). Consider a locked dev-extra or a make bootstrap target that installs/pins it reproducibly."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
Owner
Author
|
Current status: implementation complete, merge blocked by AI review policy on a0977e4. Hosted checks pass: Lint and type-check (1m16s), Flutter analyze (1m17s). Local source validation: 1435 backend/web/contract passed, 21 skipped; full browser rerun 33 passed; Flutter 65 passed. Initial browser timeout is recorded in the preceding evidence comment. AI gate now marks removal of hosted tests as P1 and demands retained hosted tests or scheduled test execution, which conflicts with the owner request for local-only testing. Do not retry merely to obtain a different verdict, restore hosted tests against the request, or bypass the gate. Keep this PR open pending an explicit reviewer-policy decision. No branch protection/ruleset settings were changed. |
Summary: - Supply the explicitly approved local-only test policy in the reviewer system prompt. - Preserve independent code/security review, P0/P1 failure handling, and untrusted input boundaries. - Require local test evidence tied to the pushed head alongside hosted checks. Changed files: - AI review workflow and regression test: narrow policy clarification, no verdict overrides. - Agent instructions and review guide: local evidence and approval requirements. Validation: - Reviewer workflow: 46 tests pass, including new prompt-policy regression. - Full local make test-all: 1469 passed, 21 skipped including 33 Playwright tests. - Flutter suite: 65 passed. - Black, Ruff, actionlint and diff checks passed. Follow-ups: - Wait for current-head hosted checks and independent AI review before merging.
Owner
Author
|
Owner approved updating the reviewer policy to permit local-only testing while retaining code review and other merge checks. Implemented narrow system-prompt policy in bafbe86, with no verdict override, no P0/P1 suppression, and no skipped/error-as-approval path. Added regression exercising the actual outgoing policy prompt; existing failure handling remains tested. Local evidence for HEAD bafbe86 (source tested immediately before commit; commit contains exactly that tested content):
Merge instructions now explicitly require local results with the pushed head SHA plus hosted checks, AI review and GitHub mergeability. Await fresh independent review on this head; do not bypass failed checks. |
AI review (Ollama: glm-5.3-flash){
"verdict": "SAFE TO MERGE",
"summary": "This PR removes hosted test execution per the owner-approved repository policy (which matches the reviewer system prompt): ci.yml keeps black/ruff, blocking scoped mypy, advisory whole-API mypy, and the PostgreSQL 'alembic upgrade head' validation; mobile-ci.yml keeps flutter analyze; the Ollama review gate is retained with prompt additions that mirror the accepted policy without weakening review duties ('Still report broken code, security defects, weakened or missing test coverage...'). Local coverage is preserved: make test-all now runs all seven Python tiers in separate processes and make test-mobile runs Flutter tests; README documents the out-of-lockfile Playwright 1.60.0 install and FLUTTER override. Updated guard tests now assert local command presence and hosted-static-only workflows, consistent with the accepted policy. The only functional code change (mobile/lib/features/admin/solver_provider.dart) is behavior-preserving: stats futures are now typed Future<SolutionStatsResponse?> via .then<...>((r) => r.data) instead of a dynamic wrapper cast, with a new local Dart test covering the stats-failure fallback. No backend, security, or tenant-isolation changes. No deceptive claims found: the PR explicitly states green CI is not test evidence, and the reported counts (1468) sum correctly across tiers (424+444+16+325+225+1+33). Nonblocking issues: recorded local results are attributed to the revision 'before commit 41c71cd' while the pushed head is bafbe861..., so evidence is not bound to the pushed head SHA as the PR's own rules require — rerun and record at head before merge. Note: no branch protection exists (404/empty rulesets per the PR), so hosted checks are advisory in enforcement; per the accepted policy this is not itself a blocking finding. Local test reports are treated as unverified claims; hosted test execution is not demanded.",
"findings": [
{
"priority": "P2",
"path": "docs/ai-pr-review.md",
"line": 99,
"detail": "Evidence traceability gap: the validation section reports make test-all results for the 'exact source revision tested before commit 41c71cd', but the pushed head is bafbe8610db82b0c037b9d95157cdfdce0a9f336. The PR's own merge rules (AGENTS.md rule 2/3, CLAUDE.md, docs/ai-pr-review.md 'Record the exact pushed head SHA alongside local results before merging') require successful local results recorded for the pushed revision. Mobile code changed in this PR (solver_provider.dart plus a new test), so make test-mobile evidence should also be head-bound. Proposed fix: before merge, rerun make test-all and make test-mobile at head bafbe861 and record the pass counts and skip counts in the PR explicitly tied to that SHA (or state that commit 41c71cd's tested tree is content-identical to the head and explain any intermediate commits). This is a procedural evidence-binding fix, not a code defect."
},
{
"priority": "P3",
"path": "Makefile",
"line": 225,
"detail": "make test-all's Playwright e2e tier depends on playwright being pip-installed outside the Poetry lockfile (README documents a manual 'poetry run pip install playwright==1.60.0' + 'playwright install chromium'). A fresh environment will run six tiers first and only fail at the e2e tier via ImportError/collection error, wasting a long run. Proposed fix: add a cheap preflight at the top of the test-all recipe (e.g., 'poetry run python -c \"import playwright\" || (echo \"Run: poetry run pip install playwright==1.60.0 && poetry run playwright install chromium\"; exit 1)') or an echo'd prerequisite note in the help target. README already documents the requirement, so this is a robustness nicety only."
},
{
"priority": "P3",
"path": "tests/unit/test_ci_has_web_lane.py",
"line": 12,
"detail": "Regression guard weakened per the accepted local-only policy: it previously asserted CI executes the web suite; it now asserts only that the Makefile text contains 'pytest tests/web/' (same pattern in tests/unit/test_e2e_lane.py, which now checks the Makefile/README text instead of the CI e2e job). These guards can no longer detect a failing or silently-skipped local suite — they verify command wiring, not execution, and nothing hosted gates results. Acceptable under the owner-approved policy and partially compensated by test_ci_test_policy.py's 'make -n' structural checks and the head-SHA result-recording rule; flagged as an assurance change. Suggested (optional) mitigation: keep guards checking that the e2e harness and conftest registration files exist (test_e2e_lane.py already does) and document in the guard docstrings that execution is proven only by recorded local runs."
}
]
}Diff-only advisory review; CI and GitHub mergeability remain mandatory. |
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
1 participant
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.
Summary:
Changed files:
Validation for HEAD bafbe86:
Evidence corrections:
Follow-ups: