Correctness/robustness bugs in modules untouched by #1/#2/#3 (store, graph, metrics, compose, edges, provenance, ablation, curation)

[jiang4wqy]

Summary

While exercising rgit on real repositories (cloned pallets/click, benjaminp/six, encode/httpx) on Windows, plus a static multi-angle review, I found a set of correctness/robustness bugs that are distinct from the areas covered by #1, #2, and #3.

Those cover capture/watch path collection, diff-header parsing, untracked/symlink capture safety, git-hook classification, rgit run failure visibility + subprocess UTF-8 decoding, and toggle parsing (astmap.py, capture/run/pending/watch parts of cli.py, gitutil.py, hooks.py, runner.py, segmenter.py, watch.py, toggles.py).

Everything below lives in the other modules — store/db.py, graphview.py, output encoding on cli.py print sites (opposite direction from the input-decoding hardening), metrics.py, compose.py, edges.py, provenance.py/astmap.py, compare.py, ablation.py, review/approve/resegment parts of cli.py/curation.py, and mcp_server.py — so there is no file/topic overlap with the open PRs/issue.

Items marked ✅ were reproduced at runtime (or unit level) on origin/main (b5bbf76); the rest are confirmed by source inspection with the exact line cited. A companion PR fixes the four highest-value items (H1–H3, M1).

Line numbers are against main @ b5bbf76.

HIGH

H1 — store/db.py:74 rewrites conflicts_with → overlaps on every Store.open(): silent data loss, and can brick the store ✅

init_schema (run inside Store.__init__ on every open) unconditionally executes

conn.execute("UPDATE edges SET type='overlaps' WHERE type='conflicts_with'")

with no schema-version guard (contrast the column migrations just above it, which are guarded by PRAGMA table_info). But conflicts_with is a current, first-class edge type (graphview._CAP_EDGE_TYPES/_RICHER_SAME_REGION/_SYMMETRIC_EDGES, the ⚔ glyph, the red DOT style, recall._SAME_REGION, and edge-judge.md instructs the agent to write it). Consequences:

• Data loss: any conflicts_with edge (written by the edge-judge, or via rgit edges --add conflicts_with a b) is silently downgraded to overlaps on the next command.
• Brick: edges has UNIQUE(src,dst,type) and add_edge is INSERT OR IGNORE, so overlaps a b and conflicts_with a b can legitimately coexist (this is the normal judge flow: baseline overlaps then upgrade to conflicts_with). The un-guarded UPDATE then collides with the existing overlaps row → sqlite3.IntegrityError: UNIQUE constraint failed: edges.src, edges.dst, edges.type, after which every command that opens the store crashes.

Repro:

rgit init
rgit edges --add conflicts_with capA capB
rgit features            # reopen triggers the UPDATE
# .rgit/graph.db now shows ('capA','capB','overlaps') — conflicts_with destroyed
# --- brick ---
rgit edges --add overlaps capA capB
rgit edges --add conflicts_with capA capB
rgit features            # -> sqlite3.IntegrityError, and every later command too

Fix: remove the two lines (it's a current type, not a legacy alias). If a real one-time legacy migration is ever needed, guard it with PRAGMA user_version and make it collision-safe.

H2 — graph/features/pending --json crash with UnicodeEncodeError on any non-UTF-8 console (e.g. cp936 on Chinese Windows) ✅

cli.main() never forces UTF-8 on stdout, so on Windows the locale codepage (cp936) is used. graphview.to_text prints literal • (line 120) and the markers ≈ ⇄ ⚔ ⇒ → (_markers, lines 71–88); several of these (•, ⚔, ⇄, ⇒) are not in cp936, and arbitrary unicode in a capsule name/intent isn't either. Result: print(render(...)) (cli.py), features (prints name/intent), and pending --json (ensure_ascii=False) raise:

rgit graph --text --runs
UnicodeEncodeError: 'gbk' codec can't encode character '•' in position 21: illegal multibyte sequence

This reproduces with pure-ASCII data (--text --runs always emits •), and for any capsule whose name/intent contains a non-cp936 char (very common when a Chinese/Japanese user writes intent in their language). Storing such a name is fine — it's purely the stdout encoding.
Fix: reconfigure stdout/stderr to UTF-8 at the start of main().

H3 — rgit resegment --from-json - reads stdin with the locale codepage, not UTF-8, corrupting agent-supplied JSON ✅

cli.py:306:

raw = sys.stdin.read() if args.from_json == "-" else Path(args.from_json).read_text(encoding="utf-8")

The file path decodes as UTF-8; the stdin path uses sys.stdin.read() (cp936 on Windows). This is the exact path the official rgit-capture skill uses (echo '<json>' | rgit resegment <pid> --from-json -). A UTF-8 intent of 重排检索 round-trips correctly via the file path but is corrupted via stdin:

FILE  db  = '重排检索'   (== intended)
STDIN db  = '閲嶆帓妫\udc80绱\udca2'   (mojibake + surrogates)

The injected surrogates then crash approve at store.add_feature with UnicodeEncodeError: '\udca2': surrogates not allowed.
Fix: raw = sys.stdin.buffer.read().decode("utf-8") (with a fallback for a patched stdin without .buffer).

MEDIUM

M1 — provenance/astmap report a present nested/dotted symbol as missing ✅

provenance._symbol_from_text (provenance.py:13-22) and astmap.read_symbol_source only scan top-level module.body for stmt.name.value == symbol. But astmap._SymbolFinder records bare method names, and the capsule-segmenter is told to "locate by symbol/class", so slice symbols like Ctx.invoke or train_step occur. They never match a top-level node → _symbol_from_text returns None → provenance flags them missing even when the code is byte-for-byte present (and compose yields current_source=''). Reproduced: a slice Klass.method on a class that genuinely defines it →

[missing] prov-test  Klass.method     # false
summary: {'clean': 0, 'adapted': 0, 'missing': 1}

Fix: resolve symbol.split(".") by descending into ClassDef.body; share one helper between provenance and astmap.

M2 — metrics.parse_metrics accepts NaN/inf ✅

float("nan")/float("inf") don't raise, so a diverged run's loss: nan enters the store (json.dumps→NaN→json.loads→nan). compare then yields delta = val - NaN = NaN for every row, and min/max winner selection becomes order-dependent (max([nan,0.5,0.9])==nan but max([0.5,0.9,nan])==0.9), so a diverged run can be crowned the winner. Fix: reject non-finite (math.isfinite).

M3 — metrics.parse_metrics doesn't validate JSON shape, so a non-dict rgit_metrics.json crashes downstream consumers ✅

metrics.py:21 return json.loads(...) or None with no isinstance(dict) check, violating the Optional[dict] contract. A rgit_metrics.json of [1,2,3] or 0.95 is stored verbatim; then graphview._fmt_metrics(...).items() → AttributeError, ablation dict([1,2,3]) → TypeError, metric-dir suggest for k in 0.9 → TypeError. Verified: parse_metrics returns the bare list/float, and the consumers raise. Fix: return data if isinstance(data, dict) and data else None.

M4 — comma-separated metric lines silently drop metrics ✅

_LINE's value group is (\S+) (metrics.py:7). print('acc=0.9,loss=0.1') → the value matches 0.9,loss=0.1, float() raises, is swallowed, and return found or None yields None → both metrics lost. acc=0.9, loss=0.1 keeps only {'loss':0.1}. Space-separated works. Fix: make the value group a numeric token.

M5 — rgit graph --runs crashes with KeyError on a dangling produced/active edge

graphview._collect (line 47) does store.get_run(rid) for every run id in produced/active edges. edges has no FK, add_edge is a raw INSERT OR IGNORE, and edges --add doesn't validate endpoints, so a dangling edge is easy to create. get_run raises KeyError(rid); the graph handler has no try/except. (_collect lacks the if m in by_id guard that graphview.py:129 uses.) Fix: skip edges whose run row is missing.

M6 — to_mermaid's _mesc escapes only ", not newlines, so a capsule name with a newline breaks the chart

_mesc (graphview.py:191-193) is str(s).replace('"', "#quot;"). Names are unsanitized agent input. A name bad\nname]here splits the node declaration across two lines, the second being invalid Mermaid — directly violating the to_mermaid docstring's "arbitrary names can't break the chart". The run-label <br> path exposes metric keys/values the same way. Fix: also neutralize \r/\n.

M7 — compose keys by capsule name (not unique), collapsing same-named contributors

compose.py:18/30 accumulate/lookup by cap.name; names aren't unique (db.py has no UNIQUE, curation doesn't dedupe). Two distinct capsules named dup touching the same file:symbol → both merge-context contributors resolve to the last one; the other's clean_slice is silently lost. Fix: key by capsule id, use name only as a display label.

M8 — edges.depends_candidates matches dotted symbols against bare identifiers, so dotted-defining capsules never get a depends_on

edges.py:63 stores raw (possibly dotted) s.symbol in defines, while uses (_used_names, _IDENT = [A-Za-z_]\w*) only produces bare identifiers. So {'CustomLoss.__call__'} ∩ {'CustomLoss', ...} is empty (line 72). overlap_pairs already normalizes via _top_symbol — the two functions are inconsistent. Fix: normalize defines with _top_symbol too.

M9 — compare crashes with KeyError on a dangling variant_of edge

_resolve_targets→_variant_cluster returns cluster ids including dangling ones; compare.py:72 caps[i].name raises KeyError for an id not in list_features(). Dangling variant_of is creatable via rgit run --from <typo> + approve (from_features is never validated). MCP compare_tool surfaces the raw KeyError. Fix: filter fids to existing capsules (mirror graphview.py:129).

M10 — rgit review --approve crashes with an uncaught TypeError on a malformed candidate

curation.approve does CodeSlice(**c) (curation.py:35); CodeSlice has 5 required fields (models.py). resegment stores agent JSON verbatim with no validation, so a slice missing anchor/kind → TypeError: missing ... arguments, and an extra field → TypeError: unexpected keyword argument. The CLI only catches (KeyError, ValueError), so TypeError escapes as a traceback. (Relatedly, cli.py's proposal list does c["name"], so a candidate missing name raises an uncaught KeyError that breaks the whole rgit review listing.) Fix: filter/validate slice fields in approve and raise ValueError; use c.get("name","?") in the listing.

M11 — approve() doesn't check proposal status, so re-approving makes duplicate capsules + edges

curation.approve only checks if not prop.candidates:, never prop.status, and candidates are never cleared. A second approve of the same proposal mints a new feat_ id and re-emits touches/produced/variant_of edges. Fix: if prop.status != 'open': raise ValueError(...); guard dismiss() similarly.

M12 — ablation keys subsets/winners by (non-unique) capsule name, producing duplicate rows and double ★

ablation.py:57/69 build subset/winners from tuple(sorted(name[c] ...)); the CLI marks every row whose subset equals the winner. Two distinct capsules named X → both rows marked winner, plus a phantom ('X','X') row. Fix: key by id tuple, display name only.

M13 — an unrelated empty-active-set run contaminates the ablation base cell

_active_set returns frozenset() for a run with no active/produced edges; frozenset() <= target is always true, so a bare rgit run (no --with/--from) — if it's the newest — occupies latest_for[frozenset()] (the base row), violating the "active set equals the subset exactly" contract. Fix: only place a run in base when it's actually associated with the swept capsules.

LOW

L1 — compose doesn't dedupe feature_ids, so passing an id twice fabricates a self-conflict

compose.py iterates the raw list; a repeated id appends cap.name twice → len(names)>1 → a fake conflicts=[{features:['alpha','alpha']}], and the regenerator is told to merge a capsule with itself. Fix: feature_ids = list(dict.fromkeys(feature_ids)).

L2 — depends_candidates doesn't exclude a capsule's own defined symbols, so same-region variants fabricate bidirectional depends_on

A wrap slice's code is the whole file diff (incl. def/class lines), so _used_names(X) includes X's own symbols, and line 72 doesn't subtract defines[x.id]. Two variants that both wrap train → v1 depends_on v2 and the reverse (a cycle) fed to the edge-judge, when it should be alternative_to/overlaps. Fix: shared = (uses[x] - defines[x]) & defines[y].

L3 — MCP read tools return an opaque error (bare id) on an unknown id

get_feature_tool/provenance_tool (mcp_server.py) pass ids straight to the store; store.get_feature/get_run raise bare KeyError(id), so the client sees just the id (the CLI has .strip('"') friendly handling; MCP doesn't). (FastMCP wraps it as isError=True rather than crashing.) Fix: translate to no capsule/run matching '<id>'.

Dedup

None of the above overlaps #1/#2/#3. Those PRs/issue are about capture/watch path collection, diff-header parsing, untracked/symlink capture safety, hook classification, rgit run failure visibility + subprocess UTF-8 decoding, empty-diff/zero-candidate guidance, and toggle parsing. The bugs here are in schema migration, graph rendering + stdout encoding (opposite direction), metrics parsing/validation, compose/edges symbol handling, provenance/astmap nested-symbol resolution, compare/ablation, review/approve/resegment, and the MCP layer.