[nisargabd]

Collapses CI to four checks, and moves every line of build/test/scan logic
out of YAML and into the Makefile. Follows OpenAgriNet/network-adapter#14 so
the two repos converge rather than drift.

The four checks

When	Check	Runs
every PR	CI / End to End verification	make build, make lint, make test-ci, make cover-diff
every PR	CI / Security Scan	make trivy-deps, make trivy-image, make trivy-gate
version tag	CI / Build Artifact (amd64), CI / Build Artifact (arm64)	make image-build, make image-push
version tag	CI / Publish Artifact	make image-publish

Nothing else posts a check. A PR shows exactly two; a tag shows exactly three.
ci.yml triggers on pull_request only — main moves solely by merging a PR,
and that PR was already verified at its merge commit, so re-running the same
checks on the push that lands it is a fifth check answering a question asked
minutes earlier.

Files

A  .github/workflows/ci.yml            two PR checks
A  .github/workflows/ci-release.yml    two tag checks
D  .github/workflows/run-tests.yml
D  .github/workflows/security.yml
D  .github/workflows/build-and-push.yml
A  .github/actions/trivy-cache/        the binary+DB cache, was pasted 3x
A  tools/trivy-comment.jq              the SARIF->markdown program, was pasted 2x
A  .githooks/pre-commit                `make lint-staged`, installed by `make hooks`
M  Makefile                            every step above is a target here
M  .gitignore                          the artefacts those targets now write locally

The inversion

Every step in both workflows is either a one-line make call or GitHub
plumbing — a ${{ }} expression, a $GITHUB_STEP_SUMMARY write, a uses:.
There is no shell in the YAML that does work.

The point is not brevity. It is that a red check reproduces locally by running
the command the log shows. Previously ~100 lines of scan and coverage shell
existed only inside a runner, so the only way to iterate on it was to push.

Why two files and not one

ci-release.yml is a separate file, deliberately named CI as well.

A job whose if: evaluates false still posts a skipped check, and its name:
is reported with ${{ matrix.* }} unexpanded, because the matrix leg never
materialised. Gating the release jobs inline would therefore put two permanent
skipped checks reading Build Artifact (${{ matrix.arch }}) on every PR. A
workflow whose trigger does not match simply does not exist on that event.

Sharing the workflow name: keeps the checks reading CI / Build Artifact (amd64). The cost is that the Actions sidebar lists two entries called CI.

Notable decisions

One security job, not three. Dependency scan and image scan were two jobs
plus a gate job — three red-or-green checks answering one question. Now one
check. The dependency scan needs only a checkout, so it runs first and its
findings are in the log before the image finishes building. Both scans are
kept: a test-only module never linked into the binary appears in trivy fs and
in no layer, and a Go toolchain CVE appears as stdlib in the image scan and
nowhere trivy fs can see it.

upload-sarif removed. It posts its own check, Code scanning results / Trivy, which is not a job and so cannot be renamed or suppressed — and it can
report the opposite of the gate, going green with "no new alerts" on findings
that are not new to the diff while make trivy-gate fails on the same SARIF.
Two checks disagreeing about one scan is worse than one check. The cost is no
Security-tab history; the PR comment and the gate are the surface instead.

Lint is now a real gate. It was continue-on-error. The tree is at 0
issues, so a check that cannot fail was only noise.

Scan before push, still. make image-build builds into the runner's local
daemon and gates on Trivy before anything is tagged for a registry, so the exact
bytes that ship are what passed. image-push re-tags that same image — no
rebuild, no re-scan.

Native arm64, not QEMU. One leg per arch on a runner of that arch. Both
labels are free for public repos. fail-fast: false, so a single-arch break
does not hide the other arch's verdict.

:latest moves only for a plain release. Any tag containing a hyphen
(v1.2.3-rc1) publishes its own tag and leaves :latest alone.

Fork PRs degrade, they do not fail opaquely. A fork gets an empty string for
every secrets.*, and the Dockerfile's bases are dhi.io/*, which 401 on an
anonymous pull. One SCAN_IMAGE flag is exported to the environment and does
double duty: it gates the image steps and the Makefile reads it to narrow the
gate's expected report list. One name, one meaning — the comment and the gate
cannot disagree about whether an image report was ever expected. Comment steps
are continue-on-error because a fork's read-only token 403s on them.

GHCR only. An earlier revision of this branch carried *_ENABLED flags for
Azure, Google Artifact Registry and Docker Hub. The repo has no variables set,
so all three were permanently false and every secret behind them an empty
string — and an unconfigured registry is not flexibility, it is a fourth way
for a release to look green while publishing nothing. Removed. ci-release.yml
went from 214 lines to 133. Adding one back is one entry in IMAGE_REPOS and
one login step.

The dhi.io login stays and is a different thing: DOCKERHUB_USERNAME /
DOCKERHUB_TOKEN are the Docker Hardened Images subscription the Dockerfile's
base images need in order to pull at all, not a Docker Hub publish target.

Verified

Local: make build, make lint, make lint-actions and make test-ci (759
tests) all exit 0.

PR checks, run 34152732151:

• Exactly two checks on the PR. No release checks present.
• End to End verification — pass, 3m58s.
• Security Scan — every step green including the dhi.io login, image build and
  image scan; fails only at the gate, on one pre-existing finding.
  trivy-deps.sarif: 1 HIGH,CRITICAL, trivy-image.sarif: 0.
• Both PR comments post and update in place via their markers.

Release path, exercised for real against tag v0.0.1-rc2,
run 34153252718
— all three checks green:

• The tag trigger fired, and ci.yml correctly did not — a tag push produces
  one run, not two.
• Build Artifact (amd64) and Build Artifact (arm64) both built natively,
  passed the pre-push Trivy gate, and pushed their arch tags.
• Publish Artifact stitched them into a manifest list and inspected it:

  Name:      ghcr.io/openagrinet/discovery-service:v0.0.1-rc2
  MediaType: application/vnd.docker.distribution.manifest.list.v2+json
  Manifests:  linux/amd64, linux/arm64
  

• :latest correctly not moved — the log says
  v0.0.1-rc2 is not a plain release — not moving :latest.

Re-run after the GHCR-only cleanup against tag v0.0.1-rc3,
run 34153701354
— all three green again, and the tag push produced one run, not two.

v0.0.1-rc2 and v0.0.1-rc3 are throwaway test tags. They and their GHCR
packages can be deleted; :latest was never moved by either.

Known issues, both pre-existing and out of scope for this PR

1. Security Scan is red on CVE-2026-17106 — github.com/moby/go-archive
   v0.2.0, fixed in 0.3.0, reached via testcontainers. Test-only; the shipped
   image scans clean. Needs a dependency bump, not a workflow change.

2. The coverage percentage is understated, badly. -coverpkg writes each
   block once per test binary — up to 22 copies here — and the report sums the
   denominator across all copies while crediting the numerator only where
   count>0. Measured on src/discover/intent_mapper.go: 8% reported vs
   93% true. Predates this PR (reproduced against the previous Makefile on
   the same profile). The fix is to key blocks and take the max rather than
   summing duplicates.

Neither is introduced here and neither is fixed here.

For the reviewer

main currently has no branch protection. When it is added, require
CI / End to End verification and CI / Security Scan. Do not require the
two release checks — they never run on a PR, so requiring them blocks every
merge.

[manjudr]

Thanks for putting together this comprehensive CI/CD update! Splitting the workflows into distinct, modular files (run-tests.yml, security.yml, build-and-push.yml), implementing native multi-arch builds (amd64 + arm64), adding pre-push Trivy gating, and bumping golang.org/x/crypto to v0.55.0 are all fantastic improvements.

Before merging, there are a few critical fixes and improvements needed:

---

🔴 Required Changes

1. Fix Tag Filter Glob Pattern in build-and-push.yml

   • File: .github/workflows/build-and-push.yml
   • Issue: GitHub Actions on.push.tags uses glob patterns (fnmatch), not regular expressions. In glob syntax, + is treated as a literal + character rather than a quantifier. Consequently, standard release tags (e.g., v0.0.1, v1.2.0) will never match or trigger this workflow.
   • Action: Update the tag filter to use valid glob matching:

     on:
       push:
         tags:
           - "v*.*.*"
           # or: ["v[0-9]*.[0-9]*.[0-9]*"]

2. Add continue-on-error: true to PR Comment Steps for Fork PRs

   • Files: .github/workflows/run-tests.yml and .github/workflows/security.yml
   • Issue: Pull requests opened from forks receive a read-only GITHUB_TOKEN by default. Even with pull-requests: write declared, peter-evans/create-or-update-comment will fail with HTTP 403 (Resource not accessible by integration), causing the entire CI check to fail for external contributors.
   • Action: Add continue-on-error: true to the PR comment steps in run-tests.yml (coverage) and security.yml (govulncheck & Trivy):

     - name: Comment coverage on PR
       if: github.event_name == 'pull_request'
       continue-on-error: true
       uses: peter-evans/create-or-update-comment@v4

3. Provide Fallbacks or Documentation for dhi.io in Dockerfile

   • File: Dockerfile
   • Issue: Switching base images to dhi.io requires authentication against a paid Docker subscription. Running make docker, make run, or docker compose up --build locally without a configured DHI_AUTH credential will fail with 401 Unauthorized.
   • Action: Parameterize base images using Docker ARG defaults to public images (e.g., golang:1.25-alpine and gcr.io/distroless/static-debian12:nonroot) with DHI passed via --build-arg in CI, or clearly document how local developers should authenticate.

---

🟡 Recommended Improvements

4. Eliminate Duplicate Test Runs in run-tests.yml

   • In run-tests.yml, the job runs make test followed immediately by make cover. Since make cover already executes go test -race across the entire codebase to compute coverage, running make test first duplicates ~3.5 minutes of CI execution. You can remove - run: make test and rely solely on make cover.

5. Explicit Exit Handling for govulncheck in security.yml

   • make security 2>&1 | tee govulncheck-report.txt swallows non-zero exit codes due to tee. If this step is intended to be purely informative/reporting prior to comment posting, make the tolerance explicit (e.g. make security > govulncheck-report.txt 2>&1 || true) to prevent unexpected failures if runner shell options change in the future.

6. Align Go Versions

   • Align Go version across go.mod (go 1.25), CI setup-go (1.25.0), and the Dockerfile build stage (1.27) to maintain parity between local dev, CI, and production builds.

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[nisargabd]

Went through the earlier feedback:

• The tag glob is actually correct as-is — GitHub's tag filter syntax supports + as a quantifier (it's not shell glob), so v[0-9]+.[0-9]+.[0-9]+ already matches plain SemVer tags. Checked this against GitHub's docs before leaving it alone.
• Added continue-on-error to every PR-comment step. A fork PR gets a read-only token by default, so posting a comment there would 403 and fail the whole job for no good reason.
• Fixed the dhi.io dependency for local development — both Dockerfile stages now take the base image as a build-arg, defaulting to dhi.io (so CI and shipped images don't change), with a public-image fallback documented for anyone without a DHI subscription.
• Tests no longer run twice. One gotestsum pass now produces both the pass/fail result and the coverage profile.
• Dropped govulncheck entirely — replaced with Trivy for both the dependency graph and the built image, so severity filtering actually works and everything gates the same way.
• Looked into aligning the Go version across go.mod, CI, and the Dockerfile, but bumping go.mod's directive forces a golangci-lint upgrade that cascades into a much bigger dependency bump than this PR should carry — confirmed locally that the newer golangci-lint crashes analyzing Go 1.27 syntax. Leaving this one for later; the Dockerfile's pinned version is what actually ships, so it's not a production risk.

Also bumped golang.org/x/crypto to fix a real CVE (SSH auth bypass) that the new Trivy gate caught on a real tag push — tested end to end including a real image landing in GHCR.

[github-actions]

📊 Test Coverage: ➖ Not applicable — no non-test Go files changed vs origin/feat/18-network-setup, so there are no lines to measure and no percentage to report

[manjudr]

CI review — naming consistency, one-liner CI, parallelism

Reviewed side by side with the sibling PR so the two repos converge rather than drift:

• OpenAgriNet/network-adapter#14 (chore/5-ci-cd-workflows)
• OpenAgriNet/discovery-service#15 (chore/9-ci-cd-workflows)

Good news first: structurally these two are already very consistent — same three workflow files, same workflow name: values, same job IDs, same concurrency group pattern (test-/security- + ref), same permissions blocks. diff on the two run-tests.yml files is 6 lines, all comments. That's a deliberate job and it shows.

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 lint / test / security consistent? Partly — they're at mismatched granularity

Here's what the checks are actually called today (no job declares name:, so the UI and branch protection see the raw job ID):

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:

1. Workflow names mix grammatical forms. Run Tests and Build and Push are verb phrases; Security is a bare noun. Pick one form.
2. File names mix forms the same way — run-tests.yml is verb-prefixed, security.yml and build-and-push.yml are not.
3. Run Tests contains a lint job, which is not a test. Either the workflow is misnamed or lint is misplaced.
4. The granularity is uneven, and this is the one that actually costs you something. lint and test are single activities. security is one job doing five: dependency scan → dependency PR comment → image build → image scan → image PR comment → gate. So a red security check 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/gate means 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 to ci.yml is 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-ci

But 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.md

discovery-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 the verify / newman / audit gates that CLAUDE.md documents 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 — no needs: 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-gate

One 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	⚠️ Mixed forms + uneven granularity (§1)
CI as one-liner	⚠️ Test path clean; ~100 lines/repo left in security + build-and-push (§2)
make docker / make security unused	⚠️ Defined but never called
govulncheck asymmetry across repos	⚠️ discovery-service only, and not in CI
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]

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	51
build-and-push.yml	56
total	119

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 in network-adapter's.
• Tag and push (22L), Create multi-arch manifests (20L) and Set registry flags (4L, twice) are byte-identical across the two repos — same md5 after normalising discovery-service/network-adapter.

security.yml L79-85 already records what that costs: capture(...)?.v is valid on jq 1.8 and a syntax error on jq 1.7.1, and it took the whole job down. That fix had to be applied in four places. One make trivy-comment target and it would have been one place — and make trivy-comment would be runnable locally, which an Actions run: block never is.

Moving the six blocks below to make targets takes this repo from 119 → ~15 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
Set registry flags (4L × 2)	a composite action, or make registry-flags
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 can overlap the image build entirely. Split into deps + image in parallel and a gate job with needs: [deps, image] consuming both SARIFs as artifacts, and you save the whole deps-scan duration off the critical path. That is also what the naming table below assumes.

Did I review the Makefile? Now yes — and it has one blocker

make test-ci drops -coverpkg, so the CI coverage gate measures a different and materially lower number than make cover does. Empirically verified, details on Makefile:88. Everything else is inline below.

One Makefile finding I could not anchor inline

help (L45-46) is outside this PR's hunks, so GitHub won't take a comment there — but it is worth fixing while you're in this file, and it is the first thing anyone typing make sees, since .DEFAULT_GOAL := help.

@grep -hE '^## ' $(MAKEFILE_LIST) | sed 's/^## / /' | sort sorts every ## line independently. Most of this file's help text is multi-line, so sort tears the continuations away from their target and alphabetises them 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: 70 lines of output, of which only 32 name a target — 38 are orphaned fragments, and the entire first screen contains no target name at all. (network-adapter has the same bug at a smaller scale: 25 lines, 15 targets.)

Two fixes, either fine — sort only the lines that carry a target, or drop sort and rely on file order:

help:
	@grep -hE '^## [a-z]' $(MAKEFILE_LIST) | sed 's/^## /  /' | sort

That keeps one line per target and drops the continuations from help entirely, leaving them as source comments where they read fine. If you want the continuations shown, they have to be emitted with their parent — which means not sorting, or grouping before sorting.

---

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:

1. No job declares name:. The check string is therefore the raw id, so
   branch protection and the PR checks list read Run Tests / lint,
   Security / security.
2. The workflow names mix styles — Run Tests and Build and Push are
   verb phrases, Security is a bare noun.
3. Run Tests under-describes what it runs. It runs lint too, so
   Run Tests / lint is 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 adding name: to a job change the check
string, which is the branch-protection contract. Run Tests / lint →
CI / Lint will 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.

[manjudr]

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 happens to have written 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.

This is also inconsistent with its three neighbours, which all name a target and therefore rebuild:

cover-total: cover      # L69
cover-report: cover     # L73
cover-html: cover       # L79
cover-diff: coverage.out  # <- this line

Either give coverage.out a rule:

coverage.out:
	$(MAKE) cover

or make it cover-diff: cover like the other three. The first is better — it keeps cover-diff cheap in CI, where the profile already exists, instead of re-running the whole suite.

[manjudr]

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:

1. Locally. Both workflows say the point of this target is that "a local make trivy-gate checks the exact same two files this job just wrote." On a clean checkout, or after make clean, or after running only make trivy-deps, make trivy-gate is green having scanned nothing. Half-green is the common case: run make trivy-deps alone and the deps half reports honestly while the image half passes vacuously.
2. The moment someone adds if: always() to the gate step — which is the natural next change, since a gate you want to see the result of should not be skipped by an earlier failure.

 	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.

[manjudr]

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:

1. No job declares name:. The check string is therefore the raw id, so
   branch protection and the PR checks list read Run Tests / lint,
   Security / security.
2. The workflow names mix styles — Run Tests and Build and Push are
   verb phrases, Security is a bare noun.
3. Run Tests under-describes what it runs. It runs lint too, so
   Run Tests / lint is 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 adding name: to a job change the check
string, which is the branch-protection contract. Run Tests / lint →
CI / Lint will 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.

[manjudr]

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:

ds security.yml block 1  vs  ds block 2   IDENTICAL
ds security.yml block 1  vs  na block 1   IDENTICAL
na security.yml block 1  vs  na block 2   IDENTICAL

One program, copy-pasted four times. The comment at L79-85 above documents what that already cost — the capture(...)?.v jq-version break had to be fixed in all four.

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.md

Putting 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.

[manjudr]

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 a docker 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, docker build,
            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:

1. The deps scan overlaps the image build entirely.
2. Security / Dependency scan and Security / Image scan become separately-red checks, so the PR says which scan found something without opening the log.
3. gate with needs: + 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 on Makefile:179 from latent into live. Fix that one first, or in the same PR.

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.

[manjudr]

Tag and push (22 lines) is byte-identical to network-adapter's. Same for Create multi-arch manifests (20 lines).

Hashed with the app name normalised:

ds tag-and-push  666e75e295cea971aaba2bdfa1afe60f
na tag-and-push  666e75e295cea971aaba2bdfa1afe60f
ds manifests     9d8f277b346056021d000d98b5ede18f
na 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)"; \
	done

and 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.

[manjudr]

Set registry flags is duplicated verbatim inside this file — build here and manifest at L179 — and again in both network-adapter jobs. Four identical copies.

ds reg-flags  43ab4d6afa376ba5ca8455c1fb65fdc2
na reg-flags  43ab4d6afa376ba5ca8455c1fb65fdc2

Steps cannot be shared between jobs in the same workflow, which is what forces the copy. Two ways out, both better than four copies:

1. A composite action — .github/actions/registry-flags/action.yml — uses:'d by both jobs. Fits the pattern already established by .github/actions/dhi-login in this very PR.
2. Compute the flags once in a tiny upstream job and expose them via outputs, with build and manifest both declaring needs:.

I'd take (1): it is local, has no extra job latency, and matches what you already did for dhi-login.

[manjudr]

One more, ported from the review I left on network-adapter#14 — this repo has the identical line, so it belongs here too. Adding it as a separate comment rather than amending the review above.

Also, one correction to that na review while I'm here: I implied this failure mode was readily reachable. It isn't — fetch-depth: 0 is correctly set on the test job in both repos, with a comment explaining exactly why, so the common paths resolve fine. Scoped accurately below.

[manjudr]

Supplementary to #5130272205 — this pass is about parity with network-adapter#14.

Why a second review: my first one was scoped to the three questions I was asked (job naming, CI-as-a-one-liner, and the Makefile). It was not an end-to-end sweep of the diff. network-adapter#14 did get that sweep, and comparing the two revealed that the near-identical workflow YAML in this repo was never checked against the same findings. I've now walked each one against this repo's actual files.

Result: eight findings, all live here. Seven already have an accepted counterpart on na#14; one is live in both repos and had only ever been raised here, on 2026-09-01, and never fixed.

#	anchor	finding	na#14
1	build-and-push.yml:9	Tag filter is a glob, not a regex — release never triggers	⚠️ not raised there either
2	build-and-push.yml:14	workload_identity_provider without id-token: write	✅ build-and-push.yml:12
3	build-and-push.yml:21	No fail-fast: false, no concurrency group	✅ build-and-push.yml:61
4	run-tests.yml:5, security.yml:5	push filter misses this PR's own base branch	✅ run-tests.yml:6, security.yml:6
5	security.yml:138	Unguarded dhi.io login takes the whole check down on fork PRs	✅ security.yml:61/139/156
6	run-tests.yml:75	Unguarded read of a file the previous step may not have written	✅ run-tests.yml:69
7	build-and-push.yml:179	44 lines byte-identical to the build job	✅ build-and-push.yml:205
8	build-and-push.yml:234	:latest gets moved by release candidates	✅ build-and-push.yml:260

#1 and #2 are the ones that make the release path non-functional rather than merely untidy — everything else degrades or duplicates, those two simply don't work.

I've also posted #1 to na#14 separately, since that repo has the byte-identical tag filter and had never been told. After both, the two PRs carry the same finding set.

One piece of housekeeping on the older review here (#5077585002, 2026-09-01): it has gone partly stale and should not be worked through as-is. Item 2 (continue-on-error on comment steps) is now fixed — 6 occurrences in security.yml, 4 in run-tests.yml. Items 4 and 5 describe a make test + make cover sequence and a govulncheck step that no longer exist in this diff (run-tests.yml now runs make build then make test-ci; security.yml has zero govulncheck references). Item 6 is half-live: go.mod and setup-go agree via go-version-file, but the Dockerfile build stage is dhi.io/golang:1.27-alpine-dev against go 1.25.0. And item 1 is #1 below — still unfixed.

[manjudr]

Blocker — permissions: has no id-token: write, so the Google Artifact Registry path cannot mint a token and will fail whenever it is enabled.

google-github-actions/auth@v3 is used twice, at L107-113 (build) and L209-215 (manifest), both with workload_identity_provider. Workload Identity Federation works by exchanging a GitHub OIDC token for Google credentials, and the workflow can only request that OIDC token if id-token: write is granted. Without it the action fails with Unable to get ACTIONS_ID_TOKEN_REQUEST_URL / Failed to get ID token.

contents: read and packages: write cover checkout and GHCR, so GHCR/Docker Hub/ACR are all fine — this fails only on the GAR leg, which is exactly why it can sit here unnoticed until someone sets GOOGLE_ARTIFACT_REGISTRY_ENABLED=true for the first time and the release breaks.

 permissions:
   contents: read
   packages: write
+  # Workload Identity Federation (google-github-actions/auth at L109/L211)
+  # exchanges a GitHub OIDC token for Google credentials; without this the
+  # workflow cannot request that token and the GAR leg fails to authenticate.
+  id-token: write

na#14 carries the same finding at build-and-push.yml:12.

[manjudr]

The release workflow has neither fail-fast: false on the matrix nor a concurrency group — the first costs you diagnosis, the second risks a corrupt manifest.

Two separate problems that both live in this block.

fail-fast: false. strategy.matrix defaults to fail-fast: true, so if the amd64 leg fails, GitHub cancels arm64 mid-flight. On a release you then can't tell whether arm64 was fine — you have to re-run the whole thing to find out. The other two workflows don't matter here, but a release is precisely where you want both legs' verdicts.

   build:
     strategy:
+      # A release wants both legs' verdicts. Default fail-fast cancels arm64
+      # the moment amd64 fails, so a single-arch break hides the other arch's
+      # result and costs a full re-run to recover it.
+      fail-fast: false
       matrix:

concurrency. run-tests.yml:18-20 and security.yml:18-20 both declare a group with cancel-in-progress: true. This workflow declares none. Two tag pushes in quick succession (a v1.2.0-rc1 followed by v1.2.0, or a re-pushed tag) therefore run fully in parallel, and both manifest jobs call docker buildx imagetools create against overlapping tags — including the shared :latest (see L234). Interleaved manifest writes to the same tag are a race with no defined winner.

Note this one wants the opposite setting from the other two workflows:

+# Unlike run-tests/security, cancel-in-progress is false: a half-cancelled
+# release can leave arch tags pushed with no manifest stitching them, which is
+# worse than two releases queueing.
+concurrency:
+  group: release-${{ github.ref }}
+  cancel-in-progress: false

na#14 carries the same finding at build-and-push.yml:61.

[manjudr]

push: branches: [main] — this PR's own base branch is feat/18-network-setup, so the push half of this trigger never fires for the branch this work merges into.

The PR half (pull_request:, unfiltered) works fine, which is why CI looks healthy on this PR. But make cover-diff's push path at L67 (BASE_REF="${{ github.event.before }}") is dead code until something lands on main — and this stack lands on feat/18-network-setup first.

The comment at L55-59 explains the design assuming main is the only landing point: "main only ever moves by PR merge … on a push to main (the merge landing) it's github.event.before." That reasoning is right for the end state and wrong for the current one, where an integration branch sits in between.

Either add the integration branch for as long as it exists:

 on:
   push:
-    branches: [main]
+    branches: [main, feat/18-network-setup]
   pull_request:

or say explicitly in the comment that the push path is dormant until this stack reaches main, so the next person doesn't debug it as broken. I'd take the first — a dormant gate that nobody has ever seen run is a gate nobody knows the state of.

security.yml:5 has the identical filter; same fix. na#14 carries this at run-tests.yml:6 and security.yml:6.

[manjudr]

Same as run-tests.yml:5 — push: branches: [main] doesn't include feat/18-network-setup, this PR's base.

The pull_request: half covers this PR, so the scans do run here. But the push half never fires for the branch this stack actually merges into, which means the post-merge scan of the integration branch never happens — and that is the one that reflects the merged state rather than the PR's simulated merge.

Same fix as run-tests.yml:5; worth doing in one commit across both files so they can't drift apart.

na#14 carries this at security.yml:6.

[manjudr]

Every fork PR's security check fails hard here — before either Trivy gate gets a chance to report.

A pull request from a fork gets an empty string for every secrets.* reference, by design. So DOCKERHUB_USERNAME and DOCKERHUB_TOKEN arrive blank, the dhi.io login fails, and — with no continue-on-error and no if: guard — the job stops here. The image build, the image scan, the SARIF upload and the gate at L217 all never run.

The asymmetry is what makes this stand out: this PR is careful about exactly this failure mode everywhere else in the file. L124-127 puts continue-on-error: true on the comment step specifically because "a fork PR's default GITHUB_TOKEN is read-only, so this 403s there even with pull-requests: write declared." The same reasoning applies with more force to a login step that gates the entire second half of the job — and it is the one step without the guard.

A guard alone isn't enough, though, because the build at L152 pulls dhi.io/* base images and will fail unauthenticated regardless. The honest options are:

1. Skip the image half on forks — guard the login, build, scan, upload and comment with if: github.event.pull_request.head.repo.full_name == github.repository. Fork PRs then get the dependency scan and a green check; the image scan runs on push and on same-repo PRs, which is where releases come from anyway.
2. Parameterise the base images with ARG defaults to public equivalents, so a fork builds an approximately-right image and scans it. This is item 3 of the 2026-09-01 review, still open, and it would fix make docker / make run locally too.

Either is fine; what isn't fine is the current state, where an external contributor sees a red Security check with a Docker login error and no scan results at all.

na#14 carries this at security.yml:61, :139 and :156.

[manjudr]

Unguarded read of a file the previous step may not have written — this turns a coverage-tooling failure into a confusing job failure that hides the real error.

Check coverage above is continue-on-error: true (L62), deliberately, so that a coverage miss still posts its report. But continue-on-error also absorbs a coverage-tooling crash — and in that case coverage-report.md was never created. This step then runs cat coverage-report.md unguarded, fails with No such file or directory, and the job goes red pointing at the summary step rather than at whatever actually broke.

The path is reachable today: cover-diff is unrunnable without coverage.out (see my note on Makefile:97), and its git diff can fail outright (Makefile:98). Both leave no report behind.

 - name: Publish coverage summary
   run: |
+    if [ ! -f coverage-report.md ]; then
+      echo "📊 **Test Coverage: ⚠️ Unavailable** — the coverage step did not produce a report; see its log" > coverage-report.md
+    fi
     printf '<!-- coverage-report -->\n%s\n' "$(cat coverage-report.md)" > coverage-report.md
     cat coverage-report.md >> "$GITHUB_STEP_SUMMARY"

That keeps the marker contract intact for find-comment below, and says the true thing — the number is unknown — instead of failing on a missing file or, worse, silently reporting a pass.

na#14 carries this at run-tests.yml:69.

[manjudr]

44 lines here are byte-identical to the build job above — the whole registry-auth preamble, copy-pasted.

I diffed build L64-124 against manifest L179-222:

build L64-124   = 61 lines
manifest L179-222 = 44 lines
differing       = 17 lines, all present only in `build`
=> all 44 of manifest's lines are byte-identical to build's

The 17 build-only lines are the dhi.io login (which manifest genuinely doesn't need, since it never builds) and two comment blocks. Everything else — Set registry flags, the GHCR / Docker Hub / ACR logins, the Google auth, gcloud auth configure-docker, Set image owner — is duplicated verbatim.

That's four registries × two jobs = every future registry change made twice, and the second one is what gets forgotten. It compounds with what I flagged on build-and-push.yml:147: the same blocks are also duplicated into network-adapter, so a new registry is currently a four-place edit across two repos.

A composite action is the fix that works within one workflow — .github/actions/registry-auth/action.yml taking the four *_ENABLED flags and the secrets, uses:'d by both jobs. This PR already establishes that pattern with .github/actions/dhi-login, so it is a small extension of a decision already made here rather than a new one.

If you'd rather go further, a workflow_call reusable workflow shared between the two repos removes the cross-repo copy at the same time — but that's a bigger change and I'd keep it separate from this PR.

na#14 carries this at build-and-push.yml:205.

[manjudr]

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 network-adapter#14.

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 suites

In 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-service build L64-124 vs manifest L179-222 is 44 lines byte-identical; network-adapter the 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:

1. Composite action for the in-repo duplication — .github/actions/registry-auth/action.yml taking the four *_ENABLED flags, 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.
2. workflow_call reusable 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]+ matches v1+.2+.0+ and not v1.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: write wherever google-github-actions/auth appears. Missing in both; the GAR leg cannot mint a token.
• The release workflow needs concurrency too — with cancel-in-progress: false, the opposite of the other two, because a half-cancelled release leaves arch tags with no manifest. And fail-fast: false on 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

⚠️ Do this one last, and coordinate it. Branch protection matches on the job id, not the display name, so adding 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

1. R4 — four small, mechanical, blocking fixes. The release path doesn't work in either repo without them.
2. R1 — make test honest in network-adapter; align cover/test-ci; pick one Dockerfile per repo.
3. R2 — move the four large blocks behind make targets, biggest first.
4. R3 — composite action for the in-repo copies; file the cross-repo reusable workflow separately.
5. 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's test/cover divergence is mostly not your problem — discovery-service's test passes and its cover has the -coverpkg it needs. What is your problem is that test-ci — the target CI actually runs — is the one missing -coverpkg, so the number CI reports is not the number make cover reports. I measured tests/acceptance at 0 profile lines without it and 2088 with it; details on Makefile:86.

[github-actions]

🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)

View full run

Go dependencies

No findings at CRITICAL,HIGH,MEDIUM,LOW.

Container image

No findings at CRITICAL,HIGH,MEDIUM,LOW.

[manjudr]

Read the whole diff. The shape is right — thin workflows, make calls only, logic in the Makefile, one check per concern — and two things here are better than what I landed in network-adapter: image-build runs trivy-release-gate before any push, and require-image-repos validates the repo list up front instead of failing mid-loop.

Four things I'd fix before merge, all in the Makefile, and four suggestions. The two loop findings are the serious ones — I reproduced both with stub binaries rather than reasoning about them, and both exit 0 on a failed push/create.

The PR body still describes run-tests.yml / security.yml / build-and-push.yml; the diff ships ci.yml / ci-release.yml. Also worth a sanity check that targeting feat/18-network-setup rather than development is intentional.

[manjudr]

; between the loop steps means a failed docker push cannot fail this target.

A recipe is one shell invocation with no set -e, so the exit status is the last command of the last iteration alone. Everything before it — including every push to every earlier registry — is discarded.

I reproduced it with the exact loop from this file, two registries, push denied on the first:

==> ghcr.io/org/svc:1.0.0-amd64
PUSH DENIED: ghcr.io/org/svc:1.0.0-amd64
==> docker.io/org/svc:1.0.0-amd64
pushed docker.io/org/svc:1.0.0-amd64
>>> recipe exit=0

Green Build Artifact, image in one of two registries. With a single registry it happens to propagate, so this only bites once someone flips a second *_ENABLED — i.e. it fails silently exactly when the multi-registry feature this loop exists for gets used.

&& between the steps and || exit 1 on the loop:

image-push: require-image-repos
	@for repo in $(IMAGE_REPOS); do \
		dest="$$repo:$(VERSION)-$(ARCH)"; \
		echo "==> $$dest" && \
		docker tag $(RELEASE_IMAGE) "$$dest" && \
		docker push "$$dest" || exit 1; \
	done

[manjudr]

Same masking here, and worse: imagetools inspect is the last command, so it overwrites a failed create.

Probe with this loop, every create failing:

CREATE FAILED
inspect ok (previous index)
CREATE FAILED
inspect ok (previous index)
>>> recipe exit=0

Publish Artifact goes green having created no tag anywhere. And because inspect succeeds on a tag that already exists, a re-tag whose create fails reports the previous release's index back as if it were the new one.

This is the same bug I just fixed in network-adapter's image-publish (138ffbe) — && between every step there, single registry. Here the loop needs || exit 1 as well.

[manjudr]

Unpinned installer, while TRIVY_VERSION right above it is pinned.

trivy/main/contrib/install.sh is fetched from whatever main happens to be and piped into sh, in a job that holds the runner's GITHUB_TOKEN. The version argument is pinned but the script that consumes it is not, so a breaking change on main breaks CI with no commit here, and the supply-chain surface is a moving target.

The tag serves the same file:

	curl -sfL https://raw.githubusercontent.com/aquasecurity/trivy/$(TRIVY_VERSION)/contrib/install.sh | \
		sh -s -- -b $(abspath $(BIN_DIR)) $(TRIVY_VERSION)

Fixed the same way in network-adapter; verified the pinned URL resolves and installs 0.74.0.

[manjudr]

Band divergence: this is HIGH,CRITICAL; the band we settled on for the adapter is CRITICAL,HIGH,MEDIUM,LOW.

network-adapter's live scan comment on PR #14 reads:

## 🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)
### Go dependencies
No findings at CRITICAL,HIGH,MEDIUM,LOW.
### Container image
No findings at CRITICAL,HIGH,MEDIUM,LOW.

SEVERITY feeds the PR gate, the comment header and trivy-release-gate, so at HIGH,CRITICAL a MEDIUM in a released image is invisible in all three. Two repos in the same network reporting different bands also makes the two comments non-comparable.

UNKNOWN deliberately stays out — it means "no vendor CVSS yet", not "severe", and it re-triages on its own once the vendor scores it.

[manjudr]

On a fork PR the whole "Container image" section disappears from the comment, with nothing saying it was skipped.

SCAN_IMAGE=false here drops trivy-image.sarif from SARIF_REPORTS, and trivy-report renders one section per listed report. The ⚠️ No report branch only fires for a report that's listed but empty — a report that was never listed produces no heading at all. So a fork PR gets a comment with only ### Go dependencies, and a reviewer can't distinguish "image is clean" from "image was never scanned".

The rest of this design is right — the gate has to narrow with the list, or every fork PR is red for a reason the contributor can't fix. It's just the reporting that goes quiet. One line after the loop closes it:

		[ "$(SCAN_IMAGE)" = true ] || \
			echo "> ℹ️ Container image not scanned: the dhi.io base images need registry credentials, which a fork PR does not get."; \

Same principle the file already applies to a missing SARIF, extended to a skipped one.

[manjudr]

fail-fast: false plus an eager arch-tag push leaves dangling tags in every enabled registry when one leg fails.

image-push publishes :$(VERSION)-amd64 immediately, so if the arm64 leg fails, :v1.0.0-amd64 is real and pullable in every registry while :v1.0.0 never gets created — and nothing cleans it up. Someone re-running the release then hits create against a half-populated tag set.

The stated reason ("a release wants both legs' verdicts") holds for a test matrix, where the second verdict is information. For a push matrix the second leg's verdict is only useful if the first succeeded; otherwise you've paid a build to publish an artifact you have to go delete by hand. network-adapter uses fail-fast: true for exactly this, and pushes by digest so nothing is tag-visible until image-publish binds it.

If both verdicts are genuinely wanted, push-by-digest=true + --metadata-file gets them without the visible intermediate tags.

[manjudr]

~60 lines of login steps duplicated verbatim between the two jobs, for three registries that are off by default.

dhi-login is already a composite action, so the pattern is established — a .github/actions/registry-logins taking the enable flags would collapse both blocks to one step each and remove the drift risk between them (they have to stay identical or publish-artifact can't read what build-artifact wrote).

Worth asking separately whether Docker Hub, ACR and GAR need to be here at all. All three are vars.*_ENABLED-gated and off, and they cost ~90 lines plus 8 secret references for paths nobody has enabled. YAGNI would say land GHCR and add a registry when there's a consumer — the flags make it look configurable, but nothing exercises those branches, so the first person to flip one is also the first person to test it.

[manjudr]

Review — verdict: merge, but land the dependency bump first or immediately after

Self-review, so weight it accordingly. Everything below was run, not reasoned about.

What actually got exercised

	Evidence
End to End verification	run 34154012020 — pass, 4m02s
Security Scan	same run — fail, and only at the gate; all 12 steps before it green
Build Artifact (amd64/arm64)	run 34153701354 — pass, real tag v0.0.1-rc3
Publish Artifact	same run — pass, manifest list verified linux/amd64 + linux/arm64
Local gates	make build, lint, lint-actions, test-ci (759 tests) all exit 0

The release path is not theoretical — it pushed real images to GHCR twice, and
:latest correctly stayed put both times (v0.0.1-rc3 is not a plain release).

Also confirmed by direct test, not inspection:

• Fork PR degradation. SCAN_IMAGE=false narrows the comment to the
  dependency section and narrows the gate's expected report list, together.
  Gate exits 0. One flag, so the two cannot drift apart.
• Missing-report is a hard failure. With SCAN_IMAGE=true and no image
  SARIF: trivy-image.sarif: MISSING — no scan produced it, exit 1. A scan that
  silently never ran cannot pass.
• Trigger hygiene. A tag push produces exactly one run; a branch push
  produces exactly one pull_request run. No duplicates.

---

One real defect found — not blocking, but worth a follow-up

VERSION is derived from git describe --tags, not from the tag that
triggered the run. If a commit carries two matching tags, describe returns
the most recently created one, which is not necessarily the one that was
pushed. Reproduced:

$ git tag -a v1.0.0 -m release ; sleep 1 ; git tag -a v1.0.1-rc1 -m rc
$ git describe --tags
v1.0.1-rc1

So a push of v1.0.0 would publish ghcr.io/…:v1.0.1-rc1 and, because the
resolved name contains a hyphen, skip :latest entirely — a release that
looks green and ships under the wrong name.

Two tags on one commit is not exotic; promoting an rc to a release on the same
commit does it. The fix is one line — VERSION ?= ${GITHUB_REF_NAME} in CI, or
pass VERSION=${{ github.ref_name }} from the workflow, falling back to
git describe locally. github.ref_name is exact by construction.

Did not fix it in this PR; flagging for a decision.

---

Why the verdict is qualified

Security Scan is red right now, on CVE-2026-17106 —
github.com/moby/go-archive v0.2.0, fixed in 0.3.0, reached through
testcontainers. It is test-only; the shipped image scans clean
(trivy-image.sarif: 0 HIGH,CRITICAL).

It is not caused by this PR — it is a pre-existing vulnerability that this PR is
the first thing to notice, which is arguably the gate working. But merging as
-is puts a red check on the branch and on every PR opened after it, and a gate
everyone learns to ignore is worse than no gate. So: bump the dependency first,
or treat it as the immediate next commit.

Second known issue, also pre-existing and also not fixed here: the coverage
percentage is understated roughly 9×. -coverpkg writes each block once per
test binary (22 copies in this repo); the report sums the denominator across all
copies while crediting the numerator only where count>0. Measured on
src/discover/intent_mapper.go: 8% reported vs 93% true. Verified against
the previous Makefile on the same profile, so this branch did not introduce
it. It has not bitten yet only because this PR changes no Go files.

---

On merge

main has no branch protection today. When it is added, require
CI / End to End verification and CI / Security Scan. Do not require the
release checks — they never run on a PR, so requiring them blocks every merge.

Housekeeping: v0.0.1-rc2 and v0.0.1-rc3 are throwaway test tags I pushed to
exercise the release path, along with their GHCR packages. Safe to delete;
:latest was never moved by either.

[manjudr]

Review — not good to merge yet

One blocking defect, one decision to make, then this is ready. The structure of the change is right: four checks, logic in the Makefile, ${{ }} and uses: left in YAML. make lint-actions is clean and the tag path is proven end-to-end. Details below.

---

🔴 Blocker — the coverage gate fails every PR that touches Go code

cover-diff's awk sums coverage per profile line. Under -coverpkg every block appears once per test binary, so the denominator is counted ~22 times while the numerator is credited only for the binaries that actually hit it.

Measured on this tree — make cover-diff against a base with five changed Go files:

$ make cover-diff BASE_REF=origin/feat/18-network-setup~20
📊 **Test Coverage: ❌ Failed** — 11% of changed lines covered, min 80%

| File | Coverage |
|---|---|
| `src/publish/controller.go`                    |  7% |
| `src/discover/controller.go`                   |  8% |
| `src/beckn/types.go`                           | 11% |
| `src/platform/middlewares/ratelimit.go`        | 12% |
| `src/platform/validation/envelope_rules.go`    | 16% |

The same five files, deduplicated on block id:

  src/beckn/types.go                              81%
  src/discover/controller.go                      89%
  src/platform/middlewares/ratelimit.go           97%
  src/platform/validation/envelope_rules.go       94%
  src/publish/controller.go                       81%
  TOTAL (deduplicated)                            88%

Cross-checked against the authoritative tools, which agree with each other and with the dedup:

$ awk -f tools/cover-report.awk coverage.out | tail -1
TOTAL                                       3502    88.5%
$ go tool cover -func=coverage.out | tail -1
total:  (statements)  88.5%

Block census on the profile: 2294 blocks × 21 copies, 156 blocks × 22 copies.

11% reported vs 88% real. With MIN_COVERAGE=80 the gate is red for any PR touching a .go file, no matter how well tested — so it will be routinely bypassed, which is worse than not having it.

This PR's own check reads ➖ Not applicable only because the branch changes no Go files. The bug is latent here and fires on the next PR.

Worth noting the repo already documents this exact trap, in tools/cover-report.awk (unchanged by this PR):

THE DEDUPLICATION IS THE WHOLE TRICK. Under -coverpkg every source block appears once per test binary that was built […] Summing the statement counts therefore multiplies the denominator by the number of binaries, and treating a block as missed because this binary did not reach it discards the coverage another binary provided.

The new awk is the one place in the repo that doesn't follow it. Fix is to key on the block rather than the line:

{ f = $1; sub(/:.*/, "", f); if (!(f in want)) next;
  st[$1] = $(NF-1); owner[$1] = f; if ($NF > 0) hit[$1] = 1 }
END {
  for (b in st) { tot[owner[b]] += st[b]; if (b in hit) cov[owner[b]] += st[b] }
  ...
}

Please add a fixture-based test for it — a checked-in profile with a known duplicated block and an expected percentage. A gate whose arithmetic nothing exercises is how this got through.

---

🟡 Decide before merging — Security Scan is red

CVE-2026-17106, github.com/moby/go-archive v0.2.0 → 0.3.0, reached only through testcontainers. It is not linked into the shipped binary and the image scan is clean, so the exposure is the test path only. Either bump it or record the acceptance — merging with the check red and no note is the option to avoid.

---

🔵 Notes, not blocking

SEVERITY now spans CRITICAL,HIGH,MEDIUM,LOW and trivy-gate fails on any finding in that band. Intentional, for consistency with network-adapter. The consequence to go in with eyes open: Trivy refreshes its DB every run, so a new LOW landing upstream turns unrelated PRs red with nothing in the diff to fix. Measured today it costs nothing (1 finding at the wide band, 1 at HIGH,CRITICAL). If it gets noisy, gating on HIGH,CRITICAL while reporting the full band is the usual split.

dhi-login interpolates the secret into the script body. AUTH=$(printf '%s:%s' "${{ inputs.username }}" "${{ inputs.token }}" | ...) — the value is substituted before bash parses the line. Passing it through env: and referencing $TOKEN keeps the secret out of the script text. Low risk for a token, but it is the cheap habit.

Dependency bumps ride along. x/crypto 0.54.0→0.55.0, x/text 0.40.0→0.41.0 in dea73eb, on a branch that is otherwise pure CI. Fine if it was clearing a scan finding — worth a line in the commit body saying so.

clean uses $(SARIF_REPORTS), which narrows under SCAN_IMAGE=false, so trivy-image.sarif can survive a clean. Harmless today; rm -f trivy-*.sarif is unambiguous.

Two orphaned bot comments above this one, from the pre-consolidation two-comment scheme. find-comment matches the literal <!-- sec-scan -->, which is not a substring of <!-- sec-scan:trivy-deps -->, so nothing will ever update them — they will sit there showing a stale severity band and a stale run link. Worth deleting.

---

✅ Verified working

• Check topology is as specified — End to End verification + Security Scan on PR, Build Artifact (amd64/arm64) + Publish Artifact on tag. Release jobs live in their own file so no permanently-skipped ${{ matrix.arch }} checks appear on PRs.
• VERSION comes from github.ref_name, not git describe. Proven against a decoy: git describe --tags 515e03e returns zz-decoy, the run published v0.0.1-rc4 to both platforms and correctly declined to move :latest on a pre-release.
• Release path is GHCR-only. The dead switchboard for three unconfigured registries is gone — checked, and the repo has no Actions variables and no matching secrets.
• Real deduplication where it counts: one jq program (was two copies, and the drift between them took the Security job down), one Trivy cache action (was three), one dhi.io login.
• make lint-actions → exit 0.
• Image is gated by Trivy before push, and image-publish re-tags rather than rebuilding, so the pushed bytes are the scanned bytes.

Verdict: hold. Fix the cover-diff dedup and settle the CVE, and this is good to go.

[manjudr]

Re-review — ✅ good to merge

Every item from the previous review is addressed. Both checks are green on 58b8af9, and the two greens are for real — evidence below rather than a claim.

Finding	Status	Commit
🔴 Coverage gate reports 11% for 88% real	Fixed + regression test	d110d4e
🟡 Security Scan red on CVE-2026-17106	Fixed	05b7be9
🔵 Secret spliced into run: script text	Fixed	bf76ddd
🔵 clean narrows under SCAN_IMAGE=false	Fixed	d110d4e
🔵 Two orphaned bot comments	Deleted	—
🔵 SEVERITY spans down to LOW	Kept, deliberately	—
🔵 Dep bumps riding along in dea73eb	Left as-is	—

---

The coverage fix

Extracted to tools/cover-diff.awk and keyed on the block rather than the profile line. The same command that failed at 11% before:

$ make cover-diff BASE_REF=origin/feat/18-network-setup~20
📊 **Test Coverage: ✅ Passed** — 88% of changed lines covered, min 80%
exit=0

$ go tool cover -func=coverage.out | tail -1
total:  (statements)  88.5%

11% → 88%, agreeing with go tool cover. That agreement is now the invariant the test pins, not a thing someone has to remember.

Writing the test surfaced a second defect that the review missed: a changed file whose blocks carry no statements divided by zero and aborted awk with exit 2 — before the T == 0 check could report EMPTY. An unmeasurable file crashed the gate rather than skipping it. Guarded, with a fixture.

Red-green, both directions:

# original per-line summing, against the new fixtures
--- FAIL: TestDuplicatedBlocksCountOnce
    total = 20%, want 60% — a block hit in one of three copies is
    covered, not one-third covered
--- FAIL: TestUnchangedFilesAreIgnored
--- FAIL: TestNoCoverableStatementsIsEmpty
    awk failed: exit status 2 / awk: division by zero

# deduplicating version
ok  github.com/OpenAgriNet/discovery-service/tests/covertool

One more thing worth flagging, since it nearly made the test useless: awk is a subprocess, so the Go test cache could not see the script change. Sabotaging the entire program to END{print "TOTAL\t0"} and rerunning still reported ok (cached) — a regression test that goes quiet exactly when the thing it guards is being edited. 58b8af9 reads the program with os.ReadFile and runs the copy, so the read is a tracked input. Same sabotage now fails immediately with no -count=1. Verified both ways.

The CVE

moby/go-archive v0.2.0 → v0.3.0 (pulling klauspost/compress and moby/sys/user along). Both scans are now empty, and the scanner is demonstrably still working — rescanning the old go.mod still reports it:

$ bin/trivy fs /tmp/oldmod --severity CRITICAL,HIGH,MEDIUM,LOW
CVE-2026-17106  github.com/moby/go-archive  v0.2.0 -> 0.3.0

$ make trivy-gate            # on HEAD, in CI
trivy-deps.sarif:  0 CRITICAL,HIGH,MEDIUM,LOW
trivy-image.sarif: 0 CRITICAL,HIGH,MEDIUM,LOW

Deliberately not changed

SEVERITY still spans CRITICAL,HIGH,MEDIUM,LOW — consistency with network-adapter was the explicit ask, and it costs nothing today (0 findings at the wide band). The trade-off stands as recorded: a new LOW landing upstream can redden an unrelated PR. Revisit by gating on HIGH,CRITICAL while still reporting the full band, if it gets noisy.

Dependency bumps in dea73eb — history already pushed; not worth a rewrite for a commit-message note.

One disclosure: the clean fix landed inside d110d4e rather than its own commit. Its rationale is in the file, next to the code.

---

Verification

make build       exit=0
make lint        exit=0     (0 issues)
make test        exit=0     (every package, incl. tests/covertool)
make lint-actions exit=0

CI on 58b8af9 — run 34156762400:

End to End verification   pass   1m59s
Security Scan             pass     47s

The tag path was proven earlier against a decoy tag: git describe --tags 515e03e returns zz-decoy, while the run published v0.0.1-rc4 to linux/amd64 + linux/arm64 and correctly declined to move :latest on a pre-release.

One caveat on scope, stated plainly

This PR changes no non-test Go files, so its own coverage check still reads ➖ Not applicable — the gate's arithmetic is not exercised by this PR's check. The proof that it works is the unit test and the manual BASE_REF run above, not the green tick on this page.

Verdict: good to merge.

Suggested follow-up, not blocking: delete the throwaway tags v0.0.1-rc2/rc3/rc4 and zz-decoy and their GHCR packages once this lands.