Skip to content

fix(pipeline): reject non-HTTP slash arguments - #1389

Merged
DeusData merged 1 commit into
DeusData:mainfrom
Yyunozor:fix/arg-url-false-positives
Sep 1, 2026
Merged

fix(pipeline): reject non-HTTP slash arguments#1389
DeusData merged 1 commit into
DeusData:mainfrom
Yyunozor:fix/arg-url-false-positives

Conversation

@Yyunozor

Copy link
Copy Markdown
Contributor

Summary

  • apply the existing HTTP route-literal guard before creating arg_url routes
  • reject raw slash expressions such as JS regex literals
  • reject slash strings passed to non-HTTP string and regex consumers
  • add focused regressions for filesystem paths and a genuine API path

Root cause

detect_url_in_args() normalized every slash-prefixed call argument and
created a Route node directly. That path bypassed the shared route-literal
guard already used by the later route-node pass, so local filesystem paths and
regex operands could become Route nodes and HTTP_CALLS edges.

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, and exec.

Validation

  • scripts/test.sh --suites 'infrascan pipeline' — 241 passed
  • the same tests on the parent fail on sink(/<table/i) and
    template.replace('/html/g', ...)
  • full suite — 6821 passed, 3 failed, 4 skipped
  • the parent reproduces the same failures: cli (255 passed, 2 failed) and
    daemon_runtime (42 passed, 1 failed)

This addresses the arg_url regex/path bucket of #598. GraphQL and
whole-config-text classification remain out of scope.

@Yyunozor
Yyunozor requested a review from DeusData as a code owner July 31, 2026 18:35
@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.

@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Aug 3, 2026
@DeusData DeusData added this to the 0.9.1-rc milestone Aug 3, 2026
@DeusData

DeusData commented Aug 3, 2026

Copy link
Copy Markdown
Owner

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 0.9.1-rc. This note is an intake acknowledgement rather than a code review; the queue is currently full, so validating false-positive and false-negative behavior across frameworks may take a little time.

@DeusData

Copy link
Copy Markdown
Owner

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.

@DeusData

Copy link
Copy Markdown
Owner

@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 main (2cc80f20): pass_parallel.c now auto-merges cleanly — the only conflict is tests/test_pipeline.c, and it is pure insertion-anchor drift (the file grew a few thousand lines under you). Re-anchor your new e2e test and its RUN_TEST line and the rebase is done.

The two smalls from that comment still stand, and they are genuinely small:

  1. Confirm the delimiter-callee extension is intended to also tighten the two guard call sites in pass_route_nodes.c (:182, :684) — we believe it is correct, same false-positive class, but it is your extension and the confirmation should be yours.
  2. Drop the now-redundant local extern at pass_route_nodes.c:40 — your header declaration in service_patterns.h supersedes it.

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 Co-authored-by: credit — the fix is priority/high and #598's junk routes are worth removing promptly either way. Whichever way it lands, the work is yours.

Signed-off-by: Yyunozor <yyunozor@icloud.com>
@Yyunozor
Yyunozor force-pushed the fix/arg-url-false-positives branch from 6b06518 to c86f1ff Compare August 31, 2026 19:42
Copilot AI lite review requested due to automatic review settings August 31, 2026 19:42
@Yyunozor

Copy link
Copy Markdown
Contributor Author

Rebased onto main (today's tip). pass_parallel.c auto-merged cleanly. tests/test_pipeline.c had exactly the one conflict you called out — pure insertion-anchor drift from the three tests that landed under our anchor in August. Re-anchored the new test and its RUN_TEST line right after them; the diff this commit carries is otherwise identical to before (same 85 insertions, same 1 deletion, verified line-for-line).

Small #2: dropped the redundant local extern in pass_route_nodes.c (:40), included service_patterns.h instead — the same pattern pass_calls.c and pass_parallel.c already use.

Small #1, confirmed: route_edge_visitor (:181) and http_call_edge_has_valid_route (:683) call cbm_service_pattern_is_http_route_literal with the same two arguments as our new detect_url_in_args check, so the extension tightens all three call sites by construction — checked with a standalone before/after build of the predicate: is_http_route_literal("/html/g", "template.replace") is true on the parent, false after. Today it's defense in depth rather than a change visible in a fresh index (detect_url_in_args is the only current producer of that callee shape, and it already self-gates at creation), but it matters for graphs built by an older binary and re-checked on an incremental run, so worth keeping.

scripts/test.sh --suites 'infrascan pipeline' on the rebased branch: 267 passed, 0 failed. Full scripts/test.sh: 7770 passed, 2 failed, 7 skipped (141 suites) — subprocess_cancel_grace_is_hard_capped and one test_cli.c VS Code/Copilot-detection case, neither touched by this diff. The subprocess one passed on two reruns (timing flake). The test_cli.c:9491 one fails identically on today's bare main in the same environment, so it is environmental, not from this branch. daemon_runtime came back clean this run. Thanks for confirming the pre-existing failures on your side — saved me from chasing them.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 for arg_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 Route nodes.
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.

Comment thread tests/test_pipeline.c
Comment on lines +5399 to +5416
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();
@DeusData
DeusData merged commit 3627eff into DeusData:main Sep 1, 2026
34 checks passed
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Merged as 3627eff5. The fast-track promise from 17 August finally honoured — two weeks later than it should have been, and that part is ours.

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: is_http_route_literal("/html/g", "template.replace") is true on the parent and false after. That converts "we think it's correct, same FP class" from a shared hunch into a measured fact, and it establishes the tightening applies to route_edge_visitor (:181) and http_call_edge_has_valid_route (:683) by construction rather than by inspection. Noting that it is defence in depth today, since detect_url_in_args is the only current producer of that callee shape, is exactly the right caveat to attach.

Small #2 is done and follows the existing convention — service_patterns.h included the way pass_calls.c and pass_parallel.c already do, rather than a second local extern.

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 +1/−2. Re-anchoring a test after a few thousand lines drifted under it is precisely where a silent change sneaks in, and you closed that off.

The underlying fix — detect_url_in_args minting Route nodes from JS regex literals and filesystem paths, routed through the shared guard — was ruled merge-worthy on day one. Everything since was drift and our latency, not your work.

@Yyunozor

Yyunozor commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

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.

@DeusData

DeusData commented Sep 3, 2026

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants