fix(pi): treat an empty mem_search as no results, not an outage - #655
fix(pi): treat an empty mem_search as no results, not an outage#655jacargentina wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe Pi plugin records successful Engram responses, distinguishes reachable empty responses from unreachable failures, renders null ChangesPi mem_search reachability and result handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/pi/index.ts`:
- Around line 840-847: Update the mem_search handling around engramFetch so a
null result is converted to [] only when the 2xx response contained valid parsed
JSON representing a no-hit result. Preserve and surface JSON parse failures
instead of allowing them to pass through the data === null branch as an empty
search result; keep the existing transport-failure check using
takeLastFetchReachedServer and unreachableMessage unchanged.
- Around line 193-211: Remove the module-global fetch status variables and make
reachability and timeout metadata per-operation. Update engramFetch and the
callMemoryTool/executeMemoryTool flow to return and consume this metadata
through the current operation’s result, ensuring concurrent requests cannot
overwrite each other’s status.
In `@plugin/pi/test/native-tool-contract.test.mjs`:
- Around line 101-131: Update the test helper around installRuntimeStubs and its
finally cleanup so it never recursively removes the repository’s
plugin/pi/node_modules directory. Use an isolated temporary fixture for stub
packages, or track and restore only the package paths created by this test,
while preserving cleanup of the test-owned resources and restoring environment
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 470692e1-e8b8-48ca-a7d0-60d741652341
📒 Files selected for processing (3)
plugin/pi/index.tsplugin/pi/test/index-source.test.mjsplugin/pi/test/native-tool-contract.test.mjs
| if (data === null && !takeLastFetchReachedServer()) { | ||
| // A null with no 2xx behind it is a genuine transport failure (unreachable/timeout), not an | ||
| // empty result. Surface it as an outage; a reachable-but-empty response falls through. | ||
| throw new Error(unreachableMessage(takeLastFetchTimeoutMethod())); | ||
| } | ||
| const result = { content: [{ type: "text" as const, text: textResult(data) }], details: { data } }; | ||
| // A no-hit search comes back as a null JSON body; render it as an empty list, not "{}". | ||
| const rendered = data === null && toolName === "mem_search" ? [] : data; | ||
| const result = { content: [{ type: "text" as const, text: textResult(rendered) }], details: { data: rendered } }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)plugin/pi/index\.ts$|plugin/.*pi.*' || true
echo "== outline =="
ast-grep outline plugin/pi/index.ts --view expanded | head -200 || true
echo "== relevant sections =="
sed -n '220,260p' plugin/pi/index.ts
echo "---"
sed -n '820,860p' plugin/pi/index.ts
echo "== json-related search =="
rg -n "res\.json|json\(|no-hit|mem_search|unreachable|timeout|Content-Type|parsed|parse" plugin/pi/index.tsRepository: Gentleman-Programming/engram
Length of output: 12461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== callMemoryTool section =="
sed -n '667,830p' plugin/pi/index.ts
echo "== parse failure shape probe =="
python3 - <<'PY'
values = [
("valid null", None),
("invalid JSON", "abc123"), # would raise on res.json() and be coerced to null in current code
]
for name, data in values:
rendered = data if data is not None else []
print(f"{name}: input={name} -> rendered={rendered}")
PYRepository: Gentleman-Programming/engram
Length of output: 7168
Preserve JSON parse failures from search results.
engramFetch maps any res.json() failure for a 2xx response to null, so mem_search can render malformed or empty JSON as [] instead of showing the protocol error. Only treat data === null as an empty no-hit result when it comes from valid parsed JSON.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/pi/index.ts` around lines 840 - 847, Update the mem_search handling
around engramFetch so a null result is converted to [] only when the 2xx
response contained valid parsed JSON representing a no-hit result. Preserve and
surface JSON parse failures instead of allowing them to pass through the data
=== null branch as an empty search result; keep the existing transport-failure
check using takeLastFetchReachedServer and unreachableMessage unchanged.
| await installRuntimeStubs(); | ||
| const registeredTools = new Map(); | ||
| const pluginUrl = pathToFileURL(join(ROOT, "index.ts")); | ||
| pluginUrl.search = `?contract=${Date.now()}-${Math.random()}`; | ||
| const { default: registerEngram } = await import(pluginUrl.href); | ||
| registerEngram({ | ||
| registerTool(tool) { | ||
| registeredTools.set(tool.name, tool); | ||
| }, | ||
| on() {}, | ||
| }); | ||
|
|
||
| const memSearch = registeredTools.get("mem_search"); | ||
| assert.ok(memSearch, "mem_search tool should be registered"); | ||
|
|
||
| return await memSearch.execute( | ||
| "tool-call-search", | ||
| { query: "state markers", project: "gentle-agent-state" }, | ||
| undefined, | ||
| undefined, | ||
| { | ||
| cwd: ROOT, | ||
| sessionManager: { getSessionId: () => "test-session" }, | ||
| ui: { setStatus() {} }, | ||
| }, | ||
| ); | ||
| } finally { | ||
| globalThis.fetch = originalFetch; | ||
| if (originalUrl === undefined) delete process.env.ENGRAM_URL; | ||
| else process.env.ENGRAM_URL = originalUrl; | ||
| await rm(NODE_MODULES, { recursive: true, force: true }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not delete the repository’s node_modules.
This helper overwrites stub package paths and then recursively removes plugin/pi/node_modules at Line 131. Running the test in an installed workspace can destroy unrelated dependencies and interfere with concurrent tests. Use an isolated fixture or back up and restore only the package paths created by this test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/pi/test/native-tool-contract.test.mjs` around lines 101 - 131, Update
the test helper around installRuntimeStubs and its finally cleanup so it never
recursively removes the repository’s plugin/pi/node_modules directory. Use an
isolated temporary fixture for stub packages, or track and restore only the
package paths created by this test, while preserving cleanup of the test-owned
resources and restoring environment state.
felipe3dfx
left a comment
felipe3dfx
left a comment
There was a problem hiding this comment.
Verdict: LGTM — the fix is correct and well tested. Two minor observations below.
Correctness — verified
- The
lastFetchReachedServerflag is reset at the start of everyengramFetchand only set after the status check passes, so it can never carry a stale value from a previous fetch within the same tool call. - The
mem_savetwo-leg case (ensureSession+ POST) is intact: ifensureSessionsucceeds but the POST fails, the POST already reset the flag, and timeouts still surface the "do not blindly retry" message. Thenullreturn contract ofengramFetchis unchanged for all call sites. - The self-consuming
takeLastFetch...()accessor mirrors the existinglastFetchTimeoutMethodpattern, so it's consistent with the file's style.
Observations
-
The behavior change is broader than the title suggests. The
data === null && !takeLastFetchReachedServer()guard applies to all tools, not justmem_search. A2xxwith anullbody onmem_context,mem_delete, etc. was previously reported as an outage and now renders as success with"{}"text (viatextResult). That's probably the right call (a2xxis a success), but it deserves a mention in the PR description — onlymem_searchgets the special[]rendering. -
Unused
reachedServergetter in the test fixture.index-source.test.mjsexposesreachedServer: () => lastFetchReachedServerbut no test asserts on it (theletdeclaration itself is needed so the extractedengramFetchbody compiles). Either add an assertion exercising the flag directly, or drop the getter. -
Note, non-blocking: the flag is module-level mutable state; two concurrent tool calls could clobber each other. Same pre-existing trade-off as
lastFetchTimeoutMethod, and Pi tool calls are sequential in practice, so no change requested here.
Test coverage
Good: the four new cases in native-tool-contract.test.mjs cover 200 null, 200 [], populated results, and a 500 with a server error. The "unreachable server → outage" case was already covered by the pre-existing test in the same file, so the matrix is complete.
engramFetch returns null both for an unreachable server and for a reachable server that answers a request with a 2xx and a null JSON body. The Engram HTTP API returns exactly such a null for a no-hit /search, so executeMemoryTool misclassified every empty search as "could not reach the Engram HTTP server", forcing harnesses to fall back from Engram-backed to file-backed artifacts even though the store was healthy. Record whether the last engramFetch actually got a 2xx back (lastFetchReachedServer) as an out-of-band signal, mirroring the existing lastFetchTimeoutMethod pattern. The tool layer now only reports an outage when a null came with no response behind it; a reachable-but-empty response falls through and mem_search renders it as an empty list. engramFetch keeps its null return contract, so the ~20 call sites and the mem_save two-leg no-duplicate-write guarantee are unaffected. Refs Gentleman-Programming#473
aff951a to
1a6dc8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/pi/index.ts`:
- Around line 193-211: Refactor engramFetch and its callers to return an
explicit fetch result envelope that distinguishes successful 2xx responses,
reachable null/empty bodies, transport failures, and timeouts. Remove the
module-level lastFetchReachedServer state and takeLastFetchReachedServer helper,
propagate the result through the relevant tool flow, and keep the adapter
focused on parsing inputs, invoking the core API, and returning the explicit
result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 790373f9-000f-4d50-995c-4c1bc9fa1228
📒 Files selected for processing (3)
plugin/pi/index.tsplugin/pi/test/index-source.test.mjsplugin/pi/test/native-tool-contract.test.mjs
| // Whether the most recent engramFetch actually got a 2xx back from the server. engramFetch returns | ||
| // null both for an unreachable server and for a reachable server that answered with a null JSON body | ||
| // (an empty result set); this out-of-band flag lets the tool layer tell those two apart. | ||
| let lastFetchReachedServer = false; | ||
|
|
||
| function takeLastFetchReachedServer(): boolean { | ||
| const reached = lastFetchReachedServer; | ||
| lastFetchReachedServer = false; | ||
| return reached; | ||
| } | ||
|
|
||
| async function engramFetch<TResponse = unknown>(path: string, opts: FetchOptions = {}): Promise<TResponse | null> { | ||
| const method = opts.method ?? "GET"; | ||
| // This call's outcome supersedes any earlier one. A tool call can issue several fetches | ||
| // (mem_save creates the session, then writes the observation); without this reset a timeout | ||
| // on the first leg would mislabel an unrelated failure on the second as "may already have | ||
| // been applied", telling the agent not to retry a write that never left the machine. | ||
| lastFetchTimeoutMethod = undefined; | ||
| lastFetchReachedServer = false; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'engramFetch|lastFetchReachedServer|executeMemoryTool' plugin/pi/index.ts
fd -t f -e go . -0 | xargs -0 -r rg -n -C 3 'mem_search|/search|Engram|unreachable|reachab' || trueRepository: Gentleman-Programming/engram
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- plugin/pi/index.ts relevant sections ---\n'
sed -n '1,290p' plugin/pi/index.ts
sed -n '820,905p' plugin/pi/index.ts
printf '\n--- tool dispatch and result handling mentions ---\n'
rg -n -C 4 'executeMemoryTool|resultToString|lastFetch|takeLastFetch|out-of-band|HTTP|outage|server unreachable|null JSON' plugin/pi/index.tsRepository: Gentleman-Programming/engram
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- plugin/pi/index.ts relevant sections ---'
sed -n '1,290p' plugin/pi/index.ts
sed -n '820,905p' plugin/pi/index.ts
printf '%s\n' ''
printf '%s\n' '--- tool dispatch and result handling mentions ---'
rg -n -C 4 'executeMemoryTool|resultToString|lastFetch|takeLastFetch|out-of-band|HTTP|outage|server unreachable|null JSON' plugin/pi/index.tsRepository: Gentleman-Programming/engram
Length of output: 22162
Move fetch result envelopes out of the adapter.
engramFetch hides reachable empty responses and transport failures behind null while storing the last-server-reached/timeout state in module globals. Return explicit result states from the core API or tool instead; then this adapter can stay limited to input parsing, calling core, and returning the result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/pi/index.ts` around lines 193 - 211, Refactor engramFetch and its
callers to return an explicit fetch result envelope that distinguishes
successful 2xx responses, reachable null/empty bodies, transport failures, and
timeouts. Remove the module-level lastFetchReachedServer state and
takeLastFetchReachedServer helper, propagate the result through the relevant
tool flow, and keep the adapter focused on parsing inputs, invoking the core
API, and returning the explicit result.
Source: Path instructions
2 participants
🔗 Linked Issue
Closes #473
🏷️ PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance, dependencies, toolingtype:breaking-change— Breaking change📝 Summary
2xxresponse with a JSONnullbody.mem_searchresponses render as an empty list instead of reporting an Engram outage.📂 Changes
plugin/pi/index.tslastFetchReachedServeras an out-of-band signal and handles reachable empty search resultsplugin/pi/test/index-source.test.mjsplugin/pi/test/native-tool-contract.test.mjs🧪 Test Plan
go test ./...go test -tags e2e ./internal/server/...npm test --prefix plugin/pi🤖 Automated Checks
Closes #N/Fixes #N/Resolves #Nstatus:approvedlabeltype:*labelgo test ./...passesgo test -tags e2e ./internal/server/...passes✅ Contributor Checklist
type:bug.go test ./....go test -tags e2e ./internal/server/....Co-Authored-Bytrailers in commits.💬 Notes for Reviewers
engramFetchretains its existingnullreturn contract, so existing call sites and themem_savetwo-legno-duplicate-write behavior remain unchanged.
The new
lastFetchReachedServersignal distinguishes a reachable2xxresponse with a JSONnullbody from a genuinetransport failure. Empty
mem_searchresults now render as[], while unreachable servers and non-2xx responses continue tosurface errors.
Summary by CodeRabbit
Bug Fixes
null.Tests