fix: encode dynamic URL path segments#5818
Conversation
|
Validation update for this PR. No test files were added or committed; the checks below were run locally against the current PR branch. What was validatedThe URL path-segment encoding behavior was checked across the four changed production files:
Representative IDs containing URL-structural characters were used, for example: Expected encoded path segment: ResultsPython SDKValidated with Covered paths included: Result: PASS. Checked path segments encoded Python CLIValidated by monkeypatching Covered paths included: Result: PASS. TypeScript SDKValidated by loading the TS source with Covered paths included: Result: PASS. Node CLIValidated by mocking Covered paths included: Result: PASS. Local commandsgit diff --check upstream/main...HEADResult: PASS. cd cli/node
./node_modules/.bin/biome.cmd check src/backend/platform.ts
./node_modules/.bin/tsc.cmd --noEmit
./node_modules/.bin/vitest.cmd runResult: PASS. cd mem0-ts
# Verified against the Git blob for this PR, not the local Windows working-tree copy.
node -e "const { execFileSync } = require('child_process'); const prettier = require('./node_modules/prettier'); (async () => { const file = 'src/client/mem0.ts'; const head = execFileSync('git', ['show', 'HEAD:mem0-ts/src/client/mem0.ts'], { encoding: 'utf8', cwd: '..' }); const opts = (await prettier.resolveConfig(file)) || {}; opts.filepath = file; const out = await prettier.format(head, opts); console.log('HEAD blob equals prettier output:', head === out); })();"Result: PASS. Note: a direct local Windows working-tree check initially reported a Prettier warning for Not covered
|
Changes RequestedThe URL encoding logic is correct and the cross-package coverage is solid — this is good, mergeable work once two blocking items are addressed. Strengths
IssuesImportant (should fix before merge)
Minor (optional)
Evidence
AssessmentReady to merge: With fixes — please restore the regression tests (commit 1 has them ready), fix the four line-length violations (two files), and push. I'll re-review once you push. |
|
Thanks for the careful review ? I addressed the blocking points you called out. Updates in this push:
I also reran the relevant checks locally:
On the size of the patch: this commit is |
LGTMBoth blocking items from the prior review are resolved — confirmed by re-running targeted tests locally at e6ed84f. What's FixedRegression tests restored (all four packages):
Line-length violations resolved: Evidence
Remaining (non-blocking, optional)
Neither item is worth holding the PR for. AssessmentReady to squash-merge. Thanks for working through the two rounds — the cross-package coverage and the restored tests make this a solid, maintainable fix. One small ask: would you mind opening a tracking issue for this bug? No linked issue at the moment, and having one makes it easier to trace back if the encoding class comes up again (the |
|
@kartik-mem0 thanks again for the review and for the quick follow-up. I opened and linked the tracking issue here: #5893. Happy to contribute to the project and help with any follow-ups where useful. |
Linked Issue
Fixes #5893. This fixes a URL construction bug found while reviewing SDK and CLI path handling.
Description
What Problem This Solves
Several hosted SDK and CLI methods build API paths by directly interpolating caller-provided IDs into URL path segments:
Those values are identifiers from the SDK or CLI caller's perspective, but
/,?, and#are structural URL characters. If an ID contains one of those characters, the request no longer targets the intended single path segment.For example, deleting a user/entity named:
can construct a path like:
That can be parsed as an extra path segment plus query/fragment syntax instead of one literal entity identifier. The same class of issue applies to memory IDs and event IDs in the affected SDK and CLI paths.
The intended request path is:
This keeps the identifier as a single URL path segment and lets the server receive the literal ID value after decoding.
Change
This PR adds local path-segment encoding helpers and uses them only where dynamic IDs are inserted into hosted API paths.
urllib.parse.quote(str(value), safe="")so/is encoded instead of being preserved byquote()'s default behavior.encodeURIComponent(String(value))for the same single-segment encoding behavior.Affected hosted API paths include:
Evidence
The bug is reproducible by comparing raw interpolation with path-segment encoding:
For memory and event IDs, the same behavior appears with values such as:
This is a URL construction issue in the clients before the request reaches the API. It does not require a server-side behavior change to demonstrate the problem: unencoded path separators and query/fragment delimiters change the request URL semantics.
Possible call chain / impact
Python SDK:
Async Python SDK:
Python CLI:
Node CLI:
TypeScript SDK:
Paths using query parameters, such as list/search filters, are not affected by this PR because they already go through query/body construction rather than path-segment interpolation.
Type of Change
Breaking Changes
N/A
Test Coverage
Manual validation performed by inspecting the generated paths before and after encoding for representative IDs containing
/,?, and#.This validation checked that those characters are preserved as part of one path segment after encoding, for example:
Expected encoded request examples:
Validation run locally:
Additional local validation was posted in the PR comments. No test files were added or committed in this PR.
Checklist