fix(pipeline): escape dynamic edge properties as JSON - #1065

Thanks for the focused JSON escaping fix. This is a high-priority 0.9.1-rc graph-correctness PR. Review will verify that dynamic edge properties are escaped identically in sequential and parallel paths and that quotes, backslashes, control characters, and non-ASCII values remain valid round-trip JSON.

The fix is correct and I want to land it — escaping parser-derived strings through cbm_json_escape before embedding them in properties JSON is exactly right (same class as the service-edge JSON-validity work in #898 and the UTF-8 fix in #1017). One thing to fix first, though:

The test as written is a vacuous guard. I verified it: reverting both production changes (the esc_ln/esc_op escaping in pass_definitions.c and pass_parallel.c) and re-running contract_edge_parallel_service_edges still passes — the AND json_valid(properties)=0 → invalid_props==0 assertion never fires because the fixture contains no string with a JSON-breaking character. So the test passes with and without your fix, which means it can't catch a regression.

To make it a genuine reproduce-first guard, the fixture needs a value that actually breaks JSON when unescaped — e.g. an import whose local_name or a service-edge URL/topic string contains a double-quote, backslash, or a CR/LF. With that, json_valid(properties) would be 0 on unfixed code (test RED) and 1 with your escaping (GREEN). A URL argument with an embedded quote is probably the easiest to construct in the fixture.

Could you add that? Once the test is RED-without-the-fix I'll merge immediately — the production change is already good. Thanks for closing a real JSON-corruption gap.

Thanks — addressed. The fixture now includes a GraphQL operation with a literal double quote (user(name: "quoted")), so reverting the esc_op escaping makes json_valid(properties) fail, while the patched code keeps the properties valid JSON.

I also cleaned the branch onto current main as a single commit (f809e98). The previous Ubuntu smoke failure was the unrelated EOF shutdown timeout (binary did not exit within 5 seconds after EOF); the fresh pr-smoke (ubuntu-latest) run is now green, as is macOS. The remaining long-running checks are still in progress. Ready for re-review — thank you!

Thanks for addressing the vacuous regression and for cleaning the branch. Current head 4811126908d77c274850956a8cc9c19e099a41e6 has a green check set. The exact-head re-review will verify that the literal-quote fixture is genuinely RED when the production escaping is reverted and GREEN with the fix. No additional contributor action is requested in the meantime.

Thank you for this — the production fix is exactly right and we want it. One problem with the regression test, and then it merges.

What is correct. Both sites interpolated parser-derived text straight into JSON properties without escaping: local_name on IMPORTS (pass_definitions.c:474) and operation on GRAPHQL_CALLS (pass_parallel.c:1997). Nice catch that callee was already escaped on that second one and operation was not — that asymmetry is easy to read past.

You reused cbm_json_escape rather than hand-rolling a second escaper, which is the right instinct. I checked the helper's truncation behaviour too: each escape tests remaining space before writing, so output can never end mid-sequence with a dangling backslash. And I did the sizing math on the wrapper — worst case for the graphql props is about 812 bytes against a 1 KB buffer, imports 144 against 256, so it never truncates the closing "}. Incidentally your fix also closes a pre-existing malformation vector there: a raw local_name over ~238 bytes previously could truncate that closing brace.

I tried to break it and could not. Bare quote, trailing backslash, control characters, embedded NUL, and a crafted-length truncation attack are all contained post-fix. The round trip is clean too: encoded once, bound verbatim through the parameterised bind_text, decoded once by readers — no double-escape anywhere.

The problem: the regression test passes with and without your fix.

The fixture uses a multiline triple-quoted Python string. That operation never reaches emit_graphql_edge at all — strip_and_validate_string_arg (extract_calls.c:1450) rejects any string argument containing \n, since only \t is exempt below MIN_PRINTABLE. So first_string_arg comes back NULL and the emitter falls back to the callee name "gql", which is perfectly valid JSON either way. Your ASSERT_EQ(invalid_props, 0) therefore holds on unpatched main.

The same applies to the mechanism described in the PR body: a multiline operation producing malformed JSON is end-to-end unreachable. The genuinely reachable vector is a bare double quote on a single line — which is a better bug than the one you described, and your fix already handles it.

The change I would like, and then this goes in:

return gql('query GetUser { user(name: "quoted") { id name } }')

A single-quoted Python string, so the inner double quote survives extraction, and every character is ≥ 0x20 so validation passes it through. Pre-fix that emits "operation":"GetUser { user(name: "quoted")…" → json_valid returns 0 → your assertion goes red.

Please confirm it fails with the production hunks reverted before pushing. That revert-check is the whole point of the exercise here — a green test that would also be green without the fix tells us nothing later.

Two smaller notes, neither blocking:

• The IMPORTS local_name path has no discriminating fixture (the alias in the test is the benign "gql"), and the "multiline Rust import alias" scenario in the description does not really exist since Rust aliases are identifiers. That is completely fine as defence-in-depth — just worth labelling it as such rather than as a reproduction.
• The whole-table json_valid(properties) = 0 sweep is a genuinely good invariant-style assertion. Please keep it; it becomes valuable the moment the fixture actually exercises the escape path.

One thing I noticed while reviewing that is not yours: emit_grpc_edge interpolates service and method raw into properties using the same pattern. Both derive from identifier-shaped text so the risk is low, but we will track it on our side.

Updated as requested. The fixture now uses the reachable single-line, single-quoted Python form, so the embedded double quote reaches emit_graphql_edge.

I also ran the explicit sensitivity check before pushing the final branch:

• RED, with both production escaping hunks reverted: https://github.com/tmonestudio/codebase-memory-mcp/actions/runs/30671818737 — contract_edge_parallel_service_edges fails with invalid_props == 1.
• GREEN, with the production fix present: https://github.com/tmonestudio/codebase-memory-mcp/actions/runs/30671817289

The PR is now rebased on current main as one signed-off commit (2d6766f6). I kept the whole-table json_valid(properties) invariant and corrected the PR description to distinguish the reachable GraphQL reproduction from the defensive IMPORTS hardening. Full CI is running on the final head.

Thank you for addressing the earlier review points, adding the RED/GREEN sensitivity evidence, and rebasing the branch. I have recorded the update and returned the current diff to the maintainer review queue. A complete review may still take a little time while the community queue is full.

Rebased this PR onto current upstream main (7f23a66a, v0.10.2+) and force-updated the head with lease protection.

• new head: 283321f0
• scope remains the two JSON-escaping production sites plus the reachable quoted GraphQL regression
• git diff --check: clean
• Windows production GCC build: GREEN (A5608EA18D4D5D2E579A90D7410ED3A0E05FC24BE9EF000E587A4E1258B715E5 for the combined updated-base candidate)
• focused native suites on the updated base (mcp lang_contract): 224 passed, 7 platform skips
• discriminating fixture contract_edge_parallel_service_edges: PASS

Local GCC 15 required suppressing two unrelated warnings-as-errors in existing daemon tests (free-nonheap-object, stringop-overflow) to reach the focused suites; no source suppression was committed. Fresh CI on this exact head is the authoritative pinned-toolchain result.

CI update: all 34 checks on 283321f0 have passed. The rebased PR remains conflict-free and limited to the JSON edge-property escaping fix plus its regression coverage.

Thank you @tmonestudio for rebasing and reporting the pinned verification. The current head 283321f0 is mergeable, all checks are green, and the scope remains the two JSON edge-property serialization sites plus their regression coverage.

This is recorded back in the maintainer review queue. The queue is still full, so the final code review may take some time, but no additional triage information is needed from you right now.