feat(query): cgis init-ontology — auto-proposed patterns.yaml (#174)#211
Conversation
|
/guardian review |
There was a problem hiding this comment.
Summary: Found 2 critical logic bugs in ontology_init.py (never-branching lineage and auto-descent loop) that cause incorrect domain discovery. 2 tests mask these bugs by expecting the wrong output. No type safety, ontology, or contract issues detected.
🤖 mistral-medium-latest · 25,011 prompt + 1,075 completion = 26,086 tokens · graph 3/7 files (43%)
There was a problem hiding this comment.
Code Review
This pull request implements the cgis init-ontology CLI command and the read-only cgis_init_ontology MCP tool to auto-propose a starter patterns.yaml from a measured graph. The changes introduce a new module src/cgis/query/ontology_init.py for domain discovery and template fitting, append the new commands to the CLI and MCP server, and add extensive unit and round-trip tests. The review feedback suggests two key improvements: parsing the static YAML header once at the module level to avoid redundant parsing during template fitting, and replacing rstrip(".") with removesuffix(".") to prevent unintended character stripping.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
zaebee
left a comment
There was a problem hiding this comment.
Review — strong PR, one blocking gate + a couple of nits
Read the core (ontology_init.py), the CLI/MCP appends, the ratchet, and both fix-rounds. Dogfooded the round-trip on the self-graph: ingest → init-ontology → drift returns exit 0, "All domains within tolerance" — "green by construction" verified end-to-end, zero EMPTY rows. The DriftScorer-as-fitter (zero new scoring math) is the right call — it's exactly why the round-trip can't diverge. MCP being read-only (returns YAML, writes nothing) cleanly sidesteps the S2083 taint class. 👏
🔴 Blocking — Sonar gate fail: new_duplicated_lines_density 4.6% > 3%
The dominant source is the three hygiene branches in _domain_entry (ontology_init.py:265-282): each repeats the same _hygiene_score → _ceil2 → profile.append → drift_tolerance.append block, only the reason string differs. Collapsing them is both the dedup fix and a clarity win (preserves exact output ordering):
hygiene_reason: str | None = None
if fp.node_count < min_nodes:
hygiene_reason = f"# below min_nodes ({fp.node_count} nodes) — census too small to label"
elif fp.edge_count == 0:
hygiene_reason = "# no intra-domain edges — nothing to fit"
elif best > _NO_FIT_THRESHOLD:
hygiene_reason = f"# no template fits (best: {best_name} at {best:.2f})"
if hygiene_reason is not None:
tolerance = _ceil2(_hygiene_score(fp, prefix, scorer, profile) + margin)
lines.append(f" profile: {profile}{profile_suffix}")
lines.append(f" drift_tolerance: {tolerance:.2f} {hygiene_reason}")
else:
comment = (
f"# measured ≈ {best:.2f} via init-ontology"
f" (runner-up: {runner_name} at {runner:.2f}) — ratchet down over time"
)
tolerance = _ceil2(best + margin)
lines.append(f" expected_pattern: {best_name}")
lines.append(f" profile: {profile}{profile_suffix}")
lines.append(f" drift_tolerance: {tolerance:.2f} {comment}")If that alone doesn't clear 3%, the secondary contributor is the per-file node-factory blocks (each new test file defines its own identical Node(...) builder) — consolidating them into one shared test helper would close the rest.
🟡 Non-blocking nits
_fit_templates:182re-parses_DEFAULT_ONTOLOGY_HEADERviayaml.safe_loadon every domain just to readpatterns.keys(). The template-name list is constant — parse once (module-level or hoisted intopropose_ontologyand threaded in). Negligible now, but it's O(domains) redundant YAML parses.- Round-trip at
margin=0.0: tolerance becomes_ceil2(score), which can equal the score exactly on a 2dp boundary. The guarantee then rests on the drift gate being<=(not<). You have themargin=0.0stress test so it's empirically fine — just confirming it's intentional, since a future flip of the gate comparator would silently break it.
Verified / liked
- Both fix-rounds are real and tested: empty-level lineage (
discover_domains:332-336) and profile-threading through_hygiene_score(the cyclic-domain round-trip). - The
cgis.query0.18→0.20 ratchet is documented with rationale — same conscious-re-baseline class as #199's extraction bump (domain legitimately grew). Note it's an increase, so when #151 (ratchet-enforce CI) lands it'll need the carve-out comment it already has.
Once the gate's green this is good to go from my side.
Sonar new-code duplication 4.6% -> shared make_chain_db in conftest; gemini: module-level _TEMPLATE_NAMES, removesuffix idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t [] (#174 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#174 task 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ery ratchet re-baseline (#174 task 2 review) BUG 1: _hygiene_score used profile=None while the yaml emitted the detected profile -> v1 weight divergence broke the round-trip on cyclic domains. BUG 2: ontology_init.py itself grew cgis.query past its 0.18 ratchet -> documented re-baseline to 0.20 (measured 0.19), precedent PR #199. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sk 5) - M1: wrap _render_init_summary cells in escape() consistent with _render_drift_table - M2: test_init_ontology_missing_db_exits_1 asserts "Graph database not found" in output - M4: reword cgis_init_ontology except comment to "translate errors to the ❌-message medium" - M5: append Amendment 1 to 2026-06-12-init-ontology-design.md (no nodes column in summary table) - Task 5: add tests/self_parsing/test_init_ontology_roundtrip.py (round-trip acceptance) propose_ontology → yaml → analyze_drift → any_critical=False, no empty rows, drift_score ≤ tolerance + ε, query+extractors prefixes discovered - Rebased on main (merged #173 cgis_find_symbol; kept both find tests and init-ontology tests) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sonar new-code duplication 4.6% -> shared make_chain_db in conftest; gemini: module-level _TEMPLATE_NAMES, removesuffix idiom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
94da683 to
9814a80
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the cgis init-ontology CLI command and cgis_init_ontology MCP tool to automatically propose a starter patterns.yaml configuration from a measured graph. It introduces the ontology_init.py module, which handles domain discovery and template fitting, alongside comprehensive unit and round-trip tests. The review feedback highlights three improvement opportunities: validating that the depth parameter is a positive integer in both the domain discovery logic and the CLI option, and updating the CLI summary table's type check to format integer tolerances consistently with floats.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…2dp (#211 gemini r3) - discover_domains: guard depth <= 0 with ValueError("depth must be a positive integer") and document the Raises section in the docstring - cli.py init-ontology: add min=1 to --depth typer.Option for upfront CLI validation - cli.py _render_init_summary: widen isinstance check to (int, float) so integer drift_tolerance values also render with .2f format Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zaebee
left a comment
There was a problem hiding this comment.
Round 2 — gate green, all prior items addressed ✅
Re-reviewed after the updates; Sonar quality gate is now OK (new-code duplication back under 3%). Re-ran the dogfood on the current head: ingest → init-ontology → drift still exit 0, "All domains within tolerance", and --depth 0 is now correctly rejected (x>=1).
Prior items:
- 🟡 nit #1 (
_fit_templatesre-parsing the header per domain) — ✅ fixed:_TEMPLATE_NAMESparsed once at module load behind anisinstance(..., dict)guard. Clean. - 🔴 duplication gate — ✅ resolved. For the record, the dominant CPD source was the per-file test fixtures (the
9814a80chain-db dedupe did it), not the_domain_entrybranches I led with — good call consolidating those. - 🟡 nit #2 (round-trip at
margin=0.0resting on a<=gate) — fine as-is, the stress test covers it; flagged only for future awareness.
New this round (verified, all correct):
discover_domainsdepth validation — typer--depth min=1plus a library-levelValueErrorfor depth≤0 that the MCP path funnels into the❌medium. Guarded at both layers. 👍rstrip(".")→removesuffix(".")— strictly more correct (single-suffix vs char-class strip), even though current inputs can't trigger the difference._render_init_summaryint-or-float tolerance guard — harmless defensiveness for hand-edited yaml.
One optional, non-blocking: the three hygiene branches in _domain_entry:270-287 are still near-identical (only reason differs). Now that the gate's green this is purely a clarity call — collapse via a single hygiene_reason path if you like, or keep the explicit ladder for readability. Your call.
LGTM — good to merge from my side. 🟢
…lleague r2, optional) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
Re: round 2 — the optional dedup taken (36c614b): the hygiene triplet collapses to a reason ladder + single emission block (−15/+10 lines, output byte-identical — the determinism test pins it). Thanks for the two-round pass and the empirical re-verification. 🤖 Generated with Claude Code |
…177) Sonar new-code duplication: the v2 fit patterns.yaml and the instar/triangle db builders were copied across test_drift_service/test_cli/test_mcp_server → shared fit_patterns_yaml/instar_db/triangle_db helpers in conftest (#211 dedup lesson). Also commits the ruff-format normalization the rebase conflict resolution left uncommitted (CI format-check fix).
…177) Sonar new-code duplication: the v2 fit patterns.yaml and the instar/triangle db builders were copied across test_drift_service/test_cli/test_mcp_server → shared fit_patterns_yaml/instar_db/triangle_db helpers in conftest (#211 dedup lesson). Also commits the ruff-format normalization the rebase conflict resolution left uncommitted (CI format-check fix).
* feat(ontology): add funnel template — transpose of layered_dag (#186, #177) The single most common intra-domain archetype across 9 repos that the hand-authored 5-template alphabet omitted (funnel = layered_dagᵀ = {021U:0.5, 021C:0.5}). Adding it rebinds nothing — existing drift scores and ratchets are unchanged — but lets fit-quality (#177) measure against a transpose-closed alphabet. Bundled header + _ALPHABET pin updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(drift): promote fit_templates to DriftScorer; iterate loaded alphabet (#177) Canonical distance-to-template ranking now on DriftScorer (iterates self._patterns → reflects whatever patterns.yaml declares, not a hardcoded list). init-ontology delegates — one source of truth for fit-quality (#177) and measure-then-label (#174). Direct tests prove the funnel shape ranks funnel nearest. * feat(drift): fit-quality (nearest template + residual + band) and coverage roll-up (#177) DriftReport.fit (FitQuality: nearest/runner-up template + residual + good|weak|none band) annotated in analyze_drift for profiled domains via the shared fit_templates ranking; band 'none' (residual > max_residual, default 0.45) is the 'no template fits' signal. DriftAnalysis.coverage lists graph prefixes no project_domain binds (drift's blind spot), reusing discover_domains. * feat(cli,mcp): fit column + no-template-fits/coverage roll-ups; --max-residual (#177) CLI drift gains a Fit column (nearest template + residual, banded), a 'no template fits' roll-up for band-none domains, and an unbound-code coverage section; --max-residual plumbs the cutoff. Notes now print below the table (uncramped by the new column). MCP cgis_drift gains max_residual + top-level coverage; per-report fit rides in asdict. * test: hoist fit fixtures to conftest; commit post-rebase formatting (#177) Sonar new-code duplication: the v2 fit patterns.yaml and the instar/triangle db builders were copied across test_drift_service/test_cli/test_mcp_server → shared fit_patterns_yaml/instar_db/triangle_db helpers in conftest (#211 dedup lesson). Also commits the ruff-format normalization the rebase conflict resolution left uncommitted (CI format-check fix). * refactor: Literal-typed fit band (drop type:ignore); hoist dom.fN literals to constants (#232 review) gemini: annotate band as Literal['good','weak','none'] so FitQuality construction is statically checked. Sonar: the dom.f1/f2/f3 node ids repeated across the conftest fit-db builders → _F1/_F2/_F3 constants. * fix(drift): band fit on gate-free shape residual; guard empty alphabet + low max-residual (#232 review) Colleague review: - #1 (semantic): fit band/residual now use DriftScorer.shape_residual (gate-FREE layered TV) so a well-shaped domain with a cycle reads gate_failed but is NOT banded 'no template fits' — the cycle is the gate's story. Ranking + init- ontology tolerance keep the full drift_score (round-trip #174 intact). - #2: _fit_quality returns None on an empty alphabet (profiles-but-no-patterns) — no more IndexError aborting drift. - #3: good-band cutoff clamped to min(0.25, max_residual) so --max-residual below 0.25 still bands near-perfect matches 'good'. - #4: coverage reuses a single get_all_nodes() (shared with the quotient build). * fix(drift): shape_residual returns None on no shape signal → fit=None (#232 review) A v1 profile (no layers), an empty census on both layers, or fully unresolved calls with no imports layer leaves total weight 0. Returning 0.0 banded 'good' — 'no signal to fit' masquerading as 'matches an archetype', the exact distinction #177 draws. shape_residual now returns None on total<=0 (covers all three sub-cases — broader than the per-cause guards suggested in review); _fit_quality maps it to fit=None. Regression: test_fit_none_when_no_shape_signal. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>



Summary
Closes #174 (roadmap #179 P0 adoption blocker — pairs with the merged #178 trust guard).
Ships
cgis init-ontology(CLI) +cgis_init_ontology(MCP, read-only): the measure-then-label proposer that turns drift from "hand-write YAML first" into one command — ingest → propose → drift passes green by construction.src/cgis/query/ontology_init.py: domain discovery (auto-descent +--depth), fingerprint per domain, DriftScorer as the fitter (zero new scoring math — proposed tolerancesmeasured + marginceil-2dp are comparable to later drift runs)min_nodes/ edge-less → hygiene-only with reasons; hygiene entries also get measured tolerances (cyclic domains round-trip)--force; MCP returns YAML text and writes nothing (sidesteps the S2083 path-write taint class)Review fix-rounds folded in
discover_domainsempty-level bug: a never-branching lineage descended into[]— now yields its deepest id (adversarial reviewer catch, hand-traced regression test)profile=Nonewhile the yaml emitted the detected profile → v1 weight divergence broke the round-trip on cyclic domains (opus reviewer reproduced 0.67 > 0.53); fix threads the emitted profile through, regression test uses a real IMPORTS cyclecgis.querytolerance 0.18 → 0.20:ontology_init.pyjoining the domain raised its measured drift to 0.19. Documented in the yaml comment; same rationale class as #199's extraction re-baseline (the domain legitimately grew, not a gaming of the gate). The drift tool flagged its own new feature — working as intended.Test Plan
margin=0.0stress), mypy strict, interrogate 99.5%cgis_find_symbolfrom main untouched🤖 Generated with Claude Code