refactor(engine): make resolution-frame addressing a type, not a grep rule - #7489
Conversation
… rule `ResolutionStack` permits three ways to reach a frame: the top, a frame adjacent to one you already hold, and the frame a `PostReplacementFrameId` names. Anything else is a positional guess about a structural relationship the stack does not guarantee — which is the class of bug #7485 fixed, where cleanup probed the top two frames and returned `None` once the continuation had raised two. That rule was enforced by `scripts/check-resolution-frame-boundaries.sh` grepping `resolution.rs` for `frames.iter().position(..)` and `frames.remove(..)`. A grep was the only option available: `frames` was private, but Rust privacy is module-scoped and the module is 7,000 lines, so "private" bought nothing against the code sitting beside it. This replaces the rule with a type. `FrameVec` (230 lines, `types/resolution/frame_vec.rs`) owns the backing `Vec`, and every accessor takes an opaque `FrameSlot` whose field is private to that module. Slots come only from `top`, `below`, `above`, `by_id`, and the documented captured-depth door. A positional scan still compiles and can no longer be spent: it yields a `usize`, and nothing accepts one. Verified by probe rather than asserted. A method scanning for a `MultiDraw` frame and feeding the result to the accessors fails to compile with exactly the two errors the design predicts: error[E0599]: no method named `remove` found for struct `FrameVec` error[E0308]: mismatched types --- ^^^^^^^ expected `FrameSlot`, found `usize` The tree was restored byte-identical after that probe (sha256 checked both sides). What the 30 converted call sites showed is that the code already obeyed the rule; it just said so in `usize` arithmetic. Every index came from `len().checked_sub(N)` for N in 1..=3, or from the identity lookup — so the conversion reads as documentation: `let continuation = self.frames.top()?; let discard = self.frames.below(continuation)?;` in place of two `checked_sub` calls whose meaning had to be recovered by counting. Two things are deliberately NOT closed: - `slot_at_captured_depth` is the one `usize` door. An effect records `resolution_stack.len()`, runs a child producer, and hands the length back so the owner can be parked beneath the child stack that producer raised. That is not a search, and closing it means giving the captured depth its own type at roughly thirty-five origins across `game/effects/`, `game/casting_costs.rs` and their neighbours — a separate change with a far wider blast radius. The door is named so misuse reads as misuse at the call site. - `frame_at_offset` returns a frame and never a slot, because `validate` must traverse the whole stack to check a whole-stack invariant. Reading is not the hazard; addressing for mutation is. The guard keeps its legacy-wire-key rules untouched and loses the two frame-boundary scans. In their place is one structural check that the design itself is intact: `FrameSlot` must be mintable only by the five documented methods, since a sixth would reopen positional addressing with no compiler error to show for it. Both directions were watched go red out of tree — a new minting method, and a renamed existing one — with the replica green either side. Gates on the committed tree: `cargo fmt -- --check` 0, `cargo clippy --all-targets -- -D warnings` 0, `--lib` 19340 passed / 0 failed, `--test integration` 5106 passed / 0 failed.
|
Warning Review limit reached
Next review available in: 14 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review Requesting an explicit pass: the automatic one reported Worth the most scrutiny:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and changed files ==='
gh pr view 7489 --repo phase-rs/phase --json baseRefName,headRefName,files,title,url \
--jq '{title, url, baseRefName, headRefName, files: [.files[].path]}'
echo
echo '=== Changed-file summary ==='
git diff --stat HEAD^ HEAD
echo
echo '=== Relevant file discovery ==='
fd -t f -a '^(resolution|frame_vec)\.rs$|check-resolution-frame-boundaries\.sh$|Cargo\.toml$' . \
| sed -n '1,160p'
echo
echo '=== Structural map: changed Rust files ==='
while IFS= read -r file; do
case "$file" in
*.rs)
echo "--- $file"
ast-grep outline "$file" --items all --type struct,impl,method,function --view compact \
| sed -n '1,220p'
;;
esac
done < <(git diff --name-only HEAD^ HEAD | grep -E '\.rs$' || true)
echo
echo '=== Diff for resolution/frame-vector/guard files ==='
git diff --find-renames --unified=80 HEAD^ HEAD -- \
':(glob)**/resolution.rs' \
':(glob)**/frame_vec.rs' \
'scripts/check-resolution-frame-boundaries.sh'Length of output: 3173 🏁 Script executed: #!/bin/bash
set -euo pipefail
base=$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD^)
echo "=== Comparison base: $base ==="
echo
echo '=== Source maps ==='
ast-grep outline crates/engine/src/types/resolution/frame_vec.rs --items all --view compact | sed -n '1,260p'
echo '--- resolution.rs matching stack/frame methods ---'
ast-grep outline crates/engine/src/types/resolution.rs --items all --view compact \
| grep -E 'ResolutionStack|active_discard_or_direct_continuation_parent_mut|replace_change_zone_parent_at_child_boundary|take_active_post_replacement_child|frame_at_offset|slot_at_captured_depth|frames' \
| sed -n '1,300p'
echo
echo '=== FrameVec source ==='
cat -n crates/engine/src/types/resolution/frame_vec.rs
echo
echo '=== Full changed ResolutionStack conversion diff ==='
git diff --find-renames --unified=35 "$base" HEAD -- crates/engine/src/types/resolution.rs
echo
echo '=== Guard diff and final guard source ==='
git diff --unified=40 "$base" HEAD -- scripts/check-resolution-frame-boundaries.sh
cat -n scripts/check-resolution-frame-boundaries.sh
echo
echo '=== FrameVec construction and direct access sites ==='
rg -n -C 4 --glob '*.rs' \
'(FrameVec|\.frames\.(top|below|above|by_id|slot_at_captured_depth|frame_at_offset)|\.frames\[[^]]+\]|frames\.iter\(\)|frames\.remove\()' \
crates/engine/srcLength of output: 1896 🏁 Script executed: #!/bin/bash
set -euo pipefail
base=12ea90497eb2ae13fcbf13ff4d50f3a937a8350b
echo '=== FrameVec source ==='
cat -n crates/engine/src/types/resolution/frame_vec.rs
echo
echo '=== ResolutionStack diff ==='
git diff --find-renames --unified=45 "$base" HEAD -- crates/engine/src/types/resolution.rs
echo
echo '=== Guard diff and final source ==='
git diff --unified=45 "$base" HEAD -- scripts/check-resolution-frame-boundaries.sh
cat -n scripts/check-resolution-frame-boundaries.sh
echo
echo '=== All FrameVec access sites in engine Rust ==='
rg -n -C 5 --glob '*.rs' \
'\.frames\.(top|below|above|by_id|slot_at_captured_depth|frame_at_offset)\s*\(' \
crates/engine/src || true
echo
echo '=== Named methods in head and base ==='
for rev in "$base" HEAD; do
echo "--- $rev"
git show "$rev:crates/engine/src/types/resolution.rs" \
| rg -n -C 28 \
'fn (active_discard_or_direct_continuation_parent_mut|replace_change_zone_parent_at_child_boundary|take_active_post_replacement_child|frame_at_offset|slot_at_captured_depth)\b' \
|| true
done
echo
echo '=== Direct FrameVec field usage outside its defining module ==='
rg -n -C 3 --glob '*.rs' '\.frames\.(0|iter|remove|swap_remove|retain|drain|truncate|clear)\b|\bframes\[[^]]+\]' crates/engine/src || trueLength of output: 180032 🏁 Script executed: #!/bin/bash
set -euo pipefail
base=12ea90497eb2ae13fcbf13ff4d50f3a937a8350b
echo '=== Changed production function names and changed line ranges ==='
python3 - <<'PY'
import subprocess, re
base = "12ea90497eb2ae13fcbf13ff4d50f3a937a8350b"
path = "crates/engine/src/types/resolution.rs"
diff = subprocess.check_output(
["git", "diff", "--unified=0", base, "HEAD", "--", path], text=True
)
for line in diff.splitlines():
if line.startswith("@@"):
print(line)
PY
echo
echo '=== FrameSlot declaration, construction, and FrameVec API use ==='
rg -n -C 4 --glob '*.rs' \
'\bFrameSlot\b|FrameSlot\s*\(|(impl\s+(?:From|TryFrom).*(FrameSlot|usize)|impl\s+FrameSlot)' \
crates/engine/src
echo
echo '=== All FrameVec method calls, including non-addressing operations ==='
rg -n -C 2 --glob '*.rs' '\.frames\.[A-Za-z_][A-Za-z0-9_]*\s*\(' \
crates/engine/src/types/resolution.rs
echo
echo '=== ResolutionStack serde declarations and wire conversion/test references ==='
rg -n -C 6 --glob '*.rs' \
'(ResolutionStack|resolution_frames|serde_json::(to_value|from_value)|serde\(transparent\)|round.?trip|wire.*v2|v2_.*reader)' \
crates/engine/src/types/resolution.rs crates/engine/src/types/resolution/frame_vec.rs \
| sed -n '1,700p'
echo
echo '=== Captured-depth and child-boundary origins outside ResolutionStack ==='
rg -n -C 5 --glob '*.rs' \
'(resolution_stack\.len\(\)|child_stack_start|insert_(change_zone_)?parent_at_child_boundary|replace_change_zone_parent_at_child_boundary)' \
crates/engine/src/game crates/engine/src/types \
| sed -n '1,850p'
echo
echo '=== Exact changed lines around every FrameVec call in the diff ==='
git diff --unified=8 "$base" HEAD -- crates/engine/src/types/resolution.rs \
| grep -E -B 12 -A 18 '^[+-].*self\.frames\.(top|below|above|by_id|slot_at_captured_depth|frame_at_offset|replace|swap|insert_below|insert_at_child_boundary)' \
|| trueLength of output: 202720
I checked the converted addressing operations. The
The two raw-offset doors remain bounded:
One non-blocking documentation issue remains. In I did not execute the Rust test suite in this sandbox. The conclusion is based on source and diff inspection.
|
|
@coderabbitai review |
|
…t supports Raised in review. `frame_vec.rs` documented FOUR minting methods in two places while the implementation and the guard both define FIVE — `top`, `below`, `above`, `by_id`, and `slot_at_captured_depth`. The miscount is the smaller half. The module doc also claimed that a `usize` obtained by scanning has "no way to spend it: every accessor takes a `FrameSlot`, and no constructor accepts a `usize`". That is false, and false in the direction that flatters the design: `slot_at_captured_depth` accepts exactly such a `usize`. The parent commit's message carries the same overclaim. The accurate statement, which is what both places now say: a positional scan still compiles, and the only thing that will accept its `usize` is `slot_at_captured_depth`, whose argument is contractually a stack length recorded before a child producer ran. Positional addressing therefore cannot be reached by accident or by ordinary-looking code, and the single deliberate route names itself at the call site. Weaker than "nothing accepts a `usize`", stronger than a grep rule, and worth stating exactly — a doc that overstates its guarantee stops the next reader from checking it, which is the same defect this branch fixed in the guard's own prose on #7485. `frame_at_offset` is now named in the module doc too, with the reason it is not a widening: it takes a `usize` but returns a frame and never a slot. Documentation only; no code changed. `cargo fmt -- --check` 0, guard PASS.
|
Fixed in a588333. Thanks — and the finding is worth more than "non-blocking The module doc also claimed a scanned
Weaker than what I wrote, stronger than a grep rule. The reason to be exact is On the rest of the review — thank you for checking the One addition to your serde note, since you flagged that you did not run the |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
|
@coderabbitai review |
|
1 participant
Follow-up to #7485, which flagged this as the real fix and deliberately left it
out of a bug fix.
What the grep guard was standing in for
ResolutionStackpermits three ways to reach a frame: the top, a frame adjacentto one you already hold, and the frame a
PostReplacementFrameIdnames.Anything else is a positional guess about a structural relationship the stack
does not guarantee — the class of bug #7485 fixed, where cleanup probed the top
two frames and returned
Noneonce the continuation had raised two.That rule was enforced by
scripts/check-resolution-frame-boundaries.shgrepping
resolution.rsforframes.iter().position(..)andframes.remove(..). A grep was the only instrument available:frameswasprivate, but Rust privacy is module-scoped and the module is 7,166 lines, so
"private" bought nothing against the code sitting beside it.
The type
FrameVec(types/resolution/frame_vec.rs, ~230 lines) owns the backingVec.Every accessor takes an opaque
FrameSlotwhose field is private to thatmodule, so slots come only from five minting methods:
top,below,above,by_id, andslot_at_captured_depth.Stated precisely, because the imprecise version is tempting and wrong: a
positional scan still compiles, and the only thing that will accept its
usizeis
slot_at_captured_depth, whose argument is contractually a stack lengthrecorded before a child producer ran. So positional addressing cannot be reached
by accident or by ordinary-looking code, and the one way to reach it
deliberately names itself at the call site. That is weaker than "nothing accepts
a
usize" — an earlier draft of this description, and of the commit message,said exactly that and was wrong — and stronger than a grep rule.
The removal operations (
remove,swap_remove,retain,drain,truncate,clear) have no wrapper. Their use count was already zero, so absence is free,and a future caller has to add the method and justify it rather than quietly
reach for one that exists.
Demonstrated, not asserted
A probe method that scans for a
MultiDrawframe and feeds the result to theaccessors fails to compile with exactly the two errors the design predicts:
The probe was written to be maximally plausible — a real scan feeding a real
accessor inside a real method — because one that fails for an incidental reason
proves nothing about the design. The tree was restored byte-identical
afterwards, sha256 recorded on both sides.
The conversion is mostly documentation
All 112
.framesaccesses were insideimpl ResolutionStack; 30 methodscarried positional use. Every index turned out to come from
len().checked_sub(N)for N in 1..=3, or from the identity lookup — the codealready obeyed the rule and merely said so in arithmetic:
post_replacement_frame_indexis gone; its search is nowFrameVec::by_id, theonly place in the crate that can turn a match into an addressable position.
Two doors deliberately left open
slot_at_captured_depthis the singleusizeentry point. An effectrecords
resolution_stack.len(), runs a child producer, and hands the lengthback so the owner can be parked beneath the child stack that producer raised.
That is not a search. Closing it means giving the captured depth its own type
at ~35 origins across
game/effects/,game/casting_costs.rs,game/casting.rsand their neighbours, plus 12_at_child_boundarywrappers — a separate invariant ("don't confuse a depth with an index") with a
far wider blast radius. The door is named so misuse reads as misuse at the
call site rather than hiding behind an ordinary-looking
get(i).frame_at_offsetreturns a frame and never a slot, becausevalidatemust traverse the whole stack to check a whole-stack invariant. The line this
module draws is that reading is not the hazard; addressing for mutation is.
What is left of the guard
The legacy-wire-key rules are untouched. The two frame-boundary scans are gone,
replaced by one structural check that the design itself is intact:
FrameSlotmust be mintable only by the five documented methods, since a sixth would reopen
positional addressing with no compiler error to show for it. The guard now
protects the design rather than policing every call site, and reads ~230 lines
instead of 7,166.
Both directions were watched go red on an out-of-tree replica, with the replica
confirmed green on either side of each run:
-> Option<FrameSlot>method addedunexpected: first_of_kindunexpected: find_frame; missing: by_idVerification
Measured on the committed tree; digests re-checked after the rebase onto
mainand found byte-identical to the tree these ran against.
cargo fmt -p phase-engine -- --checkcargo clippy -p phase-engine --all-targets -- -D warningscargo test -p phase-engine --libcargo test -p phase-engine --test integrationscripts/check-resolution-frame-boundaries.shThe integration run predates one doc-comment edit to
frame_vec.rs; clippy--all-targetscompiled those targets on the final tree, and a doc commentcannot alter behaviour.
Save compatibility is checked against a pre-change capture, not a round-trip
FrameVecis#[serde(transparent)], so it should serialize exactly as theVec<ResolutionFrame>field it replaced. A break there would be silent, and around-trip test would not catch it — it would write and read the new shape and
pass either way.
The evidence is the two committed captures behind
mycoloth_devour_drain_strand.rs. They were dumped from a player's wedged gameBEFORE this refactor existed, are byte-frozen (
gzip -9 -n, so the digest isreproducible), and carry:
load_capturefeeds that array through the productionPersistedGameStatedecoder. Had
#[serde(transparent)]been missing or misapplied,FrameVecwould demand
{"frames": [...]}one level deeper and decoding would fail withinvalid type: sequence, expected struct FrameVec. Those rows are in the 5106that pass, so the read side is proven against a genuine pre-change save.
Behaviour is unchanged by construction — this is a pure re-expression of
existing addressing, with no rules logic touched and no CR annotation added or
removed.