chore: add CI and Security workflows, drop the dead and deploy-only ones [#5] - #14
Conversation
Adds a run-tests/security/build-and-push flow: gated lint, gotestsum with diff-scoped coverage, dependency+image Trivy scans gated on HIGH/CRITICAL, multi-registry multi-arch build-and-push on release tags. Both Dockerfiles switch to dhi.io base images (with public overrides for a local build without a DHI subscription), so the security and build-and-push jobs authenticate to dhi.io first.
… dependency [#5] security.yml's Trivy gate failed on unpatched CVEs in dhi.io/debian-base's bundled perl-base, ncurses, util-linux and zlib1g -- none of which the adapter uses. Neither can move to the fully static dhi.io/static image though: the adapter loads plugin .so files at runtime via Go's `plugin` package, which needs cgo and a real glibc dynamic linker to work at all. dhi.io/static's glibc variant is the middle ground -- keeps libc6 so plugin.Open still works, drops everything else the scan was flagging. That variant has no shell, so CMD's `sh -c "./server --config=$CONFIG_FILE"` can no longer expand the env var. Switched to an exec-form ENTRYPOINT and taught main.go to read CONFIG_FILE itself as the --config default.
…wn CVEs [#5] security.yml's trivy-deps scan flagged CVE-2026-56854 (x/crypto, CRITICAL) and CVE-2026-84304 (grpc, HIGH), both with fixed versions already published upstream.
Ran on every push including this branch and always failed here -- missing Gerrit credentials for the private Terraform-CICD repo it clones. workflow_dispatch stays, so it's still runnable by hand; kept, not deleted, pending a decision on whether it's folded into build-and-push.yml or removed outright.
…ersions [#5] This is CI setup, not a dependency-bump PR -- the two CVEs trivy-deps flags (CVE-2026-56854, CVE-2026-84304) are real and the scan catching them is working as intended; the actual bump is deferred to its own change.
Dockerfile.adapter goes back to the original public base images -- Dockerfile.adapter-with-plugins is the one actually deployed, so that's the one that needs the CVE fix. Keeps dhi.io/static's glibc-only variant (clears the unpatched perl-base/ncurses/util-linux/zlib1g CVEs, keeps libc6 so plugin.Open still works) but drops the main.go change entirely: Deployment/deployment.yaml now passes --config=$(CONFIG_FILE) via args, letting the kubelet do the substitution instead of a shell inside the container.
…dapter [#5] security.yml and build-and-push.yml still pointed at Dockerfile.adapter, which we just reverted back to the original, unpatched base images -- so the security gate was scanning the wrong Dockerfile and the CVE fix never took effect. Dockerfile.adapter-with-plugins is the one actually deployed and the one carrying the DHI glibc-only runtime fix.
🛡️ Trivy — Go dependency scan (HIGH,CRITICAL)No HIGH or CRITICAL findings. |
🛡️ Trivy — Image scan (HIGH,CRITICAL)
|
…ce+coverage run [#5] pkg/plugin and benchmarks/e2e each build a real .so via a subprocess `go build -buildmode=plugin`, then load it with plugin.Open in the same test run. Instrumenting the whole module for coverage/race in one ./... build gives a shared package a different build identity than the plain subprocess build produces, so plugin.Open rejects the .so. Split them into their own go test invocation and merge the coverage output.
|
📊 Test Coverage: ✅ Passed — not applicable, no changed Go files vs origin/development |
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Requesting changes
Thanks for this — the workflows are carefully built and the inline comments explaining why each choice was made are genuinely useful; the dhi-login composite action and the Makefile-target pattern are the right structure. Detailed notes are inline. Summarising the parts that need action before merge.
Blockers
-
The security gate is already failing (
security.yml:215).trivy-deps.sarifreports 3 HIGH/CRITICAL —golang.org/x/cryptov0.54.0 (CRITICAL),grpcv1.82.1,amqp091-gov1.11.0. Merging this makes every subsequent PR and push tomainred untilgo.modis bumped. The description's "0 HIGH/CRITICAL" holds for the image scan; the dependency scan is what fails. -
Deployment/deployment.yaml:25breaks the deployed pod. The newargsoverrideCMDbut notENTRYPOINT, and the image this manifest actually runs (Dockerfile.adapter, port 8080) has noENTRYPOINT— so the container command becomes a bare--config=…string. CrashLoopBackOff. The comment's rationale describesDockerfile.adapter-with-pluginsinstead, which does set anENTRYPOINTand exposes 8081. -
trivy-gatepasses vacuously on a missing report (Makefile:141). An emptycountmakes[ "" -gt 0 ]error and evaluate false, so the gate exits 0. A scan that silently produced no SARIF reports success — the one failure mode a gate can't have.
Scope
Flagging what reaches past the PR's stated purpose:
Dockerfile.adapteris changed, contrary to the description, migrating both stages to subscription-gateddhi.ioimages.deploy-to-gke-BS.yml:43builds this file with no dhi.io login and will now 401. Please split this into its own PR — it's the biggest scope item and it carries real risk to an existing deploy path.- ~80 lines of copy-paste:
build-and-push.yml:205-232duplicates:110-170verbatim;security.yml:77-103and:161-187are byte-identical. Already biting — the missingid-tokenbelow exists in both copies. onix-gcp-terraform-deploy.yml:4disables a deploy trigger — unrelated concern, separate blast radius.Dockerfile.adapter-with-plugins:27-28— trailing-slash-only churn.
CI configuration
push: branches: [main]missesdevelopment(run-tests.yml:6,security.yml:6) — the branch this PR actually targets. Merges landing ondevelopmentrun neither tests nor the security scan. This is the cheapest high-value fix here: without it, most of what this PR builds doesn't run on the branch it's for.- Missing
id-token: write(build-and-push.yml:12) — WIF needs an OIDC token, so the GAR path can't work as written, contradicting the "just a flag flip" comment. cover-diffreports a pass whengit difffails (Makefile:73) —|| trueplusgithub.event.beforebeing all-zeros or a dangling SHA means the push-to-maincoverage gate silently self-disables.:latestmoved by release candidates (build-and-push.yml:260) — the trigger accepts-rcN, sov0.1.1-rc3republishes:latestas a pre-release, and the deployment pulls:latest.- Fork PRs fail the
securitycheck hard — bothupload-sarifcalls and the image build lackcontinue-on-errorand need permissions/secrets forks don't get. Already handled for the comment steps, just not these. - No
fail-fast: false/concurrencyon the release workflow (build-and-push.yml:61) — a failedarm64leg afteramd64pushed leaves a half-published release. dhi-login: secret interpolated into script text rather than passed viaenv:,base64 -w0is GNU-only, and the heredoc clobbers the Docker CLI config rather than merging (works only because of undocumented step ordering).- Unguarded
cat coverage-report.md(run-tests.yml:69) after acontinue-on-errorstep that may not have written it.
Minor
peter-evans/find-comment@v3 and create-or-update-comment@v4 are a major version behind (v4/v5) and emit the Node 20 deprecation warning visible in the run logs. trivy fs . runs after bin/trivy is restored into the workspace, so it scans its own binary — harmless today (the log confirms only go.mod matched) but --skip-dirs bin makes it robust. Makefile:72 declares a coverage.out prerequisite with no rule to build it.
Happy to re-review as soon as the three blockers are addressed and the Dockerfile.adapter change is split out.
| # just wrote. Last in the job, so a red build still leaves both reports | ||
| # behind. | ||
| - name: Gate on HIGH or CRITICAL findings | ||
| run: make trivy-gate || { echo "::error::HIGH or CRITICAL Trivy findings — see job log above"; exit 1; } |
There was a problem hiding this comment.
Blocker — this gate is already red, and merging it turns every future PR red.
gh pr checks 14 reports security fail. Run 33858001117 shows trivy-deps.sarif: 3 HIGH,CRITICAL:
golang.org/x/cryptov0.54.0 — CRITICAL, fixed in 0.55.0google.golang.org/grpcv1.82.1 — HIGHgithub.com/rabbitmq/amqp091-gov1.11.0 — HIGH
So make trivy-gate exits 1. Once this lands, every subsequent PR and every push to main is red until go.mod is bumped.
The PR description's "security scan (0 HIGH/CRITICAL image findings)" is accurate for the image scan — but it's the dependency half that fails. Please bump these three in go.mod as part of this PR so it merges green.
There was a problem hiding this comment.
Bumped in 26e013f: go.mod:7 golang.org/x/crypto v0.55.0, go.mod:123 google.golang.org/grpc v1.83.1, go.mod:105 github.com/rabbitmq/amqp091-go v1.13.0. The dependency half is clean now — the Security gate job on the latest run logs trivy-deps.sarif: 0 HIGH,CRITICAL.
The gate is still red, for a different reason than the one you flagged: the same log line reads trivy-image.sarif: 20 HIGH,CRITICAL, and all 20 are Package: stdlib at Installed Version: v1.26.1 (CVE-2026-27145, -32280/81/83, -33810/11/14/18, -39820/21/22/36, -42499, -42504, -46600, -56853, -56858/59/60/62), every one of them Fixed Version: 1.25.13, 1.26.6, 1.27.0-rc.3. That is the Go toolchain baked into the binary, not a module — it needs FROM golang:1.26.1-bookworm in Dockerfile.adapter-with-plugins:1 and go 1.26.1 in go.mod:3 moved to 1.26.6. Not done in this PR yet, so treat this thread as half-resolved: the three modules you named are fixed, the gate is not green.
| trivy-gate: | ||
| @fail=0; \ | ||
| for report in trivy-deps.sarif trivy-image.sarif; do \ | ||
| count=$$(jq '[.runs[].results[]?] | length' "$$report"); \ |
There was a problem hiding this comment.
Blocker — the gate passes vacuously when a report is missing or malformed.
If $$report doesn't exist or isn't parsable JSON, jq writes nothing to stdout and count becomes the empty string. The next comparison is then [ "" -gt 0 ], which errors with integer expression expected and — because it's inside an if — is treated as false. fail stays 0 and trivy-gate exits 0.
Net effect: a scan step that silently failed to write its SARIF converts the entire security gate into a no-op that reports success. That's the one failure mode a gate must not have.
@fail=0; \
for report in trivy-deps.sarif trivy-image.sarif; do \
if [ ! -s "$$report" ]; then \
echo "::error::$$report missing or empty — scan did not produce a report"; \
fail=1; continue; \
fi; \
count=$$(jq '[.runs[].results[]?] | length' "$$report") || { \
echo "::error::$$report is not valid SARIF"; fail=1; continue; }; \
count=$${count:-0}; \
...There was a problem hiding this comment.
Fixed in e99044b, trivy-gate at Makefile:216-235. The loop now guards both failure modes before the arithmetic comparison: [ ! -s "$report" ] prints MISSING - no scan produced it and sets fail=1, and a jq that produces nothing (stderr suppressed) is caught by [ -z "$count" ] and reported as UNREADABLE - not valid SARIF. Verified with the real target:
$ make trivy-gate # no sarif files in the tree
trivy-deps.sarif: MISSING - no scan produced it
trivy-image.sarif: MISSING - no scan produced it
::error::HIGH or CRITICAL Trivy findings, or a missing report - see the log above
make: *** [trivy-gate] Error 1
$ ls; make trivy-gate # deps valid+empty, image is `not json{{{`
trivy-deps.sarif: 0 HIGH,CRITICAL
trivy-image.sarif: UNREADABLE - not valid SARIF
make: *** [trivy-gate] Error 1
So the empty-string-to-[ -gt ] path no longer exists, and the report: SEVERITY line with the count elided is gone with it.
| # $(CONFIG_FILE) is expanded by the kubelet against the env below | ||
| # before the container starts -- the image's shell-less DHI base | ||
| # has no `sh -c` to do that substitution itself. | ||
| args: |
There was a problem hiding this comment.
Blocker — this breaks the deployed pod (CrashLoopBackOff).
Kubernetes args overrides the image's CMD, not its ENTRYPOINT. This manifest runs …/onix-adapter-cicd/beckn-onix:latest on containerPort: 8080, which is the Dockerfile.adapter image — and that image has no ENTRYPOINT, only:
EXPOSE 8080
CMD ["sh", "-c", "./server --config=${CONFIG_FILE}"]With CMD overridden and no ENTRYPOINT, the container's command becomes the bare string --config=/mnt/gcs/configs/onix-adapter.yaml, which is not an executable. The pod will not start.
The comment above also doesn't match this image: the "shell-less DHI base has no sh -c" rationale describes Dockerfile.adapter-with-plugins, which does set ENTRYPOINT ["/app/server"] — and exposes 8081, not 8080. So either this manifest is pointed at the wrong image, or these args belong with the other Dockerfile. Worth resolving which before merge.
(Note Dockerfile.adapter still keeps its sh -c CMD in this PR, so the shell-substitution problem the comment describes doesn't apply to it.)
There was a problem hiding this comment.
Deployment/deployment.yaml was reverted in 56f7d8b — git diff origin/development...HEAD -- Deployment/deployment.yaml is empty, so the args: ["--config=$(CONFIG_FILE)"] block and its comment are no longer in the PR. The CMD-vs-ENTRYPOINT analysis was correct and is why it went: the reverted Dockerfile.adapter has no ENTRYPOINT, only CMD ["sh", "-c", "./server --config=${CONFIG_FILE}"] at line 30, so overriding CMD from the manifest would have left a bare --config=... as argv[0]. The substitution rationale is also moot now — both reverted Dockerfiles run on cgr.dev/chainguard/wolfi-base, which has a shell, and keep the sh -c CMD. Nothing in the shipped set touches the manifest or the container command; make docker (Makefile:314) only builds a local tag for the image scan and never deploys.
| # image/golang/debian-12/1.26-dev.yaml and image/debian-base/debian-12/12.yaml. | ||
| # No cgo here, but the -dev variant's gcc/g++/make come bundled regardless — | ||
| # nothing extra to install for either the default or the public override. | ||
| ARG BUILD_IMAGE=dhi.io/golang:1.26-debian12-dev |
There was a problem hiding this comment.
Out of scope — and it breaks an existing deploy workflow.
The PR description states "Dockerfile.adapter is unchanged". It's rewritten (+27/−9), and both stages now default to subscription-gated dhi.io images.
deploy-to-gke-BS.yml:43 runs docker build -f Dockerfile.adapter with no dhi.io login, so that deploy path will now 401 on the base-image pull.
A base-image migration to a paid registry isn't required by "add test, security and build-and-push CI workflows" — please split it into its own PR. That also removes the risk to deploy-to-gke-BS from this one.
Separately, the new comment asserts a shell exists in dhi.io/debian-base:bookworm (the retained sh -c CMD depends on it). That's unverified here, and a hardened base image is exactly where that assumption tends to break — worth confirming in whichever PR carries the change.
There was a problem hiding this comment.
Dockerfile.adapter was reverted in 56f7d8b and is byte-identical to development — git diff origin/development...HEAD -- Dockerfile.adapter is empty, and line 1 is back to FROM golang:1.26.1-bookworm with cgr.dev/chainguard/wolfi-base as runtime. The DHI migration is out of this PR entirely, so the deploy-to-gke-BS 401 risk is gone twice over: that workflow was itself deleted in 6220fc4. The unverified shell-in-dhi.io/debian-base assumption goes with it — the retained sh -c CMD now runs on wolfi-base, which does have a shell. The only image the PR builds is Dockerfile.adapter-with-plugins via make docker (Makefile:314-321), on publicly pullable bases, so a fork PR's image scan needs no registry credential.
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: |
There was a problem hiding this comment.
push filter misses the branch this PR targets.
The base of this PR is development, and both develop and development exist as long-lived branches (default is main). With only main listed, direct pushes and merge commits landing on development run neither this workflow nor the other one — only pull_request covers them.
push:
branches: [main, development]There was a problem hiding this comment.
This file no longer exists — it was renamed to .github/workflows/ci.yml in 8236623 (workflow name: CI, job names Lint/Test). The equivalent lines are ci.yml:8-11:
on:
push:
branches: [main, development]Same change applied to security.yml:12-15 in 4f79614, so both workflows now run on a merge landing on development. develop is deliberately left out — nothing in this PR targets it and adding a trigger for a branch we do not merge to would just burn runner minutes.
| # on this exact string to update rather than duplicate that comment. | ||
| - name: Publish coverage summary | ||
| run: | | ||
| printf '<!-- coverage-report -->\n%s\n' "$(cat coverage-report.md)" > coverage-report.md |
There was a problem hiding this comment.
Unguarded read of a file the previous step may not have written.
Check coverage above is explicitly continue-on-error: true, but this step has no guard. Any cover-diff exit path that returns before writing coverage-report.md dies here with cat: coverage-report.md: No such file or directory — which masks the coverage outcome behind an unrelated failure.
Add an existence check, or have cover-diff guarantee the file is written on every exit path.
There was a problem hiding this comment.
Took the second option — cover-diff guarantees the file — rather than adding a guard at the call site. The file is now ci.yml (8236623); the step is Publish coverage to the job summary at ci.yml:93-94, still an unguarded cat coverage-report.md >> "$GITHUB_STEP_SUMMARY". What changed is Makefile:121 cover-diff: every exit path writes coverage-report.md first — unresolvable BASE_REF (Makefile:124), failed git diff (:129), no changed Go files (:134), no coverable statements (:154), and the pass/fail report itself (:172).
One residual gap I will not claim is fixed: the two hard-error paths at Makefile:124 and :129 write the report without $(COVER_MARKER), so on those two paths find-comment does not match and the run posts a new PR comment instead of editing the existing one. The cat no longer dies, but the marker should be on all five paths. Still open.
| steps: | ||
| - shell: bash | ||
| run: | | ||
| AUTH=$(printf '%s:%s' "${{ inputs.username }}" "${{ inputs.token }}" | base64 -w0) |
There was a problem hiding this comment.
Two issues on this line:
-
Secret interpolated into script text.
${{ inputs.token }}is expanded into the generated shell script rather than passed viaenv:. A token containing a shell metacharacter or newline will break the command or execute. Pass it throughenv:and reference"$TOKEN", or better, usedocker login --password-stdin. -
base64 -w0is GNU-only. This composite action can't run on macOS or BSD-based self-hosted runners.tr -d '\n'is portable.
Also worth validating that the inputs are non-empty here — see the note on security.yml:139 for why silently succeeding with empty credentials makes the eventual failure hard to diagnose.
There was a problem hiding this comment.
.github/actions/dhi-login/action.yml was deleted in 56f7d8b (the whole DHI migration was reverted), so this base64 -w0 line no longer exists in the PR. Neither issue has a live equivalent: no shipped workflow or action references a secret — grep -rn 'secrets\.\|base64' .github/workflows/ci.yml .github/workflows/security.yml .github/actions/ returns nothing — and the one remaining composite action, .github/actions/trivy-cache/action.yml, only computes a date and restores cache paths. The don't-interpolate-into-run-script habit is followed where it does apply: ci.yml:87-89 passes BASE_REF through env: and lets the Makefile's ?= pick it up, rather than expanding a ${{ }} value inside the shell command.
| run: | | ||
| AUTH=$(printf '%s:%s' "${{ inputs.username }}" "${{ inputs.token }}" | base64 -w0) | ||
| mkdir -p ~/.docker | ||
| cat > ~/.docker/config.json <<EOF |
There was a problem hiding this comment.
Clobbers the Docker CLI config instead of merging.
A heredoc write replaces ~/.docker/config.json wholesale, discarding any auths entries a prior docker/login-action step already wrote.
Today nothing breaks, because in build-and-push.yml this runs at line 117 and the GHCR/Docker Hub logins come after at line 130. But that ordering is load-bearing and undocumented — reordering the steps, or adding a login above this one, silently drops registry auth mid-job with no error until a push fails.
docker login --password-stdin merges into the existing config and avoids the whole class of problem.
There was a problem hiding this comment.
The action was deleted in 56f7d8b, so nothing in this PR writes the Docker CLI config file any more — there is no registry login step at all, and the load-bearing step ordering you flagged is gone with it. The only composite action left is .github/actions/trivy-cache/action.yml, which writes a $GITHUB_OUTPUT date and hands two cache paths to actions/cache@v4; it touches no Docker credential state. The merge-not-clobber point, and --password-stdin as the way to get it, stand for whenever a registry login returns.
| COPY cmd/adapter/ ./cmd/adapter/ | ||
| COPY core/ ./core/ | ||
| COPY pkg/ ./pkg/ | ||
| COPY core/ ./core |
There was a problem hiding this comment.
Nit — diff noise. Dropping the trailing slash (COPY core/ ./core vs ./core/) is semantically identical for a directory source. Same on the next line. Unrelated churn in a CI PR; worth reverting to keep the diff focused.
There was a problem hiding this comment.
Dockerfile.adapter-with-plugins was reverted in 56f7d8b — git diff origin/development...HEAD -- Dockerfile.adapter-with-plugins is empty, so the trailing-slash churn and the DHI build-arg block are both out of the PR. The diff is now scoped to CI only: .github/workflows/ci.yml, .github/workflows/security.yml, .github/actions/trivy-cache, .githooks/pre-commit, Makefile, tools/trivy-comment.jq, .gitignore, the six deleted dead/deploy workflows, and the go.mod/go.sum bumps from 26e013f.
|
|
||
| on: | ||
| push: | ||
| # push trigger disabled — this always fails (missing Gerrit credentials in |
There was a problem hiding this comment.
Disabling a deploy trigger is unrelated to "add test, security and build-and-push CI workflows". The reason given (missing Gerrit credentials) is plausible, but it's a separate concern with a separate blast radius — please split it out so it can be reviewed and reverted independently.
There was a problem hiding this comment.
There is no longer a disabled trigger to review in isolation — the entire file was deleted in 6220fc4, along with deploy-to-gke.yml, deploy-to-gke-BS.yml, build-and-deploy-plugins.yml, beckn_ci.yml and beckn_ci_test.yml. Deploy is out of this PR's scope per your direction, so the Gerrit-credential question moves with the workflow rather than being half-answered by a commented-out trigger. .github/workflows now holds ci.yml, security.yml and the pre-existing sign-beckn-constants.yml.
CI review — naming consistency, one-liner CI, parallelismReviewed side by side with the sibling PR so the two repos converge rather than drift:
Good news first: structurally these two are already very consistent — same three workflow files, same workflow Three things to fix: job naming is inconsistent in kind, ~100 lines of shell per repo still live in YAML instead of the Makefile, and one workflow is needlessly serial. 1. Are
|
Workflow name: |
File | Job IDs | Check name in UI |
|---|---|---|---|
Run Tests |
run-tests.yml |
lint, test |
lint, test |
Security |
security.yml |
security |
security |
Build and Push |
build-and-push.yml |
build (matrix), manifest |
build (amd64), build (arm64), manifest |
Four inconsistencies:
- Workflow names mix grammatical forms.
Run TestsandBuild and Pushare verb phrases;Securityis a bare noun. Pick one form. - File names mix forms the same way —
run-tests.ymlis verb-prefixed,security.ymlandbuild-and-push.ymlare not. Run Testscontains alintjob, which is not a test. Either the workflow is misnamed or lint is misplaced.- The granularity is uneven, and this is the one that actually costs you something.
lintandtestare single activities.securityis one job doing five: dependency scan → dependency PR comment → image build → image scan → image PR comment → gate. So a redsecuritycheck tells you nothing about which of the five broke, and it can't parallelise (see §3).
Suggested scheme — workflow name: is a bare noun; job ID is <verb> where the object is unambiguous and <verb>-<object> where there's more than one:
| File | name: |
Jobs |
|---|---|---|
ci.yml |
CI |
lint, test |
security.yml |
Security |
scan-deps, scan-image, gate |
release.yml |
Release |
build, manifest |
Two caveats on renaming, worth knowing before you do it:
- Job IDs are the branch-protection contract. Renaming
security→scan-deps/scan-image/gatemeans updating required status checks in both repos' settings, or merges will block on a check name that no longer exists. - discovery-service#15 deletes an existing
.github/workflows/ci.yml. Going back toci.ymlis still the better name, but it's worth being deliberate rather than accidentally re-creating what you just removed.
2. CI as a one-liner — right idea, ~60% done
make is already doing the real work in the test path, and that part is genuinely clean:
- run: make lint
- run: make build
- run: make test-ciBut the other two workflows still carry the implementation inline:
| Workflow | inline shell (network-adapter / discovery-service) |
|---|---|
run-tests.yml |
9 / 9 lines — fine as-is |
security.yml |
53 / 46 |
build-and-push.yml |
57 / 50 |
Five blocks to move, worst first:
a. Assemble Trivy … PR comment — 23 lines × 2 sites × 2 repos = ~92 lines of duplicated jq. This is the strongest case for the Makefile, and it isn't hypothetical: the comments in security.yml record that this exact block already took the whole job down once, because capture(...)?.v is valid on jq 1.8 (a workstation) and a syntax error on jq 1.7.1 (the runner image). Embedded in YAML it can only be tested by pushing a commit. As make trivy-report it runs identically in both places.
trivy-report: # REPORT=trivy-deps.sarif TITLE=... OUT=...- run: make trivy-report REPORT=trivy-deps.sarif TITLE="Go dependency scan" OUT=trivy-deps-comment.md
- run: make trivy-report REPORT=trivy-image.sarif TITLE="Image scan" OUT=trivy-image-comment.mddiscovery-service already has tools/cover-report.awk doing exactly this for the coverage report — the precedent is in the repo, this block just didn't follow it.
b. Tag and push — 22 lines, four near-identical if blocks (GHCR / Docker Hub / ACR / GAR), each doing docker tag + docker push. → make docker-push TAG=…, with the registry list looped in the Makefile.
c. Create multi-arch manifests — 20 lines, including a shell function definition (create_manifest()) and the same four if blocks again. → make docker-manifest VERSION=….
d. The image build doesn't use the target that already exists. Both Makefiles define docker:, and neither workflow calls it — security.yml inlines docker build instead. In network-adapter it's a 7-line block sourcing install/scripts/version-vars.sh; in discovery-service it's a clean one-liner. That's both a missed one-liner and cross-repo drift. → make docker IMAGE=… in both.
e. Set registry flags — 4 lines, duplicated in both jobs of build-and-push.yml. This one genuinely can't move to make (it reads vars.*, which only exists in Actions context) — but it can be hoisted to a workflow-level env: block instead of being pasted twice.
Check coverage's 5-line if/else picks BASE_REF from the event type; that also needs Actions context, so leaving it inline is defensible. Everything else above is portable.
Targets that exist but CI never calls:
- both repos:
docker(see d) - discovery-service:
security(govulncheck ./...), plus theverify/newman/auditgates thatCLAUDE.mddocuments as the stack-dependent checks
That last point is a real cross-repo asymmetry: discovery-service has a make security (govulncheck) target that never runs in CI, and network-adapter has no govulncheck at all. Also worth noting the name collision — security currently means govulncheck as a Makefile target and Trivy as a CI job. Pick one meaning.
3. Parallelism — already right in two places, one real gap
Already correct, no change needed:
- The three workflows are separate files, so they run concurrently on a push/PR.
lint∥test— noneeds:between them.build (amd64)∥build (arm64)→manifest(needs: build). Correct fan-out/fan-in.
The gap is security.yml: one job, six sequential steps, where the dependency scan and the image scan are independent — and the image scan is gated behind a full docker build. Splitting it:
jobs:
scan-deps: # make trivy-deps
scan-image: # make docker + make trivy-image
gate:
needs: [scan-deps, scan-image] # make trivy-gateOne caveat that is the actual work here: make trivy-gate reads trivy-deps.sarif and trivy-image.sarif from the working directory. Split across jobs those are different runners, so the two scan jobs need actions/upload-artifact and gate needs download-artifact. The make call stays a one-liner; the plumbing is the cost. Worth it for the clearer check names alone, but don't split it without wiring the artifacts or the gate will pass vacuously on missing files.
Summary
| Status | |
|---|---|
| Cross-repo structural consistency | ✅ Already close — keep it that way |
| Workflow / job / file naming | |
| CI as one-liner | security + build-and-push (§2) |
make docker / make security unused |
|
| govulncheck asymmetry across repos | |
| Parallelism | ✅ Except security, which is serial (§3) |
Suggested order: (1) split security into three jobs and settle the naming scheme in both repos together, since both touch branch protection; (2) move the jq report block to make trivy-report — biggest duplication win and it has already caused one outage; (3) make docker / docker-push / docker-manifest; (4) decide whether govulncheck is a gate in both repos or neither.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Reviewing the three things you asked about — job naming consistency, is the CI a one-liner, and is the logic in the Makefile — plus the Makefile itself as code, which I had not done before.
Answering the questions first, then the findings.
Is the CI a one-liner? Not yet — measured
| workflow | lines of inline shell |
|---|---|
run-tests.yml |
12 |
security.yml |
57 |
build-and-push.yml |
63 |
| total | 132 |
And the duplication is exact, not approximate. I normalised the app name and hashed the blocks:
- The 16-line jq program that renders the Trivy PR-comment table is byte-identical in all four places it appears — twice in this repo's
security.yml, twice indiscovery-service's. Tag and push(22L),Create multi-arch manifests(20L) andSet registry flags(4L, twice) are byte-identical across the two repos — same md5 after normalisingnetwork-adapter/discovery-service.
Moving the blocks below to make targets takes this repo from 132 → ~20 lines of inline shell:
| step | proposed |
|---|---|
| Assemble Trivy dependency/image PR comment (23L × 2) | make trivy-comment SARIF=… TITLE=… OUT=… |
| Tag and push (22L) | make push-tags |
| Create multi-arch manifests (20L) | make manifests |
| Resolve version vars (7L) | fold into make docker — see build-and-push.yml:136 |
Build image (7L, security.yml) |
make docker — see Makefile:160 |
| Set registry flags (4L × 2) | a composite action, like the dhi-login one this PR already adds |
| Check coverage (5L if/else) | make cover-diff with BASE_REF defaulted in the Makefile |
Can they run in parallel? Mostly already — with one real gap
lint/test are parallel jobs and build fans out over the matrix. Good. But security.yml is one serial job that does: deps scan → docker build → image scan → gate. The deps scan needs nothing but a checkout, so it sits on the critical path behind an image build it has no relationship to. Details on security.yml:23.
Did I review the Makefile? Now yes — and there is a blocker
make test and make cover, both added by this PR, fail on the two packages make test-ci deliberately carves out. Verified by running them against this branch; output on Makefile:40.
A second one worth calling out up front: make docker builds a different Dockerfile than either workflow ships — Dockerfile.adapter vs Dockerfile.adapter-with-plugins. Details on Makefile:160.
Proposed naming format — apply verbatim in both repos
The job ids are already consistent across discovery-service and
network-adapter (lint, test, security, build, manifest — byte for
byte the same). So the answer to "are these consistent?" is yes, the ids
are. Three things are not:
- No job declares
name:. The check string is therefore the raw id, so
branch protection and the PR checks list readRun Tests / lint,
Security / security. - The workflow names mix styles —
Run TestsandBuild and Pushare
verb phrases,Securityis a bare noun. Run Testsunder-describes what it runs. It runslinttoo, so
Run Tests / lintis a self-contradicting check name.
Proposed — one rule: workflow name: is a bare noun naming the stage; job id
stays lowercase and equals the make target it calls; job name: is that id,
title-cased.
| file | workflow name: |
job id | add name: |
resulting check |
|---|---|---|---|---|
run-tests.yml |
CI |
lint |
Lint |
CI / Lint |
test |
Test |
CI / Test |
||
security.yml |
Security |
deps |
Dependency scan |
Security / Dependency scan |
image |
Image scan |
Security / Image scan |
||
gate |
Gate |
Security / Gate |
||
build-and-push.yml |
Release |
build |
Build (${{ matrix.platform }}) |
Release / Build (linux/amd64) |
manifest |
Manifest |
Release / Manifest |
That removes the Security / security stutter, makes CI / Lint honest, and
gives the matrix jobs a legible per-arch name.
⚠️ Both renaming the workflow and addingname:to a job change the check
string, which is the branch-protection contract.Run Tests / lint→
CI / Lintwill silently stop matching a required check. So please settle the
final names in one pass across both repos and update branch protection once —
rather than arriving here in three commits and breaking the gate twice.
|
|
||
| ## test: run the unit and integration suites | ||
| test: | ||
| $(GO) test -race ./... |
There was a problem hiding this comment.
Blocker — test and cover run -race over ./..., which fails on exactly the two packages test-ci carves out. Both targets are added by this PR.
test-ci (L57-66) splits ./pkg/plugin and ./benchmarks/e2e/... into a second, -race-free invocation, and L61-63 says why: "a race-instrumented test binary and a non-race .so mismatch and plugin.Open refuses to load it."
That reasoning applies to test and cover verbatim — both run -race over ./..., and ./... includes those packages. (test-ci proves it does: it has to grep -vE them out.) So the workaround exists in the one target CI runs, and is missing from the two a developer actually types.
Verified against this branch (chore/5-ci-cd-workflows, go1.26.1):
$ go test -race ./pkg/plugin # what `make test` runs
--- FAIL: TestNewManagerSuccess (4.73s)
--- FAIL: TestNewManagerSuccess/valid_config_with_so_file (0.59s)
manager_test.go:347: NewManager() error = failed to open plugin dummy:
plugin.Open("testdata/dummy"): plugin was built with a different
version of package internal/runtime/sys, want nil
FAIL github.com/beckn-one/beckn-onix/pkg/plugin 5.476s
$ go test ./pkg/plugin # same, no -race — the test-ci path
ok github.com/beckn-one/beckn-onix/pkg/plugin 3.545s
$ go test -race ./benchmarks/e2e/... # also in `make test`
FAIL github.com/beckn-one/beckn-onix/benchmarks/e2e 16.236s
So make test is red on a clean checkout of this branch while make test-ci is green — the worst possible split, because the failure only appears to the person running it locally and CI will never tell you.
cover (L43-44) has the same -race ./... problem plus the coverage-build-identity problem described at L50-56, so it fails for two independent reasons.
The fix is to stop repeating the package split and name it once:
# pkg/plugin and benchmarks/e2e build a real .so with a plain `go build
# -buildmode=plugin` subprocess and load it with plugin.Open in the same run.
# A race-instrumented test binary and a non-race .so mismatch, and whole-module
# coverage instrumentation gives shared packages a build identity the subprocess
# build doesn't share — so both -race and -coverpkg make plugin.Open reject it.
PLUGIN_PKGS := ./pkg/plugin ./benchmarks/e2e/...
RACE_PKGS = $$(go list ./... | grep -vE '/pkg/plugin$$|/benchmarks/e2e$$')
test:
$(GO) test -race $(RACE_PKGS)
$(GO) test $(PLUGIN_PKGS)with cover and test-ci built from the same two variables. Today the exclusion regex is written out once, in test-ci only, and the other two targets silently disagree with it.
There was a problem hiding this comment.
Fixed in e99044b. The exclusion is now named once at Makefile:61-62 and shared by all three targets:
PLUGIN_PKGS := ./pkg/plugin ./benchmarks/e2e/...
MAIN_PKGS = $$($(GO) list ./... | grep -vE '/pkg/plugin$$|/benchmarks/e2e$$')test (L82-84) is go test -race $(MAIN_PKGS) then go test $(PLUGIN_PKGS); cover (L87-90) is the same split with -covermode=atomic and two profiles merged by merge-coverage; test-ci (L93-98) is cover through gotestsum. Structurally the same shape you proposed - the only difference is that cover/test-ci write coverage-plugin.out and concatenate it onto coverage.out, so the carve-out does not silently drop those packages' coverage.
Reproduced your finding before and after on this branch (go1.26.1, darwin/arm64): go test -race ./pkg/plugin -> FAIL github.com/beckn-one/beckn-onix/pkg/plugin 4.226s; go test ./pkg/plugin -> ok ... 2.314s. The comment block at L38-60 carries the reasoning for both -race and whole-module coverage instrumentation so the three targets have one written contract to agree with.
| $(GOLANGCI_LINT) fmt ./... | ||
|
|
||
| ## docker: build the adapter image | ||
| docker: |
There was a problem hiding this comment.
make docker builds Dockerfile.adapter; both workflows build Dockerfile.adapter-with-plugins. So make docker does not reproduce what CI scans or ships — and no workflow calls this target.
Makefile:161 docker build -f Dockerfile.adapter -t $(IMAGE) .
security.yml:142 docker build -f Dockerfile.adapter-with-plugins ... -t network-adapter:${{ github.sha }} .
build-and-push.yml:156 file: Dockerfile.adapter-with-plugins
Two separate problems:
- Different Dockerfile.
make trivy-image(which the workflows do call) scans$(IMAGE). Runmake docker && make trivy-imagelocally and you scan the plugin-less image — a clean result that says nothing about what CI gates on. The Makefile hands you a false negative. - Different build args.
security.yml:141-147sourcesinstall/scripts/version-vars.shand passesONIX_VERSION,GIT_COMMIT,GIT_TREE_STATEandBUILD_DATE.make dockerpasses none, so the local image also carries empty version metadata.
Since security.yml's Build image step (7 lines of inline shell) is exactly this command, the fix collapses both problems and shortens the workflow:
## docker: build the shipped adapter image — same Dockerfile and build args CI uses
docker:
. install/scripts/version-vars.sh && \
docker build -f Dockerfile.adapter-with-plugins \
--build-arg ONIX_VERSION="$$ONIX_VERSION" \
--build-arg GIT_COMMIT="$$GIT_COMMIT" \
--build-arg GIT_TREE_STATE="$$GIT_TREE_STATE" \
--build-arg BUILD_DATE="$$BUILD_DATE" \
-t $(IMAGE) .then security.yml's step becomes run: make docker IMAGE=network-adapter:${{ github.sha }}.
If Dockerfile.adapter is genuinely a separate artefact someone needs, keep it — but under a name that says so (docker-adapter / docker-adapter-with-plugins), so that neither target is mistakable for "the image we ship". A target called plain docker that builds the one we don't ship is the trap.
There was a problem hiding this comment.
Fixed in e99044b, docker at Makefile:314-321 - both halves, and the workflow step collapsed as you suggested:
docker:
. install/scripts/version-vars.sh && \
docker build -f Dockerfile.adapter-with-plugins \
--build-arg ONIX_VERSION="$$ONIX_VERSION" \
--build-arg GIT_COMMIT="$$GIT_COMMIT" \
--build-arg GIT_TREE_STATE="$$GIT_TREE_STATE" \
--build-arg BUILD_DATE="$$BUILD_DATE" \
-t $(IMAGE) .security.yml's inline build is now - run: make docker IMAGE=network-adapter:${{ github.sha }} (security.yml:132), with make trivy-image IMAGE=... on the next line, so the local pair scans the identical image CI gates on. Kept the single name docker rather than splitting into docker-adapter / docker-adapter-with-plugins: nothing in this repo builds Dockerfile.adapter on its own - no workflow, no script, no compose file - so a second target would be dead the day it landed. Dockerfile.adapter itself is untouched by this PR (reverted byte-identical to development in 56f7d8b), it is just no longer reachable from a make target.
|
|
||
| ## help: list the available targets | ||
| help: | ||
| @grep -hE '^## ' $(MAKEFILE_LIST) | sed 's/^## / /' | sort |
There was a problem hiding this comment.
make help is unreadable: sort tears every multi-line ## block away from its target.
This is the first thing anyone typing make sees. sort orders every ## line independently, and most help text in this file is multi-line, so the continuations get alphabetised among themselves:
$ make help | head -5
arch-tagged local image, before anything is pushed.
finding instead of writing a report — the pre-push
release gate build-and-push.yml runs once per
dependency scan above cannot see. IMAGE names the ref to scan.
so a Go toolchain CVE shows up here and nowhere else that the
Measured: 25 lines of output, of which only 15 name a target — 10 are orphaned fragments, and the entire first screen contains no target name at all. (discovery-service has the same bug, worse: 70 lines, 32 targets, 38 fragments.)
Simplest fix — grep only the lines that carry a target name, and leave the continuations as source comments, where they read fine:
help:
@grep -hE '^## [a-z]' $(MAKEFILE_LIST) | sed 's/^## / /' | sortIf you want the continuations displayed, they have to be emitted with their parent, which means grouping before sorting or dropping sort and relying on file order.
There was a problem hiding this comment.
Fixed in e99044b, Makefile:72 - took your one-line version:
help:
@grep -hE '^## [a-z]' $(MAKEFILE_LIST) | sed 's/^## / /' | sortand reflowed every multi-line ## block in the file down to a single ## target: one-line summary, with the detail demoted to plain # comments above the recipe where it reads in source. Measured on the current tree: make help is 17 lines, all 17 name a target, zero orphaned fragments. For comparison, the same pipeline against the version you reviewed (b81a00a) gives 25 lines for 15 targets.
| ## cover-diff: coverage restricted to files changed vs BASE_REF — a PR review | ||
| ## needs the diff's number, not the whole repo's. On failure, | ||
| ## names the changed files dragging the number down (worst first). | ||
| cover-diff: coverage.out |
There was a problem hiding this comment.
cover-diff declares a file prerequisite with no rule to build it, so it only works if something else ran first.
$ make cover-diff
make: *** No rule to make target `coverage.out', needed by `cover-diff'. Stop.
It survives in CI only because make test-ci (run-tests.yml:45) happens to write the file one step earlier. Locally, on a clean checkout, the target is unrunnable — and the error names a file, not the target you should have run instead.
Either give coverage.out a rule:
coverage.out:
$(MAKE) coveror make it cover-diff: cover. The first is better — it keeps cover-diff cheap in CI, where the profile already exists, rather than re-running the whole suite.
(Note this interacts with the cover blocker on Makefile:40: cover currently fails, so cover-diff: cover would inherit that failure until L43-44 is fixed.)
There was a problem hiding this comment.
Fixed in e99044b, Makefile:105-109, taking the file-rule option you preferred:
# cover-diff needs a profile but must not re-run the suites in CI, where
# test-ci already wrote one. A file rule gives it both: present (CI) and make
# skips this; absent (clean local checkout) and it runs the suites once.
coverage.out:
@$(MAKE) --no-print-directory coverYour parenthetical about the interaction with the cover blocker was the binding constraint - cover is fixed in the same commit (Makefile:87-90, split into -race $(MAIN_PKGS) plus a plain $(PLUGIN_PKGS) run merged by merge-coverage), so the file rule does not inherit a failure. Verified: with coverage.out removed, make -n cover-diff now emits make --no-print-directory cover and the two go test invocations instead of erroring out.
| trivy-gate: | ||
| @fail=0; \ | ||
| for report in trivy-deps.sarif trivy-image.sarif; do \ | ||
| count=$$(jq '[.runs[].results[]?] | length' "$$report"); \ |
There was a problem hiding this comment.
trivy-gate exits 0 when a report is missing — the gate passes having checked nothing.
jq on a nonexistent file writes an error to stderr and prints nothing, so count is the empty string; [ "" -gt 0 ] is itself an error, not a true branch, so fail stays 0 and the loop moves on. Reproduced with this exact recipe:
$ sh trivy-gate.sh # no sarif files present
jq: error: Could not open file trivy-deps.sarif: No such file or directory
trivy-deps.sarif: HIGH,CRITICAL
[: : integer expression expected
jq: error: Could not open file trivy-image.sarif: No such file or directory
trivy-image.sarif: HIGH,CRITICAL
[: : integer expression expected
>>> exit 0
Note the output line trivy-deps.sarif: HIGH,CRITICAL — it reads like a scan result with the count elided, not like a failure. That is what makes it dangerous rather than merely untidy.
Scope, stated honestly: in security.yml today this does not fire, because the two scan steps are not continue-on-error and the gate is the last step, so a failed scan aborts the job before the gate runs. It fires in two places that matter anyway:
- Locally.
security.yml:210-212says the point of this target is that "a localmake trivy-gatechecks the exact same two files this job just wrote." On a clean checkout, or aftermake clean, or after running onlymake trivy-deps,make trivy-gateis green having scanned nothing. Half-green is the common case: runmake trivy-depsalone and the deps half reports honestly while the image half passes vacuously. - The moment someone adds
if: always()to the gate step — which is the natural next change, and the one the parallel split onsecurity.yml:23would require.
for report in trivy-deps.sarif trivy-image.sarif; do \
+ if [ ! -f "$$report" ]; then \
+ echo "$$report: MISSING — no scan produced it"; \
+ fail=1; continue; \
+ fi; \
count=$$(jq '[.runs[].results[]?] | length' "$$report"); \A gate that cannot find its evidence should fail, for the same reason the comment above it gives for not rescanning: the one state nobody can act on.
There was a problem hiding this comment.
Fixed in e99044b (Makefile:216-235), and your second scope case is now the actual topology, so the guard is load-bearing rather than defensive. 4f79614 split security.yml into three jobs: dependency-scan and image-scan each upload their SARIF as an artifact under if: always() (security.yml:101-105, 167-171), and security-gate is needs: [dependency-scan, image-scan] with if: always() (security.yml:177-182) so that a failed scan job surfaces as a missing report instead of a skipped gate.
The recipe guards both shapes:
if [ ! -s "$$report" ]; then echo "$$report: MISSING - no scan produced it"; fail=1; continue; fi; \
count=$$(jq '[.runs[].results[]?] | length' "$$report" 2>/dev/null); \
if [ -z "$$count" ]; then echo "$$report: UNREADABLE - not valid SARIF"; fail=1; continue; fi; \Verified locally. With no reports present: both lines print MISSING, make: *** [trivy-gate] Error 1. With a valid empty deps report and not json{{{ as the image report: trivy-deps.sarif: 0 HIGH,CRITICAL / trivy-image.sarif: UNREADABLE - not valid SARIF, exit 1. Used -s rather than -f so a zero-byte file created by a truncated write is caught too, and 2>/dev/null on the jq so the empty-string branch is what reports the failure rather than a raw jq error competing with it.
| @@ -0,0 +1,107 @@ | |||
| name: Run Tests | |||
There was a problem hiding this comment.
Proposed naming format — apply verbatim in both repos
The job ids are already consistent across discovery-service and
network-adapter (lint, test, security, build, manifest — byte for
byte the same). So the answer to "are these consistent?" is yes, the ids
are. Three things are not:
- No job declares
name:. The check string is therefore the raw id, so
branch protection and the PR checks list readRun Tests / lint,
Security / security. - The workflow names mix styles —
Run TestsandBuild and Pushare
verb phrases,Securityis a bare noun. Run Testsunder-describes what it runs. It runslinttoo, so
Run Tests / lintis a self-contradicting check name.
Proposed — one rule: workflow name: is a bare noun naming the stage; job id
stays lowercase and equals the make target it calls; job name: is that id,
title-cased.
| file | workflow name: |
job id | add name: |
resulting check |
|---|---|---|---|---|
run-tests.yml |
CI |
lint |
Lint |
CI / Lint |
test |
Test |
CI / Test |
||
security.yml |
Security |
deps |
Dependency scan |
Security / Dependency scan |
image |
Image scan |
Security / Image scan |
||
gate |
Gate |
Security / Gate |
||
build-and-push.yml |
Release |
build |
Build (${{ matrix.platform }}) |
Release / Build (linux/amd64) |
manifest |
Manifest |
Release / Manifest |
That removes the Security / security stutter, makes CI / Lint honest, and
gives the matrix jobs a legible per-arch name.
⚠️ Both renaming the workflow and addingname:to a job change the check
string, which is the branch-protection contract.Run Tests / lint→
CI / Lintwill silently stop matching a required check. So please settle the
final names in one pass across both repos and update branch protection once —
rather than arriving here in three commits and breaking the gate twice.
One extra note specific to this file: Run Tests is also the filename, so renaming the workflow to CI without renaming the file leaves run-tests.yml containing name: CI. Rename the file to ci.yml in the same commit — a workflow file whose name disagrees with its name: is the next person's wrong grep.
There was a problem hiding this comment.
Applied, with two deliberate deviations from the table.
Done as proposed: run-tests.yml renamed to ci.yml with name: CI in the same commit (8236623), and both jobs got name: — ci.yml:33 name: Lint, ci.yml:50 name: Test. Checks now read CI / Lint and CI / Test.
Deviations, both in security.yml (4f79614): the job ids are dependency-scan / image-scan / security-gate, not deps / image / gate, and the gate is name: Security gate, not Gate. The id rule "equals the make target it calls" does not survive contact here — the targets are trivy-deps / trivy-image / trivy-gate and each scan job calls two or three of them, so the id names the stage instead. On the gate name I chose Security / Security gate over Security / Gate because "Gate" alone is unreadable sitting next to CI / Lint in a checks list; that reintroduces a mild stutter, which is a real cost of the choice.
build-and-push.yml was deleted in 56f7d8b, so the Release rows are moot for this repo.
The current check strings are CI / Lint, CI / Test, Security / Dependency scan, Security / Image scan, Security / Security gate. On your branch-protection warning: these are final from my side, one pass, no further renames — but if you want the table verbatim (gate / Gate) say so now and I will change it in this PR rather than after protection is updated.
| # of that text rather than from dedicated JSON fields. | ||
| # continue-on-error: a report-formatting step must never be able to | ||
| # fail the job — the gate at the bottom is what decides that. | ||
| - name: Assemble Trivy dependency PR comment |
There was a problem hiding this comment.
This 16-line jq program is byte-identical in four places across the two repos. Move it to the Makefile.
I extracted the four blocks, normalised the SARIF filename, and diffed:
na security.yml block 1 vs na block 2 IDENTICAL
na security.yml block 1 vs ds block 1 IDENTICAL
ds security.yml block 1 vs ds block 2 IDENTICAL
One program, copy-pasted four times. discovery-service's copy of this comment records what that already cost: capture(...)?.v is valid on jq 1.8 and a syntax error on jq 1.7.1, it took the whole job down, and the fix had to be applied in all four places.
Proposed, parameterised so one target serves both scans and both repos:
## trivy-comment: render a SARIF report as a markdown PR comment table.
## SARIF names the report, TITLE the heading, OUT the file.
trivy-comment:
@{ \
echo "<!-- sec-scan:$(basename $(notdir $(SARIF))) -->"; \
echo "### 🛡️ Trivy — $(TITLE) ($(SEVERITY))"; \
echo "[View full run]($(RUN_URL))"; \
echo; \
jq -r -f tools/trivy-comment.jq $(SARIF); \
} > $(OUT)and the step collapses to the one-liner you asked for:
- name: Assemble Trivy dependency PR comment
if: github.event_name == 'pull_request'
continue-on-error: true
run: make trivy-comment SARIF=trivy-deps.sarif TITLE="Go dependency scan" OUT=trivy-deps-comment.mdPutting the jq in tools/trivy-comment.jq rather than inside the recipe matters too: as a file it is lintable (jq -n -f), diffable, and free of the $$/backslash-continuation escaping that makes the current block hard to read and hard to edit correctly.
Worth saying plainly: this would not by itself have caught the jq 1.7.1-vs-1.8 break, because a workstation has 1.8 either way. What it fixes is that the break had to be repaired in four places instead of one — and that this repo inherited the bug by copy rather than by reference.
There was a problem hiding this comment.
Implemented close to your sketch in e99044b. tools/trivy-comment.jq holds the program; Makefile:199 holds the target:
trivy-comment:
@test -n "$(SARIF)" -a -n "$(TITLE)" -a -n "$(OUT)" || \
{ echo "::error::trivy-comment needs SARIF, TITLE and OUT"; exit 1; }
@test -s "$(SARIF)" || \
{ echo "::error::$(SARIF) missing or empty — no scan produced it"; exit 1; }then the marker/heading/run-link echoes and jq -r -f tools/trivy-comment.jq "$(SARIF)". Both call sites are one-liners: security.yml:73 and security.yml:145 (4f79614).
Two changes on top of the sketch. The test -s precondition: without it a missing SARIF renders a table saying "No HIGH or CRITICAL findings", which is the same failure mode as the vacuous gate pass, just in the comment. And the jq itself is .runs[]?.results[]? with def val(re; default): (capture(re) // {v: default}).v; — // on the null capture rather than the ?.v form that was the jq 1.7.1 syntax error, so it parses on both. Lints clean with jq -n -f tools/trivy-comment.jq.
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| security: |
There was a problem hiding this comment.
security is one serial job doing four things; two of them are independent and could overlap.
The current sequence is: deps scan → docker build → image scan → gate. The dependency scan needs nothing but the checkout, so it sits idle on the critical path behind an image build it has no relationship to.
You asked whether these can run in parallel — here is the one place in either repo where the answer is "not yet, and it's worth it":
jobs:
deps:
name: Dependency scan
steps: [checkout, trivy cache, make trivy-deps, upload-sarif,
make trivy-comment, upload-artifact trivy-deps.sarif]
image:
name: Image scan
steps: [checkout, trivy cache, dhi login, make docker,
make trivy-image, upload-sarif, make trivy-comment,
upload-artifact trivy-image.sarif]
gate:
name: Gate
needs: [deps, image]
if: always()
steps: [download-artifact, make trivy-gate]Three things this buys beyond wall-clock:
- The deps scan overlaps the image build entirely — and this repo's image build is the heavier of the two, since it builds plugins.
Security / Dependency scanandSecurity / Image scanbecome separately-red checks, so the PR says which scan found something without opening the log.gatewithneeds:+if: always()runs even when one scan fails — which is what a gate should do, and is exactly the arrangement that turns the vacuous-pass bug I flagged onMakefile:141from latent into live. Fix that one first, or in the same PR, or this refactor makes the release gate silently green.
The cost is the SARIF artifact round-trip and a duplicated checkout/cache block in two jobs. Given the gate is the release control, I think that trade is worth it — but flagging it as a trade rather than a free win.
There was a problem hiding this comment.
Split in 4f79614, essentially as sketched. security.yml:33 dependency-scan (name Dependency scan), :109 image-scan (name Image scan), :175 security-gate with needs: [dependency-scan, image-scan] and if: always(). SARIF crosses jobs as artifacts — upload-artifact at :101 and :167, both if: always() with if-no-files-found: ignore, and one download-artifact at :189 with pattern: trivy-*-sarif / merge-multiple: true. The duplicated checkout+cache block you priced in is factored into .github/actions/trivy-cache, so each scan job pays two lines for it.
The vacuous-pass bug went first, in the Makefile move (e99044b), so the split never ran against the broken gate. Makefile:216 trivy-gate now treats an absent or empty report as MISSING and an unparsable one as UNREADABLE, both setting fail=1, alongside the finding count — if-no-files-found: ignore on the upload is what routes a dead scan job into that path instead of into a second red check.
Verified live rather than by reading: the current run gate logs trivy-deps.sarif: 0 HIGH,CRITICAL and trivy-image.sarif: 20 HIGH,CRITICAL, then fails. The 20 are all Go stdlib v1.26.1 toolchain CVEs, which is a real outstanding issue tracked on the go.mod thread — not a gate defect.
|
|
||
| # Re-tag the already-scanned local image for each enabled registry and | ||
| # push it there. No rebuild, no re-scan. | ||
| - name: Tag and push |
There was a problem hiding this comment.
Tag and push (22 lines) is byte-identical to discovery-service's. Same for Create multi-arch manifests (20 lines).
Hashed with the app name normalised:
na tag-and-push 666e75e295cea971aaba2bdfa1afe60f
ds tag-and-push 666e75e295cea971aaba2bdfa1afe60f
na manifests 9d8f277b346056021d000d98b5ede18f
ds manifests 9d8f277b346056021d000d98b5ede18f
Both are also internally repetitive — four near-identical per-registry if blocks in Tag and push, and a shell function plus the same four blocks in Create multi-arch manifests.
make push-tags / make manifests, driven by a single REGISTRIES list, kills both the cross-repo duplication and the four-way repetition:
## push-tags: retag the local per-arch image into every enabled registry
push-tags:
@for reg in $(REGISTRIES); do \
[ -n "$$reg" ] || continue; \
docker tag $(LOCAL_IMAGE) "$$reg/$(IMAGE_NAME):$(TAG)"; \
docker push "$$reg/$(IMAGE_NAME):$(TAG)"; \
doneand the step becomes run: make push-tags.
Since the two files are already identical here, the stronger version of this is a shared reusable workflow (workflow_call) or a composite action in one repo referenced by both — otherwise the next registry gets added twice and the second one gets forgotten. That is not hypothetical: the jq block on security.yml:77 is the same pattern one stage further along, where the copies have already had to be fixed independently.
There was a problem hiding this comment.
Both steps were deleted with build-and-push.yml in 56f7d8b, so the cross-repo byte-identical duplication and the four-way per-registry repetition are out of this PR — there is no push or manifest logic left to hoist into make push-tags / a REGISTRIES list. The jq case you cite as the same pattern one stage further along is fixed in what shipped: the program now lives in tools/trivy-comment.jq and is invoked through one trivy-comment target (Makefile:195-210) from security.yml:73 and security.yml:145, so the dependency and image reports render from a single copy. The reusable-workflow point for a returning release path is noted.
| # pkg/version) — written to $GITHUB_ENV so the build-push-action step | ||
| # below can reference them as build-args the same way a local | ||
| # `source install/scripts/version-vars.sh` would. | ||
| - name: Resolve version vars |
There was a problem hiding this comment.
Resolve version vars — 7 lines of inline shell that exist only because make docker doesn't source this script itself.
This step sources install/scripts/version-vars.sh and re-exports four values into $GITHUB_ENV so the Build (local, unpushed) step below can pass them as build args. discovery-service has no equivalent step, which is the one genuine structural asymmetry between the two build-and-push.yml files.
The script already exists, so the logic isn't the problem — the placement is. As written, the version metadata is assembled by the workflow, so a local make docker builds an image with all four values empty. Fold the sourcing into the make target (see Makefile:160) and both the workflow step and the asymmetry disappear:
- name: Build (local, unpushed)
run: make docker IMAGE=${{ steps.image.outputs.local }}docker/build-push-action is doing real work here for the per-arch buildx path, so if you'd rather keep the action, the smaller version of this is still worth it: have the Makefile expose the vars (make print-version-vars) rather than have the workflow know the script's variable names. Right now ONIX_VERSION, GIT_COMMIT, GIT_TREE_STATE and BUILD_DATE are named in the workflow, in security.yml:141-147, and in the script — three places that have to agree.
There was a problem hiding this comment.
The step was deleted with build-and-push.yml in 56f7d8b, and the fix you described is what shipped: make docker sources install/scripts/version-vars.sh itself and passes the four values as build args (Makefile:314-321), so ONIX_VERSION, GIT_COMMIT, GIT_TREE_STATE and BUILD_DATE are named in the script and that one recipe, nowhere else. security.yml:132 is now just make docker IMAGE=network-adapter:${{ github.sha }} — no $GITHUB_ENV step, no docker/build-push-action, no var names in the workflow — which also means a local make docker produces the same version metadata CI does instead of four empty strings.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
Parity with discovery-service#15 — one finding.
I've just posted a supplementary review on ds#15 covering eight findings, seven of which this PR already carries (I raised them here in #5128738338 and #5130308856). The eighth had only ever been raised against discovery-service — on 2026-09-01, and never fixed — and the tag filter here is byte-identical, so it is live in this repo too and nobody had been told.
That's the whole content of this review: one blocker, so the two PRs end up carrying the same finding set. Nothing else new.
| on: | ||
| push: | ||
| tags: | ||
| - "v[0-9]+.[0-9]+.[0-9]+" |
There was a problem hiding this comment.
Blocker — GitHub tag filters are globs, not regexes. + is a literal character here, so no release tag will ever match and this workflow can never trigger.
on.push.tags is matched with fnmatch-style globbing, not a regex. In a glob [0-9] matches exactly one digit and + matches a literal + — so v[0-9]+.[0-9]+.[0-9]+ requires a + character after each digit group:
v1.2.0 matches: NONE <-- workflow never triggers
v0.0.1 matches: NONE
v1.2.0-rc1 matches: NONE
v1+.2+.0+ matches: v[0-9]+.[0-9]+.[0-9]+
Every tag this convention actually produces fails to match; the only strings that do match are ones nobody would ever push. So the release path has never fired, and won't.
The comment above states the intent correctly — it's the glob that doesn't express it. * is the quantifier glob has:
on:
push:
tags:
- - "v[0-9]+.[0-9]+.[0-9]+"
- - "v[0-9]+.[0-9]+.[0-9]+-rc[0-9]+"
+ - "v[0-9]*.[0-9]*.[0-9]*"
+ - "v[0-9]*.[0-9]*.[0-9]*-rc[0-9]*"That keeps the leading-digit requirement (so vfoo.bar.baz still doesn't match) while allowing multi-digit components like v1.12.0. It is looser than the stated intent — v1.2.3-rc4extra would slip through — so if excluding -test/-beta-style suffixes needs to be airtight, glob can't express it alone and the check belongs in a first job step that inspects github.ref_name and exits early. Given a tag push is deliberate and the current state is no releases at all, I'd take the glob fix now and treat the strict check as a separate change.
Worth fixing in both repos in the same pass, since the filters are identical and will otherwise drift: ds#15 build-and-push.yml:9.
Two adjacent findings I've already raised here interact with this one, so they're best fixed together: :12 (id-token: write) — because the release will still fail on the GAR leg once tags start matching — and :260 (:latest on -rc tags), which only becomes reachable once -rcN tags match at all.
There was a problem hiding this comment.
The file was deleted in 56f7d8b, so there is no on.push.tags filter anywhere in this PR and the glob-vs-regex bug has no live instance — the shipped workflows trigger only on push to main/development and on pull_request (ci.yml:7-12, security.yml:11-16). The finding is correct and worth carrying to whichever PR reintroduces the release path, along with the two it interacts with; the -rc/:latest and id-token issues are likewise gone only because their workflow is. Note that make lint-actions (actionlint, pinned at Makefile:27 and run by .githooks/pre-commit) would not have caught this one — a glob that parses fine but matches nothing is outside what it checks — so the strict-tag check as a first job step remains the reliable form.
There was a problem hiding this comment.
Correction to my earlier reply — I was wrong to call this correct, and the release path is back.
Two things changed since that reply:
on.push.tagsis live again, in.github/workflows/ci-release.yml:22-26. My earlier "no live instance" is stale.- The premise is false.
+is not a literal in GitHub filter patterns — it is a quantifier meaning "one or more of the preceding character".
Evidence, from actionlint's own validator (the same pinned binary make lint-actions runs):
tags: ["+leading"]
invalid glob pattern. unexpected character '+' while checking
special character + (one or more). the preceding character must
not be special character. [glob]
tags: ["v[0-9]+.[0-9]+.[0-9]+"]
(clean)
GitHub's own docs agree that + is special: the branches/tags keywords "accept glob patterns that use characters like *, **, +, ?, !", and those characters need escaping if you want a literal match. There would be nothing to escape if + were already literal.
So v[0-9]+.[0-9]+.[0-9]+ does match v1.2.0, and the + is required rather than harmless — v[0-9].[0-9].[0-9] would fail on a multi-digit component like v1.10.1.
The match table in the original comment is inverted: v1+.2+.0+ does not match, because once [0-9]+ consumes the 1, the literal . cannot match a +.
Not yet proven end-to-end: no tag has been pushed, so the first real release tag is what confirms the trigger fires. Patterns cover both spellings (1.0.0 and v1.0.0, plus pre-release suffixes) for that reason.
manjudr
left a comment
manjudr
left a comment
There was a problem hiding this comment.
What "consistent" should mean, concretely
discovery-service#15 and network-adapter#14 ship near-identical CI. This is the definition of consistency the two should be held to, so it can be handed to whoever picks up the fixes without them having to guess. It is the same document on both PRs — the counterpart is discovery-service#15.
Read this first, because it changes the size of the job: the naming is already consistent. I checked, expecting to find drift, and there isn't any worth fixing.
| discovery-service | network-adapter | ||
|---|---|---|---|
| workflow files | run-tests.yml, security.yml, build-and-push.yml |
same three | ✅ |
workflow name: |
Run Tests, Security, Build and Push |
identical | ✅ |
| job ids | lint, test / security / build, manifest |
identical | ✅ |
| step names | Trivy cache date, Trivy dependency scan (report), Assemble Trivy dependency PR comment, Gate on HIGH or CRITICAL findings, … |
identical | ✅ |
concurrency groups |
test-${{ github.ref }}, security-${{ github.ref }} |
identical | ✅ |
| Makefile target names | 31 targets | 15 targets, a strict subset — every na name exists in ds | ✅ |
So the ask isn't a rename. The five rules below are what's actually missing, in the order I'd do them.
R1 — Same target name ⇒ same guarantee. (the real gap)
The names match; three of them don't mean the same thing, and in one case the shared docstring is false.
Both Makefiles say, verbatim:
## test: run the unit and integration suitesIn discovery-service that is true. In network-adapter it cannot be:
$ go test -race ./pkg/plugin
plugin.Open("testdata/dummy"): plugin was built with a different version of
package internal/runtime/sys, want nil
FAIL
$ go test ./pkg/plugin
ok
network-adapter's test-ci already knows this — it excludes pkg/plugin and benchmarks/e2e, runs them separately without -race, and explains why in a comment. But test: and cover: (both added by this PR) don't, so the everyday entrypoint fails on a clean checkout while CI is green. Same for cover, which additionally lacks the -coverpkg that discovery-service needs and that discovery-service's own test-ci is also missing.
| target | discovery-service | network-adapter | should be |
|---|---|---|---|
test |
passes | fails (-race + plugins) |
passes in both, or the docstring says which packages it skips |
cover |
has -coverpkg |
no -coverpkg |
whatever each repo's layout needs — but test-ci must match cover |
docker |
root Dockerfile |
Dockerfile.adapter — and CI builds Dockerfile.adapter-with-plugins |
one image definition per repo, referenced from one place |
The rule: if two repos share a target name, an engineer moving between them must not have to re-learn what it does. Where a repo genuinely differs, the difference goes in the ## docstring rather than in the reader's memory.
R2 — CI calls make; the Makefile holds the implementation.
This is the principle both repos state and neither reaches. Counting non-make shell inside run: blocks across the three workflows:
| inline shell lines | make one-liners |
|
|---|---|---|
| discovery-service | 112 | 4 |
| network-adapter | 125 | 4 |
The Trivy path is the model to copy — run: make trivy-deps, run: make trivy-image IMAGE=…, run: make trivy-gate. Apply the same treatment to the blocks that are logic rather than glue, largest first. All of these exist in both repos:
| block | size | proposed target |
|---|---|---|
Assemble Trivy dependency PR comment |
23 lines | make trivy-comment KIND=deps |
Assemble Trivy image PR comment |
23 lines | make trivy-comment KIND=image — it is the same 16-line jq program as the one above, and the pair is byte-identical across both repos: 4 copies of one program |
Tag and push |
22 lines | make release-push (byte-identical across repos) |
Create multi-arch manifests |
20 lines | make release-manifest (byte-identical across repos) |
Build image |
1 line here / 7 lines there | make docker — see R1; today neither repo's CI uses its own docker target |
Check coverage |
5 lines | already make cover-diff; the if picking BASE_REF is the only part that must stay in YAML |
What legitimately stays inline: ${{ }} expression plumbing, $GITHUB_ENV / $GITHUB_OUTPUT writes, and if: conditions. Those are GitHub's, not the build's. Set registry flags and Set image owner are borderline — they're pure $GITHUB_ENV writes, so leaving them is defensible.
The test for whether this is done: can a developer reproduce a CI failure locally by running the one command the log shows? Today, for anything Trivy-comment- or release-related, no.
R3 — Shared code is shared, not copied.
Duplication I measured, all of it live in both repos:
build-and-push.yml: the registry-auth preamble is duplicated within each file —discovery-servicebuildL64-124 vsmanifestL179-222 is 44 lines byte-identical;network-adapterthe same at L205.Tag and push,Create multi-arch manifests,Set registry flags: byte-identical across the two repos.- The 16-line jq program: 4 copies (2 per repo).
So adding a fifth registry is currently an eight-place edit across two repos. Two mechanisms, and they compose:
- Composite action for the in-repo duplication —
.github/actions/registry-auth/action.ymltaking the four*_ENABLEDflags,uses:'d by both jobs. This PR already establishes the pattern with.github/actions/dhi-login, so it extends a decision already made rather than introducing one. workflow_callreusable workflow in a shared repo for the cross-repo duplication. Bigger; I'd keep it out of these PRs and file it.
Anything moved into a make target under R2 stops being duplicated across repos only if the Makefile itself is shared — otherwise you get 2 copies instead of 4. Worth deciding which you're buying.
R4 — Trigger and permission shape, stated once and matched.
Identical in both repos today, and wrong in the same four ways in both. Details and diffs are on the inline comments; the invariants are:
- Tag filters are globs, not regexes.
v[0-9]+.[0-9]+.[0-9]+matchesv1+.2+.0+and notv1.2.0. Live in both repos; the release path has never fired in either. push: branches:must include the branch the work actually lands on. Both list only[main]; both PRs target something else, so every push-triggered path is untested.id-token: writewherevergoogle-github-actions/authappears. Missing in both; the GAR leg cannot mint a token.- The release workflow needs
concurrencytoo — withcancel-in-progress: false, the opposite of the other two, because a half-cancelled release leaves arch tags with no manifest. Andfail-fast: falseon the matrix, so one arch failing doesn't hide the other's verdict.
R5 — Add job name: fields — last, and deliberately.
No job in either repo sets name:, so the checks render as bare ids: lint, test, security, build (linux/amd64). Proposed, identical in both:
| workflow | job id | name: |
renders as |
|---|---|---|---|
run-tests.yml |
lint |
Lint |
CI / Lint |
run-tests.yml |
test |
Test |
CI / Test |
security.yml |
security |
Dependency and image scan |
Security / Dependency and image scan |
build-and-push.yml |
build |
Build (${{ matrix.platform }}) |
Release / Build (linux/amd64) |
build-and-push.yml |
manifest |
Multi-arch manifest |
Release / Multi-arch manifest |
name: is safe — but if you also rename the workflow (Run Tests → CI), every required-check string changes and merges block until the rules are updated. Cosmetics shouldn't be what wedges the queue, so land the functional fixes first.
Suggested order
- R4 — four small, mechanical, blocking fixes. The release path doesn't work in either repo without them.
- R1 — make
testhonest innetwork-adapter; aligncover/test-ci; pick one Dockerfile per repo. - R2 — move the four large blocks behind
maketargets, biggest first. - R3 — composite action for the in-repo copies; file the cross-repo reusable workflow separately.
- R5 — job names, once the above has landed.
R4 and R1 are worth doing as one commit per repo, applied to both in the same sitting — they're the ones that will silently diverge otherwise.
Specific to this PR: R1 lands hardest here. test: and cover: are both added by this PR and both fail on a clean checkout, because -race cannot load a Go plugin built without it — while test-ci right beside them already handles this correctly and documents why. That gap between "what CI runs" and "what the README tells a new developer to run" is the single thing I'd fix first; details on Makefile:39.
None of these six can run or need to run on this repo as it stands:
beckn_ci.yml, beckn_ci_test.yml trigger on PRs to
beckn-onix-v1.0-develop, a branch that
does not exist here — they have never
fired and cannot.
deploy-to-gke.yml,
deploy-to-gke-BS.yml,
build-and-deploy-plugins.yml,
onix-gcp-terraform-deploy.yml deployment, which is out of scope for
now. Keeping them means keeping the
GCP/registry secret surface and the
maintenance cost of workflows nobody
runs.
What is left is CI, Security and sign-beckn-constants.yml.
build-and-push.yml is release/deploy, which is out of scope for now, so it
goes with the other deploy workflows. That also retires the review findings
that only existed inside it (the tag globs, id-token: write with no consumer,
:latest on a -rc tag, the duplicated registry-login block, and the multi-arch
manifest step).
The Docker Hardened Images switch went with it, and it had to:
Dockerfile.adapter was migrated to dhi.io in a PR whose stated
scope is CI workflows. dhi.io is
subscription-gated, so the image is not
pullable without the DHI entitlement.
Dockerfile.adapter-with-plugins same switch, plus trailing-slash churn. Back
to golang:1.26.1-bookworm building and
cgr.dev/chainguard/wolfi-base as the runtime
— which has a shell and glibc, so
plugin.Open works, and is publicly pullable,
so a fork PR's image scan still builds.
Deployment/deployment.yaml the added args: ["--config=$(CONFIG_FILE)"]
block does not match the adapter's flags and
would CrashLoopBackOff on rollout.
.github/actions/dhi-login only existed to authenticate to dhi.io.
All three files are now byte-identical to development.
…es [#5] The Security workflow this PR adds is only worth adding if it can be green, and on development it is not: trivy fs flags three HIGH/CRITICAL advisories in the dependency graph. golang.org/x/crypto v0.54.0 -> v0.55.0 google.golang.org/grpc v1.82.1 -> v1.83.1 github.com/rabbitmq/amqp091-go v1.11.0 -> v1.13.0 `go mod tidy` carried the transitive minimums along with them (otel 1.43 -> 1.44, x/net 0.56 -> 0.57, x/text 0.40 -> 0.41, genproto). No source change was needed: `go build -trimpath ./cmd/adapter/...` and `go vet ./...` are both clean, and the full suite passes.
Review feedback was that the workflows carried the logic and the Makefile
carried aliases — 132 lines of inline shell across the three workflows, none
of it runnable locally. That is inverted here: the Makefile is the single
source of truth and every CI step becomes a one-line `make <target>` call, so
a red check reproduces by running the command the log shows.
Fixes, not just relocation:
test / cover both failed on development. pkg/plugin and benchmarks/e2e
build a real .so and plugin.Open it in the same run, and
both -race and whole-module coverage instrumentation make
that .so unloadable. They now run as their own invocation.
The carve-out is named once (PLUGIN_PKGS/MAIN_PKGS) and
shared by test, cover and test-ci, so the three cannot
disagree about it.
-coverpkg answered in a comment rather than added: benchmarks/e2e is
the only test-only package in the module and it is in the
carve-out precisely because whole-module instrumentation is
what breaks plugin.Open. It cannot be added without
reintroducing the failure.
cover-diff no longer swallows a broken base. An unresolvable BASE_REF
and a failed `git diff` are both hard errors with an
::error:: annotation — an unknown changed-file set is not an
empty one. It writes coverage-report.md on every exit path,
including those two, so the workflow can read the file
unconditionally. BASE_REF defaults to origin/development,
the branch PRs actually target.
merge-coverage `;` not `&&`, and always removes coverage-plugin.out, so a
failed merge cannot leave a half-profile behind to be
misread. coverage-plugin.out is gitignored as well.
coverage.out a file rule, so cover-diff has a buildable prerequisite: it
reuses the profile test-ci already wrote in CI, and runs
the suites once on a clean local checkout.
trivy-gate a missing or unparsable SARIF report is now a failure. It
used to pass vacuously when a scan wrote nothing, which is
the one case a security gate must not be green for.
docker builds Dockerfile.adapter-with-plugins — the image that
ships and that security.yml scans — not Dockerfile.adapter.
Version build args come from install/scripts/version-vars.sh
so they are spelled out in one place.
trivy-deps --skip-dirs bin, so the scanner stops reporting on the
trivy binary the cache restored next to it.
help one line per target from the `## ` docstrings, instead of a
wall of text.
tools/trivy-comment.jq is the 16-line jq program that was pasted into four
places. As a file it is lintable (`jq -n -f`), diffable, and free of Makefile
`$$`/backslash escaping, and one copy means the dependency and image reports
cannot drift apart.
Removed as dead: trivy-release-gate (its only caller was build-and-push.yml,
now deleted) and the `tools` alias (nothing invoked it — every tool is already
built on demand as a prerequisite).
Verified locally: build, test, cover, cover-diff (pass, no-changed-files,
below-threshold and unresolvable-BASE_REF paths), trivy-deps, trivy-image,
trivy-comment, all five trivy-gate states, and help.
run-tests.yml becomes ci.yml, replacing the dead ci.yml that triggered on
`beck-onix-v1.0-develop` (a typo of a branch that does not exist, so it never
fired). Names now read as what they gate: the workflow is `CI` and the jobs
are `Lint` and `Test`, so branch protection matches `CI / Lint` and
`CI / Test` rather than the old `Run Tests / lint`.
The job bodies are four `make` calls plus GitHub plumbing:
make lint advisory, continue-on-error. There is no .golangci.yml in
this repo yet, so `run ./...` uses tool defaults against a
codebase that has never been linted — enough pre-existing
findings to block every PR on day one. This surfaces them.
(The comment this replaced cited a CLAUDE.md and a
pre-commit hook, neither of which exists here.)
make build
make test-ci
make cover-diff
push: added `development`, the branch PRs target and merge into, so a merge
landing there runs the same gates the PR did. It previously only ran on main
and PRs, leaving merges into development ungated.
BASE_REF goes through `env:`, not string-interpolated into the run script —
`?=` in the Makefile means the environment wins, so nothing untrusted ends
up inside a shell command.
permissions are least-privilege per job: the workflow grants contents: read
and only the Test job adds pull-requests: write for the coverage comment.
the coverage comment steps are continue-on-error — a fork PR's default
GITHUB_TOKEN is read-only, so create-or-update-comment 403s there. Coverage
is still gated by the last step; a comment that cannot post must not fail
the job over it.
find-comment v3 -> v4 and create-or-update-comment v4 -> v5.
The final gate step is `exit 1`, nothing more: cover-diff already emitted the
::error:: annotation and the per-file breakdown.
…y-gate [#5] The single `security` job scanned dependencies, then built the image, then scanned that, then gated — serially, in one job, so a dependency finding waited on an image build that has nothing to do with it, and the PR could not say which scan found what without opening a log. Three jobs now, named for what they do: `Dependency scan` and `Image scan` run in parallel (the dependency scan needs nothing but a checkout), and `Security gate` fans in with needs + if: always(). Branch protection matches `Security / Security gate`. Every step in the file is a one-line `make` call or GitHub plumbing — zero lines of inline shell, down from 132 across the three original workflows: make trivy-deps make trivy-comment SARIF=... TITLE=... OUT=... make docker IMAGE=network-adapter:<sha> make trivy-image IMAGE=network-adapter:<sha> make trivy-gate Also fixed: the gate now reads the SARIF the scan jobs produced, handed over as artifacts, rather than scanning a third and fourth time. if-no-files-found is deliberately `ignore`: a scan that produced nothing surfaces as the gate reporting MISSING — which now fails — with both verdicts visible in one place, rather than as a second red check on the scan job. push: added `development`, so a merge landing there runs the same scans the PR did. permissions are least-privilege per job: contents: read at the workflow level, with pull-requests: write and security-events: write only on the two scan jobs. The gate stays read-only. both upload-sarif steps are continue-on-error — a fork PR's GITHUB_TOKEN has no security-events scope, so they 403 there. The gate decides pass/fail; an upload that cannot happen must not fail the scan over it. a `category` per scan, so the image upload no longer replaces the dependency analysis (GitHub keys an analysis on ref + category). find-comment v3 -> v4, create-or-update-comment v4 -> v5. .github/actions/trivy-cache is the `Trivy cache date` + actions/cache pair, extracted now that two jobs need it — both jobs stay a one-line `uses:`. It keeps the trivy binary (~30MB) and the vulnerability DB (~100MB+) out of every run, which the trivy-action it replaced did for us.
] Both workflows already grouped on github.ref with cancel-in-progress, so a new commit on a PR did supersede the previous one's runs — but the intent was undocumented and the flag was a bare `true`, which also cancelled runs on main and development. cancel-in-progress is now scoped to pull_request. On a PR, pushing a new commit cancels the CI and Security runs still in flight for the commit before it: github.ref is refs/pull/<n>/merge for the whole life of the PR, so every push lands in the same group. On main and development nothing is cancelled — each commit there keeps its own recorded verdict, instead of being left with a grey check and no way to tell afterwards whether it was ever green. The two workflows keep separate groups, so cancelling a superseded CI run does not take that commit's Security run down with it.
] Nothing in this repo validated the workflow files. A bad ${{ }} expression, a mistyped `uses:` ref or a `needs:` pointing at a job that does not exist was only discoverable by running the workflow — or by running actionlint by hand. Deliberately not a CI step. The gate is a pre-commit hook, so the feedback lands before the commit exists rather than after a push: lint-actions actionlint (pinned v1.7.12, go-installable like the other tools) over the whole .github tree. Whole-repo rather than per-file because actionlint resolves `needs:` across a workflow's jobs and checks `uses: ./.github/actions/...` against the action on disk. lint-staged what the hook runs. Fires actionlint only when a workflow or composite action is staged, and gofmt only over the staged .go files. hooks sets core.hooksPath to .githooks. Run once per clone. .githooks/pre-commit is a one-line `exec make lint-staged`: the logic stays in the Makefile, so `make lint-staged` reproduces exactly what blocked a commit, and there is a single place to change what runs. core.hooksPath rather than copying into .git/hooks means the hook is versioned and reviewable, and a change to it reaches everyone on their next pull. Scope is what can pass today, so the hook never becomes something people reflexively --no-verify past: - actionlint reports nothing on the current tree, so it blocks from day one. - 22 files in the repo are not gofmt-clean. Staged-only means that history stays someone else's problem until you touch one of those files. - `golangci-lint run` is excluded: with no .golangci.yml it uses tool defaults against a codebase that has never been linted, so it would reject every commit. - the test suite is excluded: a pre-commit hook has to stay in seconds, and `make test-ci` in CI is where that belongs. Verified all six paths: nothing staged, staged Go/workflow with a clean tree, actionlint catching a bad steps.<id> reference, gofmt rejecting and then accepting a staged file, a real `git commit` blocked by the installed hook, and --no-verify still getting through.
manjudr
changed the title
Two checks on a PR, two on a tag, one workflow.
CI / End to End verification build, test, changed-line coverage, result
posted to the PR and the job summary
CI / Security Scan trivy on the dependency graph and on the
shipped image, one comment, one gate
CI / Build Artifact (amd64) push the image to ghcr.io by digest,
CI / Build Artifact (arm64) natively on a runner of that arch
CI / Publish Artifact bind the version tag (and :latest for a
plain vX.Y.Z) to both digests
Dropped the Lint check. It was continue-on-error, so it gated nothing: there
is no .golangci.yml yet and `golangci-lint run` against a never-linted
codebase reports enough to block every PR, which is why it was advisory in
the first place. `make lint` still exists for local use, and the .githooks
pre-commit hook is what actually enforces gofmt on every commit.
Folded security.yml into ci.yml. Dependency scan, image scan and the gate
were three jobs and three checks to answer one question; they are one job
now, which also removes the SARIF artifact handoff between runners and turns
the two per-scan PR comments into one. trivy-comment's SARIF/TITLE/OUT
parameters existed only to serve two jobs and are gone with them —
trivy-report renders both reports from the SARIF_REPORTS list that
trivy-gate reads, so the comment and the gate cannot disagree.
Tag triggers accept 1.0.0 and 1.0.0-RC1 as well as the v-prefixed spelling
every existing tag in this repo uses. Each arch builds on a runner of that
arch rather than under QEMU, because the image compiles every plugin with
`go build -buildmode=plugin` and a plugin .so has to match the adapter
binary's GOARCH. Build Artifact pushes by digest with no tag, so only
Publish Artifact creates something visible, and a tag can never resolve to
one architecture.
Job conditions test startsWith(github.ref, 'refs/tags/') rather than
github.ref_type: on a pull_request the ref is refs/pull/<n>/merge and
ref_type is not documented to be 'branch' there, so a ref_type test risked
skipping both PR checks and reporting green for a run that did nothing.
] Gating Build Artifact and Publish Artifact with `if: startsWith(github.ref, 'refs/tags/')` inside ci.yml put two permanently skipped checks on every PR: CI / Build Artifact (${{ matrix.arch }}) (pull_request) Skipped CI / Publish Artifact (pull_request) Skipped A job whose `if:` is false is still reported as a check, and its `name:` comes through with `${{ matrix.arch }}` unexpanded because the matrix leg never materialised — so the skipped-check noise and the raw expression in the name were the same bug. A workflow whose trigger does not match the event does not exist on that event at all, which is the only way to get zero checks. So: ci.yml is PR/trunk only (push to main and development, plus pull_request) and carries no `if:` on either job, and ci-release.yml is tag-only. Both are `name: CI`, so the check names still read "CI / Build Artifact (amd64)" and "CI / Publish Artifact". The one cost is two entries called "CI" in the Actions sidebar. A PR now shows exactly two checks, and a tag exactly three.
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependenciesNo findings at CRITICAL,HIGH,MEDIUM,LOW. Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
Note on the inline replies above:
|
| Reply says | Now |
|---|---|
security.yml, job Dependency scan |
ci.yml, job Security Scan — make trivy-deps |
security.yml, job Image scan |
ci.yml, job Security Scan — make docker && make trivy-image |
security.yml, job Security gate |
ci.yml, job Security Scan — make trivy-gate, last step |
run-tests.yml |
ci.yml, job End to End verification |
ci.yml, job Lint |
removed — see below |
build-and-push.yml |
ci-release.yml, jobs Build Artifact / Publish Artifact |
The technical dispositions in those replies still hold; only the file and job
names moved. Two substantive changes on top of them:
- The
Lintcheck is gone. It wascontinue-on-error, so it gated
nothing — there is no.golangci.ymlyet andgolangci-lint runagainst a
never-linted codebase blocks every PR, which is why it was advisory. A check
that cannot fail is noise.make lintstill exists locally, and the
.githookspre-commit hook enforces gofmt on every commit. trivy-commentis nowtrivy-report. ItsSARIF/TITLE/OUT
parameters existed only to serve two separate jobs; with one job there is one
comment covering both scans, rendered from the sameSARIF_REPORTSlist
trivy-gatereads, so the comment and the gate cannot disagree.
A PR now reports two workflow checks, CI / End to End verification and
CI / Security Scan, and a version tag reports three,
CI / Build Artifact (amd64), CI / Build Artifact (arm64) and
CI / Publish Artifact. See the PR description for the full shape.
… the gate [#5] A PR reported three checks, not two: CI / End to End verification success CI / Security Scan failure Code scanning results / Trivy success — "No new alerts in code change" The third is not a job. github/codeql-action/upload-sarif registers its own check named after the SARIF tool driver, so it cannot be renamed, given an `if:`, or suppressed while the upload happens. And on that run it was green while `make trivy-gate` failed on the same SARIF: 20 HIGH stdlib CVEs, none of them *new to the diff*, which is the only thing the code-scanning check measures. Two checks disagreeing about one scan is worse than one check, and the gate is the one that is right. Every finding is already in the PR comment with its severity, installed version, fixed version and advisory link, and `make trivy-gate` decides pass/fail. `security-events: write` goes with the upload. The cost, stated: no Security-tab history or diff annotations for Trivy findings. The comment and the gate are the surface now.
The image scan the Security Scan job runs found 20 HIGH findings, all of them the same package: `stdlib` at v1.26.1. Zero came from application dependencies — `trivy-deps` is clean. Trivy reads the Go build info embedded in the binary, which is why these show up in the image scan and nowhere the dependency scan can see. The advisories' fixed versions top out at 1.26.6; 1.26.8 is the current 1.26 patch, so it clears all 20 with room before the next one lands. Bumped in three places that have to agree: go.mod's `go` directive (which is also what setup-go resolves through `go-version-file`, so the test toolchain follows) and both Dockerfiles' builder base. Leaving Dockerfile.adapter at 1.26.1 would have kept the two images on different toolchains for no reason. This re-touches the Dockerfiles, which this PR had otherwise reverted to byte-identical with development as out of scope. Different case: a one-token patch bump to the base image tag, made because the Security gate this PR introduces cannot go green without it — not the base-image migration that was reverted. Verified: `make build` and the full `make test` (every package, including pkg/plugin and benchmarks/e2e, which build and plugin.Open a real .so) pass on 1.26.8 with zero failures.
The gate scanned at HIGH,CRITICAL only, so "0 findings" in the check meant "0 in that band" rather than "clean". Unfiltered, the tree has three findings in golang.org/x/crypto v0.55.0 that the gate never saw. UNKNOWN stays out of the band deliberately. All three of those findings carry no vendor severity, and one (GO-2026-5932, x/crypto/openpgp is unmaintained) has no fix at all, so including UNKNOWN would pin the gate red with no action available. None is reachable here: the only x/crypto import in the tree is blake2b, and the build closure of the adapter plus every plugin contains no x/crypto/ssh or x/crypto/openpgp. SEVERITY stays the single source of truth. The gate already counted rows in the SARIF rather than re-filtering, so widening the scan band was enough; the three places that restated "HIGH or CRITICAL" in prose now derive it, including the jq renderer, which takes the band via --arg so the "nothing found" line cannot drift from the scan that produced it. Verified with trivy v0.74.0, the version CI pins: deps 0 at the new band (the 3 UNKNOWN correctly excluded), wolfi-base runtime layer 0, shipped binary 0. The band change has teeth — the pre-bump go1.26.1 binary goes from 20 to 28 findings, the 8 extra all MEDIUM. Gate exit codes: 2 on findings, 2 on a missing report, 0 only when clean.
Pre-merge review statusReviewed the full diff vs Verified working
The throwaway git tag is deleted. The 1.
|
| Was | Now |
|---|---|
Run Tests / lint |
(drop — Lint is no longer a check) |
Run Tests / test |
CI / End to End verification |
security / security |
CI / Security Scan |
Do not mark CI / Build Artifact (*) or CI / Publish Artifact required — they never run on a PR, and a required-but-absent context blocks every merge.
…er [#5] image-publish joined its steps with `;`, so the recipe exit status came from `imagetools inspect` alone. A `create` that failed against an already-existing tag left inspect reporting the previous index and the job green, publishing a tag bound to the wrong digests with nothing red to say so. Every step is now `&&`-chained. The trivy installer was fetched from the repo default branch while only the version it installs was pinned, so a job holding the runner GITHUB_TOKEN piped an unreviewed remote script into sh on every cache miss. It now comes from the $(TRIVY_VERSION) tag. .gitignore also drifted from the target set: trivy-report.md, image-metadata-*.json and digest-*.txt are produced but were not ignored, while trivy-deps-comment.md and trivy-image-comment.md outlived the trivy-comment target that wrote them.
Post-fix review — all three findings closed
1.
|
| Path | Before | After |
|---|---|---|
create fails, inspect succeeds |
exit 0 ← the bug | exit 2 |
version-vars.sh fails |
reached create anyway |
exit 2 |
plain 1.2.3 |
-t repo:1.2.3 -t repo:latest |
unchanged |
pre-release 2.0.1-rc1 |
not moving :latest |
unchanged |
The && after esac was the part worth proving for real — a mis-parse there would have broken the happy path, and a stub can't rule that out. So I re-tagged (v0.0.0-citest2, run 34152324866, all three jobs green) and the live publish still produces a correct two-platform index:
Name: ghcr.io/openagrinet/network-adapter:v0.0.0-citest2
MediaType: application/vnd.oci.image.index.v1+json
…@sha256:dc6f7b83… Platform: linux/amd64
…@sha256:52b84415… Platform: linux/arm64
v0.0.0-citest2 is not a plain release — not moving :latest
2. Trivy installer pinned to $(TRIVY_VERSION) ✅
Exercised by accident-proof timing: editing the Makefile changed hashFiles('Makefile'), so the cache missed and the installer genuinely ran this time —
Cache not found for input keys: trivy-Linux-X64-34ad2a92…-2026-09-07
curl -sfL https://raw.githubusercontent.com/aquasecurity/trivy/v0.74.0/contrib/install.sh | \
…and Security Scan went on to pass, so the pinned script installs a working trivy 0.74.0 on a runner, not just on my machine (checked there too: Version: 0.74.0).
3. .gitignore back in sync ✅
git check-ignore on every artifact the Makefile produces — trivy-report.md, image-metadata-*.json, digest-*.txt, both SARIFs, all three coverage files, bin/ — now returns ignored for all nine. The two *-comment.md entries left over from the removed trivy-comment target are gone, and nothing in the tree references them.
Regression check
Re-ran everything the fixes could plausibly have broken: lint-actions clean, jq -n -f tools/trivy-comment.jq clean, make -n image-publish resolves correctly, the no-digest guard still exits 1, bin/trivy still short-circuits via its file rule on a cache hit, and trivy-gate still returns 0 / 2 / 2 for clean / findings / missing-report.
Both PR checks green on 138ffbe (run 34152280122): End to End verification and Security Scan, the latter still 0 CRITICAL,HIGH,MEDIUM,LOW on both surfaces.
Both throwaway git tags are deleted. Their two GHCR package versions are untagged and can be removed from the package's Versions tab.
Deliberately not changed
The four nits from the previous comment stand as-is — each is either covered elsewhere or not worth the churn:
image-publish's one-arch guard is existence-only. CI covers it three ways (needs:+fail-fast: true+if-no-files-found: error); only a hand-runmake image-publishcould publish a single-arch tag.- Trivy cache keyed on the whole Makefile — re-downloads ~130MB on any Makefile edit. That's exactly what happened here, and it's what made fix feat: add the OAN registry, JSONata mapper and weather provider plugins #2 testable; narrowing the key would trade that safety for bandwidth.
lint-stagedgofmt passes a file with a syntax error (gofmt exits non-zero, prints nothing).make buildin CI catches it.- PR body:
build-and-push.ymlis still listed among workflows "that could not run", though it was added and removed inside this PR. Prose only.
Unrelated housekeeping: .claude-flow/, .github/workflows/.claude-flow/, tools/area-lookups/ and tree.txt are untracked local tooling output — not part of this PR, but worth not committing by accident. GitHub only reads .github/workflows/*.yml at the top level, so the nested one is inert.
Ready to merge once branch protection is re-pointed to CI / End to End verification and CI / Security Scan (drop Run Tests / lint; don't mark the two tag checks required).
2 participants
What this does
Replaces
.github/workflows/with four checks and nothing else, and makesthe Makefile the implementation behind all of them.
main/developmentCI / End to End verificationmain/developmentCI / Security ScanCI / Build Artifact (amd64)CI / Build Artifact (arm64)CI / Publish Artifact:latest) to both digestsTwo files:
ci.ymlholds all four jobs, and the pre-existingsign-beckn-constants.ymlis untouched. Nothing runs on both event kinds — aPR never shows a publish check, and a tag never re-runs the tests its PR
already gated on.
The workflow is thin; the Makefile is the implementation
Review feedback was that the workflows held the logic and the Makefile held
aliases — 132 lines of inline shell across three workflows, none of it runnable
locally. That's inverted. Every CI step is a one-line
makecall, so a redcheck reproduces locally by running the command in the log:
tools/trivy-comment.jqis the 16-line jq program that was pasted into fourplaces. As a file it's lintable (
jq -n -f), diffable, and free of Makefile$$/backslash escaping.Publishing to GitHub Packages on a tag
ghcr.io/openagrinet/network-adapterexists but no workflow in this repoever pushed to it —
git log --all -S 'ghcr.io' -- .github/matches onlythis PR's own commits, and the deleted deploy workflows all targeted
gcr.io/Artifact Registry. That package came from a manual push or anotherrepo. This closes the gap.
Tag formats accepted. Both spellings, because every existing tag here is
v-prefixed (
v1.8.2,v2.0.1-rc1) but a release cut as1.0.0or1.0.0-RC1should publish rather than silently do nothing:
These are GitHub filter patterns, not regexes —
+and[]work, so thedigits are real digits and a tag like
release/foocan't match.Native per-arch builds, not QEMU.
Dockerfile.adapter-with-pluginscompiles every plugin with
go build -buildmode=plugin, and a plugin.somust match the adapter binary's
GOARCHexactly. Emulating an arm64 Gotoolchain build turns a few minutes into tens of them, so each arch builds on a
runner of that arch (
ubuntu-latestandubuntu-24.04-arm— both free forpublic repos). That's why Build Artifact is two checks rather than one: GitHub
surfaces one check per matrix leg, and the alternative was one slow check.
Build pushes by digest with no tag; only Publish creates a tag. So a
release that builds amd64 and then fails on arm64 never leaves behind a tag
that
docker pullresolves on one platform and 404s on the other.fail-fast: truestops the second leg early, andneeds:(notalways())means a failed leg skips Publish entirely.
:latestmoves only for a plainvX.Y.Z.git describerenders apre-release as
v2.0.1-rc1and an untagged commit asv1.8.2-3-gabc1234, andneither is what someone asking for no tag should get.
The image tag comes from
install/scripts/version-vars.sh— the same place thebinary's
-ldflagsversion comes from — so the image tag andadapter --versioncan't disagree.IMAGE_REPOis derived fromGITHUB_REPOSITORY(lowercased; ghcr.io rejects an uppercase path), so a fork publishes to its own
namespace and there's no repo name hardcoded anywhere.
What was dropped, and why
The
Lintcheck. It wascontinue-on-error, so it gated nothing. There'sno
.golangci.ymlyet, andgolangci-lint runagainst a never-linted codebasereports enough to block every PR — which is why it was advisory in the first
place. A check that can't fail is noise in the checks list.
make lintstillexists for local use, and the pre-commit hook is what actually enforces gofmt.
security.yml, folded intoci.yml.Dependency scan,Image scanandSecurity gatewere three jobs and three checks answering one question. Onejob now, which also deletes the SARIF artifact handoff between runners and
turns two per-scan PR comments into one. Both scans are still there and both
still matter: the dependency scan catches a vulnerable module only the test
suite imports (never linked into the binary, never in a layer); the image scan
reads the base layers plus the Go build info embedded in the binary, including
stdlib, so a Go toolchain CVE shows up there and nowhere else.trivy-comment'sSARIF/TITLE/OUTparameters existed only to serve twojobs and went with them.
trivy-reportrenders both reports from the sameSARIF_REPORTSlisttrivy-gatereads, so the comment and the gate can'tdisagree — including on a missing report, which the comment shows as missing
rather than skipping.
Seven workflows, none of which could run or needed to:
beckn_ci.yml,beckn_ci_test.yml— trigger on PRs tobeckn-onix-v1.0-develop, a branch that doesn't exist here. Never fired.ci.yml— triggered onbeck-onix-v1.0-develop, a typo. Never fired.deploy-to-gke.yml,deploy-to-gke-BS.yml,build-and-deploy-plugins.yml,onix-gcp-terraform-deploy.yml— deployment, out of scope. Keeping them meanskeeping the GCP/registry secret surface for workflows nobody runs.
build-and-push.yml— replaced byBuild Artifact/Publish Artifact. Thisretires the review findings that only existed inside it (
id-token: writewith no consumer,
:lateston a-rctag, the duplicated registry-loginblock).
.github/actions/dhi-loginis deleted with it.Bugs fixed along the way
Three things were broken on
development:make testandmake coverboth failed.pkg/pluginandbenchmarks/e2ebuild a real.soandplugin.Openit in the same run;both
-raceand whole-module coverage instrumentation make that.sounloadable. They now run as their own invocation. The carve-out is named
once (
PLUGIN_PKGS/MAIN_PKGS) and shared bytest,coverandtest-ci.trivy-gatepassed vacuously when a scan wrote no report. A missing orunparsable SARIF is now a failure — the one case a security gate must not be
green for.
golang.org/x/cryptov0.54.0→v0.55.0,google.golang.org/grpcv1.82.1→v1.83.1,
github.com/rabbitmq/amqp091-gov1.11.0→v1.13.0. No sourcechange needed.
Also:
cover-diffno longer swallows a broken base (an unresolvableBASE_REFor a failed
git diffare hard errors — an unknown changed-file set is not anempty one);
merge-coveragealways removes its intermediate;dockerbuildsDockerfile.adapter-with-plugins, the image that ships and that CI scans, notDockerfile.adapter;trivy fsno longer reports on its own binary.Reverted as out of scope
The first revision migrated the Dockerfiles to Docker Hardened Images in a PR
whose stated scope is CI workflows.
dhi.iois subscription-gated, so the baseimage isn't pullable without the entitlement — which would break fork PRs.
All three files are now byte-identical to
development:Dockerfile.adapterDockerfile.adapter-with-plugins— back togolang:1.26.1-bookwormbuildingand
cgr.dev/chainguard/wolfi-baseas the runtime, which has a shell andglibc (so
plugin.Openworks) and is publicly pullableDeployment/deployment.yaml— the addedargs: ["--config=$(CONFIG_FILE)"]doesn't match the adapter's flags and would CrashLoopBackOff on rollout
Hardening
contents: readat the workflowlevel; both PR jobs add
pull-requests: writefor their comment, and onlythe two tag jobs get
packages: write. Nothing asks forsecurity-events: write— see the SARIF note below.if:in this one. A jobwhose
if:is false still posts a skipped check, and itsname:isreported with
${{ matrix.* }}unexpanded because the leg nevermaterialised — so gating them inline put two permanently skipped checks
reading
Build Artifact (${{ matrix.arch }})on every PR. A workflow with atag-only trigger simply doesn't exist on a PR. It's also named
CI, so thecheck names still read
CI / Build Artifact (amd64).pushincludesdevelopment. Previously onlymainand PRs, so mergesinto the branch PRs actually target were ungated.
GITHUB_TOKENisread-only, so
create-or-update-comment403s there even withpull-requests: writedeclared. Those steps arecontinue-on-error;make trivy-gateand the coverage gate still decide pass/fail.BASE_REFgoes throughenv:, not string-interpolated into a shellscript.
?=in the Makefile means the environment wins.previous commit's runs. Not on
main/development/tags — each trunk commitkeeps its own verdict, and a publish is never aborted half-done.
CRITICAL,HIGH,MEDIUM,LOW, set once asSEVERITYinthe Makefile and threaded into the scan, the gate's error text and the PR
comment, so no copy can drift from the band actually scanned.
UNKNOWNisdeliberately out: it means no vendor has assigned a CVSS score, and the three
UNKNOWNrows here aregolang.org/x/cryptoadvisories that are unreachablefrom this binary (only
blake2bis imported), one of which(
GO-2026-5932, openpgp unmaintained) has no fix at all and would pin thegate red forever.
upload-sarif.github/codeql-action/upload-sarifposts its owncheck,
Code scanning results / Trivy, which isn't a job and can't berenamed or suppressed — a third check on every PR. Worse, it reported the
opposite of the truth: on the run that found 20 HIGH
stdlibCVEs it wentgreen with "No new alerts in code change", because none were new to the diff,
while
make trivy-gatefailed on the same SARIF. The cost is no Security-tabhistory for Trivy; the PR comment and the gate are the surface instead.
--provenance=falseon the arch builds, so each push is a plain manifestrather than an OCI index wrapping an attestation — otherwise
imagetools createcomposes indexes of indexes.find-commentv3→v4,create-or-update-commentv4→v5.Pre-commit hook, not a CI step
Nothing validated the workflow files — a bad
${{ }}expression or aneeds:pointing at a nonexistent job was only discoverable at runtime. The gate is a
pre-commit hook, so feedback lands before the commit exists:
make hooks # once per clone: core.hooksPath -> .githooks.githooks/pre-commitis one line —exec make lint-staged— so the logicstays in the Makefile, the hook is versioned and reviewable, and a change to it
reaches everyone on their next pull.
Scope is what can pass today, so it doesn't become something people
reflexively
--no-verifypast: actionlint when a workflow or compositeaction is staged (clean on the current tree, blocks from day one) and gofmt
over staged
.gofiles only (22 files repo-wide aren't gofmt-clean;staged-only keeps that history out of your way until you touch one).
Deliberately excluded:
golangci-lint run(no config yet, would reject everycommit) and the test suite (a hook has to stay in seconds).
The check names changed. Required checks must be re-pointed in one pass, or the
PR blocks on contexts that no longer report:
Run Tests / lintRun Tests / testCI / End to End verificationsecurity / securityCI / Security ScanCI / Build Artifact (*)andCI / Publish Artifactnever run on a PR, sothey must not be marked required.
Verification
Run locally on this branch:
build,test,cover(merged profile, pluginpackages included, intermediate removed),
cover-diffacross all four paths(pass, no-changed-files, below-threshold with the per-file table, unresolvable
BASE_REF),lint, a realtrivy-deps(clean at the full band),trivy-reportwith one report present and one missing,trivy-gatein allfive states,
lint-actions,lint-stagedacross all six paths including areal
git commitgetting blocked, andhelp.actionlint v1.7.12is clean and every workflow file parses. No danglingreferences to any deleted file or removed target.
image-buildandimage-publishwere checked withmake -nplus their guard paths(
IMAGE_REPOempty, nodigest-*.txt) and the:latestdecision was verifiedagainst
v1.0.0,1.0.0,v2.0.1-rc1,1.0.0-RC1andv1.8.2-3-gabc1234.Two caveats, stated plainly:
make dockeris verified in CI, not locally. Locally it got through thefull builder stage and built every plugin, then Docker Desktop died with an
input/output errorwriting its buildkit metadata DB — a local daemon fault,not the Dockerfile or the target.
CI / Security Scanbuilds and scans thereal image on every run and is green: 15
wolfipackages and the Go binarydetected, 0 findings on both surfaces at
CRITICAL,HIGH,MEDIUM,LOW.first tag on this branch's merge is what proves
Build ArtifactandPublish Artifactfor real; everything up to thedocker buildxinvocationis verified.