8: Diff renderer - #25
Merged
Merged
Conversation
Super plan for #8 — kept/dropped table + unified schema.yml diff. 14 stories (12 implementation + Quality Gate + Patterns & Memory), 21 decisions, architecture review across security / performance / data-model+API / observability / testing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_safety (US-004) Two leaf modules for the diff renderer (#8): - `signalforge.diff._ansi_safety.strip_ansi_escapes` — regex-based stripper for ANSI CSI escape sequences (DEC-007). Defends the Markdown sink against terminal-control injection from upstream manifest fields, LLM-drafted artifact text, and prune/grade reasons. - `signalforge.diff._markdown_safety.escape_markdown_scalar` — escapes backtick/pipe/backslash; HTML-entity-encodes pipe and row-breaking control chars in table-cell mode (DEC-008). Backslash is processed first so subsequent escapes can't be unwound by a crafted trailing-backslash. Tests cover bare CSI, color codes, reset, compound SGR, cursor movement, no-escape passthrough, idempotence, empty string for ANSI; backtick/pipe/backslash escaping, table-cell entity encoding, control-char encoding, idempotence, empty string for Markdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…safety, _markdown_safety)
Seven-class typed exception hierarchy under signalforge.diff.errors, mirroring the safety / draft / prune / grade precedent: every error carries a class-level default_remediation that __str__ renders on a separate ↳ Remediation: line, and every user-supplied string flowing into a message routes through repr() (DEC-022 of #6) so adversarial input cannot inject ANSI escapes or control characters into log viewers. Classes: DiffError (base) + DiffCandidateModelMismatchError, DiffPruneResultModelMismatchError, DiffGradingReportModelMismatchError (DEC-002 boundary checks at orchestrator entry); DiffInputTooLargeError (DEC-006 existing-schema YAML byte cap); DiffSidecarRecordTooLargeError (DEC-009 sidecar size cap); DiffSidecarWriteError (fail-closed sidecar-write seam wrapping underlying I/O cause via __cause__). 24 tests covering default_remediation rendering, repr() escaping for ANSI-bearing inputs, subclass-of relationships, and field exposure. Subpackage __init__.py ships only the error hierarchy in US-001; later stories (US-002 result models, US-003 config, US-010 orchestrator) extend the public surface. Traces to plans/super/8-diff-renderer.md US-001 (DEC-002, DEC-006, DEC-009). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add signalforge.diff._emitter.emit_proposed_yaml(candidate, prune_result)
that filters tests to PruneDecision.decision == "kept", preserves column
declaration order, and sorts tests within each column by (type, args_hash)
for deterministic output. Mirrors the grade layer's args_hash convention
(blake2b-4 of canonical-sorted JSON of test args, DEC-009) so the
emitter, grader, and diff renderer agree on test identity.
yaml.safe_dump invoked with sort_keys=False, default_flow_style=False,
width=4096, allow_unicode=True. AR-9 round-trip test verifies edge-case
descriptions ('---', '!tag', triple-backticks, embedded newlines, leading
quote/pipe/gt, unicode) survive emit -> yaml.safe_load byte-identical.
Leaf module — depends only on existing signalforge.draft and
signalforge.prune model types. No __init__.py created (US-001 owns it).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rror + 6 subclasses)
Add src/signalforge/diff/_artifact_id.py with byte-equal mirror of
signalforge.grade.engine._artifact_id_for. Six dotted-path shapes
(column.<col>.{description,rationale}, model.{description,rationale},
test.column.<col>.<type>[.<args_hash>], test.model.<type>[.<args_hash>])
plus _model_test_args_hash and compute_args_hashes helpers.
Cross-stage parity is load-bearing: the diff renderer joins grade-
sidecar JSON to its rendered diff via (run_id, artifact_id,
criterion_id); a single shape disagreement would silently drop grade
rows. tests/diff/test_artifact_id.py is the documented single allowed
cross-stage import seam — production diff code must not import from
signalforge.grade at runtime.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(mirror grade DEC-009)
Adds DiffEntry and DiffReport read-back-stable Pydantic v2 models to src/signalforge/diff/models.py per US-002 of issue #8. Both models are frozen=True, extra="ignore" with custom __repr__ per DEC-020 that omits prose/diff text fields. DiffEntry.tier is a Literal["kept","dropped", "flagged"] per DEC-012; DiffReport carries reproducibility-hash fields (candidate_hash, prune_result_hash, grading_report_hash) per DEC-016. Drift detector at tests/diff/test_models.py mirrors the prune / grade precedent: StrictDiffEntry / StrictDiffReport extra="forbid" mirrors validated against tests/fixtures/diff/diff_report_v1.json (which exercises all three Tier values), plus field-set parity and extra="forbid" sanity-floor checks. Public-API re-exports deferred to US-012 per the bead instructions — diff/__init__.py is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces the `diff:` top-level namespace in `signalforge.yml` per DEC-010 of plans/super/8-diff-renderer.md. `DiffConfig(extra="forbid", frozen=True)` carries the nine locked knobs (context_lines, max_why_chars, narrow_terminal_threshold, markdown_max_diff_chars, existing_schema_size_limit_bytes, existing_schema_warn_at_bytes, sidecar_size_limit_bytes, render_kind, respect_no_color_env); `_DiffConfigFile(extra="ignore")` outer wrapper silently tolerates sibling stage namespaces (`safety:`, `llm:`, `prune:`, `grade:`). `load_diff_config(project_dir, path=None) -> DiffConfig` matches the prior-stage signatures (load_grade_config / load_prune_config / load_draft_config / load_safety_config) verbatim. Numeric knobs route through a single positive-integer validator — zero/negative caps would silently disable the DEC-006/DEC-009 protections or render an empty table. The literal `render_kind` discriminator catches unknown render targets at config-load time. Per US-003 task scope, the loader raises the base `DiffError` (with remediation) for config-load failures rather than introducing a new `DiffConfigError` subclass; the seven-class hierarchy from US-001 stays intact, and a future `DiffConfigError` is a clean v0.2 refinement that won't break imports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_config (diff: namespace)
Adds the Renderer ABC + AnsiRenderer concrete in src/signalforge/diff/_renderers.py per US-008 of plans/super/8-diff-renderer.md. * DEC-007 — strip_ansi_escapes runs UNCONDITIONALLY on every user-content field (artifact_id, why, drop_reason, test_type, unified_diff body, model_unique_id) BEFORE the renderer's own colour codes are emitted. Defence-in-depth: a malicious "\x1b[31mEVIL\x1b[0m" in upstream content renders as the literal text "EVIL" in BOTH coloured and non-coloured output. * DEC-013 — narrow-TTY compact mode triggers when the effective terminal width is below DiffConfig.narrow_terminal_threshold (60 by default). Compact mode drops the WHY column from the table and emits each entry's why as an indented "└─ ..." follow-up line. * DEC-021 — six-step colour-precedence chain documented as early-returns in _should_emit_color: respect_no_color_env=False > force_color=False > force_color=True > FORCE_COLOR env > NO_COLOR env > sys.stdout.isatty(). * Module structured so US-009 can append MarkdownRenderer without restructuring; only the ABC + AnsiRenderer ship in this ticket. * No _LOGGER calls; renderer does no I/O (returns string). Tests at tests/diff/test_renderers.py cover the ABC contract, wide-TTY 6-column table, narrow-TTY compact mode, all six DEC-021 precedence positions individually, and unconditional ANSI stripping in both coloured and non-coloured output across the artifact_id, why, follow-up why, model_unique_id, and unified_diff body fields. 25 tests, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add `signalforge.diff._sidecar` — fail-closed JSON sidecar writer for the diff renderer (DEC-009). Mirrors `grade-layer.md` DEC-006/012 verbatim: single-document overwrite, ``O_WRONLY | O_CREAT | O_TRUNC | 0o600``, single ``os.write`` (looped on short returns), ``os.fsync``, descriptor-release-only ``try / finally``. No ``except`` handler around write/fsync — the propagation IS the defence. * ``_DIFF_SIDECAR_RECORD_LIMIT_BYTES = 10_000_000`` (10 MB; an order of magnitude above the grade sidecar's 1 MB cap because diff text is larger by nature). Pre-write size check raises ``DiffSidecarRecordTooLargeError`` BEFORE any ``os.open`` so an oversize payload leaves no on-disk artefact. * Symlink-hardened path canonicalisation at writer entry via ``signalforge.warehouse._path_safety.canonicalise_path``. Containment is gated against the **caller-supplied** ``project_dir``, not a derivation of ``sidecar_path`` (mirrors grade's post-QG fix). Failures wrap as ``DiffSidecarWriteError(cause=...)``. * AST defence test in ``tests/diff/test_sidecar.py`` walks the module's syntax tree and asserts: (a) exactly one ``ast.Try`` block guards the ``os.write`` / ``os.fsync`` syscalls (the descriptor-release ``try / finally``), and (b) that block has zero ``except`` handlers. An accidental ``try / except OSError`` around the write/fsync would silently swallow the exact failure mode the fail-closed pattern exists to surface. Test coverage (13 cases): happy-path round-trip via ``json.loads``, ``0o600`` mode bits, parent-directory creation, ``O_TRUNC`` overwrite semantics, single ``fsync``, oversize pre-flight (no on-disk artefact), symlink-escape rejection (containment gate against caller-supplied ``project_dir``), absolute-path-outside-project rejection, symlink-loop rejection, short-write loop covering the full payload, zero-byte ``write`` raises ``OSError``, ``OSError`` propagation, and the AST defence above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ap + symlink hardening)
Extends `signalforge.diff._renderers` with `MarkdownRenderer`: - GitHub-flavored Markdown output (heading + count summary + GFM pipe-table + fenced ```diff block). - Per-cell escaping via `escape_markdown_scalar(in_table_cell=True)` per DEC-008; pipe / backtick / backslash / row-breaking control chars are entity-encoded inside cells, while the fenced ```diff block carries raw upstream content (the fence is the defence). - DEC-005 truncation: when `unified_diff` exceeds `config.markdown_max_diff_chars` (default 60_000), the body is truncated at the last complete `@@` hunk boundary; a footer `... (N more lines truncated — see <project_dir>/.signalforge/diff.json for full diff)` lives INSIDE the fenced block. Falls back to a line-boundary cut for hunk-less bodies. - DEC-007 strip-then-escape composes: `strip_ansi_escapes` runs unconditionally on every user-content field (model_unique_id, artifact_id, why, drop_reason, test_type, diff body) BEFORE markdown-escaping or fence-wrapping. - Empty `unified_diff` suppresses the diff block entirely; empty entries tuple emits `_(no candidate artifacts)_`. - `project_dir` constructor kwarg renders into the truncation footer (placeholder `<project_dir>` when None — stable for snapshots). Tests cover: ABC subclass-of, pipe-table well-formedness, fenced diff passthrough vs. table-cell escaping (the same pipe / header content is encoded in cells but raw inside the fence), truncation at last hunk + dropped-line count + fence-internal footer placement, project_dir rendering, ANSI strip in cells + diff body, and the empty-diff / empty-entries fallbacks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (US-010) Wires every prior story (errors US-001, models US-002, config US-003, safety US-004, emitter US-005, artifact-id US-006, sidecar US-007, renderers US-008/009) into one public seam: render_diff(model, candidate, prune_result, *, grading_report, existing_schema, config, output_path, sidecar_path, project_dir) -> DiffReport. Boundary checks (DEC-002) raise BEFORE any other work; existing_schema size cap (DEC-006) + soft-warn (DEC-014) gate yaml.safe_load; symlink-hardened path canonicalisation against project_dir for both output_path and sidecar_path mirrors the grade-engine post-QG fix verbatim. Reproducibility hashes (DEC-016) computed via canonical-sort blake2b-8 of each input. Single INFO log at happy-path end with lazy-format json.dumps payload (DEC-015). JsonRenderer added to _renderers.py — model_dump_json(indent=2, by_alias=True), stateless. 19 new tests cover the boundary-check trio, the size-cap + soft-warn pair, renderer dispatch across all three concretes, JsonRenderer round-trip, sidecar write, output_path / sidecar_path symlink containment, INFO log shape, and an end-to-end happy path that asserts kept/dropped/flagged tier assignment and DEC-016 hash presence.
…iff + JsonRenderer
…ft detector (US-011) Commits the 10-case fixture matrix (DEC-017 of #8) under tests/fixtures/diff/, the deterministic snapshot-input builder at tests/diff/_snapshot_inputs.py, the regenerate script tests/fixtures/diff/regenerate.sh (mirrors tests/fixtures/regenerate.sh shape), the byte-equality snapshot tests at tests/diff/test_snapshot_fixtures.py, and the schema-drift detector tests/diff/test_drift_detector.py with StrictDiffReport / StrictDiffEntry mirrors validated against diff_report_v1.json and the new diff_entry_v1.json fixture. The 11 fixture artefacts on disk cover: full_with_grade.{ansi,md,json} — happy path, three surfaces. no_existing_schema.ansi — /dev/null source for unified diff. kept_only.ansi — every artifact tier=kept. dropped_only.ansi — every artifact tier=dropped. no_grading_report.ansi — score columns null; no flagged tier. plain_no_color.txt — ANSI surface w/ NO_COLOR=1. narrow_terminal.ansi — 40-col TTY (DEC-013 compact mode). injection_payloads.{ansi,md} — adversarial content (DEC-007/008). Static checks on the plain_no_color and injection_payloads fixtures pin DEC-007 (strip user-content ANSI escapes UNCONDITIONALLY) and DEC-021 (NO_COLOR + force_color=False produces zero ANSI escapes). The drift detector ships the standard four-test pattern (validate fixture, validate each entry, field-set parity, sanity-floor reject unknown field) per the prune / grade precedent. Validation: ruff / pyright / pytest all green; full suite 1178 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…egenerate.sh + drift detector
…S-012) Export the v0.1 public surface from signalforge.diff per DEC-004 of plans/super/8-diff-renderer.md: render_diff orchestrator, DiffConfig + load_diff_config, DiffReport / DiffEntry / Tier result models, and the seven-class DiffError hierarchy. Concrete renderers (AnsiRenderer, MarkdownRenderer, JsonRenderer) and internal helpers (_emitter, _sidecar, _artifact_id, _ansi_safety, _markdown_safety) stay private — reachable via dotted import for internal callers but absent from __all__. Add docs/diff-ops.md mirroring docs/grade-ops.md as the operational reference: overview, public API surface, configuration block, renderer-kind selection, sidecar JSON schema, reproducibility hash fields, decision matrix, operational notes (symlink hardening, log events, ANSI / Markdown injection escaping), snapshot fixture matrix, debugging, and failure-mode cross-reference. Extend tests/llm/test_logger_grep_gate.py to scan src/signalforge/diff/ as the fifth directory per DEC-019 — the project-wide ANSI-safe lazy-format logger gate now covers llm, draft, prune, grade, and diff. Planted-violation self-check confirms the gate fires on an f-string interpolated _LOGGER call in signalforge.diff.engine. Add tests/diff/test_public_api.py asserting __all__ matches the documented surface, that every public name imports via ``from signalforge.diff import ...``, that concrete renderers and internal helpers stay out of __all__, that the concrete renderers remain reachable via dotted import (the documented escape hatch), and that all six DiffError subclasses inherit from DiffError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…f-ops.md + logger grep gate
Fixes from 4-pass code review on the diff renderer epic (#8): 1. Inverted soft-warn / hard-cap defaults in DiffConfig (DEC-014 was dead code). Swap so warn_at_bytes (1MB) < size_limit_bytes (10MB), and add a model_validator that fails loud at config-load time when warn_at >= size_limit. 2. DiffConfig.sidecar_size_limit_bytes was silently ignored — wire it through render_diff to write_sidecar via a new size_limit_bytes kwarg. Adds end-to-end test pinning the orchestrator-level error. 3. Drop dead DiffSidecarRecordTooLargeError catch from the _write_rendered_text exception ladder (output_path branch can't raise that error; only sidecar path can). CodeRabbit skill not available in this session; skipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…S-014) Distil DEC-001..DEC-021 of the diff renderer (#8) into .claude/rules/diff-renderer.md, mirroring grade-layer.md section structure verbatim. Bake the post-QG-fix lessons (inverted warn/limit defaults; sidecar_size_limit_bytes wiring; dead exception catch) and the load-bearing rules (tier classification + no-grading degrade, fail-closed sidecar, ANSI strip unconditional, Markdown hunk-boundary truncation, three boundary checks at orchestrator entry, reproducibility hashes, drift detector, logger grep gate fifth dir). Update CLAUDE.md Repository status to "Eight issues shipped" with a new bullet for #8 matching the prior format; extend the Public API surface bullet list and Internals listing; remove "diff renderer #8" from the remaining-feature-work line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…laude/rules/diff-renderer.md + CLAUDE.md update)
There was a problem hiding this comment.
Pull request overview
Despite the plan-oriented PR metadata, this change appears to add the first end-to-end implementation of issue #8’s diff-renderer stage in SignalForge, sitting after prune/grade and before the future CLI flow. It introduces the new signalforge.diff package, snapshot-driven test coverage, and repo documentation/rules updates to treat diff rendering as a shipped subsystem.
Changes:
- Adds the
signalforge.diffsubpackage: public API, config, models, errors, artifact-id logic, YAML emission, ANSI/Markdown sanitization, rendering, orchestration, and sidecar persistence. - Adds extensive tests and committed fixtures covering snapshots, drift/public API enforcement, config/error behavior, emitter logic, artifact IDs, renderer safety, and sidecar durability.
- Updates repo guidance and safety gates so the diff layer is part of the documented architecture and logger-format enforcement.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/llm/test_logger_grep_gate.py | Extends the lazy-logger grep gate to scan signalforge.diff. |
| tests/fixtures/diff/regenerate.sh | Shell entrypoint for regenerating committed diff snapshot fixtures. |
| tests/fixtures/diff/plain_no_color.txt | Golden snapshot for plain/no-color ANSI rendering. |
| tests/fixtures/diff/no_grading_report.ansi | Golden ANSI snapshot for runs without grading input. |
| tests/fixtures/diff/no_existing_schema.ansi | Golden ANSI snapshot for /dev/null existing-schema diffs. |
| tests/fixtures/diff/narrow_terminal.ansi | Golden ANSI snapshot for narrow-terminal layout. |
| tests/fixtures/diff/kept_only.ansi | Golden ANSI snapshot for all-kept output. |
| tests/fixtures/diff/injection_payloads.md | Golden Markdown snapshot for hostile input payloads. |
| tests/fixtures/diff/injection_payloads.ansi | Golden ANSI snapshot for hostile input payloads. |
| tests/fixtures/diff/full_with_grade.md | Golden Markdown snapshot for full graded output. |
| tests/fixtures/diff/full_with_grade.json | Golden JSON snapshot for full graded output. |
| tests/fixtures/diff/full_with_grade.ansi | Golden ANSI snapshot for full graded output. |
| tests/fixtures/diff/dropped_only.ansi | Golden ANSI snapshot for all-dropped output. |
| tests/fixtures/diff/diff_report_v1.json | Committed fixture for DiffReport drift detection. |
| tests/fixtures/diff/diff_entry_v1.json | Committed fixture for DiffEntry drift detection. |
| tests/fixtures/diff/_regenerate.py | Python helper that renders and writes all snapshot fixtures. |
| tests/diff/test_snapshot_fixtures.py | Byte-for-byte snapshot tests and fixture presence/count checks. |
| tests/diff/test_sidecar.py | Sidecar writer tests for durability, containment, and fail-closed behavior. |
| tests/diff/test_public_api.py | Public-surface and private-export enforcement for signalforge.diff. |
| tests/diff/test_models.py | Model construction, repr, round-trip, and drift/parity tests. |
| tests/diff/test_markdown_safety.py | Unit tests for Markdown cell escaping behavior. |
| tests/diff/test_errors.py | Typed error hierarchy and message/remediation tests. |
| tests/diff/test_engine.py | Orchestrator tests for validation, rendering, sidecar, logging, and happy paths. |
| tests/diff/test_emitter.py | Tests for deterministic YAML emission of kept artifacts. |
| tests/diff/test_drift_detector.py | Separate strict-mirror drift detector for diff result shapes. |
| tests/diff/test_config.py | Config loading/default/validation tests for the diff: block. |
| tests/diff/test_artifact_id.py | Artifact ID shape/collision/parity tests against grade logic. |
| tests/diff/test_ansi_safety.py | Unit tests for ANSI stripping behavior. |
| tests/diff/_snapshot_inputs.py | Deterministic builders and recipes for snapshot cases. |
| src/signalforge/diff/models.py | Defines DiffEntry, DiffReport, and Tier. |
| src/signalforge/diff/errors.py | Defines the diff layer’s typed exception hierarchy. |
| src/signalforge/diff/config.py | Adds DiffConfig and load_diff_config. |
| src/signalforge/diff/_sidecar.py | Implements fail-closed JSON sidecar persistence. |
| src/signalforge/diff/_markdown_safety.py | Adds Markdown escaping for rendered table content. |
| src/signalforge/diff/_emitter.py | Emits canonical proposed schema.yml from kept artifacts. |
| src/signalforge/diff/_artifact_id.py | Mirrors grade-layer artifact ID generation. |
| src/signalforge/diff/_ansi_safety.py | Adds ANSI escape stripping for user-controlled output. |
| src/signalforge/diff/_renderers.py | Implements ANSI/Markdown/JSON renderers. |
| src/signalforge/diff/engine.py | Wires validation, diff generation, rendering, and persistence. |
| src/signalforge/diff/init.py | Exposes the package’s public API. |
| CLAUDE.md | Updates repo status/public API docs to include issue #8 as shipped. |
| .claude/rules/diff-renderer.md | Adds repository rules/conventions for the diff-renderer subsystem. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
tests/diff/test_drift_detector.py (1)
181-214: ⚡ Quick winField-name parity alone won’t catch schema-type drift.
These checks only compare keys, so a production change like
str -> Literal[...]or a default-value change can still slip through if the current fixtures happen to validate under both shapes. Since this file is the drift gate, it’s worth comparing annotations/defaults too.Suggested fix
def test_diff_entry_field_set_parity() -> None: @@ - strict_fields = set(StrictDiffEntry.model_fields.keys()) - prod_fields = set(DiffEntry.model_fields.keys()) - missing_in_strict = prod_fields - strict_fields - extra_in_strict = strict_fields - prod_fields + strict_fields = StrictDiffEntry.model_fields + prod_fields = DiffEntry.model_fields + missing_in_strict = set(prod_fields) - set(strict_fields) + extra_in_strict = set(strict_fields) - set(prod_fields) @@ assert not extra_in_strict, ( f"StrictDiffEntry has fields absent from DiffEntry: " f"{extra_in_strict}. Remove from StrictDiffEntry or add to DiffEntry." ) + for name in prod_fields: + assert strict_fields[name].annotation == prod_fields[name].annotation + assert strict_fields[name].default == prod_fields[name].default @@ def test_diff_report_field_set_parity() -> None: @@ - strict_fields = set(StrictDiffReport.model_fields.keys()) - prod_fields = set(DiffReport.model_fields.keys()) - missing_in_strict = prod_fields - strict_fields - extra_in_strict = strict_fields - prod_fields + strict_fields = StrictDiffReport.model_fields + prod_fields = DiffReport.model_fields + missing_in_strict = set(prod_fields) - set(strict_fields) + extra_in_strict = set(strict_fields) - set(prod_fields) @@ assert not extra_in_strict, ( f"StrictDiffReport has fields absent from DiffReport: " f"{extra_in_strict}. Remove from StrictDiffReport or add to DiffReport." ) + for name in prod_fields: + assert strict_fields[name].annotation == prod_fields[name].annotation + assert strict_fields[name].default == prod_fields[name].default🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/diff/test_drift_detector.py` around lines 181 - 214, The tests test_diff_entry_field_set_parity and test_diff_report_field_set_parity only compare the sets of keys on StrictDiffEntry.model_fields vs DiffEntry.model_fields (and StrictDiffReport vs DiffReport), so schema-type or default-value drift can be missed; update these tests to iterate each common field name and assert that the corresponding model field attributes (e.g., model_fields[name].annotation or .outer_type_ and .default / .required flags) are equal between StrictDiffEntry and DiffEntry (and between StrictDiffReport and DiffReport), failing with a clear message identifying the field and the mismatched attribute to force detection of type/default changes as well as missing/extra fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/rules/diff-renderer.md:
- Around line 90-92: The fenced code block in diff-renderer.md is missing a
language tag and triggers markdownlint MD040; update the example fence in
.claude/rules/diff-renderer.md to include a language (e.g., change the opening
triple-backtick from ``` to ```text) so the fenced example is tagged as text and
the MD040 rule is satisfied.
In `@docs/diff-ops.md`:
- Around line 307-323: The two fenced code blocks showing the log lines (the one
starting with "WARNING signalforge.diff.engine: large existing schema.yml: ..."
and the one starting with "INFO signalforge.diff.engine: rendered diff: ...")
need language tags to satisfy markdownlint MD040; update each triple-backtick
fence to include a language identifier such as text or log (e.g., change ``` to
```text or ```log) so both examples are tagged while leaving their content
unchanged.
- Around line 123-126: The sample config has existing_schema_warn_at_bytes
(10485760) greater than existing_schema_size_limit_bytes (1000000) making the
warn path unreachable; update the documented default so warn_at < size_limit
(e.g., set existing_schema_warn_at_bytes to a value lower than 1000000 or raise
existing_schema_size_limit_bytes above 10485760) and update the prose to state
the invariant clearly; also mention/enforce the DiffConfig model-validator rule
that existing_schema_warn_at_bytes < existing_schema_size_limit_bytes (DEC-014)
and ensure the symbols markdown_max_diff_chars,
existing_schema_size_limit_bytes, existing_schema_warn_at_bytes, and
sidecar_size_limit_bytes are consistent in the example.
In `@plans/super/8-diff-renderer.md`:
- Line 644: The inline code examples showing triple-backticks contain extra
spaces (e.g. "` ``` `") which trigger MD038; update the text in the markdown
around the `escape_markdown_scalar` signature so the inline code spans remove
the internal spaces (use "```" instead of " ``` "), and do the same for the
other occurrence of the triple-backtick example referenced near the second
example; ensure both instances continue to show the escaping semantics for
`escape_markdown_scalar(text: str, *, in_table_cell: bool = False) -> str` but
with corrected inline code formatting.
- Around line 351-353: The fenced code block in plans/super/8-diff-renderer.md
(the block currently starting with just ``` around the truncated diff) is
missing a language hint which triggers MD040; fix it by changing the opening
fence to include a language specifier (e.g., replace ``` with ```text) so the
code block becomes ```text and leave the block contents unchanged. Locate the
plain triple-backtick block shown in the diff and update its opening fence
accordingly.
- Around line 481-483: Replace the validation command string in the plan section
that currently runs "ruff check . && ruff format --check . && pyright && pytest"
with the repository's canonical validation command that includes the editable
install step; update the command to perform the editable install of dev deps
first (pip install -e ".[dev]") followed by ruff check, ruff format --check,
pyright and pytest so the plan uses the same validation flow as the rest of the
repo.
- Around line 1097-1099: Remove the duplicated "## Beads Manifest" heading in
the markdown so only one "## Beads Manifest" section remains; locate the
repeated heading string "## Beads Manifest" in plans/super/8-diff-renderer.md
(the block that currently reads "## Beads Manifest" followed by "*To be filled
at devolve time.*") and delete the extra occurrence, keeping the intended single
section and preserving the "*To be filled at devolve time.*" content.
In `@src/signalforge/diff/_ansi_safety.py`:
- Line 23: The CSI regex _ANSI_CSI_RE currently only matches escape sequences
ending with letters; update it to match any CSI final byte in the range '@'
through '~' by changing the character class at the end to '@-~' (so the pattern
matches \x1b\[[0-9;]*[`@-`~]) to ensure sequences like '\x1b[3~' and '\x1b[200~'
are also sanitized.
In `@src/signalforge/diff/_markdown_safety.py`:
- Around line 46-80: escape_markdown_scalar currently never neutralizes raw HTML
metacharacters; update escape_markdown_scalar to HTML-entity-encode '&', '<',
and '>' (as &, <, >) before any other escaping so user-controlled HTML
cannot be injected, keeping the existing in_table_cell handling for '|' and
_TABLE_CELL_CONTROL_CHARS; ensure you replace '&' first to avoid
double-escaping, then '<' and '>', and then continue with the existing
backslash/`/|` logic in escape_markdown_scalar.
In `@src/signalforge/diff/_renderers.py`:
- Around line 642-649: The table cell values are still allowing raw HTML like
"<...>" through because escape_markdown_scalar(..., in_table_cell=True) only
neutralizes pipes/backticks/control chars; update the markdown safety pipeline
to also entity-encode '<', '>', and '&' (use or extend the existing
_markdown_safety) and apply unconditional ANSI stripping (_ansi_safety) before
escaping for every user-content field (e.g., where escape_markdown_scalar is
called for entry.tier, clean_artifact, clean_test_type, clean_drop_reason,
score_text, clean_why); ensure escape_markdown_scalar delegates to or calls
_ansi_safety and then _markdown_safety that replaces '<'=>'<', '>'=>'>',
'&'=>'&' when in_table_cell is true so no raw HTML can be emitted in
markdown table cells.
- Around line 675-690: The current renderer uses a static "```" fence around the
diff (in the method that builds clean_body and returns the fenced block), which
can be prematurely closed if clean_body contains a run of backticks; change the
fencing to be dynamic: scan clean_body (and the footer if present) for the
longest consecutive run of backticks, compute a fence delimiter longer than that
(e.g. longest_run + 1 or +3 backticks), and use that computed fence instead of
the static "```" when building the final return string in the function that
calls _markdown_max_diff_chars(), _truncate_at_last_hunk(), and
_truncation_footer(). Ensure you also apply the same fence length to both the
opening and closing fences so the fenced block cannot be closed by content.
In `@src/signalforge/diff/config.py`:
- Around line 235-252: The code reads config_file with
config_file.read_text(...) without symlink-hardening or OSError translation;
update the load logic in the function that uses config_file (the branch that
sets config_file from path or project_dir/_DEFAULT_CONFIG_FILENAME) to
canonicalize and validate the path (e.g., resolve the config_file and the
project_dir via Path.resolve(strict=False) and ensure the resolved config_file
is inside the resolved project_dir for the default lookup), and wrap the
config_file.read_text call in a try/except that catches OSError and raises
DiffError (using the same remediation style as the YAML/validation branches) so
IsADirectoryError/PermissionError are translated into DiffError; keep references
to DiffError, DiffConfig, _DEFAULT_CONFIG_FILENAME and the config_file/read_text
usage to locate where to apply the change.
In `@src/signalforge/diff/engine.py`:
- Around line 255-267: The DiffEntry currently always uses decision.why even
when grading flips a kept artifact to flagged; change the logic after computing
grading_results, score, passed and tier (functions _aggregate_grading and
_tier_for_kept) so that when tier == Tier.FLAGGED the DiffEntry.why is derived
from the grading_results (e.g., pick the first failing rule's message or
synthesize a one-line summary of failing grading_results), otherwise keep
decision.why; ensure the change is applied where DiffEntry(...) is constructed
so flagged rows show the grading reason instead of the original decision.why.
- Around line 149-176: _lookup uses Python's built-in id() instead of the test
object's stable id() method, so change the args_hash lookup to use the test's
own id() accessor (e.g. args_hashes.get(decision.test.id())) in
_resolve_test_artifact_id and the other occurrence referenced (around the second
spot mentioned) so parameterized tests recover their args_hash across
reconstructed CandidateTest instances and artifact_id_for remains byte-equal
with the grade engine.
- Around line 275-304: The code currently returns None for ungraded
doc/rationale artifacts, causing them to disappear from DiffReport.entries;
instead always construct and return a DiffEntry even when grading_results is
empty: detect the empty case (grading_index.get(artifact_id, []) == []) and
create a DiffEntry using description as the one-line why, set score to a
sentinel (e.g. 0.0) and passed to False and derive tier by calling
_tier_for_kept(0.0, False) (or the appropriate kept-tier helper), and keep the
same fields (artifact_id, test_type=None, drop_reason=None); apply the same
change to the analogous block referenced at lines 336-341 so all doc artifacts
are recorded rather than dropped.
In `@tests/diff/test_models.py`:
- Around line 46-47: The test currently imports production literals Tier and
DropReason which lets upstream changes alter the test fixtures; instead keep the
drift mirror independent by removing the imports of Tier and DropReason and
defining test-local literal/enum aliases (or use plain string literals) that
match the expected mirror values, then use those local symbols when building
StrictDiffEntry/DiffEntry/DiffReport fixtures (reference the test uses
DiffEntry, DiffReport and mentions StrictDiffEntry) so the test no longer
inherits production alias expansions and will correctly gate intended
schema-drift updates.
In `@tests/fixtures/diff/injection_payloads.md`:
- Line 8: The markdown table cell sanitizer escape_markdown_scalar currently
encodes pipes and control chars but not angle brackets; update the function
(escape_markdown_scalar in src/signalforge/diff/_markdown_safety.py) to also
replace '<' with '<' and '>' with '>' inside the in_table_cell=True
branch, then add a unit test case in tests/diff/test_markdown_safety.py that
asserts table-cell content containing '</details>' becomes '</details>'
(and/or similar '<'/'>' examples), and finally regenerate the fixtures/snapshots
in tests/fixtures/diff/injection_payloads.md to reflect the escaped output.
---
Nitpick comments:
In `@tests/diff/test_drift_detector.py`:
- Around line 181-214: The tests test_diff_entry_field_set_parity and
test_diff_report_field_set_parity only compare the sets of keys on
StrictDiffEntry.model_fields vs DiffEntry.model_fields (and StrictDiffReport vs
DiffReport), so schema-type or default-value drift can be missed; update these
tests to iterate each common field name and assert that the corresponding model
field attributes (e.g., model_fields[name].annotation or .outer_type_ and
.default / .required flags) are equal between StrictDiffEntry and DiffEntry (and
between StrictDiffReport and DiffReport), failing with a clear message
identifying the field and the mismatched attribute to force detection of
type/default changes as well as missing/extra fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8c21bbe-9ee7-4cae-8567-062136a0fb4b
📒 Files selected for processing (45)
.claude/rules/diff-renderer.mdCLAUDE.mddocs/diff-ops.mdplans/super/8-diff-renderer.mdsrc/signalforge/diff/__init__.pysrc/signalforge/diff/_ansi_safety.pysrc/signalforge/diff/_artifact_id.pysrc/signalforge/diff/_emitter.pysrc/signalforge/diff/_markdown_safety.pysrc/signalforge/diff/_renderers.pysrc/signalforge/diff/_sidecar.pysrc/signalforge/diff/config.pysrc/signalforge/diff/engine.pysrc/signalforge/diff/errors.pysrc/signalforge/diff/models.pytests/diff/_snapshot_inputs.pytests/diff/test_ansi_safety.pytests/diff/test_artifact_id.pytests/diff/test_config.pytests/diff/test_drift_detector.pytests/diff/test_emitter.pytests/diff/test_engine.pytests/diff/test_errors.pytests/diff/test_markdown_safety.pytests/diff/test_models.pytests/diff/test_public_api.pytests/diff/test_renderers.pytests/diff/test_sidecar.pytests/diff/test_snapshot_fixtures.pytests/fixtures/diff/_regenerate.pytests/fixtures/diff/diff_entry_v1.jsontests/fixtures/diff/diff_report_v1.jsontests/fixtures/diff/dropped_only.ansitests/fixtures/diff/full_with_grade.ansitests/fixtures/diff/full_with_grade.jsontests/fixtures/diff/full_with_grade.mdtests/fixtures/diff/injection_payloads.ansitests/fixtures/diff/injection_payloads.mdtests/fixtures/diff/kept_only.ansitests/fixtures/diff/narrow_terminal.ansitests/fixtures/diff/no_existing_schema.ansitests/fixtures/diff/no_grading_report.ansitests/fixtures/diff/plain_no_color.txttests/fixtures/diff/regenerate.shtests/llm/test_logger_grep_gate.py
…ngine.py + design Q1/Q3) Six fixes to the diff orchestrator from PR #25 code review: - #1: wrap mkdir in both output_path and sidecar_path branches with try/except → raise DiffSidecarWriteError(cause=...). A project_dir whose parent is an existing FILE no longer leaks an untyped OSError out of the diff layer. - #2: doc artifacts (column/model description/rationale) ALWAYS emit a DiffEntry per Architectural Commitment #5. When grading is absent or no matching result exists, the row gets tier="kept", score=None, passed=None, why="kept (no grading)". Prune-only runs now surface every present description text in the kept/dropped/flagged table. - #3: flagged-tier why now reflects the GRADING reason, not the prune decision why. Format: "failed grading: <criterion_id> — <reasoning>" (truncated to max_why_chars). The first failing criterion drives it for determinism. - #4: replace id(decision.test) lookup with structural keying so the args_hash join survives JSON-rehydration of the prune result. Cross-stage parity with the grade engine artifact_ids is preserved (post-QG fix to enable diff↔grade-sidecar joins after the prune result has been persisted+reloaded). - #5 (Q1=A): default sidecar_path to <project_dir>/.signalforge/ diff.json when write_sidecar=True (default). New write_sidecar kwarg disables the sidecar entirely. Makes the diff sidecar an always-on durable record by default, mirroring grade/prune audit precedent. - #6 (Q3=A): skip renderer.render(report) entirely when there's no consumer (no output_path AND write_sidecar=False / sidecar serialises the report directly). Move duration_seconds capture to immediately before the INFO log so it reflects the full wall-clock including renderer + writes. Tests: 8 new regression tests added; one existing test updated for the sidecar default change; one existing assertion updated for the doc-row emission count change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on-engine fixes)
Code fixes:
- _ansi_safety: broaden CSI regex to cover the full final-byte range
(@-~), not just letters; add tests for tilde-terminated key
sequences (`\x1b[3~`) and bracketed-paste markers (`\x1b[200~`).
- _markdown_safety: HTML-entity-encode `&`, `<`, `>` before any
Markdown escape (defence against `<script>`, `</details>`,
`<img src=x>` smuggling). Order matters: `&` first to avoid
double-encoding subsequent escape entities.
- _renderers (MarkdownRenderer): four bug fixes —
(a) dropped_line_count off-by-one when body ends with `\n`;
(b) the WHOLE rendered diff block now fits under
`markdown_max_diff_chars` (subtract fence + footer overhead
before truncating);
(c) when the first hunk alone exceeds the cap, emit only the
`---`/`+++` file-header lines rather than a mid-hunk character
cut (which would produce malformed unified-diff output);
(d) dynamic fence length: scan the body for the longest backtick
run and pick `max(3, longest_run + 1)` so a YAML payload
containing literal triple-backticks cannot close the outer
fence prematurely.
- config: wrap `read_text` in try/except for OSError /
IsADirectoryError / PermissionError; symlink-harden the resolved
config path via `signalforge.warehouse._path_safety.canonicalise_path`
before any read (mirrors the orchestrator-level treatment of
`output_path` / `sidecar_path`).
Docs / plan fixes:
- errors.py: `DiffInputTooLargeError.default_remediation` now
matches production's 10 MB cap (not the obsolete 1 MB literal).
- docs/diff-ops.md: every reference to
`existing_schema_size_limit_bytes` (10 MB) and
`existing_schema_warn_at_bytes` (1 MB) defaults consistent;
fenced log-output blocks tagged as `text` (MD040); soft-warn
prose corrected ("warn 1 MB fires below hard cap 10 MB").
- .claude/rules/diff-renderer.md: tagged the truncation-footer
fence as `text`.
- plans/super/8-diff-renderer.md: DEC-006 / AR-5 / AR-11 / AR-13 /
the `DiffConfig` snippet now carry the post-QG-fix defaults
(10 MB hard cap, 1 MB soft warn — original plan had them
inverted before commit c3dc6ab); validation command snippet now
prefixes `pip install -e ".[dev]"`; MD038 inline-code-span
spaces removed; duplicate `## Beads Manifest` heading dropped;
truncation-footer fence tagged as `text`.
Tests:
- New regression tests in tests/diff/test_ansi_safety.py (3),
test_markdown_safety.py (7), test_renderers.py (5) covering
every code fix above.
- tests/diff/test_config.py: regression tests for IsADirectoryError
wrap and the symlink-escape rejection path.
Fixtures:
- tests/fixtures/diff/injection_payloads.md regenerated via
`tests/fixtures/diff/regenerate.sh` to capture the new
HTML-escaped `</details>` rendering of the hostile input.
Validation: `ruff check .`, `ruff format --check .`, `pyright`,
`pytest` all green (1222 passed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review Summary — Address all 31 threadsTwo parallel worker passes landed on
All four checks green: ruff check + ruff format --check + pyright (0 errors) + pytest (1222 passed, 10 deselected). Fixed — code (13 threads)
Fixed — design questions (3 threads, user-confirmed)
Fixed — docs (9 threads)
Fixture regenerations
Other
False positives — noneEvery thread either landed a code fix, a doc fix, or implements a user-confirmed design decision. No deferrals. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/rules/diff-renderer.md:
- Around line 31-32: The docs claim that sidecar_path=None skips sidecar writing
but the code actually defaults to writing to
<project_dir>/.signalforge/diff.json when sidecar writing is enabled; update the
sentence referencing sidecar_path to reflect the real behavior: state that
sidecar_path=None disables sidecar output, but when sidecar writing is enabled
and no explicit path is provided the runtime/CLI will default to
<project_dir>/.signalforge/diff.json (mention the CLI `#9` wiring if relevant),
and ensure references to the diff layer, sidecar_path, and the default path are
consistent with the implementation.
- Around line 65-66: The documentation for the ANSI-strip regex is out of date:
update the text describing signalforge.diff._ansi_safety.strip_ansi_escapes so
it reflects the broader CSI/ANSI grammar actually handled (including
intermediate bytes and alternate terminators like tilde) rather than the old
strict pattern `\x1b\[[0-9;]*[a-zA-Z]`; mention that both AnsiRenderer and
MarkdownRenderer call strip_ansi_escapes on user fields (`description`,
`rationale`, `evidence`, `reasoning`, `why`, `drop_reason`, `artifact_id`)
before adding color/markdown, and either document the precise regex/grammar now
used or state clearly that the implementation accepts extended CSI sequences
(intermediates and non-letter terminators) to match tests.
In `@docs/diff-ops.md`:
- Around line 76-77: Update the docs to match the current render_diff signature
and default sidecar behavior: document that render_diff(model, candidate,
prune_result, *, grading_report=None, existing_schema=None, config=None,
output_path=None, sidecar_path=None, project_dir=None, write_sidecar: bool =
True) -> DiffReport includes the boolean write_sidecar flag (defaults to True)
and that when enabled the sidecar is written by default to
<project_dir>/.signalforge/diff.json; adjust the descriptive text around
render_diff (and the corresponding sections mentioned at lines ~192-200) to
reflect the opt-out (write_sidecar=False) semantics and the default sidecar path
and mention project_dir defaulting to Path.cwd() as used for canonicalisation.
In `@src/signalforge/diff/engine.py`:
- Around line 193-204: The current assignment out[key] = base_hash if ordinal ==
1 else f"{base_hash}:{ordinal - 1}" collapses exact-duplicate tests to a single
scalar per structural key and causes artifact_id parity loss; change the storage
for structural keys (the dict referenced as out) from a single scalar to a
list/queue of suffixes (e.g., map _StructuralKey -> List[str]) and populate it
in the same place where base_hash/ordinal are computed (the block with out[key]
assignment), then update the decision traversal/lookup logic to use a per-key
consumption counter to pop or index the next suffix deterministically for each
occurrence so duplicates are disambiguated in order across JSON rehydration and
grading joins.
- Around line 801-809: The DiffReport is constructed with a placeholder
duration_seconds but never updated before the report (and sidecar payload) are
written/returned; update report.duration_seconds to time.monotonic() -
started_at at the same point where the final wall-clock duration is
computed/logged (the code that currently logs the final duration), and ensure
any sidecar/write call and the return value use this updated report instance so
the persisted and returned DiffReport contains the true final duration_seconds;
look for references to DiffReport, report, duration_seconds, started_at and the
final logging call and refresh the field immediately prior to writing/returning.
In `@tests/diff/test_engine.py`:
- Around line 282-283: The test filters caplog.records only by level (e.g.,
warns = [rec for rec in caplog.records if rec.levelname == "WARNING"]) which can
match other loggers and cause flakiness; update each occurrence (the list
comprehension assigning warns at the three locations) to also scope to the
engine logger by adding a logger-name check (e.g., rec.name == "engine" or
rec.name.startswith("engine")) so the comprehension becomes something like: [rec
for rec in caplog.records if rec.levelname == "WARNING" and rec.name ==
"engine"]; apply the same change to the other two occurrences mentioned so all
assertions only consider engine logger records.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 310d025a-996d-406f-94fd-7363dfa214a5
📒 Files selected for processing (15)
.claude/rules/diff-renderer.mddocs/diff-ops.mdplans/super/8-diff-renderer.mdsrc/signalforge/diff/_ansi_safety.pysrc/signalforge/diff/_markdown_safety.pysrc/signalforge/diff/_renderers.pysrc/signalforge/diff/config.pysrc/signalforge/diff/engine.pysrc/signalforge/diff/errors.pytests/diff/test_ansi_safety.pytests/diff/test_config.pytests/diff/test_engine.pytests/diff/test_markdown_safety.pytests/diff/test_renderers.pytests/fixtures/diff/injection_payloads.md
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/diff/injection_payloads.md
- src/signalforge/diff/_ansi_safety.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/signalforge/diff/_markdown_safety.py
- src/signalforge/diff/config.py
- src/signalforge/diff/_renderers.py
- src/signalforge/diff/errors.py
…Rabbit on second pass) Six fixes from the second-pass CodeRabbit review: 1. engine.py — exact-duplicate structural-key collapse. Replace the ``dict[_StructuralKey, str | None]`` that lost duplicate args_hash entries (last-assignment-wins) with a per-key queue ``dict[_StructuralKey, list[str | None]]``. ``_resolve_test_artifact_id`` now pops from the front of the queue per-decision so two byte-identical ``CandidateTest`` instances in ``prune_result.decisions`` get distinct artifact_ids matching grade engine's id()-keyed ordinal suffixes. Iteration order in the queue matches ``signalforge.prune.engine._iter_candidate_tests`` (columns first, then model-level) so positional consumption aligns with the prune walk. 2. engine.py — ``DiffReport.duration_seconds`` refresh. Construct the report with placeholder ``0.0``, refresh via ``model_copy`` after rendering and the optional output_path write but BEFORE the sidecar write, so the persisted JSON, returned report, and INFO log all carry the same wall-clock value. The sidecar write block moved below the refresh. 3. tests/diff/test_engine.py — caplog scoping. Three call sites that filtered by ``levelname`` only now also gate on ``rec.name == "signalforge.diff.engine"`` so unrelated logger records can't false-positive the assertions. 4. docs/diff-ops.md — sidecar API/default sync. The ``render_diff`` signature now shows the ``write_sidecar=True`` kwarg; the sidecar section documents the on-by-default semantics with the ``<project_dir>/.signalforge/diff.json`` path and the ``write_sidecar=False`` opt-out. 5. .claude/rules/diff-renderer.md — sidecar default semantics. The rule prose now reflects the post-Q1 default-on shape. 6. .claude/rules/diff-renderer.md — ANSI strip regex contract. The documented regex now matches the broadened ECMA-48 / ISO 6429 CSI grammar (``\x1b\[[0-?]*[ -/]*[@-~]``) shipped during US-014. Tests added (3): exact-duplicate not_null, exact-duplicate accepted_values, and a cross-stage parity test that compares diff-side artifact_ids against grade engine's ``_artifact_id_for`` output for the duplicate scenario. Validation: ruff clean, ruff format clean, pyright clean, pytest 1225 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review Round 2 — Address all 6 follow-up threadsCodeRabbit re-reviewed after
New tests (3)
Validation
False positives — noneEvery thread landed a real fix. Commit: |
6 tasks
This was referenced May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Super plan for #8 — kept/dropped table + unified
schema.ymldiff.Phase: detailing (awaiting approval)
Stories: 12 implementation + Quality Gate + Patterns & Memory = 14
Decisions: 21 captured (DEC-001 … DEC-021)
Reviews: security / performance / data-model+API / observability / testing — 3 blockers, 14 concerns, 38 passes; all blockers resolved into DEC-### items inside the locked design.
Plan document
plans/super/8-diff-renderer.md— full plan (1075 lines).Locked design highlights
signalforge.diffsubpackage; sits between grade (Quality grader: rubric scoring of surviving artifacts #7) and CLI (CLI: signalforge generate command, config, exit codes #9). Zero new deps (PyYAML present,difflibstdlib, raw ANSI ~30 lines, plain text fixtures).AnsiRenderer(terminal),MarkdownRenderer(GitHub-flavored, 60k char cap),JsonRenderer(sidecar). Concretes private; orchestrator selects viaconfig.render_kind.<project_dir>/.signalforge/diff.jsonas the durable record (no separate JSONL audit log — Q5 → A; this is the first stage to skip the fail-closed JSONL pattern, justified by render-is-not-a-decision).<MODEL_SQL>-style protections:_strip_ansi_escapesruns unconditionally on user content (DEC-007);_escape_markdown_scalarfor table cells (DEC-008).grade-layer.mdprecedent (DEC-002).candidate_hash,prune_result_hash,grading_report_hash) on everyDiffReport(DEC-016).Out of scope (deferred to v0.2+)
Next steps
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
diff:configuration namespace insignalforge.ymlfor rendering customization (output kind, diff truncation, size limits).signalforge/diff.jsonsidecar generation with reproducibility hashes