fix(pipeline): reject non-HTTP slash arguments - #1389
Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
|
Thank you for narrowing the route heuristic to HTTP-shaped slash arguments and for adding focused extraction coverage. This is now routed as a high-priority parsing fix in |
|
The substance here is merge-worthy: detect_url_in_args minting Route nodes from JS regex literals and filesystem paths is the arg_url bucket of #598, and routing it through the shared cbm_service_pattern_is_http_route_literal guard is the right unification. Your documented pre-existing-failure validation was honest work. It needs only a rebase — pass_parallel.c moved under you in August, and GH now reports conflicts; your green checks predate that drift. Two smalls while you're in there: confirm the delimiter-callee extension (replace/match/search/test/exec) tightening pass_route_nodes via the shared guard is intended (we think it's correct — same FP class), and drop the now-redundant local extern at pass_route_nodes.c:40 in favor of the new header declaration. Rebase and we'll fast-track the re-review. |
|
@Yyunozor — a nudge plus some good news that should make the rebase easier than it looked on 17 August. The conflict has shrunk to one file. Re-checked against today's The two smalls from that comment still stand, and they are genuinely small:
The substance was ruled merge-worthy on 17 August and the fast-track promise holds: rebase, and the re-review happens promptly. Your pre-existing-failure validation on the parent tree was, as said then, honest work — for what it's worth, the three unrelated failures you documented have since been confirmed as known flakes on our attribution ledger, independently of your PR. So this does not sit open-ended: if we have not heard from you by ~11 September, we will do the rebase and the two smalls ourselves and land it with |
Signed-off-by: Yyunozor <yyunozor@icloud.com>
6b06518 to
c86f1ff
Compare
|
Rebased onto Small #2: dropped the redundant local Small #1, confirmed:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new pipeline regression test asserts before cleanup, so a failure can leak CBM_WORKERS and temp directories into subsequent tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the “arg_url” route-detection heuristic in the parallel pipeline so that slash-prefixed non-route arguments (notably JS regex literals and non-HTTP string/regex consumers) don’t get misclassified into Route nodes and HTTP_CALLS edges, while preserving genuine API paths.
Changes:
- Filter out slash-prefixed raw expressions (e.g., JS/TS regex literals) before URL normalization in
detect_url_in_args(). - Reuse the shared
cbm_service_pattern_is_http_route_literal()guard forarg_url-minted routes and extend callee filtering to common non-HTTP string/regex consumers (e.g.,replace,match,test,exec). - Add regressions covering filesystem paths, regex operands, and a real API path to prevent reintroducing false-positive
Routenodes.
File summaries
| File | Description |
|---|---|
tests/test_pipeline.c |
Adds a regression test to ensure the arg-url heuristic doesn’t mint Route nodes for filesystem paths or regex operands while keeping a real API route. |
tests/test_infrascan.c |
Extends the HTTP route-literal guard test to reject /html/g when used by template.replace. |
src/pipeline/pass_route_nodes.c |
Switches from a local forward declaration to including service_patterns.h for the shared guard. |
src/pipeline/pass_parallel.c |
Applies raw-expression rejection and the shared HTTP route-literal guard before creating arg_url route nodes. |
internal/cbm/service_patterns.h |
Exposes the cbm_service_pattern_is_http_route_literal() declaration in the header. |
internal/cbm/service_patterns.c |
Expands non-HTTP consumer detection to include common string/regex methods to prevent route misclassification. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| cbm_store_t *s = cbm_store_open_path(db_path); | ||
| ASSERT_NOT_NULL(s); | ||
|
|
||
| ASSERT_EQ(count_nodes_named(s, project, "/html/g"), 0); | ||
| ASSERT_EQ(count_nodes_named(s, project, "/<table/i"), 0); | ||
| ASSERT_EQ(count_nodes_named(s, project, "/tmp/pgv_fuzz.bin"), 0); | ||
| ASSERT_GTE(count_nodes_named(s, project, "/api/data"), 1); | ||
|
|
||
| cbm_store_close(s); | ||
| cbm_pipeline_free(p); | ||
| if (saved) { | ||
| cbm_setenv("CBM_WORKERS", saved, 1); | ||
| free(saved); | ||
| } else { | ||
| cbm_unsetenv("CBM_WORKERS"); | ||
| } | ||
| th_rmtree(tmp); | ||
| PASS(); |
|
Merged as Both confirmations were answered properly, and the first one was answered the right way: Small #1. You did not just assert the extension was intended — you built the predicate standalone and showed it: Small #2 is done and follows the existing convention — And you verified the rebase rather than trusting it. "The diff this commit carries is otherwise identical to before — same 85 insertions, same 1 deletion, verified line-for-line" is the claim that let me merge this without re-reviewing the substance: the arithmetic checks out, since the only delta is small #2's The underlying fix — |
|
Thanks for the merge, and for the clear pointers at every step — the one-file conflict note made the rebase trivial. No worries on the timing. #1766 is rebased on current main and green on the targeted suites, whenever you get to it. |
Maintainer notice: please disregard comments from @adfjadfj16-a11y on this thread@adfjadfj16-a11y is not a maintainer of this project and does not speak for it. That account has posted replies on 17 threads here written in the project's voice — promising merges, announcing that a case has been "escalated to the development team", asking to close issues, and in some threads replying as though it were the author of someone else's pull request. None of those were maintainer decisions, and none of them carried any weight. @DeusData is the only account that gives a maintainer response on this repository. If a comment about the fate of your issue or pull request did not come from @DeusData, it is not a decision, however official it reads. If you were waiting on something because of one of those comments — a promised merge, a review "immediately", a request to close your ticket — I am sorry. That was noise you had no way to identify as noise, and it should not have been on your thread. Your issue or PR is judged on its own merits, and I will answer it here myself. Nothing in this notice reflects on your contribution. Thank you for your patience, and thank you for the work. |
Summary
arg_urlroutesRoot cause
detect_url_in_args()normalized every slash-prefixed call argument andcreated a
Routenode directly. That path bypassed the shared route-literalguard already used by the later route-node pass, so local filesystem paths and
regex operands could become
Routenodes andHTTP_CALLSedges.The change first rejects slash-prefixed raw expressions (string literals and
propagated constants are carried separately), routes the remaining heuristic
through the shared guard, and extends the callee filter to non-HTTP string
consumers such as
replace,match,search,test, andexec.Validation
scripts/test.sh --suites 'infrascan pipeline'— 241 passedsink(/<table/i)andtemplate.replace('/html/g', ...)cli(255 passed, 2 failed) anddaemon_runtime(42 passed, 1 failed)This addresses the
arg_urlregex/path bucket of #598. GraphQL andwhole-config-text classification remain out of scope.