ci: release train + coverage ratchet (release slice; split from the omnibus)#1317
Closed
SharedQA wants to merge 29 commits into
Closed
ci: release train + coverage ratchet (release slice; split from the omnibus)#1317SharedQA wants to merge 29 commits into
SharedQA wants to merge 29 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multiple CI workflows (security, coverage, docs gates, helm validate, nightly install test, release train, AI investigator), a top-level Makefile, docs mapping/waivers, a branch-protection enforcement script, and an installer test script. ChangesCI/CD & Quality Gate Orchestration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
SharedQA
force-pushed
the
claude/modest-blackburn-4cc697
branch
from
June 13, 2026 07:57
3ea4a5e to
dc1a430
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (8)
.github/workflows/release-train.yml (2)
40-40: ⚡ Quick winScope
contents: writeto thepromotejob only.The
contents: writepermission is only required for creating tags and releases in thepromotejob (lines 191-213). The other jobs (resolve,validate-artifact,rc-report) only need read access. Scoping permissions to the job level reduces the attack surface if earlier jobs are compromised.🔒 Proposed fix to scope permissions
permissions: - contents: write # create tags + releases + contents: read packages: read # resolve chart versions from GHCR id-token: read attestations: read # verify SLSA provenanceThen add to the
promotejob:promote: name: promote-to-release if: ${{ github.event_name == 'workflow_dispatch' && inputs.promote }} needs: [resolve, validate-artifact] runs-on: ubuntu-latest timeout-minutes: 10 environment: production + permissions: + contents: write # create tags + releases steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml at line 40, The workflow currently grants top-level permission "contents: write"; change the workflow-level permissions to only "contents: read" and then add a job-level permissions block in the promote job that sets "contents: write" so only the promote job has write access; specifically, update the global permissions to contents: read, leave resolve, validate-artifact and rc-report without write changes, and add a permissions: { contents: write } entry inside the promote job definition to scope tag/release creation to that job.Source: Linters/SAST tools
116-117: ⚡ Quick winVerify checksum of downloaded kubeconform binary.
The workflow downloads and executes the
kubeconformbinary from GitHub releases without verifying its checksum or signature. This creates a supply-chain risk if the release artifact is compromised.🔐 Proposed fix to add checksum verification
- name: kubeconform rendered manifests run: | - curl -sSL https://github.com/yannh/kubeconform/releases/download/v0.6.7/kubeconform-linux-amd64.tar.gz | tar xz + EXPECTED_SHA256="8a55bbf43c1e244b0efcf888e2bb5e6fb8ea8a77f6395ce7b4ca8474d5186e58" + curl -sSL https://github.com/yannh/kubeconform/releases/download/v0.6.7/kubeconform-linux-amd64.tar.gz -o kubeconform.tar.gz + echo "$EXPECTED_SHA256 kubeconform.tar.gz" | sha256sum -c - + tar xzf kubeconform.tar.gz ./kubeconform -strict -ignore-missing-schemas -summary rendered.yamlNote: Replace
EXPECTED_SHA256with the actual checksum from the kubeconform v0.6.7 release page.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml around lines 116 - 117, Replace the current direct download-and-extract of the kubeconform tarball and immediate execution of ./kubeconform with a checksum verification step: download the tarball to a file (instead of piping to tar), obtain the expected SHA256 (either from the release page or a checked-in EXPECTED_SHA256 variable), compute the tarball’s sha256sum and compare it to EXPECTED_SHA256 (fail the job if they differ), then extract and run ./kubeconform only after the match; update the shell commands that currently use "curl -sSL https://...kubeconform-linux-amd64.tar.gz | tar xz" and "./kubeconform -strict -ignore-missing-schemas -summary rendered.yaml" to perform these steps (or alternatively fetch a signed checksum file and verify it before extraction).scripts/ci/enforce-quality-gates.sh (1)
82-99: ⚡ Quick winValidate team existence before applying changes.
If
team_id()fails (team doesn't exist or permission denied), the script will exit mid-execution with a crypticgh apierror, potentially after branch protection is already applied but before environments are configured. This leaves the repository in a partially-configured state.Consider validating both teams exist before starting mutations:
♻️ Proposed improvement for graceful failure
org="${REPO%%/*}" -team_id() { gh api "orgs/${org}/teams/$1" --jq .id; } +team_id() { gh api "orgs/${org}/teams/$1" --jq .id 2>/dev/null || { echo "error: team '$1' not found in org '$org'" >&2; exit 1; }; } + +echo "→ Validating team access…" +QA_TEAM_ID=$(team_id "$QA_TEAM") +RM_TEAM_ID=$(team_id "$RM_TEAM") echo "→ Applying branch protection on main…"Then use
$QA_TEAM_IDand$RM_TEAM_IDin the environment payloads instead of inline command substitution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/enforce-quality-gates.sh` around lines 82 - 99, The script currently inlines $(team_id "$QA_TEAM") and $(team_id "$RM_TEAM") during environment creation which can fail part-way and leave repo partially configured; call team_id for both teams up front (e.g. QA_TEAM_ID=$(team_id "$QA_TEAM") and RM_TEAM_ID=$(team_id "$RM_TEAM")), check their values/exit status immediately and emit a clear error + non-zero exit if either lookup fails, then use $QA_TEAM_ID and $RM_TEAM_ID in the environment payloads instead of inline command substitution; ensure the validation runs before any mutating calls (branch protection or environment creation) and surface the gh API error output in your error message for debugging.tests/install/test-install.sh (1)
164-165: 💤 Low valueConsider using explicit
if-elsefor clarity.The
A && B || Cpattern can be confusing and is not a true if-then-else (if B fails, C runs even if A was true). While safe here sinceexit 0terminates the script, explicit branching improves readability.♻️ Proposed refactor
-[[ $FAIL -eq 0 ]] && { echo "RESULT: INSTALLATION TEST PASSED"; exit 0; } \ - || { echo "RESULT: INSTALLATION TEST FAILED"; exit 1; } +if [[ $FAIL -eq 0 ]]; then + echo "RESULT: INSTALLATION TEST PASSED" + exit 0 +else + echo "RESULT: INSTALLATION TEST FAILED" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/install/test-install.sh` around lines 164 - 165, Replace the short-circuit boolean expression ([[ $FAIL -eq 0 ]] && { echo "RESULT: INSTALLATION TEST PASSED"; exit 0; } || { echo "RESULT: INSTALLATION TEST FAILED"; exit 1; }) with an explicit if-then-else block so the intent is clear: use if [[ $FAIL -eq 0 ]]; then echo "RESULT: INSTALLATION TEST PASSED"; exit 0; else echo "RESULT: INSTALLATION TEST FAILED"; exit 1; fi, preserving the same echo messages and exit codes.Source: Linters/SAST tools
.github/workflows/nightly-install-test.yml (4)
34-36: ⚡ Quick winPin the
azure/setup-helmaction to a commit hash.The action is not pinned to a commit SHA, violating the repository's security policy per static analysis.
🔒 Proposed fix
- - uses: azure/setup-helm@v4 + - uses: azure/setup-helm@fe7b79cd5ee1e45176fcad797de68ecaf3ca4814 # v4.2.0 with: version: v3.14.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-install-test.yml around lines 34 - 36, The workflow currently uses the unpinned action reference azure/setup-helm@v4; replace that with the action pinned to a specific commit SHA (e.g. azure/setup-helm@<commit-sha>) so the runner uses an immutable release. Locate the upstream repository tag for v4, copy its commit SHA, and update the uses line to azure/setup-helm@<that-commit-sha> while keeping the existing with: version: v3.14.0 entry intact.Source: Linters/SAST tools
38-43: ⚡ Quick winConsider pinning the
pyyamlversion for reproducibility.Installing
pyyamlwithout a version constraint may result in non-deterministic CI behavior if the latest version introduces breaking changes.📌 Proposed fix
- pip install --quiet pyyaml + pip install --quiet pyyaml==6.0.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-install-test.yml around lines 38 - 43, The CI step named "Preflight deps (the audit's environment-trap fixes)" currently runs pip install --quiet pyyaml; change this to pin a stable, tested PyYAML release (for example pip install --quiet pyyaml==6.0) or use a bounded spec (e.g. pyyaml>=6.0,<7.0) to ensure reproducible installs and prevent unexpected breakages from newer releases.
27-27: ⚡ Quick winConsider setting
persist-credentials: falseand pinning actions to commit SHAs.The checkout step does not disable credential persistence, which could leave GitHub tokens in
.git/configaccessible to subsequent steps or malicious code. Additionally, the action is not pinned to a commit hash, which is flagged by the repository's security policy.🔒 Proposed fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-install-test.yml at line 27, Update the checkout step that currently uses "uses: actions/checkout@v4" to (1) pin the action to a specific commit SHA instead of the tag (replace `@v4` with the exact commit SHA) and (2) add the input "persist-credentials: false" under that step so GitHub tokens are not left in .git/config; modify the step where "uses: actions/checkout@v4" appears to include these changes.Source: Linters/SAST tools
60-64: ⚡ Quick winPin the
actions/upload-artifactaction to a commit hash.The action is not pinned to a commit SHA, violating the repository's security policy per static analysis.
🔒 Proposed fix
- uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 with: name: install-test-log path: /tmp/insight-install-test-*.log if-no-files-found: ignore🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-install-test.yml around lines 60 - 64, The workflow currently references the reusable action by tag "actions/upload-artifact@v4" which violates pinning policy; update the uses line in the nightly-install-test workflow to pin to a specific commit SHA instead (e.g., replace actions/upload-artifact@v4 with actions/upload-artifact@<commit-sha>), commit the exact SHA for the version you intend to use, and verify the workflow still uploads /tmp/insight-install-test-*.log with if-no-files-found: ignore unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ai-investigate.yml:
- Around line 39-41: Replace the floating checkout reference and enable
non-persistent creds: change the step that currently says uses:
actions/checkout@v4 to use the exact commit SHA for actions/checkout (pin to the
latest recommended commit SHA) and under the existing with: block add
persist-credentials: false (keep the ref: ${ {
github.event.workflow_run.head_sha } } line). This ensures the checkout action
is pinned and credentials are not persisted after checkout.
- Around line 43-55: The step "Collect failing job logs" is vulnerable because
it interpolates ${{ github.event.workflow_run.* }} directly into shell commands;
change it to pass those values via environment variables and reference the env
vars in the shell to avoid template-driven command injection. Concretely, add
env entries like WORKFLOW_NAME: ${{ github.event.workflow_run.name }},
WORKFLOW_BRANCH: ${{ github.event.workflow_run.head_branch }}, WORKFLOW_SHA: ${{
github.event.workflow_run.head_sha }} (keeping GH_TOKEN as-is), then replace the
echo lines to use quoted shell variables (e.g., echo "workflow: $WORKFLOW_NAME"
> /tmp/failure/meta.txt, echo "branch: $WORKFLOW_BRANCH" >>
/tmp/failure/meta.txt, echo "sha: $WORKFLOW_SHA" >> /tmp/failure/meta.txt) so
untrusted branch/name values are not expanded by the action templating phase.
- Around line 57-86: The workflow step currently references the mutable tag
"anthropics/claude-code-action@v1" which should be pinned to an exact commit SHA
for supply-chain integrity; update the "uses" value in the AI investigation step
(the line containing anthropics/claude-code-action@v1) to the corresponding full
commit SHA from the action's repository (e.g.,
anthropics/claude-code-action@<commit-sha>) so the workflow always runs a fixed
revision, and commit that change to .github/workflows/ai-investigate.yml.
In @.github/workflows/coverage.yml:
- Around line 38-40: The workflow .github/workflows/coverage.yml currently uses
floating action refs (actions/checkout@v4, dtolnay/rust-toolchain@stable,
Swatinem/rust-cache@v2, taiki-e/install-action@v2, actions/upload-artifact@v4)
which must be pinned to immutable commit SHAs: replace each `uses:
<owner>/<repo>@<tag>` with the corresponding `uses: <owner>/<repo>@<commit-sha>`
for those specific actions to ensure CI stability. Also add
`persist-credentials: false` to both checkout steps that use actions/checkout so
credentials are not persisted. Update the refs in the lines that reference
actions/checkout, dtolnay/rust-toolchain, Swatinem/rust-cache,
taiki-e/install-action, and actions/upload-artifact, and ensure both checkout
steps include `persist-credentials: false`.
In @.github/workflows/docs-gates.yml:
- Line 28: The workflow step using actions/checkout@v4 should be hardened:
replace the tag pin with a specific commit SHA (pin to the exact
actions/checkout commit) instead of `@v4` and add the input persist-credentials:
false to the checkout step (also consider setting fetch-depth: 1 if shallow
clone is acceptable); update the step that currently references
actions/checkout@v4 to use the checked commit SHA and include
persist-credentials: false to avoid exposing default job credentials.
In @.github/workflows/helm-validate.yml:
- Around line 75-77: Loop that lints charts uses "helm lint \"$c\"" which
applies --strict only to the umbrella chart; update the lint invocation to "helm
lint --strict \"$c\"" so subcharts are linted with strict rules as well (i.e.,
change the shell loop's helm lint command to include the --strict flag).
- Around line 59-67: The workflow step "Forbid :latest and empty image tags in
chart values" currently greps files and can match commented lines and misses
empty-tag forms; update the step's loop (the code using rc/while reading git
ls-files) to first strip or ignore YAML comment lines (e.g., remove lines
matching /^\s*`#/` before testing) and then run two robust checks on the cleaned
content: (1) detect explicit :latest image references using a pattern that only
matches actual image tags (e.g., image:.*:latest\b) and (2) detect empty or
missing tags by matching tag: with empty string/null (tag:\s*(""|'')|null) and
image entries with no tag part (image:.*\b[^:]\s*$ or an explicit image key
lacking a colon). Apply these changes to the loop that processes files from git
ls-files 'charts/**/values.yaml' 'src/**/helm/values.yaml' so commented lines
are ignored and empty-tag forms are reliably caught.
- Around line 33-35: Pin the GitHub Actions entries referenced as
actions/checkout@v4 and azure/setup-helm@v4 to their immutable commit SHAs and
disable credential persistence for the checkout step; specifically, replace the
tag references with the corresponding full commit SHA for both actions and add a
with: persist-credentials: false block to the actions/checkout step so checkout
does not retain repository credentials after the job.
- Around line 91-92: Replace the unsafe "curl | tar xz" download-and-extract
pattern and direct execution of "./kubeconform" with a verified-download flow:
download the kubeconform tarball to disk (instead of streaming), fetch the
corresponding SHA256 (or a GPG/cosign signature) from the release, verify the
tarball integrity/signature, only then extract and run the binary; update the
step that currently uses "curl -sSL https://.../kubeconform-linux-amd64.tar.gz |
tar xz" and the subsequent "./kubeconform -strict -ignore-missing-schemas
-summary /tmp/rendered.yaml" invocation to perform these staged download,
verify, extract, and execute actions so the workflow fails if verification does
not pass.
In @.github/workflows/release-train.yml:
- Around line 84-86: The workflow uses the unpinned action reference
azure/setup-helm@v4; update the uses line to pin the action to a specific commit
SHA (e.g., replace "azure/setup-helm@v4" with "azure/setup-helm@<commit-sha>")
to satisfy the security policy, ensure the chosen SHA corresponds to the
intended v4 release, and leave the existing with: version: v3.14.0 unchanged.
- Line 189: Replace the unpinned checkout action and enable disabling credential
persistence: change the step that uses "actions/checkout@v4" to a commit-pinned
reference (e.g., "actions/checkout@<commit-sha-for-v4>") and add the input
"persist-credentials: false" to that checkout step to prevent the GitHub token
from being written to .git/config; ensure any subsequent push steps authenticate
explicitly (e.g., use GH_TOKEN or a deploy key) since persist-credentials is now
disabled.
- Around line 67-68: The workflow assigns inputs.chart_version directly to the
shell variable VERSION, allowing shell injection; change this to first copy
inputs.chart_version into an intermediate environment variable and
validate/sanitize it before assigning to VERSION (e.g., read into a safe
variable like RAW_CHART_VERSION, validate against a strict semver/allowed-chars
regex such as only digits, dots, hyphens and alphanumerics, reject or fail the
job on mismatch), then set VERSION to the validated value using proper
double-quoting (VERSION="$SANITIZED_CHART_VERSION"); ensure all further uses
reference VERSION (or the sanitized variable) so untrusted input never gets
interpolated into shell without validation.
In @.github/workflows/security-gates.yml:
- Around line 37-43: Pin floating action refs and disable checkout credential
persistence: replace trufflesecurity/trufflehog@main, taiki-e/install-action@v2,
actions/setup-python@v5 and the semgrep/semgrep container image with immutable
references (a specific released tag or a full commit SHA) rather than
branch/major tags, and ensure aquasecurity/trivy-action uses an explicit fixed
version (e.g., keep 0.28.0 if already pinned or change to the exact release
SHA); also add persist-credentials: false to every actions/checkout@v4 step.
Update the workflow entries that reference the symbols
trufflesecurity/trufflehog, taiki-e/install-action, actions/setup-python,
aquasecurity/trivy-action, semgrep/semgrep and each actions/checkout@v4 so they
use pinned refs and include persist-credentials: false.
In `@Makefile`:
- Around line 47-49: The Makefile target currently uses semicolon-chaining so
later commands (cargo llvm-cov) run regardless of the e2e test exit status;
change the command separators so failures are preserved by replacing the `; \`
between the e2e test command `cd ../ingestion/tests/e2e &&
INSIGHT_BIN_DIR=../../../backend/target/debug ./e2e.sh test --tb=short -q; \`
and the following `cd ../../../backend && \` with `&& \` (and make the same
change for the other occurrence noted in the comment), so subsequent steps only
run if the e2e tests succeed.
- Around line 29-30: The Makefile currently swallows any failure from the
command chain using "dotnet test ... || { echo '⚠ dotnet not available locally —
CI will enforce'; }"; change this to first check for the dotnet binary (e.g. use
"command -v dotnet >/dev/null 2>&1 || { echo '⚠ dotnet not available locally —
CI will enforce'; exit 0; }") and then run "dotnet test Insight.Identity.sln
--configuration Release" on its own so real test failures from dotnet are not
masked (do not append a fallthrough || that hides non-"missing binary"
failures). Ensure you reference the same target invocation using
$(BACKEND)/services/identity and the Insight.Identity.sln path when making the
change.
In `@scripts/ci/docs_map.py`:
- Around line 59-60: The current checks use substring matches (hs = " |
".join(headings(p)); s.lower() not in hs.lower()) which can false-pass; instead
build an exact normalized set of heading tokens and test membership: call
headings(p) directly, normalize each heading (strip and lower) into a set (e.g.,
hs_set = {h.strip().lower() for h in headings(p)}) and then return [s for s in
required if s.lower() not in hs_set]; apply the same exact-token approach to the
analogous check around line 104 so both template and artifact registration
checks use exact structural matching rather than substring matching.
---
Nitpick comments:
In @.github/workflows/nightly-install-test.yml:
- Around line 34-36: The workflow currently uses the unpinned action reference
azure/setup-helm@v4; replace that with the action pinned to a specific commit
SHA (e.g. azure/setup-helm@<commit-sha>) so the runner uses an immutable
release. Locate the upstream repository tag for v4, copy its commit SHA, and
update the uses line to azure/setup-helm@<that-commit-sha> while keeping the
existing with: version: v3.14.0 entry intact.
- Around line 38-43: The CI step named "Preflight deps (the audit's
environment-trap fixes)" currently runs pip install --quiet pyyaml; change this
to pin a stable, tested PyYAML release (for example pip install --quiet
pyyaml==6.0) or use a bounded spec (e.g. pyyaml>=6.0,<7.0) to ensure
reproducible installs and prevent unexpected breakages from newer releases.
- Line 27: Update the checkout step that currently uses "uses:
actions/checkout@v4" to (1) pin the action to a specific commit SHA instead of
the tag (replace `@v4` with the exact commit SHA) and (2) add the input
"persist-credentials: false" under that step so GitHub tokens are not left in
.git/config; modify the step where "uses: actions/checkout@v4" appears to
include these changes.
- Around line 60-64: The workflow currently references the reusable action by
tag "actions/upload-artifact@v4" which violates pinning policy; update the uses
line in the nightly-install-test workflow to pin to a specific commit SHA
instead (e.g., replace actions/upload-artifact@v4 with
actions/upload-artifact@<commit-sha>), commit the exact SHA for the version you
intend to use, and verify the workflow still uploads
/tmp/insight-install-test-*.log with if-no-files-found: ignore unchanged.
In @.github/workflows/release-train.yml:
- Line 40: The workflow currently grants top-level permission "contents: write";
change the workflow-level permissions to only "contents: read" and then add a
job-level permissions block in the promote job that sets "contents: write" so
only the promote job has write access; specifically, update the global
permissions to contents: read, leave resolve, validate-artifact and rc-report
without write changes, and add a permissions: { contents: write } entry inside
the promote job definition to scope tag/release creation to that job.
- Around line 116-117: Replace the current direct download-and-extract of the
kubeconform tarball and immediate execution of ./kubeconform with a checksum
verification step: download the tarball to a file (instead of piping to tar),
obtain the expected SHA256 (either from the release page or a checked-in
EXPECTED_SHA256 variable), compute the tarball’s sha256sum and compare it to
EXPECTED_SHA256 (fail the job if they differ), then extract and run
./kubeconform only after the match; update the shell commands that currently use
"curl -sSL https://...kubeconform-linux-amd64.tar.gz | tar xz" and
"./kubeconform -strict -ignore-missing-schemas -summary rendered.yaml" to
perform these steps (or alternatively fetch a signed checksum file and verify it
before extraction).
In `@scripts/ci/enforce-quality-gates.sh`:
- Around line 82-99: The script currently inlines $(team_id "$QA_TEAM") and
$(team_id "$RM_TEAM") during environment creation which can fail part-way and
leave repo partially configured; call team_id for both teams up front (e.g.
QA_TEAM_ID=$(team_id "$QA_TEAM") and RM_TEAM_ID=$(team_id "$RM_TEAM")), check
their values/exit status immediately and emit a clear error + non-zero exit if
either lookup fails, then use $QA_TEAM_ID and $RM_TEAM_ID in the environment
payloads instead of inline command substitution; ensure the validation runs
before any mutating calls (branch protection or environment creation) and
surface the gh API error output in your error message for debugging.
In `@tests/install/test-install.sh`:
- Around line 164-165: Replace the short-circuit boolean expression ([[ $FAIL
-eq 0 ]] && { echo "RESULT: INSTALLATION TEST PASSED"; exit 0; } || { echo
"RESULT: INSTALLATION TEST FAILED"; exit 1; }) with an explicit if-then-else
block so the intent is clear: use if [[ $FAIL -eq 0 ]]; then echo "RESULT:
INSTALLATION TEST PASSED"; exit 0; else echo "RESULT: INSTALLATION TEST FAILED";
exit 1; fi, preserving the same echo messages and exit codes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c24b2ad-3907-4469-bf97-83cb8a32ef7a
📒 Files selected for processing (13)
.github/workflows/ai-investigate.yml.github/workflows/coverage.yml.github/workflows/docs-gates.yml.github/workflows/helm-validate.yml.github/workflows/nightly-install-test.yml.github/workflows/release-train.yml.github/workflows/security-gates.ymlMakefiledocs/.docs-gate-waiversdocs/DOCS_MAP.mdscripts/ci/docs_map.pyscripts/ci/enforce-quality-gates.shtests/install/test-install.sh
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
.github/workflows/docs-gates.yml (1)
28-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin and harden
actions/checkout(unresolved from prior review).The checkout step still uses a tag reference (
@v4) instead of a commit SHA, and does not setpersist-credentials: false. For a mandatory blocking gate, these gaps degrade supply-chain security and unnecessarily expose credentials.🔒 Proposed fix
- - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docs-gates.yml at line 28, The checkout step currently uses the loose tag "actions/checkout@v4" and doesn't set persist-credentials; update the workflow to pin the actions/checkout action to a specific commit SHA (replace `@v4` with the corresponding full commit SHA for the chosen v4 release) and add the input persist-credentials: false (and optionally fetch-depth: 1) to the checkout step so credentials aren't persisted to the workspace; locate the checkout invocation in the docs-gates.yml workflow and make these changes to harden supply-chain security.Source: Linters/SAST tools
.github/workflows/coverage.yml (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all GitHub Actions and disable checkout credential persistence in both jobs.
Line 38 and Line 69 use
actions/checkout@v4withoutpersist-credentials: false, and alluses:refs in these ranges are floating tags. For a mandatory gate workflow, this leaves supply-chain and token-hardening gaps.Suggested hardening patch
- - uses: actions/checkout@v4 + - uses: actions/checkout@<pinned-commit-sha> + with: + persist-credentials: false ... - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@<pinned-commit-sha> ... - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@<pinned-commit-sha> ... - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@<pinned-commit-sha> ... - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@<pinned-commit-sha> ... - - uses: actions/checkout@v4 + - uses: actions/checkout@<pinned-commit-sha> + with: + persist-credentials: false ... - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@<pinned-commit-sha> ... - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@<pinned-commit-sha> ... - - uses: taiki-e/install-action@v2 + - uses: taiki-e/install-action@<pinned-commit-sha> ... - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@<pinned-commit-sha>#!/bin/bash set -euo pipefail f=".github/workflows/coverage.yml" echo "== Unpinned uses refs ==" rg -nP '^\s*-\s*uses:\s*[^@\s]+@(?![0-9a-f]{40}$)[^\s]+' "$f" || true echo "== Checkout steps ==" rg -n 'uses:\s*actions/checkout@' "$f" || true rg -n 'persist-credentials:\s*false' "$f" || true echo "== Context around checkout steps ==" nl -ba "$f" | sed -n '34,44p' nl -ba "$f" | sed -n '65,75p'Also applies to: 44-47, 57-57, 69-71, 75-78, 83-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage.yml around lines 38 - 40, The workflow uses floating action refs and leaves checkout credentials enabled; update every "uses:" entry (e.g., uses: actions/checkout@v4 and uses: dtolnay/rust-toolchain@stable and any other uses: lines in the file) to a pinned immutable ref (prefer a full commit SHA or an exact tag that your org approves) and, for every checkout step (lines with "uses: actions/checkout@..."), add persist-credentials: false under that step to disable token persistence; locate and fix each occurrence of unpinned uses and each checkout step so all action refs are pinned and all checkouts set persist-credentials: false.Source: Linters/SAST tools
🧹 Nitpick comments (1)
tests/install/test-install.sh (1)
95-100: 💤 Low valueConsider using heredocs for multi-line YAML generation.
The long
printfstatements with embedded newlines are hard to read and maintain. Heredocs would improve clarity.♻️ Proposed refactor
- printf 'global:\n tenantDefaultId: "11111111-1111-1111-1111-111111111111"\ningestion:\n reconcile:\n tenantId: "citest"\nclickhouse:\n initDatabases: []\nidentity:\n deploy: false\n' > "$OVERRIDES" - printf 'global:\n tenantDefaultId: "11111111-1111-1111-1111-111111111111"\ningestion:\n reconcile:\n tenantId: "citest"\n' > "$OVERRIDES_B" + cat > "$OVERRIDES" <<'EOF' +global: + tenantDefaultId: "11111111-1111-1111-1111-111111111111" +ingestion: + reconcile: + tenantId: "citest" +clickhouse: + initDatabases: [] +identity: + deploy: false +EOF + cat > "$OVERRIDES_B" <<'EOF' +global: + tenantDefaultId: "11111111-1111-1111-1111-111111111111" +ingestion: + reconcile: + tenantId: "citest" +EOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/install/test-install.sh` around lines 95 - 100, Replace the long printf invocations that write multi-line YAML into the OVERRIDES and OVERRIDES_B files with heredocs for readability: stop using the printf '...'> "$OVERRIDES" and printf '...'> "$OVERRIDES_B" forms and instead write the same YAML blocks via a quoted heredoc (e.g., cat <<'EOF' > "$OVERRIDES" ... EOF and the same for OVERRIDES_B) so content is easier to read and maintain; ensure you use a quoted heredoc to prevent variable expansion and preserve the exact YAML, keep the same tenantDefaultId, ingestion.reconcile.tenantId, clickhouse.initDatabases and identity.deploy values, and preserve the trailing newline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/coverage.yml:
- Around line 83-87: Change the upload-artifact step for the e2e LCOV so the
workflow fails when the artifact is missing: update the actions/upload-artifact
invocation (the step that sets name: coverage-e2e-lcov and path:
src/backend/target/coverage-e2e.lcov) to use if-no-files-found: fail instead of
warn so missing LCOV causes the job to fail.
- Line 30: The workflow currently sets COVERAGE_MIN_LINES to "0", which disables
the --fail-under-lines gate; change the COVERAGE_MIN_LINES value in the workflow
to a non-zero baseline (e.g., the current measured project coverage or the
agreed minimum like "70") so the coverage check can fail when below the
threshold; update the COVERAGE_MIN_LINES environment variable in
.github/workflows/coverage.yml accordingly and commit the new baseline.
In @.github/workflows/nightly-install-test.yml:
- Line 27: The workflow uses actions pinned by tags (e.g., actions/checkout@v4)
which are mutable; replace each tag-pinned action in this workflow with the
corresponding immutable commit SHA (e.g., actions/checkout@<full-commit-sha>) so
the workflow references a specific commit. For each action occurrence referenced
by a tag in the file (including the example actions/checkout@v4), look up the
action repo's tag on GitHub and copy the full commit SHA for that tag, then
update the workflow entry to use that SHA; repeat for every tag-pinned action in
the workflow so all actions are pinned immutably.
- Line 27: The checkout step currently uses actions/checkout@v4 without
disabling credential persistence; update the checkout action invocation (the
step that uses "actions/checkout@v4") to include the input persist-credentials:
false so git credentials are not left in the workspace (i.e., add the
persist-credentials: false option to the existing checkout step).
- Around line 44-49: The CI step "Fresh install + BVT" currently passes --clean
to tests/install/test-install.sh which ends up calling tests/install/cleanup.sh
that prompts interactively and silently cancels in CI; change the workflow so
cleanup is performed non-interactively instead of relying on --clean: either
remove the --clean flag and run cleanup.sh explicitly with a non-interactive
confirmation (e.g., pipe a "yes" response or pass a dedicated
non-interactive/force flag if cleanup.sh/test-install.sh support one), or update
test-install.sh/cleanup.sh to respect a CI-friendly env var (e.g., CI=true or
SKIP_PROMPT/YES) and use that to skip the read prompt; ensure the workflow step
"Fresh install + BVT" invokes the non-interactive path so the old cluster is
actually deleted before install.
In @.github/workflows/release-train.yml:
- Around line 114-117: Both workflows currently download and execute the
kubeconform binary without verifying its integrity; for each site add SHA256
checksum verification before extracting/execing the binary: in
.github/workflows/release-train.yml (lines 114-117) update the "kubeconform
rendered manifests" step to download the corresponding kubeconform .tar.gz
checksum (or a pinned SHA256 value), verify it with sha256sum (or curl the
.sha256 and compare) and only then tar xz and run ./kubeconform; do the same
change in .github/workflows/helm-validate.yml (lines 88-92) for its kubeconform
download step. Optionally, extract this logic into a reusable composite action
(e.g., .github/actions/download-verify-kubeconform/action.yml) that performs
download + SHA256 verification + extraction and then call that action from both
workflows to avoid duplication.
- Line 42: Replace the invalid permission value "id-token: read" with a valid
setting: either remove the id-token permission line if OIDC tokens are not
required, or set it to "id-token: write" if the SLSA provenance/verification
step needs OIDC access; update the workflow permission entry named "id-token"
accordingly so the YAML validates and the provenance step can obtain tokens when
required.
In `@Makefile`:
- Around line 55-58: Change the dbt-validate and security targets to first check
for the tool(s) using an explicit if ...; then ...; else echo "not installed";
fi pattern so real failures propagate instead of being masked; specifically in
Makefile (lines 55-58) replace the `command -v dbt && dbt parse ... || echo "not
installed"` idiom in the dbt-validate target with an if-statement that checks
`command -v dbt` and runs `dbt parse --no-partial-parse` in the then-branch (no
fallback), and in Makefile (lines 71-75) update the security target to check
each tool individually with if checks and run the respective scans in the
then-branches (do not use `&& ... || echo`), using `else echo "tool not
installed"` for each missing tool so vulnerability findings are not hidden.
In `@tests/install/test-install.sh`:
- Around line 81-85: The interactive cleanup (invoking ./cleanup.sh) can
hang/exit early in CI because it prompts; change the logic in
tests/install/test-install.sh to skip the interactive cleanup when running in CI
or a non-interactive shell: detect CI (e.g. [ -n "$CI" ] or ! tty -s) and in
that case run kind delete cluster --name insight >>"$LOG" 2>&1 directly,
otherwise keep the current (cd "$REPO" && ./cleanup.sh) >>"$LOG" 2>&1 || kind
delete cluster --name insight >>"$LOG" 2>&1; ensure references to CLEAN,
cleanup.sh and the fallback kind delete cluster remain so behavior is unchanged
for local interactive runs.
---
Duplicate comments:
In @.github/workflows/coverage.yml:
- Around line 38-40: The workflow uses floating action refs and leaves checkout
credentials enabled; update every "uses:" entry (e.g., uses: actions/checkout@v4
and uses: dtolnay/rust-toolchain@stable and any other uses: lines in the file)
to a pinned immutable ref (prefer a full commit SHA or an exact tag that your
org approves) and, for every checkout step (lines with "uses:
actions/checkout@..."), add persist-credentials: false under that step to
disable token persistence; locate and fix each occurrence of unpinned uses and
each checkout step so all action refs are pinned and all checkouts set
persist-credentials: false.
In @.github/workflows/docs-gates.yml:
- Line 28: The checkout step currently uses the loose tag "actions/checkout@v4"
and doesn't set persist-credentials; update the workflow to pin the
actions/checkout action to a specific commit SHA (replace `@v4` with the
corresponding full commit SHA for the chosen v4 release) and add the input
persist-credentials: false (and optionally fetch-depth: 1) to the checkout step
so credentials aren't persisted to the workspace; locate the checkout invocation
in the docs-gates.yml workflow and make these changes to harden supply-chain
security.
---
Nitpick comments:
In `@tests/install/test-install.sh`:
- Around line 95-100: Replace the long printf invocations that write multi-line
YAML into the OVERRIDES and OVERRIDES_B files with heredocs for readability:
stop using the printf '...'> "$OVERRIDES" and printf '...'> "$OVERRIDES_B" forms
and instead write the same YAML blocks via a quoted heredoc (e.g., cat <<'EOF' >
"$OVERRIDES" ... EOF and the same for OVERRIDES_B) so content is easier to read
and maintain; ensure you use a quoted heredoc to prevent variable expansion and
preserve the exact YAML, keep the same tenantDefaultId,
ingestion.reconcile.tenantId, clickhouse.initDatabases and identity.deploy
values, and preserve the trailing newline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a71b711-65a7-4cfa-b7c3-f526a1521fe4
📒 Files selected for processing (13)
.github/workflows/ai-investigate.yml.github/workflows/coverage.yml.github/workflows/docs-gates.yml.github/workflows/helm-validate.yml.github/workflows/nightly-install-test.yml.github/workflows/release-train.yml.github/workflows/security-gates.ymlMakefiledocs/.docs-gate-waiversdocs/DOCS_MAP.mdscripts/ci/docs_map.pyscripts/ci/enforce-quality-gates.shtests/install/test-install.sh
✅ Files skipped from review due to trivial changes (1)
- docs/DOCS_MAP.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/.docs-gate-waivers
- scripts/ci/enforce-quality-gates.sh
- scripts/ci/docs_map.py
SharedQA
force-pushed
the
claude/modest-blackburn-4cc697
branch
from
June 13, 2026 08:48
dc1a430 to
a8ebbf2
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
.github/workflows/release-train.yml (4)
84-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin third-party actions to immutable commit SHAs.
Lines [84] and [189] use tag-based refs (
@v4), which remain mutable. Pin both to full commit SHAs.Also applies to: 189-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml at line 84, Replace mutable tag refs for the GitHub Action azure/setup-helm@v4 with immutable commit SHAs: find both occurrences of "azure/setup-helm@v4" in the workflow and change each to the corresponding full 40-character commit SHA (e.g., azure/setup-helm@<full-commit-sha>), ensuring you pin both instances to their specific commit SHAs so the action is immutable.Source: Linters/SAST tools
67-68:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSanitize
workflow_dispatchinput before shell use.Lines [67]-[68] directly interpolate
inputs.chart_versioninto shell. Validate via a strict allowlist before assigningVERSION.🔒 Suggested patch
- name: Pick chart version (input or latest published) id: pick env: GH_TOKEN: ${{ github.token }} + INPUT_CHART_VERSION: ${{ inputs.chart_version || '' }} run: | set -euo pipefail - if [[ -n "${{ inputs.chart_version }}" ]]; then - VERSION="${{ inputs.chart_version }}" + if [[ -n "$INPUT_CHART_VERSION" ]]; then + [[ "$INPUT_CHART_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]] || { + echo "::error::invalid chart_version: $INPUT_CHART_VERSION"; exit 1; + } + VERSION="$INPUT_CHART_VERSION" else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml around lines 67 - 68, Sanitize the workflow_dispatch input before using it in the shell by validating inputs.chart_version against a strict allowlist and only assigning it to VERSION when it matches; update the block that assigns VERSION to first check the value (e.g., via a regex or list of permitted semantic versions or tags) and reject or fallback to a safe default if it fails validation, and ensure any assignment to the VERSION variable uses the validated value (not raw ${{ inputs.chart_version }}).Source: Linters/SAST tools
42-42:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix invalid
id-tokenpermission value.Line [42] uses
id-token: read, which is not a valid permission and will fail workflow validation.✅ Minimal fix
permissions: contents: write # create tags + releases packages: read # resolve chart versions from GHCR - id-token: read + id-token: write attestations: read # verify SLSA provenance#!/bin/bash set -euo pipefail rg -n '^\s*id-token:\s*read\b' .github/workflows/release-train.yml && { echo "Invalid id-token permission found"; exit 1; } echo "No invalid id-token permission found."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml at line 42, Replace the invalid permission literal "id-token: read" with a valid value (e.g., "id-token: write") wherever it appears in the workflow; locate the exact occurrence of the string "id-token: read" and change it to "id-token: write" (or remove the key if OIDC is not needed) so the workflow passes validation.Source: Linters/SAST tools
114-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify kubeconform checksum before execution.
Lines [116]-[117] execute a downloaded binary without checksum verification.
🛡️ Suggested hardening
- name: kubeconform rendered manifests run: | - curl -sSL https://github.com/yannh/kubeconform/releases/download/v0.6.7/kubeconform-linux-amd64.tar.gz | tar xz + KVER=v0.6.7 + curl -sSLO "https://github.com/yannh/kubeconform/releases/download/${KVER}/kubeconform-linux-amd64.tar.gz" + curl -sSLO "https://github.com/yannh/kubeconform/releases/download/${KVER}/CHECKSUMS" + grep 'kubeconform-linux-amd64.tar.gz$' CHECKSUMS | sha256sum -c - + tar xzf kubeconform-linux-amd64.tar.gz ./kubeconform -strict -ignore-missing-schemas -summary rendered.yaml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml around lines 114 - 117, The "kubeconform rendered manifests" step currently runs a downloaded kubeconform binary without verification; modify that step to fetch the corresponding checksum (and optionally signature) for the v0.6.7 release, verify the downloaded archive against the checksum (e.g., with sha256sum or gpg verification) and fail the job on mismatch, and only then extract and execute ./kubeconform -strict -ignore-missing-schemas -summary rendered.yaml; ensure the step explicitly downloads the checksum URL matching the tarball and performs the verification before the tar xz and execution commands.tests/install/test-install.sh (1)
81-84:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMake
--cleannon-interactive in CI to preserve fresh-install semantics.Line [83] relies on
cleanup.shfirst; in CI this can no-op while returning success, so cluster deletion is skipped and the “fresh install” contract is broken.🧪 CI-safe cleanup pattern
if $CLEAN; then echo "═══ Phase 1: clean slate (deleting kind cluster) ═══" - (cd "$REPO" && ./cleanup.sh) >>"$LOG" 2>&1 || kind delete cluster --name insight >>"$LOG" 2>&1 + if [[ -n "${CI:-}" ]] || ! tty -s; then + kind delete cluster --name insight >>"$LOG" 2>&1 || true + else + (cd "$REPO" && ./cleanup.sh) >>"$LOG" 2>&1 || kind delete cluster --name insight >>"$LOG" 2>&1 + fi echo " cluster deleted" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/install/test-install.sh` around lines 81 - 84, When CLEAN is true, ensure the CI path runs cleanup non-interactively and always removes the kind cluster: invoke ./cleanup.sh in non-interactive mode (e.g., pass the script's --yes/--force/--non-interactive flag or export CI=1) rather than relying on its exit code, and then always run kind delete cluster --name insight (do not short-circuit the kind deletion based on cleanup.sh success). Update the invocation around CLEAN so it calls (cd "$REPO" && ./cleanup.sh --yes) >>"$LOG" 2>&1; kind delete cluster --name insight >>"$LOG" 2>&1 (or equivalent flags your cleanup.sh supports), referencing the CLEAN variable, the ./cleanup.sh call, and kind delete cluster --name insight..github/workflows/nightly-install-test.yml (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all action references to commit SHAs in this workflow.
Lines [27], [34], and [60] use mutable tag refs (
@v4), which violate immutable-action hardening.Also applies to: 34-34, 60-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/nightly-install-test.yml at line 27, Replace mutable action tags with pinned commit SHAs: find every "uses: ...@v4" entry in the workflow (e.g., the visible actions/checkout@v4 and the other `@v4` refs referenced on lines 34 and 60) and update each to the exact commit SHA for that action release (obtain the authoritative commit SHA from the action's GitHub repo, e.g., github.com/actions/checkout, and replace the tag with the full 40-char SHA). Ensure each "uses:" line is changed to use the pinned SHA so no `@v4` mutable tags remain.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ai-investigate.yml:
- Around line 15-25: The workflow uses a workflow_run trigger which runs on the
default branch and can grant secrets to runs that checkout untrusted PR head
code and invoke the AI with a writable toolset; to fix, add a guard to the
workflow_run job to skip runs triggered by forks (e.g., require
github.event.workflow_run.head_repository.full_name == github.repository and
github.event.workflow_run.conclusion == 'failure'), remove or restrict
Bash-capable tooling from the AI invocation by changing the claude_args to only
use read-only tools (e.g., Read,Grep,Glob) instead of --allowedTools Bash, and
ensure ANTHROPIC_API_KEY is replaced with or limited to a dedicated,
minimal-scope key (or removed) so that the checkout step that uses the PR head
SHA and the AI invocation steps that pass claude_args no longer expose
high-privilege secrets to untrusted code.
- Around line 79-86: The template currently interpolates `${{
github.event.workflow_run.head_branch }}` and `${{
github.event.workflow_run.name }}` directly into the AI prompt and also allows
`--allowedTools Bash` via the `claude_args` string, which enables
prompt-injection via crafted branch names; fix by (1) adding a preceding step
that writes the branch and workflow name to a file (for example
`/tmp/failure/meta.txt`) using safe shell-escaping, (2) update the prompt text
to reference that file (e.g. "see /tmp/failure/meta.txt for branch and workflow
name") instead of inlining `${{ ... }}`, and (3) remove `Bash` from the
`--allowedTools` list in the `claude_args` value to eliminate shell execution
privilege (leave only necessary tools like Read/Grep/Glob). Ensure the `gh pr
comment` invocation reads the branch/workflow values from
`/tmp/failure/meta.txt` rather than relying on inline interpolation.
In @.github/workflows/helm-validate.yml:
- Around line 59-67: The image-tag check step uses a git ls-files invocation
that currently only lists 'charts/**/values.yaml' and 'src/**/helm/values.yaml',
which omits helmfile charts; update the git ls-files argument in the "Forbid
:latest and empty image tags in chart values" run step (the while-IFS read loop
and its input) to include the missing helmfile/charts/**/values.yaml glob so
those files are scanned as well.
In @.github/workflows/nightly-install-test.yml:
- Around line 31-32: Add SHA256 verification for the downloaded kind binary:
after downloading kind (current curl command), also curl the matching SHA file
from
https://github.com/kubernetes-sigs/kind/releases/download/v0.23.0/kind-linux-amd64.sha256sum,
run sha256sum -c against that checksum file to verify the binary, and only if
verification succeeds make the file executable and sudo mv it to /usr/local/bin;
update the sequence around the existing curl/chmod/mv commands (the lines that
currently download "kind" and run "chmod +x kind && sudo mv kind
/usr/local/bin/") to include the checksum download and sha256sum -c step and
fail the job on mismatch.
In @.github/workflows/release-train.yml:
- Around line 39-43: Change the workflow-level permissions block so that
contents is read (not write) and then add job-level permissions { contents:
write } to only the jobs that perform release writes: the rc-report job and the
promote job; no other jobs should get write access. Specifically, update the
top-level permissions map to "contents: read" and add a permissions section
inside the rc-report and promote job definitions that sets "contents: write"
(leaving other permission entries unchanged).
In @.github/workflows/security-gates.yml:
- Line 78: The workflow currently installs the latest pip-audit via the line
"pip install --quiet pip-audit", which makes security-gate results
non-reproducible; change that install command to pin pip-audit to a specific
compatible version (e.g., replace it with "pip install --quiet
pip-audit==2.10.0") so CVE findings are stable across runs and compatible with
Python 3.12.
In `@tests/install/test-install.sh`:
- Around line 102-105: The test currently mutates Chart.yaml in-place using sed
-i.bak; ensure the original file is restored on exit by registering a trap that
moves the .bak back to the original and cleans up the .bak file (e.g., capture
the backup path created by sed -i.bak for "$REPO/charts/insight/Chart.yaml", set
trap 'if [ -f "$BACKUP" ]; then mv "$BACKUP" "$REPO/charts/insight/Chart.yaml";
fi; rm -f "$BACKUP"' EXIT) so the workaround applied by the sed command is
reverted when the test finishes or is interrupted.
---
Duplicate comments:
In @.github/workflows/nightly-install-test.yml:
- Line 27: Replace mutable action tags with pinned commit SHAs: find every
"uses: ...@v4" entry in the workflow (e.g., the visible actions/checkout@v4 and
the other `@v4` refs referenced on lines 34 and 60) and update each to the exact
commit SHA for that action release (obtain the authoritative commit SHA from the
action's GitHub repo, e.g., github.com/actions/checkout, and replace the tag
with the full 40-char SHA). Ensure each "uses:" line is changed to use the
pinned SHA so no `@v4` mutable tags remain.
In @.github/workflows/release-train.yml:
- Line 84: Replace mutable tag refs for the GitHub Action azure/setup-helm@v4
with immutable commit SHAs: find both occurrences of "azure/setup-helm@v4" in
the workflow and change each to the corresponding full 40-character commit SHA
(e.g., azure/setup-helm@<full-commit-sha>), ensuring you pin both instances to
their specific commit SHAs so the action is immutable.
- Around line 67-68: Sanitize the workflow_dispatch input before using it in the
shell by validating inputs.chart_version against a strict allowlist and only
assigning it to VERSION when it matches; update the block that assigns VERSION
to first check the value (e.g., via a regex or list of permitted semantic
versions or tags) and reject or fallback to a safe default if it fails
validation, and ensure any assignment to the VERSION variable uses the validated
value (not raw ${{ inputs.chart_version }}).
- Line 42: Replace the invalid permission literal "id-token: read" with a valid
value (e.g., "id-token: write") wherever it appears in the workflow; locate the
exact occurrence of the string "id-token: read" and change it to "id-token:
write" (or remove the key if OIDC is not needed) so the workflow passes
validation.
- Around line 114-117: The "kubeconform rendered manifests" step currently runs
a downloaded kubeconform binary without verification; modify that step to fetch
the corresponding checksum (and optionally signature) for the v0.6.7 release,
verify the downloaded archive against the checksum (e.g., with sha256sum or gpg
verification) and fail the job on mismatch, and only then extract and execute
./kubeconform -strict -ignore-missing-schemas -summary rendered.yaml; ensure the
step explicitly downloads the checksum URL matching the tarball and performs the
verification before the tar xz and execution commands.
In `@tests/install/test-install.sh`:
- Around line 81-84: When CLEAN is true, ensure the CI path runs cleanup
non-interactively and always removes the kind cluster: invoke ./cleanup.sh in
non-interactive mode (e.g., pass the script's --yes/--force/--non-interactive
flag or export CI=1) rather than relying on its exit code, and then always run
kind delete cluster --name insight (do not short-circuit the kind deletion based
on cleanup.sh success). Update the invocation around CLEAN so it calls (cd
"$REPO" && ./cleanup.sh --yes) >>"$LOG" 2>&1; kind delete cluster --name insight
>>"$LOG" 2>&1 (or equivalent flags your cleanup.sh supports), referencing the
CLEAN variable, the ./cleanup.sh call, and kind delete cluster --name insight.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 551518cf-db37-4557-9047-13c4a07d9b28
📒 Files selected for processing (13)
.github/workflows/ai-investigate.yml.github/workflows/coverage.yml.github/workflows/docs-gates.yml.github/workflows/helm-validate.yml.github/workflows/nightly-install-test.yml.github/workflows/release-train.yml.github/workflows/security-gates.ymlMakefiledocs/.docs-gate-waiversdocs/DOCS_MAP.mdscripts/ci/docs_map.pyscripts/ci/enforce-quality-gates.shtests/install/test-install.sh
✅ Files skipped from review due to trivial changes (1)
- docs/DOCS_MAP.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/.docs-gate-waivers
- scripts/ci/docs_map.py
- scripts/ci/enforce-quality-gates.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release-train.yml (1)
152-156:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
rc-reportjob missingcontents: writepermission — will fail at runtime.This job uses
gh release create(line 187) andgh release edit(line 185), both of which requirecontents: writepermission. The job currently inherits onlycontents: readfrom the workflow level (line 42), so it will fail with a 403 Forbidden error.The past review flagged that both
rc-reportandpromotejobs needcontents: write. Onlypromotewas updated (lines 203-205).🐛 Proposed fix to add missing permissions
rc-report: name: rc-report needs: [resolve, validate-artifact] runs-on: ubuntu-latest timeout-minutes: 5 + permissions: + contents: write # create/edit RC prerelease steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml around lines 152 - 156, The rc-report job is missing the required GitHub Actions permission and will 403 when calling gh release create/edit; add a permissions block for that job (under the job named rc-report) setting contents: write (or extend its existing permissions block to include contents: write) so gh has write access for release operations; no other code changes are needed (promote was already updated).
🧹 Nitpick comments (1)
.github/workflows/release-train.yml (1)
44-44: ⚡ Quick winScope
id-token: writeto thevalidate-artifactjob only.Static analysis flags this as overly broad at the workflow level. Only the
validate-artifactjob uses OIDC forgh attestation verify(line 112). Other jobs don't need OIDC tokens.Move this permission (along with
attestations: read) to a job-levelpermissionsblock invalidate-artifact:🔒 Proposed fix to scope OIDC permissions
validate-artifact: name: validate-artifact needs: resolve runs-on: ubuntu-latest timeout-minutes: 30 + permissions: + packages: read # helm/docker registry login + id-token: write # OIDC for SLSA provenance verification + attestations: read # verify SLSA provenance steps:And at workflow level:
permissions: contents: read packages: read # resolve chart versions from GHCR - id-token: write # OIDC for SLSA provenance verification - attestations: read # verify SLSA provenanceNote: When specifying job-level permissions, all unspecified scopes default to
none, so you must explicitly list all permissions that job needs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-train.yml at line 44, Remove the overly-broad workflow-level permission entries (remove id-token: write and attestations: read from the top-level permissions block) and instead add a job-level permissions block inside the validate-artifact job that explicitly sets id-token: write and attestations: read (and any other scopes that job requires), remembering that unspecified job permissions default to none; update the validate-artifact job (named "validate-artifact") to include this permissions block and leave other jobs without OIDC permissions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/release-train.yml:
- Around line 152-156: The rc-report job is missing the required GitHub Actions
permission and will 403 when calling gh release create/edit; add a permissions
block for that job (under the job named rc-report) setting contents: write (or
extend its existing permissions block to include contents: write) so gh has
write access for release operations; no other code changes are needed (promote
was already updated).
---
Nitpick comments:
In @.github/workflows/release-train.yml:
- Line 44: Remove the overly-broad workflow-level permission entries (remove
id-token: write and attestations: read from the top-level permissions block) and
instead add a job-level permissions block inside the validate-artifact job that
explicitly sets id-token: write and attestations: read (and any other scopes
that job requires), remembering that unspecified job permissions default to
none; update the validate-artifact job (named "validate-artifact") to include
this permissions block and leave other jobs without OIDC permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13309a55-5723-4918-8315-b340fb6046ab
📒 Files selected for processing (11)
.github/workflows/ai-investigate.yml.github/workflows/coverage.yml.github/workflows/docs-gates.yml.github/workflows/helm-validate.yml.github/workflows/nightly-install-test.yml.github/workflows/release-train.yml.github/workflows/security-gates.ymlMakefiledocs/DOCS_MAP.mdscripts/ci/docs_map.pytests/install/test-install.sh
✅ Files skipped from review due to trivial changes (1)
- docs/DOCS_MAP.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/docs-gates.yml
- .github/workflows/coverage.yml
- .github/workflows/security-gates.yml
- .github/workflows/nightly-install-test.yml
- .github/workflows/helm-validate.yml
- Makefile
- tests/install/test-install.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/security-gates.yml (1)
40-44:⚠️ Potential issue | 🟠 MajorFix
fetch-depthfor scheduled runs (nightly sweep is still shallow).
github.event_name == 'schedule' && 0 || 50evaluates to50for scheduled events because numeric0is falsy, so the workflow never fetches the intended full history.Patch
- fetch-depth: ${{ github.event_name == 'schedule' && 0 || 50 }} + fetch-depth: ${{ github.event_name == 'schedule' && '0' || '50' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/security-gates.yml around lines 40 - 44, The fetch-depth expression in the actions/checkout step incorrectly uses "github.event_name == 'schedule' && 0 || 50" which treats 0 as falsy and results in 50 for scheduled runs; replace that expression with a proper conditional such as "${{ github.event_name == 'schedule' ? 0 : 50 }}" so the checkout action will use full history (0) for scheduled events and 50 otherwise, keeping the change confined to the fetch-depth value in the checkout step.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/security-gates.yml:
- Around line 40-44: The fetch-depth expression in the actions/checkout step
incorrectly uses "github.event_name == 'schedule' && 0 || 50" which treats 0 as
falsy and results in 50 for scheduled runs; replace that expression with a
proper conditional such as "${{ github.event_name == 'schedule' ? 0 : 50 }}" so
the checkout action will use full history (0) for scheduled events and 50
otherwise, keeping the change confined to the fetch-depth value in the checkout
step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b220566-d33a-490e-bd47-e75d3388cbb3
📒 Files selected for processing (6)
.github/workflows/ai-investigate.yml.github/workflows/helm-validate.yml.github/workflows/nightly-install-test.yml.github/workflows/release-train.yml.github/workflows/security-gates.ymltests/install/test-install.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/helm-validate.yml
- .github/workflows/release-train.yml
- .github/workflows/nightly-install-test.yml
- .github/workflows/ai-investigate.yml
- tests/install/test-install.sh
SharedQA
marked this pull request as draft
June 13, 2026 22:00
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jun 15, 2026
Merged
Makes the agreed QA operating model mechanical rather than prose. Every
mandatory step in the release process now has a CI artifact that enforces it.
Workflows (.github/workflows/):
- security-gates.yml secrets (TruffleHog), SAST (Semgrep), deps audit,
Trivy config scan; nightly deep sweep
- helm-validate.yml lint --strict, full template render, kubeconform,
appVersion-format guard + no-:latest guard
- docs-gates.yml `make docs-check` + DOCS_MAP staleness gate
- coverage.yml instrumented unit coverage ratchet (per-PR) +
e2e coverage profile (nightly), unit/e2e separate
- release-train.yml nightly RC from latest published chart; promotion
gated by the `production` environment; creates the
signed v<version> tag (tag is the OUTPUT, never the
trigger)
- nightly-install-test.yml fresh Kind install via tests/install/test-install.sh
- ai-investigate.yml on a red gate, posts a spec-traced root-cause comment
(advisory, never a gate)
Tooling:
- Makefile shift-left: `make check` mirrors CI exactly; targets
for fmt/lint/unit/coverage-unit/coverage-e2e/
coverage-gaps/dbt-validate/docs/helm/security/e2e/
contracts/aio
- scripts/ci/docs_map.py generates docs/DOCS_MAP.md; --check enforces PRD/
DESIGN template conformance + artifacts.toml mapping
(with expiring waivers)
- scripts/ci/enforce-quality-gates.sh sets branch-protection required checks
and creates staging/production environments with
reviewer teams
- docs/DOCS_MAP.md generated map of all specs
- docs/.docs-gate-waivers 4 time-boxed waivers (expire Jul 15 / Jul 31)
- tests/install/test-install.sh the install validation script wired to nightly
Open-source note: the secrets/security gates run BEFORE merge by design, since
merging to main publishes the code publicly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
Supply-chain hardening (zizmor unpinned-uses / artipacked):
- pin every third-party action to an immutable commit SHA and the Semgrep
SAST container to a digest; add persist-credentials: false to all checkouts
- release-train promote job authenticates its tag push explicitly via GH_TOKEN
(since persist-credentials is now off)
Security / correctness bugs:
- release-train: id-token: read → write (invalid value; OIDC needs write);
scope contents: write to the promote job only (least privilege)
- release-train: validate workflow_dispatch chart_version as strict semver via
an env var — closes a shell-injection vector
- ai-investigate: pass workflow_run.{name,branch,sha} via env, not template
interpolation — closes a command-injection vector
- nightly-install / test-install.sh: --clean cleanup is non-interactive in CI
(cleanup.sh's [y/N] prompt was silently cancelling, leaving the cluster up)
- helm-validate + release-train: verify kubeconform tarball SHA256 before exec
Gate strength:
- Makefile: stop masking real failures — `if command -v` guards for dotnet /
dbt / security tools, and preserve e2e exit status instead of `;`-chaining
- coverage: COVERAGE_MIN_LINES 0 → 55 (measured ~58.5%, now actually gates) +
validate it is numeric/>0; e2e artifact if-no-files-found warn → error
- docs_map.py: structural (numbering-tolerant) heading match instead of prose
substring; validate waiver expiry as a real ISO date
- helm-validate: lint subcharts with --strict too; robust :latest / empty-tag
detection that ignores commented lines
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…ope rc-report write, helmfile glob, restore Chart.yaml - ai-investigate: gate the job to same-repo branches (workflow_run secret-exfil vector) - nightly-install: SHA256-verify the downloaded kind binary - release-train: rc-report also needs contents: write (creates the RC release) - helm-validate: include helmfile/charts/**/values.yaml in the :latest/empty-tag scan - security-gates: pin pip-audit==2.10.1 - test-install.sh: restore Chart.yaml on exit so the workaround never dirties the checkout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…ifact Least-privilege follow-up: id-token: write + attestations: read move from the workflow level to the only job that verifies SLSA provenance. resolve keeps the inherited contents/packages read; rc-report and promote keep their job-scoped contents: write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…history
`&& 0 || 50` collapses to 50 because numeric 0 is falsy in GitHub Actions
expressions, so the scheduled deep sweep was still shallow. Use string operands
('0' / '50') which are truthy.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…ew-assignment Mandatory review is already enforced by branch protection (1 approving review from a CODEOWNER). The 'two random reviewers' part is GitHub's native team Code Review Assignment (count 2, load-balance) on the owning teams — there is no stable API for it, so enforce-quality-gates.sh now documents it as a follow-up. Removed the shuf/reviewers.txt workflow: with require_code_owner_reviews it would assign non-owners whose approvals don't satisfy the gate, and the default Actions token can't read org team membership anyway. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
- docs-gates: drop the volatile "Generated <date>" line from docs_map.py so DOCS_MAP.md is reproducible (the date alone made it perpetually "stale"); regenerated. - helm-validate: skip vendored upstream subcharts (helmfile/charts/*) in the appVersion/version guard — clickhouse "25.3" is upstream's version, not ours. - deps-audit: pin cargo-audit@0.22.2 (older builds abort parsing the CVSS 4.0 advisory RUSTSEC-2026-0124). - trivy-config: bump trivy-action 0.28.0 -> 0.36.0 (0.28.0 referenced the yanked setup-trivy@v0.2.1). - sast: scope semgrep to the curated p/default ruleset, run report-only during ratchet-in (p/default surfaces real pre-existing findings to triage first), and waive the already-mitigated workflow_run checkout in ai-investigate. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…n deps-audit cargo-audit now parses (0.22.2) and correctly flags RUSTSEC-2023-0071 (rsa 'Marvin', no upstream fix), RUSTSEC-2024-0436 (paste) and RUSTSEC-2026-0173 (proc-macro-error2) — all transitive, none with a drop-in fix today. Waive via --ignore with each tracked in constructorfabric#1339; the gate stays --deny warnings for anything new or fixable. Verified locally: exit 0. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
trivy-config now runs (action bumped to v0.36.0) and flags 8 HIGH findings: root build/test containers (AVD-DS-0002), toolbox apt hygiene (AVD-DS-0029), and the frontend Deployment securityContext gap (AVD-KSV-0118/0014). Waive these 4 documented rules so the gate stays blocking for any new/other misconfiguration; each tracked in constructorfabric#1340. Verified locally: trivy config exit 0; trivy fs already clean. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
SharedQA
force-pushed
the
claude/modest-blackburn-4cc697
branch
from
June 15, 2026 15:47
f9287c4 to
6ad5664
Compare
This was referenced Jun 15, 2026
Split ai-investigate.yml out so constructorfabric#1317's quality gates don't wait on the ANTHROPIC_API_KEY funding decision. Tracked separately as an optional, advisory (non-blocking) workflow. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
ktursunov
reviewed
Jun 16, 2026
ktursunov
reviewed
Jun 16, 2026
Review feedback on constructorfabric#1317 (cyberantonz, "Where are the values?"): the validation render passed only tenantId, so it failed immediately — the umbrella fail-fasts without infra image tags / DB names (by design). Supply the required values via --set, and pin the SERVICE image tags to the release version V so the image-existence check that follows verifies the actual published images (previously it would have had nothing valid to check). Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…latest check Review feedback on constructorfabric#1317 (cyberantonz): - enforce-quality-gates.sh: drop the staging/production GitHub Environment creation. It assumed org teams (insight-qa-leads / insight-release-managers) and environments that don't exist yet, so --apply would have failed. Keep the part that's real today — branch protection on main (required checks, CODEOWNER review, no force-push). The deploy-environment gating can be added in its own change once those teams/environments exist. - release-train.yml: the image :latest/floating-tag check duplicated helm-validate.yml (which scans the source values.yaml). Drop it here and keep only the part nothing else does — verify each image in the rendered released artifact actually exists in the registry. The render now pins service tags to the release version, so a floating tag can't reach this step anyway. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
cyberantonz
reviewed
Jun 16, 2026
Review follow-up on constructorfabric#1317 (cyberantonz): clarify the header — a repo ADMIN runs it by hand (not CI, nothing auto-triggers it); editing branch protection needs admin rights so a contributor or the default CI token can't. Documents the dry-run-vs---apply split and a one-liner to verify admin rights. Also notes why the staging/production Environment idea was dropped (nothing deploys via Actions, so an Environment would gate nothing). Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
cyberantonz
reviewed
Jun 16, 2026
cyberantonz
reviewed
Jun 16, 2026
cyberantonz
reviewed
Jun 16, 2026
cyberantonz
reviewed
Jun 16, 2026
Review feedback on constructorfabric#1317 (cyberantonz): - "Why is coverage separate from unit tests?" — it wasn't justified: the suite ran twice (cargo test in `check`, then again instrumented in `coverage-unit`). Merge them: `check` now runs the suite ONCE under cargo-llvm-cov, which both runs the unit tests and measures line coverage + ratchets the floor. Deleted the duplicate `coverage-unit` job (nothing consumed its artifact separately). - "What about C# coverage?" — documented in place: .NET line coverage for identity is wired via the coverlet collector in constructorfabric#1329 (different tooling from Rust's cargo-llvm-cov, so it can't share the same command). - release-train: "What is infra/insight-gitops?" — it's the private GitOps deploy repo (documented in README). Reworded the release note to point at docs/components/deployment/gitops/README.md and dropped two unverified claims (an "allowlist gate" and approval on a `production` environment — no workflow uses a GitHub Environment). Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
…ed nothing) Review feedback on constructorfabric#1317 (cyberantonz): a separate coverage job isn't warranted. It re-ran the whole e2e suite, and it couldn't capture coverage anyway — the rig drives prebuilt container images, not instrumented host binaries, so there are no profraw files to collect (that wiring is roadmap 2.15). Remove the job; leave a note that when 2.15 lands, e2e coverage folds into the existing `e2e` job (one instrumented run), not a parallel one. `make coverage-e2e` stays for local use. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
Review feedback on constructorfabric#1317 (cyberantonz): the guard allowed version/appVersion forms no chart uses. Checked every Chart.yaml — `version` is always plain semver (0.1.0 / 0.1.72) and `appVersion` is always the release build tag (YYYY.MM.DD.HH.MM-<sha7>); no chart uses a `1.2.3-suffix` version or a plain-semver appVersion. So: - version regex → plain X.Y.Z (dropped the unused -/+ pre-release suffix) - appVersion → build-tag only (dropped the dead semver branch) Still rejects the "---" placeholder (constructorfabric#1303), which is the point. Verified the patterns pass all five real charts and reject "---". Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
SharedQA
marked this pull request as ready for review
June 22, 2026 15:19
- helm-validate: drop the empty-image-tag check. 'tag: ""' in base values.yaml is the repo's deliberate convention (operator/release-train supplies the tag; CI renders supply it via ci/ci-values.yaml), documented as 'MUST be set by the operator' across all services. The :latest guard (the real floating-ref defect class) stays. - docs-gates: regenerate docs/DOCS_MAP.md so it is no longer stale vs scripts/ci/docs_map.py (picks up ADRs/READMEs merged from main). Signed-off-by: Kenan Salim <ks@constructor.tech>
…p-audit nltk and cryptography CVEs are unreachable from here: every connector depends on airbyte-cdk (>=7.23.1,<8.0.0), whose latest 7.x hard-pins nltk==3.9.1 and caps cryptography<47.0.0, so the upstream fixes (nltk 3.9.4, cryptography 48.0.1) cannot resolve. Suppress with explicit --ignore-vuln + tracking, the same fix-or-tracked-waiver policy the Rust audit in this gate already uses. Tracked in constructorfabric#1339; remove each as airbyte-cdk bumps them. Signed-off-by: Kenan Salim <ks@constructor.tech>
…s/redpanda hosts) The helm template render step never actually ran green: the earlier empty-tag check always failed first and masked it. With that check fixed, the render surfaced the chart's required infra hosts that ci-values.yaml never supplied — clickhouse.host, mariadb.host, redis.host, redpanda.brokers (all UNSET/'MUST be set' in the chart). Added render-only in-cluster placeholders. Verified locally: helm template + lint --strict (umbrella + subcharts) + kubeconform all pass. Signed-off-by: Kenan Salim <ks@constructor.tech>
…uctorfabric#1431) constructorfabric#1431 deleted helmfile/, so the glob 'helmfile/charts/*' no longer matches and bash left it literal. The loop's final '[ -f $c/Chart.yaml ]' test then returned false, and under 'set -e' that false became the loop's — and the step's — exit status, so helm-validate failed even though all 5 charts linted clean (0 chart(s) failed). Drop the dead helmfile/ references and guard with '|| continue' so an unmatched glob can never poison the exit status. Verified locally: lint (umbrella + subcharts) + template render + kubeconform all green. Signed-off-by: Kenan Salim <ks@constructor.tech>
Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com> # Conflicts: # .github/workflows/backend-checks.yml
The main-merge (coverage baseline + new connectors) shifted the doc set; regenerate the generated map so the docs-gates staleness check passes. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
This was referenced Jun 23, 2026
… (split 4/4) Per review, constructorfabric#1317 was too big (docs/security/helm/release). The other three concerns moved to dedicated PRs: - docs-gates -> constructorfabric#1462 - security-gates -> constructorfabric#1463 - helm-validate -> constructorfabric#1464 constructorfabric#1317 now carries only the release/quality slice: - .github/workflows/release-train.yml (version/publish train) - .github/workflows/backend-checks.yml (coverage ratchet folded in) - .github/workflows/e2e-bronze-to-api.yml (coverage note) - scripts/ci/enforce-quality-gates.sh (required-checks bootstrap) - Makefile (the local dev-loop aggregator) Merge order: this slice lands LAST. The Makefile's aggregate targets (check/ci-pr) and the enforce script reference the gates that now live in Signed-off-by: Kenan Salim <ks@constructor.tech> constructorfabric#1462/constructorfabric#1463/constructorfabric#1464, so they resolve once those merge to main and this rebases.
SharedQA
changed the title
Contributor
Author
|
Split it up as you suggested — it was genuinely too much to review in one go. Four PRs now, each self-standing:
Each carries only its own concern; helm and security run their tools directly (no Makefile), docs runs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per review (this PR was too large — docs / security / helm / release), the four concerns are now separate PRs. This PR is the release/quality slice.
Split:
What stays here
.github/workflows/release-train.yml— version/publish train..github/workflows/backend-checks.yml— the coverage ratchet (COVERAGE_MIN_LINES), folded in so all PR-time backend checks live together..github/workflows/e2e-bronze-to-api.yml— coverage note (instrumented e2e folded into the existing job, not a parallel one).scripts/ci/enforce-quality-gates.sh— one-shot admin bootstrap of branch-protection required checks.Makefile— the local dev-loop aggregator (make check/ci-prand the per-concern targets).Merge order
This slice lands last: the Makefile's aggregate targets and the enforce-checks script reference the gates that now live in #1462/#1463/#1464, so they resolve once those merge to main and this rebases.