Skip to content

fix(ci): exclude vendored source comments with the shared scanner - #7469

Merged
devantler merged 3 commits into
mainfrom
codex/ksail-vendored-todos-7468
Oct 3, 2026
Merged

devantler merged 3 commits into
mainfrom
codex/ksail-vendored-todos-7468

Conversation

@devantler

Copy link
Copy Markdown
Contributor

🤖 Generated by the Agentic Engineer

Why

Vendored source comments should not create maintenance work for KSail, while comments in owned source must remain visible.

What

Use the released shared vendor filter and canonical catalogue. Verify the actual scanner excludes only root vendor directories and keeps owned, nested, and nearby paths, with controls that detect broken filtering and changed issue contents. Existing project integration remains unchanged.

Fixes #7468

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

✅⚠️MegaLinter analysis: Success with warnings

⚠️ BASH / bash-exec - 1 error
Results of bash-exec linter (version 5.3.9)
See documentation on https://megalinter.io/10.1.0/descriptors/bash_bash_exec/
-----------------------------------------------

✅ [SUCCESS] .github/actions/cleanup-hetzner/cleanup-hetzner.sh
✅ [SUCCESS] .github/actions/cleanup-hetzner/cleanup-hetzner.test.sh
✅ [SUCCESS] .github/actions/free-disk-space/free-disk-space.sh
✅ [SUCCESS] .github/actions/ksail-system-test/offline-ecr-upstream.sh
✅ [SUCCESS] .github/actions/ksail-system-test/offline-ecr-upstream.test.sh
✅ [SUCCESS] .github/actions/ksail-system-test/talos-kubernetes-upgrade.sh
✅ [SUCCESS] .github/actions/ksail-system-test/verify-offline-ecr-pull.sh
✅ [SUCCESS] .github/actions/ksail-system-test/verify-offline-ecr-pull.test.sh
✅ [SUCCESS] .github/actions/ksail-test-workload/install-with-recovery.sh
✅ [SUCCESS] .github/actions/ksail-test-workload/install-with-recovery.test.sh
✅ [SUCCESS] .github/actions/ksail-test-workload/reject-remote-kustomize-resources.sh
✅ [SUCCESS] .github/actions/ksail-test-workload/remote-resource-guard.test.sh
✅ [SUCCESS] .github/actions/pull-registry-image/pull-registry-image.sh
✅ [SUCCESS] .github/actions/rate-limit-gate/wait-for-rate-limit.sh
✅ [SUCCESS] .github/actions/rate-limit-gate/wait-for-rate-limit.test.sh
✅ [SUCCESS] .github/actions/warm-mirror-cache/cache-validation.test.sh
✅ [SUCCESS] .github/scripts/assert-gitops-deployed.sh
✅ [SUCCESS] .github/scripts/assert-gitops-deployed.test.sh
✅ [SUCCESS] .github/scripts/assert-k3k-server-creator.sh
✅ [SUCCESS] .github/scripts/assert-k3k-server-creator.test.sh
✅ [SUCCESS] .github/scripts/cask-pr-handoff-workflow.test.sh
✅ [SUCCESS] .github/scripts/collect-cask-pr-handoff.sh
✅ [SUCCESS] .github/scripts/collect-cask-pr-handoff.test.sh
✅ [SUCCESS] .github/scripts/delete-eks-smoke-cluster.sh
✅ [SUCCESS] .github/scripts/delete-eks-smoke-cluster.test.sh
✅ [SUCCESS] .github/scripts/delete-old-workflow-runs.sh
✅ [SUCCESS] .github/scripts/delete-old-workflow-runs.test.sh
✅ [SUCCESS] .github/scripts/find-merged-cask-pr-handoff.sh
✅ [SUCCESS] .github/scripts/find-merged-cask-pr-handoff.test.sh
✅ [SUCCESS] .github/scripts/install-release.test.sh
✅ [SUCCESS] .github/scripts/redraft-evergreen-cask-prs.sh
✅ [SUCCESS] .github/scripts/redraft-evergreen-cask-prs.test.sh
✅ [SUCCESS] .github/scripts/resolve-release-publish-state.sh
✅ [SUCCESS] .github/scripts/resolve-release-publish-state.test.sh
✅ [SUCCESS] .github/scripts/style-clean-cask-branch.sh
✅ [SUCCESS] .github/scripts/style-clean-cask-branch.test.sh
✅ [SUCCESS] .github/scripts/upload-release-assets.sh
✅ [SUCCESS] .github/scripts/upload-release-assets.test.sh
✅ [SUCCESS] .github/scripts/validate-cask-pr-handoff.sh
✅ [SUCCESS] .github/scripts/validate-cask-pr-handoff.test.sh
✅ [SUCCESS] .github/scripts/validate-release-ref.sh
✅ [SUCCESS] .github/scripts/validate-release-ref.test.sh
✅ [SUCCESS] .github/scripts/verify-desktop-codeql.sh
✅ [SUCCESS] .github/scripts/verify-desktop-codeql.test.sh
✅ [SUCCESS] .github/scripts/verify-go-archive-parity.sh
✅ [SUCCESS] .github/scripts/verify-go-archive-parity.test.sh
✅ [SUCCESS] .github/scripts/verify-otelzap-parity.sh
✅ [SUCCESS] .github/scripts/verify-otelzap-parity.test.sh
❌ [ERROR] .github/scripts/verify-vendored-comment-scan.sh
    Error: File:[.github/scripts/verify-vendored-comment-scan.sh] is not executable

✅ [SUCCESS] desktop/scripts/make-icns.sh
✅ [SUCCESS] desktop/scripts/make-info-plist.sh
✅ [SUCCESS] desktop/scripts/make-macos-app.sh
✅ [SUCCESS] docs/demos/render.sh
✅ [SUCCESS] install.sh
✅ [SUCCESS] pkg/svc/provisioner/cluster/vcluster/containerd_registry_hosts.sh
✅ [SUCCESS] scripts/stage-webui.sh

✅ Linters with no issues

actionlint, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint

Notices

⚠️ Your configuration references items that have been removed from MegaLinter and are ignored: REPOSITORY_GITLEAKS. See Removed linters to find their replacements.

See detailed reports in MegaLinter artifacts

MegaLinter is provided by OX Security
Show us your support by starring ⭐ the repository

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 13 minutes.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository YAML (base), Organization UI (inherited)
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: c8e730f9-3a21-46e7-b60e-58458ba2791f
📥 Commits

Reviewing files that changed from the base of the PR and between 8977afb and 0c62b85.

📒 Files selected for processing (4)
  • .github/scripts/verify-vendored-comment-scan.sh
  • .github/workflows/todos.yaml
  • .github/workflows/verify-comment-scan.yaml
  • internal/ciharness/todos_workflow_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (10)
  • GitHub Check: 🔐 Audit Shipped Dependency Graphs
  • GitHub Check: 🏠 Home Isolation Guard
  • GitHub Check: 🏗️ Build
  • GitHub Check: 🧪 Test
  • GitHub Check: 📊 Code Coverage
  • GitHub Check: 🧹 Lint - golangci-lint
  • GitHub Check: 🔍 Dead Code Analysis
  • GitHub Check: 🛡️ Vulnerability Scan
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (go)
🔇 Additional comments (4)
.github/workflows/todos.yaml (1)

14-14: LGTM!

Also applies to: 16-16

internal/ciharness/todos_workflow_test.go (1)

20-20: LGTM!

Also applies to: 25-25, 32-32, 38-39, 50-53, 59-64, 67-67

.github/scripts/verify-vendored-comment-scan.sh (1)

1-79: LGTM!

.github/workflows/verify-comment-scan.yaml (1)

1-48: LGTM!


📝 Walkthrough

Walkthrough

The TODO scanner job now uses the pinned shared workflow with exclude-vendored: true. Its contract test checks the workflow reference and input. A new script checks workflow declarations and released fixtures, then runs the native scanner against a healthy case and three negative controls. A new GitHub Actions workflow runs these checks manually and on relevant pull requests or pushes to main.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 0c62b

No actionable issue was identified. The change is mergeable after normal CI checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0c62b

The scanner remains pinned to an exact revision, with unchanged caller permissions, secret forwarding, and production triggers. No introduced security vulnerability was established. However, the replacement workflow’s credential use and issue-update recovery behavior could not be verified, so the migration retains some uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The visible sensitive outcomes are repository issue mutations and use of the forwarded App private key by the external workflow. Any additional repository, project, or service exposure depends on App configuration and downstream credential handling that were unavailable; repository-only exposure cannot be assumed.

Security Findings and Attack Paths

  • observed — The newly added pull-request execution path is the read-only verification workflow, not the production scanner caller. Its declaration forwards no APP_PRIVATE_KEY. The production caller remains limited to main pushes and manual dispatch, countering a direct new pull-request-to-production-secret path.

Trust Boundaries and Controls

  • observed — The secret recipient changes to a different external workflow repository while remaining commit-pinned. Verification checkouts disable credential persistence. Runner hardening uses egress audit mode, not a blocking network policy; the step name claiming no network or live credentials does not establish the external fixture runner’s isolation.

Resilience and Maintainability Implications

  • observed — The verification harness isolates temporary files per invocation and removes them on shell exit. Its checks address configuration, filtering, and expected payload diagnostics, but do not establish production issue-mutation atomicity, idempotency, concurrent-run safety, or recovery after interruption.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed #7468 requires the immutable shared release, exclude-vendored: true, removal of the local ignore expression, and preservation of consumer integration. .github/workflows/todos.yaml pins `devantler-…
Out of Scope Changes check ✅ Passed The added verification workflow, replay script, and todos_workflow_test.go contract updates support #7468 by checking the shared release integration and vendor-filter behavior. The reviewed changes …
Title check ✅ Passed The title clearly summarizes the main change: enabling the shared scanner to exclude vendored source comments.
Description check ✅ Passed The description explains why the change is needed and describes the shared vendor filter, scanner verification, and expected path coverage.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 unsupported.)

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@devantler devantler left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Generated by the Agentic Engineer

Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)

Reviewed commit: fab517e

  • CodeRabbit: current-PR native refusal, event 2026-10-03T19:39:46Z, included reviews unavailable for 13 minutes. PR/account allowance; recovery at the stated reset. No paid review requested.
  • Codex: native account refusal, event 2026-10-03T15:31:41Z, freshly read this round. Account included review allowance exhausted; recovery requires restored included allowance. No reset time stated.
  • Cursor Bugbot: native user/team refusal, event 2026-10-03T15:35:36Z, freshly read this round. User/team usage or spend limit; recovery requires restored included allowance. No reset time stated.

Reviewed all four changed files for correctness, source and authentication boundaries. The caller binds the immutable canonical release and enables only its root vendor filter; the old consumer ignore is removed. Comparison with the former callee confirms unchanged contents-read/issues-write permissions, App-key forwarding, optional authentication default and configured project default. This change does not claim configured-project integration proof.

The helper derives inputs from the parsed candidate caller and released defaults, requires a clean catalogue at that exact full SHA, validates bare reusable delegation and the production input forwarding, and preserves the released scenario's literal file, request and output expectations. The hosted workflow has only contents-read authority, no scanner/project secrets, credential-free checkouts and a bounded timeout. Its actual pinned scanner runs with no network and synthetic authentication against the strict loopback replay.

Scoped contract RED to GREEN, fixture-schema binding, shellcheck, actionlint and diff hygiene pass locally. The wider local harness hits the existing macOS Bash 3 empty-array limitation in unrelated Talos upgrade tests; that result is not reported as a local pass. Native consumer CI 37148160956 completed successfully at this head: https://github.com/devantler-tech/ksail/actions/runs/37148160956/job/111276260098. The actual scanner completed all 13 requests; disabled filtering, overbroad ignore and corrupted expected payload each fail at their intended replay diagnostic, ruling out acquisition/build failures as controls. Required full CI must settle before promotion. Complete review thread pagination, direct review objects, inline comments and provider check-runs were checked with no remaining findings.

Verdict: no P0/P1 findings

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 4 minutes.

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@github-code-quality

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: Go

Go / code-coverage/go

The overall line coverage in commit 0c62b85 in the codex/ksail-vendored... branch remains at 70%, unchanged from commit 8977afb in the main branch.

Show a line coverage summary of the most impacted files.
File main 8977afb codex/ksail-vendored... 0c62b85 +/-
pkg/cli/cluster...i/distconfig.go 81% 78% -3%
pkg/svc/provisi...etzner/token.go 73% 77% +4%
pkg/client/reconciler/poll.go 80% 87% +7%

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

Ready at 0c62b85097a0c2436d089292fe1084bb0db40f41 against freshly fetched main 8977afb074a9e4b33aa2ab60a2a698293929c398. All current checks, including full Go validation, coverage and product CI, are settled. CodeRabbit’s substantive current-head review reports no actionable findings; the complete thread and inline census is empty. Both automatic successors were reviewed statically and change formatting only; all bot-authored successor execution remained in CI. Their commit signatures are valid.

The exact-head native consumer job passed the actual pinned scanner’s 13-request scenario and the disabled-filter, overbroad-ignore and corrupted-payload controls at their intended diagnostics. Auth/project defaults, App forwarding and caller permissions are preserved. This adoption does not claim configured-project integration or the separate scanner error-boundary repair.

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.

Adopt the reviewed shared vendor filter for repository comment scanning

1 participant