chore(ci): medium/low findings from the 2026-07-28 full review · Issue #1594 · LeanerCloud/cloud-commitments-cli · GitHub
Skip to content

chore(ci): medium/low findings from the 2026-07-28 full review #1594

Open

Description

@cristim

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

Grouped MEDIUM and LOW findings from the CI/CD and supply-chain slice of the 2026-07-28 full review. The CRITICAL and HIGH findings from the same slice are filed separately; four MEDIUM/LOW items were folded into existing issues as comments instead of being listed here (#1297 gosec version drift, #1394 gosec rule-set divergence, #1330 verify-image quoting plus AWS error swallowing, LeanerCloud/cloud-commitments-platform#45 golang-migrate pin drift).

Scope covered by the review: all 17 .github/workflows/, .github/runbooks/, 20 scripts/, the three Dockerfiles, both compose files, Makefile and Makefile.terraform, and the linter / scanner / ignore configs (.golangci.yml, .pre-commit-config.yaml, .gitallowed, .gitleaksignore, .trivyignore, .snyk, .tflint.hcl, .hadolint.yaml, .markdownlint.yaml, .env.example, .dockerignore, go.work).


MEDIUM

  • AWS Terraform IaC misconfigurations are ungated end to end

    • Where: .pre-commit-config.yaml:130 (trivy config ... --skip-dirs terraform/environments/aws ...); .github/workflows/ci.yml:386-396 (Trivy IaC scan with no exit-code, documented at :382-385 as deliberately non-gating); pre-commit.yml:162 installs Trivy v0.69.3 while CI pins v0.72.0.
    • What: the local and CI pre-commit gate skips the entire AWS environment directory, and CI's Trivy IaC scan reports to the Security tab without failing the job. ci.yml:364-370 reveals the likely origin: Trivy below v0.72.0 panics in adaptDefaultTags on terraform/environments/aws/main.tf. The CI pin has already fixed that bug; the skip is a stale workaround.
    • Failure scenario: a CRITICAL misconfiguration introduced under terraform/environments/aws/ (a public S3 bucket, 0.0.0.0/0 ingress, an unencrypted RDS instance) fails no gate anywhere: pre-commit skips the directory and CI's IaC scan does not fail. .trivyignore additionally blanket-accepts 15 AVD rule IDs (AVD-AWS-0011/0013/0052/0053/0104/0132/0136/0164, AVD-AZU-0004/0012, AZU-0013, AZU-0047, GCP-0015, GCP-0001) repo-wide rather than per-resource.
    • Fix direction: bump the pre-commit Trivy pin to v0.72.0, drop --skip-dirs terraform/environments/aws, and give the CI IaC step exit-code: 1 for CRITICAL now that .trivyignore documents the accepted set.
  • Makefile security and lint targets pass silently when the tool is absent, and never fail on findings

    • Where: Makefile:100-106 (lint), :146-153 (security-scan-go), :155-162 (security-scan-docker), :164-171 (security-scan-terraform), :219-226 (security-scan-snyk); scripts/security-scan.sh:44,109.
    • What: every one is @if command -v <tool> > /dev/null; then <tool> ...; else echo "<tool> not installed"; fi. The else branch exits 0. Worse, in the then branch make runs the whole if as one shell command whose status is that of the trailing echo "✓ ... complete", so even when the tool runs and finds issues the target exits 0. scripts/security-scan.sh makes it explicit with gosec ... || true (:44) and tfsec ... --soft-fail || true (:109).
    • Failure scenario: make security-scan prints "✓ Go security scan complete" and exits 0 both on a machine with no gosec installed and on a machine where gosec found ten CRITICAL issues. A developer reasonably reads that as a pass. This is a direct instance of the repo's own "no silent fallbacks - a masked gate is worse than no gate" rule, which .pre-commit-config.yaml:116-119 cites to justify removing exactly this pattern from the trivy hook.
    • Fix direction: else echo "..."; exit 1; fi in each target, and drop the trailing success echo (or chain with &&) so the tool's exit status propagates. security-scan-terraform should also be retired: tfsec was removed from CI at ci.yml:377-381 in favour of Trivy.
  • make ci and make terraform-validate are broken and cannot serve as a local pre-flight

    • Where: Makefile:174-180 (terraform-validate loops for dir in terraform/environments/*/dev), invoked by ci at :206.
    • What: the environments are terraform/environments/{aws,azure,gcp} with .tf files at the top level; there is no */dev subdirectory. The glob does not expand, sh iterates the literal string, cd fails, and || exit 1 fails the target. make ci therefore always fails at that step regardless of code quality.
    • Failure scenario: the documented local CI pre-flight (make ci, and the make full-test target the repo's contributor guide points at) cannot be used, so nobody runs it, so the root-module-only vet / test-unit and the never-failing security-scan above it are never exercised locally either. The two defects compound.
    • Fix direction: loop terraform/environments/* and skip non-directories, aligning with ci.yml:256-264, which already does this correctly.
  • pre-commit.yml checks out with persisted credentials while executing third-party hook repositories

    • Where: .github/workflows/pre-commit.yml:24-25 - actions/checkout with no with: persist-credentials: false. Same omission in deploy-aws-lambda.yml:131-132, deploy-aws-fargate.yml:103-104, cleanup-staging.yml:55-56, destroy-fargate-dev.yml:44-45. Contrast every job in ci.yml (:39,84,154,206,241,294,417,444,475,494) and every job in rollback.yml / database-migration.yml, which do set it.
    • What: the job leaves a GITHUB_TOKEN in .git/config and then runs pre-commit run --all-files, which clones and executes code from four external repositories (.pre-commit-config.yaml:7,25,38,66,73), runs hadolint/hadolint:v2.14.0 as a Docker container, and executes npm ci over the frontend dependency tree.
    • Failure scenario: a compromised release of any pre-commit hook repo reads .git/config and uses the token. permissions: contents: read (pre-commit.yml:9-10) caps the damage for this workflow, but the four deploy workflows listed above have no such cap and hold AWS / Azure / GCP session credentials in the same environment at the same time.
    • Fix direction: add persist-credentials: false to every actions/checkout that does not subsequently push. Note this correction to INF-09: ci.yml has no workflow-level permissions block; most jobs run with default GITHUB_TOKEN scope #1196, which asserts the whole ci.yml permissions concern is "mitigated by persist-credentials: false on checkouts" - true for ci.yml, false for pre-commit.yml and the deploy family.
  • Third-party pre-commit hook repos are pinned to mutable tags, not commit SHAs

    • Where: .pre-commit-config.yaml:8 (rev: v0.5.1), :26 (v1.105.0), :39 (v6.0.0), :67 (v2.14.0), :74 (v0.47.0).
    • What: every GitHub Action in this repo is SHA-pinned with a version comment, a deliberate and consistently applied policy. The pre-commit hook repos, which execute arbitrary code in the same CI job, are pinned only to Git tags, which the upstream owner can move.
    • Failure scenario: an upstream maintainer account is compromised and v0.5.1 is re-pointed at a malicious commit. The next pre-commit run --all-files in CI executes it with the persisted GITHUB_TOKEN from the item above. pre-commit caches by rev, so the poisoned environment persists across runs until the cache key (pre-commit.yml:205, keyed on hashFiles('.pre-commit-config.yaml')) changes.
    • Fix direction: pre-commit accepts a full SHA in rev:. Pin each and keep the version in a trailing comment, matching the Actions convention.
  • ci-success treats skipped jobs as success, and Snyk is continue-on-error and outside the gate

    • Where: .github/workflows/ci.yml:503-529 (needs: nine jobs, if: always(), checks only for 'failure' and 'cancelled'); :534-535 (the success banner also runs if: always()); :408-411,426 (snyk-scan is continue-on-error: true and is not in ci-success's needs).
    • What: a needs job that is skipped - because a job-level if: evaluates false, or because a future refactor introduces an inter-job dependency - yields result == 'skipped', which matches neither branch, so ci-success passes. The "Post status" step then unconditionally prints "✅ All CI checks completed successfully!" even on the failure path.
    • Failure scenario: ci-success is the natural single required status check for branch protection. If e2e-tests is skipped by a path filter or by an if: added later, the branch-protection gate reports green while the E2E suite never ran, and the summary text actively asserts that everything passed.
    • Fix direction: assert needs.*.result == 'success' for each job rather than scanning for failure strings, and make the "Post status" step conditional on the check actually passing.
  • Trivy filesystem scan and the coverage threshold are advisory only

    • Where: .github/workflows/ci.yml:356-370 (Trivy scan-type: fs, severity: 'CRITICAL,HIGH', no exit-code, which defaults to 0) and :122-128 (coverage check emits ::warning:: and never fails).
    • What: both appear as gates in the job list and neither is one. A CRITICAL dependency CVE surfaced by Trivy lands in the Security tab and CI stays green. (govulncheck at :301-315 is a real gate for Go, so the Trivy fs gap mostly covers the npm and OS-package surface.) Coverage can fall to any value.
    • Failure scenario: a CRITICAL CVE in a frontend transitive dependency is reported but not blocked; the same PR also drops backend coverage 20 points; CI is green on both counts and the PR merges.
    • Fix direction: set exit-code: '1' on the Trivy fs step (.trivyignore already exists to record accepted findings), and decide whether the 80% coverage line is a gate or documentation. If it is a gate, exit 1.
  • docker-build builds the image twice and its smoke test cannot fail

    • Where: .github/workflows/ci.yml:211-226; Makefile:203 has the identical || true.
    • What: docker/build-push-action (:212-220) builds with GHA caching, then :224 runs a second, uncached docker build -t cudly:test ., duplicating the most expensive step in the job. Both smoke invocations end in || true (:225-226), so the step passes even if the entrypoint crashes on start.
    • Failure scenario: a change to scripts/entrypoint.sh (the container's ENTRYPOINT, Dockerfile:169) makes every container exit immediately. docker-build reports success, ci-success is green, and the break is found when the Lambda or Fargate deploy health check times out.
    • Fix direction: reuse the first build (load: true plus tags) instead of rebuilding, and drop || true so a non-zero --version / --help fails the job.
  • Get migration version writes untrusted command output into $GITHUB_ENV

    • Where: .github/workflows/database-migration.yml:229-236 - VERSION=$(migrate ... version 2>&1 || echo "unknown") then echo "MIGRATION_VERSION=$VERSION" >> $GITHUB_ENV; the value is echoed to the log at :235.
    • What: 2>&1 folds stderr into $VERSION. golang-migrate / pq error text is multi-line and can echo connection details. Writing a multi-line value to $GITHUB_ENV in KEY=VALUE form either fails the step with "Invalid format" or, if the text contains a newline followed by NAME=value, injects an arbitrary environment variable into every subsequent step.
    • Failure scenario: a connection failure produces a multi-line error; the step fails with a confusing "Invalid format" instead of the real DB error, or an error string carrying attacker-influenced text (a hostname or DB name embedded in a message) sets PATH / LD_PRELOAD for later steps. Nothing ever reads MIGRATION_VERSION afterwards, so the step earns nothing in exchange.
    • Fix direction: capture stdout only and use the heredoc delimiter form ({ echo "MIGRATION_VERSION<<EOF"; echo "$VERSION"; echo EOF; } >> "$GITHUB_ENV"), or delete the step, since nothing consumes the value.
  • No Dependabot or Renovate configuration, so the SHA pins and digests cannot be maintained

    • Where: .github/ contains only runbooks/ and workflows/ - no dependabot.yml, no renovate.json, no CODEOWNERS. Dockerfile:17 and Dockerfile.dev:6 both note "A Renovate / Dependabot config can automate this if desired"; neither exists.
    • What: the repo SHA-pins every Action, digest-pins every Docker base image (Dockerfile:18,92,118, Dockerfile.dev:8, Dockerfile.test:8) and version-pins every CI tool. That is good hygiene, and it becomes a liability without update automation.
    • Failure scenario: a CVE lands in the pinned alpine:3.21.3 digest, the pinned golang:1.26.5-alpine3.24 builder, or a pinned Action, and nothing notices. govulncheck covers Go module CVEs and Trivy reports base-image CVEs to the Security tab non-gatingly, so pinned-and-forgotten images can rot indefinitely. Absent CODEOWNERS, workflow changes also require no security review.
    • Fix direction: add .github/dependabot.yml covering github-actions, docker, gomod (all six module directories) and npm (frontend/); add CODEOWNERS covering .github/workflows/** and terraform/**.
  • Terraform version pinned to three different values across the pipeline

    • Where: ci.yml:251 and pre-commit.yml:60 pin 1.10.5; deploy-aws-lambda.yml:77, deploy-aws-fargate.yml:61, deploy-azure.yml, deploy-gcp.yml, cleanup-staging.yml:29, destroy-fargate-dev.yml:21 pin 1.10.0; rollback.yml:41 pins 1.6.0.
    • What: ci.yml:249-250 states the intent ("Matches the pin in pre-commit.yml so both workflows resolve to the same binary") but the deploy family is one patch behind and rollback is four minors behind.
    • Failure scenario: terraform validate passes in CI on 1.10.5 and terraform apply runs on 1.10.0 with a different HCL evaluation or provider-resolution path. A behavioural difference is discovered mid-apply against production.
    • Fix direction: define the version once (a repo-level variable or a .terraform-version file) and reference it everywhere. The rollback.yml pin is a separate, higher-severity problem because it is below the required_version floor; see the dedicated issue from this review.

LOW

  • check-yaml pre-commit hook excludes every workflow file

    • Where: .pre-commit-config.yaml:47 - exclude: '^(docker-compose.*\.yml|\.github/workflows/.*|cloudformation/.*|iac/.*/cloudformation/.*)$'.
    • What: the only YAML syntax check in the pipeline skips the 17 workflow files, and nothing else validates them - there is no actionlint step anywhere.
    • Failure scenario: a malformed workflow is committed. GitHub silently refuses to run it: workflows with parse errors do not appear in the Actions tab and produce no failing check. A gate can disappear from CI with no red signal at all.
    • Fix direction: drop \.github/workflows/.* from the exclusion (the exclusion exists for GitHub-specific tags in other file types; workflow YAML is plain) and add an actionlint hook.
  • postgres:16-alpine is unpinned by digest in both CI and the E2E compose stack

    • Where: .github/workflows/ci.yml:137 (integration-tests service) and docker-compose.test.yml:7.
    • What: these are the only mutable-tag container references in the repo. Every other image is digest-pinned (Dockerfile:18,92,118, Dockerfile.dev:8, Dockerfile.test:8) and Dockerfile:13-17 documents the reasoning.
    • Failure scenario: a Docker Hub tag mutation or a routine 16-alpine rebuild changes Postgres behaviour and the integration plus E2E suites go red, or worse green-but-wrong, with no repo-side change to explain it.
    • Fix direction: pin both to postgres:16-alpine@sha256:....
  • Unquoted heredocs interpolate workflow context into JSON payloads

  • .golangci.yml gocyclo threshold (15) differs from the enforcing gates (10)

    • Where: .golangci.yml:42-43 (gocyclo: min-complexity: 15) versus ci.yml:65 (gocyclo -over 10) and .pre-commit-config.yaml:85 (gocyclo -over 10).
    • What: golangci-lint reports nothing for functions at complexity 11-15; the standalone gocyclo steps fail them. ci.yml:62-64 documents the test-file half of this alignment but not the threshold half.
    • Failure scenario: already a recorded regression in this repo (chore(providers/aws): 4 AWS service-client funcs exceed pre-commit gocyclo -over 10 (keeps pre-commit CI red) #1388): a function lands at complexity 11-15, passes make lint, and reddens Lint Code and pre-commit --all-files repo-wide, blocking every other PR until it is fixed.
    • Fix direction: set min-complexity: 10 so all three tools agree.
  • .env.example ships a working admin password and API key

    • Where: .env.example:69 (ADMIN_PASSWORD_DEV=LocalDev!Pass123) and :71 (ADMIN_API_KEY_DEV=cudly-local-dev-api-key-not-for-prod).
    • What: clearly labelled dev placeholders and correctly outside git-secrets' concern, but they are functional bootstrap credentials, and the file's own header instructs readers to copy it to .env.local.
    • Failure scenario: an operator copies .env.example, fills in the cloud settings, misses line 69, and deploys with a publicly known admin password.
    • Fix direction: replace both values with obviously non-functional placeholders (<set-me>) so the app fails loudly rather than starting with a known credential.
  • deploy-all.yml failure check ignores cancelled child deployments

    • Where: .github/workflows/deploy-all.yml:238-256 - tests only == "failure" for each of the four child results.
    • What: a cancelled reusable-workflow result passes the check and the job prints "✅ All deployments completed successfully!". ci.yml:525-528 handles cancelled explicitly, so the omission here is an inconsistency rather than a house style.
    • Failure scenario: a production multi-cloud deploy is cancelled mid-terraform apply, leaving infrastructure half-applied, and the summary reports success. Nobody investigates.
    • Fix direction: treat cancelled, and skipped-when-expected, as failures.
  • .dockerignore omits the test compose/Dockerfile pair it excludes for the dev pair

    • Where: .dockerignore:26-28 excludes docker-compose.yml and Dockerfile.dev but not docker-compose.test.yml or Dockerfile.test.
    • What: both are COPY . .-ed into the builder stage (Dockerfile:71). No secrets are involved - docker-compose.test.yml:12,46 contains only the throwaway test_password - but it is unnecessary build context and an inconsistency in a file whose entire purpose is this.
    • Fix direction: add both to the "Docker compose dev files" block.
  • Makefile.terraform clean-locks deletes provider lock files

    • Where: Makefile.terraform:84-87 - find terraform/environments -name ".terraform.lock.hcl" -type f -delete.
    • What: .terraform.lock.hcl is the provider checksum pin and is committed (terraform/environments/aws/.terraform.lock.hcl). Deleting it and re-running init re-resolves providers to whatever the ~> constraint currently admits and regenerates checksums from whatever is downloaded. The adjacent clean target at :78 is correctly scoped and its help text says "preserves .terraform.lock.hcl", so the intent is understood elsewhere in the same file.
    • Failure scenario: an operator runs make -f Makefile.terraform clean-locks to clear a resolution problem, commits the regenerated lock, and the repo's provider supply-chain pinning silently moves to a newer, unreviewed provider build with no diff that reads as a supply-chain change.
    • Fix direction: require an explicit confirmation variable, and print a warning that the regenerated lock must be reviewed as a supply-chain change.

Checked and clean (recorded so a future review need not re-derive)

  • Workflow injection from untrusted PR content: no pull_request_target anywhere, and no github.event.{issue,pull_request,comment}.{title,body} reaches any run: block. frontend-build.yml:13-16 documents the reasoning explicitly. Every injection finding from this review is dispatch-input driven, not fork driven.
  • Fork PR credential exposure: aws_sanity.yml and azure_sanity.yml run on unfiltered pull_request with id-token: write and build/execute PR-head Go code, but their precheck step (aws_sanity.yml:22-34, azure_sanity.yml:20-33) short-circuits when the secrets are empty, which is exactly the fork case. Same-repo PRs get a read-only role, acceptable given the author already has write access.
  • Action pinning: every uses: in all 17 workflows is a 40-character SHA with a version comment. No @main, @master, or @v<n> tag references.
  • Tool installers: tflint (pre-commit.yml:70-75) and trivy (:162-167) are fetched from pinned release tags into a file before execution rather than curl | bash; git-secrets (:177-183) is cloned at tag 1.3.0 and its pattern registration is asserted, closing the silent-no-op hole.
  • Docker hardening: all base images digest-pinned; the runtime runs as non-root (Dockerfile:132-133,149); HEALTHCHECK present (:165-166); golang-migrate downloads are SHA256-verified (:43-54); build args carry only VERSION / GIT_COMMIT / BUILD_DATE, no secrets in layers; Dockerfile.test also runs non-root (:14,21).
  • State-lock handling in deploy-aws-fargate.yml: the operator-gated clear_stale_lock input (:40-44,117-118) plus the explanatory comment at :160-167 is the correct model, and is the template cited by the lock-delete findings against the Lambda and destroy workflows.
  • govulncheck / gosec module coverage: ci.yml:312 and :333 correctly loop all six Go modules, and the per-module SARIF automationDetails.id rewrite (:341-342) correctly avoids the code-scanning category collision.
  • .trivyignore / .gitallowed / .gitleaksignore: every entry is individually justified with a written rationale; .gitallowed explicitly forbids adding real account IDs; .gitleaksignore is scoped to one test file. The remaining concern (entries are repo-wide rule suppressions rather than per-resource) is folded into the AWS-IaC item above rather than filed separately.
  • .snyk: no active ignore: entries - the block is commented-out example text. The exclude: list covers only test, vendor and generated paths.
  • .tflint.hcl: force = false (:9) correctly means tflint returns non-zero on findings; plugin versions are pinned.
  • database-migration.yml down-migration guards: the typed confirm: rollback-prod gate (:119-122), the positive-steps requirement (:109-113), the exit 1 on invalid input (:143-146), and the defence-in-depth re-check in each migrate job (:221-224, :298-301, :363-366) are correct and fail closed, including for workflow_call, which does not expose steps and so cannot reach a down migration at all.

Related: #1478, #1269, #1196, #1297, #1394, #1330, LeanerCloud/cloud-commitments-platform#45, #1542, #1388, #1180, LeanerCloud/cloud-commitments-platform#56, #1007.

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A13-013 (medium)

A correction to one of the .trivyignore entries named in this issue, from finding A13-013. AVD-AWS-0013's suppression text claims TLS 1.0 "was tightened in a prior PR" and that the finding only trips because the value is "inherited rather than set explicitly". It is set explicitly: terraform/modules/frontend/aws/main.tf:113 is minimum_protocol_version = var.acm_certificate_arn != null ? "TLSv1.2_2021" : "TLSv1", and the null branch is the default for every environment. The finding is live rather than an artifact. It is unexploitable only because enable_cdn = false in every shipped tfvars, which is a different reason from the one written down, so the suppression should be rewritten to that reason or the branch dropped. Related to LeanerCloud/cloud-commitments-platform#131's minimum_protocol_version item, which tracks the code side.

A14-034 (medium)

The make ci / make terraform-validate item has a larger sibling: the same non-existent terraform/environments/<provider>/<profile> layout is assumed by scripts/tf-deploy.sh:62, which every deploy target routes through. ENV_DIR is built as terraform/environments/${PROVIDER}/${PROFILE}, a path the repository does not have, since the environments are single flat roots selected by -var-file. The script then creates that directory and symlinks only main.tf into it (line 83), leaving a root with no variables.tf or backend.tf, and runs terraform init at line 113 with no -backend-config. Makefile:102 (deploy) and every target in Makefile.terraform route through this script, so the documented local deployment path is inoperable in the same way the local CI pre-flight is. In practice make deploy exits at the profile check first, because terraform/profiles/aws/ holds only *.tfvars.example and make profile-list reports no profiles. Fixing this alongside terraform-validate keeps one directory assumption rather than two. Audit finding A14-034.

A13-016 (low)

Two corrections to the Checked-and-clean notes on the suppression files, since a future review would otherwise trust them. First, .trivyignore is not uniformly justified: the GCP-0015 entry at :92-98 says the GCP database module "exposes require_ssl as a variable" and is "Tracked as a hardening follow-up to set require_ssl = true in the module default". No require_ssl variable exists anywhere under terraform/modules/database/gcp/; both ip_configuration blocks hardcode ssl_mode = "ENCRYPTED_ONLY" (main.tf:84, :183), which already requires TLS. So the entry is either stale against a fixed finding or masking a different one, and it points a reviewer at a follow-up that cannot be performed. Removing it and re-running trivy config would settle which. Second, .snyk is clean on ignore: as noted, but it declares two keys the Snyk policy format does not read: failOnSeverity: high at :26 and a license allow/deny map at :29-43. Both real enforcement points pass the threshold on the command line instead (Makefile:232 and ci.yml:832 both --severity-threshold=high), and nothing reads the license map, so a GPL-3.0 dependency would be admitted despite the file appearing to forbid it. Deleting the two inert blocks, or moving the license policy to a real enforcement point with a comment saying where it lives, removes the false assurance. Audit findings A13-016 and A13-017.

A13-021 (low)

One correction to the postgres:16-alpine pinning item: it says these "are the only mutable-tag container references in the repo", and two more exist in docker-compose.yml. nginx:alpine at line 96 floats a whole major, and dpage/pgadmin4:9.2 at line 110 is minor-pinned but not digest-pinned. The same file pins postgres by digest at line 6, and docker-compose.test.yml:7-11 carries the written rationale ("a floating tag lets the database change under an unchanged repo"), so this is drift within one convention rather than an unconsidered choice. Scope is the local dev stack only, and the nginx half matters most because scripts/nginx.conf depends on the reverse-proxy behaviour. Worth pinning both in the same pass as the postgres ones. Audit finding A13-021.

A14-037 (low)

The .golangci.yml gocyclo item covers the threshold axis; there is a second axis at a third site. Makefile:126 runs gocyclo -over 10 . with no ignore, while ci.yml:70 passes -ignore "_test\.go" and .golangci.yml:42-43,86-92 sets min-complexity: 15 plus a _test\.go exclusion. So the local target fails on table-driven tests that the merge gate accepts, and both ci and pre-commit depend on it. That is the stricter-locally direction, which trains people to ignore the local gate rather than blocking a merge, but it is the same three-tools-disagree problem. Adding -ignore "_test\.go" to the Makefile target belongs in the same change as setting min-complexity: 10. Separately, the 2>&1 capture on that line folds a gocyclo tool error into the "complexity issues" branch, so a missing binary reads as findings. Audit finding A14-037.

A14-041 (low)

A third gate belongs in the advisory-only item alongside the Trivy fs scan and the coverage threshold. ci.yml:595-608 ends both tfvars format checks with terraform fmt -check "github-${env}.tfvars" || echo "Note: github-${env}.tfvars may need formatting", so nothing they find can fail the step, which is named "Validate environment tfvars files". terraform fmt -check returns non-zero on a genuine parse error as well as on formatting, so a broken github-prod.tfvars reaches the deploy workflows with the validating job green. Simplest fix is to let the non-zero exit propagate, or fold these files into the existing terraform fmt -check -recursive step. Audit finding A14-041.


Scope after the split

The CUDly monorepo was split into cloud-commitments-cli (this repo, CLI-only: cmd/*.go plus docs/cli/), cloud-commitments-go (pkg/, providers/), cloud-commitments-mcp (the MCP server), and cloud-commitments-platform (internal/, frontend/, terraform/, iac/, etc.). Re-verified against each repo's current tree (2026-09-28), most of the findings above are either about files that no longer live in this repo, or were already fixed in this repo's copy of the shared tooling files by the time of the split (this repo's Makefile lint/security-scan-* targets already fail hard rather than pass silently, ci-success already treats a skipped job as failure, and pre-commit.yml's checkout already sets persist-credentials: false).

This issue is now scoped to only what remains applicable to this repo's own tree: this repo's copy still independently reproduces three of the original review's findings about tooling files that are copied identically into all four split repos, plus the missing Dependabot/CODEOWNERS config:

  • Third-party pre-commit hook repos pinned to mutable tags, not commit SHAs (.pre-commit-config.yaml: rev: v6.0.0 for pre-commit/pre-commit-hooks, rev: v0.47.0 for igorshubovych/markdownlint-cli). Pin each to a full commit SHA with the version as a trailing comment.
  • check-yaml pre-commit hook excludes every workflow file (.pre-commit-config.yaml check-yaml hook exclude pattern still includes \.github/workflows/.*). Drop that from the exclusion and add an actionlint hook.
  • .golangci.yml gocyclo threshold (15) differs from the enforcing gates (10) (.golangci.yml: min-complexity: 15 vs ci.yml's and the Makefile's gocyclo -over 10). Set min-complexity: 10 so all tools agree.
  • No Dependabot/Renovate config and no CODEOWNERS in .github/ (confirmed: only workflows/ exists). Add .github/dependabot.yml covering github-actions and gomod; add CODEOWNERS if this repo wants review gating on workflow changes.

All other checklist items in the body above are superseded by the sibling issues linked above and can be treated as resolved-by-split for this repo.

Activity

  1. cristim commented on Sep 27, 2026

    @cristim
    MemberAuthor

    Split as part of the monorepo split (cloud-commitments-cli / -go / -mcp / -platform). This repo is one of the genuinely affected repos (it still reproduces three of the shared-tooling findings plus the missing Dependabot/CODEOWNERS config), so this issue stays open, scoped down to that CLI-repo-specific remainder. See the "Scope after the split" section added to the issue body.

    Sibling issues for the parts that moved elsewhere:

  2. self-assigned this
    on Oct 16, 2026
  3. cristim commented on Oct 16, 2026

    @cristim
    MemberAuthor

    Claiming: working on this [claude-cli-impl]. Scope: pin the pre-commit hook revs to commit SHAs, add an actionlint hook (the check-yaml workflow exclude was already dropped in #2234), and align the golangci gocyclo threshold with the enforcing gates if that doesn't force refactors. Dependabot/CODEOWNERS are left for an owner decision.

  4. cristim commented on Oct 17, 2026

    @cristim
    MemberAuthor

    Status from [claude-cli-impl]: #2241 pinned the hook revisions to SHAs and aligned the gocyclo threshold to 10. actionlint was already on main, and #2234 dropped the check-yaml workflow exclude. What's left is adding Dependabot/Renovate and CODEOWNERS, which is an owner policy decision: dependency bots open PRs automatically, and CODEOWNERS gates reviews. I'm unassigning myself; tell me which of the two to add and I'll pick it up.

  5. removed their assignment
    on Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions