feat(registry): index property feed events#2326
Conversation
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
0145c5d to
c030b64
Compare
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
Request changes. The property index bootstraps from an endpoint that returns publishers, not properties — so every seeded record is a publisher wearing a fabricated property_rid, and the SDK is a witness, not a translator.
Blocking
1. Bootstrap fabricates property_rid and indexes publisher aggregates as properties. bootstrapProperties() seeds from client.listProperties() → /api/properties/registry, which returns PropertyRegistryItem (src/lib/registry/types.generated.ts:2210): { domain, source, property_count, agent_count, verified }. No property_rid, no rid, no id. So propertyFromRegistryItem (sync.ts ~L1226) hits its final fallback on 100% of bootstrapped rows and mints property_rid = propertyBootstrapRid(domain) = the bare lowercased domain (~L1287). PropertyRegistry.getProperty() / getPropertiesForDomain() then hand callers a record whose property_rid is a value the registry never issued, carrying publisher-aggregate fields (property_count, agent_count) mislabeled as a property. That's fabrication at a seam — re-shaping a publisher aggregate into a property record is the same bug as injecting mock data. Second-order: two distinct properties on one domain collapse to one synthetic key, so setPropertyEntry silently overwrites — data loss. propertyRidFromPayload (~L1272) has the same failure mode on property.* events that omit property_rid and fall back to entity_id (the publisher-domain routing key). Either skip rid-less rows/events, or store bootstrap-derived rows behind a distinct surface that property_rid consumers can't mistake for a registry RID. If /api/properties/registry is genuinely aggregate-only, the bootstrap should build a publisher_domain→count view — not synthesize properties. ad-tech-protocol-expert: unsound on this path, cited spec divergence.
2. bootstrapProperties crashes when the response omits stats. totalPropertiesFromStats(response.stats) dereferences stats.total. listProperties is an unvalidated this.get(...); if the wire omits stats, this throws TypeError and aborts the entire bootstrap. It's on the first-full-page path (properties.length === limit), i.e. every registry with >200 properties — the normal case. Fix: this.totalPropertiesFromStats(this.asRecord(response.stats)). code-reviewer: High.
Things I checked
- Changeset present (
.changeset/registry-property-sync.md),minor, matches the impact — new exports (RegistrySyncProperty,ListBrandsResponse,ValidateAdagentsResponse, ...) and new methods (getProperty,getPropertiesForDomain), all additive. Correct type. types.generated.tswas regenerated via the matchingscripts/generate-registry-types.tstemplate edit, not hand-edited — generator and output in sync.format_ids?: unknown[]→string[]andAuthorizationEntry=AuthorizationEventPayload & { effective_to? }are both real narrowings.- Event taxonomy is spec-correct:
authorization.modifiedandproperty.mergedwith{alias_rid, canonical_rid}are genuineRegistryFeedEventmembers;collection.*/publisher.*no-op through theignoredEventhook is defensible (not yet consumed). resolvePropertyRidalias-cycle handling is safe (seenset);handlePropertyMergeguardsaliasRid === canonicalRid.cloneJsonValueis correct for JSON data and defends internal state from caller mutation.- The runtime guards (
isAuthorizationPayload,isAgentInventoryProfile,isAgentCompliance) replacing the oldpayload as unknown as Tcasts are the right direction — fail-closed on malformed payloads beats trusting a cast.
Follow-ups (non-blocking — file as issues)
- Fabricated authorization
id.authorizationRowIdFromEntityId(~L1121) derives anidfromevent.entity_idwhen the payload lacks one, then stores and surfaces it via the authorization getters — callers see an id the registry never sent. It also drives dedup: a compositeentity_idthat doesn't contain bothagent_urland domain substrings gets misclassified as a real row id. - O(total-authorizations) rescan per auth event.
removeAuthorizationEntriesiterates and rebuilds everyauthByDomain/authByAgentbucket on eachauthorization.*event, versus the old targeted domain+agent lookup. Throughput regression on a busy feed; the grant/modify path knowsagent_url+publisher_domainand could target those buckets except in the id-only revoke case. - Stale placeholder on domain-less real events.
upsertPropertyEvent(~L1158) only deletes the domain-keyed bootstrap placeholder whennext.publisher_domainis set; a changed-fields-onlyproperty.updatedcarrying the real RID but no domain leaves the placeholder indexed, sogetPropertiesForDomainreturns a duplicate. - No max-iteration guard in the pagination loop (~L544): a server that always returns full pages with no
stats.totalloops unbounded. effective_tois never mapped toeffective_until— a legacy deployment sending onlyeffective_toleaves the spec field blank for consumers.
Minor nits (non-blocking)
isAuthorizationPayloadaccepts empty strings.typeof '' === 'string'passes foragent_url/publisher_domain(~L1430); the old granted guard rejected them. Could bucket an entry under an empty domain key.
Fix the two blockers — the fabricated bootstrap RID especially, since it's the primary surface this PR ships — and I'll re-review. The guard-hardening and merge/alias machinery are the right shape.
There was a problem hiding this comment.
One blocking issue: the authorization-revoke rework introduces a fail-open regression. Everything else — the property-feed indexer, alias resolution, witness-not-translator discipline — is clean and well-tested. Fix the revoke matching and this ships.
Blocking
authorizationMatches id-only match silently drops a revoke — fail-open regression. src/lib/registry/sync.ts (new authorizationMatches, cited by both code-reviewer and security-reviewer at the diff's L610 / reported L1142):
if (target.id && options?.matchByRowId) return entry.id === target.id;
Pre-PR, authorization.revoked matched by agent_url + publisher_domain + (optional) authorization_type and ignored row id. This PR replaces that with id-only equality whenever the revoke carries an id. Concrete sequence:
authorization.grantedarrives withoutid(the grant path never requires one; schema marksidas "present when backed by a materialized effective authorization row" — so non-materialized grants legitimately lack it). Entry stored;authLocationsByIdnot populated.authorization.revokedlater arrives withid(plus theagent_url+publisher_domainthe spec guarantees).matchByRowId = Boolean(entry.id || !entry.authorization_type)→true.removeAuthorizationEntrieslooks upauthLocationsById.get(id)→ miss. It scans the target's own agent/domain buckets, which do contain the no-id grant — butauthorizationMatchesreturnsundefined === 'auth-row-1'→false. Not removed.
Result: isAuthorized(agent, domain) returns true for a revoked authorization. The class JSDoc sanctions isAuthorized() for enforcement (gated on state/lag), so this is a fail-open decision for the consumer. The old tuple match would have purged it — this is a regression, not a pre-existing gap. security-reviewer flags the same root cause producing a duplicate/ghost entry on authorization.modified (grant-no-id + modify-with-id appends a second row). Both resolve with one change: when target.id && matchByRowId finds no id match, fall back to the agent_url + publisher_domain (+ type) tuple comparison instead of returning early. Add a test for grant-without-id → revoke-with-id (the existing revoke test at test/lib/registry-sync.test.js:611 only covers no-id/no-id).
fail-closed beats fail-open.
Things I checked
- Property indexer holds witness-not-translator.
propertyFromPayloadcopies wire fields verbatim throughcloneJsonValueand invents nothing; thedoes not synthesize a domain-keyed placeholderanddoes not bootstrap aggregate property registry rowstests confirm properties are populated only from feed events, never coerced from the/api/properties/registryaggregate.ad-tech-protocol-expert: sound-with-caveats — event families (property.*,authorization.*) and payload fields match the registry OpenAPI exactly. - Alias-cycle guard is correct.
resolvePropertyRid'sseenset breaks A→B→A; verified against the merge tests. - Deep-clone isolation works.
property.created indexes...mutates a returned object and re-reads to prove the index is unshared. Right shape. authorization_typeoptional is more spec-correct, not less —AuthorizationEventPayload(types.generated.ts:3437) documents revoked events omit it. The old required-field type was lying.- Changeset present (
.changeset/registry-property-sync.md,minor) — correct category for new methods + new exports + new event. format_ids: string[]narrowing is spec-accurate on the event path (AgentEventPayload.format_idsisstring[]).
Follow-ups (non-blocking — file as issues)
- Un-normalized auth index keys.
authByDomain/authByAgentkey on rawpublisher_domain/agent_urlwhile every other index routes throughnormalizeDomainKey(sync.ts:1121). A grant onExample.com+ revoke onexample.comland in different buckets and the revoke no-ops — same fail-open class as the blocker. Schema even shipsagent_url_canonicalfor this reason. (security-reviewer, LOW/MEDIUM.) agent.removedbypassesremoveAuthorizationEntries. Pre-existing, but this PR's newauthLocationsByIdcompounds it:agent.removedcallsauthByAgent.delete(id)directly, leavingauthByDomainrows (staleisAuthorized) and now leakingauthLocationsByIdentries. Route it through the new remover. (code-reviewer.)isAgentComplianceenum drift. Hardcodesstatus ∈ {passing,degraded,failing,unknown}but the schema documentsopted_out(types.generated.ts:1204). A conformingcompliance_summarywithopted_outis silently dropped from the index whilecompliance_changedstill emits — index and event disagree. Route guard-rejected-but-valid payloads throughignoreEvent()so the drop is observable, or derive the set from the schema.effective_to → effective_untilis the one translator behavior in the diff.normalizeAuthorizationEntrysynthesizeseffective_untilfrom a legacy alias that isn't in the HEAD schema. Non-destructive (both preserved), but confirm a real deployment emitseffective_to— otherwise it's dead code dressed as a compat shim. (ad-tech-protocol-expert+code-reviewer.)
Minor nits (non-blocking)
- Changeset text understates the narrowings.
format_ids: unknown[] → string[]and the tightenedlistBrands/getBrandJson/validateAdagentsreturn types are compile-visible to TS consumers. One line in the changeset noting the tightening would be honest.
Fix the revoke matching and re-request review.
c4cf714 to
c3681ee
Compare
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
One blocking fail-open in the authorization revoke path, plus a changeset that understates the wire impact. The property index itself is the right shape — RID-keyed, alias-resolved, feed-authoritative — and correctly refuses to fabricate RIDs from the aggregate /api/properties/registry rollup.
MUST FIX
1. Revoke fails to purge a canonicalized grant — fail-open on the authorization witness. src/lib/registry/sync.ts, authorizationMatches (authAgentKey(entry) !== authAgentKey(target)), reached from case 'authorization.revoked'.
The schema is decisive (types.generated.ts:3432-3437): agent_url_canonical is the "Registry-canonicalized form of agent_url for equality checks," and authorization.revoked "carries only agent_url + publisher_domain." So:
authorization.grantedarrives withagent_url_canonical.upsertAuthorizationEntrykeys the entry byauthAgentKey(entry), which prefersagent_url_canonical.authorization.revokedarrives with noagent_url_canonicaland noid. The tuple match computesauthAgentKey(target) = authAgentKeyFromUrl(agent_url)— scheme+authority lowercased, path preserved.- The registry's canonical form exists precisely because it differs from naive normalization (trailing slash, default port, path resolution — otherwise the field is dead weight). So
authAgentKey(storedEntry)(canonical)!== authAgentKey(revokeTarget)(naive),authorizationMatchesreturnsfalse, and the granted row is never removed.
What breaks for adopters: on any registry that populates agent_url_canonical on grants, a de-authorized agent stays live in the index — getAuthorizationsForDomain / getAuthorizationsForAgent keep listing it and isAuthorized keeps returning true. That is fail-open on the exact path whose own docstring warns against fail-open. security-reviewer: High, confirmed against the schema contract. The new revoke tests all use payloads with matching keys, so they miss it.
Fix (any one): index grants under both authAgentKey(entry) and authAgentKeyFromUrl(entry.agent_url); or in authorizationMatches, when the target has no canonical, match on entry.agent_url_canonical OR authAgentKeyFromUrl(entry.agent_url). Add a regression test: grant with agent_url_canonical differing from agent_url, revoke with only agent_url + publisher_domain, assert the domain bucket is empty.
2. Changeset is minor but the diff ships TS-breaking public type narrowing. .changeset/registry-property-sync.md:2. AuthorizationEntry.authorization_type goes required→optional (the hand-written type had authorization_type: string; it now flows from AuthorizationEventPayload where it's ?), and three public return types narrow from Record<string, unknown>: listBrands → ListBrandsResponse, getBrandJson → GetBrandJsonResponse | null, validateAdagents → ValidateAdagentsResponse (src/lib/registry/index.ts:492,504,1009), plus format_ids unknown[] → string[]. Consumers doing entry.authorization_type.x or reading an off-type key off the old Record return fail tsc after this ships. Per the repo's own semver table that's major. Bump it, or justify in the PR body why these are treated as non-breaking. code-reviewer flagged the same.
Things I checked
- Property index is sound.
resolvePropertyRidhas a cycle guard;handlePropertyMergededups the alias out of its domain set and re-points via the alias map;setPropertyEntryremoves the RID from the prior domain on a domain change; returns are deep-cloned. No stale-entry or double-listing path. - No aggregate fabrication. Bootstrap does not ingest
/api/properties/registryrollups (test assertspropertiesCallCount === 0) — those rows carry noproperty_rid, so indexing them would mean synthesizing RIDs. Correct call.ad-tech-protocol-expert: sound. property.mergedalias→canonical matches the wire contract (alias_ridmerged away,canonical_ridsurvives). Dropping the alias row and re-keying is the right realization.agent.removednow purgesauthByDomaintoo, fixing a prior leak whereauthByAgent.delete()left domain-side authorizations stranded. Good catch.effective_to→effective_untilpreserves the original (normalizeAuthorizationEntry) rather than rewriting it — documented legacy alias, not fabrication.
Follow-ups (non-blocking — file as issues)
isAgentInventoryProfilegates the whole profile onformat_ids. A single non-string element informat_idsdiscards the entireinventory_profileupdate — existing agents retain stale inventory, new agents getemptyInventoryProfile(). That's silent lossiness against "return exactly what agents provide." Don't let one non-critical field nuke the object.isAgentCompliancedropscompliance_summaryon any out-of-enumstatus/lifecycle_stage. When the registry adds a new status, the summary is discarded (stale retained) even thoughcompliance_changedstill fires with the raw value — an inconsistent, silently-lossy state.isAuthorizedlookup uses weaker canonicalization than the index key. It probes withauthAgentKeyFromUrlwhile entries are stored under registry canonical — fail-closed, but worth a doc note on the public method. (Same root cause as MUST FIX #1, from the read side.)- Properties are feed-tail-only — a property created before the retention window that never re-emits won't be indexed. Consistent with the existing authorization design; document it alongside the staleness note.
Minor nits (non-blocking)
propertyDomainFromPayloadreaches into the opaquepropertyblob forpublisher_domain. Defensible for index keying, but it trusts a field inside a shape the spec doesn't guarantee. Worth a comment noting the fallback is best-effort.
Fix the revoke matching and re-cut the changeset. The index design and merge handling are the right shape — this is the auth path, not the property work, that needs another pass.
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
Blocking on one thing: the reworked authorization indexing has a revocation false-allow on the enforcement path. RegistrySync.isAuthorized() is documented as the gate for authorization decisions, so a revoked agent that keeps reading as authorized is a correctness bug, not a trust-model caveat. Everything else — the property-feed indexing, the witness discipline, the event-name coverage — is right.
MUST FIX
Revoked authorization survives because grant and revoke normalize the agent key differently. src/lib/registry/sync.ts, authAgentKey / authorizationMatches / the authorization.revoked handler.
The schema is explicit at schemas/registry/registry.yaml:3005: "Present on authorization.granted; authorization.revoked carries only agent_url + publisher_domain." So a revoke carries no id, no authorization_type, and no agent_url_canonical. Grants carry agent_url_canonical "for equality checks" (registry.yaml:2999) — a form whose entire purpose is to differ from the raw agent_url.
Trace it:
- Grant
agent_url: https://ads.example.com/agent-card,agent_url_canonical: https://ads.example.com→ bucketed underauthAgentKey(entry)=https://ads.example.com. - Revoke arrives with
agent_url: https://ads.example.com/agent-cardonly.authorization.revokedcomputesmatchByRowId: Boolean(entry.id || !entry.authorization_type)=true, but with noidthe row-id branch is skipped and it falls to the tuple match. The domain bucket still holds the grant, soauthorizationMatches(grantEntry, revokeTarget)runs and comparesauthAgentKey(entry)(https://ads.example.com, canonical) againstauthAgentKey(target)=authAgentKeyFromUrl(agent_url)(https://ads.example.com/agent-card, path preserved). They differ → returns false → the entry is removed from neither index. isAuthorized('https://ads.example.com', 'example.com')still returnstrue.
One sentence for adopters: a benign publisher revocation never lands, so isAuthorized() and getAuthorizationsForDomain() keep reporting a de-authorized agent as authorized until the next full re-bootstrap. This is a regression — pre-PR revoke matched on raw e.agent_url === agentUrl, symmetric with the grant because both event families always carry agent_url.
security-reviewer: HIGH, revocation-survival false-allow. code-reviewer: same finding as an issue-to-fix. Both converged independently.
Fix direction: key and match on a form every event family carries. Make authAgentKeyFromUrl implement the registry's own canonicalization (lowercase scheme+host, strip default port, normalize dot-segments) and have authAgentKey derive from agent_url — keeping agent_url_canonical as stored metadata / tie-breaker applied symmetrically on the query side. That closes the false-allow and the matching query-side false-deny (below) in one move. Add a test where the revoke carries only agent_url + publisher_domain and the grant's canonical strips a path — the current 'authorization indexes use canonical agent urls' test feeds the revoke a canonical the wire never sends, so it verifies a shape that can't occur.
Things I checked
- Property indexing is a faithful witness.
propertyFromPayloadcopies payload keys verbatim throughcloneJsonValue; the only derived value isproperty_ridresolving to the canonical RID afterproperty.merged, which is legitimate alias resolution of a real registry value. Confirmed the SDK does not seed properties from the aggregate/api/properties/registryendpoint — the test attest/lib/registry-sync.test.js:1207assertspropertiesCallCount === 0. Aggregate rows are publisher-shaped, not RID records; treating them as feed properties would fabricate RIDs. Right call. resolvePropertyRidbounds the alias chain with aseenset — no infinite loop on a cyclicA→B, B→Amerge pair.setPropertyEntryre-keyspropertyRidsByDomainwhen a property changespublisher_domain, and prunes the empty domain set. Correct.- Event-name coverage verified against
schemas/registry/registry.yaml:property.created/updated/reactivated/stale/merged,authorization.modified, and thecollection.*/publisher.*families routed toignoreEvent()are all real enum members. No invented types.alias_rid/canonical_rid/property_ridmatchPropertyEventPayload. - Changeset present (
.changeset/registry-property-sync.md,minor) — correct for additive API surface (RegistrySyncProperty,ListBrandsResponse,GetBrandJsonResponse,ValidateAdagentsResponse, the property getters).types.generated.tsis a genuine regen (timestamp +format_ids: string[]), not a hand edit.
Follow-ups (non-blocking — file as issues)
effective_tois not in the spec.ad-tech-protocol-expert:effective_tohas zero occurrences inschemas/;normalizeAuthorizationEntrysynthesizes the canonicaleffective_untilfrom it.AuthorizationEventPayloadisadditionalProperties: {}so a deployment could emit it, but the "legacy alias still returned by some registry deployments" comment is an unverified claim. Either cite the deployment/changelog that shippedeffective_to, or drop the& { effective_to? }extension and the normalization — right now it's speculative code writing a canonical field's value.- Query-side false-deny (Medium). Same root cause:
isAuthorized/getAuthorizationsForAgentderive the key withauthAgentKeyFromUrl(canonical-blind) while entries are stored underagent_url_canonical, so a caller passing the agent's advertised URL misses a live authorization. Fails closed, so not a bypass — the MUST-FIX resolution covers it. - No property delete path.
property.staleis treated as an upsert and there's noproperty.removed/deletedcase, so stale properties and everypropertyAliasesByRidentry accumulate untilreset(). Bounded by registry record count, consistent with the class-doc replica caveat — worth a follow-up ifproperty.staleshould evict.
Minor nits (non-blocking)
authAgentKeyFromUrlfolds userinfo case and leaves scheme-less URLs unnormalized.https://User:Pass@hostlowercases the credential; a URL with no://is returned verbatim so a mixed-case bare host won't unify. No impact for well-formed absolute agent origins — worth a comment.- Dead test mocks. The
/api/properties/registryshims attest/lib/registry-feed-stream.test.js:176,755and the 404-fallback intest/lib/registry-sync.test.jsare leftovers from the earlier bootstrap approach; never exercised now.
Fix the revocation asymmetry and add the schema-accurate revoke test, and this is ready.
There was a problem hiding this comment.
Right shape overall — the property index is a faithful witness (only feed-supplied property_rids land, aggregate-list bootstrap was correctly dropped) and the authorization refactor is well-tested. Two findings on the authorization path keep me from a clean approve; neither is a hard block, both want your explicit call.
Things I checked
- Witness invariant holds.
ad-tech-protocol-expert: sound.upsertPropertyEvent/propertyRidFromPayload(sync.ts:1268-1364) bail tonullunless the payload carries a realproperty_rid/rid/id; property handlers takepayloadonly, neverevent.entity_id, so the deleted domain→RID synthesis can't sneak back in.propertyDomainFromPayloadreadspublisher_domainonly from the payload. No fabricated fields, no placeholder substitution. - Event-type coverage is spec-exact.
property.created/updated/reactivated/stale/merged,authorization.granted/modified/revoked, and the new explicitignoreEventbranches forcollection.*/publisher.adagents_*all match the generatedCatalogEventunion (types.generated.ts:3160-3313).authorization.modifiedis a real event type. Type-less revoke → tuple removal; id-bearing revoke → row-id removal, consistent with the schema's "revoked carries only agent_url + publisher_domain" note. - Changeset is
majorand correct.format_ids?: unknown[]→string[]and theAuthorizationEntryretype are compile-breaking for TS consumers;majoris the right bump. Generated-file edit tracks thescripts/generate-registry-types.tstransform — regenerated output, not a hand-edit. - Property alias/merge is cycle-safe.
resolvePropertyRidhas aseenguard,handlePropertyMergeearly-returns onaliasRid === canonicalRid, andpropertyRidsByDomainonly ever stores canonical RIDs. Domain reindex onpublisher_domainchange is handled insetPropertyEntry.
Open questions (what would flip me to approve)
1. Agent-URL identity is host-level, and path is never load-bearing — even when a path-bearing agent_url_canonical is present. authAgentKeysForUrl (sync.ts:1234) always adds both the full URL and the origin-only key from authAgentKeyFromUrl (${protocol}//${host}, path stripped, sync.ts:1249-1273), and authorizationAgentMatches succeeds on any set overlap. So isAuthorized('https://ads.streamhaus.example.com/agent-card', 'nytimes.com') returns true off a grant for the bare origin — which your own test asserts (registry-sync.test.js, the /agent-card and :443/other/../agent-card cases). security-reviewer: Medium. The prior code was exact-string; this is a strict widening in the security-sensitive direction. Concrete failure: a shared multi-tenant agent host with path-based tenancy (https://sales.saas.com/tenant-a/mcp vs .../tenant-b/mcp) — an adopter gating on isAuthorized treats tenant-b as authorized off tenant-a's grant.
I read this as intentional (the test is deliberate, and same-agent-referenced-by-agent-card-path is a real interop need). Graded Medium because it needs both path-based multi-tenant hosting and an adopter gating on an advisory, staleness-warned helper. Two things would flip me: (a) confirm origin-level agent identity is the intended contract, and (b) either honor a path-bearing agent_url_canonical verbatim as the primary key (only collapse to origin when neither side carries a path), or add one line to the isAuthorized docstring stating identity is origin-scoped so path-based tenancy is not distinguished. Right now the code silently overrides the registry's stated equality key.
2. Idless-then-id grant of the same authorization double-counts and can survive a revoke. code-reviewer, Issue (not Blocker): the grant path calls upsertAuthorizationEntry with no fallbackToTupleOnMissingRowId, so an id-bearing grant dedups by row-id only. Sequence — authorization.granted{agent,pub,type} (no id) → bucket [A]; then the materialized authorization.granted{id:'row-1',agent,pub,type} → authorizationMatches(A,B,{matchByRowId:true}) compares undefined === 'row-1' → false, so A survives. Result: getStats().authorizations counts 2, getAuthorizationsForDomain returns the dupe, and a later authorization.revoked{id:'row-1'} removes only B — phantom A keeps isAuthorized true after revocation. This is a realistic ordering given the schema's own framing (id? present only once the grant is backed by a materialized row). Fix is one flag: pass fallbackToTupleOnMissingRowId: true on the grant path too, or tuple-dedup on grant in addition to row-id.
Follow-ups (non-blocking)
- LLM-safety doc on property getters.
getProperty/getPropertiesForDomainreturn registry-influenced strings (reason,evidence,classification,identifiers) through the open[key: string]: unknownshape.RegistryClient.getBrandJsonalready carries a "sanitize before LLM prompts" caution; mirror it on the property surface. __proto__in computed assignment.propertyFromPayload/cloneJsonValuedoobj[key] = ...overObject.entriesof aJSON.parsed payload. Impact is confined to the per-property object (not global prototype pollution), but skipping__proto__/constructoror usingObject.create(null)is cheap insurance.cloneJsonValueassumes acyclic input (sync.ts:1560) — safe for JSON feed payloads; a one-line comment noting the assumption is enough.
Findings 1 and 2 both sit on the authorization path and both were flagged independently by the security and code reviewers, so I'd rather see them consciously resolved than merged on assumption. Confirm the origin-identity contract and address the double-count, and this is an approve.
There was a problem hiding this comment.
Clean, well-tested addition — property feed indexing, an onIgnoredEvent hook, and the authorization rework all land as replica-of-truth code. The design principle holds: properties arrive only from property.* feed events (no aggregate bootstrap), payload fields are copied verbatim, and returned objects are cloned. Witness, not translator.
Things I checked
- No property bootstrap.
bootstrap()(sync.ts:425-449) paginatessearchAgentsand drains the feed only — properties are seeded exclusively fromproperty.*events. Confirmed by the "does not bootstrap aggregate property registry rows" test, which assertspropertiesCallCount === 0. Right shape: no synthetic domain-keyed stubs standing in for realproperty_ridrecords. - Changeset present and correctly typed.
.changeset/registry-property-sync.mdismajor— justified by the public response-type tightening (listBrands→ListBrandsResponse,getBrandJson,validateAdagents,format_ids: unknown[]→string[], and theAuthorizationEntryreshape onto the generatedAuthorizationEventPayload). Type impact matches the bump. getStats().authorizationsis still accurate. It sumsauthByDomain(sync.ts:395-396), where each entry is pushed exactly once. The new multi-key duplication lives inauthByAgent, which the counter never touches. No double-count.effective_to→effective_untilmapping is real and tested.normalizeAuthorizationEntry(diff L643-646) copies the legacy alias intoeffective_until; the "maps legacy effective_to to effective_until" test asserts both fields resolve. (ad-tech-protocol-expertclaimed this mapping "is a comment, not behavior" — it reviewed a stale read; the mapping is on the wire.)- Property merge is cycle-safe.
resolvePropertyRidfollows the alias chain with aseenguard (diff L911-919);handlePropertyMergeresolves both sides before aliasing and bails whenaliasRid === canonicalRid(diff L816-842). Domain reindexing on merge/update is covered by the alias-cleanup tests. - Event vocabulary matches the registry schema.
property.created/updated/reactivated/stale/merged,collection.*,publisher.adagents_*, andauthorization.granted/revoked/modifiedall matchtypes.generated.ts. Post-PR nothing is silently dropped — thedefaultcase and the named ignore cases route throughignoreEvent, which fires the callback and emitsignoredEvent. Real improvement over the prior silent no-op.
Open question (non-blocking — flagging, not holding)
Agent identity in isAuthorized collapses to protocol//host. authAgentKeyFromUrl (sync.ts:781-803) drops path/query/fragment, and authAgentKeys puts that host key into the match set alongside the registry-supplied agent_url_canonical. So https://host/tenant-a and https://host/tenant-b share the key https://host and cross-match — security-reviewer walked a concrete multi-tenant-single-host confusion where an unauthorized co-hosted agent reads as authorized (Medium; not exploitable in the common one-host-per-agent shape). The host key also broadens revocation, since revokes omit authorization_type and match-all on the tuple.
This is intentional and tested (the .../agents/../agent-card case), and isAuthorized self-documents as advisory — "For decisions where staleness is unsafe (e.g. authorization enforcement)... rather than trusting a lookup blindly" (sync.ts:146-151) — which is why it isn't a block. But the collapse overrides the finer identity the registry hands you in agent_url_canonical. Worth a decision: when agent_url_canonical is present, is host-level matching still what you want, or should the canonical value win and the fallback preserve the full path? Not blocking; file as a follow-up if the host model is deliberate.
Follow-ups (non-blocking — file as issues)
- Authorization getters return live internal references while properties/brand now clone.
getAuthorizationsForDomain/getAuthorizationsForAgent(sync.ts:377-391 in diff) hand back the internal arrays and entry objects;getProperty/getPropertiesForDomain/getBrandHierarchyall clone. The PR's own selling point is caller-mutation safety — the security-sensitive surface is the one spot left exposing references. Pre-existing, but this PR makes the inconsistency conspicuous. Cloning here would corrupt-proofgetStats().authorizationstoo. agent_url_canonicalis unvalidated.isAuthorizationPayload(diff L972-988) validates every field exceptagent_url_canonical, whichauthAgentKeysthen.trim()s (diff L760). A non-string value from a self-hosted/older feed throws insideapplyEventbefore the cursor advances — a hot re-fetch loop. Add thestringValuecheck alongside the others.- Fallback URL normalization keeps userinfo. When
new URL()throws, the fallback (sync.ts:792-802) slices authority on/ ? #only — it doesn't cut@, souser:pass@hostkeys differently from the parsed branch. Fail-closed (under-matches), but inconsistent with the primary path. property.staleis upserted as a live record. A stale property stays returned bygetProperty/getPropertiesForDomainwith no status surfaced through the lookup API; callers must inspect rawreason/last_resolved_at. Fine for v1 (fields are preserved verbatim), but worth deciding whether stale should flag or filter.
One aside: an expert pass flagged a listProperties/PropertyRegistryItem bootstrap and "dead RID fallbacks" — none of that is in this diff. Reviewing against a branch that isn't the PR is its own kind of drift. The code here is cleaner than that read suggested.
Approving on the strength of the feed-only property sourcing plus the cycle-safe merge and the dedicated auth-rework test coverage. Follow-ups noted above.
Summary
Validation
Closes #2325