feat(registry): allow property identity facts on save#2317
Conversation
There was a problem hiding this comment.
Right direction, and the authorized_agents: [] hardening is the honest fix — the generated schema (src/lib/registry/types.generated.ts:4014-4016) documents that field as dropped server-side, so forcing [] at the client boundary makes the SDK match the wire instead of pretending a caller-supplied value sticks. Holding at comment on one wire-coherence question I can't resolve from the diff.
Things I checked
authorized_agents: []forcing is correct.RegistryClient.saveProperty(src/lib/registry/index.ts:614) moves fromproperty.authorized_agents ?? []to unconditional[]. The generatedsavePropertyschema documents the field as ignored ("any value here is dropped and the stored document carries authorized_agents:[]"). Client now matches server. Tests assert it on both the direct and CLI paths.- Widened
SavePropertyRequestcompiles.Omit<RegistrySavePropertyRequest, 'authorized_agents' | 'properties'> & { authorized_agents?: []; properties?: SavePropertyIdentity[] }is clean; the discriminatedSavePropertyIdentityunion forces at least one ofproperty_type/typeplusname.PropertyRegistryItemintersection is safe — the generated type (types.generated.ts:2100-2108) has nopropertiesfield, so nonevercollision. - CLI guard is sound.
bin/adcp-registry.jsrejects a second positional that doesn't start with{/@with exit 2 and a named message.parsePayloadonly accepts inline JSON objects or@filerefs, so no valid documented payload is wrongly rejected.code-reviewer: no blockers. - Changeset exists (
minor), covers the library + CLI surface.
Open question (what flips me to approve)
property_type is labeled "preferred" but the registry keys on type — the primary feature path may drop the type on the wire. ad-tech-protocol-expert: sound-with-caveats, one HIGH. The generated registry contract uses type: string as the property discriminator on both the write path (types.generated.ts:4031) and the read path (ResolvedProperty, :2083). property_type is the adagents.json vocabulary (src/lib/discovery/types.ts:40), not the registry API's. Your own validation step — generate-registry-types --sync && git diff --exit-code passing — proves the freshly-synced registry OpenAPI still does not declare property_type on the property object. So SavePropertyIdentity advertises property_type as "preferred, aligned with adagents.json" and brands the registry's actual wire field type as "deprecated alias," while emitting property_type to an endpoint whose documented schema reads type.
The union permits type-only payloads, so this isn't a hard break. But a caller who follows the SDK's own "preferred" steer — saveProperty({ properties: [{ property_type: 'website', name: 'X' }] }) — sends a field the synced registry schema doesn't declare. The tests only assert the SDK forwards the field (deepStrictEqual(body.properties, [propertyIdentity])); nothing verifies the registry reads it. If the server keys on type, the property type is silently lost on the primary happy path.
I flip to approve on either:
- Confirmation the live registry honors
property_typeon write (link the server handler or the registry spec PR that lands it), or - Flip the labels so
typeis primary andproperty_typethe forward-looking alias until the registry OpenAPI declares it.
The PR body says the canonical spec "can land separately" — that ordering (SDK emits the field before the registry declares it) is exactly the risk.
Follow-ups (non-blocking — file as issues)
skills/adcp/SKILL.md:245reintroduces the rejected command. It still showsadcp registry save-property example.com https://agent.com --auth $KEY— the exact invocation this PR now hard-fails with exit 2. Agent-facing skill doc;ci:doc-linkswon't catch it (it scansblob/mainURLs, not command examples). Fix in this PR — an LLM following the skill will generate the footgun the CLI guard exists to close.- Read-side widening is aspirational.
PropertyRegistryItem.properties?andRegistryPropertyIdentity.property_type?are hand-overlaid onto generated types the registry OpenAPI doesn't declare, and thelistPropertiestest mints a mock carrying them. If the live list endpoint returns nested identity, the canonical fix is regen (spec → mock → SDKper CLAUDE.md), not a hand-maintained overlay. Note the assumption in the PR body so the next reader knows it's ahead of the spec. - Changeset understates the CLI break. The
[agent-url]positional removal is a breaking CLI shape change shipped underminor. Defensible — the arg was a server-side no-op and now fails loudly with guidance — but the changeset prose only describes the additive identity facts. A line on the CLI cutover would keep the changelog honest.
Minor nits (non-blocking)
- Runtime drops
authorized_agentssilently where the sibling throws.saveProperty(index.ts:614) overwrites caller-supplied agents to[]; the community-mirror path rejects non-empty values loudly. The TS typeauthorized_agents?: []fences typed callers, but an untyped@file.jsonpayload with agents gets them dropped without a peep. Matching the mirror's reject-loudly convention would be more consistent. type: PropertyType | stringwidens the discriminator tostring. Defeats enum-checking for the legacy alias. Acceptable for a compat field — worth a one-word comment that it's deliberate.
Confirm the wire question and this is a quick approve.
There was a problem hiding this comment.
Approving. Identity-vs-authorization is the right seam — property saves carry what a property is (property_type, identifiers[], tags[]), and agent authorization lives in adagents.json, not on /api/properties/save.
The one behavior change that looks scary — saveProperty forcing authorized_agents: [] (was property.authorized_agents ?? [] at src/lib/registry/index.ts:614) — is a wire no-op. The generated request schema already documents the registry drops the field and stores [] regardless, so nothing observable changes upstream. That keeps minor defensible and keeps this out of major-bump territory. ad-tech-protocol-expert: sound-with-caveats — property_type + enum match the canonical adagents.json model, type legacy alias correctly modeled as a discriminated union. code-reviewer: sound-with-caveats. dx-expert: good-with-caveats.
Things I checked
- Wire fidelity holds.
savePropertyspreads...propertyand overrides onlyauthorized_agents;properties[]passes through verbatim. Tests assertdeepStrictEqual(body.properties, requests[i].properties)— no normalization, no inflation. Witness, not translator. - Semver.
authorized_agentsforced-[]is inert on the wire (registry already stored[]).minoris correct for the additive identity facts. Not a breaking-contract block. - Type soundness.
Omit<RegistrySavePropertyRequest, 'authorized_agents' | 'properties'> & { authorized_agents?: []; properties?: SavePropertyIdentity[] }type-checks against the generated request body. TheSavePropertyIdentitydiscriminated union requires at least one ofproperty_type/type— an object with neither is rejected, which is the intent. - Re-export chain.
PropertyRegistryItemdropped from the generated re-export block and re-added as the augmented local alias;registry/index.tsandsrc/lib/index.tsresolve to the augmented type.SavePropertyIdentityadded to both blocks. - Changeset present (
.changeset/registry-property-identity.md,minor) on asrc/lib/**+bin/**change. CLI is bundled — changeset required, and it's here. - Enum parity.
website/mobile_app/ctv_app/dooh/podcast/radio/streaming_audiomatchesPropertyTypeinsrc/lib/discovery/types.ts:35.
Follow-ups (non-blocking — file as issues)
- Changeset prose undersells the CLI break. It names the additive identity facts but not the removed
[agent-url]positional.save-property <domain> https://agent...now exits 2 — worth a line so CLI-script users aren't surprised. RegistryPropertyIdentityisn't re-exported. It's the element type of the augmentedPropertyRegistryItem.properties[]but adopters can't name it from@adcp/sdk. If callers are meant to work with individual identity items off a list result, add it alongsideSavePropertyIdentityin both re-export blocks.- Ships ahead of the live wire. The registry OpenAPI still only declares
type: stringonsaveProperty—property_typeis spec-canonical but not yet regenerated. Confirm the AAO handler actually readsproperty_type(or that the CI regen carries it) before callers rely on the preferred field name. SDK forwards verbatim, so this is a registry-side confirmation, not an SDK bug.
Minor nits (non-blocking)
- CLI error doesn't show the way out.
bin/adcp-registry.js— the exit-2 message ("no longer accepts an agent URL; pass identity facts as payload JSON") prints the usage shape but not a worked example. The example already exists inprintRegistryUsage(); echo it on the error path so a migrating user sees the concrete'{\"properties\":[...]}'form. - Guard misattributes bare arrays.
if (extraArg && !extraArg.startsWith('{') && !extraArg.startsWith('@'))fires the "agent URL" message for any non-{/@positional — including a user who forgot the{\"properties\":...}wrapper and passed a bare[{...}]. Broaden the guard to accept[, or soften the message to "expected payload JSON or @file". - Silent drop for JS callers. A JS adopter passing
authorized_agents: [{url}]has it discarded client-side (TS callers get a compile error from the[]narrowing). Inert on the wire, so fine — but the community-mirror helper in this same file rejects a non-emptyauthorized_agentsrather than silently emptying it (src/lib/registry/index.ts:275). Matching that reject-on-present pattern would be more honest. Optional.
LGTM. Follow-ups noted below.
There was a problem hiding this comment.
Request changes on one thing: the write-path normalizer inverts the field it's supposed to canonicalize, and the SDK ends up contradicting its own generated contract on a mutation. Everything else is clean and well-tested.
MUST FIX — normalizeSavePropertyIdentity deletes the wire field the registry actually reads
src/lib/registry/index.ts (normalizeSavePropertyIdentity) maps legacy type → property_type and then delete normalized.type. But the write contract you regenerated in this very PR keys on type, not property_type:
src/lib/registry/types.generated.ts:4029-4033—savePropertyrequest body:properties?: { type: string; name: string }[].typeis the declared field;property_typeis not in the write shape.schemas/registry/registry.yaml— same:properties[].itemscarrytype, noproperty_typeon the write path.
So the SDK's own documented happy path — the SavePropertyIdentity JSDoc calls property_type the "preferred field name" — emits { property_type, name, ... } with type stripped, to an endpoint whose regenerated OpenAPI declares type and doesn't mention property_type. One sentence of what breaks for adopters: if /api/properties/save keys on type (as its published + regenerated contract says), every identity save either 400s on the missing declared field or silently drops the property-type fact — code-reviewer flagged this as "the one change that can lose caller data," ad-tech-protocol-expert as a HIGH-severity witness→translator inversion. Two independent stacks, same finding.
This is the witness-not-translator invariant read backwards: instead of forwarding the field the wire declares, the SDK rewrites it into a field the wire doesn't. The PR body claims the live registry accepts property_type ahead of the published OpenAPI, but nothing verifies it — the new tests only assert the SDK forwards the payload, not that the registry reads it, and the Validation list has no live round-trip against the real /api/properties/save. The primary user-facing change ships unvalidated against the contract it's changing.
Fix is small: send both type and property_type (stop deleting type) until the generated OpenAPI declares property_type, or link the server handler proving it honors property_type on write. Send-both is the fail-closed choice — it can't lose data against either server contract.
Things I checked
- Changeset present,
minor, descriptive (registry-property-identity.md). Additive type widening + new optional fields —minoris the right bump. Not a MUST FIX #5/#7. authorized_agents: []forcing is spec-coherent, not a dropped write path —registry.yaml+types.generated.ts:4014-4016both document the endpoint as identity-only ("any value here is dropped"). Wire no-op. Correctly narrowed to the[]empty tuple and covered by tests.PropertyTypeenum (website|mobile_app|ctv_app|dooh|podcast|radio|streaming_audio) andPropertyIdentifierTypematchsrc/lib/discovery/types.ts.PropertyRegistryItemredefinition is additive (generated ∩{ properties?: RegistryPropertyIdentity[] }) — not a breaking export change.- CLI exit-2-on-legacy-URL is the right call: converts a silent stripped-authorization no-op into a loud failure. Exit 2 matches the sibling "domain is required" path.
Follow-ups (non-blocking — file as issues)
- Stale skill example.
skills/adcp/SKILL.md:245still showsadcp registry save-property example.com https://agent.com --auth $KEY— the exact invocation this PR now hard-fails with exit 2.ci:doc-linksscansblob/mainURLs, not command examples, so it won't catch it. An LLM following the skill will generate the footgun the guard exists to close. Worth fixing in this PR since it documents the removed behavior directly. - Test mocks are ahead of the wire.
listProperties/resolvePropertytests now mint mocks carrying nestedproperties[].{property_type,identifiers,tags}. Aspirational until the registry is proven to emit them. Fine to ship, but the tests assert a shape the OpenAPI doesn't declare — note it so it's not mistaken for a verified read contract. - TODO(#2318) premise.
ad-tech-protocol-expertnotes AAO #5787 documentsresolveIdentifiers/disputeFact, a separate funnel — it does not add identity facts tosaveProperty. The "collapse onto generated shape once OpenAPI republishes" assumption may not hold; confirm which spec PR actually landsproperty_typeon thesavePropertyrequest body.
Minor nits (non-blocking)
- CLI rejects payload JSON with leading whitespace.
bin/adcp-registry.jsguard!extraArg.startsWith('{') && !extraArg.startsWith('@')treats' {\"properties\":...}'(leading space from a heredoc) as a legacy agent URL and exits 2 with the wrong message.trim()before the check, or attemptparsePayloadand fall back to the rejection on parse failure. - Error message names the symptom, not the fix. "no longer accepts an agent URL; pass identity facts as payload JSON" doesn't tell the caller what to do with their URL — the honest answer is "authorization is managed at your origin adagents.json; there's nothing to migrate the URL to." One concrete example payload in the message closes the loop for humans and code-gen agents both.
Send-both on the write path plus a live round-trip (or the server handler link) flips this to approve.
There was a problem hiding this comment.
Approving. Additive registry surface — three new catalog methods plus property-identity facts on save — and the write path stays a witness: the SDK back-fills the required type field and only mirrors the not-yet-published property_type alongside it, rather than dropping or inventing a wire field.
Things I checked
- Dual-emission is forward-compat, not fabrication.
saveProperty'sproperties[]item in the published schema (schemas/registry/registry.yaml) is an open object —required: [type, name], noadditionalProperties: false— so emittingproperty_type/identifiers/tagsis adding keys the contract tolerates, andnormalizeSavePropertyIdentity(src/lib/registry/index.ts:1464-1470) always keeps the requiredtypepopulated.ad-tech-protocol-expert: sound. Mirrors the adcp#4844format_ids/format_optionsdual-emission pattern, gated behindTODO(#2318)with a defined exit. authorized_agents: []is forced at the boundary and cannot be bypassed. Spread order innormalizeSavePropertyRequest(src/lib/registry/index.ts:1452-1461) is load-bearing —...propertythen unconditionalauthorized_agents: []— and bothsavePropertyand thesavePropertiesfan-out route through it.security-reviewer: clean. Matches the server contract, which documents the field as dropped.- Auth gates fail closed.
resolveIdentifiersrequires a key unlessmodeis exactly'lookup'((request.mode ?? 'resolve') !== 'lookup');fileCatalogDisputeunconditionally requires one;getCatalogDisputeusesencodeURIComponenton the id path segment withnullOn404. Tests cover the lookup-without-key path (test/lib/registry.test.js). - CLI migration fails loud. Legacy
http(s)://positional now exits 2 with a message pointing at originadagents.json(bin/adcp-registry.js:461-475); array-payload and non-object payloads each get a distinct exit-2 message. Regression coverage attest/lib/registry-cli.test.js:1061+. - Changeset present (
.changeset/registry-property-identity.md,minor) — correct for the new exports and methods.
Follow-ups (non-blocking — file as issues)
- The
authorized_agentsnarrowing deserves an explicit changeset line.SavePropertyRequest.authorized_agentsnarrows from{ url; authorized_for? }[]to[](src/lib/registry/types.ts). Runtime was already a no-op (server drops the field; the pre-PR client forced?? []), so nothing breaks at runtime — but a TS adopter still passingauthorized_agents: [{ url }]now failstsc. That's a compile-only break on a documented no-op field, which is why I'm not blocking, but the changeset body explains the CLI rationale and never states the exported type narrows. Add one line calling it out, and confirm with the team whether the semver table wantsmajorhere.code-reviewerflagged the same. - Confirm the deployed registry tolerates the extra keys. The schema permits
property_type/identifiers/tags(open object), and the readback assertions are mocked — the write round-trip against a live registry that actually persists these is unverified.ad-tech-protocol-expert's open question. Worth a smoke against staging before adopters lean on it.
Minor nits (non-blocking)
fileCatalogDisputeskips a guard the schema marks required. It validatessubject_type/subject_value/claimbut notdispute_type, whichCatalogDisputeRequestrequires. A caller omitting it gets a server 400 instead of the fast local throw the other three fields get.src/lib/registry/index.ts:507.- Payload overload is example-only in the CLI help. The
[payload-json]positional accepts inline{...}or@file, but the usage block shows them on separate lines with no note that it's one slot.dx-expert: the error messages teach the shape better than the help does — worth a one-linepayload-json: inline JSON object or @filein the usage text.
Codex second-opinion came back empty on backend quota, per the PR body — noted, not held against it.
LGTM. Follow-ups noted below.
Summary
saveProperty/savePropertiesrequest type soproperties[]accepts full property identity facts:property_type, legacy input aliastype,identifiers[], andtags[]typeandproperty_typeonsavePropertywrites until the official OpenAPI declares the aligned field name, preserving compatibility with the current generated write contractauthorized_agents: []at theRegistryClient.savePropertyboundarysave-propertycommand to accept identity payload JSON instead of the old[agent-url]authorization positionalresolveIdentifiers,fileCatalogDispute, andgetCatalogDisputeSDK methodstypenormalization, readback shapes,savePropertiesfan-out, CLI identity payloads, and authoritative-property409preservationNotes
AAO #5787 has now republished in the official registry OpenAPI, so this PR includes the regenerated
schemas/registry/registry.yamlandsrc/lib/registry/types.generated.tschanges for the new resolve/dispute endpoints. The publishedsavePropertywrite shape still declares the olderproperties?: { type; name }[]item, so the SDK sends bothtypeandproperty_typefor identity writes. Follow-up #2318 tracks collapsing the hand-written SDK identity types once the official OpenAPI publishes the fullsavePropertyidentity contract.Per RFC #5782 / AAO #5787, this PR covers the authenticated owner/member
savePropertyidentity contribute-back slice and the newly published identifier resolve/dispute endpoints. The provenance-weighted identifier contribution path is exposed through the officialresolveIdentifierssurface rather than by expandingsaveProperty.savePropertyintentionally has no caller-provided provenance envelope: the authenticated registry member API key is the provenance for an owner declaration. That is different from the future third-partyreportIdentifiers()path, where explicit provenance is load-bearing.Target package version after changesets:
@adcp/sdk@9.7.0.Validation
npm run build:libNODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/registry.test.jsNODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/registry-cli.test.jsnpm run generate-registry-types -- --sync && git diff --exit-code -- schemas/registry/registry.yaml src/lib/registry/types.generated.tsnpm run ci:schema-check && npm run ci:docs-checknpm run format:checkgit diff --checknpm run typecheckand build passedExpert Review
[agent-url]behavior.npm run review:codex -- --all --base mainwas attempted, but the local Codex review command failed with backend quota exceeded and produced no findings.