Skip to content

ci: add CodeQL security analysis workflow#40

Closed
lml2468 wants to merge 1 commit into
mainfrom
ci/add-codeql
Closed

ci: add CodeQL security analysis workflow#40
lml2468 wants to merge 1 commit into
mainfrom
ci/add-codeql

Conversation

@lml2468

@lml2468 lml2468 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What

Add CodeQL static analysis for Go, using the org-level reusable workflow (reusable-codeql.yml).

Why

octo-cli is the only Go repository in the org without CodeQL coverage. As a CLI tool that processes user input (CLI arguments, config files, API responses), it should have the same security scanning as peer repos.

Details

  • Language: go
  • Reusable workflow: Mininglamp-OSS/.github/.github/workflows/reusable-codeql.yml@main
  • Triggers: push to main, PRs (excluding drafts), daily schedule (0 6 * * *), manual dispatch
  • Permissions: minimal (actions: read, contents: read, security-events: write)
  • Top-level permissions: {} for least privilege
  • Concurrency: cancel-in-progress for PRs
  • Draft skip: if: !github.event.pull_request.draft

Follows the exact same caller pattern as octo-server, octo-lib, octo-matter, octo-im, octo-speech, and octo-smart-summary.

Ref: Workflow optimization task T09

Add CodeQL static analysis for Go using the org-level reusable workflow.
Runs on push to main, PRs, daily schedule, and manual dispatch.

octo-cli processes user input (CLI args, config files, API responses) and
should have the same security scanning coverage as other Go repositories
in the org.

Follows the same caller pattern as octo-server, octo-lib, octo-matter,
octo-im, octo-speech, and octo-smart-summary.

Ref: Workflow optimization task T09
@lml2468 lml2468 requested a review from a team as a code owner June 4, 2026 08:51
@github-actions github-actions Bot added the size/S PR size: S label Jun 4, 2026

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Summary: The PR is project-relevant, but it appears to reintroduce a CodeQL workflow that this repository recently removed as duplicate and failing.

🔴 Blocking

🔴 Critical — .github/workflows/codeql.yml:23: This PR restores the same codeql.yml workflow that commit 8d1add7 removed because it duplicated GitHub CodeQL default setup and the custom workflow runs were failing with SARIF upload not enabled. That directly contradicts the PR premise that octo-cli lacks CodeQL coverage. Merging this risks reintroducing failing/noisy CodeQL runs and duplicate scanning. Please either keep the workflow removed and rely on the existing default setup, or provide evidence that default setup has been disabled and this reusable workflow now uploads SARIF successfully for this repository.

💬 Non-blocking

🔵 Suggestion — .github/workflows/codeql.yml:25: @main is mutable. If org policy allows, pinning the reusable workflow to a tag or SHA would make the security workflow more reproducible. This is consistent with the repository’s pinned third-party actions, though existing org reusable workflows already use @main.

✅ Highlights

The workflow uses scoped job permissions at .github/workflows/codeql.yml:28-31, and the draft PR guard plus PR-only cancellation at .github/workflows/codeql.yml:18-24 are reasonable patterns if this workflow is ultimately needed.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review — PR #40 (octo-cli)

Summary

Adds a CodeQL static-analysis caller workflow (.github/workflows/codeql.yml, +31/-0) that delegates to the org-level reusable workflow Mininglamp-OSS/.github/.github/workflows/reusable-codeql.yml@main for Go. Brings octo-cli to parity with the other Go repos in the org. This is a security-positive, low-risk change.

Verification

Claim Status Evidence
Caller follows the established org pattern The caller block is byte-identical to octo-server/.github/workflows/codeql.yml and octo-lib/.github/workflows/codeql.yml (triggers, permissions: {}, concurrency, draft skip, language input, per-job permissions all match).
Reusable workflow exists and accepts language reusable-codeql.yml defines workflow_call.inputs.language (required string); the caller passes language: go.
Least-privilege permissions Top-level permissions: {}; the analyze job grants only actions: read, contents: read, security-events: write — exactly what init/analyze need to upload SARIF.
Pinned/trusted actions in the reusable workflow Reusable workflow pins actions/checkout@de0fac2… (v6.0.2) and github/codeql-action/{init,autobuild,analyze}@9e0d7b8d… (v4) by full commit SHA, runs on ubuntu-24.04, sets timeout-minutes: 30, persist-credentials: false. No floating tags.
Build strategy suits octo-cli octo-cli is a standard Go module (go 1.24.0, root go.mod, cmd/ + internal/). No build-command is passed, so the reusable workflow uses CodeQL Autobuild, which handles a conventional Go module.
Query depth Reusable workflow runs queries: security-extended.
No duplicate/conflicting workflow No pre-existing codeql.yml in .github/workflows/; no concurrency-group or name collision with the existing CI workflows.

Findings

No P0/P1 issues. Permissions, action pinning, and trigger configuration are correct and consistent with the rest of the org.

Notes (non-blocking)

  • nit — paths-ignore + required status check interaction. The pull_request trigger uses paths-ignore (**/*.md, docs/**, .github/ISSUE_TEMPLATE/**). If CodeQL is later configured as a required status check on main, a docs-only PR would skip the workflow and the check would never report, potentially blocking merge. This is the same pattern used by all peer Go repos, so it is an org-wide convention rather than a defect in this PR — flagging only so whoever configures branch protection is aware. No change requested here.
  • info — security-events upload from PRs. Because this is a caller within the same org/repo (not a fork PR), security-events: write and SARIF upload behave as intended. Forked-PR runs would have reduced permissions by GitHub default, which is the desired safe behavior.

Manual-verification call-outs (security-sensitive PR)

Nothing in the diff itself requires manual security verification — it introduces no secrets, no inline run: shell beyond the trusted reusable workflow, and no elevated permissions. The one thing worth a human glance is org-level branch-protection config (whether/how CodeQL becomes a required check), which lives outside this repo and outside this diff.

Verdict

Approving. The change is correct, minimal, matches the proven peer pattern exactly, and improves the repo's security posture.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review — PR #40 (octo-cli)

Correction notice: I previously submitted an Approve on this PR based only on the diff and the org peer-pattern. That was a mistake — I had not verified the repository's current CodeQL state. After verifying against this repo's history and live Actions runs, this review supersedes that approval. Changes are required.

Verdict: Changes Requested

This PR re-introduces a custom CodeQL workflow that conflicts with the GitHub CodeQL default setup that is already enabled on this repository. It is functionally a revert of a deliberate, recent cleanup, and it fails on its own PR run.

P0 — Blocking: conflicts with active CodeQL default setup

Evidence (all verified live):

  1. Default setup is enabled and covers Go.
    GET /repos/Mininglamp-OSS/octo-cli/code-scanning/default-setup
    {"state":"configured","languages":["actions","go","javascript","javascript-typescript","typescript"],"query_suite":"default","updated_at":"2026-05-31T08:28:02Z"}
    It is producing successful analyses on main (categories /language:go, /language:javascript-typescript, /language:actions).

  2. This exact file was removed 3 days ago for this exact reason.
    Commit 8d1add7 ("chore(ci): remove duplicate CodeQL workflow (#33)", 2026-06-01) deleted .github/workflows/codeql.yml. Its message states the custom workflow's runs were ALL FAILURE (Code Security SARIF upload not enabled) while Default Setup CodeQL was ALL SUCCESS. The file this PR adds is byte-for-byte identical to the one that commit removed.

  3. The workflow fails on this PR.
    Run 26941409810 (branch ci/add-codeql), job analyze / CodeQL (go)failure at step Perform CodeQL Analysis with:

    Code Scanning could not process the submitted SARIF file: CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled
    This is the documented incompatibility: a repo cannot run advanced-config CodeQL SARIF upload while default setup is on.

  4. The PR premise is factually incorrect.
    The description states octo-cli "is the only Go repository in the org without CodeQL coverage." It already has CodeQL coverage via default setup (since at least 2026-05-31). The peer repos (octo-server, octo-lib, etc.) presumably use the advanced caller because they have default setup disabled — that precondition does not hold here.

Required to unblock — pick one:

  • Recommended: close this PR / keep the workflow removed. octo-cli already has working CodeQL via default setup; no action needed for coverage.
  • OR, if the team specifically wants advanced config (e.g. security-extended queries, the daily cron, custom config) instead of default setup: first disable CodeQL default setup for this repo (Settings → Code security → Code scanning → switch from Default to Advanced / set default setup to "Not configured"), and include evidence in the PR that the advanced workflow then uploads SARIF successfully. Without that step, this workflow will keep failing on every push/PR/schedule and produce pure noise — the same condition PR #33 cleaned up.

P2 / Non-blocking notes

  • @main reusable-workflow ref is mutable. Pinning to a tag/SHA would improve reproducibility for a security workflow, though all org reusable callers currently use @main, so this is a wider convention question rather than a defect in this PR.
  • The caller's permission scoping, draft guard, and PR-only cancel-in-progress are themselves well-formed — the problem is not the YAML quality, it is that an advanced caller is the wrong tool while default setup is enabled.

Security-sensitive call-out

No secret/permission risk in the diff itself. The security-relevant concern is operational: merging this degrades the security signal (failing/noisy CodeQL runs, duplicate scanning attempts) rather than improving it. A human should confirm the intended CodeQL mode for this repo (default vs. advanced) before any version of this lands.

@lml2468 lml2468 closed this Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants