Skip to content

ci: add an aggregate gate, CodeQL and the AI review configuration - #14

Open
gabrielspadon wants to merge 4 commits into
mainfrom
ci/house-standard-and-codeql
Open

ci: add an aggregate gate, CodeQL and the AI review configuration#14
gabrielspadon wants to merge 4 commits into
mainfrom
ci/house-standard-and-codeql

Conversation

@gabrielspadon

Copy link
Copy Markdown
Collaborator

Brings CiteForge to the same CI shape as its five sibling repositories, without changing its toolchain. CiteForge deliberately stays on GitHub Actions and gains no .circleci/; pip, the --require-hashes lock install and mypy are all kept exactly as they were. Matching the siblings means matching structure and naming, not forcing a different toolchain onto a repository that made its own choices.

tests.yml

Adds what the house pattern has and this file lacked: a top-level permissions: contents: read, a concurrency block that cancels only on pull requests, timeout-minutes and a display name on every job, and persist-credentials: false on both checkouts.

The substantive addition is a Required CI aggregate gate with if: always() and a needs list naming every other job. That single check is what branch protection should require, so a job added later is covered automatically and a renamed job cannot silently stop being required. Verified by set difference on the job keys that the needs list covers every non-gate job with no extras, and the gate script was extracted and executed against success, failure, skipped and cancelled.

The coverage floor is preserved at 68, not reinvented. Note it currently has 0.48 points of headroom (the suite reports 68.48%), so a small untested addition will break the build.

One extra fix: the pip cache key hashed only pyproject.toml while the install reads requirements-dev.lock, so the key never rotated when the lock was regenerated. It now hashes both. Performance only, since --require-hashes meant correctness was never at risk.

codeql.yml (new)

CiteForge is the only public repository in the group, which makes GitHub Advanced Security free here and unavailable on the five private siblings. CodeQL runs for Python on pull requests, pushes to main, and weekly, with security-events: write granted per job rather than at the top level.

AI review configuration

.coderabbit.yaml, greptile.json and .pr_agent.toml, adapted from the siblings rather than copied. Rust, SQL, TypeScript, Helm and CircleCI rules are dropped; the required check is retargeted to Required CI. The invariants are quoted from CLAUDE.md rather than invented: the Three-Way Fix Pattern, determinism through sorted(), _norm_doi() with its 0.55 title-similarity threshold, the 0.95 orphan-safety bound, the 13-source trust hierarchy, and the ban on automated audit modules.

.pre-commit-config.yaml (new)

Python-only hooks; no Rust or SQL copied across. ruff-format is deliberately omitted because pyproject.toml configures ruff for linting only and CI never runs a format check, so adding it would reformat the tree.

The mypy hook is language: system rather than mirrors-mypy. In an isolated hook venv rapidfuzz and unidecode are absent, so ignore_missing_imports types them Any and warn_return_any fires two errors CI never sees. Running against the same interpreter keeps the hook and CI honest, and pins the version by the lock instead of a second drifting rev.

Verification

All seven distinct action SHAs re-resolved against their real repositories; none fabricated. actionlint reports zero diagnostics on tests.yml and codeql.yml, and the only output is two pre-existing style notes in monthly-refresh.yml, which is untouched. Every workflow command was executed for real against the hash-pinned lock: ruff check clean, mypy clean across 33 files, pytest 810 passed with 68.48% coverage.

Two things for the maintainer

Required CI will exist as a check but is not yet required — no ruleset on this repository carries a required_status_checks rule at all. It only gates once added to the ruleset.

Separately, running the test suite mutates tracked files under data/api_cache/.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

ci: add Required CI aggregate gate, CodeQL, and AI review configs

⚙️ Configuration changes ✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Add a single Required CI aggregate job for branch protection stability.
• Harden GitHub Actions defaults with permissions, concurrency, timeouts, and safer checkouts.
• Enable Python CodeQL scanning and standardize AI review and pre-commit configuration.
Diagram

graph TD
  E["GitHub event"] --> TWF["Tests workflow"] --> L["Lint+types"] --> G["Required CI"] --> BP["Branch protection"]
  TWF --> TT["Test matrix"] --> G
  E --> QWF["CodeQL workflow"] --> QA["Analyze Python"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Require each CI job directly in branch protection
  • ➕ No aggregate gate logic to maintain
  • ➕ Immediate visibility into exactly which job is required
  • ➖ A renamed/added job can silently stop being required
  • ➖ Branch protection configuration must be kept in sync with workflow jobs
2. Use GitHub ruleset “required workflow” / check suite constraints (if available)
  • ➕ Centralizes enforcement without an extra gate job
  • ➕ Potentially less custom scripting
  • ➖ Less portable across org/repo settings
  • ➖ Still easy to miss coverage when jobs evolve, depending on ruleset capabilities

Recommendation: Keep the aggregate Required CI gate as the single protected check. It decouples branch protection from job naming churn and ensures future jobs become enforceable by a single, explicit needs update. The custom result-check logic is small and readable, and the repo already standardizes on “CI is the gate” semantics.

Files changed (6) +496 / -2

Other (6) +496 / -2
.coderabbit.yamlAdd CodeRabbit review scope, tone, and path-specific invariants +169/-0

Add CodeRabbit review scope, tone, and path-specific invariants

• Introduces a schema-validated CodeRabbit configuration tailored to this repo. Excludes generated/lock/cache/key paths and encodes review guidance and invariants (config-driven values, determinism, DOI/orphan thresholds, Three-Way Fix Pattern). Disables review-driven merge blocking by keeping commit status and request-changes workflow off.

.coderabbit.yaml

codeql.ymlAdd Python CodeQL scanning workflow with least-privilege permissions +54/-0

Add Python CodeQL scanning workflow with least-privilege permissions

• Adds a CodeQL workflow running on PRs, pushes to main, and a weekly cron. Uses pinned actions, PR-only concurrency cancellation, and job-scoped 'security-events: write' to upload SARIF results.

.github/workflows/codeql.yml

tests.ymlHarden CI workflow and add 'Required CI' aggregate gate +60/-2

Harden CI workflow and add 'Required CI' aggregate gate

• Adds top-level 'contents: read' permissions, PR-only concurrency cancellation, per-job names/timeouts, and 'persist-credentials: false' for checkouts. Fixes pip cache key rotation by hashing both 'pyproject.toml' and 'requirements-dev.lock'. Introduces an 'if: always()' aggregate job ('Required CI') that fails if any required job fails or is unexpectedly skipped.

.github/workflows/tests.yml

.pr_agent.tomlAdd PR-Agent configuration with ignore globs and repo-specific guidance +75/-0

Add PR-Agent configuration with ignore globs and repo-specific guidance

• Configures PR-Agent to focus on actionable defects, require tests/security review, and scan for TODOs. Excludes lock files, generated output, caches, and keys from analysis and embeds the repo’s invariants and CI expectations.

.pr_agent.toml

.pre-commit-config.yamlAdd Python-only pre-commit hooks aligned with CI gates +66/-0

Add Python-only pre-commit hooks aligned with CI gates

• Adds a pre-commit configuration mirroring CI checks (basic hygiene hooks, ruff check, and mypy). Explicitly excludes generated outputs and lock files, and runs mypy from the system environment to match the project’s pinned dependency set.

.pre-commit-config.yaml

greptile.jsonAdd Greptile config with invariant-aware review context +72/-0

Add Greptile config with invariant-aware review context

• Introduces Greptile configuration to provide consistent, defect-focused review feedback. Adds ignore patterns and encodes key repository invariants (determinism, config-driven thresholds, DOI/orphan rules, workflow pinning) plus pointers to authoritative docs (CLAUDE.md, README.md, pyproject.toml).

greptile.json

@qodo-code-review

qodo-code-review Bot commented Jul 29, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 20 rules

Grey Divider


Action required

1. Gate misses new jobs ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new Required CI job only checks needs.lint.result and needs.test.result, so a future job
added to needs can still fail without failing Required CI if the script is not updated. This
breaks the stated goal that adding a job later is covered by branch protection as soon as it joins
the needs list.
Code

.github/workflows/tests.yml[R134-163]

+    needs:
+      - lint
+      - test
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+    steps:
+      - name: Check job results
+        env:
+          # ${{ needs[shell-var] }} does not work because the templating engine
+          # evaluates the expression before the shell loop runs and can't see
+          # bash variables. Pre-render all required job results into env vars,
+          # then iterate over them in the shell.
+          RESULT_LINT: ${{ needs.lint.result }}
+          RESULT_TEST: ${{ needs.test.result }}
+        run: |
+          # A required check must either `success` or skip-on-purpose via its own
+          # `if:` guard. No job in this workflow carries an `if:`, so a `skipped`
+          # result means a dependency never ran and is treated as a failure.
+          FAILED=0
+          check_result() {
+            local job="$1" result="$2"
+            case "$result" in
+              success) echo "OK: $job ($result)" ;;
+              skipped) echo "FAIL: $job was skipped because required checks must run"; FAILED=1 ;;
+              *)       echo "FAIL: $job ($result)"; FAILED=1 ;;
+            esac
+          }
+          check_result lint "$RESULT_LINT"
+          check_result test "$RESULT_TEST"
+          exit $FAILED
Evidence
The aggregate gate declares needs: [lint, test] but the validation logic is hard-coded to only two
rendered env vars and two check_result calls; there is no mechanism that automatically validates
every entry in needs.

.github/workflows/tests.yml[128-163]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Required CI` is intended to be the single required check, but it currently hard-codes which `needs.*.result` values it validates. If a new job is added and appended to `needs` but not also added to the env vars and `check_result` calls, the aggregate can still exit 0.

### Issue Context
This workflow explicitly documents that branch protection should only require `Required CI`, so this aggregate must reliably fail whenever any required job fails.

### Fix Focus Areas
- .github/workflows/tests.yml[128-163]

### Implementation notes
- Replace the hard-coded `RESULT_LINT/RESULT_TEST` env vars and `check_result` calls with a dynamic enumeration of `needs`.
- One robust pattern is to pass `NEEDS_JSON: ${{ toJSON(needs) }}` and then parse it in a small Python snippet to enforce:
 - `success` => ok
 - `skipped` => fail (per your current intent)
 - anything else (`failure`, `cancelled`, `timed_out`) => fail
- This ensures the gate automatically covers every entry in `needs` without requiring multiple manual updates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Secret paths excluded ✓ Resolved 🐞 Bug ⛨ Security
Description
The new CodeRabbit and PR-Agent configs exclude keys/** and **/*.key from analysis, so an
accidentally force-added API key file in the repo’s designated key location would not be reviewed by
these tools. Given the repo documentation and .gitignore explicitly treat keys/ and *.key as
the key storage location, excluding them reduces defense-in-depth for secret leakage.
Code

.coderabbit.yaml[R49-56]

+  path_filters:
+    - "!requirements.lock"
+    - "!requirements-dev.lock"
+    - "!output/**"
+    - "!data/api_cache/**"
+    - "!keys/**"
+    - "!**/*.key"
+    - "!.venv/**"
Evidence
The configs explicitly exclude keys/** and *.key, while the repo itself documents keys/ as the
key location and .gitignore marks those paths as sensitive/untracked; excluding them from analysis
makes accidental commits easier to miss.

.coderabbit.yaml[45-59]
.pr_agent.toml[14-33]
README.md[29-38]
.gitignore[41-44]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
AI review tooling is configured to ignore the `keys/` directory and `*.key` files, which are exactly where this repository instructs users to place API keys. If a key file is mistakenly committed, these tools will not flag it.

### Issue Context
- README instructs placing API keys under `keys/`.
- `.gitignore` expects `keys/` and `*.key` to be untracked.

### Fix Focus Areas
- .coderabbit.yaml[45-59]
- .pr_agent.toml[14-33]

### Implementation notes
- Remove `keys/**` and `**/*.key` from CodeRabbit `path_filters`.
- Remove `keys/**` and the `.*\.key$` ignore regex from PR-Agent.
- Keep ignores for generated output and caches if desired, but keep secret-bearing paths visible to reviewers/scanners so accidental commits are caught early.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. CodeQL fork upload risk ✓ Resolved 🐞 Bug ☼ Reliability
Description
The CodeQL workflow runs on pull_request while requesting security-events: write, which is
commonly unavailable to GITHUB_TOKEN on fork-based PRs, leading to missing SARIF uploads or a
failing CodeQL check for external contributors. This undermines PR-time CodeQL signal even though
the workflow is configured to run on PRs.
Code

.github/workflows/codeql.yml[R7-33]

+on:
+  push:
+    branches: [ main ]
+  pull_request:
+    branches: [ main ]
+  schedule:
+    # Weekly, Monday 04:31 UTC. Off the hour to avoid the scheduling stampede,
+    # and it catches newly published queries against unchanged code.
+    - cron: '31 4 * * 1'
+
+permissions:
+  contents: read
+
+concurrency:
+  group: codeql-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
+jobs:
+  analyze:
+    name: Analyze Python
+    runs-on: ubuntu-latest
+    timeout-minutes: 30
+    permissions:
+      contents: read
+      # Required to upload the SARIF results to the code scanning API.
+      security-events: write
+
Evidence
The workflow is configured to run on PRs and explicitly requests security-events: write for SARIF
upload, which is the permission that tends to be constrained for fork PR contexts.

.github/workflows/codeql.yml[7-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`codeql.yml` requests `security-events: write` and runs on `pull_request`. On fork PRs, the token often cannot be granted that permission, so the workflow may not be able to upload code scanning results.

### Issue Context
This repo is public-facing and likely to receive fork PRs; the workflow is explicitly enabled for PRs.

### Fix Focus Areas
- .github/workflows/codeql.yml[7-33]

### Implementation notes
Pick one safe approach:
1) **Skip CodeQL on fork PRs** and rely on `push` + `schedule` for trusted scanning:
  - Add an `if:` guard on the job, for example checking that `github.event.pull_request.head.repo.full_name == github.repository`.
2) **Use `pull_request_target` with strict safety**:
  - Trigger on `pull_request_target`.
  - Checkout the PR head SHA explicitly (no credentials persisted).
  - Keep `build-mode: none` for Python to avoid executing PR code.

Either way, ensure the workflow behavior is deterministic and does not introduce a permanently failing/non-actionable check on fork PRs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread .github/workflows/tests.yml
Comment thread .github/workflows/codeql.yml Outdated
Comment thread .coderabbit.yaml
@gabrielspadon

Copy link
Copy Markdown
Collaborator Author

Removed codeql.yml. It cannot work here, and it was also redundant.

The upload failed with:

Code Scanning could not process the submitted SARIF file: CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled

CodeQL default setup is already configured on this repository, and it covers more than the workflow I added:

default setup (already on) the workflow I added
Languages actions, python python
Query suite extended security-and-quality
Schedule weekly weekly

So the right move is to keep default setup and drop the advanced config, rather than disable default setup to make my file work. The pull request title still mentions CodeQL; the remaining substance is the Required CI aggregate gate, the house-pattern changes to tests.yml, the three AI review configs, and the pre-commit config.

@gabrielspadon

Copy link
Copy Markdown
Collaborator Author

Both Qodo findings were correct and are fixed in b00ee22.

1. The gate did not actually self-maintain. The script enumerated needs.lint.result and needs.test.result into env vars, so a job added to needs: would have been ignored unless someone also edited the script. That is precisely the failure the fan-in exists to prevent, so the comment above it was describing behaviour the code did not have.

It now takes the whole context as NEEDS: ${{ toJSON(needs) }} and iterates, so joining needs is sufficient and nothing else has to change. It also fails closed when needs is empty, which would otherwise pass vacuously.

Exercised against five scenarios before pushing:

needs contents exit
lint success, test success 0
lint success, test failure 1
lint success, test skipped 1
lint success, test success, newjob failure (never named in the script) 1
{} empty 1

The fourth row is the one that mattered: under the old script that combination passed.

2. Key paths are no longer excluded from review. keys/** and **/*.key are removed from greptile.json, .coderabbit.yaml and .pr_agent.toml. Excluding them meant that the one location where a leaked credential would actually land was the one location no reviewer would look at. The exclusions came from adapting sibling configs where those paths do not exist; carrying them here was a mistake.

actionlint exits 0 on the workflow and all three config files parse.

@gabrielspadon
gabrielspadon enabled auto-merge (squash) July 29, 2026 19:05
@gabrielspadon

Copy link
Copy Markdown
Collaborator Author

A follow-up audit caught that my earlier commit fixed three of four places, not four.

b00ee227 removed the key-path exclusions from .coderabbit.yaml and greptile.json, and from .pr_agent.toml's glob list. It left a second exclusion in the same file under a different key:

[ignore]
regex = [
    ".*\\.key$",
]

So Qodo still could not see any *.key file. That matters more here than anywhere else in the fleet: this is the only public repository, and it is currently the only one where Qodo actually reviews (it is paused on the five private siblings). The config's own instructions two sections below read "Treat anything that could commit an API key ... as a defect rather than a nit", which the exclusion directly contradicted.

Removed. [ignore] now carries only glob, and that list is generated artefacts and lockfiles, nothing credential-bearing.

@gabrielspadon
gabrielspadon disabled auto-merge July 29, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant