Skip to content

fix(pipeline): emit and reconstruct route properties as valid JSON - #2738

Open
petterip wants to merge 3 commits into
DeusData:mainfrom
petterip:fix/route-properties-json
Open

petterip wants to merge 3 commits into
DeusData:mainfrom
petterip:fix/route-properties-json

Conversation

@petterip

@petterip petterip commented Oct 20, 2026 •

Copy link
Copy Markdown

What does this PR do?

Makes src/pipeline/pass_route_nodes.c write and read Route and HANDLES properties as valid JSON. It is the groundwork for #759 (Next.js App Router routes), which follows as a separate PR stacked on this one: a route path from a directory with a quote in its name has to come out of the route pass intact.

On every index the route pass also rebuilds a Route from the route_path/route_method stored on each Function, so an unchanged handler keeps its Route across an incremental re-index. Those were read with a quote scan, and the writers didn't escape everything they wrote:

Defect Fix Test (fails on 4cf44147 without the fix)
Rebuild read route_path/route_method up to the next ": an escaped quote or backslash cut the path short, and a value nested in another object, cut off, or holding an escaped NUL still named a Route (3 invented Routes in the test) decode with yyjson; anything that is not a top-level string that fits names no Route infrascan_decorator_route_metadata_keeps_its_escapes, infrascan_decorator_route_metadata_rejects_malformed_values
Rebuilt HANDLES handler unescaped cbm_json_escape same
HANDLES properties in char[512] around a 512-byte escape buffer: a long QN loses the closing "} (emitter and rebuild) CBM_SZ_1K, as in pass_parallel.c and pass_calls.c infrascan_handles_properties_hold_a_long_handler_qn
Route method unescaped; a DRF @action(methods=['a"b']) method is source text (emitter and rebuild) cbm_json_escape infrascan_route_properties_escape_the_method
SvelteKit +server HANDLES: QN unescaped, char[256] sveltekit_handles_props helper, escaped, CBM_SZ_1K infrascan_sveltekit_handles_properties_escape_the_handler_qn

The parse uses YYJSON_READ_ALLOW_INVALID_UNICODE, as internal/cbm/diagram.c does: properties hold source text as its bytes (a Latin-1 docstring is stored as-is beside route_path), and a strict parse rejects the whole object, so the pass would skip that handler where the quote scan rebuilt its Route. infrascan_decorator_route_metadata_survives_invalid_utf8 guards that; it passes on 4cf44147 and fails with a strict parse.

All five are the same defect (properties that aren't valid JSON) in one file, so I kept them together. If you'd rather have the emitter and SvelteKit fixes as a separate PR, I'll split them.

I checked the other "handler":"%s" and "method":"%s" writers under src/: they already escape into 1K buffers, take values from fixed tables (cbm_service_pattern_*, the shell METHODS[], the SvelteKit verbs), or write constants.

The SvelteKit write moved into a helper so the diff-scoped clang-tidy check doesn't flag the existing complexity of sveltekit_file_visitor; that function gets simpler, not more complex.

Evidence

Tested on 4cf44147. The branch now carries current main through the merge in 671bc495; the second commit, d9766ae8, is the UTF-8 fix described above. Machine: x86_64, 16 cores, WSL2 (Linux 6.18) on Ubuntu 22.04, gcc 11.4; clang-format 20.1.8, cppcheck 2.20.0 and clang-tidy 21.1.8, the versions CI uses.

  • The five defect tests on 4cf44147 without the fix: 5 failed, the 12 existing infrascan tests passed. The UTF-8 guard passes there.
  • infrascan suite with the fix: 18 passed, 0 failed; each new test passes in under 0.2 s on its own.
  • Full scripts/test.sh: rc 0, 9904 passed, 0 failed, 9 skipped (SKIP_PLATFORM), 191 suites. Run with step 0w (published language-count contract) removed, because that step fails on 4cf44147 itself; fix(ci): repair language count and Windows CodeQL retrace contracts #2732 fixes it.
  • scripts/lint.sh --ci CLANG_FORMAT=clang-format-20: rc 0, "All linters passed".
  • make -f Makefile.cbm lint-tidy-diff CC=clang-21 CLANG_TIDY=clang-tidy-21 on the staged change: rc 0, no findings on changed lines.
  • make -f Makefile.cbm lint-mem-ci CC=clang-21 CLANG_TIDY=clang-tidy-21: rc 0, "memory gate clean".

AI assistance

Implementation and tests were written with AI assistance (Claude Code; planning with Codex). I reviewed the change, ran the commands listed above, and take responsibility for it under my DCO sign-off.

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

An unchanged handler's Route is rebuilt from the route_path and
route_method stored on its Function. They were read with a quote scan:
an escaped quote or backslash cut the path short, and a nested, broken
or non-string value still named a Route. They are now decoded with
yyjson, and anything that is not a top-level string that fits names no
Route.

Every Route and HANDLES properties writer in pass_route_nodes.c now
escapes its values into a buffer that fits them: the rebuilt handler QN
was unescaped, a QN filling the 512-byte escape buffer left the closing
"} off the 512-byte properties, a method is source text where a DRF
@action names it, and the SvelteKit +server writer had both problems
in 256 bytes.

Tests: each of the five new infrascan tests fails on 4cf4414 without
this change.

Signed-off-by: Petteri Ponsimaa <petterip@gmail.com>
@petterip
petterip force-pushed the fix/route-properties-json branch from add0565 to 925703d Compare October 20, 2026 21:57
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

Thanks for the route-properties JSON fix and the regression coverage! I merged current main into this branch in 671bc49 so the PR picks up our updated CI configuration. The merge was conflict-free, and your two-file patch is unchanged. This preserves your commit and sign-off; the new head will now run the current required checks.

Review follow-up to the previous commit. Properties hold source text
as its bytes, so a Latin-1 docstring stored beside route_path is not
UTF-8. The strict yyjson parse rejected that whole object, so the
route pass skipped rebuilding the handler's Route, where the old quote
scan had rebuilt it. Parse with YYJSON_READ_ALLOW_INVALID_UNICODE, as
internal/cbm/diagram.c does.

Test: infrascan_decorator_route_metadata_survives_invalid_utf8 fails
with the strict parse, and passes with this change and on 4cf4414.

Signed-off-by: Petteri Ponsimaa <petterip@gmail.com>
@petterip

Copy link
Copy Markdown
Author

Written with AI assistance (Claude Code); I reviewed it and stand behind it.

Thanks for syncing the branch, @DeusData. I added one commit on top of your merge, d9766ae, rather than force-pushing over it.

Why: a review pass found a regression in my first commit. Properties store source text as raw bytes, so a Latin-1 docstring beside route_path is not valid UTF-8. The strict yyjson parse rejected the whole object, so the route pass stopped rebuilding that handler's Route, which the old quote scan did. The fix parses with YYJSON_READ_ALLOW_INVALID_UNICODE, as internal/cbm/diagram.c already does. The new test infrascan_decorator_route_metadata_survives_invalid_utf8 fails with the strict parse, and passes with the fix and on 4cf44147. The PR description is updated to match.

On the previous run, lint / lint was cancelled at the 45-minute job limit while cppcheck was still running. scripts/lint.sh --ci passes locally.

This branch has not been deployed

No deployments
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.

2 participants