From 3b35f8ff07a0da97579fb5f45c18aed1bcca6d2a Mon Sep 17 00:00:00 2001 From: wjduenow Date: Fri, 22 May 2026 08:52:22 -0700 Subject: [PATCH] fix: address CodeRabbit findings on ingest + prune-existing (#104/#105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3 (ingest reader): enforce the DEC-005 size cap from stat().st_size BEFORE reading file bytes for a Path input, so an oversize schema.yml is rejected without first being slurped into memory. The post-resolve length check stays as the str-path enforcement + backstop. #2 (prune-existing --dry-run docs): correct cli-ops.md — --dry-run suppresses the .signalforge/diff.json sidecar, but the fail-closed .signalforge/prune.jsonl audit is STILL written (every prune run leaves a durable receipt — the cross-stage invariant; mirrors generate). Pinned by strengthening test_dry_run_writes_no_sidecar to assert prune.jsonl is present. (#1, env-var restore, is left as-is — DEC-023 'mutate, don't restore' is the deliberate, consistent pattern across all subcommands.) reader.py back to 100% coverage; full suite 1945 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/cli-ops.md | 9 +++++--- src/signalforge/ingest/reader.py | 29 +++++++++++++++++++++---- tests/cli/test_prune_existing.py | 6 +++++- tests/ingest/test_reader.py | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/docs/cli-ops.md b/docs/cli-ops.md index 34722074..1da099a5 100644 --- a/docs/cli-ops.md +++ b/docs/cli-ops.md @@ -424,7 +424,7 @@ Flag reference: | `--scope {sample,full}` | no | from config | Override `prune.scope`. Applied via `PruneConfig.model_validate` so validators re-run (DEC-002). | | `--sample-strategy {oneshot,materialised}` | no | from config | Override `prune.sample_strategy`. Applied via `PruneConfig.model_validate` (DEC-002). | | `--format {ansi,markdown,json}` | no | `ansi` | Select the diff renderer. ANSI: coloured terminal output. Markdown: GitHub-friendly report. JSON: stdout receives the JSON sidecar's contents. | -| `--dry-run` | no | off | Run ingest → prune → diff and print the diff to stdout, but write nothing — suppresses the default-on `.signalforge/diff.json` sidecar. There is **no `--write`** (read-only w.r.t. your `schema.yml`). | +| `--dry-run` | no | off | Run ingest → prune → diff and print the diff to stdout, suppressing the default-on `.signalforge/diff.json` sidecar. The fail-closed `.signalforge/prune.jsonl` audit is **still written** (every prune run leaves a durable receipt — the cross-stage fail-closed invariant; mirrors `generate`). There is **no `--write`** (read-only w.r.t. your `schema.yml`). | | `--quiet` | no | off | Suppress per-stage stderr progress lines and the skipped-test report, and raise the log level to `WARNING`. Mutually exclusive with `--verbose`. | | `--verbose` | no | off | Raise the log level to `DEBUG`, list each skipped test in detail, and surface panic-path tracebacks. Mutually exclusive with `--quiet`. | | `--no-color` | no | off | Strip ANSI colour codes from stdout. Sets `NO_COLOR=1` in the current process environment. | @@ -441,8 +441,11 @@ warehouse knobs that take its place. `--schema` file is hand-authored, so silently overwriting it would be surprising and destructive. The command prints the rendered diff to stdout and writes the `.signalforge/diff.json` sidecar by default; -`--dry-run` suppresses the sidecar for a pure-stdout, zero-disk run. -Re-pruning into the file is a possible v0.3 follow-up with a +`--dry-run` suppresses that sidecar for a pure-stdout diff. (The +fail-closed `.signalforge/prune.jsonl` audit is still written even under +`--dry-run` — every prune run leaves a durable receipt by design, the +same as `generate`; `--dry-run` governs only the end-of-run diff +sidecar.) Re-pruning into the file is a possible v0.3 follow-up with a confirmation/backup story. #### Unified diff against your file (DEC-004) diff --git a/src/signalforge/ingest/reader.py b/src/signalforge/ingest/reader.py index ea034687..1a48279d 100644 --- a/src/signalforge/ingest/reader.py +++ b/src/signalforge/ingest/reader.py @@ -110,9 +110,14 @@ def read_schema( IngestAnchorContractError: one or more tests reference a column absent from ``model.columns`` (whole-file, collect-all). """ - content_bytes = _resolve_input_bytes(schema, project_dir) + content_bytes = _resolve_input_bytes( + schema, project_dir, size_limit=_INGEST_SCHEMA_SIZE_LIMIT_BYTES + ) - # DEC-005: size cap BEFORE any parse. + # DEC-005: size cap BEFORE any parse. For a ``Path`` input the cap is + # ALSO enforced from ``stat().st_size`` before the file is read into + # memory (see ``_resolve_input_bytes``); this post-resolve check covers + # the ``str`` (already-in-memory) input and is a backstop for both. size = len(content_bytes) if size > _INGEST_SCHEMA_SIZE_LIMIT_BYTES: raise IngestSchemaTooLargeError(size, _INGEST_SCHEMA_SIZE_LIMIT_BYTES) @@ -132,8 +137,13 @@ def read_schema( return IngestResult(candidate=candidate, skipped=tuple(skipped)) -def _resolve_input_bytes(schema: str | Path, project_dir: Path | None) -> bytes: - """Resolve the ``schema`` argument to raw bytes per the str-vs-Path contract.""" +def _resolve_input_bytes(schema: str | Path, project_dir: Path | None, *, size_limit: int) -> bytes: + """Resolve the ``schema`` argument to raw bytes per the str-vs-Path contract. + + For a ``Path`` input the ``size_limit`` cap is enforced from + ``stat().st_size`` BEFORE the file is read into memory (DEC-005) — a + multi-gigabyte ``schema.yml`` is rejected without first being slurped. + """ if isinstance(schema, Path): base = project_dir if project_dir is not None else schema.parent try: @@ -145,6 +155,17 @@ def _resolve_input_bytes(schema: str | Path, project_dir: Path | None) -> bytes: ) from exc if not resolved.is_file(): raise IngestSchemaNotFoundError(schema) + # DEC-005: cap from the file's metadata BEFORE reading bytes, so an + # oversize file never lands in memory. + try: + stat_size = resolved.stat().st_size + except OSError as exc: + raise IngestSchemaParseError( + f"schema.yml metadata could not be read: {exc}", + cause=exc, + ) from exc + if stat_size > size_limit: + raise IngestSchemaTooLargeError(stat_size, size_limit) try: return resolved.read_bytes() except OSError as exc: diff --git a/tests/cli/test_prune_existing.py b/tests/cli/test_prune_existing.py index e4b0cfff..b76da421 100644 --- a/tests/cli/test_prune_existing.py +++ b/tests/cli/test_prune_existing.py @@ -376,12 +376,16 @@ def test_skipped_suppressed_by_quiet(tmp_path: Path, capsys: pytest.CaptureFixtu def test_dry_run_writes_no_sidecar(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: - """``--dry-run`` runs the pipeline but leaves no ``.signalforge/diff.json``.""" + """``--dry-run`` suppresses the diff.json sidecar but the fail-closed + prune.jsonl audit is still written (every prune run leaves a receipt — + the cross-stage invariant; mirrors ``generate``).""" project_dir, schema_path = _setup_project(tmp_path) argv = [*_base_argv(project_dir, schema_path), "--dry-run"] code = _run(argv) assert code == 0 assert not (project_dir / ".signalforge" / "diff.json").exists() + # The prune audit is fail-closed and written even under --dry-run. + assert (project_dir / ".signalforge" / "prune.jsonl").is_file() assert "Traceback" not in capsys.readouterr().err diff --git a/tests/ingest/test_reader.py b/tests/ingest/test_reader.py index 5f2e8870..c32c12d8 100644 --- a/tests/ingest/test_reader.py +++ b/tests/ingest/test_reader.py @@ -192,6 +192,43 @@ def test_read_schema_oversize_content() -> None: assert excinfo.value.size > _INGEST_SCHEMA_SIZE_LIMIT_BYTES +def test_read_schema_path_oversize_rejected_from_stat_before_read( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # For a Path input the cap is enforced from stat().st_size BEFORE the file + # is read into memory (DEC-005). Shrink the cap so a tiny file trips it; + # the reported size comes from stat, not from a slurped read. + monkeypatch.setattr("signalforge.ingest.reader._INGEST_SCHEMA_SIZE_LIMIT_BYTES", 10) + schema = tmp_path / "schema.yml" + schema.write_text("models: []\n") # > 10 bytes + with pytest.raises(IngestSchemaTooLargeError) as excinfo: + read_schema(schema, _make_orders_model(), project_dir=tmp_path) + assert excinfo.value.limit == 10 + assert excinfo.value.size == schema.stat().st_size + + +def test_read_schema_stat_failure_raises_parse_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The pre-read stat() can fail (TOCTOU race after is_file). Stub the + # canonicalised result so is_file() passes but stat() raises OSError, and + # assert it surfaces as IngestSchemaParseError, not an unhandled crash. + class _FakeResolved: + def is_file(self) -> bool: + return True + + def stat(self): # noqa: ANN202 — mimics Path.stat, only raises here + raise OSError("stat boom") + + monkeypatch.setattr( + "signalforge.ingest.reader.canonicalise_path", + lambda schema, base: _FakeResolved(), + ) + with pytest.raises(IngestSchemaParseError) as excinfo: + read_schema(Path("schema.yml"), _make_orders_model(), project_dir=tmp_path) + assert "metadata could not be read" in str(excinfo.value) + + def test_read_schema_anchor_contract_violation() -> None: # A test referencing a column the model does not have fails loud. raw = (