Skip to content

feat(codeql): add the native CodeQL scan dispatch handler (not wired up yet) - #1776

Merged
seonghobae merged 4 commits into
mainfrom
claude/codeql-scan-dispatch-handler
Sep 3, 2026
Merged

feat(codeql): add the native CodeQL scan dispatch handler (not wired up yet)#1776
seonghobae merged 4 commits into
mainfrom
claude/codeql-scan-dispatch-handler

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Summary

Step 2 of #1772's implementation follow-up (design ADR, still open for review). This is the native execution half of the dispatch+poll architecture: a new .github/workflows/codeql-scan-dispatch.yml that runs github/codeql-action outside any required-workflow context, since GitHub categorically refuses to admit init/analyze inside one (docs/doctoring/codeql-pr-required-workflow-always-fails.md).

  • validate-dispatch re-authenticates the dispatch actor/sender against the same OPENCODE_REPOSITORY_DISPATCH_ACTOR/_TARGETS org-variable allowlist opencode-review-dispatch.yml already uses, validates the matrix payload shape, and cross-checks every supplied field against a live gh api pulls/{n} read.
  • scan re-validates live PR metadata again immediately before privileged work (closing the TOCTOU window between jobs — mirrors opencode-review-dispatch.yml's "Validate pull request head repository trust" step), materializes the target repo's exact head SHA via manual git (mirroring strix.yml's isolation posture — no actions/checkout with a foreign token), runs codeql-action with zero source-root override so it scans $GITHUB_WORKSPACE exactly the way codeql-pr.yml and scheduled-security-scan.yml already do today, gates on scripts/ci/codeql_sarif_gate.py fetched at the exact dispatching commit (github.workflow_sha), and publishes a codeql-dispatch/<language> commit status back onto the target repo using the same multi-token fallback chain strix.yml uses.

Depends on #1774 landing first (not a hard technical blocker for this PR's own tests — this workflow only fires on repository_dispatch, and nothing dispatches to it yet — but it fetches scripts/ci/codeql_sarif_gate.py from that PR at runtime, so #1774 should merge before this is ever wired up).

Deliberately has no workflow_dispatch trigger. An earlier draft added one for manual QA before wiring this into codeql-pr.yml, but tests/test_required_workflow_queue_contract.py::test_no_central_workflow_exposes_branch_selected_manual_dispatch forbids workflow_dispatch on every central workflow — it would let a caller run this token-minting, cross-repo-status-publishing workflow from an arbitrary non-default ref, defeating the trusted-source pinning the whole design depends on. A real repository_dispatch POST (which always runs the default-branch version) is the way to exercise this end-to-end.

NOT YET WIRED UP. codeql-pr.yml does not dispatch here yet — that rewrite is the highest-blast-radius part (it touches the org's central required workflow and cannot be tested live before merging), and is a deliberately separate follow-up PR so this handler gets reviewed on its own first. Opening as draft for exactly that reason.

Test plan

  • coverage run -m pytest tests && coverage report --show-missing — 2666 passed, 1 skipped, 100% coverage.
  • interrogate — 100% docstring coverage.
  • New tests/test_codeql_scan_dispatch_workflow_contract.py: bash -n syntax check on every run: block, structural assertions, and functional exercises of the real validate-dispatch shell logic (actor mismatch, target not allowlisted, malformed matrix, stale head SHA, closed PR, and the accepting happy path) against a faked gh.
  • python3 -c "import yaml; yaml.safe_load(open('.github/workflows/codeql-scan-dispatch.yml'))" — parses.
  • Not yet tested against a real repository_dispatch POST — needs a maintainer or a follow-up to fire one manually before this is trusted enough to wire into codeql-pr.yml.

🤖 Generated with Claude Code

…up yet)

Step 2 of ADR 0025's implementation follow-up: the native execution half of
the dispatch+poll design that lets codeql-pr.yml stay required-workflow-safe
while the actual codeql-action work runs unrestricted in .github.

codeql-scan-dispatch.yml mirrors the proven validate/checkout/publish
patterns already used by strix.yml and opencode-review-dispatch.yml:
- validate-dispatch re-authenticates the dispatch actor/sender against the
  same OPENCODE_REPOSITORY_DISPATCH_ACTOR/_TARGETS allowlist those handlers
  already use, validates the matrix payload shape, and cross-checks every
  supplied field against a live `gh api pulls/{n}` read before trusting it.
- scan re-validates live PR metadata again immediately before the privileged
  work (closing the TOCTOU window between jobs), materializes the target
  repo's exact head SHA via manual git (matching strix.yml's isolation
  posture -- no actions/checkout with a foreign token), runs codeql-action
  with zero source-root override (so it scans $GITHUB_WORKSPACE exactly the
  way codeql-pr.yml and scheduled-security-scan.yml already do), gates on
  scripts/ci/codeql_sarif_gate.py fetched at the exact dispatching commit,
  and publishes a codeql-dispatch/<language> commit status back onto the
  target repo with the same multi-token fallback chain strix.yml uses.

Deliberately has NO workflow_dispatch trigger: an early draft added one for
manual testing, but tests/test_required_workflow_queue_contract.py's
test_no_central_workflow_exposes_branch_selected_manual_dispatch forbids
workflow_dispatch on every central workflow, since it would let a caller run
this token-minting, cross-repo-status-publishing workflow from an arbitrary
non-default ref. A real repository_dispatch POST is the way to test this
end-to-end before wiring it into codeql-pr.yml.

NOT YET WIRED UP: codeql-pr.yml does not dispatch here yet. That rewrite --
the highest-blast-radius part, since it touches the org's central required
workflow and cannot be tested live before merging -- is a separate follow-up
PR, deliberately kept out of this change so this handler can be reviewed on
its own first.

New tests/test_codeql_scan_dispatch_workflow_contract.py checks bash syntax
on every run: block, structural invariants, and exercises the real
validate-dispatch shell logic (actor/target/matrix/live-metadata rejection
paths and the accepting happy path) against a faked `gh`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: e832804f-110b-4b2e-8a96-c8107c7cc802

📥 Commits

Reviewing files that changed from the base of the PR and between 0e195d3 and e299761.

📒 Files selected for processing (2)
  • .github/workflows/codeql-scan-dispatch.yml
  • tests/test_codeql_scan_dispatch_workflow_contract.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

seonghobae and others added 2 commits September 3, 2026 13:15
Same class of issue Devin flagged on #1774: this file and its test cited
docs/adr/0025-codeql-required-workflow-dispatch-architecture.md, which only
exists on the separate, still-unmerged .github#1772 branch. Point at the PR
instead of a file path that may or may not exist yet regardless of merge
order.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…owlist

Drafting the codeql-pr.yml dispatch step surfaced a real gap here: this
handler reused vars.OPENCODE_REPOSITORY_DISPATCH_TARGETS, a ~12-repo
allowlist that scopes a deliberately gradual OpenCode review rollout.
Confirmed live (gh api orgs/ContextualWisdomLab/rulesets/18156473) that
ruleset 18156473 covers ~ALL org repos except noema/.github/IRT-bibliography-set.
Reusing the narrower list would have silently broken CodeQL dispatch for
every repo not already on the OpenCode rollout list, the moment this handler
gets wired up and re-admitted to the ruleset.

Keeps the actor-identity check (same token-exchange mechanism as
opencode-review-dispatch.yml) but replaces the target-repo allowlist with the
existing ^ContextualWisdomLab/ regex check further down -- CodeQL is meant to
run for every org repo, not a curated subset.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 4 potential issues.

Devin Review


if [ "$live_state" != "open" ] ||
[ "$live_base_repository" != "$TARGET_REPOSITORY" ] ||
[ "$live_head_repository" != "$TARGET_REPOSITORY" ] ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Fork pull requests never scan

For fork-based contributions, live_head_repository rejects the valid pull request before scanning. The required CodeQL check can never pass.

Prompt for agents
Update .github/workflows/codeql-scan-dispatch.yml so valid fork-based pull requests can be scanned without weakening exact-head validation. The initial and pre-scan metadata checks must validate the live base repository, head repository identity, refs, state, and exact SHA. Materialize the validated fork head safely, for example through the base repository's pull-request head ref with an explicit post-fetch SHA equality check, or through the validated head repository with an appropriately scoped read token. Add contract tests for an accepted fork PR, a changed fork head, and a mismatched fetched commit.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

TARGET_REPOSITORY: ${{ needs.validate-dispatch.outputs.target_repository }}
HEAD_SHA: ${{ needs.validate-dispatch.outputs.head_sha }}
LANGUAGE: ${{ matrix.language }}
GATE_OUTCOME: ${{ steps.gate.outcome }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing evidence still passes polling

When SARIF upload fails after a clean gate, GATE_OUTCOME still publishes success. The required check can pass without audit evidence.

Prompt for agents
Bind the published CodeQL status to the complete scan result, not only steps.gate.outcome. Give the SARIF artifact step an id and make publication return error unless analysis, gating, and required evidence preservation all completed successfully. Preserve failure statuses for actual Medium+ findings while distinguishing infrastructure or artifact failures. Add an executable contract test where the gate succeeds but artifact upload fails and assert that no success status is published.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +165 to +170
matrix_json="$(printf '%s' "$SUPPLIED_MATRIX" | jq -c '.' 2>/dev/null || true)"
if [ -z "$matrix_json" ] ||
[ "$(printf '%s' "$matrix_json" | jq 'type == "array" and length > 0')" != "true" ] ||
[ "$(printf '%s' "$matrix_json" | jq '[.[] | select((.language | type == "string") and (.language | test("^[a-z0-9-]+$")) and (."build-mode" | type == "string"))] | length == ($ARGS.positional[0] | tonumber)' --args "$(printf '%s' "$matrix_json" | jq 'length')")" != "true" ]; then
printf '::error::CodeQL scan dispatch matrix was missing, empty, or contained an entry without a valid language/build-mode. matrix=%s\n' "${SUPPLIED_MATRIX:-<empty>}"
exit 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Matrix contract remains too broad

Validation accepts arbitrary syntactic languages and string build modes. Constrain supported combinations before wiring this handler to the required workflow.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

name: CodeQL dispatch scan (${{ matrix.language }})
needs: validate-dispatch
runs-on: ubuntu-latest
timeout-minutes: 30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Scan timeout lacks production evidence

All languages share a 30-minute timeout. Run representative repository canaries before making this handler a required merge gate.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@seonghobae
seonghobae merged commit c594efa into main Sep 3, 2026
8 of 22 checks passed
@seonghobae
seonghobae deleted the claude/codeql-scan-dispatch-handler branch September 3, 2026 08:03
seonghobae added a commit that referenced this pull request Sep 3, 2026
* feat(codeql): rewrite codeql-pr.yml as dispatch+poll (not wired to the ruleset)

Step 3 of ADR 0025's implementation follow-up
(docs/adr/0025-codeql-required-workflow-dispatch-architecture.md, .github#1772,
still open): the last and highest-risk piece, since this is the org's central
required-workflow file. Removes every github/codeql-action reference (the
platform restriction root-caused in
docs/doctoring/codeql-pr-required-workflow-always-fails.md and fixed by
removing this file from ruleset 18156473 in #1767) -- this PR does NOT
re-admit it to the ruleset, so merging carries zero required-workflow
admission risk; it can only self-trigger on .github's own PRs until someone
explicitly does that re-admission as a separate, later step.

- detect-languages: unchanged.
- analyze-head: two sequential steps in ONE job (mirroring
  opencode-review.yml's opencode-review-target job exactly) -- "Request
  current-head CodeQL scan dispatch" then "Fail closed without a current-head
  CodeQL dispatch verdict". Dispatch+poll live in the same job, not two
  jobs linked by `needs:`, specifically so a dispatch failure fails the job
  directly with no needs-based skip to reason about.
- analyze-merge: deleted. Required nowhere per PR #1766; migrating it doubles
  this change's risk for a check that gates nothing today (ADR's explicit
  scope decision).

Two bugs caught and fixed during implementation, before either was pushed:
1. A job-level `if:` on analyze-head would have reintroduced the exact
   unexpanded-matrix-name bug live evidence (run 33708209086) already proved
   real -- caught by the existing
   test_codeql_pr_gates_analyze_head_at_step_level_not_job_level contract
   test. Fixed by keeping analyze-head's admission unconditional (matching
   the original's proven-safe `needs: detect-languages` with no job-level
   `if:`) and gating only at step level.
2. An initial two-job (dispatch-analysis + analyze-head) split would have let
   analyze-head's matrix duplicate the dispatch N times (once per language),
   each carrying the full language matrix -- triggering N redundant
   full-matrix scans on the .github side. Fixed by merging dispatch+poll into
   one job and restricting the dispatch step to fire from only the first
   matrix shard via `matrix.language == fromJSON(needs.detect-languages.outputs.matrix).include[0].language`.

tests/test_codeql_pr_workflow_contract.py rewritten for the new structure
(previously pinned the old codeql-action/inline-SARIF-gate shape byte-for-byte).
tests/test_docs_only_pr_runner_admission.py's job-vs-step-level gating test
updated to match the new two-step shape; its core assertion (no job-level
`if:` on analyze-head) is unchanged and still enforced.

Depends on #1776 (the native dispatch handler)
existing before this dispatch step can ever succeed against a real PR --
opened as draft for that reason, and because this is genuinely untestable
live before merge.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(codeql): dispatch per-shard so a dispatch failure fails closed, not silently

Peer review on #1778 found a real gap: only the first matrix shard dispatched
(carrying the full language matrix), so if THAT dispatch failed, every other
shard had no way to know -- each would poll the full 3-hour deadline before
self-timing-out for a scan that was never actually requested. A repo with 3
CodeQL languages could turn one dispatch failure into ~9 wasted runner-hours,
working directly against the org's active 60-job-ceiling capacity fight.

Fixed by having every shard dispatch, but only its own single language (not
the full matrix): N single-language dispatches cost the same total
.github-side work as one N-language dispatch, while letting each shard read
its own steps.dispatch.outcome and fail closed immediately instead of only
detecting the failure 3 hours later.

The peer's second finding (scope the concurrency group by exact head SHA,
mirroring opencode-review.yml) does NOT apply here as a drop-in fix:
tests/test_required_workflow_queue_contract.py::test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs
explicitly requires codeql-pr.yml's group to omit head SHA, because this
file has no dedicated cancel-on-close cleanup job. Adding head SHA without
one would let a stale in-flight run for a superseded head survive a close
event indefinitely (it and the closing run would land in different groups
and never cancel each other) -- opencode-review.yml can safely add head SHA
only because it also runs a separate cancel-superseded-opencode-review-runs
job that sweeps stale runs via direct API calls regardless of head SHA.
Documented the real, narrower residual risk in a code comment and left it as
a tracked follow-up requiring a dedicated cleanup job, not a one-line group
change that would regress an existing, deliberately-tested invariant.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(codeql): verify dispatch-status creator identity, not just context

codeql-pr.yml's poll step matched a commit status by context alone
("codeql-dispatch/<language>"), which ADR 0025's own Security
considerations section already flagged as unresolved: anyone with
statuses:write on the target repository can publish an arbitrary
context, so a malicious PR could forge its own passing status and
skip being scanned entirely.

codeql-scan-dispatch.yml mints its publishing token via the same OIDC
audience (opencode-github-action) opencode-review-dispatch.yml uses,
so the legitimate status always carries that app's bot identity.
Mirror opencode-review.yml's existing opencode-agent/opencode-agent[bot]
creator check in the poll's jq filter instead of trusting the context
name alone.

Adds two real-shell-exec regression tests against a faked `gh`: one
proving a forged success status from another creator is ignored in
favor of the legitimate (here, failing) verdict, one proving the
legitimate creator's status is accepted normally.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@github-project-automation github-project-automation Bot moved this from Todo to Done in naruon Platform Roadmap Sep 3, 2026
seonghobae added a commit that referenced this pull request Sep 3, 2026
…sitive_log (#1808)

Skip json.loads() for log lines that obviously can't be JSON (checked
via a first-character set), avoiding expensive JSONDecodeError exception
handling on large non-JSON CI log files. Scalar JSON values (numbers,
true/false/null) still parse successfully but fall through to the
unstructured redactor rather than round-tripping through json.dumps,
matching existing behavior for those lines.

Also fixes a pre-existing bug this change newly exercises:
_consume_sensitive_assignment's unquoted-value scanner didn't stop at
a bare quote character, so an assignment embedded inside an outer
JSON-string literal (e.g. "token=secret123456789") silently swallowed
the closing quote into the redacted value, corrupting line structure.

Clean rebuild of #1751 (branch bolt/optimize-json-decode-8663451122774961708):
that branch's own diff against current main is 91 files / 7805 deletions,
including deleting codeql-scan-dispatch.yml and gutting codeql-pr.yml back
toward a pre-#1772/#1774/#1776/#1778 state -- its merge-base with main
(0c9a5ab) is 12 commits stale and its own in-branch "Merge branch 'main'"
commit did not actually pick up main's content correctly. This branch
carries forward only #1751's real, intended change (verified against its
isolated file diff) rebuilt on current main, plus the one pre-existing
bug that change exposed.
seonghobae added a commit that referenced this pull request Sep 3, 2026
dependabot#1517 and #1518 proposed this bump against codeql-pr.yml's
inline init/analyze steps, but those steps moved to
codeql-scan-dispatch.yml during the CodeQL dispatch+poll rebuild
(#1772/#1774/#1776/#1778) after the PRs were opened -- both are now
DIRTY (git can't find the lines they target). Applying the same
verified v4.37.9 SHA (cdf488f595d80d6e07e03d4674febd5ab45fa938,
confirmed against github/codeql-action's own tag ref) at all 12 live
call sites instead: codeql-scan-dispatch.yml, scheduled-security-scan.yml,
scorecard-analysis.yml, python-security.yml, sast-semgrep.yml,
secret-scan.yml, security-scan.yml (x3).

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae added a commit that referenced this pull request Sep 5, 2026
* fix(codeql): serialise the dispatched scan matrix with toJSON

codeql-pr.yml sends client_payload.matrix as an array, and the dispatch handler
assigned it straight into env:, where a value must be a scalar. GitHub rejects
the step with "A sequence was not expected", so the step dies before running any
of its script and the dispatched scan is skipped. The handler has 0 successes
against 136 failures since #1776 added it.

The validate step already reads the value through jq and checks
`type == "array" and length == 1`, so JSON text is what it was written to
consume; no consumer changes. Dropping the `|| ''` fallback is safe because an
absent matrix yields the string "null", which fails the same array check and
reaches the existing error path.

Neither yaml.safe_load nor actionlint 1.7.12 reports this file as invalid -- it
is an Actions template rule rather than YAML syntax, so only GitHub's validator
rejects it and no local gate catches the class. The added string contract test is
therefore the only guard that runs before a dispatch does.

Refs #1925

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(codeql): the matrix env: failure is step-scoped, not a whole-file rejection

The validate-dispatch job does get a runner and its first steps run; GitHub
rejects only the step whose env: receives the array, when that env: is
evaluated. Docstring wording corrected to match the observed job timeline.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit f90c23b3c0386e22528cc1ab8680c4a31fa630b9)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant