fix: address copilot review + improve diff output

- graph_utils: clear error message on invalid git ref
- scan_analysis: remove unused imports (degree, largest_scc_size)
- html_renderer: defensive isinstance guard in match_group;
  fix expanded cluster edges — children now show actual connections
  via rawEdges projection instead of appearing isolated
- scan_topology: eliminate duplicate graph/SCC computation in cmd_scan;
  smart diff default (dirty worktree → HEAD vs worktree, clean → HEAD~1 vs HEAD);
  expand diff human output with SCC changes, fixed/new violations per edge
- add requirements.txt declaring json5 dependency
- AGENTS.md: document diff default behavior and ref syntax

Author: jensenojs

scripts/dependency-topology/AGENTS.md

@@ -25,20 +25,29 @@ python3 scripts/dependency-topology/scan_topology.py scan -o /tmp/deps.html
 # JSON output (for scripts/agents)
 python3 scripts/dependency-topology/scan_topology.py scan --json
 
-# Compare HEAD vs working tree (default)
+# Diff — smart default:
+#   worktree has uncommitted Lua changes → HEAD vs worktree
+#   worktree is clean                   → HEAD~1 vs HEAD (last commit)
 python3 scripts/dependency-topology/scan_topology.py diff
 
-# Compare specific refs
-python3 scripts/dependency-topology/scan_topology.py diff --from main --to HEAD
+# Compare specific refs (branch names, commit SHAs, remote refs)
+python3 scripts/dependency-topology/scan_topology.py diff --from upstream/main --to clean-code-remove-core
+python3 scripts/dependency-topology/scan_topology.py diff --from HEAD~5 --to HEAD
 ```
 
 ## Snapshot References
 
 - `worktree` — current working tree (uncommitted changes)
 - `HEAD` — latest commit
-- Any git ref — branch name, tag, commit SHA
+- Any git ref — branch name (e.g. `upstream/main`), tag, short or full commit SHA
+- Relative refs — `HEAD~1`, `HEAD^`
 
-**diff defaults:** `--from HEAD --to worktree`
+**diff defaults (no args):**
+- Worktree has uncommitted Lua changes → `HEAD` vs `worktree`
+- Worktree is clean → `HEAD~1` vs `HEAD`
+
+Note: ambiguous short names (e.g. `upstream` when both a local branch and remote exist)
+produce a git warning. Prefer fully-qualified refs: `upstream/main`, `refs/heads/mybranch`.
 
 ## Output
 

scripts/dependency-topology/graph_utils.py

@@ -52,7 +52,14 @@ def _worktree_files(repo: Path) -> List[Tuple[str, str]]:
 
 def _git_files(repo: Path, ref: str) -> List[Tuple[str, str]]:
     cmd = ["git", "ls-tree", "-r", "--name-only", ref, "lua/opencode"]
-    ls = subprocess.check_output(cmd, cwd=repo, text=True)
+    try:
+        ls = subprocess.check_output(cmd, cwd=repo, text=True, stderr=subprocess.PIPE)
+    except subprocess.CalledProcessError as e:
+        stderr = e.stderr.strip() if e.stderr else ""
+        raise ValueError(
+            f"Invalid snapshot ref '{ref}'. Valid values: HEAD, worktree, branch name, commit SHA.\n"
+            f"git error: {stderr}"
+        ) from None
 
     out: List[Tuple[str, str]] = []
     for rel in ls.splitlines():

scripts/dependency-topology/html_renderer.py

@@ -37,9 +37,13 @@ class ClusterEdge:
 def match_group(module: str, groups: Dict[str, Any]) -> str:
     """Match module to group using fnmatch patterns."""
     for group_name, group_data in groups.items():
+        if not isinstance(group_data, dict):
+            continue
         patterns = group_data.get("modules", [])
+        if not isinstance(patterns, list):
+            continue
         for pattern in patterns:
-            if fnmatch.fnmatch(module, pattern):
+            if isinstance(pattern, str) and fnmatch.fnmatch(module, pattern):
                 return group_name
     return "ungrouped"
 
@@ -134,7 +138,7 @@ def render_html(payload: dict, groups: Dict[str, Any], cluster_depth: int = 2) -
         node_list, edge_list, scc_nodes, groups, depth=cluster_depth
     )
 
-    # Build GRAPH data
+    # Build GRAPH data — cluster edges for default view, rawEdges for expand
     graph_data = {
         'nodes': [
             {
@@ -149,13 +153,25 @@ def render_html(payload: dict, groups: Dict[str, Any], cluster_depth: int = 2) -
         ],
         'edges': [
             {
-                'src': e.src, 
-                'dst': e.dst, 
+                'src': e.src,
+                'dst': e.dst,
                 'weight': e.weight,
                 'isViolation': e.is_violation,
                 'rule': e.rule
             }
             for e in cluster_edges
+        ],
+        # Raw module-level edges — used when clusters are expanded so children
+        # can show their actual connections instead of appearing isolated.
+        'rawEdges': [
+            {
+                'src': src,
+                'dst': dst,
+                'isViolation': bool(match_group(src, groups) and
+                    _edge_rule(match_group(src, groups), match_group(dst, groups))),
+                'rule': _edge_rule(match_group(src, groups), match_group(dst, groups)),
+            }
+            for src, dst in edge_list
         ]
     }
 
@@ -451,19 +467,50 @@ def render_html(payload: dict, groups: Dict[str, Any], cluster_depth: int = 2) -
     }
   });
   
-  // Build edges between visible nodes
+  // Build edges between visible nodes.
+  // If any cluster is expanded, use rawEdges (module-level) and project them:
+  //   - if endpoint is a visible child → use as-is
+  //   - if endpoint's parent cluster is collapsed → map to cluster id
+  //   - if endpoint is ungrouped leaf → use as-is
   const nodeIdSet = new Set(currentNodes.map(n => n.id));
-  
-  // Use original edges, map to visible nodes
-  GRAPH.edges.forEach(e => {
-    let src = e.src, dst = e.dst;
-    // If src/dst is expanded cluster, need to find which child
-    // For simplicity, show edge if both endpoints visible or map to cluster
-    if (nodeIdSet.has(src) && nodeIdSet.has(dst)) {
-      currentEdges.push({ src, dst, isViolation: e.isViolation, rule: e.rule });
+
+  // Build child→parentCluster reverse lookup for projection
+  const childToCluster = {};
+  currentNodes.forEach(n => { if (n.parentCluster) childToCluster[n.id] = n.parentCluster; });
+  GRAPH.nodes.forEach(n => {
+    if (n.isCluster && n.children) {
+      n.children.forEach(c => { if (!childToCluster[c]) childToCluster[c] = n.id; });
     }
   });
-  
+
+  function resolveEndpoint(id) {
+    if (nodeIdSet.has(id)) return id;           // directly visible
+    const cluster = childToCluster[id];
+    if (cluster && nodeIdSet.has(cluster)) return cluster;  // map to collapsed cluster
+    return null;
+  }
+
+  if (expandedClusters.size > 0) {
+    // Use raw module-level edges and project to visible nodes
+    const seen = new Set();
+    GRAPH.rawEdges.forEach(e => {
+      const src = resolveEndpoint(e.src);
+      const dst = resolveEndpoint(e.dst);
+      if (!src || !dst || src === dst) return;
+      const key = src + '|' + dst;
+      if (seen.has(key)) return;
+      seen.add(key);
+      currentEdges.push({ src, dst, isViolation: e.isViolation, rule: e.rule });
+    });
+  } else {
+    // All clusters collapsed: use pre-aggregated cluster edges (faster)
+    GRAPH.edges.forEach(e => {
+      if (nodeIdSet.has(e.src) && nodeIdSet.has(e.dst)) {
+        currentEdges.push({ src: e.src, dst: e.dst, isViolation: e.isViolation, rule: e.rule });
+      }
+    });
+  }
+
   renderGraph();
 }
 

scripts/dependency-topology/requirements.txt

@@ -0,0 +1,4 @@
+# Dependencies for the dependency topology scanner.
+# Install with: pip install -r scripts/dependency-topology/requirements.txt
+
+json5>=0.9.0   # JSONC parser — required for topology.jsonc (supports // and /* */ comments)

scripts/dependency-topology/scan_analysis.py

@@ -9,7 +9,7 @@
 from pathlib import Path
 from typing import Any, Dict, List, Set, Tuple
 
-from graph_utils import back_edges, degree, largest_scc_size, load_snapshot_graph, tarjan_scc, find_cycle_in_scc
+from graph_utils import back_edges, load_snapshot_graph, tarjan_scc, find_cycle_in_scc
 
 
 _POLICY_RULES: List[Dict[str, Any]] = []
@@ -304,6 +304,10 @@ def scc_severity(size: int) -> str:
 
         # Layer coverage — confirms all modules are classified
         "group_coverage": grouped_counts,
+
+        # Internal: raw graph for HTML rendering (not included in --json output)
+        "_graph": graph,
+        "_sccs": comps,
     }
 
 

scripts/dependency-topology/scan_topology.py

@@ -31,7 +31,7 @@
 import textwrap
 from pathlib import Path
 
-from graph_utils import load_snapshot_graph, tarjan_scc
+from graph_utils import load_snapshot_graph
 from html_renderer import render_html
 from scan_analysis import (
     init_policy,
@@ -52,13 +52,15 @@ def cmd_scan(args: argparse.Namespace) -> int:
 
     payload = build_scan_payload(repo, snapshot, strategy, top_n=8)
 
+    # Pop internal fields — not part of public JSON output
+    graph = payload.pop("_graph")
+    sccs = payload.pop("_sccs")
+
     if args.json:
         print(json.dumps(payload, indent=2, ensure_ascii=False))
         return 0
 
-    # Generate HTML visualization
-    graph = load_snapshot_graph(repo, snapshot)
-    sccs = tarjan_scc(list(graph.nodes.keys()), list(graph.edges))
+    # Generate HTML visualization — reuse already-computed graph and sccs
     html_payload = {
         "node_list": sorted(graph.nodes.keys()),
         "edge_list": list(graph.edges),
@@ -79,11 +81,27 @@ def cmd_scan(args: argparse.Namespace) -> int:
     print(f"{payload['health']['cycles']['count']} cycles, {violations} violations, {status} → {output}")
     return 0
 
+def _diff_default_snapshots(repo: Path) -> tuple[str, str]:
+    """Smart default for diff with no args:
+    - If worktree has uncommitted changes: HEAD vs worktree
+    - If worktree is clean: HEAD~1 vs HEAD (last commit)
+    """
+    result = subprocess.run(
+        ["git", "status", "--porcelain"],
+        cwd=repo, capture_output=True, text=True
+    )
+    if result.stdout.strip():
+        return "HEAD", "worktree"
+    return "HEAD~1", "HEAD"
+
 
 def cmd_diff(args: argparse.Namespace) -> int:
     repo = Path(args.repo).resolve()
-    from_snap = args.from_snapshot or "HEAD"
-    to_snap = args.to_snapshot or "worktree"
+    if args.from_snapshot or args.to_snapshot:
+        from_snap = args.from_snapshot or "HEAD"
+        to_snap = args.to_snapshot or "worktree"
+    else:
+        from_snap, to_snap = _diff_default_snapshots(repo)
     strategy = load_strategy(args.strategy, DEFAULT_STRATEGY)
     init_policy(strategy.get("policy", {}).get("rules", []))
 
@@ -93,32 +111,66 @@ def cmd_diff(args: argparse.Namespace) -> int:
         print(json.dumps(payload, indent=2, ensure_ascii=False))
         return 0
 
-    # Human-readable summary
+    # Human-readable — show meaningful detail, not just counts
     hc = payload["health_comparison"]
     cyc = hc["cycles"]
     vio = hc["violations"]
 
-    parts = [f"{from_snap} → {to_snap}:"]
-    parts.append(f"edges {payload['edge_changes']['added']:+d}/{-payload['edge_changes']['removed']}")
+    print(f"{from_snap} → {to_snap}")
+    print()
 
+    # Health overview
     cyc_delta = cyc["largest"]["delta"]
-    if cyc_delta:
-        parts.append(f"largest_cycle {cyc['largest']['from']}→{cyc['largest']['to']} ({cyc_delta:+d})")
-
     vio_delta = vio["delta"]
-    parts.append(f"violations {vio['from']}→{vio['to']} ({vio_delta:+d})")
-
+    print(f"  cycles:     {cyc['count']['from']}→{cyc['count']['to']}  "
+          f"largest {cyc['largest']['from']}→{cyc['largest']['to']} ({cyc_delta:+d})")
+    print(f"  violations: {vio['from']}→{vio['to']} ({vio_delta:+d})")
+    ec = payload["edge_changes"]
+    print(f"  edges:      +{ec['added']}/-{ec['removed']} (net {ec['net']:+d})")
+    print()
+
+    # SCC changes
+    scc_changes = payload.get("scc_changes", [])
+    if scc_changes:
+        print("  SCC changes:")
+        for ch in scc_changes:
+            if "resolved" in ch:
+                print(f"    ✓ resolved  size={ch['from_size']} → gone")
+            elif "new_cycle" in ch:
+                members = ", ".join(ch["new_cycle"][:4])
+                ellipsis = f" +{len(ch['new_cycle'])-4} more" if len(ch["new_cycle"]) > 4 else ""
+                print(f"    ✗ new cycle size={ch['to_size']}: {members}{ellipsis}")
+            else:
+                delta = ch["delta"]
+                if ch["gained_members"]:
+                    print(f"    ~ grew      {ch['from_size']}→{ch['to_size']} ({delta:+d})")
+                    for m in ch["gained_members"]:
+                        print(f"        + {m}")
+                if ch["lost_members"]:
+                    print(f"    ~ shrank    {ch['from_size']}→{ch['to_size']} ({delta:+d})")
+                    for m in ch["lost_members"]:
+                        print(f"        - {m}")
+        print()
+
+    # Violations fixed / introduced
     fixed = payload["violations_fixed"]
-    new = payload["violations_new"]
+    new_v = payload["violations_new"]
     if fixed:
-        parts.append(f"fixed={len(fixed)}")
-    if new:
-        parts.append(f"new={len(new)}")
+        print(f"  Fixed violations ({len(fixed)}):")
+        for v in fixed:
+            print(f"    ✓ [{v['rule']}]  {v['src']} → {v['dst']}")
+        print()
+    if new_v:
+        print(f"  New violations ({len(new_v)}):")
+        for v in new_v:
+            print(f"    ✗ [{v['rule']}]  {v['src']} → {v['dst']}")
+        print()
+    if not fixed and not new_v:
+        print("  No violation changes.")
+        print()
 
-    print("  ".join(parts))
     return 0
 
-
 def main() -> int:
     parser = argparse.ArgumentParser(
         description="Dependency topology scanner — detect layering violations and visualize module dependencies.",