From 1e74cc9e6fa88887a590608d5d91b0b364207c6e Mon Sep 17 00:00:00 2001 From: SkyeAv Date: Fri, 31 Jul 2026 13:55:41 -0700 Subject: [PATCH 1/2] refactor!: remove build-kg --table-config and require explicit validate --schema BREAKING CHANGE: slim the build-kg and validate CLI surface around explicit, graph-only inputs. - build-kg: remove the --table-config/-tc flag and its throwaway TEMP_KG wrapper (and the --fullmap/-fm flag that existed solely to feed it). build-kg now always takes a graph YAML; wrap a single table config in a graph config to build it (the agent already writes a ready graph.yaml under builds//). - build-kg: rename the configuration_file parameter to graph_configuration_file (CLI flags --configuration-file/-f and positional usage unchanged; only the positional metavar GRAPH-CONFIGURATION-FILE and the bound-argument name change). - validate: remove YAML auto-detection; require an explicit --schema {graph,table}/-s flag selecting which schema to validate against. Docs (cli.md, agent.md, configuration/graph.md) and the SSOT CLI-coverage tests updated to match; CHANGELOG Unreleased entry added. Gate: ruff + ruff-format + pyright clean; pytest 621 passed / 27 skipped, cli.py at 100% coverage. --- CHANGELOG.md | 3 ++ docs/agent.md | 6 ++- docs/cli.md | 33 ++++++++-------- docs/configuration/graph.md | 2 +- src/tablassert/cli.py | 73 ++++++++++-------------------------- tests/test_agent_storage.py | 10 ++--- tests/test_cli_validation.py | 43 ++++++--------------- tests/test_cover_cli.py | 23 +++++------- tests/test_e2e_smoke.py | 2 +- 9 files changed, 70 insertions(+), 125 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddfee285..63043e68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to this project are documented in this file. ### Breaking Changes - **Removed the `TABLASSERT_FULLMAP_SHARDS` environment variable.** The number of on-disk RECORDS shard files is now fixed at the compile-time cap (`16`) and can no longer be overridden at build time; the variable is silently ignored if set. Default builds are unaffected — the previous default was already `16`. The read path still honors the shard count recorded in an existing database's `meta` table (`shards`), so databases built with fewer shards under the old variable continue to open and resolve correctly. See `docs/fullmap.md`. +- **Removed the `build-kg --table-config`/`-tc` flag** (and the `build-kg --fullmap`/`-fm` flag, which existed solely to feed it). `build-kg` now always takes a graph YAML; the throwaway `TEMP_KG` wrapper for building a bare table (Section) config is gone. To build a single table config, wrap it in a graph config — `tablassert agent` already writes a ready `graph.yaml` alongside each build under `builds//`. (`validate --schema table` still checks a bare table config on its own.) +- **Renamed `build-kg`'s configuration-file parameter** from `configuration_file` to `graph_configuration_file` to reflect that it is always a graph config. The CLI flags are unchanged (`--configuration-file`/`-f` and positional); only the positional metavar (`GRAPH-CONFIGURATION-FILE`) and the Python / bound-argument name change. +- **`validate` no longer auto-detects the config kind.** The YAML-sniffing heuristic (a top-level `tables` key ⇒ graph, otherwise table) is removed; `validate` now requires an explicit `--schema {graph,table}`/`-s` flag selecting which schema to validate against. `tablassert validate foo.yaml` now fails without `--schema`; use `--schema graph` (validates the `Graph` model and every referenced table) or `--schema table` (validates section syntax only). ## 8.0.1 - 2026-07-30 diff --git a/docs/agent.md b/docs/agent.md index 40c84663..5d10f3eb 100644 --- a/docs/agent.md +++ b/docs/agent.md @@ -214,10 +214,12 @@ skipped. The `downloads/` payload persists on disk across runs. ### Reusing agent outputs with the full pipeline The best config's `source.local` points at the downloaded table under `downloads//`, so the full -(non-agent) pipeline can reuse the agent's output **without re-fetching**: +(non-agent) pipeline can reuse the agent's output **without re-fetching**. The agent already writes a +ready-to-build `graph.yaml` (wrapping `table.yaml` with the resolved fullmap) into `builds//`: ```bash -tablassert build-kg .tablassert/agent/configs/PMC11708054.yaml --table-config --fullmap ./fullmap +cd .tablassert/agent/builds/PMC11708054 +tablassert build-kg graph.yaml ``` !!! warning "Not relocatable" diff --git a/docs/cli.md b/docs/cli.md index 70f2b21a..bc92e27a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -104,24 +104,21 @@ Use this to build a KGX NDJSON knowledge graph (nodes, edges, and a Resource Ing YAML configuration file. ```bash -tablassert build-kg CONFIGURATION-FILE [ARGS] +tablassert build-kg GRAPH-CONFIGURATION-FILE [ARGS] ``` -By default the positional `CONFIGURATION-FILE` (also `--configuration-file`, `-f`) is a **graph** YAML. +The positional `GRAPH-CONFIGURATION-FILE` (also `--configuration-file`, `-f`) is a **graph** YAML. | Option | Type | Required | Default | Description | | --- | --- | --- | --- | --- | -| `CONFIGURATION-FILE` (`--configuration-file`, `-f`) | Path | Yes | — | Graph YAML (or a table YAML with `--table-config`) | +| `GRAPH-CONFIGURATION-FILE` (`--configuration-file`, `-f`) | Path | Yes | — | Graph YAML | | `--release`, `-r` | Flag | No | `False` | Emit a slim, significant-only graph (drops `biolink:not_significant` edges before resolution) | | `--qc`, `-q` | Flag | No | `False` | Audit resolved mappings (exact → fuzzy → BioBERT) so low-confidence edges are flagged; requires the `[qc]` extra | | `--log`, `-l` | Flag | No | `False` | Enable verbose per-section logging | | `--head`, `-hd` | Flag | No | `False` | Fast output-shape preview: ≤5 random rows/section, cached to `.head.parquet`, never clobbers a full build | -| `--table-config`, `-tc` | Flag | No | `False` | Build/test one table (Section) config without writing a graph config (wrapped in a throwaway `TEMP_KG` graph) | -| `--fullmap`, `-fm` | Path | No | `./fullmap` | Fullmap path for the throwaway `TEMP_KG` graph when `--table-config` is passed | ```bash tablassert build-kg graph.yaml --qc --log -tablassert build-kg table-config.yaml --table-config --fullmap ./fullmap ``` Output is written to the current directory as `{name}_{version}.nodes.ndjson`, @@ -140,26 +137,26 @@ Output is written to the current directory as `{name}_{version}.nodes.ndjson`, ## validate -Use this to validate a graph or table configuration without running the build — ideal for CI and -pre-commit hooks. Both forms work: `tablassert validate ` and `tablassert validate -f `. +Use this to validate a configuration against a schema without running the build — ideal for CI and +pre-commit hooks. The required `--schema` flag selects which schema to validate against (the kind is +no longer sniffed from the YAML). ```bash -tablassert validate CONFIGURATION-FILE -tablassert validate -f CONFIGURATION-FILE +tablassert validate CONFIGURATION-FILE --schema {graph,table} +tablassert validate -f CONFIGURATION-FILE --schema {graph,table} ``` | Option | Type | Required | Default | Description | | --- | --- | --- | --- | --- | -| `CONFIGURATION-FILE` (`--configuration-file`, `-f`) | Path | Yes | — | Graph **or** table configuration to validate | +| `CONFIGURATION-FILE` (`--configuration-file`, `-f`) | Path | Yes | — | Configuration file to validate | +| `--schema`, `-s` | `graph` \| `table` | Yes | — | Schema to validate against: `graph` validates the `Graph` model **and** every referenced table; `table` validates section syntax only | -The config kind is detected from the YAML: a mapping with a top-level `tables` key is a **graph** -config (validates the `Graph` model **and** every referenced table); anything else is a **table** -config (validates section syntax). Exits non-zero on any schema error. See -[Table Configuration](configuration/table.md) and [Graph Configuration](configuration/graph.md). +Exits non-zero on any schema error. See [Table Configuration](configuration/table.md) and +[Graph Configuration](configuration/graph.md). ```bash -tablassert validate table-config.yaml -tablassert validate graph.yaml +tablassert validate table-config.yaml --schema table +tablassert validate graph.yaml --schema graph ``` --- @@ -167,7 +164,7 @@ tablassert validate graph.yaml ## Typical workflow 1. Author a table config, then a graph config that references it. -2. `tablassert validate graph.yaml` — fail fast on schema errors. +2. `tablassert validate graph.yaml --schema graph` — fail fast on schema errors. 3. `tablassert build-kg graph.yaml` — produce KGX NDJSON + RIG (add `--qc` to audit mappings). ## Next Steps diff --git a/docs/configuration/graph.md b/docs/configuration/graph.md index 8a4b410f..fe0c5d59 100644 --- a/docs/configuration/graph.md +++ b/docs/configuration/graph.md @@ -1,6 +1,6 @@ # Graph Configuration Reference -Graph configurations orchestrate one or more [table configurations](table.md) into a single knowledge-graph build — author one to produce KGX output with `tablassert build-kg`. To build or test a single table config without authoring a graph file, use `build-kg --table-config`, which wraps it in a throwaway graph (see the [CLI reference](../cli.md#build-kg)). +Graph configurations orchestrate one or more [table configurations](table.md) into a single knowledge-graph build — author one to produce KGX output with `tablassert build-kg` (see the [CLI reference](../cli.md#build-kg)). To check a single table config on its own, use `tablassert validate --schema table`. ## Purpose diff --git a/src/tablassert/cli.py b/src/tablassert/cli.py index e0575143..090052f7 100644 --- a/src/tablassert/cli.py +++ b/src/tablassert/cli.py @@ -85,41 +85,23 @@ def _extract_sections_indexed(args: tuple[int, object, Path]) -> tuple[int, list return idx, to_sections(raw, table) # pyright: ignore -def _load_graph(configuration_file: Path, table_config: bool, fullmap: Path) -> Graph: +def _load_graph(configuration_file: Path) -> Graph: """Load and validate the Graph config that drives a build. - By default ``configuration_file`` is a Graph YAML loaded directly. With - ``table_config=True`` it is instead a table (Section) YAML wrapped in a - throwaway ``TEMP_KG`` graph so a single table config can be built or tested - without authoring a full graph config; ``contributions`` and ``ui_explanation`` - then fall back to the Graph model defaults. - Args: - configuration_file: Graph YAML path, or a table YAML path when ``table_config``. - table_config: When ``True``, wrap the table YAML in a throwaway ``TEMP_KG`` graph. - fullmap: Fullmap path for the wrapped graph when ``table_config`` is ``True``. + configuration_file: Graph YAML path. Returns: The validated Graph model. Raises: - GraphValidationError: If the (possibly wrapped) graph fails Pydantic validation. + GraphValidationError: If the graph fails Pydantic validation. """ from tablassert.ingests import from_yaml from tablassert.models import Graph from tablassert.progress import flatten_pydantic_error - raw: object - if table_config: - raw = { - "name": "TEMP_KG", - "version": "0.0.0", - "description": "Temporary knowledge graph generated to test a table configuration", - "tables": [configuration_file], - "fullmap": fullmap, - } - else: - raw = from_yaml(configuration_file) + raw: object = from_yaml(configuration_file) try: return Graph.model_validate(raw) except pydantic.ValidationError as e: @@ -127,14 +109,7 @@ def _load_graph(configuration_file: Path, table_config: bool, fullmap: Path) -> def build_pipeline( - configuration_file: Path, - progress: PipelineProgress, - release: bool = False, - qc: bool = False, - log: bool = False, - head: bool = False, - table_config: bool = False, - fullmap: Path = Path("./fullmap"), + configuration_file: Path, progress: PipelineProgress, release: bool = False, qc: bool = False, log: bool = False, head: bool = False ) -> None: """Build a knowledge graph from a YAML configuration file. @@ -142,16 +117,12 @@ def build_pipeline( Tcodes → collect instructions → build subgraphs → compile graph. Args: - configuration_file: Path to the graph YAML file (or a table YAML - file when ``table_config`` is ``True``). + configuration_file: Path to the graph YAML file. progress: Pipeline progress reporter. release: When ``True``, emit release-mode artifacts. qc: When ``True``, run quality-control audits on each section. log: When ``True``, enable per-section verbose logging. head: When ``True``, preview a random sample of up to 5 rows per section (fast schema/shape check). - table_config: When ``True``, treat ``configuration_file`` as a table - (Section) YAML and wrap it in a throwaway ``TEMP_KG`` graph. - fullmap: Fullmap path used to wrap a table config when ``table_config`` is ``True``. Raises: GraphValidationError: If the graph YAML fails Pydantic validation. @@ -164,7 +135,7 @@ def build_pipeline( # Stage 1/6: load tables. progress.stage("Loading Tables") - g: Graph = _load_graph(configuration_file, table_config, fullmap) + g: Graph = _load_graph(configuration_file) # imap_unordered yields in completion order, so each worker carries its input # index and we reassemble by index to keep raw[i] aligned with g.tables[i]. start, advance, _ = progress.section_loop(len(g.tables), "Load") @@ -492,36 +463,32 @@ def _download_detail(downloaded: int, total: int) -> str: @APP.command(name="build-kg") def build_kg( - configuration_file: Annotated[Path, cyclopts.Parameter(name=["--configuration-file", "-f"])], + graph_configuration_file: Annotated[Path, cyclopts.Parameter(name=["--configuration-file", "-f"])], release: Annotated[bool, cyclopts.Parameter(name=["--release", "-r"], negative="")] = False, qc: Annotated[bool, cyclopts.Parameter(name=["--qc", "-q"], negative="")] = False, log: Annotated[bool, cyclopts.Parameter(name=["--log", "-l"], negative="")] = False, head: Annotated[bool, cyclopts.Parameter(name=["--head", "-hd"], negative="")] = False, - table_config: Annotated[bool, cyclopts.Parameter(name=["--table-config", "-tc"], negative="")] = False, - fullmap: Annotated[Path, cyclopts.Parameter(name=["--fullmap", "-fm"])] = Path("./fullmap"), ) -> None: """Build a knowledge graph from a YAML configuration file. - By default the positional config is a Graph YAML. With ``--table-config`` it is a - table (Section) YAML wrapped in a throwaway ``TEMP_KG`` graph (``--fullmap`` sets - the fullmap path) so a single table config can be built or tested without - authoring a full graph config. + The positional config is a Graph YAML that orchestrates one or more table + configs into a single knowledge-graph build. """ - run(6, build_pipeline, configuration_file, release=release, qc=qc, log=log, head=head, table_config=table_config, fullmap=fullmap) + run(6, build_pipeline, graph_configuration_file, release=release, qc=qc, log=log, head=head) @APP.command(name="validate") -def validate(configuration_file: Annotated[Path, cyclopts.Parameter(name=["--configuration-file", "-f"])]) -> None: - """Validate a graph or table YAML configuration file. +def validate( + configuration_file: Annotated[Path, cyclopts.Parameter(name=["--configuration-file", "-f"])], + schema: Annotated[Literal["graph", "table"], cyclopts.Parameter(name=["--schema", "-s"])], +) -> None: + """Validate a YAML configuration file against the graph or table config schema. - Detects the config kind from the YAML: a mapping with a top-level ``tables`` key - is a graph config (validates the Graph model AND every referenced table); anything - else is treated as a table config (validates section syntax only). + ``--schema graph`` validates the Graph model AND every referenced table; ``--schema + table`` validates section syntax only. The schema is selected explicitly rather than + sniffed from the YAML, so a config is always checked against the schema you expected. """ - from tablassert.ingests import from_yaml - - loaded: object = from_yaml(configuration_file) - if isinstance(loaded, dict) and "tables" in loaded: + if schema == "graph": run(2, validate_graph_pipeline, configuration_file) else: run(3, validate_pipeline, configuration_file) diff --git a/tests/test_agent_storage.py b/tests/test_agent_storage.py index 609966c2..468efed4 100644 --- a/tests/test_agent_storage.py +++ b/tests/test_agent_storage.py @@ -358,11 +358,11 @@ def fake_fetch(pmc_id: str, outdir: Path, *, timeout: int = 120) -> list[Path]: def test_supervisor_best_config_pipeline_reuse(tmp_path: Path, fullmap_db: Path, monkeypatch: pytest.MonkeyPatch) -> None: """The BEST config references the STABLE download and rebuilds from a FRESH cwd (REQ-LAYOUT-5/8). - Why: the pipeline-reuse contract. ``tablassert build-kg --table-config --fullmap `` must - be able to reuse the supervisor's accepted config WITHOUT re-fetching: its ``source.local`` must be - the REAL, persisted download under ``state_dir/downloads//`` (not a temp path), and because that - path is ABSOLUTE the config must build from ANY cwd. This proves the download is real + referenced - and that the best config is self-sufficient for downstream reuse. + Why: the pipeline-reuse contract. ``tablassert build-kg`` (via the agent-written + ``builds//graph.yaml`` wrapper) must be able to reuse the supervisor's accepted config WITHOUT + re-fetching: its ``source.local`` must be the REAL, persisted download under ``state_dir/downloads//`` + (not a temp path), and because that path is ABSOLUTE the config must build from ANY cwd. This proves the + download is real + referenced and that the best config is self-sufficient for downstream reuse. """ pytest.importorskip("smolagents") import yaml diff --git a/tests/test_cli_validation.py b/tests/test_cli_validation.py index b82b32ae..f17a6516 100644 --- a/tests/test_cli_validation.py +++ b/tests/test_cli_validation.py @@ -5,10 +5,9 @@ import pytest -from tablassert.cli import _load_graph, build_pipeline, validate, validate_pipeline +from tablassert.cli import build_pipeline, validate, validate_pipeline from tablassert.errors import GraphValidationError, SectionValidationError from tablassert.ingests import to_yaml -from tablassert.models import DEFAULT_RIG_CONTRIBUTIONS, DEFAULT_RIG_UI_EXPLANATION, Graph from tablassert.progress import PipelineProgress @@ -64,46 +63,28 @@ def _valid_table_config() -> dict[str, Any]: } -def test_load_graph_wraps_table_config_as_temp_kg(fixtures_path: Path) -> None: - """Guard: `build-kg --table-config` wraps a table YAML in a throwaway TEMP_KG graph. +def test_validate_command_selects_schema_explicitly(tmp_path: Path) -> None: + """Guard: `validate` checks a file against the schema the caller selects via `--schema`. - A single table config must build/test without authoring a full graph config. The - wrapper fixes name/version/description, points `tables` at the config path, uses the - given fullmap, and leaves contributions/ui_explanation on the Graph model defaults. - """ - table: Path = fixtures_path / "minimal_section.yaml" - g: Graph = _load_graph(table, table_config=True, fullmap=Path("./fullmap")) - assert g.name == "TEMP_KG" - assert g.version == "0.0.0" - assert g.description == "Temporary knowledge graph generated to test a table configuration" - assert g.tables == [table] - assert g.fullmap == Path("./fullmap") - assert g.contributions == DEFAULT_RIG_CONTRIBUTIONS - assert g.ui_explanation == DEFAULT_RIG_UI_EXPLANATION - - -def test_validate_command_dispatches_graph_and_table(tmp_path: Path) -> None: - """Guard: generic `validate` accepts both a table config and a graph config. - - `validate` detects the config kind from a top-level `tables` key: a table config - validates section syntax only; a graph config validates the Graph model AND each - referenced table. Both return None when valid. + `--schema table` validates section syntax only; `--schema graph` validates the Graph model + AND each referenced table. The kind is chosen explicitly (never sniffed from the YAML), so a + config is always checked against the schema the caller expected. Both return None when valid. """ table: Path = tmp_path / "table.yaml" to_yaml(table, _valid_table_config()) - # Table branch: no top-level `tables` key. - assert validate(table) is None - # Graph branch: top-level `tables` key -> validates the graph and its tables. + # Table schema: section syntax only. + assert validate(table, schema="table") is None + # Graph schema: validates the graph and its referenced tables. graph_file: Path = tmp_path / "graph.yaml" to_yaml(graph_file, {"name": "TEST", "version": "1.0.0", "description": "test graph", "tables": [str(table)], "fullmap": ".fullmap"}) - assert validate(graph_file) is None + assert validate(graph_file, schema="graph") is None def test_validate_command_graph_branch_rejects_invalid_table(tmp_path: Path, fixtures_path: Path) -> None: - """Guard: generic `validate` on a graph fails fast when a referenced table is invalid.""" + """Guard: `validate --schema graph` fails fast when a referenced table is invalid.""" bad_table: Path = fixtures_path / "invalid_section_missing_source.yaml" graph_file: Path = tmp_path / "graph.yaml" to_yaml(graph_file, {"name": "TEST", "version": "1.0.0", "description": "test graph", "tables": [str(bad_table)], "fullmap": ".fullmap"}) with pytest.raises(SectionValidationError) as exc_info: - validate(graph_file) + validate(graph_file, schema="graph") assert exc_info.value.code == "section-validation-failed" diff --git a/tests/test_cover_cli.py b/tests/test_cover_cli.py index b1e48b79..08758e42 100644 --- a/tests/test_cover_cli.py +++ b/tests/test_cover_cli.py @@ -197,22 +197,20 @@ def _fake_run(stages: int, fn: Any, arg: Path, **kwargs: Any) -> None: calls.append((stages, fn, arg, kwargs)) monkeypatch.setattr(cli, "run", _fake_run) - build_kg(config, release=True, qc=True, log=True, head=True, table_config=True, fullmap=Path("./fm")) - assert calls == [ - (6, cli.build_pipeline, config, {"release": True, "qc": True, "log": True, "head": True, "table_config": True, "fullmap": Path("./fm")}) - ] + build_kg(config, release=True, qc=True, log=True, head=True) + assert calls == [(6, cli.build_pipeline, config, {"release": True, "qc": True, "log": True, "head": True})] def test_build_kg_configuration_file_flag_parses(tmp_path: Path) -> None: """Guard: ``build-kg``'s config binds positionally AND via ``-f``/``--configuration-file``. ``build-kg`` previously exposed its configuration file ONLY positionally, while ``validate`` - accepted ``--configuration-file``/``-f``. This pins the now-consistent parsing and that the - ``--fullmap`` short alias is ``-fm`` (``-f`` belongs to the configuration file). cyclopts' - ``parse_args`` binds tokens WITHOUT executing the command, so no build runs. + accepted ``--configuration-file``/``-f``. This pins the now-consistent parsing (``-f`` belongs + to the configuration file). The bound argument is keyed by the ``graph_configuration_file`` + parameter name. cyclopts' ``parse_args`` binds tokens WITHOUT executing the command, so no + build runs. """ config: Path = tmp_path / "graph.yaml" - fm: Path = tmp_path / "fm" def parse(argv: list[str]) -> dict[str, Any]: fn, bound, _ = cli.APP.parse_args(argv, exit_on_error=False) @@ -220,13 +218,10 @@ def parse(argv: list[str]) -> dict[str, Any]: return dict(bound.arguments) # Positional usage is unchanged. - assert parse(["build-kg", str(config)])["configuration_file"] == config + assert parse(["build-kg", str(config)])["graph_configuration_file"] == config # The configuration file now also binds via -f and --configuration-file (matches validate). - assert parse(["build-kg", "-f", str(config)])["configuration_file"] == config - assert parse(["build-kg", "--configuration-file", str(config)])["configuration_file"] == config - # --fullmap keeps its long form and gains the -fm short alias (-f is the config's now). - assert parse(["build-kg", str(config), "--fullmap", str(fm)])["fullmap"] == fm - assert parse(["build-kg", str(config), "-fm", str(fm)])["fullmap"] == fm + assert parse(["build-kg", "-f", str(config)])["graph_configuration_file"] == config + assert parse(["build-kg", "--configuration-file", str(config)])["graph_configuration_file"] == config def test_build_fullmap_pipeline_reports_download_progress(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/tests/test_e2e_smoke.py b/tests/test_e2e_smoke.py index e6317940..299f4e41 100644 --- a/tests/test_e2e_smoke.py +++ b/tests/test_e2e_smoke.py @@ -132,4 +132,4 @@ def test_validate_command_happy_path(tmp_path: Path) -> None: # The three-stage validate pipeline alone does not raise on a valid section. assert validate_pipeline(config, PipelineProgress(total_stages=3)) is None # The cyclopts command wrapper (cli.py validate -> run(3, validate_pipeline, ...)). - assert validate(config) is None + assert validate(config, schema="table") is None From e2a4265ca113ef54a891c888764e748d84173be3 Mon Sep 17 00:00:00 2001 From: SkyeAv Date: Fri, 31 Jul 2026 14:09:15 -0700 Subject: [PATCH 2/2] test: address CodeRabbit feedback (parser locks + shell-safe validate docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs/cli.md: replace the shell-unsafe `--schema {graph,table}` brace expansion in the validate usage block with concrete `--schema graph` / `--schema table` commands (brace expansion passed a stray positional argument). - tests/test_cover_cli.py: assert parse_args rejects the removed --table-config/-tc and --fullmap options for build-kg (locks the removal). - tests/test_cli_validation.py: add parse_args coverage proving validate binds --schema/-s and fails when it is omitted (required-option contract). - tests/test_agent_storage.py: correct the reuse-contract docstring — the test passes the bare best config straight to build_and_audit() (Python API), not `tablassert build-kg` / builds//graph.yaml. Gate: ruff + ruff-format + pyright clean; pytest 622 passed / 27 skipped, cli.py at 100% coverage. --- docs/cli.md | 4 ++-- tests/test_agent_storage.py | 11 ++++++----- tests/test_cli_validation.py | 22 ++++++++++++++++++++++ tests/test_cover_cli.py | 10 ++++++++-- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index bc92e27a..104e7abc 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -142,8 +142,8 @@ pre-commit hooks. The required `--schema` flag selects which schema to validate no longer sniffed from the YAML). ```bash -tablassert validate CONFIGURATION-FILE --schema {graph,table} -tablassert validate -f CONFIGURATION-FILE --schema {graph,table} +tablassert validate CONFIGURATION-FILE --schema graph +tablassert validate -f CONFIGURATION-FILE --schema table ``` | Option | Type | Required | Default | Description | diff --git a/tests/test_agent_storage.py b/tests/test_agent_storage.py index 468efed4..1eb2c612 100644 --- a/tests/test_agent_storage.py +++ b/tests/test_agent_storage.py @@ -358,11 +358,12 @@ def fake_fetch(pmc_id: str, outdir: Path, *, timeout: int = 120) -> list[Path]: def test_supervisor_best_config_pipeline_reuse(tmp_path: Path, fullmap_db: Path, monkeypatch: pytest.MonkeyPatch) -> None: """The BEST config references the STABLE download and rebuilds from a FRESH cwd (REQ-LAYOUT-5/8). - Why: the pipeline-reuse contract. ``tablassert build-kg`` (via the agent-written - ``builds//graph.yaml`` wrapper) must be able to reuse the supervisor's accepted config WITHOUT - re-fetching: its ``source.local`` must be the REAL, persisted download under ``state_dir/downloads//`` - (not a temp path), and because that path is ABSOLUTE the config must build from ANY cwd. This proves the - download is real + referenced and that the best config is self-sufficient for downstream reuse. + Why: the pipeline-reuse contract. The supervisor's accepted (best) config must be reusable WITHOUT + re-fetching by passing it straight back through the Python API (``build_and_audit`` — the same call + the supervisor uses): its ``source.local`` must be the REAL, persisted download under + ``state_dir/downloads//`` (not a temp path), and because that path is ABSOLUTE the config must + build from ANY cwd. This proves the download is real + referenced and that the best config is + self-sufficient for downstream reuse. """ pytest.importorskip("smolagents") import yaml diff --git a/tests/test_cli_validation.py b/tests/test_cli_validation.py index f17a6516..801de550 100644 --- a/tests/test_cli_validation.py +++ b/tests/test_cli_validation.py @@ -4,7 +4,9 @@ from typing import Any import pytest +from cyclopts.exceptions import MissingArgumentError # pyright: ignore[reportMissingImports] +from tablassert import cli from tablassert.cli import build_pipeline, validate, validate_pipeline from tablassert.errors import GraphValidationError, SectionValidationError from tablassert.ingests import to_yaml @@ -80,6 +82,26 @@ def test_validate_command_selects_schema_explicitly(tmp_path: Path) -> None: assert validate(graph_file, schema="graph") is None +def test_validate_schema_flag_parses_and_is_required(tmp_path: Path) -> None: + """Guard: ``validate`` binds ``--schema``/``-s`` through the parser and REQUIRES it. + + The direct ``validate()`` calls above bypass Cyclopts; this pins the live CLI contract — the + schema binds via ``--schema`` and ``-s``, and omitting it fails (it is a required option, no + longer sniffed from the YAML). ``parse_args`` binds WITHOUT executing, so no validation runs. + """ + config: Path = tmp_path / "config.yaml" + + def parse(argv: list[str]) -> dict[str, Any]: + fn, bound, _ = cli.APP.parse_args(argv, exit_on_error=False) + assert fn is validate + return dict(bound.arguments) + + assert parse(["validate", str(config), "--schema", "table"])["schema"] == "table" + assert parse(["validate", "-f", str(config), "-s", "graph"])["schema"] == "graph" + with pytest.raises(MissingArgumentError): + parse(["validate", str(config)]) + + def test_validate_command_graph_branch_rejects_invalid_table(tmp_path: Path, fixtures_path: Path) -> None: """Guard: `validate --schema graph` fails fast when a referenced table is invalid.""" bad_table: Path = fixtures_path / "invalid_section_missing_source.yaml" diff --git a/tests/test_cover_cli.py b/tests/test_cover_cli.py index 08758e42..6d9ecc9b 100644 --- a/tests/test_cover_cli.py +++ b/tests/test_cover_cli.py @@ -16,6 +16,7 @@ from urllib.error import HTTPError, URLError import pytest +from cyclopts.exceptions import UnknownOptionError # pyright: ignore[reportMissingImports] from tablassert import cli, rs from tablassert.cli import build_fullmap_pipeline, build_kg, download_babel_file, validate_graph_pipeline @@ -207,8 +208,9 @@ def test_build_kg_configuration_file_flag_parses(tmp_path: Path) -> None: ``build-kg`` previously exposed its configuration file ONLY positionally, while ``validate`` accepted ``--configuration-file``/``-f``. This pins the now-consistent parsing (``-f`` belongs to the configuration file). The bound argument is keyed by the ``graph_configuration_file`` - parameter name. cyclopts' ``parse_args`` binds tokens WITHOUT executing the command, so no - build runs. + parameter name. It also locks the removal of the deprecated ``--table-config``/``-tc`` and + ``--fullmap`` options (parsing them now raises). cyclopts' ``parse_args`` binds tokens WITHOUT + executing the command, so no build runs. """ config: Path = tmp_path / "graph.yaml" @@ -222,6 +224,10 @@ def parse(argv: list[str]) -> dict[str, Any]: # The configuration file now also binds via -f and --configuration-file (matches validate). assert parse(["build-kg", "-f", str(config)])["graph_configuration_file"] == config assert parse(["build-kg", "--configuration-file", str(config)])["graph_configuration_file"] == config + # The removed --table-config/-tc and --fullmap options are now rejected (locks the removal). + for removed in (["--table-config"], ["-tc"], ["--fullmap", str(config)]): + with pytest.raises(UnknownOptionError): + parse(["build-kg", str(config), *removed]) def test_build_fullmap_pipeline_reports_download_progress(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: