Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 295b4abe2d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .prepare("UPDATE memory_records SET writer_id = ?, sequence = ?, value_json = ?, value_sha256 = ?, recorded_at = ? WHERE scope_id = ? AND memory_id = ?") | ||
| .run(this.options.writerId, sequence, valueJson, valueSha256, mutation.recordedAt, this.options.scopeId, mutation.memoryId); |
There was a problem hiding this comment.
When a curated fact is updated, this overwrites the existing row with MAX(sequence) + 1, removing its old sequence from the writer history. A full bundle can then start above sequence 1 or contain a gap and be rejected by the contiguous-sequence checks, while an incremental bundle sent to a reader that already has the old row is rejected as a duplicate memory ID. Consequently, the first UPDATE can make the existing manual bundle transport unusable; updates need an append-only representation that readers can apply.
Useful? React with 👍 / 👎.
| && stored.fact === candidate.fact | ||
| && stored.category === candidate.category | ||
| && (stored.projectTaskId ?? null) === (candidate.projectTaskId ?? null); |
There was a problem hiding this comment.
When an existing record has the same fact/category/task but the new candidate supplies a newer revision, corrected subjectKey, observedAt, supersession links, or evidence/assets, this comparison returns NOOP without considering any of those fields. The quality loop subsequently sees the same memory ID during recovery and can return PASS and SAFE TO DELETE ... YES, even though the newly selected provenance was never stored and is lost if the source is deleted. Treat changed durable metadata as an update or keep deletion safety at NO.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
| if (options.memoryDatabasePath !== undefined) { | ||
| const relativePath = relative(root, memoryDatabasePath); | ||
| if (relativePath.length === 0 || (!relativePath.startsWith("..") && !isAbsolute(relativePath))) { | ||
| throw new InvalidTaskStateError("Memory database path must be outside the configured workspace"); |
There was a problem hiding this comment.
When XDG_DATA_HOME/LOCALAPPDATA is configured inside the target checkout, the default database path skips this containment check entirely; an explicit outside-looking path whose parent is a symlink into the checkout also passes because relative() is only lexical. The first curation then creates the private SQLite database under the repository, where it can be staged or published. Apply a canonical, symlink-aware outside-workspace check to both default and explicit paths before mkdir.
AGENTS.md reference: AGENTS.md:L119-L119
Useful? React with 👍 / 👎.
| projectHint: projectHintSchema.optional(), | ||
| }).strict(); | ||
| const allowedPayloadKeys = new Set(["version", "candidates"]); | ||
| const allowedCandidateKeys = new Set(["candidateId", "memoryId", "fact", "category", "source", "privacyRisk", "revision", "projectTaskId"]); |
There was a problem hiding this comment.
The MCP bridge rejects subjectKey, observedAt, supersedesMemoryIds, evidenceRefs, and assetRefs as unknown fields even though they are now part of CurationCandidate. In particular, after a fact exists, the quality loop requires an explicit supersession pointer for a competing correction, so MCP clients cannot perform that supported update at all; omitting the pointer produces CURRENT_COMPETITION, while including it is rejected before the curator. Add the new candidate fields to this strict allowlist while retaining their downstream validation.
Useful? React with 👍 / 👎.
|
Superseded by clean dependency-complete promotion PR #42; retaining this branch for historical review. |
1 participant
Summary
Verification
Scope
No changes to real user memory, durable task state, product files, package dependencies, or protected worktrees.