scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-10-04 02:31:36 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 7
2136 commits 115 branches 1023 tags 246 MiB
Commit graph

3 commits

Author SHA1 Message Date
ChunxueLi
678a0e11c9
fix(server,web): honor the grep tool contract — real regex, fileFilter, caseSensitive (#3109) 
* fix(server,web): honor grep tool contract — real regex, fileFilter, caseSensitive (Patch 12)

Background
==========

The web chat's grep tool schema has always promised regex search with an
optional path-substring fileFilter and caseSensitive control, but the
GET /api/grep handler escapeRegExp()'d every pattern into a literal
substring (a ReDoS hardening from fa36254e / #1317 that never re-synced
the tool contract). Consequences, verified in production use against the
sr-next backend repo (23k-file Java monorepo):

- An agent sending the documented alternation form ("sign|Sign") got
  zero hits and concluded the sign/签署 interface did not exist.
- The schema's own example pattern ("console\\.log") could never match:
  the escaped literal searched for a backslash in the source.
- fileFilter / caseSensitive were read by nobody — pure schema fiction.
- The web handler worked around the server with a (?=.*filter).*pattern
  lookahead splice that the same escaping also defeated.
- Collateral: the impact tool's grep fallback (\b${escapeRegex(name)}\b)
  was silently dead code under literal semantics; it comes back to life
  with this fix (expected improvement, noted for reviewers).

Fix
===

Server (gitnexus):
- New src/server/grep-params.ts — pure query-param parser (no Express /
  native imports, per the #2790 helper-extraction convention):
  regex construction (default real regex; literal=1 restores the old
  escaped-substring semantics as an opt-out), lowercase path-substring
  fileFilter, caseSensitive flag, limit clamp [1,200] default 50,
  BadRequestError error paths (mapped to 400 by statusFromError).
- /api/grep handler in api.ts becomes thin wiring: fileFilter path
  filtering before the (unchanged) traversal guard, a 5s wall-clock
  budget checked between files (partial results plus timedOut: true),
  read-only DB open unchanged. The regex is deliberately built WITHOUT
  the 'g' flag: the handler tests line-by-line and a stale lastIndex
  would skip matches (the old code had to reset it manually); 'm' is
  likewise omitted — each test sees one line, so ^/$ already anchor at
  string boundaries.

Web (gitnexus-web):
- tools.ts: drop the lookahead splice; description now tells the model
  the truth (real regex, alternation works, path-substring filter,
  case-insensitive default, result cap and time budget).
- backend-client.ts: grep() takes GrepOptions {fileFilter,caseSensitive}
  and forwards them as query params.
- useAppState.tsx: assembly site threads the options through.

Security — residual ReDoS exposure (read this before deploying)
===============================================================

The literal-only era was accidentally ReDoS-immune; this patch knowingly
trades that immunity back for the promised contract. The bounds (200-char
pattern cap, line-by-line matching, result cap, 5s budget) do NOT cover a
single catastrophically backtracking regex.test(): it blocks the Node
event loop synchronously, the budget (checked between files) cannot
interrupt it, and the whole server is unresponsive for the duration
(measured: (a+)+$ against a 35-char line exceeds 120 seconds). Accepted
because local serve binds loopback by default and hosted deploys gate
/api/grep behind the edge token; documented in SECURITY.md (new section)
with the worker_threads+terminate / optional-re2 follow-up called out.
literal=1 restores full immunity for untrusted callers.

Compatibility audit
===================

Repo-wide: /api/grep's only HTTP caller is backend-client.grep(); the MCP
tool surface has no grep tool; eval/ uses shell grep, not this endpoint;
the endpoint is undocumented (docs/llms.txt) with no known third-party
consumers. Breaking surface ≈ zero. Pattern metacharacter semantics
change for direct curl users ("array[0]" now needs escaping or literal=1).

Tests
=====

+20 cases in test/unit/grep-params.test.ts: alternation (the regression
that burned the agent), the schema's own example, case flags, literal
compat, CJK patterns, ^/$ line anchors, fileFilter normalization +
array-form rejection, limit clamping, type-confusion guards, invalid
regex, and a source-level handler wiring assertion (api-readonly-wiring
style). Full unit suite: no new failures (22 pre-existing failures
reproduced identically with this patch stashed — analyzer-identity dist
fingerprint + lbug native-env classes).

Upstream plan
=============

Issue + PR to abhigyanpatwari/GitNexus; the PR description must front the
ReDoS trade-off with the worker-isolation follow-up. Repro for the issue:
curl ".../api/grep?pattern=TODO%7CFIXME" — 0 hits under literal
semantics, both marker classes under regex semantics.

Custom-patch ledger: CUSTOM_PATCHES.md Patch 12.

* style: prettier

* fix(web): surface grep timedOut so partial scans are not silent misses

Propagate the server timeout flag through the backend client and chat tool, check the 5s budget between lines, and document the accepted regex-injection CodeQL finding next to new RegExp.

* Address PR review feedback (#3109)

- Cover empty and null fileFilter in the grep client test
- Keep timedOut as a required boolean and reuse GrepOptions
- Sample the grep deadline every 256 lines instead of every line

Note: pre-existing failure in impact-tool.test.ts not addressed by this PR.

* Address PR review feedback (#3109)

Run /api/grep matching in a worker_threads worker so terminate() can
cut a catastrophic regex.test without blocking the parent event loop.

* Address PR review feedback (#3109)

Reset lastIndex per line, restore the missing-pattern 400 message, and
make the traversal test create a real outside file.

* fix(web): align agent grep opts with GrepOptions

Use GrepOptions so fileFilter null is accepted by the local GraphRAGBackend stub.

* fix(server): silence CodeQL js/regex-injection on intentional grep regex

Split literal vs regex construction and suppress with the correct rule id
(js/regex-injection). Real regex remains the default contract; literal=1
still escapes.

* Address PR review feedback (#3109)

Clean up grep-scan temp dirs after each test, and exclude the intentional
grep-params RegExp site from CodeQL so js/regex-injection does not re-file.

---------

Co-authored-by: l.cx <l.cx@winning.com.cn>
Co-authored-by: ChunxueLi <mecoloud@users.noreply.gitee.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
 2026-08-31 20:43:28 +01:00
Gergő Magyar
6dc6544365
fix(web): chat-only mode for large projects to prevent WebUI hang (#2178) (#2185) 
* feat(web): add graph-load skip decision helper and node threshold (#2178)

* feat(web): skip graph download in connectToServer for chat-only mode (#2178)

* feat(web): add graphMode state and empty-graph chat-only handling (#2178)

* feat(web): read and thread ?skipGraph URL param through connect flow (#2178)

* feat(web): chat-only empty state with load-graph-anyway escape hatch (#2178)

* style(web): apply prettier formatting to graph-load files (#2178)

* fix(review): apply autofix feedback

- Fail-safe confirm + authoritative node count (P1: prevent re-triggering the hang via Load-graph-anyway when count unknown)
- In-flight guard on loadGraphAnyway (P1: double-fire)
- Honor explicit ?skipGraph in onAnalyzeComplete and DropZone (R6/U4)
- Extract buildGraphFromConnectResult shared helper (DRY across 3 connect sites)
- Add tests: switchRepo skip path, threshold config override, loadGraphAnyway error path, confirm fail-safe, in-flight guard

* fix(review): address tri-review findings

- P1 (correctness+adversarial+risk): stop the cross-repo / F5 chat-only leak.
  loadGraphAnyway no longer persists ?skipGraph=0, and onAnalyzeComplete +
  DropZone no longer inherit a stale ?skipGraph for a different repo — both
  could bypass auto-detect and re-trigger the #2178 hang. ?skipGraph is now a
  bookmark hint honored only by the initial auto-connect; in-session repo
  changes auto-detect.
- P2 (performance): auto-detect now also skips on edge count (edge-driven
  force-layout cliff), not just nodes; LARGE_GRAPH_EDGE_THRESHOLD default 50K.
- P2 (julik): reset graphMode/chatOnlyNodeCount at the top of switchRepo so a
  failed switch can't leave a stale chat-only overlay.
- P2 (julik): set serverBaseUrl before awaiting handleServerConnect in
  auto-connect so the Load-graph-anyway button isn't briefly a no-op.
- P2 (risk): hide the misleading '0 nodes / 0 edges' stats in chat-only mode
  (Header + StatusBar).
- P2 (performance): guard the GraphCanvas layout effect against the empty
  chat-only graph.
- Tests: edge-threshold decision + connectToServer edge-trigger; load-anyway
  no longer asserts URL persistence.

* fix(web): make Load-graph-anyway cancellable, unmount-safe, fail-safe confirm (#2178)

- AbortController + mountedRef: cancel the in-flight download on unmount and
  guard every post-await setState by the mounted ref (an abort surfaces as a
  BackendError, not a DOMException AbortError, so name-checks would miss it)
- Stale-result guard: a load-anyway that resolves after a concurrent switchRepo
  no longer clobbers the new repo's graph/mode/count
- GraphCanvas confirm fails SAFE (treat as declined) when window.confirm is
  unavailable or throws, instead of silently proceeding into a large download

* fix(web): make the AI agent and chat surface aware of chat-only mode (#2178)

- buildDynamicSystemPrompt + createGraphRAGAgent take a chatOnly flag and append
  a note (both prompt branches) that supersedes VISUAL GROUNDING: the graph isn't
  loaded, [[Type:Name]] node citations won't highlight, prefer [[path:START-END]]
- initializeAgent resolves chatOnly = opts ?? graphModeRef.current==='chatOnly':
  connect-flow callers (handleServerConnect, switchRepo, loadGraphAnyway re-init)
  pass it explicitly; lazy/settings re-inits fall back to live mode via the ref
- loadGraphAnyway re-inits the agent (chatOnly:false) after a full load so the
  prompt drops the note
- RightPanel shows a chat-only banner so the degradation is visible where AI
  output renders (en + zh-CN)

* fix(web): streaming circuit breaker for graphs with missing size stats (#2178)

- GraphTooLargeError + a mid-stream breaker in parseNdjsonGraphResponse: count
  nodes/relationships as they arrive and abort (cancel reader in try/finally,
  then throw) the moment either crosses its limit — reusing the existing node/
  edge thresholds, no new magic constant. Throwing right after the offending
  push means a later error record in the same chunk can't pre-empt it.
- fetchGraph gains optional maxNodes/maxEdges (off by default → existing callers
  unchanged). connectToServer arms them only for auto-detect downloads
  (skipGraph !== false) and catches GraphTooLargeError → chat-only, re-throwing
  every other error. This backstops the no-stats fail-open path that could
  otherwise re-trigger the original hang.

* chore(autofix): apply prettier + eslint fixes via /autofix command

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
 2026-06-13 11:07:58 +01:00
Gergő Magyar
78ad6bc07e
fix(web): align agent system prompt with registered tools (#1984) 
* fix(web): align agent system prompt with registered tools

Rewrites BASE_SYSTEM_PROMPT to fix tool-name mismatches, citation format,
and schema guidance from PR #14 tri-review, and adds unit tests that
guard prompt ↔ tool registry parity.

Co-authored-by: Cursor <cursoragent@cursor.com>

* test(web): enforce agent prompt/tools parity and harden assertions

U1: assert GRAPH_RAG_TOOL_NAMES equals the names createGraphRAGTools actually registers (via a no-op stub backend), closing the const<->registration drift gap the prompt-parity test previously missed.

U2: make the forbidden-name guard word-boundary (catches bare-prose mentions, not just backticked); make the highlight_in_graph guarantee registry-level (reword-proof) plus a presence check; add a parser-recognized [[Type:Name]] symbol-citation assertion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(web): drop test-only GRAPH_RAG_TOOL_NAMES from llm barrel

U3: GRAPH_RAG_TOOL_NAMES has no runtime consumer -- the parity test imports it directly from ./tools -- so remove it from the public index.ts barrel re-export. Update the constant's doc comment to name the registration<->const<->prompt coupling now enforced by agent-prompt.test.ts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(test): derive symbol-ref assertion from NODE_REF_REGEX

Source the symbol-citation assertion from the UI parser's own NODE_REF_REGEX instead of a hardcoded 4-label subset, so the test tracks the parser's allowlist rather than forking it. Also drop a redundant array spread and an unnecessary readonly-tuple cast surfaced by the simplify pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(web): forbid affirmative highlight_in_graph call instructions

Code review noted the registry-absence + bare-presence pair would pass if a future prompt edit affirmatively instructed calling highlight_in_graph (string present, still not registered). Add an assertion that the prompt never says use/call/invoke highlight_in_graph -- restoring the protective intent of the replaced negation check without its brittleness.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
 2026-06-03 08:00:09 +01:00