feat: vera/addEffect call-graph effect propagation (#222 Phase F3)#723
Merged
Conversation
The multi-site workflow completing the skill layer: {uri, fn, effect}
computes the transitive-caller closure over the Phase B call walker,
rewrites every affected effects(...) clause by span (pure -> <E>;
<A> -> <A, E> appending after the original source verbatim; functions
already naming the effect skipped, identity = base name before type
args), and runs ONE candidate through the proposeEdit pipeline. The
response lists rewritten functions in declaration order; an
already-satisfied row state short-circuits to the documented no-op
shape. Handler-unaware by design; single-file by design.
13 new tests: closure goldens (diamond order, leaf, unknown, recursion
once), rewrite goldens (pure->singleton, source-preserving append,
already-present, base-name identity), diamond propagation with
bystander untouched, mixed append/replace, the no-op shape, refusal
paths. Release prep for v0.0.168; ROADMAP Phase F row removed;
HISTORY rows for v0.0.166-168.
Co-Authored-By: Claude <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhase F3 vera/addEffect is a multi-site effect-propagation workflow for the LSP server. It computes transitive callers via call-graph closure, rewrites affected functions' effect rows using idempotent span-based splicing, and applies edits through the proposeEdit gate. This release includes comprehensive test coverage and updates all project metadata to 0.0.168. ChangesPhase F3 addEffect workflow implementation and release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #723 +/- ##
==========================================
- Coverage 90.93% 90.90% -0.03%
==========================================
Files 70 70
Lines 24772 24854 +82
Branches 292 292
==========================================
+ Hits 22526 22594 +68
- Misses 2239 2253 +14
Partials 7 7
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
tests/test_lsp.py (1)
1093-1145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd handler-level tests for
vera/addEffectboundary behaviour.Line 1097 onwards validates the workflow directly, but the new protocol path in
vera/lsp/server.py(Line 262-280) is not exercised. Please add in-process handler tests for method registration, blank-effect refusal, andValueError→JsonRpcInvalidParamsmapping.Suggested test shape
+class TestAddEffectHandler: + def test_blank_effect_refused_as_invalid_params(self) -> None: + from vera.lsp.server import create_server + server = create_server() + protocol = server.protocol + fm = protocol.fm if hasattr(protocol, "fm") else server.feature_manager + handler = fm.features["vera/addEffect"] + with pytest.raises(JsonRpcInvalidParams, match="non-empty effect"): + handler({"uri": URI, "fn": "f", "effect": " "}) + + def test_workflow_valueerror_mapped_to_invalid_params(self) -> None: + from vera.lsp.server import create_server + server = create_server() + protocol = server.protocol + fm = protocol.fm if hasattr(protocol, "fm") else server.feature_manager + handler = fm.features["vera/addEffect"] + with pytest.raises(JsonRpcInvalidParams): + handler({"uri": URI, "fn": "ghost", "effect": "Async"})As per coding guidelines,
tests/**/*.pyreviews should flag missing edge cases for new compiler features.🤖 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 `@tests/test_lsp.py` around lines 1093 - 1145, Add in-process tests that exercise the LSP protocol path for the "vera/addEffect" handler (not just the helper add_effect()); instantiate the real LSP server used in vera/lsp/server.py (via your existing _server helper), then invoke the registered method entrypoint for "vera/addEffect" (call it through the server's method-dispatch API or directly via the server's registered handler map) to assert three behaviors: the method is actually registered, a blank/empty effect parameter is rejected with a JSON-RPC invalid-params response, and any ValueError thrown by the underlying logic is translated into JsonRpcInvalidParams; reuse symbols from this test file (URI, _server, _FakeServer, add_effect) to set up requests and to compare outcomes.Source: Coding guidelines
🤖 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 `@HISTORY.md`:
- Line 315: Update the v0.0.168 HISTORY entry to explicitly mention that the
workflow rewrites effects(...) clauses as part of Phase F3: locate the entry for
"v0.0.168" and amend the sentence about transitive-caller closure and the single
verified multi-site edit to include wording like "and rewrites effects(...)
clauses" (or equivalent) so the line states closure + a single verified
multi-site edit that rewrites effects(...), matching the CHANGELOG/release
contract.
In `@README.md`:
- Line 207: The README release count ("Vera is in **active development** at
v0.0.168 — ... 168 releases") is out of sync with the HISTORY.md footer (which
reports 165 tagged releases); update the mismatched value so both locations
match exactly: either correct the README phrase "168 releases" to the exact
tagged-release total from HISTORY.md or update HISTORY.md's footer to the
correct total after verifying tag count (use git tag --list to confirm), and
ensure both the README string and the HISTORY.md footer are identical.
---
Outside diff comments:
In `@tests/test_lsp.py`:
- Around line 1093-1145: Add in-process tests that exercise the LSP protocol
path for the "vera/addEffect" handler (not just the helper add_effect());
instantiate the real LSP server used in vera/lsp/server.py (via your existing
_server helper), then invoke the registered method entrypoint for
"vera/addEffect" (call it through the server's method-dispatch API or directly
via the server's registered handler map) to assert three behaviors: the method
is actually registered, a blank/empty effect parameter is rejected with a
JSON-RPC invalid-params response, and any ValueError thrown by the underlying
logic is translated into JsonRpcInvalidParams; reuse symbols from this test file
(URI, _server, _FakeServer, add_effect) to set up requests and to compare
outcomes.
🪄 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
Run ID: 714a12a4-07f9-412d-aca1-edef1abf3447
⛔ 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 (10)
CHANGELOG.mdHISTORY.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_lsp.pyvera/__init__.pyvera/lsp/server.pyvera/lsp/workflows.py
…count footer (#723 round 1) The HISTORY footer roll-up (165 tagged releases) had not tracked the F1/F2/F3 README bumps; now 168 in both locations. Skip-changelog: docs-only amendments to rows added in this same unreleased PR
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
1 participant
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
Phase F3 of the issue 222 skill layer —
vera/addEffect, the multi-site workflow that completes the arc.Per the Phase F design comment:
{uri, fn, effect}computes the transitive-caller closure by inverting the Phase B call walker (direct_callee_names): adding an effect to a function invalidates every transitive caller's row, so the whole closure rewrites in one candidate through the proposeEdit pipeline — all-or-nothing, never a half-propagated document.PureEffect.spancovers exactlypure;EffectSet.spanincludes the brackets):effects(pure)→effects(<E>);effects(<A>)→effects(<A, E>)appending after the original source verbatim; functions already naming the effect are skipped. Effect identity is the base name before type arguments, soState<Int>is not added next toState<Bool>.rewritten(declaration order). A row state already satisfied everywhere short-circuits to the documented no-op shape (applied: false, ok: true, proof_delta: null, rewritten: []) without touching the verifier.handle[E]is still rewritten — bounding at handlers is noted future work); single-file (module-qualified calls never propagate across the file boundary); declared-but-unused effects verified legal empirically, so add-then-use agent ordering works.Tests (13 new; 4,312 total)
None, recursion appears once.<IO, State<Int>>→<IO, State<Int>, Async>), already-presentNone, base-name identity.effects(pure)untouched.Release prep (v0.0.168) + issue-closing bookkeeping
Version across all 6 tracked sites, CHANGELOG + compare links, ROADMAP Phase F row deleted, HISTORY rows for v0.0.166–v0.0.168 (Stage 12 table), TESTING/README counts, module map (
lsp/→ 1389 lines), site assets.The three skill-layer methods (
proposeEditv0.0.166,strengthenContractv0.0.167,addEffectthis PR) complete the Phase F design; with Phases A–E (v0.0.161–v0.0.165) this finishes the issue's full arc.Closes #222
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation