feat(memory): capture rejected approaches - #77
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
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: e780870ba5
ℹ️ 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 category = requireTaxonomyCategory(args.category?.trim()); | ||
| const antiArm = category === ANTI_MEMORY_CATEGORY || args.antiMemory !== undefined; | ||
| if (antiArm) { |
There was a problem hiding this comment.
When revise supplies only changed content, this check classifies the request as a positive-memory write without considering the target claim's stored category. If the token belongs to a REJECTED_APPROACH claim, the generic revision path preserves that category but does not insert a claim_anti_memory_revision_payloads row; subsequent reads then throw ClaimGraphCorruptionError. Require the structured antiMemory payload whenever the target is already an anti-memory.
Useful? React with 👍 / 👎.
| FROM compartment_events events | ||
| JOIN session_projects projects ON projects.session_id = events.session_id | ||
| LEFT JOIN compartments |
There was a problem hiding this comment.
When OpenCode and Pi contain the same session_id but bind it to different projects—a case supported by the (session_id, harness) key—this join associates each event with both bindings because it ignores events.harness. The corresponding pending-count query has the same join, so one project's retrospective can harvest another project's correction into its durable memory and globally receipt the event before the owning project processes it. Join on both session ID and harness.
Useful? React with 👍 / 👎.
| ? stageCreateAntiMemoryInCurrentTransaction( | ||
| db, | ||
| { | ||
| projectId, | ||
| payload: item.value.payload, |
There was a problem hiding this comment.
Once this path creates a REJECTED_APPROACH claim, readDreamerProjectClaims includes it in both hygiene and verification lanes, whose prompts and apply paths still treat every item as one of the five positive categories. A curator or verifier update therefore calls the generic revision stage, retains the anti-memory category, and creates a revision without a typed payload row; later reads fail with ClaimGraphCorruptionError. Either filter anti-memories out of those lanes or add typed anti-memory handling before enabling these creation paths.
Useful? React with 👍 / 👎.
| for (const event of getProjectCompartmentEvents( | ||
| args.db, | ||
| args.projectIdentity, | ||
| "trajectory_correction", | ||
| )) { |
There was a problem hiding this comment.
After a project accumulates processed correction events, every later retrospective still loads and iterates the entire historical set because this reader does not apply the receipt predicate used by countPendingCorrectionEvents. For each valid old event the loop also reruns payload validation, message-history corroboration SQL, and receipt replay while holding the lease-guarded write transaction, so runtime and lock duration grow without bound even when only one new correction is pending. Fetch only events without a matching consumer receipt.
Useful? React with 👍 / 👎.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| content: "string", | ||
| category: { type: "enum", values: V2_MEMORY_CATEGORIES }, | ||
| category: { type: "enum", values: WRITABLE_MEMORY_CATEGORIES }, | ||
| antiMemory: { |
There was a problem hiding this comment.
CRITICAL: unwrapImitatedReducedArgs fails when antiMemory contains optional fields
validObjectField in unwrap-imitated-reduced-args.ts validates objects by requiring exact key count equality (Object.keys(record).length !== declared.length). Declaring only 3 fields here causes any decoded imitated reduced tool call that supplies optional/nullable fields (such as saferAlternative, preconditions, rootCause, etc.) to fail validation. The unparsed wrapper is returned and args.action evaluates to undefined, rejecting valid agent tool calls.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| content: "string", | ||
| category: { type: "enum", values: V2_MEMORY_CATEGORIES }, | ||
| category: { type: "enum", values: WRITABLE_MEMORY_CATEGORIES }, | ||
| antiMemory: { |
There was a problem hiding this comment.
CRITICAL: unwrapImitatedReducedArgs fails when antiMemory contains optional fields
validObjectField validates decoded objects by asserting Object.keys(record).length === declared.length. Because only trigger, rejectedStrategy, and rejectionReason are declared in this schema, any imitated reduced argument payload containing valid optional fields (like saferAlternative) fails unwrapping, leaving rawAction as undefined and throwing an action error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| function childText(inner: string, tag: string): string | null { | ||
| const match = inner.match(new RegExp(`<${tag}>([\\s\\S]*?)<\\/${tag}>`, "i")); |
There was a problem hiding this comment.
WARNING: Exact tag match in childText drops fields formatted with attributes or whitespace
Matching <${tag}> without allowing for tag attributes or trailing whitespace causes any XML tag produced by the model with whitespace (e.g. <trigger >) or optional attributes (e.g. <safer_alternative note="...">) to fail to match. When required fields like trigger, rejected_strategy, or rejection_reason fail extraction, the learning is silently discarded. Consider matching <${tag}\b[^>]*>([\s\S]*?)<\/${tag}>.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| projectPath, | ||
| options.retrospectiveWatermarkMs ?? null, | ||
| ); | ||
| const pending = pendingSessions + countPendingCorrectionEvents(db, projectPath); |
There was a problem hiding this comment.
WARNING: Unbounded backlog count when retrospectiveWatermarkMs option is omitted
When getDreamTaskBacklog is called without explicit options (such as in status queries), options.retrospectiveWatermarkMs defaults to null, causing countProjectSessionsSince(db, projectPath, null) to count all historical sessions across the project's lifetime instead of falling back to the persisted watermark from getTaskScheduleState.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (16 files)
Fix these issues in Kilo Cloud Reviewed by gemini-3.7-flash · Input: 231.8K · Output: 22.6K · Cached: 1.4M |
1 participant
Summary
Correction and retrospective flows can now record a failed approach through one typed
ctx_memoryoperation. Writers preserve the rejected approach, reason, conditions, evidence, and replacement guidance without routing any of it through positive claims.This is the third PR in the anti-memory stack. It builds on PR #75 and PR #76; PR #78 consumes these records as warn-only search results.
Design
Validation
bun run check:allStack created with GitHub Stacks CLI • Give Feedback 💬