[Development] Discovery Service Setup - #20
Merged
Conversation
Store two covers per geometry instead of one — cells_full (CONTAINMENT_FULL, a guaranteed subset) and cells_cover (CONTAINMENT_OVERLAPPING, a guaranteed superset). A geometry represented as a set of cells turns every CQL2 operator into indexed array algebra on BIGINT[], answerable with no spatial extension. Seven of nine operators are now answered over all seven RFC 7946 types on both sides of the comparison; the previous design answered S_DWITHIN for Point only. S_TOUCHES and S_CROSSES are refused with 400 / SPT_UNSUPPORTED_OPERATOR rather than deferred. Touching is measure-zero and cells have measure, so refining the resolution does not converge — listing them as "later" would promise something no resolution can deliver. A geometry that cannot be proven to fail is returned, so the result set is a superset of the exact answer under ANY and a subset under NONE. That also makes quantifier ALL answerable for the first time, since every geometry type is now decidable. Also in this change: - An offer's geometry attributes to that offer's resourceIds rather than to the whole catalog: one resource_geometries row per id, falling back to catalog-level when the array is absent or empty, which is the meaning offers.resource_ids already carries. `touched` now follows offers too, or a patch that moves a shopfront and names no resource would re-derive the geometry and write it nowhere. - JSONPath attribute filtering moves into Phase 1. Both columns and both jsonb_path_ops indexes were already landing in Task 14, so it costs a parser, a rebase and a WHERE clause, and no migration. - SEARCH_FAIL_ON_UNAVAILABLE_MODE defaults to false. Phase 1 ships EMBEDDING_PROVIDER=noop, so defaulting to refuse would make every textSearch a 400 until an Ollama deployment exists. Open Items is now empty; each decision is recorded with its reasoning.
The Task 19 test pin said an absent discover `networkId` falls back to APP_NETWORK_ID. Four other places say it searches every network: the service.Discover pseudocode, the Scope struct commentary, scenario 29, and the closing decisions table. The pin is the stale one — it predates the decision — and it is the one that matters, because a test pin is the sentence that becomes an assertion. Implemented from that bullet alone, Task 19 would have reintroduced single-network scoping and failed scenario 29. Kept rather than deleted: scenario 29 asserts the behaviour end to end, this pin asserts the mapper emits no network predicate at all, which is the thing that actually breaks. Also annotate `networkId` in the discover wire shape. It carried a bare OPTIONAL while the identical field in publish's identical context block is annotated "defaults to APP_NETWORK_ID" — an invitation to assume symmetry, and the likeliest way the wrong bullet got written.
] Comments, Abstraction, No globals, Errors, Test doubles and TODOs join Global Constraints, which is the section that says "every task inherits these." A separate "conventions not yet formalized" note was the other option and is worse: a section with no claim on any task is a section nobody enforces. Five are new statements of what the plan already does. Abstraction is not — as usually phrased ("no interface until the second implementation needs it") it would forbid the Embedder, CatalogReplicator and Keyring seams this plan deliberately ships. So it is stated in the form A6/A7 already uses: a seam ships with a conformance test or a second implementation behind it. That admits the three named seams, each of which has a constructing task and an exercising test, and still refuses the speculative interface the rule is aimed at. Errors draws the boundary the codebase would otherwise blur: %w-wrapping is for Go errors, while a validation Fault is a value with its own Path and Code, aggregated rather than returned, and never wrapped.
Task 1 of docs/design/discover-and-publish.md. - Module github.com/OpenAgriNet/discovery-service on Go 1.25, directory skeleton per the design document's File Structure. - make build|test|lint|sqlc|migrate, plus security, docker, up/down. Test targets pin EMBEDDING_PROVIDER=hashing rather than inheriting the production noop default (A5); without the pin the whole semantic path goes untested from the day semantic search was deferred. - Toolchain pinned in a separate tools/ module so the service go.mod states only what ships: a licence audit and the T4 supply-chain scan cannot tell a linter's transitive dependency from a runtime one once the two graphs are merged. golangci-lint, sqlc, govulncheck and golang-migrate build into bin/ on demand, so a clean checkout needs nothing installed but Go. - .golangci.yml enables gosec, errcheck, govet, revive and staticcheck, with revive configured to enforce the Global Constraints that were otherwise review conventions: godoc on exported symbols, 50-line functions, control nesting depth 3, early returns. - Multi-stage Dockerfile, dependency layer first, -trimpath for reproducible bytes, distroless static runtime. schemas/, migrations/ and config/ are copied; beckn.yaml deliberately is not, because it is fetched at boot from VALIDATION_SPEC_URL and a baked copy is a second source of truth that ages with the image. - docker-compose pins pgvector 0.8 on PostgreSQL 16 (D6). - CI runs lint, test and security (govulncheck plus a Trivy image scan failing on HIGH/CRITICAL — T4). The scan is of the image rather than of go.sum so the pinned toolchain cannot fail a release it never ships in. - ADRs 0001-0015: 0001-0011 are the Technology Decisions table (D1-D11), 0012 and 0013 come from T5, 0014 is the seam-ships-with-a-test rule applied to CatalogReplicator and Keyring (A7), 0015 is the master-catalog-and-inheritance rejection (A1). - MIT LICENSE, matching the rest of the OpenAgriNet org.
- assign memory backend + conformance suite ownership across Tasks 11/12/15/16 - add no-op Trace middleware in Task 8, upgraded by Task 23, fixing the ordering bug where Task 20 tested a chain position Task 23 hadn't built yet - pin stats.itemCount/providerCount/categoryCount assertions in Task 18 - remove stale (PARKED) tags on filter_parser.go/jsonpath.go (Task 22 ships them) - complete A4/A7 task citations, correct A1's mapper attribution - add missing tests/testdata/beckn-v2.0.0.yaml to Task 4's Files list
…ng drift - File Structure tree pointed at the retired documentations/ layout instead of docs/adr + docs/design - filter-subset validation now lives where the doc calls it load-bearing (src/platform/jsonpath/), with Task 22's Files list to match; postgres/ jsonpath.go only executes the already-validated expression - Trace's Task 8 no-op gets an observable X-Beckn-Trace-Seen marker so Task 20's order test can actually assert its position; Task 23 drops it - Task 18's Tests-pin used defaultDirective; the pseudocode calls it applyDirectiveDefaults - A1 no longer cites Task 17, which never mentions MASTER/extends - A9 no longer claims all five defaulted fields are mapper-resolved -- catalogType/updateMode/visibleTo are directive fields resolved in publishOne before the mapper runs
…der pin [#11] sqlc.narg('spatial_op') was declared @spatial_op at two of its three uses in the discover EXISTS block — the exact mistake the paragraph below it calls fatal, since sqlc generates from whichever declaration it reads and the first line uses NULL as the no-spatial-constraint sentinel. The catalog-geometry insert carried a comment saying there was no ContainmentFull cover to write; cells_full is that cover. The mode with no column is ContainmentCenter. Task 17's Produces named ExtractCatalogGeometries(catalogIndex, provider), the pre-walker signature, three lines above prose saying it takes the merged catalog. Now ExtractGeometries(catalogIndex, merged), spelled the same way at its definition and its call site. Task 20's middleware-order assertion could not fail: Trace stamped its marker before calling next, so it survived a recovered panic under either nesting. Trace and Recover now both append to one ordered X-Beckn-Chain header, and the test reads the order of the two entries rather than the presence of one.
The plan is the contract; this is how it gets executed — one task per session, with a review gate between tasks. It was sitting untracked, so a clone had the plan and no way to drive it.
A Point and a LineString contain no H3 cell, so their cells_full is the
empty array — and '{}' <@ anything is TRUE in PostgreSQL while
'{}' && anything is FALSE. Three operator branches were phrased over
cells_full on the degenerate side and therefore stopped being predicates:
S_WITHIN and S_CONTAINS fell back to the bounding box, and S_DISJOINT,
which skips the box by design, matched every Point in the corpus. Since a
shopfront is a Point, that is the commonest row in the table.
Refute with cover, prove with full. A ⊆ Q implies cover(A) ⊆ cover(Q), so
NOT (A.cover <@ Q.cover) refutes S_WITHIN soundly and strictly more
tightly than the full form it replaces; S_CONTAINS is the same statement
with the arguments swapped. S_DISJOINT gains two refutations by the same
reasoning: A.cover <@ Q.full means A ⊆ Q, and a non-empty A inside Q
meets it. Scenario 30 could not have caught any of this — Polygon vs
Polygon is exactly the case where both full covers are non-empty.
Also in this change:
- `touched` now unions an offer's resourceIds from BEFORE and AFTER the
merge. Relocating an offer from r1 to r2 touched only r2, so r1's
geometry row was never deleted and a spatial search on the offer path
kept returning a resource that offer no longer covered — for ever.
- Three CHECK constraints on resource_geometries, on the argument the
CHECK (id <> '') block already makes. cells_cover being non-empty is
load-bearing now rather than tidy: it is what the cover <@ cover
refutations rest on.
- plan_cache_mode = force_custom_plan on the pool. Every nullable
predicate in discover.sql is index-usable only when the planner can
fold the IS NULL arm away, which a generic plan cannot — so the fast
plan is the one a cold connection gets and the slow one is what a warm
connection settles into. EXPLAIN assertions now run six times first.
- A redundant sargable = ANY beside the schema EXISTS, so
idx_resources_schema can drive the scan instead of filtering after it.
- IS DISTINCT FROM on the gate propagate: the rule is that every resource
ENDS the transaction carrying the catalog's gate, not that every row is
rewritten into a fastupdate = off GIN index on every publish.
- idx_resources_catalog_id and idx_offers_catalog_id dropped; both tables
are keyed (catalog_id, id) and the primary key's btree already leads
with catalog_id.
- source_path wildcards the catalog's own index, which is a property of
the request rather than of the catalog. Fault paths keep the concrete
one. target_path = wildcard(source_path) is now an asserted invariant.
- The same catalog id twice in one request is REJECTED rather than
silently last-write-wins across two transactions.
- geo_haversine_m is expected NOT to inline and now says so: least() is
non-strict, and STRICT is what stops a NULL coordinate returning half
the Earth's circumference. Asserted with EXPLAIN (VERBOSE).
Scenarios 6a, 10a, 30a and a third leg on 35 pin the four defects; Task
12 gains a non-empty-cover property test and its Go twin of 30a.
Config, Load and Defaults over the T1 layering: envDefault tags, then config/common.yaml, then the optional config/instance.yaml, then the process environment on top so a secret store beats a checked-out path (TRD §8). One env.Parse rather than one per layer: env.Parse applies every envDefault tag whose variable is absent, so a second pass over an already-populated struct resets each field the environment does not name. The YAML layers are folded in as environment entries instead, which keeps the tags as the floor and leaves precedence to map insertion order — and gives the unknown-key refusal a place to live, since the overlay walks each document against the struct. Search carries three separate bounds under names that say what each one bounds: DefaultPageSize and MaxPageSize clamp a page, MaxCandidatesPerMode bounds one retrieval mode's candidate list and is also the reachable pagination depth. Database.MaxConns defaults to 32 from the concurrency model (A2), not from pgxpool's max(4, numCPU), under which the performance scenario's sixteen concurrent discovers would measure the pool queue. validate reports every problem in one boot rather than one per restart.
… below [#9] maps.Copy put every environment entry into the override map, including the present-but-blank ones. env/v11 then reads a blank entry two different ways: with an envDefault tag it returns the tag value, without one it sets "". So `SEARCH_MAX_PAGE_SIZE=""` over `search.maxPageSize: 40` in instance.yaml resolved to 100 — not the file's answer, not the operator's, but the struct tag's, silently. `value: ""` in a pod spec and a blank key arriving through envFrom are the ordinary shapes this arrives in, and instance.yaml is the layer an operator is most likely to have set, so this defeated the precedence the package exists to establish exactly where it was least visible. Skipping blanks makes the rule uniform: a blank variable is not a value at any layer, so the layer below stands. That also settles the five fields with no envDefault (App.Network, Database.URL, Validation.SpecURL, OTel.Endpoint, Replication.Targets), where a blank used to win and set the field empty — one spelling meaning "clear this" for those five and "restore the default" for the other twenty, with nothing on the operator's side to distinguish them. Precedence is a property of the layer, not of whether a field happens to carry a tag. A deployment that wants a value cleared leaves it out of the file, or gets an explicit knob; it does not overload a blank string. The seventh precedence pin covers both halves — a field with a tag and a field without — and fails on the previous commit with the reported values.
Task 2's Groups list was the only place config groups are defined, and it did not name the three knobs the plan states elsewhere as configuration: ERROR_INCLUDE_LEGACY_TYPE (C1, line 84 and Task 5), EXT_ALLOW_NETWORK_FETCH (Task 10's SSRF boundary) and ResolutionCells (the Constants table, marked "config, not a constant"). As it stood, Tasks 5, 10 and 12 each had to reopen config.go and invent a home with nothing in the plan sanctioning it. The doc is amended first so the groups exist in the plan before they exist in the struct. Group names: Errors, Ext and Geo, matching the prefixes the plan already fixed for the two variables it names (ERROR_, EXT_) and reading GEO_RESOLUTION_CELLS for the third, which had no fixed name. Each is its own group rather than a field on an existing one: - Ext.AllowNetworkFetch, not Validation.AllowNetworkFetch. Validation already holds SpecURL, and an AllowNetworkFetch beside it reads as though it gated the L1 spec fetch — which always happens. What it forbids is dereferencing a @context URL out of a request body. Separating the groups removes the misread without inventing a longer name than the variable it maps to. - Errors is the error *body*, not error handling; nothing else in Config shapes a response body, so there was no existing group it belonged in. - Geo will hold whatever else the spatial path makes configurable. The other three entries in the Constants table stay constants. A one-field group is already this file's shape (Log, Auth), so none of these is a new kind of thing. validateGeo rejects a resolution outside H3's 0 through 15. Without it an r16 fails inside the first cover a publish or discover builds, which is a request error for a startup mistake; the boot is the only place that reads as an operator's problem. Neither false is written into common.yaml. Both are struct-tag defaults with the same value, and a second copy is a second place to change; common.yaml carries a value only where the project chose one against a different tag.
…er below [#9] 828abdb made a blank environment variable a no-op. One layer down the opposite still held: a blank value in common.yaml or instance.yaml overwrote whatever the layer below set, and env.Parse then ignored it — so the envDefault tag came back where a field had one and the zero value stood where it did not. `log.level: ""` over a reviewed `warn` produced `info`, and `validation.specURL: ""` cleared it. One operator gesture with two opposite outcomes is the ambiguity the unknown-key refusal exists to prevent, and it was living inside the loader that enforces it. A blank in a file is a deliberate keystroke, so scalar refuses it the way it already refuses a nil value, rather than skipping it as the environment layer does. Nothing needs an empty string from YAML: the two fields where empty is meaningful (Database.URL, App.Network) are required non-empty by validate, and for the rest omitting the key already means "take the layer below" — which is the only spelling the refusal costs. An empty sequence is refused on the same terms. It renders to the same empty string, and its effect today is exactly omitting the key (env skips set on a blank, env.go:507, so Targets stays nil rather than becoming [""]), so it buys one rule instead of two and removes the trap that a slice field with an envDefault would otherwise inherit. Task 2's layering description now states the rule for both halves at once, since after this there is only one rule, and its Tests pin line names the new check.
0e28756 refused an empty sequence on the same terms as a blank string, which closed the last spelling any layer had for clearing a value a lower one set. With common.yaml naming a target, `targets: []` and `targets: ""` in instance.yaml both failed startup and a blank REPLICATION_TARGETS was ignored — so a deployment that replicates to nothing had no way to say so. Replication.Targets is the field where that matters, because empty there is a value rather than an absence: it is how the plan selects the no-op replicator. `key: ""` keeps the refusal. It reads as both "no value" and "the empty string", and the ambiguity is the whole reason a blank is not a value. `[]` has one meaning in YAML and an operator who writes it means it, so the rule is about blanks and an explicit empty collection is not one. Clearing works because env.Parse never sets a blank (env.go:507), leaving the field zero. That holds only where no envDefault comes back in its place, so a slice field must not carry one — asserted over the struct by reflection rather than left as a comment, since the trap would otherwise arrive silently with the first slice field that wants a default.
440bcf0 decided the empty sequence by the value's shape rather than the field's, which let it through on a scalar field: `log.level: []` rendered blank, and a blank on a field with an envDefault comes back as the tag — so it erased a reviewed `warn` and produced `info`, which is the failure 0e28756 closed for `key: ""`, arriving under a different spelling. Nested, `[remote-a, []]` became an empty element inside the slice, junk that reaches the field while the same position spelled `""` was correctly refused. collectField is the only place that knows the field's type, so the decision moves there: a list on a field that is not a slice fails startup whether it is empty or populated — populated, it was being joined into a string the field never asked for — and an empty one is answered there. scalar refuses an empty rendering again, which is now only reachable nested, where empty is never a value. The clearing spelling is unchanged and still pinned: instance.yaml's `targets: []` clears the two targets common.yaml names, and leaves every other key standing.
Each task is driven by a fresh session, which is what keeps the plan honest — a task section that a new reader cannot execute has a gap, and finding that on task 3 is cheaper than on task 19. The cost is that every driver prompt re-types the same standing rules, and a rule that lives only in a prompt is one nobody can review. What goes here is what is not already written down: the working agreement, the commit and issue convention, why make test pins the embedding provider, and the handful of rules that generalised out of task 2 — that a name the plan chose is load-bearing, and that a pin holding only because someone remembered it is not a pin. What does not go here is the Global Constraints table. It is binding and it is in the plan; a second copy would be a second thing to keep true, and by the doc's own Comments rule it is the copy that rots.
Task 3. logger.New builds the production JSON logger, NewContext installs it for a request, With adds fields to what is already there, and FromContext reads it back — returning a no-op logger rather than nil, so no call site has to ask whether the middleware that installs one ran. New takes config.Log, not config.Config. The narrow parameter states what the logger reads, which is the level and nothing else; and Config.Database.URL carries a password, so passing the whole struct would put a secret inside the one component whose job is to write things down. Later tasks inherit the narrow form, which is the point: a component that cannot reach a secret cannot log one. Two decisions the doc does not spell out: Production sampling is off. zap's production sampler keys on level and message only and drops all but the first hundred entries sharing a pair each second. Task 8's RequestLogger writes one completion line per request under a single message, differing only in its fields, so leaving sampling on would discard most of the request log at exactly the load worth reading it at. The four field names Task 3 names — request_id, transaction_id, message_id, action — ship as typed constructors here rather than as string literals at each call site. One key spelled two ways is two fields to whatever queries the logs, and the mistake stays invisible until someone searches for the spelling that is missing. Typed constructors are also what the Logging constraint asks for in place of zap.Any.
e4b2145 stated the "no Sugar() on the request path" rule as an existing .golangci.yml entry. Task 3 is the task that adds it, so a fresh agent could have read that line as licence to skip the very thing the rule around it demands — which is exactly the failure the bullet warns about. It now says what the plan's "asserted by make lint" phrasing is asking for. Also: the storage row was garbled mid-sentence, and tests/ was missing from the layout, which would send a new reader looking for a home that already exists.
Task 3's pin is "no Sugar() on the request path (a lint rule, asserted by make lint)". Avoiding Sugar() by hand is not a pin: it holds only as long as everyone who touches the request path remembers it, and it fails silently the first time someone does not. forbidigo makes both spellings a build failure. Both forms are forbidden because both take any. Sugar()'s loosely typed API and zap.Any are the same hole: the field names stop being greppable, and whatever a value's %v prints goes to the log — which is how a connection string with a password in it reaches a line nobody audited. No carve-out for _test.go, unlike the gosec and function-length exclusions above it. The rule is about the request path, but a helper a test shares with the request path is on the request path, and the cost of the strict form is one nolint with a reason on a startup line that genuinely wants Sugar. Verified by running it against a deliberate violation before and after: the chained form, the stored form, and zap.Any all fire and make lint exits non-zero; 0 issues with the violation removed.
5a46643 forbids `.Sugar` and `zap.Any`, but the same hole is reachable through zap's package-level singletons, and `zap.S().Infow("boot", "dsn", dsn)` passed `make lint` with 0 issues — the connection-string-in-a-log-line failure the rule exists to prevent, arriving by the one door left open. It is the likeliest door, too. You reach for zap.S() exactly when you do not have a logger in hand, which is the position of anyone who was not passed the context one; someone who already holds a *zap.Logger has no reason to sugar it. They are package-level singletons besides, so the No globals constraint forbids them independently of anything about typed fields. Matched on source text rather than resolved types, and the comment now says why: analyze-types would close the aliased-import spelling (import z "go.uber.org/zap"; z.Any) but resolves a chained call to the method at its end, so zap.S().Infow becomes SugaredLogger.Infow and matches nothing. The alias is a spelling nobody reaches for by accident; the chain is the one everybody writes. Verified against a deliberate violation: no forbidigo issue before, both forms reported and exit 1 after, 0 issues on the tree.
Produces is what the next task's implementer reads to learn a signature they cannot see, so a wrong one is worse than a missing one. It said `logger.New(cfg)` where New takes cfg.Log, and omitted NewContext entirely — the function the request middleware in a later task has to call to install the logger at all. Records why New narrows its argument: Database.URL carries a password, and the component whose job is writing things down should not be handed it. Also records that LOG_LEVEL is deliberately not in validate — the level table belongs to zap, and a copy in config is a second one to keep true — so the gap is not closed later with a list that drifts. Tests pin now names what the tests actually hold, including sampling being off, and the lint rule covers zap.S and zap.L alongside Sugar and zap.Any.
sha256 22ec5593dad45f54bbd803c4179cd5c2f0154cc5055bcaf9a23de53f43638932, copied verbatim from the protocol-specifications-v2 repo at api/v2.0.0. This is not the spec the service validates against. beckn.yaml is fetched at boot from VALIDATION_SPEC_URL precisely so a spec bump does not require a rebuild, and baking a second copy into the image would give the running process two sources of truth that drift apart silently. A committed copy under tests/ is a different object with a different job: it is the version this build's wire structs were written against, so the conformance walk can answer "have the structs and the spec parted company" at CI time rather than at the first consumer strict enough to check. Bumping it is meant to be a reviewed diff — that is the whole value of pinning the hash. Committed separately from the code it pins because it is 5k lines of vendored third-party YAML, and mixing it into the same diff as the structs would bury them.
The shapes that cross the network, plus the one place a request body is read.
Three rules decide how a schema property becomes a Go field, and each exists to
keep a later task honest rather than to look tidy here:
- No declared default means a pointer or a json.RawMessage. MERGE is RFC 7396
(A8) and encoding/json is the last thing in the chain that still knows the
difference between a key the publisher omitted and one they set to null.
Flatten it here and the merge cannot tell "keep what is stored" from
"delete it" — which is why TimePeriod's four members are raw rather than
*string: a patch that clears endDate and keeps startDate needs three states
per member, and *string collapses two of them into nil.
- A field stored verbatim stays a json.RawMessage. provider, descriptor and
resourceAttributes land in JSONB and are rendered back unchanged, so parsing
them would only produce a second, lossier copy of a document this package
does not interpret.
- A Go type never narrows the schema. timestamp stays a string so a malformed
one comes back as a CTX_ fault carrying $.context.timestamp, instead of
failing json.Unmarshal and telling the caller only that the body was
unreadable.
Targets resolves beckn.yaml's oneOf once, in UnmarshalJSON, so everything
downstream sees a slice. It refuses every other shape rather than reading it as
empty: an empty targets would drop the spatial predicate and answer with the
whole index, and a silently widened result is the failure the spatial path is
built to avoid on every branch.
ParseEnvelope deliberately does not call DisallowUnknownFields. The spec permits
unknown keys, this service is one hop in a chain it does not own, and a v2.0.x
sender carrying a key this build has never heard of must reach the L1 validator
— which knows which schemas close additionalProperties — rather than being
turned away by a decoder that knows only this build's structs. It does use a
Decoder and More() rather than json.Unmarshal, because Unmarshal accepts a
leading value and ignores whatever follows: two concatenated documents would
parse as the first one, and reading half a request nobody meant to send is
worse than refusing all of it.
OnDiscoverAction is exactly {Catalogs []Catalog} and must stay that way. The
schema is additionalProperties:false with catalogs as its only property, so a
Degraded field would not be an extension but a response that fails its own
schema; omitempty would hide that on the ordinary path and ship the invalid body
on precisely the degraded path that matters. Degradation travels as the
X-Beckn-Degraded header (C11), and the conformance walk now catches the field if
anyone adds it.
The walk compares every wire struct's JSON tags against its schema node in both
directions — an extra tag ships a key the schema forbids, a missing one drops a
publisher's data in transit. Its allowlist holds one entry: Error.type, the C1
legacy error category, written only when ERROR_INCLUDE_LEGACY_TYPE is true.
C4 and C5 are documented conflicts but are not allowlist entries, because the
plan resolved both by following the schema — C4 keeps the JSON-LD pair scalar
against a reference implementation that accepts arrays, C5 drops category
against a PRD that assumed one. Neither produces a property-name mismatch, so
each is pinned by an assertion that actually holds: that Attributes still types
both members as required scalar strings, and that no schema in the document
declares a category property. Entries that match nothing would have been worse
than useless — the walk checks its allowlist in both directions and would have
failed on them.
TestEveryWireStructIsBound parses the package source, because Go cannot
enumerate a package's types at run time, and fails on an exported struct with no
binding. Without it the walk keeps passing while the drift it exists to catch
goes in beside it.
Both parsers claimed to refuse every shape they do not understand, and both
had the same hole: encoding/json hands `null` to an UnmarshalJSON method like
any other value, and unmarshalling it into a string or decoding it into a
struct succeeds as a no-op rather than failing. So `null` was the one input
that got past a guard written to be total, silently.
Targets read `targets: null` as Targets{""} — one empty JSONPath pointer that
no sender wrote. That is precisely the outcome the type exists to prevent: its
whole argument is that a shape the oneOf does not admit must be refused here
rather than left for a downstream component to notice, because a spatial
predicate that quietly resolves to nothing answers with the whole index. The
array arm had the same hole one level down, so it now reads []*string and
refuses a null element: ["$.a", null] is not an array of strings, and []string
renders that null as "" with nothing to show for it. L1 validation would catch
both while VALIDATION_ENABLE_L1_SCHEMA is on, which is exactly the dependency
the type was written not to have.
ParseEnvelope returned a zero envelope and a nil error for a body of `null`.
Every other non-object — an array, a number, a bare string — fails the decode
with a type error; null does not, so the one body that could not be read at all
would have been answered as a context fault over missing fields instead of as
the transport failure the plan reserves a NACK for. The classification is the
point: a caller who sent an unreadable body and a caller who sent a readable
body with a bad context need different answers.
Neither shape satisfies the schema — `targets` is a oneOf over a string and an
array, and an envelope is an object — so refusing them narrows nothing a
conformant sender can send.
Task 5 was written assuming `SPT_` and `DOM_` are code families the spec admits. Checked against the fixture Task 4 pinned, neither is: `ErrorCode` has 76 members across exactly CTX_, NET_, AUT_, SCH_, POL_ and BIZ_. `SPT_` is absent from the document entirely; `DOM_` survives only in the prose of `Error.code`'s own description, with no enum members behind it. Six codes the plan spends are not in the enum. What makes this worth writing down rather than fixing in passing is that nothing fails when it is wrong. `Error.code` is declared `type: string`, not `$ref: ErrorCode`, so the "MUST be a value from the ErrorCode enum" is prose: L1 validation will not reject an invented code, and Task 4's conformance walk compares property names, not values. An agent implementing Task 5 would write the constructors the section names, watch every test go green, and ship codes no conformant consumer can interpret. So the section now asks for the choice to be made — map the six onto existing members, or state that this service treats `code` as open and quote the prose it departs from — and says that the Produces line and the Tests pin move with whichever is chosen. The checklist entry repeats it, because that table is what a fresh session reads before opening the task. Also adds Carried chores for the four plan-design commits missing their [#11] tag. They live outside the task list because they are not blocked on any task and are already on origin, so clearing them needs a force-with-lease that is the human's call — and a chore with no home is a chore that gets rediscovered by the next person to audit the log.
Azure, Google Artifact Registry and Docker Hub were carried as `*_ENABLED` flags reading repo variables. The repo has no variables set — checked — so all three resolved to false and every `secrets.*` behind them to an empty string. The verified release (tag v0.0.1-rc2) pushed to GHCR and nothing else, which is what the switchboard was always going to do. Removed, rather than left off, because an unconfigured registry is not flexibility. It is a fourth way for a release to look green while publishing nothing: turn a flag on without its namespace secret and the path collapses to a bare registry and a slash. Fewer states that can be wrong beats more states nobody has exercised. Adding one back is one entry in IMAGE_REPOS and one login step. GHCR is now unconditional rather than a flag defaulting true, so IMAGE_REPOS cannot be empty and require-image-repos has one thing left to check: OWNER, which CI always sets and a laptop never does. That is now what it says. id-token: write goes with them — it existed only so google-github-actions/auth could exchange an OIDC token for Google credentials. Nothing left in either job requests one. Not touched: the dhi.io login. It reads DOCKERHUB_USERNAME/DOCKERHUB_TOKEN, but those are the Docker Hardened Images subscription the Dockerfile's base images need to pull at all, not a Docker Hub publish target. Deleting it would break every image build. ci-release.yml: 214 lines to 133.
main moves solely by merging a PR, and GitHub verifies that PR at its merge
commit — the exact tree the merge produces. Re-running build, lint, test and
both scans on the push that lands it re-asks a question answered minutes
earlier, on the same content, and posts a fifth check nobody reads.
Dropping the push trigger makes three expressions unreachable, so they go too
rather than sit there implying an event that can no longer arrive:
- BASE_REF's `github.event.before` arm, now just the PR's base ref
- SCAN_IMAGE's `github.event_name != 'pull_request'` arm
- `if: github.event_name == 'pull_request'` on four comment steps
- cancel-in-progress' conditional, which existed only to keep main's runs
from cancelling each other
What this gives up: a merge queue or a direct push to main would land
unverified. Neither is in use — main has no branch protection today — and when
protection is added, requiring the two PR checks is what enforces it.
Two comments explained why the jq program and the trivy cache were extracted by naming the files they were extracted from — security.yml and build-and-push.yml, neither of which exists any more. The reasoning is still worth keeping; the filenames now send a reader grepping for something that is not there. Reworded to describe the shape of the problem instead of the path.
…ed [#9] VERSION came from `git describe --tags`. On a commit carrying more than one tag, describe returns whichever was *created* last, which is not necessarily the one whose push started the run: 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 pushing v1.0.0 would publish ghcr.io/...:v1.0.1-rc1, and because that name contains a hyphen the :latest rule would read it as a pre-release and leave :latest pointing at the previous version. A green release, shipped under the wrong name, with the tag users actually pull never updated. Two tags on one commit is not exotic — promoting an rc to a release does exactly that. ci-release.yml now sets VERSION from github.ref_name, the triggering ref itself, and the Makefile's `?=` lets it win. Nothing is re-derived. The old comment argued *for* describe on the grounds that one answer on a runner and on a workstation is simpler. That was the error: the two contexts do not hold the same information. CI knows the triggering ref exactly; a workstation has no triggering ref at all, so describe stays the local fallback and still renders an untagged commit as v0.0.1-rc3-1-gabc1234 rather than a branch name that would then be pushed as an image tag. Encoded rather than remembered, per the standing rule: require-image-repos now rejects a VERSION that is empty or contains a slash or a space. That is what a non-tag ref reaching a release target looks like — a pull_request would give "15/merge" — so the guard fails loudly instead of pushing a malformed ref.
Both repos post a coverage comment and a Trivy comment, but they disagreed on two things a reader would notice putting the two PRs side by side. Aligning on network-adapter#20, since it is the one already merged. Severity: HIGH,CRITICAL -> CRITICAL,HIGH,MEDIUM,LOW. Measured before changing it — the dependency scan reports the same single finding either way, so this costs nothing today and means a MEDIUM that appears next month is seen here as soon as it is seen there. The image scan cannot be measured locally (dhi.io needs a subscription login), so CI is what will report its wider count. Coverage states: "✅ Passed — not applicable, no changed Go files" was a contradiction. Passed says something was measured and cleared the bar; nothing was measured. There is now a distinct "➖ Not applicable" for both paths that have nothing to measure — no non-test Go files in the diff, and changed files carrying no coverable statements — matching the sibling repo's wording. Still exit 0, so the gate is unchanged; only the claim is honest now.
cover-diff summed the profile per LINE. Under -coverpkg every source block appears once per test binary that was built — measured here, 2294 blocks with 21 copies and 156 with 22 — so the denominator was multiplied by the number of binaries while the numerator was credited only for the binaries that reached the block. The result was not a rounding error. src/publish/controller.go read 7% against a real 81%, src/discover/controller.go 8% against 89%, and an aggregate of 11% where `go tool cover -func` says 88.5%. With MIN_COVERAGE=80 that is a gate that fails EVERY pull request touching a .go file regardless of how well tested it is — which is a gate people learn to bypass, and worse than no gate. It was latent on the branch that introduced it, because that branch changes no Go files and cover-diff exits early on an empty changed set. It would have fired on the first functional PR after the merge. tools/cover-report.awk already documents this exact trap and already handles it: key on the block, keep its statement count once, count it covered if ANY binary hit it. cover-diff is now the same, and its TOTAL agrees with `go tool cover -func` — which is the invariant the new test pins. Moved out of the Makefile recipe into tools/cover-diff.awk for the reason cover-report.awk lives in a file: make eats `$`, so every `$1` has to be written `$$1` and a mis-escaped field reads as the empty string rather than failing. Being a file is also the only way it can be tested, which is the second half of the fix — this is gate logic, and nothing exercised its arithmetic. Writing the test surfaced a second defect: 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 instead of skipping it. Guarded.
v0.2.0 -> v0.3.0, which is the fixed version Trivy names. Reached only through testcontainers, so it is never linked into the shipped binary and the image scan was already clean — but the dependency scan is there precisely to catch what the image scan structurally cannot, and leaving it red trains people to read a red Security Scan as normal. Pulls klauspost/compress 1.18.6 -> 1.18.7 and moby/sys/user 0.4.0 -> 0.4.1 as part of the same module graph resolution. `make trivy-deps` now reports 0 findings at CRITICAL,HIGH,MEDIUM,LOW.
A `${{ }}` expression is substituted into the run block's TEXT before bash
parses it, so the token was part of the program rather than an input to it — a
value carrying a quote or a backtick would be parsed as shell. Through env: it
is only ever data.
No behaviour change for any well-formed token; this is about the one that
isn't.
The test shelled out to `awk -f tools/cover-diff.awk`. awk is a subprocess, so
a file only it opens is invisible to the go test cache: edit the awk, rerun,
and `go test` replays a stale "ok (cached)" because no Go input changed.
Demonstrated — replacing the whole program with `END{print "TOTAL\t0"}` and
rerunning without -count=1 still reported ok (cached). A regression test that
goes quiet exactly when the thing it guards is being edited is worse than none,
because it reads as a pass.
Now the test reads the program with os.ReadFile and runs the copy. The read is
an input the cache records, so the same sabotage fails immediately with no
-count=1 needed. Verified both directions.
chore: collapse CI to four checks and move the logic into the Makefile [#9]
🛡️ 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% |
…/6-discover-service-full-coverage # Conflicts: # src/discover/intent_mapper_test.go # src/discover/service_test.go
The merge from feat/18-network-setup brought in the rule that an intent
naming no retrieval criterion is refused (SCH_INVALID_FORMAT). Two
pre-existing tests here passed a bare beckn.Intent{} to isolate the
offset/page-depth boundary, which the new rule now refuses before the
boundary logic ever runs. Give both a TextSearch so they test only the
thing they name.
…ig assumptions [#6] Review on PR #18 found make test hanging for 10 minutes on TestRunReturnsAListenFailureRatherThanPanicking: it held a loopback-only address while Run binds the wildcard, so the collision it expects never happens and serve blocks on an uncancelled context.Background(). Fixes: - listenLocal now binds the wildcard too, so it holds the exact address Run asks for; TestRunReturnsAListenFailureRatherThanPanicking gets a bounded context so a future miss on this premise fails in seconds instead of hanging the package again. - The renamed-to-match-behaviour Run test (was ...ServesUntilCancelled, which a Port=0 config that's never dialed cannot claim) polls Run's own "listening" log line instead of a fixed sleep, removing the flake risk on a loaded CI box. - container_test.go's buildableConfig hardcodes 17 values with no tie back to config/common.yaml; a new TestBuildableConfigMatchesCommonYAML parses the real file and fails loudly if the subset it commits to (timezone, page/candidate bounds, embeddings provider, L1 schema flag) ever drifts. Loading common.yaml directly via config.Load isn't an option here: it resolves paths relative to the process working directory, which a go test binary run from its package directory does not share with a deployed container. - tests/architecture/boundary_test.go gained a third import-graph ban: only src/storage/postgres, src/app, tests/acceptance and tests/dbtest itself may import tests/dbtest. container_test.go's dbtest.DSN(t) made src/app the first package outside that set to require Docker on a plain `go test`, and nothing marks a test as integration in this repo (no build tag, no naming convention) — this makes the boundary an assertion instead of something only found by reading imports. - The two embedder-selector tests were four/two if-statements pretending to be one test each; split into named t.Run cases so a failure names which provider broke instead of requiring a line-number lookup.
…s tests [#6] Review on PR #18 found the same gen.DBTX-failure wrapper written twice (dbtxFailsAt in catalog_repository_internal_test.go, queryFailsAfter in read_path_test.go — different files, and package postgres vs the external package postgres_test, which is why it drifted into two copies). Exported the former as DBTXFailsAt and deleted the latter; both files now share one wrapper, one ErrBoom, and read_path_test.go drops the pgx and gen imports it no longer needs. Also, all in src/storage/postgres: - t.Run around every bare table loop that drives a real Postgres query (write()'s 11/13-statement ordinals, GetCatalog's four reads, Hydrate's four queries): a t.Fatalf on a rollback failure mid-loop was calling runtime.Goexit on the parent goroutine and silently abandoning the remaining ordinals rather than reporting them unverified. - wantsay -> want across every case table it appeared in; not a word. - seedCatalog rebuilds republishPatch(id) instead of restating an identical domain.CatalogPatch literal that could drift from it. - read_path_test.go's bare 768 (20 sites) is now columnDimensions; the wrongWidth test double's claims field is declaredDimensions, since "claims" alone read as a verb. - migrate_test.go: a new test had been spliced between an existing doc comment and the function it belonged to, leaving both comments describing the wrong test. Re-split them and noted why the failure test dials loopback directly instead of going through skipIfShort. - hydrator_internal_test.go's "Hydrate's own four queries above" pointed at a different file (read_path_test.go) and a different package (postgres_test) — named both explicitly.
…st [#6] TestValidateConstraintReportsEveryFaultNotJustTheFirst checked len(fatal) != 4, which passes for four faults of any kind — not the behaviour the name claims (one per bad field, not just the first). Now checks the actual code and path each of the four faults carries. TestTwoDegradedModesAreBothNamedInTheHeader never built a response or read a header; it asserted on Discover's own return slice. Renamed to TestTwoDegradedModesAreBothReturned and pointed its comment at TestTheDegradedHeaderJoinsMultipleModesWithACommaAndNoSpace (controller_test.go), which is what actually pins the header string and its separator.
publish/text_test.go had the same orphaned-comment mistake fixed in postgres/migrate_test.go earlier in this PR: a new test was spliced between an existing doc comment and the function it documented, leaving both comments attached to the wrong test. Re-split them. src/indexing/geo/h3_test.go's depth-limit test built its nesting with a bare `for range 9`, unconnected to maxCollectionDepth = 8 despite the comment's own claim that "the limit itself is what the test names" — the test would still pass if the constant changed. h3_test.go is package geo_test and can't see the unexported constant directly, so h3_internal_test.go now mirrors it as the exported MaxCollectionDepthForTests, and the test loops geo.MaxCollectionDepthForTests + 1. Also named the two literals in ringsFor(1000, 99) (anyDistanceMeters, pastH3MaxRes15) — they read as unexplained magic numbers otherwise.
…native [#6] TestLoadReadsPathsRelativeToTheProcessWorkingDirectory only checked that Load fails outside a directory with config/common.yaml beneath it — which a Load resolving paths relative to the compiled binary would also do, so it never distinguished the claim in its own name from that alternative. Added the complement: t.Chdir into the repo root (where common.yaml genuinely is) and assert Load succeeds, which fails if resolution is ever changed to be binary-relative.
TestListCatalogResources was the one bare, method-named test in a repo of sentence-style claims, and it checked two unrelated things (an unknown catalog, then a published one) under one name. Split into TestListingResourcesOfAnUnknownCatalogIsErrCatalogNotFound and TestListingResourcesReturnsThePublishedOnes. domain/gate_test.go was package domain_test while its siblings (catalog_test.go, mergepatch_test.go) are package domain, for no reason tied to what it tests — everything it calls (ScopeGate, Resource, PruneOfferReferences, Faults) is exported. Moved it to package domain for consistency, and wrapped TestScopeGateMatchesOnClockBoundsBothPresentBothAbsentAndOneEach's case table in t.Run so a failure is selectable and reports the case name.
…copy [#6] spec_index_test.go held a 19-line, whitespace-exact copy of /discover's requestBody block from tests/testdata/beckn-v2.0.0.yaml, mutated by strings.Replace in two tests. It matched today, but any reindentation of the real fixture would desync the copy without either test naming that as the cause — the failure would surface as a spec-compile error inside the validator instead. discoverPathBlock now slices /discover's own path item out of the live document at test time (from its path key up to the next top-level path key), so both edits are scoped to /discover without a hand-maintained duplicate to keep in sync. Also documented, rather than silently accepted, that TestWriteCacheWrapsACreateTempFailure is a no-op under a root-run CI container: it needs a read-only directory to force CreateTemp's own failure, and permission bits are exactly what root ignores. Unlike its neighbour (a plain file blocking MkdirAll), there's no substitute mechanism for THIS branch that root can't also bypass.
The value 8 existed under six local names across ten sites (resolution, testResolution x2 in different package/package_test splits of the same directory, indexResolution x2, res, and a bare cfg.Geo.ResolutionCells = 8 x3) — review on PR #18 found this PR added two more of them into directories that already had the constant under a different name, which is exactly how a repo-wide fixture value drifts without anyone deciding to change it. geo.DefaultTestResolution (src/indexing/geo/testsupport.go, a new file so it stays visibly separate from the package's real exported API) is now the one source every site references — including the ones this PR did not touch, since they turned out to share the same problem the review named one instance of. It is explicitly documented as test-only: production takes its value from config.Geo.ResolutionCells (GEO_RESOLUTION_CELLS), which happens to default to the same 8 for no reason either side depends on.
…just claim it [#6] Verified rather than asserted: writeCache runs MkdirAll(directory, ...) and CreateTemp(directory, ...) against the same directory value, so the reviewer's suggested alternative (block a path component with a plain file, which fails ENOTDIR) trips MkdirAll first and never reaches CreateTemp — it would just be TestWriteCacheWrapsAMkdirFailure again. Confirmed with a standalone MkdirAll probe against a blocking file before writing this down.
manjudr
marked this pull request as ready for review
September 8, 2026 12:38
This comment has been minimized.
This comment has been minimized.
The Trivy install script was fetched from aquasecurity/trivy's main branch while only the binary version was pinned, so a compromised or rewritten install.sh on main would run at build time with no version gate — the exact supply-chain risk TRIVY_VERSION exists to close. Fetching install.sh from the TRIVY_VERSION tag ref instead pins the whole install path, not just the binary it downloads. image-push and image-publish loop over IMAGE_REPOS/RELEASE_ARCHES with plain `for ... do ... done` recipes. Without set -e, a failed docker push or imagetools create on one iteration doesn't stop the loop and doesn't fail the recipe unless the *last* command also fails — so a partial, silent publish was possible. Adding `set -e` makes the first failure abort the recipe immediately.
Member
Author
|
Both issues fixed in 931a603 on
|
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.