Enforce + backfill diagnostic-field discipline; gate spec_ref validity (#682, #738) - #826
Conversation
… mark GC-reclamation suite stress (#738) Add scripts/check_diagnostic_fields.py — an AST gate (wired into pre-commit + the CI lint job, mirroring the #597 walker-coverage gate) that fails when any Diagnostic(...) constructor or self._error/_warning call under vera/ omits rationale, fix, or spec_ref (spec §0.5.1). Exemptions are explicit and reasoned, never silently inferred (DESIGN.md §"Explicitness over convenience"): a warning needs no fix (no corrected-code template); the codegen internal / "function skipped" helpers are registry-exempt from fix/spec_ref; and a one-off defensive site carries a `# diag-fields-exempt: <reason>` marker — a dedicated token, deliberately not `# noqa:`-prefixed so it cannot collide with ruff's suppression namespace. Backfill the 54 under-tagged checker sites (calls/control/expressions/core/ resolution) plus partial direct Diagnostic() constructions in the parser-error factories, the tester, the monomorphizer, and the verifier's obligation fallback, with concrete rationale/fix/spec_ref. `vera check` on @Bool.0 + @Int.0 (E140) now emits a Fix: paragraph (test_E140_carries_a_fix_paragraph_682). #738: mark TestHostHandleReclamation573 (full GC-reclamation programs, ~minutes locally) with @pytest.mark.stress so it is deselected from the default per-PR pytest run and runs under `pytest -m stress` / nightly CI. Scope notes: error_code enforcement, plus a batch of pre-existing spec_ref section/title drift the build-out's adversarial audit surfaced (30 sites, none introduced here), are tracked as a follow-up; the interim CodeRabbit "full metadata" advisory check is narrowed to the residual error_code field. Not a release (no version bump); CHANGELOG [Unreleased] documents the change. Co-Authored-By: Claude <noreply@anthropic.invalid>
…f validity (#682) Fold-in to the diagnostic-field discipline work. The adversarial audit behind the gate found 30-plus diagnostics whose spec_ref cited the wrong section or title — all predate this branch (e.g. every arithmetic/comparison/logical operator error cited §4.3 "Operators", but §4.3 is "Slot References"; arithmetic is §4.4, comparison §4.5, logical §4.6). A present-but-wrong spec_ref is a misleading instruction — worse than a missing one — so: - Corrected every wrong spec_ref under vera/ (checker, parser-error factories in errors.py, codegen, verifier) to resolve to a real spec section/chapter with a matching title. - Documented typed holes (?) in a new spec/04-expressions.md §4.17 "Typed Holes" — they had NO spec section at all, so W001 and the codegen E614 cited a fabricated §3.10. Both diagnostics now cite §4.17. - Extended scripts/check_diagnostic_fields.py to enforce spec_ref VALIDITY: every cited section/chapter must exist with a matching title. Title match is lenient (case, backticks, parentheticals ignored) so a cosmetic spec re-title doesn't break the gate, while a wrong section (right number, wrong rule) still fails. New TestSpecRefValidity covers it; the live-tree test now asserts both presence and validity. - Synced the E001 canonical example's See-line (now §5.4 "Contract Clauses") across README.md, docs/index.html, and spec/00-introduction.md. Full floor green: pytest (5437 passed), mypy, ruff, conformance, examples, and every scripts/check_*.py. CHANGELOG [Unreleased] updated; no version bump. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a diagnostic-field checker for ChangesDiagnostic field enforcement and backfill
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
==========================================
- Coverage 92.05% 92.05% -0.01%
==========================================
Files 89 89
Lines 27299 27304 +5
Branches 324 324
==========================================
+ Hits 25131 25134 +3
- Misses 2160 2162 +2
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Stage the release machinery so v0.0.188 can be tagged + published right after this PR merges (everything except the tag and `gh release`). - Version bumped 0.0.187 -> 0.0.188 across pyproject.toml, vera/__init__.py, README.md, docs/index.html, and uv.lock (check_version_sync clean across all six sync points; `uv lock --check` clean). - CHANGELOG: [Unreleased] promoted to [0.0.188] - 2026-06-30 with a fresh empty [Unreleased] and the compare link reference. - HISTORY: opens Stage 16 "The safety net" (the soundness campaign wrapped at v0.0.187) with the v0.0.188 row. - Regenerated site assets (docs/llms.txt, llms-full.txt, index.md, index.html). The release bundles the diagnostic-field discipline gate + spec_ref validity + backfill (#682), the GC-reclamation stress marking (#738), and the earlier CodeRabbit Pro+ config (#825) — all previously sitting in [Unreleased]. Co-Authored-By: Claude <noreply@anthropic.invalid>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 23: Update the CHANGELOG entry to match the live CodeRabbit
configuration: the advisory check in .coderabbit.yaml was not retired, it was
narrowed. Reword the sentence around the `#682` diagnostic-metadata gate so it
says the metadata fields were deterministically gated and the interim advisory
check was narrowed to “Diagnostics carry an error code,” rather than claiming it
was retired.
In `@CONTRIBUTING.md`:
- Around line 92-94: The commit-stage hook count is stale and conflicts with the
updated total in the previous sentence. Update the count in the CONTRIBUTING.md
section that introduces the “commit-stage” hooks so it matches the current
number of hooks described by the surrounding text, keeping the wording around
the hook list consistent with the updated 28 commit-stage / 2 push-stage
breakdown.
In `@scripts/check_diagnostic_fields.py`:
- Around line 197-199: The exemption logic in `check_diagnostic_fields` is too
broad because `STRUCTURAL_EXEMPTIONS` is applied for every `(fam, method)` call
site, which lets unrelated codegen diagnostics skip required fields. Tighten the
bypass in the `method is not None` block by keying it to the specific diagnostic
class or error code (for example via a dedicated helper) so only the documented
E699/“function skipped” cases can omit `fix` and `spec_ref`.
- Around line 175-177: The severity extraction in the Diagnostic call handling
is too narrow: in the severity parsing logic that uses sev_kw and severity,
non-literal values like severity=level currently fall back to "error" and can
wrongly trigger fix checks. Update the Diagnostic(...) analysis to explicitly
handle non-constant severity expressions instead of silently defaulting them to
error semantics, or emit a specific violation when the severity cannot be
resolved statically.
In `@TESTING.md`:
- Line 9: The skipped-test counts are inconsistent between the overview table
and the “Skipped tests” section in TESTING.md. Update the TESTING.md content
around the skipped-tests summary so the overview count and the §Skipped tests
prose/tables match: either revert the overview to 20, or add the two new skipped
tests to the existing tables and change the line-143 summary to 22. Use the
existing “Skipped tests” section and its two tables as the source of truth when
reconciling the counts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63add640-c8f7-4786-915b-7a0d99c41ade
⛔ Files ignored due to path filters (1)
docs/index.htmlis excluded by!docs/**
📒 Files selected for processing (29)
.coderabbit.yaml.github/workflows/ci.yml.pre-commit-config.yamlCHANGELOG.mdCLAUDE.mdCONTRIBUTING.mdREADME.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pyspec/00-introduction.mdspec/04-expressions.mdtests/test_checker.pytests/test_codegen.pytests/test_diagnostic_fields.pyvera/checker/calls.pyvera/checker/control.pyvera/checker/core.pyvera/checker/expressions.pyvera/checker/registration.pyvera/checker/resolution.pyvera/codegen/compilability.pyvera/codegen/core.pyvera/codegen/modules.pyvera/codegen/monomorphize.pyvera/errors.pyvera/tester.pyvera/transform.pyvera/verifier.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
Per review: v0.0.188 continues the same no-silent-failures campaign (Tier 0 -> Tier 1), not a new theme. #682's spec_ref-validity gate closes a silent-failure class in the diagnostics layer, #738 is a Tier 1 safety-net item, and Stage 15 already carries non-verifier rows (the --json registries, ch8 conformance). So the v0.0.188 row stays in Stage 15. Co-Authored-By: Claude <noreply@anthropic.invalid>
- CHANGELOG: the interim CodeRabbit check was NARROWED (to "Diagnostics carry an error code"), not retired — reword to match the live .coderabbit.yaml. - CONTRIBUTING: commit-stage hook count 27 -> 28 (stale vs the 30 total / 28 commit / 2 push breakdown in the preceding sentence). - check_diagnostic_fields.py: a non-literal `severity=` on a direct Diagnostic(...) is now flagged rather than silently defaulting to "error" (which would wrongly demand a `fix` of a possibly-warning diagnostic). New test; the live tree has no such site so the gate stays clean. - TESTING.md: the §Skipped-tests tables were missing ch09_builtin_redefinition (the E151 negative from #815, x2 stages) — added the two rows and bumped the line-143 summary 20 -> 22 to match the overview's live count. Not changed (CodeRabbit STRUCTURAL_EXEMPTIONS finding): the exemption is keyed to the codegen _error/_warning helpers, which have no fix/spec_ref parameters, so every call through them physically cannot carry those fields (non-codegen (fam,method) get the empty-set default). Keying on E699 would wrongly flag the legitimate E604/E605 "function skipped" warnings that also route through the param-less helper. Counts resync'd (now 5,486 tests); site assets regenerated. Co-Authored-By: Claude <noreply@anthropic.invalid>
Co-Authored-By: Claude <noreply@anthropic.invalid>
A four-dimension adversarial review of the PR (gate soundness, test mutation, content accuracy, release docs) surfaced these — each independently verified: - GATE SOUNDNESS (medium): the per-call opt-out was matched by a raw line scan, so a diagnostic whose description merely CONTAINED the text "# diag-fields-exempt: ..." was silently exempted from all required fields. Detection is now comment-aware via tokenize (only a real COMMENT token counts). New test pins it. - CONTENT (medium): vera/codegen/modules.py's undefined-call backstop carried a factually-wrong opt-out rationale (claimed the checker's E130 shadows it / it is unreachable — but the checker only WARNS, E200, so a check-clean program reaches this fatal codegen error). Removed the exemption and gave it a real fix + spec_ref (Chapter 8, Section 8.5.1 "Bare Calls", mirroring E200), so it is now a fully-tagged user-facing error. - TEST RIGOR (low x3): mutation testing showed three spec_ref-validity branches were green-for-wrong-reason — added negative tests for section-under-wrong- chapter, unrecognised-format, and chapter-only-wrong-title. - CONTENT (low x2): tester.py E700 rationale broadened (a trial failure is an ensures violation OR a runtime trap); transform.py opt-out comment corrected to E009/E010 (the factory does not emit E008). Release/docs and the §4.17 typed-hole section were verified clean. Gate clean (presence + spec_ref validity), 28 gate tests pass, mypy + ruff clean, test_codegen_modules green. Counts resync'd (5,490 tests). Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 147-149: The opt-out check in check_diagnostic_fields.py is too
broad because OPT_OUT is matched as a substring in tok.string, so unrelated
comments can accidentally disable the gate. Update the comment parsing in the
token loop to only honor an explicitly anchored directive using the tok.string
value from tokenize.COMMENT, and keep the rest extraction in the same path so
only the exact opt-out marker is accepted.
In `@vera/codegen/modules.py`:
- Around line 568-585: The diagnostic in the unresolved-call path should be
split based on the qualified flag inside the code generation logic that builds
the Diagnostic in vera/codegen/modules.py. When qualified is true, update the
fix text to refer to the imported module symbol and avoid telling the user to
define the fully qualified name locally, and point spec_ref to the
qualified-call section instead of Chapter 8, Section 8.5.1 "Bare Calls"; keep
the current bare-call wording and reference only for unqualified calls. Use the
existing conditional around display/name in the diagnostic construction to
choose the correct fix/spec_ref.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e037aed5-870b-4805-8589-27c87bb891f1
📒 Files selected for processing (8)
README.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.pyvera/codegen/modules.pyvera/tester.pyvera/transform.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 147-149: The opt-out check in check_diagnostic_fields.py is too
broad because OPT_OUT is matched as a substring in tok.string, so unrelated
comments can accidentally disable the gate. Update the comment parsing in the
token loop to only honor an explicitly anchored directive using the tok.string
value from tokenize.COMMENT, and keep the rest extraction in the same path so
only the exact opt-out marker is accepted.
In `@vera/codegen/modules.py`:
- Around line 568-585: The diagnostic in the unresolved-call path should be
split based on the qualified flag inside the code generation logic that builds
the Diagnostic in vera/codegen/modules.py. When qualified is true, update the
fix text to refer to the imported module symbol and avoid telling the user to
define the fully qualified name locally, and point spec_ref to the
qualified-call section instead of Chapter 8, Section 8.5.1 "Bare Calls"; keep
the current bare-call wording and reference only for unqualified calls. Use the
existing conditional around display/name in the diagnostic construction to
choose the correct fix/spec_ref.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e037aed5-870b-4805-8589-27c87bb891f1
📒 Files selected for processing (8)
README.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.pyvera/codegen/modules.pyvera/tester.pyvera/transform.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
🛑 Comments failed to post (2)
scripts/check_diagnostic_fields.py (1)
147-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor the opt-out directive instead of substring-matching it.
if OPT_OUT in tok.stringexempts any comment that merely mentions the token, so comments like# not-diag-fields-exemptor explanatory prose can suppress the gate by accident. That turns this into a false-negative path.Suggested fix
- if tok.type == tokenize.COMMENT and OPT_OUT in tok.string: - rest = tok.string.split(OPT_OUT, 1)[1] + if tok.type == tokenize.COMMENT: + match = re.match( + rf"\s*#\s*{re.escape(OPT_OUT)}(.*)$", + tok.string, + ) + if not match: + continue + rest = match.group(1) out[tok.start[0]] = rest.lstrip(" :").strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if tok.type == tokenize.COMMENT: match = re.match( rf"\s*#\s*{re.escape(OPT_OUT)}(.*)$", tok.string, ) if not match: continue rest = match.group(1) out[tok.start[0]] = rest.lstrip(" :").strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_diagnostic_fields.py` around lines 147 - 149, The opt-out check in check_diagnostic_fields.py is too broad because OPT_OUT is matched as a substring in tok.string, so unrelated comments can accidentally disable the gate. Update the comment parsing in the token loop to only honor an explicitly anchored directive using the tok.string value from tokenize.COMMENT, and keep the rest extraction in the same path so only the exact opt-out marker is accepted.vera/codegen/modules.py (1)
568-585: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Split the qualified-call branch from the bare-call diagnostic.
When
qualifiedis set, this now tells the user to define'm::f'locally and cites"Bare Calls". That guidance only fits baref(...)calls; form::f(...)it is impossible or misleading. Please branch the fix/spec reference onqualified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vera/codegen/modules.py` around lines 568 - 585, The diagnostic in the unresolved-call path should be split based on the qualified flag inside the code generation logic that builds the Diagnostic in vera/codegen/modules.py. When qualified is true, update the fix text to refer to the imported module symbol and avoid telling the user to define the fully qualified name locally, and point spec_ref to the qualified-call section instead of Chapter 8, Section 8.5.1 "Bare Calls"; keep the current bare-call wording and reference only for unqualified calls. Use the existing conditional around display/name in the diagnostic construction to choose the correct fix/spec_ref.
…agnostic (#682) Two CodeRabbit findings (inline posting failed on GitHub's side; addressed here): - scripts/check_diagnostic_fields.py: the per-call opt-out was matched as a SUBSTRING of the comment token, so a near-miss (`# diag-fields-exempt-foo`) or a mid-comment mention could silently disable the gate. It is now an ANCHORED directive — the comment must BE the marker, or the marker immediately followed by ':' or whitespace. New test (`test_unanchored_marker_does_not_exempt`). - vera/codegen/modules.py: the undefined-call backstop emitted one fix/spec_ref for both call shapes. Split on the `qualified` flag — a module-qualified call (`m::f`) now gets import-oriented fix text + spec_ref §8.5.3 "Module-Qualified Calls"; a bare call keeps the define-or-import guidance + §8.5.1 "Bare Calls". Gate clean (presence + spec_ref validity), 29 gate tests + test_codegen_modules pass, mypy + ruff clean, counts resync'd (5,491 tests). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Both inline findings (which failed to post inline) are addressed in f53b73c — verified valid against current code, fixed, validated: 1. Opt-out matched as a substring ( 2. Qualified-vs-bare undefined-call diagnostic ( Gate clean (presence + spec_ref validity), 29 gate tests + |
|
Tip For best results, initiate chat on the files or code changes.
The anchored ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
TESTING.md (1)
552-552: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese hook descriptions overstate the gate.
check_diagnostic_fields.pyintentionally does not requirefixon warning diagnostics, but both entries still describe the rule as unconditionalrationale/fix/spec_ref. That will send contributors looking for missing fixes on valid_warning(...)sites. Please mirror the warning exemption here as well.Also applies to: 651-651
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TESTING.md` at line 552, The hook description for check_diagnostic_fields.py overstates the requirement by implying every diagnostic must include rationale/fix/spec_ref, but warning diagnostics are exempt from needing fix. Update the relevant TESTING.md entries so they explicitly mention the warning exception, matching the actual behavior of check_diagnostic_fields.py and avoiding confusion around valid _warning(...) diagnostics.scripts/check_diagnostic_fields.py (2)
273-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCache the spec by directory, not globally.
_load_spec()memoises the firstspec_dirfor the whole process. A later call with a different directory will validate against the wrong chapter/section map and produce false passes or failures. Becausespec_ref_violations_in_source(..., spec_dir=...)already exposes directory injection, the cache needs to be keyed by the resolved path, or bypassed for non-default dirs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_diagnostic_fields.py` around lines 273 - 294, The _load_spec() cache is currently shared across all spec_dir inputs, so later calls can reuse the wrong section/chapter map. Update _load_spec() to cache per resolved spec directory (or skip caching when a non-default directory is passed) so spec_ref_violations_in_source(..., spec_dir=...) always validates against the correct files. Use the existing _spec_cache handling in _load_spec(), and make the cache key include the spec_dir identity.
325-355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-literal
spec_refstill slips past the validator.
spec_ref_violations_in_source()only checks refs yielded as literal strings, soself._error(..., spec_ref=COMMON_REF)satisfies the presence gate but bypasses the section/title resolver entirely. That undercuts the main guarantee this script is meant to enforce. Please reject non-literalspec_refvalues explicitly, the same way non-literalseverity=is now rejected for directDiagnostic(...)calls, and add a regression test for it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_diagnostic_fields.py` around lines 325 - 355, The validator in spec_ref_violations_in_source only resolves literal spec_ref strings, so non-literal values can still pass the presence check and skip chapter/section validation. Update the spec_ref handling to explicitly reject dynamic/non-literal spec_ref arguments in the same way severity is rejected for direct Diagnostic(...) calls, using the existing _iter_spec_refs and spec_ref_violations_in_source flow to flag them as violations. Add a regression test covering a call like self._error(..., spec_ref=COMMON_REF) to ensure it is reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 154-157: The opt-out parsing in check_diagnostic_fields.py only
matches a literal space after OPT_OUT, so valid anchored comments using other
whitespace like tabs are missed. Update the parsing branch around the OPT_OUT
handling to accept any whitespace by checking the first suffix character with
isspace() instead of comparing only to " ", and keep the existing stripping
behavior when populating out[tok.start[0]]. Add a regression test covering a
tab-separated opt-out comment to verify the helper accepts it.
In `@tests/test_diagnostic_fields.py`:
- Around line 232-243: The `test_unanchored_marker_does_not_exempt` coverage
only checks the suffix near-miss and misses the mid-comment anchoring
regression. Extend this test in `test_diagnostic_fields.py` by adding a second
assertion against `mod.check_source(...)` for a comment like `# note
diag-fields-exempt later`, and verify it still reports one violation with
missing fields `rationale`, `fix`, and `spec_ref`; this will lock down the
`_optout_lines()` anchoring behavior.
---
Outside diff comments:
In `@scripts/check_diagnostic_fields.py`:
- Around line 273-294: The _load_spec() cache is currently shared across all
spec_dir inputs, so later calls can reuse the wrong section/chapter map. Update
_load_spec() to cache per resolved spec directory (or skip caching when a
non-default directory is passed) so spec_ref_violations_in_source(...,
spec_dir=...) always validates against the correct files. Use the existing
_spec_cache handling in _load_spec(), and make the cache key include the
spec_dir identity.
- Around line 325-355: The validator in spec_ref_violations_in_source only
resolves literal spec_ref strings, so non-literal values can still pass the
presence check and skip chapter/section validation. Update the spec_ref handling
to explicitly reject dynamic/non-literal spec_ref arguments in the same way
severity is rejected for direct Diagnostic(...) calls, using the existing
_iter_spec_refs and spec_ref_violations_in_source flow to flag them as
violations. Add a regression test covering a call like self._error(...,
spec_ref=COMMON_REF) to ensure it is reported.
In `@TESTING.md`:
- Line 552: The hook description for check_diagnostic_fields.py overstates the
requirement by implying every diagnostic must include rationale/fix/spec_ref,
but warning diagnostics are exempt from needing fix. Update the relevant
TESTING.md entries so they explicitly mention the warning exception, matching
the actual behavior of check_diagnostic_fields.py and avoiding confusion around
valid _warning(...) diagnostics.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a9580a2-a9ab-467e-bfc5-3b28097f8707
📒 Files selected for processing (6)
README.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.pyvera/codegen/modules.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…on-literal spec_ref (#682) CodeRabbit round (2 inline + 3 outside-diff), all verified valid against current code: - scripts/check_diagnostic_fields.py opt-out boundary: accept ANY whitespace after the marker (tab, not only a literal space) via isspace(), keeping the existing reason-stripping. New load-bearing test (bare-tab boundary, no colon — exercises the whitespace path, not the `OPT_OUT + ":"` path). - test_unanchored_marker_does_not_exempt: add the mid-comment case (`# note diag-fields-exempt later`) alongside the suffix near-miss, locking down the anchoring of _optout_lines(). - _load_spec: cache per RESOLVED spec_dir (dict keyed by path), not a single shared global — a later call with a different spec_dir (e.g. a test fixture) no longer reuses another directory's section/chapter map. New test. - spec_ref validity: a NON-literal spec_ref (variable / f-string) passed the presence check but skipped validation entirely. _iter_spec_refs now also surfaces non-literal spec_refs (and gains the same plumbing-skip as check_source so the _error/_warning helper constructions aren't flagged); spec_ref_violations_in_source flags them — mirroring the non-literal-severity rule. This immediately caught codegen/modules.py:_emit_cross_module_error, whose last-round qualified/bare split made spec_ref a branched variable that was never validated; reworked to emit each branch with a LITERAL spec_ref, so the gate now validates both §8.5.1 "Bare Calls" and §8.5.3 "Module-Qualified Calls". New tests (flagged + plumbing-skipped). - TESTING.md: hook description noted the warning `fix` exception (warnings are exempt from carrying a fix). All 4 new tests mutation-validated (RED against the pre-fix behavior). Gate clean, 66 tests pass (test_diagnostic_fields + test_codegen_modules), mypy + ruff clean, counts resync'd (5,495 tests). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai All five findings (2 inline + 3 outside-diff) verified valid against current code and fixed in 04191ca — replying here since inline posting failed: 1. Opt-out boundary only matched a literal space ( 2. 3. 4. Non-literal 5. TESTING.md hook description overstated the requirement. Updated both rows to note errors carry All 4 new tests mutation-validated (each RED against the pre-fix behavior). Gate clean, 66 tests pass, mypy + ruff clean, counts resync'ed (5,495). |
|
Tip For best results, initiate chat on the files or code changes.
The fixes all line up with the underlying correctness concerns:
The mutation-tested regressions and clean validation run are exactly what I wanted to see. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TESTING.md`:
- Line 552: Update the remaining TESTING.md summaries so they reflect the newly
added check_diagnostic_fields.py gate: revise the validation-scripts intro that
still says “Twenty-one scripts” and update the CI Pipeline lint-job row to
include this check. Keep the affected summary sections consistent with the
current workflow and use the existing TESTING.md section headings and script/job
names to locate the stale text.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52c37cd7-7f6b-455b-a031-a05f3099d3ea
📒 Files selected for processing (6)
README.mdROADMAP.mdTESTING.mdscripts/check_diagnostic_fields.pytests/test_diagnostic_fields.pyvera/codegen/modules.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…STING.md summaries (#682) CodeRabbit (inline, TESTING.md:552): adding the gate to the validation-scripts table left two summaries stale — the intro count ("Twenty-one" → "Twenty-two", one validator added) and the CI Pipeline lint-job row (omitted the gate though ci.yml runs it). Both updated; gate listed in ci.yml execution order. Co-Authored-By: Claude <noreply@anthropic.invalid>
…ip test load-bearing (#682) Adversarial multi-agent review of the round-3 diff surfaced three confirmed findings; two fixed here, one deferred to #827. - spec_ref validity only checked the FIRST citation: `_REF_SEC.search(ref)` ran once, so a wrong/bogus SECOND citation in a multi-section ref (5 such refs are live in verifier.py, e.g. `§4.7 "Let Bindings" and §11.2.1 "Nat as i64"`) shipped silently — the exact misleading-instruction failure the gate exists to prevent. Now iterates `finditer` over EVERY citation, and rejects stray non-citation text around the citations (a `LIES ` prefix that `.search()` had tolerated). Gate stays green on the live tree, confirming all 5 refs' second citations resolve. Tests: valid multi-section passes, wrong-second flagged, garbage-prefix rejected. - test_plumbing_non_literal_spec_ref_skipped was green-before-AND-after: the helper-skip and the non-literal-flagging shipped together and masked each other, so the test never went RED against the real prior baseline. Reworked to assert on a LITERAL-but-invalid spec_ref inside the helper, so it goes RED via the literal validity path whenever the skip is removed — independent of the non-literal code. Renamed test_plumbing_spec_ref_skipped; keeps the threaded case as a second assertion. All three new/changed tests mutation-validated (RED against the pre-fix gate). The third finding — the name-based plumbing-skip is broader than the helper's own construction (a stray/second Diagnostic in a helper escapes both passes; latent, no live escape) — is deferred to #827 (decision-bearing fix, separable). Gate clean, 36 tests pass, mypy + ruff clean, counts resync'd (5,498). Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Adversarial multi-agent review of the gate diff (5 lenses × default-refute verification) — recorded for the trail. 8 candidate findings raised, 3 confirmed real (all high-confidence, severity minor), 5 refuted. Acted on in 721b5cb:
All 3 new/changed tests mutation-validated (RED against the pre-fix gate). |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vera/checker/calls.py`:
- Around line 618-630: Update the E220 diagnostic in calls.py where Calls._error
reports “Unresolved qualified call” so its spec_ref points to Chapter 7, Section
7.4 instead of 7.4.1. This warning is about unresolved effect-operation lookup,
so keep the existing message, rationale, and fix text, but change the spec
reference on the same _error call to the correct subsection for effect
operations.
In `@vera/checker/registration.py`:
- Line 211: The E132 spec_ref in registration should reference the alias-cycle
rule instead of the generic type-alias resolution section. Update the value used
in the registration logic around the spec_ref assignment so it points to the
rule that explicitly forbids cyclic aliases, or, if that rule does not exist
yet, add/extend the spec section to cover alias cycles and reference that from
the checker.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da916d50-d42f-4eff-9fca-6bb5b2beda4a
⛔ Files ignored due to path filters (5)
docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (32)
.coderabbit.yaml.github/workflows/ci.yml.pre-commit-config.yamlCHANGELOG.mdCLAUDE.mdCONTRIBUTING.mdHISTORY.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomlscripts/check_diagnostic_fields.pyspec/00-introduction.mdspec/04-expressions.mdtests/test_checker.pytests/test_codegen.pytests/test_diagnostic_fields.pyvera/__init__.pyvera/checker/calls.pyvera/checker/control.pyvera/checker/core.pyvera/checker/expressions.pyvera/checker/registration.pyvera/checker/resolution.pyvera/codegen/compilability.pyvera/codegen/core.pyvera/codegen/modules.pyvera/codegen/monomorphize.pyvera/errors.pyvera/tester.pyvera/transform.pyvera/verifier.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
#827 is a latent, minor refinement to a CI lint gate (no live escape, developer-facing), not runtime/user-facing safety-net work — so it belongs in Tier 3 (polish), not Tier 1. Co-Authored-By: Claude <noreply@anthropic.invalid>
…ly-correct sections (#682) Two CodeRabbit findings — both cited a real section (so the validity gate passed them) that is semantically wrong for the error: - E220 "Unresolved qualified call" fires when `Effect.op` resolves to NO effect operation, but cited §7.4.1 "Ambiguous Operations" (the multi-MATCH case). Repointed to §7.4 "Performing Effects", which states the resolution rule the unresolved call violates. - E132 (type-alias cycle) cited §3.8 "Type Alias and Reference Resolution" — which is actually the slot-NAMESPACE rule (`@PosInt.0` vs `@Int.0`), unrelated to cycles. The rule E132 enforces ("an alias must resolve to a concrete type; the alias chain must be acyclic") had no spec home, so a sentence stating it was added to spec/02-types.md §2.6.3 "Type Aliases with Refinements", which E132 now cites. Gate clean (both refs resolve), spec examples pass, 622 checker/errors tests pass, E132/E220 confirmed firing with the corrected refs via `vera check --json`. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai full review |
… the gate (#682, #828) Closes the cheap deterministic half of the error_code-collision gap surfaced this PR. check_diagnostic_fields.py now verifies every LITERAL error_code on a diagnostic under vera/ is registered in vera/errors.py ERROR_CODES (AST-extracted from the registry), catching typos / unregistered codes. Plumbing (helper-def Diagnostic constructions) and non-literal (threaded) codes are skipped — they can't be checked statically. This does NOT detect collisions (one registered code reused for two unrelated concepts) — the four found this PR were all *registered*, just reused. The semantic collision-detection gate + making ERROR_CODES the single home of each code's concept (the proper fix) + error_code presence enforcement are tracked in #828 (ROADMAP Tier 2). +5 tests (unregistered flagged, registered passes, non-literal skipped, plumbing skipped, _load_error_codes reads the live registry); the membership check is mutation-validated; the live-tree integration test now also runs the new pass. Gate clean, mypy + ruff clean, counts synced (5,505). Co-Authored-By: Claude <noreply@anthropic.invalid>
CodeRabbit (errors.py:208 review of 26d13aa): AGENTS.md's embedded E001 example JSON cited 'Chapter 5, Section 5.1 "Function Structure"' — doubly stale (§5.1 is "Overview"; "Function Structure" is not a real title), and now inconsistent with the repointed E001 diagnostic. Updated to 'Chapter 5, Section 5.2 "Function Declaration Syntax"', matching errors.py + the README/docs/spec mirrors. (This is a 5th E001 mirror not covered by TestErrorDisplaySync, which guards only README/docs/index.html/spec/00 — noted in #828.) Co-Authored-By: Claude <noreply@anthropic.invalid>
…682) Root of the docs/index.md drift CodeRabbit flagged: scripts/build_site.py hard- codes the E001 example with 'See: Chapter 5, Section 5.1 "Function Structure"' (stale — §5.1 is "Overview"). Updated to §5.2 "Function Declaration Syntax" and regenerated docs/. This was the 6th and last E001 spec_ref mirror; all now agree with errors.py. (Only README/docs/index.html/spec/00 are guarded by TestErrorDisplaySync; AGENTS.md and this generator were not — noted in #828.) Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…-filtering in the spec-example harness (#682) CodeRabbit (spec/04-expressions.md §4.17): the Typed Holes section introduced `?` with no code example. Added a canonical block — `@Int.0 + ?` in a real function — showing the hole is inferred to type @int, type-checks with W001, and is rejected at compile with E614. Making that example pass check_spec_examples.py surfaced a harness bug: its check AND verify stages treated warning-severity diagnostics as failures (`if errors:` on the typecheck result), unlike the CLI `vera check` (exits 0 on warnings) and unlike the verify stage's own error-only filter one line later. Both stages now filter to error severity, so a legitimately-checkable warning-emitting example is validated rather than mis-failed. No ripple — the baseline had no warning-emitting spec blocks, so the §4.17 hole example is the sole (and self-testing) case. check_spec_examples clean (89 checked, 76 check-OK, 75 verify-OK, 0 failures), mypy + ruff + changelog + doc-count + site-asset gates pass. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai The outside-diff §4.17 Typed Holes finding ( |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- spec/04-expressions.md typed holes section ---"
rg -n "4.17|Typed Holes|`@Int.0` \+ \?|W001|E614" spec/04-expressions.md
echo "--- check_spec_examples.py severity filters ---"
rg -n "severity" scripts/check_spec_examples.pyLength of output: 1148
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/04-expressions.md`:
- Around line 433-441: The fenced example in the spec is missing a language tag,
so update the code block in the documented expression example to use the
established Vera syntax highlighting tag. Locate the example around the scale
function snippet in the expressions spec and change the opening fence to the
appropriate language identifier, keeping the rest of the example unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46bb8c51-b279-4acb-bca6-612b45ed7ce1
📒 Files selected for processing (3)
CHANGELOG.mdscripts/check_spec_examples.pyspec/04-expressions.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
CodeRabbit re-raised the missing language tag. Reversing my earlier decline: verified 'vera' is not in check_spec_examples.py's skip_langs, so the tagged block stays fully validated (parse + check + verify — confirmed lang=vera, is_parseable=True, check=OK), and vera-tagged fences are an established spec convention (spec/09). Tagging satisfies MD040 + syntax highlighting with no loss of validation. check_spec_examples clean (Check OK 76, 0 failures). Co-Authored-By: Claude <noreply@anthropic.invalid>
1 participant
Summary
Enforces and backfills diagnostic-field discipline (#682), folds in the GC-reclamation stress marking (#738), and — folded in during the build-out's adversarial audit — corrects every pre-existing wrong
spec_refundervera/and adds spec_ref-validity to the gate.Release-staged as v0.0.188. Version bumped across all six sync points, CHANGELOG
[Unreleased]promoted to[0.0.188]with its compare link, and HISTORY opens Stage 16 "The safety net" — so the only post-merge steps are the tag +gh release. The release bundles #682, #738, and the earlier CodeRabbit Pro+ config (#825), all previously sitting in[Unreleased].The gate —
scripts/check_diagnostic_fields.pyNew deterministic gate (pre-commit + CI
lint, mirroring the #597 walker-coverage gate). It AST-parses everyDiagnostic(...)constructor andself._error(...)/self._warning(...)call undervera/and fails when:rationale,fix, orspec_ref(spec §0.5.1), orspec_refis present but wrong — it must resolve to a real spec section/chapter with a matching title (e.g. citing §4.3 "Operators" when §4.3 is actually "Slot References").The exemption surface is explicit and reasoned, never silently inferred (grounded in DESIGN.md §"Explicitness over convenience" — see the design discussion):
warningis not required to carry afix(no corrected-code template);_error/_warninghelpers (internal-compilerE699/ "function skipped" diagnostics with no user fix or spec section) are exempt via a documented registry in the script;# diag-fields-exempt: <reason>marker — a dedicated token, deliberately not# noqa:-prefixed so it cannot collide with ruff's suppression namespace.Validity title-matching is lenient (case, backticks, parentheticals ignored), so a cosmetic spec re-title doesn't break the gate while a wrong section still fails.
What changed
calls/control/expressions/core/resolution) + partial directDiagnostic(...)in the parser-error factories, tester, monomorphizer, and verifier fallback — each now has a concreterationale,fix, andspec_ref. The issue's canonical example,vera checkon@Bool.0 + @Int.0(E140), now emits aFix:paragraph (AC5 — pinned by a test).spec_refcitations (all predate this branch) so everyspec_refundervera/resolves. The wrong refs were surfaced by an adversarial verify pass and confirmed by a deterministic spec-heading validator.?) in a newspec/04-expressions.md§4.17 "Typed Holes" — they had no spec section, soW001and codegenE614cited a fabricated §3.10. Both now cite §4.17.TestHostHandleReclamation573GC-reclamation suitestress, deselecting it from the default per-PRpytestrun (it runs full reclamation programs at scale, ~minutes locally).error_codefield now that the deterministic gate covers the rest (closing the loop flagged on Enforce diagnostic-tagging discipline: pre-commit hook + backfill of 76 partial sites #682).Scope notes (deliberately deferred)
error_codeenforcement is not yet in the gate (it's near-universal already; a handful of internal sites + theerror_code/registry-name mismatches the audit noticed are a follow-up). The gate matches Enforce diagnostic-tagging discipline: pre-commit hook + backfill of 76 partial sites #682's AC exactly (rationale/fix/spec_ref) plus the validity check.Verification
pytest— 5437 passed, 22 skipped, 26 deselected (the now-stresssuite);mypy vera/clean;ruff check .clean.scripts/check_conformance.py,check_examples.py,check_diagnostic_fields.py,check_doc_counts.py,check_site_assets.py, and the doc-example validators all pass.tests/test_diagnostic_fields.py(23 tests) covers presence detection, the severity rule, the codegen registry, the# diag-fields-exemptopt-out, plumbing-skip, spec_ref validity, and a live-tree integration check; AC5 is pinned intests/test_checker.py.Closes #682
Closes #738
🤖 Generated with Claude Code
Summary by CodeRabbit
rationale,fix, andspec_ref, with explicit opt-out support.rationale/fixguidance and more consistentspec_reftargets and error-code mappings.