framework: error message ergonomics — physical justifications + source-line attribution (Phase 2b.1)#35
Conversation
…e-line attribution (Phase 2b.1)
Every framework exception now teaches the user *why* the rule exists and
*where* the offending wire() call lives, turning the diagnostic from
"tells you what's wrong" into "teaches you why the rule exists."
Two additions per exception class, both append-only:
- Every WirebenchError subclass declares a PHYSICAL_JUSTIFICATION
ClassVar — one or two sentences anchored to a physical-world referent.
The justification renders as a "Why: …" line in str(exception).
- Every wire() call captures its source location (skipping pydantic's
validate_call wrappers via a filename-walk) and accumulates it on the
resulting Node. Errors raised from inside wire() or Circuit._validate
surface those locations as a "Wired at: …" line. The WireRecord
format round-trips source_location through .wirebench so reconstructed
designs still attribute back to user source rather than the loader.
The shape:
ShortCircuitError: wire() has multiple drivers ('y_1', 'y_2') — short circuit
Why: Two OUT-direction ports on one net fight each other on the
copper — current sinks through the losing output stage until the
FETs overheat; one driver per shared conductor.
Wired at: hello_led.py:14
Rules are unchanged; only the messages got richer. Existing
pytest.raises(..., match='...') patterns continue to match the head of
the original message; CLI JSON extractors operate on the first line
only.
Suite: 4726 passed (95 new tests), mypy clean.
There was a problem hiding this comment.
Pull request overview
This PR enhances framework diagnostics by (1) adding per-exception “physical justifications” to error stringification and (2) attributing wiring-related errors back to the originating wire() call sites, including round-tripping that attribution through the .wirebench format.
Changes:
- Extend
WirebenchErrorrendering to appendWhy:(per-classPHYSICAL_JUSTIFICATION) and optionalWired at:source-line attribution. - Capture and accumulate
wire()source locations onNodes; thread those locations intowire()-time andCircuit._validateerrors. - Persist a
source_locationthrough.wirebenchsave/load and adjust CLI regex extractors to match only the first line.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/framework/errors.py |
Adds PHYSICAL_JUSTIFICATION, optional source_locations, and enhanced __str__ rendering. |
src/framework/node.py |
Stores and exposes immutable source_locations accumulated across wire() calls. |
src/framework/wire.py |
Captures caller source location (with pydantic-wrapper skipping) and attaches it to nodes/errors; allows loader override. |
src/framework/circuit.py |
Collects net source locations and includes them in validation-time wiring errors; includes them for orphan-wire detection. |
src/framework/format.py |
Saves first node source location into WireRecord and restores it via wire(..., source_location=...). |
src/framework/format_records.py |
Adds WireRecord.source_location with serializer omission when absent. |
src/cli/validate_extractors.py |
Matches extractor regexes against the message head-line to avoid new suffix lines breaking parsing. |
tests/framework/test_wire_source_location.py |
Verifies capture, accumulation, explicit override, and immutability of node source locations. |
tests/framework/test_source_location_roundtrip.py |
Verifies .wirebench round-trip behavior and omission when absent. |
tests/framework/test_error_with_source_location.py |
Verifies wiring/validation errors carry and render source attribution. |
tests/framework/test_error_messages.py |
Verifies justifications exist and appear in str(e), and that source-locations render when provided. |
tests/framework/test_dynamically_driven.py |
Adjusts assertion to avoid false-positive substring match due to filenames in source_location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import sys | ||
| import warnings | ||
| from types import FrameType |
| frame: 'FrameType | None' = sys._getframe(1) # skip wire() itself | ||
| while frame is not None: | ||
| filename = frame.f_code.co_filename | ||
| if 'pydantic' not in filename and filename != __file__: | ||
| return (filename, frame.f_lineno) | ||
| frame = frame.f_back |
Reviewer flagged that the loader passing `source_location=None` into the public `wire()` triggered auto-capture pointing at format.py frames — fabricating attribution to framework internals and corrupting legacy files on re-save. Fix: - Split `wire()` into a public auto-capturing API and an internal `_wire_with_attribution(ports, *, dynamically_driven, source_location)` that takes attribution as-is. `None` now genuinely means *no attribution* — the node carries no source_locations entry and the emitted .wirebench keeps the field omitted. - The .wirebench loader uses `_wire_with_attribution` directly so legacy records (where `source_location` was absent) reconstruct unattributed instead of inheriting loader-frame attribution. Regression coverage: - `test_legacy_file_loads_without_fabricating_source_attribution` — hand-rolled pre-Phase-2b.1 JSON (no source_location field) loads with `port.node.source_locations == ()` for every reconstructed net. - `test_legacy_file_round_trips_without_injecting_source_location` — load → save of that file produces output whose wire records still omit `source_location`, so the round-trip is field-preserving. - `test_internal_helper_with_none_leaves_node_unattributed` — explicit `source_location=None` to the internal helper bypasses auto-capture even though a valid caller frame exists. Suite: 4729 passed (3 new), mypy clean.
|
Thanks — that was a real bug, fixed in 6e4574d. Decision on the open question: preservation wins. Re-saving a legacy file should never inject attribution that wasn't there originally; "we don't know where this came from" is a meaningful signal that a fabricated loader-frame line would erase. Fix: split The public Regression tests added in
Suite: 4729 passed, mypy clean. |
Two inline review concerns on `_capture_user_call_site`:
- `sys._getframe(1)` is documented to raise `ValueError` when the
requested depth isn't available. Vanishingly rare from inside a
real function, but guarding it costs nothing and makes the
helper robust to bizarre invocation contexts.
- `frame.f_code.co_filename` is typically an absolute path; persisting
that into `.wirebench` saved on one machine and loaded on another
would carry someone else's `/Users/...`/`/home/...`/`C:\\...` paths
through the file. Normalise to `os.path.basename` at capture time
so attribution stays portable. Aligns with the spec's own example
rendering (`Wired at: hello_led.py:14`, not a full path).
Added `test_captured_filename_is_basename_for_portability` pinning the
basename behavior; existing `.endswith('filename.py')` assertions
already match basenames so other tests stay green.
Suite: 4730 passed (1 new), mypy clean.
|
Inline comments addressed in e7ee507:
Added Suite: 4730 passed (1 new), mypy clean. |
Summary
First task of .plans/phase-2b-spec.md — keep the rules, sharpen the diagnostics. Every framework exception now teaches the user why the rule exists and where the offending
wire()call lives.WirebenchErrorsubclass declares aPHYSICAL_JUSTIFICATIONClassVar; rendered asWhy: …instr(exception).wire()call captures its source-line (skipping pydantic'svalidate_callwrappers via filename-walk) and accumulates it on theNode. Errors fromwire()andCircuit._validatesurface those asWired at: ….WireRecord.source_locationround-trips through.wirebench(omitted-when-None, same shape asdynamically_driven) so reconstructed designs attribute back to user source rather than to the loader.New shape:
Rules are unchanged; only the messages got richer.
Files
src/framework/errors.py—PHYSICAL_JUSTIFICATIONon every class;__str__rendersWhy:/Wired at:.src/framework/node.py—_source_locationsslot + read-onlysource_locationsproperty.src/framework/wire.py— stack-walk capture, accepts explicitsource_location=for the loader.src/framework/circuit.py—_validatethreads net source-locations intoShortCircuitError/FloatingNetError; orphan-port detector likewise.src/framework/format_records.py+src/framework/format.py— round-trip the new field.src/cli/validate_extractors.py— regex matchers now operate on the first line so the new suffix doesn't break CLI JSON output.Test plan
uv run pytest— 4726 passed (95 new), 16 skippeduv run mypy src/ demos/— cleanpytest.raises(..., match='...')patterns continue to match (verified by full suite green)wire()short-circuit yields the new three-line shape with correct user-source line