fix(pi): treat an empty mem_search as no results, not an outage#655
Open
jacargentina wants to merge 1 commit into
Open
fix(pi): treat an empty mem_search as no results, not an outage#655jacargentina wants to merge 1 commit into
jacargentina wants to merge 1 commit into
Conversation
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
📝 WalkthroughWalkthroughThe Pi plugin now records whether Engram fetches reached the server, distinguishes unreachable failures from reachable empty responses, 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
Comment on lines
+193
to
+211
| // 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.
🎯 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/'
echo "Outline:"
ast-grep outline plugin/pi/index.ts --view expanded || true
echo "Relevant lines 180-280:"
sed -n '180,280p' plugin/pi/index.ts
echo "Search for shared state/readers:"
rg -n "lastFetchReachedServer|takeLastFetchReachedServer|lastFetchTimeoutMethod|engramFetch|executeMemoryTool" plugin/pi/index.tsRepository: Gentleman-Programming/engram
Length of output: 13551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant executeMemoryTool lines 820-890:"
sed -n '820,890p' plugin/pi/index.ts
echo "All occurrences of reachability/timeout markers:"
rg -n "takeLastFetchReachedServer|takeLastFetchTimeoutMethod|lastFetchReachedServer|lastFetchTimeoutMethod|reach.*server|reachable|outage|timeout" plugin/pi/index.ts
echo "Read-only JavaScript probe for interleaving semantics:"
node - <<'JS'
async function interleavingDemo() {
let lastFetchTimeoutMethod;
let lastFetchReachedServer = false;
function takeLastFetchTimeoutMethod() {
const method = lastFetchTimeoutMethod;
lastFetchTimeoutMethod = undefined;
return method;
}
function takeLastFetchReachedServer() {
const reached = lastFetchReachedServer;
lastFetchReachedServer = false;
return reached;
}
async function engramFetch({ name, timedOut, ok, reached }) {
lastFetchTimeoutMethod = undefined;
lastFetchReachedServer = false;
if (timedOut) lastFetchTimeoutMethod = name;
if (!ok) return null;
if (reached) lastFetchReachedServer = true;
return null;
}
// Original async function semantics:
// request A is unreachable/timeout before it has set A's marker;
// request B completes a reachable no-hit response after A sets module-wide marker to false.
const reqALeftOffWithNull = await engramFetch({ name: "A", timedOut: false, ok: false, reached: false });
const beforeRequestBCompletesReached = takeLastFetchReachedServer();
await engramFetch({ name: "B", timedOut: false, ok: true, reached: true });
const afterRequestBCompletesReached = takeLastFetchReachedServer();
console.log(JSON.stringify({
reqALeftOffWithNull,
lastReachedAfterRequestAFailedAndBeforeRequestBCompletes: beforeRequestBCompletesReached,
lastReachedAfterRequestBCompletes: afterRequestBCompletesReached,
}));
}
interleavingDemo().catch(console.error);
JSRepository: Gentleman-Programming/engram
Length of output: 6686
Keep fetch status out of module-global state.
lastFetchReachedServer and lastFetchTimeoutMethod are shared across all tool/lifecycle requests, so a concurrent successful fetch can clear or overwrite the marker for another request before executeMemoryTool reads them. Scope this reachability/timeout metadata to the same call as callMemoryTool, e.g. by returning it through the per-operation 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, 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.
Source: Path instructions
Comment on lines
+840
to
+847
| 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.
Comment on lines
+101
to
+131
| 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 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