From 5c314dde774b06322da7711979026301638c4320 Mon Sep 17 00:00:00 2001 From: himanshupatro-334 Date: Mon, 17 Aug 2026 23:56:32 +0530 Subject: [PATCH 1/2] fix: render rationale in graph read surfaces --- graphify/cli.py | 30 +++- graphify/security.py | 64 ++++++- graphify/serve.py | 68 +++++++- tests/test_explain_cli.py | 58 +++++- tests/test_query_cli.py | 36 ++++ tests/test_security.py | 102 +++++++++++ tests/test_serve.py | 358 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 699 insertions(+), 17 deletions(-) diff --git a/graphify/cli.py b/graphify/cli.py index 06b742e809..acd61b94d8 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -1530,7 +1530,7 @@ def dispatch_command(cmd: str) -> None: sys.exit(1) elif cmd == "query": if len(sys.argv) < 3: - print("Usage: graphify query \"\" [--dfs] [--context C] [--seed X] [--budget N] [--graph path]", file=sys.stderr) + print("Usage: graphify query \"\" [--dfs] [--context C] [--seed X] [--budget N] [--graph path] [--rationale]", file=sys.stderr) sys.exit(1) from graphify.serve import _QUERY_NO_MATCH, _query_graph_text from graphify.security import sanitize_label @@ -1543,6 +1543,7 @@ def dispatch_command(cmd: str) -> None: graph_path = _default_graph_path() context_filters: list[str] = [] seeds: list[str] = [] + include_rationale = False args = sys.argv[3:] i = 0 while i < len(args): @@ -1579,6 +1580,9 @@ def dispatch_command(cmd: str) -> None: elif args[i] == "--graph" and i + 1 < len(args): graph_path = args[i + 1] i += 2 + elif args[i] == "--rationale": + include_rationale = True + i += 1 else: i += 1 gp = Path(graph_path).resolve() @@ -1650,6 +1654,7 @@ def dispatch_command(cmd: str) -> None: seeds=seeds or None, # Same root derivation as `affected` (#2706): /graphify-out/graph.json. seed_root=_graph_root_for(gp), + include_rationale=include_rationale, ) querylog.log_query( kind="query", @@ -2074,18 +2079,26 @@ def dispatch_command(cmd: str) -> None: elif cmd == "explain": if len(sys.argv) < 3: - print('Usage: graphify explain "" [--all] [--graph path]', file=sys.stderr) + print('Usage: graphify explain "" [--all] [--graph path] [--rationale]', file=sys.stderr) sys.exit(1) from graphify.serve import _is_ambiguity_refusal, _resolve_single_node from networkx.readwrite import json_graph label = sys.argv[2] graph_path = _default_graph_path() + include_rationale = False args = sys.argv[3:] show_all = "--all" in args - for i, a in enumerate(args): - if a == "--graph" and i + 1 < len(args): + i = 0 + while i < len(args): + if args[i] == "--graph" and i + 1 < len(args): graph_path = args[i + 1] + i += 2 + elif args[i] == "--rationale": + include_rationale = True + i += 1 + else: + i += 1 gp = Path(graph_path).resolve() if not gp.exists(): print(f"error: graph file not found: {gp}", file=sys.stderr) @@ -2112,6 +2125,15 @@ def dispatch_command(cmd: str) -> None: ) print(f" Type: {d.get('file_type', '')}") print(f" Community: {d.get('community_name') or d.get('community', '')}") + if include_rationale: + from graphify.security import sanitize_rationale, MAX_DETAIL_RATIONALE_CHARS + rat = sanitize_rationale( + d.get("rationale"), + single_line=False, + max_chars=MAX_DETAIL_RATIONALE_CHARS, + ) + if rat: + print(f" Rationale: {rat}") # Work-memory overlay: a derived experiential hint from `graphify reflect`, # merged in display-only from the .graphify_learning.json sidecar next to # graph.json. No line when the node has no overlay entry. diff --git a/graphify/security.py b/graphify/security.py index ab052e9542..980e6ae187 100644 --- a/graphify/security.py +++ b/graphify/security.py @@ -387,7 +387,7 @@ def check_graph_file_size_cap(path: Path) -> None: # Label sanitisation (mirrors code-review-graph's _sanitize_name pattern) # --------------------------------------------------------------------------- -_CONTROL_CHAR_RE = re.compile(r"[\x00-\x1f\x7f]") +_CONTROL_CHAR_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]") _MAX_LABEL_LEN = 256 @@ -405,6 +405,68 @@ def sanitize_label(text: str | None) -> str: return text +MAX_QUERY_RATIONALE_CHARS = 512 +MAX_DETAIL_RATIONALE_CHARS = 2048 + +_CONTROL_CHAR_NO_NL_RE = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]") + + +def sanitize_rationale( + text: str | None, + *, + single_line: bool = False, + max_chars: int | None = None, +) -> str: + """Sanitize and optionally truncate a node rationale string. + + In compact (single-line) mode: + - Normalizes all whitespace runs (newlines, tabs, spaces) to single spaces + - Strips control characters + - Caps output at `max_chars` (default MAX_QUERY_RATIONALE_CHARS = 512) + - Adds an explicit '...' truncation marker when capped + + In detail mode: + - Preserves newlines/paragraphs while stripping unsafe control characters + - Caps output at `max_chars` (default MAX_DETAIL_RATIONALE_CHARS = 2048) + - Adds an explicit '...' truncation marker when capped + + Returns empty string if text is None, empty, or whitespace-only. + """ + if text is None: + return "" + raw = str(text) + if not raw.strip(): + return "" + + if max_chars is None: + max_chars = MAX_QUERY_RATIONALE_CHARS if single_line else MAX_DETAIL_RATIONALE_CHARS + + if single_line: + # Collapse all whitespace sequences to a single space, then strip control chars + flat = " ".join(raw.split()) + cleaned = _CONTROL_CHAR_RE.sub("", flat).strip() + if not cleaned: + return "" + if len(cleaned) > max_chars: + marker = "..." + if max_chars <= len(marker): + return cleaned[:max_chars] + return cleaned[: max_chars - len(marker)].rstrip() + marker + return cleaned + else: + # Preserve newlines/paragraphs, normalize CRLF to LF, strip other control chars + normalized = raw.replace("\r\n", "\n").replace("\r", "\n") + cleaned = _CONTROL_CHAR_NO_NL_RE.sub("", normalized).strip() + if not cleaned: + return "" + if len(cleaned) > max_chars: + marker = "..." + if max_chars <= len(marker): + return cleaned[:max_chars] + return cleaned[: max_chars - len(marker)].rstrip() + marker + return cleaned + + # --------------------------------------------------------------------------- # Metadata sanitisation (recursive, bounded, HTML-safe) # --------------------------------------------------------------------------- diff --git a/graphify/serve.py b/graphify/serve.py index 638aa07e0b..dcf33abdc3 100644 --- a/graphify/serve.py +++ b/graphify/serve.py @@ -13,7 +13,14 @@ from typing import NamedTuple import networkx as nx from networkx.readwrite import json_graph -from graphify.security import sanitize_label, check_graph_file_size_cap, _CONTROL_CHAR_RE +from graphify.security import ( + sanitize_label, + check_graph_file_size_cap, + sanitize_rationale, + MAX_QUERY_RATIONALE_CHARS, + MAX_DETAIL_RATIONALE_CHARS, + _CONTROL_CHAR_RE, +) from graphify.build import edge_datas, has_parallel_links from graphify.paths import default_graph_json as _default_graph_json @@ -1117,7 +1124,7 @@ def _dfs(G: nx.Graph, start_nodes: list[str], depth: int) -> tuple[set[str], lis return visited, edges_seen -def _subgraph_to_text(G: nx.Graph, nodes: set[str], edges: list[tuple], token_budget: int = 2000, *, seeds: list[str] | None = None) -> str: +def _subgraph_to_text(G: nx.Graph, nodes: set[str], edges: list[tuple], token_budget: int = 2000, *, seeds: list[str] | None = None, include_rationale: bool = False) -> str: """Render subgraph as text, cutting at token_budget (approx 3 chars/token). seeds: exact-match nodes rendered first before the degree-sorted expansion, @@ -1197,6 +1204,15 @@ def _adj(n): if len(attrs_summary) > 200: attrs_summary = attrs_summary[:197] + "..." attrs_suffix = f" attrs={{{sanitize_label(attrs_summary)}}}" + why_suffix = "" + if include_rationale: + rationale_text = sanitize_rationale( + d.get("rationale"), + single_line=True, + max_chars=MAX_QUERY_RATIONALE_CHARS, + ) + if rationale_text: + why_suffix = f" WHY {rationale_text}" line = ( f"NODE {sanitize_label(d.get('label', nid))} " f"[src={sanitize_label(str(d.get('source_file', '')))} " @@ -1204,6 +1220,7 @@ def _adj(n): f"community={sanitize_label(str(d.get('community_name') or d.get('community', '')))}" f"{learning_suffix}" f"{attrs_suffix}]" + f"{why_suffix}" ) lines.append(line) for u, v in edges: @@ -1421,6 +1438,7 @@ def _query_graph_text( graph_path: str | None = None, seeds: list[str] | None = None, seed_root: Path | None = None, + include_rationale: bool = False, ) -> str: # Explicit start nodes: an external ranker -- embeddings, a learned # retriever, grep -- can choose where the traversal begins instead of the @@ -1443,7 +1461,8 @@ def _query_graph_text( if not start_nodes: return "No unique node match for --seed: " + ", ".join(seeds) return _render_query(G, question, start_nodes, mode=mode, depth=depth, token_budget=token_budget, - context_filters=context_filters, graph_path=graph_path, given=True, unresolved=unresolved) + context_filters=context_filters, graph_path=graph_path, given=True, unresolved=unresolved, + include_rationale=include_rationale) terms = _query_terms(question) # One graph scoring pass produces both the combined ranking (used to drive # the gap-based seed selection below) and the per-token singleton winners @@ -1469,7 +1488,8 @@ def _query_graph_text( if not start_nodes: return _QUERY_NO_MATCH return _render_query(G, question, start_nodes, mode=mode, depth=depth, token_budget=token_budget, - context_filters=context_filters, graph_path=graph_path) + context_filters=context_filters, graph_path=graph_path, + include_rationale=include_rationale) def _render_query( @@ -1484,6 +1504,7 @@ def _render_query( graph_path: str | None, given: bool = False, unresolved: list[str] | None = None, + include_rationale: bool = False, ) -> str: resolved_filters, filter_source = _resolve_context_filters(question, context_filters) traversal_graph = _filter_graph_by_context(_traversal_view(G), resolved_filters) @@ -1512,7 +1533,14 @@ def _render_query( # Pass the seeds so the queried symbol renders first and survives truncation # (#BUG2): a branch merge had silently dropped this argument, leaving the # seed-first ordering as dead code. - return header + _subgraph_to_text(traversal_graph, nodes, edges, token_budget, seeds=start_nodes) + return header + _subgraph_to_text( + traversal_graph, + nodes, + edges, + token_budget, + seeds=start_nodes, + include_rationale=include_rationale, + ) def _resolve_path_scoped_symbol(G: nx.Graph, path_part: str, symbol_part: str) -> list[str]: @@ -2215,6 +2243,11 @@ async def list_tools() -> list[types.Tool]: "items": {"type": "string"}, "description": "Optional explicit edge-context filter, e.g. ['call', 'field']", }, + "include_rationale": { + "type": "boolean", + "default": False, + "description": "Include WHY rationale on nodes when available", + }, }, "required": ["question"], }, @@ -2227,6 +2260,11 @@ async def list_tools() -> list[types.Tool]: "properties": { "label": {"type": "string", "description": "Node label or ID to look up"}, "node_id": {"type": "string", "description": "Alias for label (node id)"}, + "include_rationale": { + "type": "boolean", + "default": False, + "description": "Include node rationale if available", + }, }, }, ), @@ -2362,6 +2400,7 @@ def _tool_query_graph(arguments: dict) -> str: depth = min(int(arguments.get("depth", 3)), 6) budget = int(arguments.get("token_budget", 2000)) context_filter = arguments.get("context_filter") + include_rationale = arguments.get("include_rationale") is True _t0 = _time.perf_counter() result = _query_graph_text( G, @@ -2371,6 +2410,7 @@ def _tool_query_graph(arguments: dict) -> str: token_budget=budget, context_filters=context_filter, graph_path=str(active_graph_path), + include_rationale=include_rationale, ) querylog.log_query( kind="mcp_query", @@ -2389,6 +2429,7 @@ def _tool_get_node(arguments: dict) -> str: if not raw: return "Provide a node label or id (accepted keys: label, node_id, id)." label = raw.lower() + include_rationale = arguments.get("include_rationale") is True nid, err = _resolve_single_node(G, label) if err: return err @@ -2401,7 +2442,7 @@ def _tool_get_node(arguments: dict) -> str: raw_attrs = raw_attrs[:997] + "..." attrs_line = [f" Attributes: {_CONTROL_CHAR_RE.sub('', raw_attrs)}"] # Sanitise every LLM-derived field before concatenation (F-010). - return "\n".join([ + lines = [ f"Node: {sanitize_label(d.get('label', nid))}", f" ID: {sanitize_label(nid)}", f" Source: {sanitize_label(str(d.get('source_file', '')))} {sanitize_label(str(d.get('source_location', '')))}", @@ -2413,9 +2454,18 @@ def _tool_get_node(arguments: dict) -> str: if d.get("definition_file") else []), f" Type: {sanitize_label(str(d.get('file_type', '')))}", f" Community: {sanitize_label(str(d.get('community_name') or d.get('community', '')))}", - f" Degree: {G.degree(nid)}", - *attrs_line, - ]) + ] + if include_rationale: + rat = sanitize_rationale( + d.get("rationale"), + single_line=False, + max_chars=MAX_DETAIL_RATIONALE_CHARS, + ) + if rat: + lines.append(f" Rationale: {rat}") + lines.append(f" Degree: {G.degree(nid)}") + lines.extend(attrs_line) + return "\n".join(lines) def _tool_get_neighbors(arguments: dict) -> str: raw = _node_arg(arguments) diff --git a/tests/test_explain_cli.py b/tests/test_explain_cli.py index b9ad108029..61aea84e0d 100644 --- a/tests/test_explain_cli.py +++ b/tests/test_explain_cli.py @@ -32,10 +32,12 @@ def _write_graph(tmp_path): return p -def _run(monkeypatch, graph_path, label, capsys): +def _run(monkeypatch, graph_path, label, capsys, extra_args=None): monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) - monkeypatch.setattr(mainmod.sys, "argv", - ["graphify", "explain", label, "--graph", str(graph_path)]) + argv = ["graphify", "explain", label, "--graph", str(graph_path)] + if extra_args: + argv.extend(extra_args) + monkeypatch.setattr(mainmod.sys, "argv", argv) mainmod.main() return capsys.readouterr().out @@ -400,3 +402,53 @@ def test_explain_double_colon_label_not_hijacked_by_a_coincidental_path_match(mo out = _run(monkeypatch, p, "mylib::Bar", capsys) assert "Ambiguous" not in out assert "ID: mylib_bar" in out + + +def test_explain_rationale_flag_shows_rationale(monkeypatch, tmp_path, capsys): + graph_data = { + "directed": False, "multigraph": False, "graph": {}, + "nodes": [ + { + "id": "cluster_node", + "label": "Clustering per deposito", + "source_file": "deposito.py", + "source_location": "L10", + "file_type": "code", + "community": 0, + "rationale": "cannot work on ORIGINALS_NORMALIZED as it stands, because timestamps and coordinates are missing", + } + ], + "links": [], + } + p = tmp_path / "graph.json" + p.write_text(json.dumps(graph_data)) + + # Without --rationale flag + out_default = _run(monkeypatch, p, "Clustering per deposito", capsys) + assert "Rationale:" not in out_default + + # With --rationale flag + out_rat = _run(monkeypatch, p, "Clustering per deposito", capsys, extra_args=["--rationale"]) + assert " Rationale: cannot work on ORIGINALS_NORMALIZED as it stands, because timestamps and coordinates are missing" in out_rat + + +def test_explain_rationale_empty_omitted(monkeypatch, tmp_path, capsys): + graph_data = { + "directed": False, "multigraph": False, "graph": {}, + "nodes": [ + { + "id": "empty_rat_node", + "label": "EmptyNode", + "source_file": "empty.py", + "source_location": "L1", + "file_type": "code", + "community": 0, + "rationale": " \n\t ", + } + ], + "links": [], + } + p = tmp_path / "graph.json" + p.write_text(json.dumps(graph_data)) + out = _run(monkeypatch, p, "EmptyNode", capsys, extra_args=["--rationale"]) + assert "Rationale:" not in out diff --git a/tests/test_query_cli.py b/tests/test_query_cli.py index 0db4e6fa8a..d3465ab09c 100644 --- a/tests/test_query_cli.py +++ b/tests/test_query_cli.py @@ -123,3 +123,39 @@ def test_query_cli_rejects_oversized_graph(monkeypatch, tmp_path, capsys): err = capsys.readouterr().err assert "exceeds" in err assert "byte cap" in err + + +def test_query_cli_rationale_flag(monkeypatch, tmp_path, capsys): + G = nx.Graph() + G.add_node( + "auth_node", + label="authenticate", + source_file="auth.py", + source_location="L10", + community=0, + rationale="Decision: Argon2id hashing.\n\nWHY: Resistant to GPU cracking.", + ) + graph_path = tmp_path / "graph.json" + graph_path.write_text(json.dumps(json_graph.node_link_data(G, edges="links"))) + + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + + # Without --rationale + monkeypatch.setattr( + mainmod.sys, + "argv", + ["graphify", "query", "authenticate", "--graph", str(graph_path)], + ) + mainmod.main() + out_default = capsys.readouterr().out + assert "WHY" not in out_default + + # With --rationale + monkeypatch.setattr( + mainmod.sys, + "argv", + ["graphify", "query", "authenticate", "--graph", str(graph_path), "--rationale"], + ) + mainmod.main() + out_rat = capsys.readouterr().out + assert "WHY Decision: Argon2id hashing. WHY: Resistant to GPU cracking." in out_rat diff --git a/tests/test_security.py b/tests/test_security.py index 74669f9383..500ca394e5 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -461,3 +461,105 @@ def test_sanitize_metadata_bool_not_coerced_to_int(): assert out["flag_t"] is True assert out["flag_f"] is False assert out["num"] == 1 + + +# --------------------------------------------------------------------------- +# sanitize_rationale +# --------------------------------------------------------------------------- + +from graphify.security import ( + sanitize_rationale, + MAX_QUERY_RATIONALE_CHARS, + MAX_DETAIL_RATIONALE_CHARS, +) + + +def test_sanitize_rationale_empty_or_none(): + assert sanitize_rationale(None) == "" + assert sanitize_rationale("") == "" + assert sanitize_rationale(" \n\t ") == "" + + +def test_sanitize_rationale_single_line_normalizes_whitespace(): + raw = "Decision: Use tree-sitter.\n\nWHY: Deterministic\tand fast.\r\nRobust." + out = sanitize_rationale(raw, single_line=True) + assert "\n" not in out + assert "\t" not in out + assert "\r" not in out + assert out == "Decision: Use tree-sitter. WHY: Deterministic and fast. Robust." + + +def test_sanitize_rationale_single_line_strips_control_chars(): + raw = "Good\x00 rationale\x1b[31m with ansi\x07" + out = sanitize_rationale(raw, single_line=True) + assert "\x00" not in out + assert "\x1b" not in out + assert "\x07" not in out + assert "Good rationale[31m with ansi" in out + + +def test_sanitize_rationale_single_line_truncation(): + long_text = "A" * (MAX_QUERY_RATIONALE_CHARS + 100) + out = sanitize_rationale(long_text, single_line=True) + assert len(out) == MAX_QUERY_RATIONALE_CHARS + assert out.endswith("...") + + +def test_sanitize_rationale_detail_mode_preserves_newlines(): + raw = "Decision: Pick SQLite.\n\nWHY: Zero config." + out = sanitize_rationale(raw, single_line=False) + assert out == "Decision: Pick SQLite.\n\nWHY: Zero config." + + +def test_sanitize_rationale_detail_mode_truncation(): + long_text = "B" * (MAX_DETAIL_RATIONALE_CHARS + 200) + out = sanitize_rationale(long_text, single_line=False) + assert len(out) == MAX_DETAIL_RATIONALE_CHARS + assert out.endswith("...") + + +def test_sanitize_rationale_strips_c1_control_chars_compact_and_detail(): + # Test full range of C1 controls U+0080 through U+009F + c1_chars = "".join(chr(cp) for cp in range(0x80, 0xA0)) + raw = f"Prefix{c1_chars}Suffix" + + out_compact = sanitize_rationale(raw, single_line=True) + assert all(chr(cp) not in out_compact for cp in range(0x80, 0xA0)) + + out_detail = sanitize_rationale(raw, single_line=False) + assert all(chr(cp) not in out_detail for cp in range(0x80, 0xA0)) + assert out_detail == "PrefixSuffix" + + # Non-whitespace C1 controls strip without word splitting + c1_non_ws = "".join(chr(cp) for cp in range(0x80, 0xA0) if cp != 0x85) + assert sanitize_rationale(f"Prefix{c1_non_ws}Suffix", single_line=True) == "PrefixSuffix" + + +def test_sanitize_rationale_strips_dangerous_c1_introducers(): + # U+009B (CSI), U+009D (OSC), U+0090 (DCS), U+009F (APC) + raw = "Safe\x9b31mColor\x9dTitle\x90Device\x9fApp" + + out_compact = sanitize_rationale(raw, single_line=True) + assert "\x9b" not in out_compact + assert "\x9d" not in out_compact + assert "\x90" not in out_compact + assert "\x9f" not in out_compact + assert out_compact == "Safe31mColorTitleDeviceApp" + + out_detail = sanitize_rationale(raw, single_line=False) + assert "\x9b" not in out_detail + assert "\x9d" not in out_detail + assert "\x90" not in out_detail + assert "\x9f" not in out_detail + assert out_detail == "Safe31mColorTitleDeviceApp" + + +def test_sanitize_rationale_preserves_printable_unicode(): + raw = "Decisión: café con leche.\n\nWHY: 日本語サポート and fast 🚀 emojis." + + out_compact = sanitize_rationale(raw, single_line=True) + assert out_compact == "Decisión: café con leche. WHY: 日本語サポート and fast 🚀 emojis." + + out_detail = sanitize_rationale(raw, single_line=False) + assert out_detail == "Decisión: café con leche.\n\nWHY: 日本語サポート and fast 🚀 emojis." + assert "\n\n" in out_detail diff --git a/tests/test_serve.py b/tests/test_serve.py index 224f9786b6..19d0ebdb8b 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -1991,3 +1991,361 @@ def test_query_graph_text_surfaces_parallel_relationships(tmp_path): text = _query_graph_text(G, "applySeed", token_budget=2000) assert "--contains " in text, text assert "--calls " in text, text + + +# --- Rationale Rendering Tests (#2821) --- + +def test_subgraph_to_text_rationale_disabled_by_default(): + G = nx.DiGraph() + G.add_node("n1", label="Parser", source_file="p.py", source_location="L10", community=0, rationale="Use tree-sitter for speed") + out = _subgraph_to_text(G, {"n1"}, []) + assert "WHY" not in out + assert "tree-sitter" not in out + assert out.strip() == "NODE Parser [src=p.py loc=L10 community=0]" + + +def test_subgraph_to_text_rationale_enabled_renders_why(): + G = nx.DiGraph() + G.add_node("n1", label="Parser", source_file="p.py", source_location="L10", community=0, rationale="Use tree-sitter for speed") + out = _subgraph_to_text(G, {"n1"}, [], include_rationale=True) + assert "WHY Use tree-sitter for speed" in out + assert out.strip() == "NODE Parser [src=p.py loc=L10 community=0] WHY Use tree-sitter for speed" + + +def test_subgraph_to_text_rationale_multiline_normalized(): + G = nx.DiGraph() + G.add_node("n1", label="Auth", source_file="a.py", source_location="L1", community=0, rationale="Decision: JWT tokens.\n\nWHY: Stateless auth across services.") + out = _subgraph_to_text(G, {"n1"}, [], include_rationale=True) + assert "\n" not in out.strip() + assert "WHY Decision: JWT tokens. WHY: Stateless auth across services." in out + + +def test_subgraph_to_text_rationale_empty_or_missing_omitted(): + G = nx.DiGraph() + G.add_node("n1", label="NoRat", source_file="a.py", source_location="L1", community=0) + G.add_node("n2", label="EmptyRat", source_file="b.py", source_location="L2", community=0, rationale=" \n\t ") + G.add_node("n3", label="NoneRat", source_file="c.py", source_location="L3", community=0, rationale=None) + out = _subgraph_to_text(G, {"n1", "n2", "n3"}, [], include_rationale=True) + assert "WHY" not in out + + +def test_subgraph_to_text_rationale_capped_at_512(): + G = nx.DiGraph() + long_rationale = "Word " * 200 + G.add_node("n1", label="Long", source_file="l.py", source_location="L1", community=0, rationale=long_rationale) + out = _subgraph_to_text(G, {"n1"}, [], include_rationale=True) + node_line = out.strip() + why_part = node_line.split(" WHY ", 1)[1] + assert len(why_part) == 512 + assert why_part.endswith("...") + + +def test_subgraph_to_text_rationale_counts_toward_token_budget(): + G = nx.DiGraph() + G.add_node("s1", label="SeedNode", source_file="s.py", source_location="L1", community=0, rationale="A" * 200) + G.add_node("n1", label="Neighbor1", source_file="n.py", source_location="L1", community=0) + G.add_edge("s1", "n1", relation="calls") + # Small budget where rationale pushes neighbor past cut threshold + out_without = _subgraph_to_text(G, {"s1", "n1"}, [("s1", "n1")], token_budget=45, seeds=["s1"], include_rationale=False) + out_with = _subgraph_to_text(G, {"s1", "n1"}, [("s1", "n1")], token_budget=45, seeds=["s1"], include_rationale=True) + assert "Neighbor1" in out_without + assert "TRUNCATED" in out_with + assert "WHY" in out_with + + +def test_query_graph_text_forward_include_rationale(): + G = nx.DiGraph() + G.add_node("auth_mod", label="AuthModule", source_file="auth.py", source_location="L1", community=0, rationale="Central auth gateway") + out_no_rat = _query_graph_text(G, "AuthModule", include_rationale=False) + out_rat = _query_graph_text(G, "AuthModule", include_rationale=True) + assert "WHY" not in out_no_rat + assert "WHY Central auth gateway" in out_rat + + +def test_tool_get_node_and_query_graph_mcp(tmp_path): + import asyncio + import mcp.types as types + from graphify.serve import _build_server + from graphify.export import to_json + + G = nx.DiGraph() + G.add_node( + "auth_service", + label="AuthService", + source_file="auth.py", + source_location="L10", + file_type="code", + community=1, + rationale="Decision: OAuth2 compliance.\n\nWHY: Standard protocol.", + ) + gp = tmp_path / "graph.json" + to_json(G, {1: ["auth_service"]}, str(gp)) + + server = _build_server(str(gp)) + + # Test list_tools schema + list_handler = server.request_handlers[types.ListToolsRequest] + tools_res = asyncio.run(list_handler(types.ListToolsRequest())) + tools_by_name = {t.name: t for t in tools_res.root.tools} + + assert "include_rationale" in tools_by_name["query_graph"].inputSchema["properties"] + assert "include_rationale" in tools_by_name["get_node"].inputSchema["properties"] + + # Test call_tool get_node without rationale (default) + call_handler = server.request_handlers[types.CallToolRequest] + res_no_rat = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": False}, + ) + ) + ) + ) + text_no_rat = res_no_rat.root.content[0].text + assert "Rationale:" not in text_no_rat + + # Test call_tool get_node with rationale + res_rat = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": True}, + ) + ) + ) + ) + text_rat = res_rat.root.content[0].text + assert " Rationale: Decision: OAuth2 compliance.\n\nWHY: Standard protocol." in text_rat + + # Test call_tool query_graph without rationale + q_no_rat = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": False}, + ) + ) + ) + ) + q_text_no_rat = q_no_rat.root.content[0].text + assert "WHY" not in q_text_no_rat + + # Test call_tool query_graph with rationale + q_rat = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": True}, + ) + ) + ) + ) + q_text_rat = q_rat.root.content[0].text + assert "WHY Decision: OAuth2 compliance. WHY: Standard protocol." in q_text_rat + + +def test_mcp_backward_compatibility_omitted_include_rationale(tmp_path): + """Regression test: calling get_node and query_graph without include_rationale in arguments + + must succeed, omit rationale/WHY, and preserve exact previous default output. + """ + import asyncio + import mcp.types as types + from graphify.serve import _build_server + from graphify.export import to_json + + G = nx.DiGraph() + G.add_node( + "auth_service", + label="AuthService", + source_file="auth.py", + source_location="L10", + file_type="code", + community=1, + rationale="Decision: OAuth2 compliance.\n\nWHY: Standard protocol.", + ) + gp = tmp_path / "graph.json" + to_json(G, {1: ["auth_service"]}, str(gp)) + + server = _build_server(str(gp)) + call_handler = server.request_handlers[types.CallToolRequest] + + # Call get_node with ONLY {"label": "AuthService"} + res = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService"}, + ) + ) + ) + ) + text = res.root.content[0].text + assert "Rationale:" not in text + expected_lines = [ + "Node: AuthService", + " ID: auth_service", + " Source: auth.py L10", + " Type: code", + " Community: 1", + " Degree: 0", + ] + assert text == "\n".join(expected_lines) + + # Call query_graph with ONLY {"question": "AuthService"} + q_res = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService"}, + ) + ) + ) + ) + q_text = q_res.root.content[0].text + assert "WHY" not in q_text + assert "NODE AuthService [src=auth.py loc=L10 community=1]" in q_text + + +def test_mcp_strict_boolean_include_rationale(tmp_path): + """Regression test: string 'false'/'true' or truthy non-booleans must NOT enable rationale.""" + import asyncio + import mcp.types as types + from graphify.serve import _build_server + from graphify.export import to_json + + G = nx.DiGraph() + G.add_node( + "auth_service", + label="AuthService", + source_file="auth.py", + source_location="L10", + file_type="code", + community=1, + rationale="Decision: OAuth2 compliance.\n\nWHY: Standard protocol.", + ) + gp = tmp_path / "graph.json" + to_json(G, {1: ["auth_service"]}, str(gp)) + + server = _build_server(str(gp)) + call_handler = server.request_handlers[types.CallToolRequest] + + # 1. String "false" + res_get_str_false = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": "false"}, + ) + ) + ) + ) + assert "Rationale:" not in res_get_str_false.root.content[0].text + + res_q_str_false = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": "false"}, + ) + ) + ) + ) + assert "WHY" not in res_q_str_false.root.content[0].text + + # 2. String "true" + res_get_str_true = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": "true"}, + ) + ) + ) + ) + assert "Rationale:" not in res_get_str_true.root.content[0].text + + res_q_str_true = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": "true"}, + ) + ) + ) + ) + assert "WHY" not in res_q_str_true.root.content[0].text + + # 3. None / integer truthy + res_get_int = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": 1}, + ) + ) + ) + ) + assert "Rationale:" not in res_get_int.root.content[0].text + + # 4. Actual True + res_get_true = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": True}, + ) + ) + ) + ) + assert " Rationale: Decision: OAuth2 compliance.\n\nWHY: Standard protocol." in res_get_true.root.content[0].text + + res_q_true = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": True}, + ) + ) + ) + ) + assert "WHY Decision: OAuth2 compliance. WHY: Standard protocol." in res_q_true.root.content[0].text + + # 5. Actual False + res_get_false = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="get_node", + arguments={"label": "AuthService", "include_rationale": False}, + ) + ) + ) + ) + assert "Rationale:" not in res_get_false.root.content[0].text + + res_q_false = asyncio.run( + call_handler( + types.CallToolRequest( + params=types.CallToolRequestParams( + name="query_graph", + arguments={"question": "AuthService", "include_rationale": False}, + ) + ) + ) + ) + assert "WHY" not in res_q_false.root.content[0].text From dda8e2ef33b39dcd9a436e896142acafa01011f7 Mon Sep 17 00:00:00 2001 From: yotamleo <88616986+yotamleo@users.noreply.github.com> Date: Tue, 20 Oct 2026 03:09:56 +0200 Subject: [PATCH 2/2] fix: harden rationale rendering against forged fields and invisible characters (#2821) Review follow-ups on the rebased #2830: - indent continuation lines in explain/get_node so a rationale cannot pose as a sibling field - strip bidi/zero-width characters; treat U+2028, U+2029 and NEL as line breaks - keep the shared control-char regex unchanged; the C1 range applies to rationale only - ignore non-string rationale values; clamp a negative max_chars - list --rationale in the main help text Co-Authored-By: Claude Sonnet 5.5 --- graphify/__main__.py | 2 ++ graphify/cli.py | 4 +-- graphify/security.py | 58 ++++++++++++++++++++---------------------- graphify/serve.py | 3 ++- tests/test_security.py | 31 +++++++++++++++++++++- tests/test_serve.py | 4 +-- 6 files changed, 65 insertions(+), 37 deletions(-) diff --git a/graphify/__main__.py b/graphify/__main__.py index 2b2f44dd7d..64a18a404e 100644 --- a/graphify/__main__.py +++ b/graphify/__main__.py @@ -862,6 +862,7 @@ def _run_cli() -> None: print(" --undirected ignore edge direction") print(" explain \"X\" plain-language explanation of a node and its neighbors") print(" --all list every connection (default: the 20 most connected)") + print(" --rationale show the node's stored rationale when present") print(" --graph path to graph.json (default graphify-out/graph.json)") print(" diagnose multigraph report same-endpoint edge collapse risk in graph.json") print(" --graph path to graph/extraction JSON") @@ -911,6 +912,7 @@ def _run_cli() -> None: print(" --seed X start at node X (id, label or file path; repeatable)") print(" instead of the question's keyword matches") print(" --budget N cap output at N tokens (default 2000)") + print(" --rationale show each node's stored rationale (WHY) when present") print(" --graph path to graph.json (default graphify-out/graph.json)") print(" query, path and explain exit 0 with an answer, 3 when a name is ambiguous") print(" (the candidates are listed), 4 when no node matches") diff --git a/graphify/cli.py b/graphify/cli.py index acd61b94d8..8fe7ac87f4 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -2126,14 +2126,14 @@ def dispatch_command(cmd: str) -> None: print(f" Type: {d.get('file_type', '')}") print(f" Community: {d.get('community_name') or d.get('community', '')}") if include_rationale: - from graphify.security import sanitize_rationale, MAX_DETAIL_RATIONALE_CHARS + from graphify.security import sanitize_rationale, indent_continuation, MAX_DETAIL_RATIONALE_CHARS rat = sanitize_rationale( d.get("rationale"), single_line=False, max_chars=MAX_DETAIL_RATIONALE_CHARS, ) if rat: - print(f" Rationale: {rat}") + print(" Rationale: " + indent_continuation(rat)) # Work-memory overlay: a derived experiential hint from `graphify reflect`, # merged in display-only from the .graphify_learning.json sidecar next to # graph.json. No line when the node has no overlay entry. diff --git a/graphify/security.py b/graphify/security.py index 980e6ae187..74c5f765b8 100644 --- a/graphify/security.py +++ b/graphify/security.py @@ -387,7 +387,7 @@ def check_graph_file_size_cap(path: Path) -> None: # Label sanitisation (mirrors code-review-graph's _sanitize_name pattern) # --------------------------------------------------------------------------- -_CONTROL_CHAR_RE = re.compile(r"[\x00-\x1f\x7f-\x9f]") +_CONTROL_CHAR_RE = re.compile(r"[\x00-\x1f\x7f]") _MAX_LABEL_LEN = 256 @@ -409,6 +409,23 @@ def sanitize_label(text: str | None) -> str: MAX_DETAIL_RATIONALE_CHARS = 2048 _CONTROL_CHAR_NO_NL_RE = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]") +_INVISIBLE_CHAR_RE = re.compile("[\u200b-\u200f\u202a-\u202e\u2060-\u2069\ufeff]") +_LINE_BREAK_RE = re.compile("[\u2028\u2029\x85]") + + +def _cap_rationale(text: str, max_chars: int) -> str: + if len(text) <= max_chars: + return text + marker = "..." + if max_chars <= len(marker): + return text[:max_chars] + return text[: max_chars - len(marker)].rstrip() + marker + + +def indent_continuation(text: str, prefix: str = " ") -> str: + """Indent every non-empty line after the first so a multi-line value cannot pose as a sibling field.""" + first, *rest = text.split("\n") + return "\n".join([first, *(prefix + line if line else line for line in rest)]) def sanitize_rationale( @@ -421,50 +438,29 @@ def sanitize_rationale( In compact (single-line) mode: - Normalizes all whitespace runs (newlines, tabs, spaces) to single spaces - - Strips control characters + - Strips control, bidi-override and zero-width characters - Caps output at `max_chars` (default MAX_QUERY_RATIONALE_CHARS = 512) - Adds an explicit '...' truncation marker when capped In detail mode: - - Preserves newlines/paragraphs while stripping unsafe control characters + - Preserves newlines/paragraphs while stripping unsafe characters - Caps output at `max_chars` (default MAX_DETAIL_RATIONALE_CHARS = 2048) - Adds an explicit '...' truncation marker when capped - Returns empty string if text is None, empty, or whitespace-only. + Returns empty string if text is not a string, empty, or whitespace-only. """ - if text is None: - return "" - raw = str(text) - if not raw.strip(): + if not isinstance(text, str) or not text.strip(): return "" if max_chars is None: max_chars = MAX_QUERY_RATIONALE_CHARS if single_line else MAX_DETAIL_RATIONALE_CHARS + max_chars = max(0, max_chars) + normalized = _LINE_BREAK_RE.sub("\n", text.replace("\r\n", "\n").replace("\r", "\n")) + cleaned = _INVISIBLE_CHAR_RE.sub("", _CONTROL_CHAR_NO_NL_RE.sub("", normalized)) if single_line: - # Collapse all whitespace sequences to a single space, then strip control chars - flat = " ".join(raw.split()) - cleaned = _CONTROL_CHAR_RE.sub("", flat).strip() - if not cleaned: - return "" - if len(cleaned) > max_chars: - marker = "..." - if max_chars <= len(marker): - return cleaned[:max_chars] - return cleaned[: max_chars - len(marker)].rstrip() + marker - return cleaned - else: - # Preserve newlines/paragraphs, normalize CRLF to LF, strip other control chars - normalized = raw.replace("\r\n", "\n").replace("\r", "\n") - cleaned = _CONTROL_CHAR_NO_NL_RE.sub("", normalized).strip() - if not cleaned: - return "" - if len(cleaned) > max_chars: - marker = "..." - if max_chars <= len(marker): - return cleaned[:max_chars] - return cleaned[: max_chars - len(marker)].rstrip() + marker - return cleaned + cleaned = " ".join(cleaned.split()) + return _cap_rationale(cleaned.strip(), max_chars) # --------------------------------------------------------------------------- diff --git a/graphify/serve.py b/graphify/serve.py index dcf33abdc3..8950a9249a 100644 --- a/graphify/serve.py +++ b/graphify/serve.py @@ -17,6 +17,7 @@ sanitize_label, check_graph_file_size_cap, sanitize_rationale, + indent_continuation, MAX_QUERY_RATIONALE_CHARS, MAX_DETAIL_RATIONALE_CHARS, _CONTROL_CHAR_RE, @@ -2462,7 +2463,7 @@ def _tool_get_node(arguments: dict) -> str: max_chars=MAX_DETAIL_RATIONALE_CHARS, ) if rat: - lines.append(f" Rationale: {rat}") + lines.append(" Rationale: " + indent_continuation(rat)) lines.append(f" Degree: {G.degree(nid)}") lines.extend(attrs_line) return "\n".join(lines) diff --git a/tests/test_security.py b/tests/test_security.py index 500ca394e5..7d8082daac 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -469,6 +469,7 @@ def test_sanitize_metadata_bool_not_coerced_to_int(): from graphify.security import ( sanitize_rationale, + indent_continuation, MAX_QUERY_RATIONALE_CHARS, MAX_DETAIL_RATIONALE_CHARS, ) @@ -528,7 +529,7 @@ def test_sanitize_rationale_strips_c1_control_chars_compact_and_detail(): out_detail = sanitize_rationale(raw, single_line=False) assert all(chr(cp) not in out_detail for cp in range(0x80, 0xA0)) - assert out_detail == "PrefixSuffix" + assert out_detail == "Prefix\nSuffix" # U+0085 (NEL) is a line break # Non-whitespace C1 controls strip without word splitting c1_non_ws = "".join(chr(cp) for cp in range(0x80, 0xA0) if cp != 0x85) @@ -563,3 +564,31 @@ def test_sanitize_rationale_preserves_printable_unicode(): out_detail = sanitize_rationale(raw, single_line=False) assert out_detail == "Decisión: café con leche.\n\nWHY: 日本語サポート and fast 🚀 emojis." assert "\n\n" in out_detail + + +def test_sanitize_rationale_unicode_line_separators_and_invisibles(): + raw = "a
b
c\x85d‮evil​x" + + assert sanitize_rationale(raw, single_line=True) == "a b c devilx" + assert sanitize_rationale(raw, single_line=False) == "a\nb\nc\ndevilx" + + +def test_sanitize_rationale_non_string_and_negative_cap(): + assert sanitize_rationale({"a": 1}) == "" + assert sanitize_rationale(0) == "" + assert sanitize_rationale("abc", single_line=True, max_chars=-1) == "" + + +def test_sanitize_rationale_control_between_spaces_collapses_once(): + assert sanitize_rationale("a \x01 b", single_line=True) == "a b" + + +def test_sanitize_label_unchanged_by_rationale_support(): + assert sanitize_label("a\x9b31mb") == "a\x9b31mb" + + +def test_indent_continuation_blocks_forged_fields(): + out = indent_continuation("ok\n Degree: 9999\n\nNode: fake") + + assert out == "ok\n Degree: 9999\n\n Node: fake" + assert not any(line.startswith(" Degree") for line in out.splitlines()) diff --git a/tests/test_serve.py b/tests/test_serve.py index 19d0ebdb8b..a72300e8f0 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -2118,7 +2118,7 @@ def test_tool_get_node_and_query_graph_mcp(tmp_path): ) ) text_rat = res_rat.root.content[0].text - assert " Rationale: Decision: OAuth2 compliance.\n\nWHY: Standard protocol." in text_rat + assert " Rationale: Decision: OAuth2 compliance.\n\n WHY: Standard protocol." in text_rat # Test call_tool query_graph without rationale q_no_rat = asyncio.run( @@ -2311,7 +2311,7 @@ def test_mcp_strict_boolean_include_rationale(tmp_path): ) ) ) - assert " Rationale: Decision: OAuth2 compliance.\n\nWHY: Standard protocol." in res_get_true.root.content[0].text + assert " Rationale: Decision: OAuth2 compliance.\n\n WHY: Standard protocol." in res_get_true.root.content[0].text res_q_true = asyncio.run( call_handler(