fix: preserve Content database property mutations - #3314
Conversation
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest revision broadens the change from property-shape repair to typed agent entries, server-derived authority, discovery-first evaluation, and compatibility updates to database mutation callers. The typed schema and server-side type assertions are directionally sound, and the tests cover the main happy path plus mismatched types and discovery behavior. Four parallel reviewers found four new medium-severity issues, including two idempotency compatibility gaps and a dedicated eval runner that cannot satisfy its own discovery requirements. This is standard risk because it changes shared mutation contracts, replay semantics, and evaluation gates.
Key Findings
- 🟡 MEDIUM — Legacy authority changes retry identity: the optional legacy
authorityScoperemains in the digest even though authority is now server-derived. - 🟡 MEDIUM — Empty typed entry lists execute as empty mutations:
propertyEntries: []is accepted despite the contract requiring omission when no values are requested. - 🟡 MEDIUM — Typed retries can fail after schema changes: type assertions run before durable receipt replay, breaking retries that should be independent of model-only assertions.
- 🟡 MEDIUM — Dedicated preservation runner lacks required discovery tools: it registers only the mutation action while the scenario/scorer requires list-then-inspect discovery, making the runner fail even for correct model behavior.
🧪 Browser testing: Skipped — the changes affect actions, eval infrastructure, and tests, with no browser-visible UI surface changed.
| const { propertyTypeAssertions: _propertyTypeAssertions, ...canonicalInput } = | ||
| input; | ||
| return digest({ operation, ...canonicalInput }); |
There was a problem hiding this comment.
🟡 Exclude legacy authority assertions from retry identity
authorityScope is now an optional legacy assertion while the server derives the authoritative scope from the selected database, but databaseMutationPayloadDigest still hashes the target unchanged. The same mutation and idempotency key can therefore hash differently when retried through an old caller with authorityScope versus the new agent contract without it, returning IDEMPOTENCY_KEY_REUSED instead of replaying. Hash only the server-derived stable target fields while retaining the assertion for mismatch validation.
| export const databasePropertyEntriesSchema = z | ||
| .array(databasePropertyEntrySchema) | ||
| .max(1_000) |
There was a problem hiding this comment.
🟡 Reject empty typed property entry lists
propertyEntries is optional, but when supplied as [] it is accepted and canonicalized to an empty propertyValues object plus empty type assertions. This lets an agent perform a successful no-op create/update/upsert despite the contract saying an empty list must never be used, preserving the original dropped-property failure mode. Require .min(1) when the field is present or canonicalize an empty list to the omitted representation.
| const initial = await loadContext(input.target, "editor"); | ||
| const inputDigest = payloadDigest("create", input); | ||
| assertPropertyTypeAssertions(initial, input.propertyTypeAssertions); |
There was a problem hiding this comment.
🟡 Check durable replay before typed assertion validation
assertPropertyTypeAssertions now runs before replayReceipt. If a committed typed mutation is retried after its property definition changes type, the assertion throws before the existing durable receipt can be returned, even though propertyTypeAssertions is excluded from the payload digest and legacy propertyValues retries still replay. Preserve the replay-first path, then validate type assertions only before a new write.
| const runner = await createAgentRunner({ | ||
| actions: { |
There was a problem hiding this comment.
🟡 Register discovery actions in the dedicated preservation runner
The dedicated runner registers only add-database-item, but the scenario and expected_property_values scorer require list-content-databases followed by get-content-database and validate their inputs. A correct model cannot perform discovery in this runner, so the gated pnpm eval:property-preservation command will always fail the discovery checks. Register fixture-only discovery actions with deterministic responses, or use a create-only scenario/scorer for this runner.

Problem
Content users can ask the built-in AI to create or update rows in a database. In live testing, the AI reached the correct mutation action but either dropped requested property values or confused a number in the database title (such as
PR #3314) with the database's stable ID. The action then truthfully rejected the request or stored only the incomplete input it received.Approach
Make the agent discover the database and its mutation contract first, then give it a typed property-entry format that is difficult to construct incorrectly. Authority identity is no longer authored by the model: the authenticated server derives it from the selected database after the normal access check.
Typed entries are canonicalized into the existing
propertyValuescontract before validation, retry hashing, or writing. UI and programmatic callers can continue usingpropertyValues, and equivalent inputs retain the same retry identity.What changed
{ propertyId, propertyType: "number", value: 3314 }; each property type exposes the correct JSON value shape.Safety and operations
The authorization boundary remains server-side: selecting a stable target does not grant access, and the backing database document is authorized before its authority scope is derived. Schema revision checks, row revision checks, sparse-update behavior, idempotency receipts, verified read-back, and loud typed failures are preserved. There is no migration, feature flag, credential, or destructive data operation in this follow-up.
Verification
d01629941a4bddd18d7186747706525f8441995c; earlier live evidence is intentionally not claimed for this revised artifact.Review focus
propertyValuescallers and retry identities?