feat(memory): model rejected approaches - #75
Conversation
|
Warning Review limit reachedNext included review available in 27 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
Comment |
ahrav
changed the title
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 695c8def45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ( | ||
| (current.category === ANTI_MEMORY_CATEGORY) !== | ||
| (nextAttributes.category === ANTI_MEMORY_CATEGORY) | ||
| ) { |
There was a problem hiding this comment.
This XOR rejects only category conversions, so a generic revision whose current and next categories are both REJECTED_APPROACH still proceeds. The maintenance readers include anti-memory claims, and the classify, curate, and verify flows call this staging function directly; when one changes content or metadata, it appends a rejected-approach revision without inserting the corresponding claim_anti_memory_revision_payloads row. Subsequent readAntiMemory() calls then throw ClaimGraphCorruptionError, so generic same-category revisions must also be refused or routed through the typed payload writer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The XOR isn't the guard for that case — the check immediately above it is. storage-claim-operations.ts:1317-1325 refuses any revision whose current or next category is REJECTED_APPROACH unless antiMemoryWriter === true:
if (
(current.category === ANTI_MEMORY_CATEGORY ||
nextAttributes.category === ANTI_MEMORY_CATEGORY) &&
input.antiMemoryWriter !== true
) {
throw new ClaimOperationInputError(
"generic anti-memory revision is refused; use the typed anti-memory API",
);
}A generic same-category revision throws there and never reaches the XOR. The XOR is deliberately narrower: it catches a conversion that the typed writer, which does set the flag, would otherwise be permitted to make.
So the classify, curate, and verify flows cannot append a payload-less rejected-approach revision. They call the stage without antiMemoryWriter, hit the first check, and fail loudly instead of leaving a revision that readAntiMemory() would later reject as corrupt. storage-anti-memory.test.ts:271 ("refuses a generic staging revise so a payload-less revision cannot be minted") covers this.
| export function createAgentAntiMemory( | ||
| db: Database, | ||
| producer: ProducerIdentity, | ||
| input: Omit<CreateAntiMemoryInput, "provenance"> & { | ||
| provenance: Omit<ClaimEvidenceProvenance, "sourceTrustClass">; | ||
| }, | ||
| ): ClaimOperationRunResult { | ||
| return createAntiMemory(db, producer, { |
There was a problem hiding this comment.
This new agent-facing entry point has no production caller anywhere in the repository; it is referenced only by tests. The model-accessible ctx_memory schema still accepts only V2_MEMORY_CATEGORIES, and claim-actions.ts rejects REJECTED_APPROACH, while historian promotion explicitly drops that category. Consequently no model or host workflow can create the records introduced by this feature, so the writer must be wired into an extraction/tool action or another reachable runtime path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional for this PR. It is the first of a four-PR stack, and the split is recorded in the PR description: #76 adds the Rust boundary, #77 adds the writers, #78 adds warning retrieval and lifecycle verification.
The observations you list are the point rather than the gap. ctx_memory accepting only V2_MEMORY_CATEGORIES, claim-actions.ts rejecting REJECTED_APPROACH, and promotion dropping the category are the positive-memory paths staying closed to anti-memory by construction — the stack's central invariant is that a rejected approach never becomes advice through a broad memory read. Adding a reachable runtime caller here would land the writer ahead of the boundary that is supposed to contain it.
| const current = readAntiMemory(db, input.token.publicClaimId); | ||
| if (current === null) throw new ClaimOperationInputError("unknown anti-memory claim"); | ||
| if (current.expiresAt === null || input.expiresAt <= current.expiresAt) { | ||
| throw new ClaimOperationInputError("anti-memory TTL extension must move expiry forward"); |
There was a problem hiding this comment.
After an extension succeeds, an identical retry with the same producer, operation key, token, and expiry reads the newly extended record, finds input.expiresAt <= current.expiresAt, and throws before runClaimOperation() can return the stored receipt. This breaks the durable replay contract precisely for retries after an ambiguous success; perform the receipt lookup before this state-dependent check, or move the check into the staged operation that only runs when no receipt exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The check already runs inside the staged operation. validateStage is invoked at storage-anti-memory.ts:310, inside the callback handed to runClaimOperation, and runClaimOperationInCurrentTransaction reads the stored receipt at line 498 and only reaches stage(db) at line 514:
const existing = readStoredReceipt(db, envelope);
if (existing) { /* ... returns the replayed receipt ... */ }
// ...
staged = stage(db); // validateStage runs in hereAn identical retry therefore returns its receipt before the expiry comparison is ever evaluated. storage-anti-memory.test.ts:211 asserts exactly this sequence — extend, then retry with the same producer, operation key, token, and expiry — and expects replayed: true with the stored expiry unchanged.
You were right that this operation's replay was broken, though; the cause sat one layer over. The request digest was folding in the payload the extension read back from the current revision, so a retry diverged whenever an unrelated revision had changed that payload, raising ClaimOperationKeyReuseError. Fixed in 2d3034f with a regression test that revises between the two attempts.
| payload: payloadDigestShape(payload), | ||
| projectId: input.projectId, | ||
| provenance: provenanceDigestShape(input.provenance), | ||
| requestScope: input.requestScope ?? null, | ||
| }), |
There was a problem hiding this comment.
input.importance affects the persisted revision attributes but is omitted from this request digest. Reusing a producer/operation key with the same payload and provenance but a different importance therefore replays the first receipt instead of raising ClaimOperationKeyReuseError, silently ignoring the changed request; include the normalized/defaulted importance in the digest shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2d3034f. The create digest now carries the resolved importance:
importance: input.importance ?? DEFAULT_MEMORY_IMPORTANCE,Resolved rather than raw, matching how createProjectMemoryClaim digests attributesRequestShape(resolveAttributes(input)), so an omitted importance and an explicit 50 stay one request while a genuinely changed one raises ClaimOperationKeyReuseError.
expiresAt deliberately stays out of this digest even though it also reaches the attributes: it is derived as nowMs + ANTI_MEMORY_DEFAULT_TTL_MS, so digesting it would make every retry with a fresh Date.now() diverge.
Regression test: "refuses an operation-key reuse that changes only importance".
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| actor: args.input.actor, | ||
| expiresAt: args.expiresAt ?? null, | ||
| operation: args.operation, | ||
| payload: payloadDigestShape(args.payload), |
There was a problem hiding this comment.
WARNING: Request digest for extend-anti-memory-ttl embeds ephemeral payload state
extendAntiMemoryTtl accepts an ExtendAntiMemoryTtlInput which does not include a payload. It reads current.payload from a pre-flight database read and passes it to reviseWithPayload, which embeds payload: payloadDigestShape(args.payload) into the operation request digest.
If an extension operation succeeds on revision 1 (payload ClaimOperationKeyReuseError instead of replaying the stored receipt. The request digest for extend-anti-memory-ttl should be computed strictly from the operation inputs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed, and fixed in 2d3034f. This was the real replay defect in this operation.
reviseWithPayload now takes an explicit digestPayload that is separate from the payload it writes. reviseAntiMemory passes the caller's normalized payload; extendAntiMemoryTtl passes null, because an extension supplies no payload:
payload:
args.digestPayload === null ? null : payloadDigestShape(args.digestPayload),The extension digest is now token, expiry, provenance, actor, and request scope only, so the stored payload can move underneath an in-flight operation key without breaking the retry.
The regression test creates a record, extends its TTL, revises the payload through an unrelated operation, then retries the original extension request and asserts replayed: true. It fails against the previous digest.
| provenance: provenanceDigestShape(args.input.provenance), | ||
| requestScope: args.input.requestScope ?? null, | ||
| token: args.input.token, | ||
| }), |
There was a problem hiding this comment.
WARNING: Pass canonicalized token shape to computeClaimOperationRequestDigest
Line 281 passes token: args.input.token directly into computeClaimOperationRequestDigest. All other claim operations across storage-claim-operations.ts normalize mutation tokens with tokenRequestShape(token) to project the canonical 7 fields before digest computation. Passing the raw token object risks digest divergence if extra or non-enumerable properties are present.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2d3034f. tokenRequestShape is now exported from storage-claim-operations.ts and used here, so the anti-memory digests project the same canonical seven fields as every other claim operation.
| } | ||
|
|
||
| function optionalText(value: unknown, field: string): string | null { | ||
| if (value === undefined || value === null) return null; |
There was a problem hiding this comment.
SUGGESTION: optionalText throws "must be non-empty" when passed empty string
optionalText checks only for undefined and null before delegating to requiredText:
function optionalText(value: unknown, field: string): string | null {
if (value === undefined || value === null) return null;
return requiredText(value, field);
}If an optional field in AntiMemoryPayload (e.g., preconditions, saferAlternative, recovery) is passed as "" or whitespace, requiredText throws ClaimOperationInputError("anti-memory <field> must be non-empty"). For optional fields, empty strings should normalize to null to align with the schema check constraint CHECK (col IS NULL OR length(trim(col)) > 0).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 2d3034f. optionalText now short-circuits a blank string before delegating:
if (value === undefined || value === null) return null;
if (typeof value === "string" && value.trim().length === 0) return null;
return requiredText(value, field);That collapses the three-way split you describe: undefined, null, and whitespace-only all map to the single stored form the column constraint allows. Non-string values still throw, so genuinely malformed payloads are unaffected.
Regression test asserts saferAlternative: "" and preconditions: " " both read back as null, and that the renderer omits their lines rather than emitting an empty label.
| return attachEvidenceStage(db, claim, input.provenance, nowMs); | ||
| } | ||
| const normalizedHash = computeNormalizedHash(nextContent); | ||
| const normalizedHash = computeNormalizedHash(input.dedupText ?? nextContent); |
There was a problem hiding this comment.
WARNING: Revision unchanged fast-path ignores changes to dedupText
ReviseProjectMemoryClaimInput accepts an optional dedupText to allow deduplication identity to differ from display content. In stageReviseProjectMemoryClaimInCurrentTransaction, the unchanged check only verifies contentUnchanged (SHA-256 of nextContent) and standard attributes.
If a caller updates dedupText while leaving content and attributes unchanged, unchanged evaluates to true and returns attachEvidenceStage early. As a result, the revision is not created, normalized_hash in claim_memory_revision_attributes and claim_memory_current_heads is not updated, and duplicate detection is skipped. The unchanged check should also verify that computeNormalizedHash(input.dedupText ?? nextContent) === current.normalizedHash.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in 2d3034f, close to your suggested shape. normalizedHash is now computed before the unchanged check and compared against the current row (line 1345):
const normalizedHash = computeNormalizedHash(input.dedupText ?? nextContent);
const unchanged =
contentUnchanged &&
normalizedHash === current.normalizedHash &&
/* ...attributes... */;Comparing the already-computed hash rather than re-deriving it inside the predicate keeps the input.dedupText ?? nextContent fallback in one place.
One correction on reachability: the exposed path is the public reviseProjectMemoryClaim, not the anti-memory writer. There dedupText is JSON.stringify([trigger, rejectedStrategy]) and both fields appear in the rendered content, so dedup identity cannot change while the content bytes stay identical. The regression test therefore lives in the generic revise suite.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (23 files)
Previous Review Summaries (4 snapshots, latest commit b1519fb)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b1519fb)Status: No Issues Found | Recommendation: Merge Files Reviewed (20 files)
Previous review (commit 2d3034f)Status: No Issues Found | Recommendation: Merge Files Reviewed (20 files)
Previous review (commit 3dfdc07)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (19 files)
Fix these issues in Kilo Cloud Previous review (commit 695c8de)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (18 files)
Reviewed by gemini-3.7-flash · Input: 135.1K · Output: 16.1K · Cached: 829.2K |
Review findings on the rejected-approach model, fixed together because they share one root cause: the isolation guards lived in wrapper functions while in-transaction callers (dreamer curate/classify/verify, relocation) reach the stage functions directly. - Refuse REJECTED_APPROACH in stageCreate/stageRevise unless the typed writer sets antiMemoryWriter, so a generic revise can never mint a revision without its append-only payload row; drop the now-redundant wrapper copies. - Exclude anti-memory from every read surface except explicit_search. The maintenance lanes feed the dreamer pipeline, which re-creates content it consumes; a rejected approach reaching them could be laundered back into auto-injected positive memory. Also filter it in the candidate SQL so automatic surfaces stop paying hydration for rows the surface check discards. - readAntiMemory reports the stored memory_scope/sharing and fails closed when they are not project/private instead of hardcoding them. - extendAntiMemoryTtl checks forward progress inside the staged callback so an identical same-operation-key retry replays its receipt instead of throwing. - Drop the unreachable auto_search surface; the hoisted exclusion is fail-closed for any surface a later PR adds. - Share provenanceRequestShape with the typed writer instead of a byte-identical private copy that could drift request digests. - Remove unreferenced WRITABLE_MEMORY_CATEGORIES/WritableMemoryCategory, which advertised REJECTED_APPROACH as generically writable while the guards refuse exactly that. - Update the frozen schema inventory for the five anti-memory objects the cutover gate did not yet account for.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dfdc0786f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (current === null) throw new ClaimOperationInputError("unknown anti-memory claim"); | ||
| return reviseWithPayload(db, producer, { | ||
| input, | ||
| payload: current.payload, |
There was a problem hiding this comment.
When a TTL extension commits and another valid anti-memory revision changes the payload before the original client retries, this rereads the newer payload and reviseWithPayload() incorporates it into the extension's request digest. The retry therefore raises ClaimOperationKeyReuseError instead of replaying the stored receipt, even though its producer, operation key, token, expiry, provenance, and actor are unchanged. The new in-stage forward-progress check fixes an immediate retry but not this intervening-revision case; compute the extension digest only from caller-supplied request fields and defer reading the payload until the staged callback runs after a receipt miss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The digest half of this landed in 2d3034f, just before this round — you were reviewing that commit, but the assertion describes the code before it. extendAntiMemoryTtl now passes digestPayload: null, and the digest reads that field rather than the payload it writes:
payload:
args.digestPayload === null ? null : payloadDigestShape(args.digestPayload),So the extension digest is token, expiry, provenance, actor, and request scope only, and the intervening-revision case you describe replays. There is a regression test for exactly that sequence: extend, revise the payload through an unrelated operation, retry the original extension, assert replayed: true.
Your second recommendation was still live, though, and is now done in b1519fb. The pre-flight readAntiMemory was still running before runClaimOperation, which left a state read on the replay path even with the digest fixed — a retry after the claim had gone away would throw unknown anti-memory claim instead of replaying its receipt. The payload now comes from a resolver invoked inside the staged callback, and that resolver absorbed the forward-progress check too, so the operation has one hook instead of two, one read instead of two, and nothing before runClaimOperation depends on current state.
| return reviseWithPayload(db, producer, { | ||
| input, | ||
| payload: current.payload, | ||
| expiresAt: input.expiresAt, | ||
| operation: "extend-anti-memory-ttl", |
There was a problem hiding this comment.
When an anti-memory was promoted by a verification event rather than explicit-user evidence, routing a TTL-only extension through the generic revision stage creates a new revision without the old revision's verification or applicability state. In the direct-schema path, a VERIFIED anti-memory with an exact applicability path becomes a CANDIDATE revision with unknown applicability after this call, so extending a revalidated warning can immediately strip the authority and scope needed by later warning or veto retrieval. Preserve revision-bound verification/applicability for this metadata-only operation, or represent the expiry extension without minting a content revision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this one was reachable today. A probe against the direct schema:
recordProjectMemoryVerificationaccepts aREJECTED_APPROACHclaim — no guard refused it — returningappliedand writing averification_eventsrow against revision 1.extendAntiMemoryTtlthen appends revision 2. The extension succeeds and the expiry moves, but revision 2 has zero verification events.
Both verification and applicability are revision-bound (verification_events.revision_id, and applicabilityHeadsDigestForRevision reading claim_revision_applicability_streams per revision), and the shared revise stage does not copy either onto the revision it appends. The existing carry-forward logic there covers supports observations only, which is why explicit-user standing survives a metadata-only revision and verification does not.
Fixed in b1519fb by closing the gap rather than by copying state. Verification and applicability mapping now refuse anti-memory at their generic entry points, matching the guards the create and revise stages already carry:
refuseGenericAntiMemoryRevisionAccess(db, claim.currentRevisionId, "verification");I deliberately did not make the extension preserve verification instead. Carrying verification events across a metadata-only revision changes maturity semantics for every claim in the system, not just anti-memory, and whether a revalidated warning should keep its authority across a TTL extension belongs with the PR that introduces anti-memory verification and warning retrieval (#78). Until then the boundary fails loudly instead of downgrading a warning where nobody would see it. Test asserts both refusals and that no verification event is left behind.
Four independent defects in the anti-memory writers and the shared revision stage, each of which lets a correct-looking call lose data. The `extend-anti-memory-ttl` request digest folded in the payload the extension read back from the current revision. That payload is not part of the request: an extension only supplies a token and an expiry. Once any unrelated revision changed the payload, an idempotent retry of the original extension computed a different digest and raised `ClaimOperationKeyReuseError` instead of replaying its receipt, which is precisely the case durable replay exists to serve. The digest now describes what the caller supplied, so the stored payload can move underneath it without breaking the retry. The create digest omitted `importance`, which does reach the persisted revision attributes. Reusing an operation key with the same payload and provenance but a different importance replayed the first receipt and dropped the new value silently rather than reporting the reused key. The digest now carries the resolved importance, so an omitted value and an explicit default remain one request. Mutation tokens went into the anti-memory digests unprojected, unlike every other claim operation. A token that carries an extra property — one round-tripped through JSON by a retrying caller, say — would digest differently from the same token built in-process. `tokenRequestShape` is now exported and used here too, so both spellings agree. The revision fast path compared content and attributes but not deduplication identity, so a revision that changed only `dedupText` counted as unchanged. It returned early without appending a revision, leaving the superseded `normalized_hash` on both the attributes row and the current head and skipping the duplicate check for the identity the claim had just moved to. The comparison now includes the hash. Blank optional payload fields were rejected rather than normalized. The payload columns are `CHECK (col IS NULL OR length(trim(col)) > 0)`, so an absent optional field has exactly one legal stored form, and a blank string is that same absence spelled differently — common in generated payloads. Whitespace-only optional values now become null instead of failing the whole write.
Two follow-ons in the same neighborhood as the request-digest fixes. Extending a TTL read the current payload before entering the operation, so the read itself sat on the replay path: a retry whose receipt should have short-circuited instead re-read the claim and could fail on state that had moved since the first attempt. The payload is now produced by a resolver that runs inside the staged callback, after the receipt lookup misses, and that resolver also carries the forward-progress check the separate validation hook used to hold. One hook instead of two, one read instead of two, and nothing before `runClaimOperation` depends on current state. Verification and applicability mapping still accepted anti-memory claims through their generic entry points, which the create and revise stages already refuse. Both attach to one exact revision, and the typed writer appends a fresh revision on every extension without carrying either forward, so a verified anti-memory with an exact applicability path lost its authority and scope on its next TTL extension — silently, since the extension itself succeeded. Whether an anti-memory should hold verification across an extension is a question for the PR that introduces anti-memory verification; until then these entry points refuse, so the boundary is a failed call rather than a downgrade nobody observes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1519fb004
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (current.expiresAt === null || input.expiresAt <= current.expiresAt) { | ||
| throw new ClaimOperationInputError( |
There was a problem hiding this comment.
When two clients extend the same anti-memory concurrently, a client holding the older token can request an expiry that was forward progress from its snapshot but is now below the winner's expiry. resolvePayload() performs this comparison before stageReviseProjectMemoryClaimInCurrentTransaction() validates the token, so the call throws ClaimOperationInputError instead of producing and persisting the operation contract's zero-effect stale result. Validate the token first, then apply the forward-progress check only for a current token.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 38f7e6f. You're right about the ordering: resolvePayload() runs at the top of the staged callback and stageReviseProjectMemoryClaimInCurrentTransaction validates the token on its first line, so the comparison was reached first and a lost race surfaced as ClaimOperationInputError with no receipt, where the contract calls for a zero-effect stale the caller can retry with a fresh token. Moving the check into the resolver in b1519fb carried the ordering over from the validateStage hook it replaced.
Rather than reorder the two, the check is now conditional on the token still being current:
if (validateProjectMemoryMutationToken(db, input.token).ok) {
if (current.expiresAt === null || input.expiresAt <= current.expiresAt) {
throw new ClaimOperationInputError(
"anti-memory TTL extension must move expiry forward",
);
}
}That reads as the rule you'd want stated directly: forward progress is only a meaningful judgement against a snapshot the caller still holds. A superseded token means someone already won, so the request falls through to the stage and gets stale.
Regression test drives the exact race — two extensions from one shared token, the loser asking for an expiry that is forward progress from that snapshot but behind the winner's — and asserts outcome: "stale" with no effects and the winner's expiry intact.
| ); | ||
| } | ||
| return attachEvidenceStage(db, claim, input.provenance, nowMs); | ||
| return attachEvidenceStage(db, claim, input.provenance, nowMs, input.content); |
There was a problem hiding this comment.
When an existing pair-based dedup slot is found, input.content can differ materially from the stored revision because rejection reason and every optional payload field are excluded from the dedup identity. Attaching that observation as supports lets unrelated stronger provenance authorize the old bytes: for example, a model-created reason: insecure record followed by an explicit-user reason: too slow record keeps the model payload but makes hasExplicitUserEvidence() true and promotes it from CANDIDATE to VERIFIED. Only attach supporting evidence when it actually supports the stored payload, or append/reconcile a typed revision instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced exactly as described, including the promotion. Probe against the direct schema:
create A: model_inference, reason "Redis is insecure here"
hasExplicitUserEvidence(r1) = false
create B: explicit_user, reason "Redis is too slow" (same trigger + strategy)
outcome: applied, kind: evidence_attached
stored reason still: "Redis is insecure here"
hasExplicitUserEvidence(r1) = true
The mechanism has two halves. attachEvidenceStage is called with input.content as the extracted text, so the observation records B's bytes but the supports row lands on A's revision. Then hasExplicitUserEvidence accepts any explicit-user supporting observation on a first revision unconditionally — its cr.revision = 1 branch short-circuits before the content-digest comparisons that guard later revisions. So a user statement about a different rejection reason confers explicit-user standing on the model's payload.
Worth naming why this bites anti-memory and not generic memory: for generic claims the dedup hash is the normalized content, so a hash collision really is the same assertion and attaching support is sound. Anti-memory hashes a two-field subset by design, so a collision can carry a materially different record. The soundness of the attach silently depended on an equivalence that no longer holds.
I have not fixed it here, because every remedy is a semantic decision rather than a mechanical one, and each has consequences past this diff:
- Withhold
supportswhen the payload differs. The narrowest fix, and it preserves the existing dedup assertions — except that the create stage writes the observation and the evidence row together inattachEvidenceStage, and aneffectsoutcome must declare at least one effect. "Observation recorded, not attached" is not currently an expressible outcome, so this means a new effect or payload kind in a contract-tested protocol (claim_operation_effects, the result payload kinds,resultEncodingVersion). - Reconcile into a typed revision. Your second suggestion, and probably the right long-term answer, but that is a feature: it needs a merge rule for two payloads that disagree, and it changes what "preserving the first payload" means.
- Tighten
hasExplicitUserEvidencefor revision 1. Fixes the class rather than the instance, but the unconditional revision-1 branch is what lets legitimate independent corroboration — the R7 case, where another source's extracted text differs — support a first revision at all. Requiring an exact content match would break that. - Put the rejection reason into the dedup identity. Closes the hole by making these separate claims, but abandons pair-based dedup, which is deliberate and asserted.
Flagging for the author rather than picking one. The proof above is the useful part: the escalation is reachable today, needs no verification event, and does not depend on anything I changed in this PR.
| if ((request.surface ?? "explicit_search") !== "explicit_search") { | ||
| clauses.push( | ||
| `NOT EXISTS ( |
There was a problem hiding this comment.
The new non-explicit reader filter is not mirrored by ACTIVE_CLAIM_BASE_SQL or countProjectMemoryClaims() in dreamer/task-gates.ts. Consequently, a project containing only active anti-memories has permanently nonzero map, verify, curate, cue-compression, and classification gates/backlogs, while each corresponding maintenance reader returns an empty pool; the scheduler repeatedly starts work that can never drain its gate. Apply the same category exclusion to the maintenance gate predicates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 38f7e6f. Neither counter filtered category: ACTIVE_CLAIM_BASE_SQL gates on project, lifecycle-active, and uniformlyAbsentClaimSql only, and countProjectMemoryClaims takes no surface at all, so both counted anti-memory while the readers behind those lanes excluded it.
Fixed by pulling the predicate into one shared fragment rather than writing it a third time, since the failure mode here is precisely the two sides drifting:
export function antiMemoryClaimSql(revisionExpr: string): stringIt now backs the reader filter in storage-claim-current-state.ts (replacing the inline NOT EXISTS and its bound parameter) plus both counters. countProjectMemoryClaims has exactly one caller — countActiveMemories, which feeds only gates and backlog telemetry — so the exclusion is unconditional there rather than surface-dependent.
Regression test seeds a project holding nothing but an anti-memory and asserts {pending: 0, total: 0} for curate, verify, and map, plus classify-memories not gating open.
A lost race to extend a TTL reported a caller defect instead of a stale outcome. The forward-progress check ran before the stage validated the token, so a client holding a superseded token — one whose requested expiry was forward progress from its own snapshot but behind the concurrent winner's — got `ClaimOperationInputError` and no receipt, where the operation contract calls for a zero-effect stale result it can retry with a fresh token. Forward progress is now judged only for a token that is still current; a superseded one falls through to the stage, which reports stale. The reader excludes anti-memory from every surface except explicit search, but the maintenance gate predicates did not mirror it. A project holding only active anti-memory therefore reported permanent backlog for map, verify, curate, cue compression, and classification while each corresponding reader returned an empty pool, so the scheduler would reopen work that could never drain. Both counters now apply the same exclusion through one shared SQL fragment, so the reader and the gates cannot drift apart again.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38f7e6f130
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }), | ||
| }, | ||
| () => { | ||
| const staged = stageCreateProjectMemoryClaimInCurrentTransaction( |
There was a problem hiding this comment.
When the same trigger/strategy is observed after its 90-day expiry, this generic create stage still finds the old claim because its dedup query checks only lifecycle_state = 'active'; expiry does not change that lifecycle state. The stage therefore merely attaches evidence to revision 1 and leaves its old expires_at unchanged, so the call reports applied while every visibility surface continues treating the newly re-observed warning as expired. The typed create path should detect an expired dedup holder and append a payload-bearing revision with a fresh retention window, or otherwise exclude expired holders from this dedup operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced. Probe: create at nowMs = 1_000, then re-observe the same trigger and strategy one day past the 90-day window.
second outcome: applied, kind: evidence_attached
same claim: true
expiry before: 7776001000
expiry after: 7776001000 <- unchanged
still expired at the re-observation time: true
So the call succeeds, the observation is recorded, and the re-observed warning stays invisible on every surface. Your reading of the cause is right: the dedup query filters lifecycle_state = 'active', expiry is carried in claim_memory_revision_attributes.expires_at and never touches lifecycle state, so an expired holder is still a dedup target — while uniformlyAbsentClaimSql treats it as absent for readers. Dedup and visibility disagree about whether the claim exists.
This is the third finding in this round that shares one root cause, so I want to name it rather than patch it a third time. createAntiMemory delegates dedup to stageCreateProjectMemoryClaimInCurrentTransaction, and that stage decides — invisibly to the caller — between "create" and "attach evidence to the holder", using signals that cannot express anti-memory's semantics:
- lifecycle-only holder matching, so an expired holder absorbs a fresh observation (this comment);
- hash-subset equality treated as payload corroboration, so an explicit-user record authorizes a different stored payload (3873009142);
- rendered-content equality as the change signal, so a payload-only change is dropped (3873157346).
Each is a different consequence of the typed writer not owning a decision that depends on the typed payload. Three independent special cases threaded into shared, contract-tested code would be worse than one honest change: have the typed create resolve the holder itself — it can compute the dedup hash — and choose explicitly among create, refresh-with-a-payload-bearing-revision, and attach. That also gives the expired case somewhere natural to live, which is the behaviour you're asking for.
Leaving all three for that change rather than picking at them here. The proofs are the durable part, and none of them depend on anything in this PR's later commits.
| db, | ||
| { | ||
| token: args.input.token, | ||
| content: renderAntiMemoryContent(payload), |
There was a problem hiding this comment.
When two distinct structured payloads render to the same text, this generic revision stage considers the request unchanged and only attaches evidence, so no new payload row is inserted. For example, rejectionReason: "why\nSafer alternative: use X" with no alternative renders identically to rejectionReason: "why" plus saferAlternative: "use X"; revising from the former to the latter reports applied but readAntiMemory() still returns the former payload and revision. Since payload fields accept embedded newlines, the typed writer must compare the structured payload or force a revision whenever its payload differs rather than using rendered content equality as the sole change signal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced, including your exact collision:
renderAntiMemoryContent(collide) === renderAntiMemoryContent(split) -> true
revise collide -> split: applied, kind: evidence_attached
stored reason: "why\nSafer alternative: use X" <- unchanged
stored saferAlternative: null <- unchanged
revision: 1
The renderer emits Label: value lines with no escaping, so a field carrying a newline can synthesize a line that looks like another field, and rendered-content equality then reports two different structured payloads as the same request.
Worth adding that the same ambiguity is a spoofing vector independent of change detection: a model-supplied rejectionReason can inject a Safer alternative: <text> line that agents read in the rendered content while readAntiMemory() reports saferAlternative: null. The structured payload and the text agents actually see disagree.
Two directions, and both are decisions past this diff:
- Forbid newlines and control characters in payload fields. Contained, and it closes the spoofing vector too, but it decides that fields like
observedFailureandrecoverymay not carry multi-line text — a stack trace or a numbered recovery sequence being the obvious cases. That is a product call, not a mechanical one. - Make the typed writer compare structured payloads and force a revision when they differ. The right shape, and the same change 3873157334 and 3873009142 need, since all three come from the typed writer delegating a payload-dependent decision to a stage that only sees rendered bytes.
Grouping this with those two rather than fixing it in isolation. If you take the escaping route instead, an unambiguous render would fix the change signal and the spoof together without restricting content.
1 participant
Summary
Rejected approaches now have a separate typed persistence model with immutable events, bounded retention, and explicit archive, restore, and retirement transitions. They cannot enter positive memory, promotion, or mural paths.
This is the foundation of a four-PR stack that records failed approaches and warns agents before they repeat them. PR #76 adds the Rust boundary, PR #77 adds writers, and PR #78 adds warning retrieval and lifecycle verification.
Design
Validation
bun run check:allNew concepts
Anti-memory
Anti-memory stores approaches that failed and the conditions that made them fail. It is separate from positive memory because a rejected approach must never become advice through a broad memory read. Use it for specific, evidenced failures. Do not use it for preferences, tentative ideas, or ordinary task history.
Stack created with GitHub Stacks CLI • Give Feedback 💬