Skip to content

fix: pre-release hardening for 0.9.22 — regex correctness + MCP env guard#588

Merged
rohitg00 merged 1 commit into
mainfrom
fix/pre-release-hardening
May 21, 2026
Merged

fix: pre-release hardening for 0.9.22 — regex correctness + MCP env guard#588
rohitg00 merged 1 commit into
mainfrom
fix/pre-release-hardening

Conversation

@rohitg00
Copy link
Copy Markdown
Owner

@rohitg00 rohitg00 commented May 21, 2026

Summary

Pre-release hardening for 0.9.22. Three concerns surfaced auditing PRs merged since v0.9.21:

1. Graph parser regex correctness (real regression from #494)

#494 added self-closing entity support with:

/<entity\s+type="([^"]+)"\s+name="([^"]+)"[^>]*(?:\/>|>([\s\S]*?)<\/entity>)/g

The greedy [^>]* eats the trailing / of a self-closing tag, so the alternation falls through to the explicit-close branch, and [\s\S]*? runs ahead to the next </entity> — merging two entity declarations into one match and silently dropping a node. The test that #494 itself added (graph-extract accepts self-closing entity tags) was failing on main but the failure was masked by being a single test in the suite.

Direct repro:

const re = /<entity\s+type="([^"]+)"\s+name="([^"]+)"[^>]*(?:\/>|>([\s\S]*?)<\/entity>)/g;
const xml = `<entity type="file" name="a.ts"/>
<entity type="function" name="main"><property key="lang">ts</property></entity>`;
// matches once, "file"+"a.ts" with body=`<entity type="function" name="main"><property key="lang">ts</property>`
// expected 2 matches

Fix: switch greedy [^>]* → lazy [^>]*? so the self-closing branch is reached before the explicit-close branch greedily consumes the next entity. Both branches now match correctly; #494's test passes; allows attribute values containing / (e.g. path="a/b/c").

2. MCP env-var literal-placeholder guard

#386 ships ${AGENTMEMORY_URL} / ${AGENTMEMORY_SECRET} in plugin/.mcp.json. Claude Code and Cursor expand those at MCP launch; hosts that don't expand pass the literal string through. A truthy literal "${AGENTMEMORY_URL}" defeats the || DEFAULT_URL fallback in src/mcp/rest-proxy.ts:35 — the shim would POST to ${AGENTMEMORY_URL}/agentmemory/... (DNS failure).

Fix: new exported resolveEnvOrEmpty(name) in rest-proxy.ts that returns "" for any value matching /^\$\{.*\}$/. baseUrl() and authHeader() route through it. Mirrored in src/mcp/standalone.ts's mode-announce log line so error messages don't show ${AGENTMEMORY_URL} literal.

3. CHANGELOG for 0.9.22

Entries for all PRs landed since v0.9.21:

Diff

  • src/functions/graph.ts+7/-1 lazy quantifier
  • src/mcp/rest-proxy.ts+16/-2 placeholder guard + helper export
  • src/mcp/standalone.ts+12/-1 displayAgentmemoryUrl() for log line
  • test/mcp-env-placeholder.test.ts+57 8 unit tests on the guard contract
  • CHANGELOG.md+39/-0 unreleased section

131 insertions, 4 deletions across 5 files.

Validation

  • npm test98 test files, 1091 tests pass (was failing on 1 test before this PR — fix: parse self-closing graph entities #494's self-closing test).
  • npm pack + fresh install in clean /tmp/: 142 files shipped, 180 deps resolved, iii-sdk@0.11.2 pinned, plugin/{hooks,opencode,scripts,skills}/ present.

Release readiness

After this lands, 0.9.22 is shippable. Open contributor PRs still recommended for merge before tag:

Or release 0.9.22 with what's already on main + this hardening, defer those to 0.9.23. Your call.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed XML entity parsing regex to properly handle self-closing tags without merging adjacent entity declarations
    • Hardened environment variable placeholder handling to treat unresolved ${...} patterns as empty values, preventing misuse as fallback values
    • Improved MCP server URL display to safely default to localhost when environment variables are missing or unresolved
  • Tests

    • Added test suite verifying correct behavior of environment variable placeholder resolution
  • Documentation

    • Updated CHANGELOG with new Unreleased section documenting recent fixes and improvements

Review Change Stack

…uard

Three concerns surfaced when auditing PRs merged since v0.9.21:

1) Graph parser regex from #494 was correct for self-closing tags ONLY
   when they're the only entity in the document. With a self-closing
   entity followed by an explicit-close entity, the greedy `[^>]*` ate
   the trailing `/` of the self-close, the alternation fell through to
   the explicit-close branch, and `[\s\S]*?` ran ahead to the *next*
   `</entity>` — merging two entity declarations into one match and
   silently dropping a node. Switch to lazy `[^>]*?` so the
   self-closing alternation gets first chance. Test from #494 (which
   was failing on main but I didn't notice) now passes.

2) #386 shipped `${AGENTMEMORY_URL}` / `${AGENTMEMORY_SECRET}`
   placeholders in plugin/.mcp.json. Claude Code and Cursor expand
   those at MCP launch time; some hosts pass the literal string
   through. A truthy literal `"${AGENTMEMORY_URL}"` defeats the
   existing `|| DEFAULT_URL` fallback and would have the shim POST to
   `${AGENTMEMORY_URL}/agentmemory/...` (DNS failure). Add a
   defensive guard in src/mcp/rest-proxy.ts that strips any value of
   the form `${...}` so the fallback engages. Mirror in
   src/mcp/standalone.ts's mode-announce log line.

3) CHANGELOG entries for all PRs landed since v0.9.21 (#321, #325,
   #386, #454, #494, #526, #542, #560, #561, #562, #564, #567) plus
   the regex + env-guard hardening here.

Validation:
- npm test → 98 files, 1091 tests pass
- npm pack → 142 files, fresh install clean, iii-sdk@0.11.2 pinned,
  plugin/ shipped with hooks/scripts/skills/opencode/
- New test file covers 8 placeholder cases (unset, empty,
  `${VAR}` literal, mismatched braces, real values with $,
  unclosed `${`, no-braces `$VAR`).
@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agentmemory Ready Ready Preview, Comment May 21, 2026 9:21am

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e6c4dad8-c05f-4f5c-baf9-f9ae89a63a63

📥 Commits

Reviewing files that changed from the base of the PR and between c14bdc5 and fceb12f.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • src/functions/graph.ts
  • src/mcp/rest-proxy.ts
  • src/mcp/standalone.ts
  • test/mcp-env-placeholder.test.ts

📝 Walkthrough

Walkthrough

This PR fixes environment placeholder handling in MCP configuration and XML entity parsing in graph functions. It introduces resolveEnvOrEmpty() to treat unexpanded ${...} placeholder strings as empty, refactors the MCP proxy and logging to safely fall back to defaults, and fixes a greedy regex that could merge self-closing XML tags. All changes are covered by tests and documented in the changelog.

Changes

Bug Fixes and Hardening

Layer / File(s) Summary
Core environment placeholder resolution and auth fallback
src/mcp/rest-proxy.ts, test/mcp-env-placeholder.test.ts
New exported function resolveEnvOrEmpty(name) detects literal ${VAR} placeholder strings and returns empty, preventing placeholders from being used as truthy values. baseUrl() and authHeader() are refactored to use it, so the proxy falls back to DEFAULT_URL and omits the Authorization header when env values are placeholders. Comprehensive test suite validates placeholder detection, real value preservation, and edge cases.
Safe URL display for logging
src/mcp/standalone.ts
New function displayAgentmemoryUrl() safely derives the MCP server URL for log output, falling back to http://localhost:3111 when AGENTMEMORY_URL is unset or contains an unexpanded placeholder. announceMode()'s "no server reachable" message is updated to use this function.
XML entity parser self-closing tag fix
src/functions/graph.ts
The parseGraphXml entity regex is updated from greedy to lazy attribute matching ([^>]*?) to prevent consuming the / in self-closing tags and incorrectly merging adjacent entity declarations.
Changelog documentation
CHANGELOG.md
New Unreleased section documents all fixes (env-var placeholder handling, entity tag parsing, SDK pinning, stream forcing, search normalization), additions (benchmark harness, codex hooks, CI matrix), and infrastructure improvements.

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly Related PRs

  • rohitg00/agentmemory#386: Introduces literal "${...}" placeholder strings for AGENTMEMORY_URL and AGENTMEMORY_SECRET in plugin/.mcp.json, which this PR addresses with resolveEnvOrEmpty() and safe fallback logging.

Poem

🐰 With placeholders resolved and XML tags tight,
Environment strings now parse just right,
No greedy matches merge what should stand,
Localhost fallbacks ready at hand.
Tests guard the path, the changelog glows—
One hardened hop closer as agentmemory grows! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: regex correctness for the graph parser and MCP environment variable placeholder guarding.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pre-release-hardening

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rohitg00 rohitg00 merged commit bc64107 into main May 21, 2026
7 checks passed
@rohitg00 rohitg00 deleted the fix/pre-release-hardening branch May 21, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant