Wire pre-commit into CI, fix triggers that never ran, fix gates that never blocked#126
Draft
cryptoxdog wants to merge 9 commits into
Draft
Conversation
.hypothesis/ has been in .gitignore all along, but 92 of its cache files were already committed to the repo at some point before the ignore rule was added (git only ignores new paths, it doesn't untrack existing ones). Every test run that exercises Hypothesis-based property tests mutates these files, so they showed up as noisy, unrelated diffs every time `pre-commit run --all-files` (trailing-whitespace, end-of-file-fixer) touched them locally or in CI. Untracking them so the ignore rule actually takes effect. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
trailing-whitespace / end-of-file-fixer (from .pre-commit-config.yaml) were failing on 5 pre-existing docs. Whitespace-only changes, no content edits. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
tools/contract_scanner.py's SEC-001 rule (Cypher label interpolation without sanitize_label()) is a single-line regex; it already allowlists engine/sync/generator.py and engine/handlers.py with the comment "labels sanitized via sanitize_label()" for exactly this pattern: the label variable is sanitized a few lines above the f-string that embeds it, so the same-line regex can't see it. engine/causal/attribution.py:72 and engine/causal/causal_compiler.py: 62-63 hit the identical pattern (sanitize_label() called on the preceding lines: attribution.py:60,63 and causal_compiler.py:50-51) but were never added to the allowlist, so `l9-contract-scan` currently reports 3 false-positive CRITICAL violations and blocks every commit that touches those files (or, since pre-commit was never wired into CI, blocks nothing and nobody -- see the CI-pipeline PR). Verified both files pass `python tools/contract_scanner.py <file>` clean after this change. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
…ust one pytest.ini's addopts had -x, applied globally to every pytest invocation that doesn't override addopts -- including the pytest-unit pre-commit hook and ci.yml's/ci-quality.yml's full test-suite runs. -x aborts the entire run after the very first failing test, so a run with N failing tests only ever reports test #1: fixing it, re-running, and discovering test #2 takes N push/CI round-trips instead of one. Removed -x; added -ra so the run also prints a one-line summary of every non-passing test (failed, error, or skipped-with-reason) at the end. Runs still exit non-zero (fail the job) if anything fails -- this only changes how much is surfaced in a single run, not whether failures block. Verified: running a file with 4 pre-existing failures now reports all 4 in one pass instead of stopping after the first (previously reproduced with -x). Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
Two changes to .github/workflows/ci.yml: 1. New "precommit" job (Phase 2.5), required by ci-gate. Runs `pre-commit run --all-files` via pre-commit/action so every hook in .pre-commit-config.yaml is enforced on every PR/push -- regardless of whether a contributor (human or agent) ran `pre-commit install` locally, or bypassed hooks with `git commit -n`. Skips two hooks with SKIP=gitleaks,packet-envelope-prohibited (both explained inline and in docs/CI_PIPELINE.md): gitleaks's `--staged` semantics don't apply to a plain CI checkout and secret scanning is already handled correctly by the `security` job's gitleaks-action; the packet-envelope hook has 23 pre-existing violations representing a real, separately tracked architecture migration (PacketEnvelope -> TransportPacket), not a CI-config problem. 2. Fixed the "Run Mypy Type Checker" step in the lint job. It ran `mypy .` (whole repo) piped through `|| echo "... non-blocking"`, which silenced every result -- and `mypy .` itself has always failed immediately with "Source file found twice under different module names" (tools/auditors/base.py vs auditors.base) before checking a single file, so this step has effectively always been a no-op. Now runs the same scoped, verified-working invocation as `make lint` / ci-quality.yml (`mypy engine/ --config-file=pyproject.toml --ignore-missing-imports --exclude chassis`), with no silencing, so real type errors now fail the job. ci-gate's fan-in now also requires `precommit` and fails if it fails. Verified locally: `pre-commit run --all-files SKIP=gitleaks,packet-envelope-prohibited` exits 0 on the current tree (after the preceding commits in this PR); `mypy engine/ ...` exits 0. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
ci-quality.yml's quality-gate job referenced needs.lint-format.result, needs.secrets-scan.result, and needs.yaml-validate.result using GitHub Actions dot notation. Dot notation doesn't work for job IDs containing a hyphen -- `-` is parsed as subtraction -- so those three always evaluated to an empty string, not the job's actual result. Every `if [[ "" == "failure" ]]` was therefore always false: quality-gate could report a passing summary even when lint-format or secrets-scan failed. semgrep's result wasn't checked at all (missing from the fail conditions, independent of the hyphen bug). Fixed by switching to bracket syntax (needs['lint-format'].result, etc.) for the three hyphenated job IDs, and added the missing semgrep check. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
…ate fix Documents the new Phase 2.5 (precommit job), the mypy scope/blocking fix, the ci-quality.yml quality-gate hyphen-notation bug fix, the pytest -x removal, and the current SKIP list for the precommit job with rationale for each skipped hook. Also notes that ci-gate and quality-gate need to be configured as required status checks in branch protection to actually block merges -- a workflow job failing does not, by itself, prevent merging. Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
|
❌ Too Many Files Changed
📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
…rkflows never ran
ci.yml, docker-build.yml, supply-chain.yml, codeql.yml, and
release-drafter.yml all had `on.push.branches` / `on.pull_request.branches`
set to `${{ vars.PRIMARY_BRANCH || 'main' }}` / `${{ vars.DEVELOP_BRANCH
|| 'develop' }}`.
GitHub Actions does not support context expressions in `on:` trigger
filters -- they're evaluated by the platform before a workflow run is
even created, so `vars`/`env`/etc. aren't available there (they are
supported in `env:`, job steps, etc. -- just not `on:`). A workflow file
using an expression there is rejected outright: "This run likely failed
because of a workflow file issue", 0 jobs created, regardless of what
the workflow actually contains.
Confirmed via `gh run list`: every push-triggered run of these 5 files
shows `failure` at `0s` duration, and none of them ever appear as a
successful pull_request-triggered run either -- meaning ci.yml (the
main "CI Pipeline" workflow, including everything fixed earlier in this
PR) has never actually executed in this repository, on any event, since
this line was written.
Fixed by hardcoding the branch names the `|| 'default'` fallback already
implied (main, develop where applicable). This is the single highest-
leverage fix in this PR: none of the mypy/pre-commit/quality-gate fixes
elsewhere in this stack could ever be verified by real CI without it.
Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
Co-authored-by: Igor Beylin <cryptoxdog@users.noreply.github.com>
|
❌ Too Many Files Changed
📋 Best Practices for Large Changes
🚫 This PR is blocked from merging until size limits are met. |
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.
Summary
Narrowly scoped to the CI pipeline itself (no lint/style remediation here — that's #125, which this branch is stacked on top of so the new checks below can actually run green). Addresses: "ci scripts/tests must run at pre-commit, wired that way so errors are caught before they hit PRs or the codebase" + "CI should surface every error, not just one, and PRs with CI failures should be blocked."
0. CRITICAL:
ci.yml(and 4 other workflows) have never actually triggeredWhile verifying this PR against live GitHub Actions,
gh run listshowedci.yml(anddocker-build.yml,supply-chain.yml,codeql.yml,release-drafter.yml) failing at 0 seconds on every push, with 0 jobs created — "This run likely failed because of a workflow file issue."Root cause:
on.push.branches/on.pull_request.brancheswere set to${{ vars.PRIMARY_BRANCH || 'main' }}/${{ vars.DEVELOP_BRANCH || 'develop' }}. GitHub Actions does not support context expressions inon:trigger filters — they're evaluated by the platform before a workflow run is even created. A workflow file using an expression there is rejected outright, regardless of what the rest of the file contains.env:/step-level${{ vars.X }}usage elsewhere in these same files is fine — only theon:block is affected.This means
ci.yml— the repo's main "CI Pipeline" workflow, with all 7-8 phases includingci-gate— has never actually executed in this repository, on any push or pull_request, since this line was written. All the mypy/pre-commit/ci-gate work below existed only on paper until this fix. Fixed by hardcoding the branch names the|| 'default'fallback already implied (main,developwhere applicable) across all 5 affected workflows.1. Pre-commit hooks are now enforced in CI, not just locally
.pre-commit-config.yamlhas 15+ hooks (ruff, mypy, contract scanner, deprecated-API checks, gitleaks, ...), but nothing ran them except a contributor's localgit commit— and only if they'd runpre-commit install. There was no CI-side backstop.Added a
precommitjob to.github/workflows/ci.yml(Phase 2.5) that runspre-commit run --all-filesviapre-commit/actionon every PR/push, and made it required byci-gate. Two hooks are skipped with a documented reason (SKIP=gitleaks,packet-envelope-prohibited):gitleaks's pre-commit hook runsgitleaks protect --staged, which inspects the git index — that has no equivalent in a plain CI checkout. Secret scanning is already handled correctly elsewhere inci.ymlviagitleaks/gitleaks-action.packet-envelope-prohibitedcurrently has 23 pre-existing violations acrossengine//chassis/— a real, tracked architecture migration (PacketEnvelope→TransportPacketfromconstellation_node_sdk, seedocs/contracts/SHARED_MODELS.md), not a CI-config problem. Skipping it here keeps this job actionable now instead of permanently red on unrelated, out-of-scope migration debt. (It turns out.github/workflows/terminology-guard.ymlalso runs this same check natively, independent of pre-commit — see "additional findings" below.)Running the full hook suite locally also surfaced and let me fix three small, low-risk pre-existing issues that were blocking hooks from ever passing:
l9-contract-scanhad 3 CRITICAL false-positive findings (Cypher label interpolation) inengine/causal/attribution.py/causal_compiler.py— the labels are sanitized viasanitize_label(), just a few lines above the interpolation, which the single-line regex can't see. Two other files already had this exact allowlist entry for this exact reason; added these two..hypothesis/(gitignored) had 92 cache files already tracked from before the ignore rule existed, causing constant unrelated whitespace-hook noise. Untracked them (this is why the file-count diff below looks big — the actual code diff is small).2. Fixed mypy's CI step, which has effectively never run
ci.yml's "Run Mypy Type Checker" step ranmypy .(whole repo) piped through|| echo "... non-blocking". Turns outmypy .has always failed immediately withSource file found twice under different module names(tools/auditors/base.pyvsauditors.base) before checking a single file — so this step was a silent no-op even setting aside issue #0 above. Fixed the scope to match the working, already-verified invocation (mypy engine/ --config-file=pyproject.toml --ignore-missing-imports --exclude chassis, same asmake lint/ci-quality.yml) and removed the silencing, so real type errors now fail the job.3. Fixed
quality-gateinci-quality.yml, which never actually checked most of its inputsneeds.lint-format.result,needs.secrets-scan.result, andneeds.yaml-validate.resultused GitHub Actions dot notation on hyphenated job IDs —-is parsed as subtraction in that syntax, so all three silently evaluated to an empty string instead of the job's real result. Everyif [[ "" == "failure" ]]was therefore always false: this gate could report ✅ even whenlint-formatorsecrets-scanfailed.semgrep's result also wasn't checked at all. Fixed with bracket syntax (needs['lint-format'].result) and added the missingsemgrepcheck.4. Fail-open testing: surface every failure in one run, still block on any of them
pytest.inihad-xinaddopts, applied globally (including thepytest-unitpre-commit hook and every CI test-suite run).-xstops at the very first failing test — so a run with N failures only ever reports failure #1, and fixing it just reveals #2 on the next run. Removed-x; added-raso a short summary of every non-passing test prints at the end. Still exits non-zero (still fails the job) on any failure — this only changes how much a single run surfaces, not whether it blocks.Verification
ruff check ./ruff format --check ./mypy engine/ ...all exit 0pre-commit run --all-files SKIP=gitleaks,packet-envelope-prohibitedexits 0 locally (every hook either passes or is a documented skip)-xverified to surface multiple failures in one run instead of stopping at the first (tested against a file with 4 pre-existing failures)yaml.safe_load, plus a targeted scan confirming no remainingvars.expressions inside anyon:blockon:trigger fix itself can't be verified via a live run on this PR, sinceci.yml's trigger is (correctly) scoped tomain/develop, and this PR's base is a feature branch (see below) — but the root cause and fix are unambiguous per GitHub's documented behavior, and match the exact 0-second/0-job failure signature observed viagh run listboth before and after other unrelated commits, confirming it's a trigger-level, not content-level, problemAdditional pre-existing CI issues discovered (NOT fixed here — out of narrow scope, flagging for visibility)
Confirmed all of the following are pre-existing and unrelated to any change in this PR or #125:
contracts.ymlruns a third, separatemypy engine/ --strict ...invocation (ci.ymlandci-quality.ymlare the other two, both now fixed/consistent) that surfaces 16--strict-only errors (missing type annotations/generic params) inengine/inference_rule_registry.py,engine/graph/graph_sync_client_fix.py,engine/startup_wiring.py,engine/graph/community_export.py, plus one staletype: ignoreinmultihop.pythis stack made newly-unused. Three inconsistent mypy invocations across three workflows is itself a consistency problem, but fixing the underlying--strictgaps is a distinct, larger task.terminology-guard.ymlindependently runscheck_packet_envelope_prohibited.py(same 23 violations noted above) as its own CI step, unrelated to theprecommitjob's SKIP.docs-code-sync.yml's "Check Markdown Links" step has a bash bug:target="${link./}"is invalid parameter-substitution syntax (bad substitution), so it fails on any PR touching markdown docs, regardless of link validity.pr-review-enforcement.yml's size-policy bot flags this PR for file count (102 files) — almost entirely the.hypothesis/untracking (92 deletions); the actual code diff is ~10 files.get_ci_statusreportsRequired checks: 0 failed, 0 pendingon both this PR and Enable working CI lint autofix + remediate pre-existing ruff/mypy/pytest errors #125 — i.e. none of these checks (including the now-fixedci-gate/quality-gate) are currently configured as required status checks in branch protection, so nothing here actually blocks a merge today regardless of red/green. This is likely the single biggest remaining gap relative to "PRs with CI failures should be blocked" — see below.Not fixed here (flagged, deliberately out of scope)
packet-envelope-prohibited's 23 violations — real architecture migration, needs its own dedicated effort.ghaccess doesn't have permission to view or modify branch protection rules (403 onGET .../branches/main/protection). Forci-gateandquality-gateto actually block merges (not just show red), a repo admin needs to mark them (and ideallyprecommit) as required status checks in Settings → Branches. Noted indocs/CI_PIPELINE.md.tests/security/test_compliance_security.pyhas the sameDomainPackLoader(domains_dir=...)bug fixed in Enable working CI lint autofix + remediate pre-existing ruff/mypy/pytest errors #125'stest_injection.py) are left alone — those are codebase content, not CI plumbing, and out of scope for "narrow to the CI pipeline itself."Base branch note
This PR is based on
cursor/enable-ci-autofix-ddaf(#125) rather thanmain, since the newprecommit/mypyCI jobs need that branch's lint/mypy/pytest-config fixes to actually pass. Merge order: #125 → this PR → main. Because of this,ci.yml'spull_requesttrigger (scoped tomain/developbases) won't fire on this PR itself — the fix from item #0 will start taking effect once this lands onmain/develop.