Repository navigation
fix(recall): show untyped memories as [general] instead of null - #217
Conversation
The display line concatenated m.memory_type in SQL, and NULL concatenation yields NULL, so memories without a type surfaced as a bare null in proactive surfacing, recall and the session/subagent context blocks. COALESCE to the schema default 'general' at all 8 sites, and have the proactive-surface hook skip results without a display.
|
@codex review |
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. |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
|
@codex review |
![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: c53330c2c1
ℹ️ 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".
`memory_type NOT IN (...)` and `memory_type != '...'` evaluate to NULL for a row without a type, so the keyword, cold-semantic, session-start and subagent queries dropped untyped memories before the COALESCE display fix could format them, and the feedback digest never decayed or archived them. Add an IS NULL branch to every type exclusion, COALESCE the two remaining unqualified and graph-expansion concatenations, and widen the regression scan to unqualified columns and exclusion predicates. A behavioral SessionStart test covers the project and universal prefetch paths.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2743e1738
ℹ️ 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".
memory-browse.sh built its delete preview and type stats with `|| memory_type ||`, which is NULL for an untyped row: the preview came back empty, so `delete` reported "No active memory found" and an untyped memory could not be removed, and the stats printed a blank line for that group. COALESCE both (`general` for the preview, `none` for the stats group, matching the OpenCode plugin's stats label) and fix the same concatenation in the OpenCode plugin's graph expansion, rebuilding dist from src. The regression scan now covers hooks and scripts subdirectories and the OpenCode TypeScript sources, and matches any concatenation touching the column rather than only a leading '['. A behavioral test runs memory-browse.sh stats and delete against an untyped row.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19575dee75
ℹ️ 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".
The proactive-surface hook dropped results without a display only when rendering them. The hit count and the dedup-ledger ids still came from the unfiltered response, so an all-null response injected an empty header, spent the per-session token budget and recorded memories as surfaced although their content was never shown. Filter `.results` once right after the ok check and derive the count, the injected lines and the ledger ids from that subset. The test runs the hook's own jq programs in order against a mixed response and an all-null one.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5217bc7ed2
ℹ️ 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".
The recall daemon and the resurface, topic-shift and cross-session pickers serialized an untyped memory's type as '', and memory-retrieval.sh rebuilds the Q4 display from that field, so those memories rendered as "[|src:...]" even though the daemon's own display said "[general]". The MCP session-context, quality and stats outputs and the surfacing engine interpolated the raw value and printed "[None]". Send `general` from the daemon and the pickers, fall back to `general` in the retrieval hook for a daemon started before this fix, and print `general` (or `none` for stats groups) in the Python displays. New tests cover the recall field, the resurface picker and the surfacing context, and a scan rejects f-string interpolation of the raw column and `'memory_type': ... or ''` serialization.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd19b68f37
ℹ️ 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".
memory_refine validated candidates with str(c.get("memory_type", "general")). A .get() default only applies to a missing key, so a candidate sent with "memory_type": null became the string "None" before the display fallback could run, in both the formatted response and the returned JSON; the refine pipeline's sanitizer did the same. Use `or "general"` (and `or ""` for tags) in both places. The test sends an explicit null type and tags through memory_refine with a fake refiner.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c52174cc94
ℹ️ 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".
getSessionContext() copied the raw memory_type into its project and universal rows, and the OpenCode session-start hook rendered them with `[${m.memory_type}]`, so an untyped memory printed as "[null]". Fall back to "general" where the rows are built and where they are rendered, type the column as nullable, and rebuild dist from src. A scan of the plugin's TypeScript rejects template interpolation of the raw column and copying it into a row unchanged.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1787ce0fe
ℹ️ 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".
…t recall Opening the summary exclusions to untyped rows also let in an untyped row tagged session-summary, which is still a summary: the feedback digest would decay it after seven days and mark it dormant after sixty, and subagent cold recall would inject it. In those three predicates the untyped branch now requires that the row does not carry the session-summary tag; typed rows behave as on main. session-start and memory-retrieval already check the tag on every row. A test requires every hook statement that opens a summary exclusion to untyped rows to check the tag, and the predicate scan now accepts the IS NULL branch on the line before the exclusion.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49697ecff7
ℹ️ 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".
memory-retrieval.sh rewrites every final result as "[type|src:...|imp:...|proj:...]" from one last query, which selected the raw m.memory_type, so an untyped memory came out as "[|src:...]" again after the earlier fallbacks. COALESCE it to 'general' in that query. The test extracts the query from the hook and runs it against an untyped row.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47c5eacb8
ℹ️ 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".
Graph expansion filtered neighbours only on deleted_at. A typed session_summary or progress neighbour was already injected on main; an untyped neighbour carrying the session-summary tag used to vanish only because its NULL display dropped the line, and the COALESCE in this PR turned it into an injected "[general|...]" summary. Apply the primary retrieval paths' exclusion (type not session_summary/progress, tag not session-summary) to graph neighbours in memory-retrieval.sh and in the OpenCode plugin, rebuilding dist. The test runs the hook's graph query as written against an untyped, a tagged-summary and a typed-summary neighbour.
|
@codex review |
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # CHANGELOG.md
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f336e07ea2
ℹ️ 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".
Only stats and delete normalized the type; list, search, show and project rendered an untyped memory as "[]" and types printed a blank group. Use `general` for the row commands and `none` for the types group, matching stats. The test now runs list, search, show, project and types against an untyped row; without the change list prints "abcdef01 []".
LoCoMo bench — skippedlocomo10.json fixture not available on this runner The regression gate runs against the monthly scheduled bench and explicit |
Summary
A memory stored without a
memory_typewas mishandled in five different ways, all from the same NULL:'[' || m.memory_type || '] ' || contentis NULL when the type is NULL, so the memory surfaced as a barenullline and its content was lost (proactive surfacing, recall, semantic search, session-start and subagent blocks, graph expansion,memory-browse.shstats and delete preview, the OpenCode plugin's graph expansion), and the OpenCode session-start blocks rendered[null].memory-browse.sh deletecould not delete an untyped memory at all: the empty preview made it exit with "No active memory found".memory_type NOT IN (...)andmemory_type != '...'are NULL for an untyped row, so the hooks' keyword, cold-semantic, session-start and subagent queries dropped untyped memories entirely, and the feedback digest never decayed or archived them. The daemon, long-session and MCP server queries onmainalready usememory_type IS NULL OR ...; the hooks now match that convention, except that in the hooks an untyped row taggedsession-summaryis still excluded as a summary.'', whichmemory-retrieval.shrendered as[|src:...]; the MCP session-context, quality and stats outputs and the surfacing engine printed[None], andmemory_refineturned an explicit JSONnulltype into the string"None".deleted_at, so session summaries (typed, or untyped with the tag once displayed) were injected; they now get the primary retrieval exclusion, in the hook and the OpenCode plugin.On one real database, 234 of about 16k active memories have no
memory_type; 192 of them are legacy session summaries written by an older Codex adapter (the current adapter types and tags them).Untyped memories now display and serialize as
general(the schema default), stats group them asnone, type filters keep them, and proactive surfacing filters.resultsonce before counting, injecting and recording.memory-retrieval.shalso maps an empty type togeneralfor a daemon started before this change.Size
Net diff is 401 lines, 101 over the 300 cap. All five parts come from the same NULL and the review rounds found them one layer at a time; splitting them would leave the type-filter fix shipping displays that still print
[|src:...]or[None], so they are kept together.Test plan
scripts/tests/test_null_memory_type_display.py:_recall/_semantic_searchdisplays and the recallmemory_typefield; the resurface picker; the surfacing engine context;memory_refinewith an explicit null type and tags; the proactive-surface hook's own jq programs (filter, count, display, ledger ids) on a mixed and an all-null response; repo-wide scans for unguarded SQL concatenation (hooks, scripts, OpenCode TypeScript, any position), unguarded type exclusions, f-string interpolation of the raw column and'memory_type': ... or '', and in the OpenCode TypeScript${m.memory_type}interpolation or copying the raw column into a rowtests/hooks/test_session_start_untyped_memory.py: the real SessionStart hook prefetches untyped project and universal memories as[general] ...tests/hooks/test_browse_untyped_memory.py:memory-browse.sh statsanddelete(preview and soft-delete) on an untyped rowbash -non the changed hooks; 112 related tests inscripts/teststhat open no processes or sockets pass locally; full suite in CIdist/index.jsrebuilt fromsrc(a fresh build of the previoussrcmatched the committed dist byte for byte)bun testnot run locally (itsbetter-sqlite3dependency is not installed here); the plugin changes are covered by the static scansDocs sync
README.md,docs/architecture.md: not neededCHANGELOG.mdentry under Unreleased / FixedAnti-Claude trailer check
Co-Authored-BytrailersDependencies