Repository navigation
release: bring development into release-0.2.0 - #61
Merged
Merged
Conversation
A PR carried 4 entries: End to End verification, Security Scan, CodeQL (its own job), and GitHub's native Code scanning results / CodeQL for the SARIF upload. Folded CodeQL's init/build/analyze steps into ci.yml's security-scan job, so a PR now shows exactly 2 controllable checks — End to End verification and Security Scan, the latter running Trivy and CodeQL as steps in one job. codeql.yml drops its pull_request trigger entirely rather than becoming a skipped check on every PR: it keeps only push (trunk baseline for the "new alerts" comparison) and its weekly schedule (deep re-scan against updated query packs). Nothing here shares a file with a job of a different trigger scope, so nothing posts a permanently-skipped entry anywhere — that was the failure of an earlier version of this branch, reverted before this commit. Residual, not removable by any workflow restructuring: GitHub still posts "Code scanning results / CodeQL" itself whenever CodeQL uploads SARIF, no matter which job the analyze step lives in. It is not a job, carries no `name:`, and cannot be suppressed — confirmed by 2e33655 on this same repo. Accepted as a platform annotation, not a CI run: it consumes no separate Actions minutes and runs nothing of its own. Verified: actionlint 0 issues across all 5 workflow files; a Python simulation of every file's `on:` against (event, ref, schedule) shows the PR event firing only ci.yml's two jobs and every other event firing only the jobs that apply to it, with zero skipped entries anywhere; full test suite green (EMBEDDING_PROVIDER=hashing go test -race ./...).
A push to release-0.0.1/main fired CodeQL (its own job in codeql.yml) and Scan the trunk (Trivy + govulncheck in security.yml) as two separate checks - the same duplication ci.yml's security-scan job already fixed on the PR side. Every such push also lands on PR #36 (release-0.0.1 -> main, still open), so this is what that PR sees accumulate on top of Publish coverage. Folded CodeQL's init/build/analyze steps into security.yml's scan job (same steps, same category codeql-go, as ci.yml's security-scan job runs for a PR) and deleted codeql.yml. A push to the trunk now posts 2 job-checks (Publish coverage, Scan the trunk) instead of 3. Nothing dropped: same Trivy dep+image scan, same govulncheck, same CodeQL security-extended queries, same SARIF categories, same gates. One schedule entry instead of two: codeql.yml's weekly cron existed only to run CodeQL an hour apart from security.yml's own, so the two jobs' scheduled runs never queued against each other. There is only one job now, so only one cron stays (17 3 * * 1). Verified: actionlint 0 issues on all 4 remaining workflow files; Python simulation of on: triggers confirms a push to the trunk now fires exactly 2 jobs, a PR fires exactly 2 jobs, nothing skips anywhere; full test suite green (EMBEDDING_PROVIDER=hashing go test -race ./...).
CodeQL flagged make(map[string]any, len(targetObject)+len(patchObject)) at mergepatch.go:54 (go/allocation-size-overflow, CWE-190) across two alerts on PR #36 — the sum of two int lengths feeds an allocation with no overflow check. Reviewer deferred it to 0.2 instead of fixing it. The operands are decoded-map key counts, not attacker-controlled byte sizes, so real overflow needs ~2^63 map entries — not reachable on any real machine. Still applied CodeQL's own recommended remediation (guard the arithmetic before allocating) rather than dismiss the alert: a wider type doesn't apply here since make()'s size argument must be int-compatible, and validating upstream input size doesn't touch this specific arithmetic. Falling back to capacity 0 on overflow costs a map resize, never a crash.
The prior commit guarded len(targetObject)+len(patchObject) with an overflow check before allocating, per CodeQL's own remediation advice. CodeQL re-flagged it anyway (alerts #6, #7) on the guarded line itself - it flags any addition of two map-derived lengths reaching an allocation, regardless of a bounding condition around it. It doesn't do the range analysis to see the guard makes that addition safe. So the only fix CodeQL's scanner actually accepts is removing the sum, not bounding it. Dropped the capacity hint entirely: the map now grows through its normal resize path instead of being pre-sized. No behaviour change, confirmed by the unchanged MergePatch test suite.
…lows chore: fold CodeQL into Security Scan/Scan the trunk, 2 checks per event [#99]
…overflow-guard fix: guard mergepatch map-capacity sum against overflow [#38]
Prompts a description with a required issue link, plus a short self-review / end-to-end-verification / docs-updated checklist. Deliberately excludes build/lint/test and breaking-change checkboxes since CI and the commit-message convention already enforce those.
chore: add a lightweight PR template [#41]
`make image-build` ran Trivy over the freshly built image with `--exit-code 1` at `SEVERITY=CRITICAL,HIGH,MEDIUM,LOW`. Any finding at any severity aborted the target, so the `make image-push` that follows it never ran and the release produced no artifact at all. Tag 0.1.0 died exactly that way on both arch legs, on 3 LOW and 1 MEDIUM: ``` make[1]: *** [Makefile:353: trivy-release-gate] Error 1 make: *** [Makefile:545: image-build] Error 2 ``` The scan is removed rather than made non-blocking, because by the time a tag exists this image has already been scanned three times and none of those three is this one. - `ci.yml` builds the same Dockerfile on the PR, scans it, and `trivy-gate` blocks the merge on any finding. - `security.yml` builds and scans it again on the push to the branch the tag is cut from, and uploads the SARIF to the Security tab. - `security.yml` again, weekly — the only one of the three that can catch a CVE published after the code stopped changing, which is the one thing a tag-time scan could otherwise have added. So a fourth scan re-reported what was already reported and acted on, and the only new thing it could produce was a reason not to ship. It cost a Trivy install, a DB fetch and a full image scan on each of the two arch legs. Removed: the `trivy-release-gate` target and its `.PHONY` entry, the `$(MAKE) trivy-release-gate` line in `image-build`, and the `trivy-cache` step in the release job — nothing there runs Trivy any more. `TRIVY_IMAGE_SCAN` is inlined back into `trivy-image`; the gate was its only other caller. The `image-build` and `image-push` doc comments no longer describe a scan, and `image-build`'s records why there isn't one so the next person does not re-add it. `trivy-deps`, `trivy-image`, `trivy-report` and `trivy-gate` are untouched, and the PR gate stays blocking at every severity.
CVE-2026-81871 (MEDIUM) in otlploggrpc v0.20.0 -- the log gRPC exporter ignores env TLS certs, bypassing mTLS/pinning -- and CVE-2026-81870 (LOW) in otlptrace v1.44.0, where exporter config logging may leak endpoint URLs into info logs. Four findings in the shipped binary between them. A minimal bump is not available: MVS pulls the whole release train, and the stable v1.x and experimental v0.x lines ship in lockstep, so otel/log has to move to v0.21.0 with the rest. v0.21.0 drops the log package's own attribute constructors -- Record.SetBody now takes attribute.Value and Record.AddAttributes takes attribute.KeyValue -- so log.StringValue and log.String become attribute.StringValue and attribute.String. attribute.Key is a named string type rather than a bare string, hence the conversion at the test's map index. That is a type-namespace move onto otel's standard attribute type, not a change in what is emitted: the same five spec-Required keys with the same values on the same Record. The audit tests assert those keys off the emitted record and are unchanged. log.Record, log.SeverityInfo4, log.Logger and log.WithInstrumentationVersion all still live in otel/log and are untouched.
…without-blocking-release fix: stop scanning the release image at tag time and clear the otel CVEs
A discover answered one page and said nothing about it. The page could only be asked for in the query string, which the request signature does not cover and which no Beckn caller knows to send, and the answer had no member to say which page it was or whether more existed - so a short page and the last page read the same. The validator now loads the OAN profile of beckn.yaml, pinned at commit dfc1097 of OpenAgriNet/protocol-specifications-v2 (no tag exists yet; swap the SHA for one when it is cut, never for the branch). It adds limit/offset to DiscoverAction and an optional pagination to OnDiscoverAction. Recorded as A28 in the plan. - The page is read from the body, each field falling back to the deprecated query parameter of the same name, so existing callers (verify.sh among them) keep working. The body wins because it is the signed half. - pagination reports the page APPLIED, after default and clamp: a caller that asked for 5000 must read back 100, or its next offset skips results. - total is len(fused) and is sent only when exact. It is omitted whenever any retrieval list came back as long as MaxCandidatesPerMode, because that list may have been cut by its LIMIT. Rejected: reviving the count query, which A19 deleted for costing 100x the search. Reverting the cap check makes TestATotalIsReportedOnlyWhenNoModeHitItsCap report 8 where 10 matched. - The past-depth refusal is unchanged and now points at $['message']['offset']. - config refuses maxPageSize above 100, the schema's maximum, which would otherwise answer query-string callers with a pagination.limit that fails its own schema. Not in scope: the same profile lets context.networkId be an array. beckn.Context still reads a string, so a list is refused by the envelope parser exactly as before; its meaning for scope and visibleTo is its own issue.
x/net v0.58.0 carries CVE-2026-78669 (HIGH), CVE-2026-78663 (MEDIUM) and CVE-2026-97032 (MEDIUM), all fixed in v0.60.0. trivy-gate fails on them, so every PR into development is blocked until they clear. x/net is indirect, pulled in by grpc for the OTLP gRPC exporters. MVS moves the rest of golang.org/x with it: crypto v0.57.0, sync v0.23.0, sys v0.48.0, text v0.42.0. crypto is the one direct use, blake2b only, and its API is unchanged. grpc and the go directive stay where they are.
fix: bump golang.org/x/net off the CVE-affected version [#53]
kin-openapi v0.147.0 carries CVE-2026-100352, -100353 and -100354, all HIGH. trivy-gate fails on them, so every PR into development is blocked. None is reachable here: we decode JSON and use the built-in visitor, never import openapi3filter, and neither spec has a circular allOf/anyOf/oneOf chain. v0.150.0, not the v0.149.0 Trivy names. v0.149.0's cycle guard stops validating self-referencing schemas below the root, so an invalid member of a GeometryCollection passes L1; v0.150.0 restores that. Pinning back to v0.149.0 reopens the hole. v0.150.0 also stops checking const in the built-in visitor. That changes no request outcome: context.version is enforced by the envelope rules, and context.action is rewritten to its canonical value before L1 runs. A differential run of 1,416 bodies through ValidateEnvelope and L1 gave the same verdict, message and path as v0.147.0, on both the current spec and #52's. Transitively santhosh-tekuri/jsonschema/v6 v6.0.2 -> v6.0.3.
fix: bump kin-openapi off the CVE-affected version [#55]
Pagination now comes only from message.limit and message.offset; the query string is no longer read. The response returns the applied page as message.pagination.
0.1.0 read ?limit=/?offset=; ignoring them now would silently return the default page. Refuse with SCH_INVALID_FORMAT naming the new location. BREAKING CHANGE: discover pagination moved from ?limit=/?offset= to message.limit/message.offset; the query parameters are now refused with 400.
Total is the ranked list's length before paging, capped at the retrieval depth so it never points past the last page a caller can fetch.
The pinned dfc1097 capped the request limit at 100, so the validator refused an oversize limit before the clamp could run. Pin to a8451c4, bind the response to DiscoverPagination with a required total, and move the pagination decisions from code comments into ADR-0017.
Both depth guards refused a page whose tail crossed the retrieval depth, but the page slice already truncates that tail. At depth 500, limit 30 refused offset 480 with 30 results still unread, and the caller could never satisfy the spec's end-of-results test (offset + count == total). It only looked right at limit 20, which divides 500 evenly. The guard now reads the offset alone: a page that starts past the depth is refused, one that starts inside it is answered, short if need be.
The depth counts ids PER MODE, so fusing two modes yields more ids than either retrieved — 14 from a depth of 8 on the test corpus. Only `total` was trimmed to the depth; the page was still cut from the untrimmed list, so the last page carried more resources than `total` left room for and offset + count == total never came true. Trim the fused list once, before the page is cut, so the page and the total come from the same list.
The depth rule was written in two places and the page was read by a function doing two jobs. - Search.OffsetBeyondDepth and Search.CapToDepth hold the comparison once; the request layer and the storage layer both call them. - pageFrom reads the signed body; rejectQueryStringPagination refuses a page the signature does not cover. - SearchResult.Total says what it holds and where it is capped. - ADR-0017 records why the cap is applied before the page is cut, and why the refusal tests the offset rather than offset + limit.
`ranked` meant the ranked modes in one function and the ranked id lists in another, in the same file, and `fold` said nothing about what it folded into. - negotiate returns rankedModes, partitionOutcomes returns rankedIDs. - withinRetrievalDepth reads as a predicate but returns an error, so it is ensureWithinRetrievalDepth. - pageOf's manual clamp is min(). - RRF presizes for the fused worst case: 6 allocations and 79KB against 32 and 169KB, at the same wall time.
…eryservice feat: read discover pagination from the request body [#51]
…ks it named [#58] Two halves of one rule. A publisher who omits visibleTo reaches every network instead of their own, and a caller names the networks it wants rather than being handed some of them. visibleTo keeps its name and signature; the default is one literal. The same literal replaces the one in applyDirectiveDefaults, so neither of the two places that resolve it has moved. context.networkId is a string or a non-empty array (spec release-0.2.0, pinned at 7a9fdf0d — the only schema change in a 39-line diff). beckn.NetworkIDs decodes both and remembers which arrived, because the response echoes the request's context and a caller that sent a string must not get an array back. Everything inside the service reads Networks() and never asks which shape it was. The scope gate collapses to one array overlap. Five queries lose the narg IS NULL dance and the separate wildcard arm for r.visible_to && @network_ids::text[] so '*' appears in no query at all: it is an ordinary entry the caller sends. The default sits in discover.Service and nowhere else, which is what lets the storage layer stop defaulting and visibleOn disappear rather than grow. idx_resources_visible_to is GIN array_ops, which indexes && as it indexes @>. An omitted networkId now means the public catalogs rather than every catalog. That is the one behaviour an existing caller loses without asking, and it is the price of the field meaning the same thing in all three forms. Four scenarios, not a matrix: an unstated audience is public; a search naming one network gets that network; a search naming a network and '*' gets both; the string and array forms answer the same page.
…#58] Split from the code PR, which is about something a reviewer can check by reading Go; this is the plan, which is the spec and changes first. A28: an omitted visibleTo is the wildcard. A9 is NOT carved up — the default still applies in both update modes, by the same two lines. The first draft of A28 made an omitted visibleTo mean "keep what is stored" under MERGE; review took it out as a change to what MERGE means rather than to a default, and the hazard it named is now a Deferred row so the argument survives the code that was removed. A29: context.networkId is one network or several, and a search matches exactly the networks it names. Public stops being something a scoped caller is handed and becomes a network they ask for, which is what lets the five queries collapse to one array overlap with no literal in them. Two Deferred rows carry what was tried and pulled: the MERGE keep rule, and deleting Catalog.NetworkID, which gofmt punishes out of proportion to its value in a review about something else. Scenarios 26a, 29 and 29a state what the four tests assert. The scenario count stands at 37.
Review asked for a smaller diff and no renames. Five test and conformance functions go back to the names they had, with only their bodies changed: the two in discover/service_test.go, the one in publish/service_test.go, the telemetry one, and both conformance cases. directives() keeps its name AND its arity — the second parameter is now `_`, so its call site does not move either. One rename stays. TestOmittedNetworkIDSearchesEveryNetwork now asserts that a search matches only the networks it named, so the old name says the opposite of the body. It is TestASearchMatchesTheNetworksItNamed, with the former name in its comment so `git log -S` still finds it. Comment churn trimmed where the old text was still true: query.go was four code lines against eleven of prose. What is left changed because it stated the old rule as fact — `"" means UNSCOPED, every network` is not something to leave sitting in the file. examples/ is reverted entirely, at the author's request. make verify, make audit and make newman fail on this branch as a result (20, 13 and 19), all from one cause: the samples publish to local-network and then search with no networkId, which now means the public catalogs. Four lines out of the publish sample fixes all three.
Five things the scope change left behind, found by running the gates the PR
body said were green.
make test was red in src/storage/postgres, 18 tests. The conformance suite's
searched() helper got a wildcard default for a query naming no network; this
package's search() did not, so every case going through it sent
`visible_to && '{}'::text[]` and matched nothing — which is why paging,
geometry, hydration and schema-filter tests all failed for a reason that has
nothing to do with them. The default now lives in four places that each needed
it: search(), filterFor(), and the new publicScope()/publicQuery() for the
cases that call a retriever or the hydrator directly, where nothing above them
is running to fill the scope in. A fixture that names no audience is now
public, which is the same default the publish mapper applies (A28) — without
it, every unrelated case would have to state a network to be found at all.
TestAnOmittedNetworkIdSearchesEveryNetworkAndAGivenOneNarrows asserted the rule
A29 reverses, and its name said so. It is now
AnOmittedNetworkIdMatchesOnlyPublicAndANamedOneNarrows and pins all three
rules against three catalogs, because a fix aimed at any one of them is a
regression in another.
An empty networkId was a 200 holding nothing. "" unmarshalled to a one-element
scope no row's visible_to can hold, and the spec declares no minLength on the
string branch, so L1 let it through. tests/testdata/beckn-v2.0.0.yaml is
byte-identical to the pinned upstream and must stay so — the fetch path and an
air-gapped mount agree only because it is — so the refusal goes in
NetworkIDs.UnmarshalJSON instead, the one place that sees the value before the
scope is built. It becomes a NACK at the edge rather than a page a caller
cannot tell from "nothing matched".
visibleTo took a parameter it did not read. The blank name said the argument
was dead while the signature said it was not; the caller still computed it.
`network` keeps its real reader two lines up.
The memory backend's comment above admitted() still described the pre-A29
rule — an empty scope as UNSCOPED, emitting no predicate. The code beneath it
returns nothing for an empty scope. The comment now says what the code does.
The 17 discover examples named no network and were answered by nothing, which
is what made verify, audit and newman fail at 20, 13 and 19. They now name
["local-network"], which is what 01-publish names in visibleTo, and the
Postman collection is regenerated from them.
make build, make lint (0 issues), make test, make verify (42/0),
make audit (47/0) and make newman (107 assertions, 0 failed) all pass.
Three passages still described the shape A29 replaced, and A29's own row described a marshal the code does not perform. The row claimed NetworkIDs collapses a one-element array to a string, so a caller who sent ["n1"] is echoed "n1", and argued for it on the grounds that the alternative means carrying arrival-shape state. The code carries exactly that state — one bool — and echoes back the shape that arrived. It is the better behaviour: the response context is the request's context read back, and a field that changes spelling in it reads as something having happened to it. The oneOf permits either, so this was never a conformance question, only a surprise question. The row now says what ships and why. The resources DDL commentary still carried the three-arm `sqlc.narg(...) IS NULL OR ... @> ARRAY['*'] OR ... @> ARRAY[$1]` with two paragraphs arguing for it, while A29 further down the same document replaced all of it with one `&&`. Two SQL shapes in one plan means whoever implements against it picks one. The block is now the shipped predicate, and the prose says why overlap is the whole gate: the scope is never empty, so there is no absent-predicate case to write. force_custom_plan survives, with network_id removed from the list of clauses that need it — target_paths, spatial_op, the four box bounds and schema_contexts are still nullable, so the setting is unchanged. Task 16's test pins say what the test now reads, and the omitted-networkId scenario is restated as A29's three rules against three catalogs, which is what the shipped test asserts.
feat: make an unstated visibleTo public
docs: record the public-by-default audience and one-or-many networkId
🛡️ Trivy security scan (CRITICAL,HIGH,MEDIUM,LOW)Go dependenciesNo findings at CRITICAL,HIGH,MEDIUM,LOW. Container imageNo findings at CRITICAL,HIGH,MEDIUM,LOW. |
|
📊 Test Coverage: ✅ Passed — 95% of changed lines covered, min 80% |
manjudr
approved these changes
Oct 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings release-0.2.0 up to date with development.
visibleTois public (feat: make an unstated visibleTo public #59, docs: record the public-by-default audience and one-or-many networkId #60)