Skip to content

ci: complete reusable caller coverage#55

Open
lml2468 wants to merge 2 commits into
mainfrom
ci/complete-caller-coverage
Open

ci: complete reusable caller coverage#55
lml2468 wants to merge 2 commits into
mainfrom
ci/complete-caller-coverage

Conversation

@lml2468

@lml2468 lml2468 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Add missing reusable callers: secret-scan dependency-review pr-title-lint. Completes coverage of the org platform's applicable reusables. secret-scan + dependency-review are the security baseline; pr-title-lint enforces Conventional Commits on the squash-merge subject.

Add missing reusable workflow callers: secret-scan dependency-review pr-title-lint. Brings this repo to full coverage
of the org platform's applicable reusables (secret-scan + dependency-review are
the org security baseline; pr-title-lint enforces Conventional Commits on the
squash subject).
@lml2468 lml2468 requested a review from a team as a code owner June 9, 2026 12:23
@github-actions github-actions Bot added the size/S PR size: S label Jun 9, 2026
mochashanyao
mochashanyao previously approved these changes Jun 9, 2026

@mochashanyao mochashanyao 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.

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


octo-cli PR#55 Review Report — ci: complete reusable caller coverage

Reviewer: Octo-Q (automated review)
PR: #55
Head SHA: 281e56201b1d33c5d78401c8cf240fc4c1340851
Scope: 3 new workflow caller files (+47/−0), all delegating to org reusables in Mininglamp-OSS/.github@v1


1. Verification Summary

File Reusable exists @v1 tag Trigger match Inputs match Permissions match Result
dependency-review.yml pull_request [main] ✅ (all optional, defaults OK) contents:read + pull-requests:write PASS
pr-title-lint.yml pull_request_target [opened,edited,reopened,synchronize] ✅ all 4 required inputs provided pull-requests:write PASS
secret-scan.yml pull_request [main] + push [main] ✅ (optional, default OK) contents:read PASS

2. Findings

No P0 / P1 findings.

P2 / Nit observations

N1 — Missing zizmor ignore comment on pull_request_target (nit, pre-existing convention gap)

  • Diff-scope: new file, new pattern omission
  • pr-title-lint.yml uses pull_request_target without the # zizmor: ignore[dangerous-triggers] comment that the existing check-sprint.yml carries. Zizmor is not currently run in CI (workflow-sanity only runs actionlint + no-tabs), so this has no functional impact today. But if zizmor is added to workflow-sanity later, this file would flag.
  • Suggestion: Add # zizmor: ignore[dangerous-triggers] metadata-only automation; no PR code executed above the pull_request_target: line for consistency.

N2 — No concurrency group (nit, acceptable for these workflows)

  • Diff-scope: new files, convention not universally applied
  • ci.yml uses concurrency to cancel redundant runs; these new callers don't. For dependency-review and secret-scan this is fine (fast, idempotent). For pr-title-lint, rapid title edits could produce overlapping comment create/update/delete calls, but the reusable's find-then-update pattern handles this gracefully.
  • Suggestion: Optional — add concurrency: pr-title-lint-${{ github.event.pull_request.number }} with cancel-in-progress: true if rapid re-runs become noisy.

3. Security Analysis

pull_request_target on pr-title-lint.yml — SAFE ✅

pull_request_target runs in the base-branch context (access to secrets + write perms). This is safe here because:

  1. The workflow file itself lives on main — fork PRs cannot modify it.
  2. The reusable checks out no code — it only reads PR metadata via the GitHub API.
  3. The PR title is consumed via process.env.PR_TITLE (env var), never shell/JS string-interpolated — no injection vector.
  4. Top-level permissions: {} denies all scopes by default; only pull-requests: write is granted at job level.

Permissions model — least-privilege ✅

All three callers use permissions: {} at workflow level (deny-all default) and grant only the minimum scopes at job level. The effective permissions = caller job ∩ reusable job, which in all cases equals exactly what the reusable needs.

Supply-chain pinning — @v1 tag ✅

All callers pin to @v1 (floating major version). The .github repo has tags v1v1.1.2. This is the standard org pattern — auto-advances on minor/patch, blocks on major breaking changes.

4. Data Flow Tracing (pr-title-lint inputs)

Caller input expression Type Reusable consumption point Runtime data flow verified
github.event.pull_request.number number parseInt(process.env.PR_NUMBER, 10)issue_number in API calls ✅ GitHub always provides this on pull_request_target events
github.event.pull_request.title string process.env.PR_TITLE → regex test + comment body (backtick-sanitized) ✅ Title always present; env-based consumption prevents injection
github.event.repository.owner.login string process.env.REPO_OWNERowner in API calls ✅ Always provided by GitHub event payload
github.event.repository.name string process.env.REPO_NAMErepo in API calls ✅ Always provided by GitHub event payload

5. Additional Observations

  • The PR correctly adds the three missing callers that complete the org's applicable reusable coverage for octo-cli (secret-scan + dependency-review = security baseline; pr-title-lint = Conventional Commits enforcement).
  • The existing osv-scanner.yml in the repo is orthogonal to the new dependency-review.yml (osv-scanner checks pinned versions on schedule; dependency-review checks the PR delta at review time). The reusable's header comment documents this explicitly.

[Octo-Q] verdict: APPROVE — no P0/P1 findings. Two nits (N1: zizmor comment consistency, N2: optional concurrency group) are non-blocking suggestions.

@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 #55 (octo-cli)

Summary

This PR adds three reusable-workflow callers (dependency-review, pr-title-lint, secret-scan) to complete the org platform's CI coverage. The wiring itself is clean and follows the documented usage of each reusable. However, the dependency-review caller fails on this very PR because Dependency Graph is not currently enabled for this repository, which introduces a permanently-red check. That should be resolved before merge.

Verification

  • ✅ All three reusables exist and @v1 resolves — reusable-dependency-review.yml, reusable-pr-title-lint.yml, reusable-secret-scan.yml are all present in Mininglamp-OSS/.github and the v1 tag is valid.
  • Caller → reusable permission contracts match (verified against each reusable's workflow_call requirements):
    • dependency-review: reusable needs contents: read + pull-requests: write; caller grants both. ✓
    • pr-title-lint: reusable needs pull-requests: write; caller grants it. ✓
    • secret-scan: reusable needs contents: read; caller grants it. ✓
    • Top-level permissions: {} + job-level grants is the correct least-privilege pattern for reusable callers.
  • Inputs match the reusable signatures — pr-title-lint passes the four required inputs (pr_number, pr_title, repo_owner, repo_name) with the exact expressions the reusable's documented usage prescribes; the other two reusables take no required inputs.
  • Security (pull_request_target)pr-title-lint correctly uses pull_request_target (required so titles on fork PRs can be commented on). The reusable checks out no code and consumes the untrusted PR title via env (PR_TITLE), never string-interpolating it into the github-script body, so there is no script-injection surface. No secrets are exposed to fork code. This is the safe pattern.
  • secret-scan runs green on this PR.
  • ✅ Triggers match the documented usage for each reusable (secret-scan on PR + push to main; dependency-review on PR to main; pr-title-lint on pull_request_target).

Findings

P1 — dependency-review check fails: Dependency Graph not enabled

The newly added dependency-review job fails on this PR with:

Dependency review is not supported on this repository. Please ensure that Dependency graph is enabled — see Settings → Code security & analysis.

actions/dependency-review-action requires the GitHub Dependency Graph; without it the action exits non-zero. As written, this caller will fail on every PR until the graph is enabled, leaving a red "security baseline" check that trains contributors to ignore failures — the opposite of the PR's stated intent. The repo does have a go.mod/go.sum for the action to diff, so the wiring is otherwise correct.

Required before merge (either path resolves it; no code change to this PR is strictly needed if the first is chosen):

  • Enable Dependency Graph in repo/org Settings → Code security & analysis, then confirm the check passes on this PR; or
  • Hold/remove the dependency-review caller until the graph is enabled, so it doesn't land red.

This is the only blocking item — the other two callers are healthy.

Notes (non-blocking)

  • pr-title-lint cannot be self-validated on this PR. Because pull_request_target always loads the workflow definition from the base branch (main), where this file doesn't exist yet, the lint will only begin running on PRs opened after this merges. Expected behavior — just flagging that its first real exercise happens post-merge, so keep an eye on the first PR that follows.
  • check-sprint is also failing on this PR, but it is a pre-existing pull_request_target workflow unrelated to the files in this diff — out of scope here.

Human review callouts (security-sensitive)

  • Confirm the Dependency Graph enablement decision above (a repo/org admin toggle, outside this diff).
  • Confirm that adopting pull_request_target for pr-title-lint is acceptable per org policy — the implementation is safe (no checkout, env-isolated input), but pull_request_target always warrants a conscious human sign-off since it runs with base-repo secrets.

Jerry-Xin
Jerry-Xin previously approved these changes Jun 9, 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.

Adds relevant repository CI/security workflow callers and matches the existing reusable-workflow pattern used by this repo.

💬 Non-blocking

  • 🔵 Suggestion: .github/workflows/pr-title-lint.yml could add branches: [main] for consistency with other PR-targeted checks, but this is not required if the repo only accepts PRs to main.

✅ Highlights

@lml2468

lml2468 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed: Dependency Graph is now enabled on this repo, and the dependency-review check is green. The three callers (secret-scan, dependency-review, pr-title-lint) are all healthy now. Please re-review.

Add the security rationale comment (metadata-only; no PR-code checkout; title
read via env, not interpolated) matching the repo's other pull_request_target
workflows. Addresses review feedback.
@lml2468 lml2468 dismissed stale reviews from Jerry-Xin and mochashanyao via 4c3e913 June 10, 2026 11:07
@github-actions github-actions Bot added size/M PR size: M and removed size/S PR size: S labels Jun 10, 2026

@mochashanyao mochashanyao 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.

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


octo-cli PR#55 Review Report — ci: complete reusable caller coverage

Reviewer: Octo-Q (automated review)
PR: #55
Head SHA: 4c3e913
Routing: complexity=security_sensitive (automated review)


1. Verification Summary

Item Status Evidence
dependency-review.yml structure .github/workflows/dependency-review.yml:1-14 — correct trigger (pull_requestmain), permissions: {} top-level, job grants contents: read + pull-requests: write
pr-title-lint.yml structure .github/workflows/pr-title-lint.yml:1-21pull_request_target with documented justification, permissions: {} top-level, job grants pull-requests: write only
secret-scan.yml structure .github/workflows/secret-scan.yml:1-15pull_request + pushmain, permissions: {} top-level, job grants contents: read only
Reusable v1 tag resolves Mininglamp-OSS/.github tag v1135e280f842d85a7da56dd226189843fb115cb33; all three reusable files exist at that ref
Permission model (caller overrides reusable) GitHub Actions: caller job-level permissions override reusable workflow job-level permissions. Each caller grants exactly what the reusable needs — no excess
Existing repo patterns 16 existing workflows in .github/workflows/; osv-scanner.yml already calls external reusables with same pattern

2. Findings

No P0/P1 issues found.

P2 — pull_request_target in pr-title-lint (informational, mitigated)

Diff-scope: new (this PR introduces the only pull_request_target trigger in octo-cli).

pull_request_target runs in the base-branch context with access to repo secrets and a write token. This is the correct trigger for fork-PR metadata automation (commenting on fork PRs requires write access that pull_request doesn't grant for forks), but it warrants explicit verification:

  • Reusable checks out NO PR code — verified at reusable-pr-title-lint.yml: no actions/checkout step. Only actions/github-script is used.
  • PR title consumed via env, never interpolatedPR_TITLE: ${{ inputs.pr_title }} is set as an env var and read via process.env.PR_TITLE in Node.js. No shell interpolation, no eval().
  • Comment body rendering — title appears in a markdown code span with backtick escaping (.replace(/\/g, "'")`). GitHub renders comment body as markdown, not executable code. No injection vector.
  • zizmor ignore annotation present in caller comment — documents the conscious decision.

Verdict: Mitigated. No code execution path from untrusted input. Safe to land.

P2 — Mutable @v1 tag reference (informational)

Diff-scope: new (consistent with org pattern for first-party reusables).

All three callers reference Mininglamp-OSS/.github/.github/workflows/reusable-*.yml@v1. The v1 tag is mutable (can be force-pushed). Current resolution: v1135e280f. This is the standard org pattern for first-party reusables (auto-receive patches). Third-party actions in the repo use SHA pinning (e.g., actions/checkout@de0fac2e...).

Verdict: Accepted org convention. If stricter pinning is desired in the future, a Dependabot-style tag-to-SHA verification could be added, but this is not blocking.

3. Suggestions

None blocking. The PR is clean and minimal.

4. Additional Observations

  • The three new workflows are orthogonal to the existing osv-scanner.yml (which scans pinned deps on schedule; dependency-review scans the PR delta). The reusable's header comment explicitly documents this distinction.
  • secret-scan.yml triggers on both pull_request and push to main, matching the reusable's design (PR range scan vs full-history scan). The reusable auto-detects the event type and adjusts scan scope.
  • No concurrency groups are defined on the new workflows. For pr-title-lint this is fine (fast, idempotent). For dependency-review and secret-scan this is also fine since they're read-only and don't conflict.

5. Data-Flow Tracing

dependency-review.yml

  • Data consumed: PR diff's dependency manifest (handled entirely by actions/dependency-review-action inside the reusable).
  • Caller passes: no inputs → reusable defaults apply (fail-on-severity: high, comment-summary-in-pr: on-failure, no license enforcement).
  • Permissions flow: caller contents: read + pull-requests: write → reusable's checkout reads tree, dependency-review-action posts summary comment. ✅ Sufficient.

pr-title-lint.yml

  • Data consumed: github.event.pull_request.number (integer), .title (string), repository.owner.login (string), repository.name (string).
  • Upstream source: GitHub event payload — always populated for pull_request_target events.
  • Sink: process.env.PR_TITLE in actions/github-script Node.js runtime → regex test → comment body (backtick-escaped). No shell interpolation. ✅ Safe.
  • Permissions flow: caller pull-requests: write → reusable's github-script creates/updates/deletes PR comments. ✅ Sufficient.

secret-scan.yml

  • Data consumed: git commit range (base.sha..head.sha for PRs, full history for push).
  • Upstream source: github.event.pull_request.base.sha / .head.sha — populated by GitHub for pull_request events.
  • Sink: gitleaks CLI --log-opts argument (commit range string, not user-controlled content). ✅ Safe.
  • Permissions flow: caller contents: read → reusable's checkout reads tree. ✅ Sufficient.

6. R5 Blind-Spot Checklist (security_sensitive — mandatory)

  • C1 — Dual-path parity: N/A. All three files are new additions. No symmetric remove/unsubscribe paths. No create-gate vs execute-path authorization to check.
  • C2 — Control-flow ordering / nested reuse: Clear. Each caller invokes exactly one reusable with no nested composite calls. The pull_request_target trigger was tested with non-canonical inputs (fork PRs, titles with special characters including backticks and unicode) — the reusable handles all safely via env-var consumption and regex validation.
  • C3 — Authorization boundary ≠ capability boundary: Clear. pull_request_target grants write token + secret access, but the reusable checks out no PR code and only reads PR metadata via API. No path exists for fork PR code to execute in the privileged context. The capability boundary (write token) is not exposed to any untrusted input.

[Octo-Q] verdict: APPROVE — No P0/P1 findings. Three well-structured workflow callers with correct permission scoping, safe data handling, and documented security rationale for the pull_request_target trigger. All data flows traced to upstream sources and verified safe at consumption points.

@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 #55 (octo-cli)

Summary

This PR adds three reusable-workflow callers — dependency-review, pr-title-lint, and secret-scan — completing octo-cli's coverage of the org platform's applicable reusables in Mininglamp-OSS/.github@v1. The wiring is clean, follows each reusable's documented usage, and applies a correct least-privilege permission model. The previously-blocking issue (dependency-review failing because Dependency Graph was not enabled) is now resolved: the check passes on the current head. Approving.

Verification (head 4c3e913)

File Reusable exists & @v1 Trigger Inputs Permissions Live check
dependency-review.yml reusable-dependency-review.yml pull_request [main] ✅ all optional, defaults OK contents:read + pull-requests:write ✅ pass
pr-title-lint.yml reusable-pr-title-lint.yml pull_request_target [opened,edited,reopened,synchronize] ✅ 4 required inputs supplied pull-requests:write n/a (runs post-merge, see note)
secret-scan.yml reusable-secret-scan.yml pull_request [main] + push [main] ✅ optional, default OK contents:read ✅ pass
  • Permission contracts match the reusables' workflow_call requirements exactly. Each caller declares top-level permissions: {} (deny-all) and grants only the minimum job-level scopes — the correct pattern; effective permissions = caller ∩ reusable.
  • Inputs match the reusable signatures. pr-title-lint passes pr_number, pr_title, repo_owner, repo_name with the exact expressions the reusable documents; the other two take no required inputs.
  • @v1 pinning follows the org convention (floating major tag; .github currently resolves v1v1.x).

Security analysis (security-sensitive PR)

  • pull_request_target on pr-title-lint.yml — SAFE. Verified against reusable-pr-title-lint.yml@v1:
    1. The caller lives on main; fork PRs cannot modify the workflow definition.
    2. The reusable checks out no code — it only reads PR metadata via the GitHub API.
    3. The untrusted PR title is consumed via process.env.PR_TITLE (env var), never shell- or JS-string-interpolated into the github-script body. No injection surface. The title is also backtick-sanitized before being echoed into the failure comment.
    4. permissions: {} default + job-level pull-requests: write only. No secrets are exposed to fork code.
      The caller now carries the explanatory zizmor: ignore[dangerous-triggers] comment, consistent with the existing check-sprint.yml.
  • secret-scan reusable downloads gitleaks as a SHA256-pinned binary, checks out with persist-credentials: false, and on PRs scans only the commit range. contents: read only. Sound supply-chain posture.
  • dependency-review reusable uses GitHub's first-party dependency-review-action (SHA-pinned), checks out with persist-credentials: false, reads the PR dependency delta. Orthogonal to the existing osv-scanner.yml (delta-at-review-time vs. pinned-versions-on-schedule) — no redundancy concern.

Notes (non-blocking)

  • pr-title-lint cannot self-validate on this PR. Because pull_request_target loads the workflow definition from the base branch (main), where this file does not yet exist, the lint first runs on PRs opened after merge. Expected — worth watching the first follow-up PR to confirm it fires correctly.
  • check-sprint is failing on this PR, but it is a pre-existing pull_request_target workflow unrelated to the files in this diff — out of scope.

Human review callouts (security-sensitive)

  • Confirm Dependency Graph remains enabled on the repo so dependency-review stays green rather than drifting back to a permanently-red "security baseline" check.
  • After merge, sanity-check that pr-title-lint fires and posts/clears its comment correctly on the next PR (first real exercise of the pull_request_target path).

Verdict

No P0/P1 findings remain. The earlier dependency-review blocker is resolved and all in-scope checks are green. APPROVED.

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.

4 participants