[Mathih13]

The Module moves a mover along its leg only on the 500 ms creature tick. A stop between two firings wrote the position the last firing stored. The client had already drawn the mover further on, so it snapped back. The runner stop in the playerbots Package and Core's creature death stop both did this.

Since #587 the gap is larger for a creature out of combat: its stored row can lag the leg by up to 4 yd. A durable test on unchanged main stopped a dead wolf 3.9 yd short of where the client drew it.

creatures::tick::stop_where_rendered now stops the mover by the rules a leg advance follows at the stop time. It takes the point from the tick's own rendered_at model, so a Route Path continues into the next waypoint segment and clamps at its end, and it faces the next waypoint. If the advance would halt the path (a blocked segment or changed navigation inputs), the stop keeps the halt point, so it never lands past an obstruction. It moves the row there, with grid address and cell, and sends the zero-duration stop at the same point. The stored position and the stop packet always agree.

The creature death stop uses it, before the Loot Tag and the rewards read the corpse position. The Package runner must switch to it in a separate packages PR.

Tests:

• Harness tests for cycle::stop_on_leg: a stop after a Route Path segment ends (point and heading), a stop after the path ends, an obstructed path, and a non-finite leg.
• stop_between_firings unit tests: a stop that crosses a cell edge writes the new grid address and cell, a stop without a heading keeps the facing, and the spline id rises within one millisecond.
• creature_death_stop durable test: kills a walking wolf between firings and checks the corpse rests on the interpolated point, before and after the stop leg plays. It fails on main.
• The production adapter pin now covers stop_on_stored_leg.

What a human should check in a client, after the packages change lands: watch a bot on a Route Path that stops or retargets right after it passes a waypoint, and kill a patrolling creature mid-walk. Neither should jump back when it stops.

Refs #570

Authored by Claude Opus 5.5 in Claude Code.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Creatures that die while moving now stop at the position shown on screen instead of snapping back to an earlier stored position.
  • Stopping movement mid-route preserves the rendered position, including when the route ends or an obstruction prevents further travel.

• Documentation

  • Clarified how stopping movement relates to the rendered position and route obstructions.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 6cdf66c8-b5ff-4f8d-a440-ef73fe558b27

📥 Commits

Reviewing files that changed from the base of the PR and between 45d5ebb and 4124e9b.

📒 Files selected for processing (7)

• CONTEXT.md
• docs/package-api.md
• module/src/combat/death.rs
• module/src/creatures/cycle/ctx.rs
• module/src/creatures/cycle/harness.rs
• module/src/creatures/cycle/mod.rs
• module/src/creatures/tick/mod.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The stop helper places a moving creature at its client-rendered position and emits a zero-duration stop spline. Creature death uses this helper before resolving loot entitlement. Tests cover route positions, obstruction handling, and corpse position after death.

Changes

Rendered-Position Stops

Layer / File(s)	Summary
Rendered-point stop behavior CONTEXT.md, docs/package-api.md, module/src/creatures/cycle/*, module/src/creatures/tick/mod.rs	The cycle calculates a stop position and optional heading from the stored route leg, subject to obstruction and finite-position checks. The tick helper updates the mover’s position and grid address, applies a supplied heading, and emits a zero-duration stop spline. Tests and documentation describe the behavior.
Creature-death stop integration module/src/combat/death.rs, module/tests/creature_death_stop.rs	Creature death calls the stop helper before resolving Loot Tag entitlement. An ignored integration test checks the corpse position immediately after death and after later stop-leg firings.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 4124e

The documented stop and persistence behavior is consistent with its caller; no confirmed issue blocks merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 4124e

The change keeps existing gameplay authority and makes corpse positioning consistent with movement. No introduced security vulnerability was established in the reviewed flow. Failure recovery and the separate movement-runner migration remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The reviewed transition operates on the targeted creature and its guid-keyed spline, with downstream effects on corpse visibility and eligible reward recipients. It does not accept a new caller-supplied stop coordinate or partition override.

Trust Boundaries and Controls

• observed — The new helper remains crate-internal. The resolved death caller rejects player and already-dead targets before invoking it, while the scheduled tick entrypoint retains its database-identity sender check. Stop coordinates come from stored movement state and retained-route validation.

Resilience and Maintainability Implications

• observed — The stop follows the advance path's obstruction-before-finite-validation ordering. Invalid interpolation does not overwrite the entity with non-finite coordinates; the stop helper instead emits at its retained position. This differs from normal advancement, which drops the malformed leg.

Hardening Proposals

• proposed — Validate that interrupted death processing rolls back the replacement spline, corrected entity, and reward effects together. Preserve same-transaction entity persistence when the separate Package runner adopts the helper.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 6 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: stopped movers remain at the client-rendered position instead of snapping backward.

Full details: Docstring Coverage

Explanation

Docstring coverage is 77.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 6 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Mathih13]

Code review: Standards and Spec

Reviewed main...45d5ebb (1 commit). Spec: #570. The diff was read, not run: I did not run cargo test or the durable test.

The core fix is correct. rendered_point makes the same as_leg(..).rendered_at(now) call that advance_legs makes before commit_position. mover_gone has no effect on rendered_at. The early return when there is no spline row does the same job as the old is_some() gate. Using max(now_ms, spline_id + 1) follows the rule in encounter.rs and world.rs. The chase plant and gate halts run inside a firing, so death is the only stop in core that can happen between firings.

Standards

No hard violations of CODING_STANDARDS.md or AGENTS.md.

• Medium: glossary term misuse (AGENTS.md "Domain language"). The stop_between_firings tests in module/src/creatures/tick/mod.rs call the mover a Character: stored_at ("The Character as the last firing stored it"), the character local, and "the Character is 1.5 yd up the second leg". In CONTEXT.md, Character means a player entity. Rename to creature. ("With no Character left" in creature_death_stop.rs is correct.)
• Medium: possible Duplicated Code / Shotgun Surgery. move_to_rendered_point writes x/y/z, grid_x/y and cell = grid_cell_id(..) by hand again, with its own finite_point guard. The same block is in the place adapter in cycle/ctx.rs and in the advance commit. tick/mod.rs:136 requires the cell to be "ALWAYS written from grid_cell_id in the SAME statement". A shared position writer for a Point would keep that rule in one place.
• Low: possible Middle Man. cycle::rendered_point is a single line that lets tick reach the private as_leg. It also needs a new adapter pin in harness.rs. This is acceptable because as_leg is the adapter, but you could put the helper in cycle and call it from tick.
• Low: possible Primitive Obsession. move_to_rendered_point returns (f32, f32, f32), but Point exists and rendered_point returns one.
• Low: legacy comment in death.rs. The edit is accurate. The block around it is still in the old essay style and has the stray fragment "Retired the old per-caller copies.". Because you changed lines next to it, the boy scout rule says to clean that up in this PR.
• Low: test timing. creature_death_stop.rs uses thread::sleep(1_730) / 1_200. It is #[ignore]d, but timing-based durable tests have caused flaky tests here before (test(playerbots): recovery revisit no longer fails on creature tick timing #569).

Spec

• Medium (a) partial: Confirm on a real client that a stopped bot or a dying creature no longer snaps back #570 says "a bot stopped between a Route leg end and the creature tick snaps back short". This PR does not fix the bot runner stop. It adds the helper and changes the death stop. The runner change is in a later packages PR, so Refs #570 is the correct keyword. Keep Confirm on a real client that a stopped bot or a dying creature no longer snaps back #570 open when this merges.
• Low (c): The PR says the stop lands at "the point the advance phase would commit". That is true only when no obstruction is found. stop_where_rendered skips path_obstruction. When navigation inputs changed, the advance phase can halt at the stored position or at the obstruction point, so a Route Path stop could land past an obstruction. This has little effect on death, but it matters once the runner uses the helper.
• Low (c): The stop does not change orientation. commit_position sets heading along the path, so a stop just past a waypoint keeps the old segment's facing on the server.
• Low (c): In death.rs, death_entitlement(.. target.x, target.y ..) and award_tag_rewards run before the stop, so they read the old position. Call the stop before them so the corpse, the loot tag and the rewards use the same point.
• Low (b): The spline_id floor and the CONTEXT.md / package-api.md text go a little past what Confirm on a real client that a stopped bot or a dying creature no longer snaps back #570 asked for. All of it is justified. Keep it.
• Note: The durable test should fail on main almost every time. It assumes the wolf does not wander after the Character is deleted. If an idle wander replaces the leg before the kill, the test fails on this branch too.

Summary: Standards 6 findings, worst is Character used for a creature in the unit tests (medium). Spec 5 findings plus 1 note, worst is that the bot runner stop from #570 is still unfixed until the packages PR lands (medium, expected).

---

Generated by Claude Code

[Mathih13]

Addressed in 070177b, with the independent review's findings.

Fixed:

• Obstruction (Spec c, CodeRabbit's retained concern). The new cycle::stop_on_leg runs the same path_obstruction check as the advance phase. A blocked segment or changed navigation inputs keep the halt point. The production side, ctx::stop_on_stored_leg, uses the CtxWorld adapter, so it is the same check and not a copy. A harness test covers the blocked case, and it fails when the check is removed.
• Heading (Spec c). A stop on a Route Path now faces the next waypoint, as commit_position does. A halted or landed stop keeps its heading.
• Death order (Spec c). stop_where_rendered now runs right after the dead and pet returns, before death_entitlement and award_tag_rewards.
• Character for a creature (Standards). The tick tests now say creature.
• The stray "Retired the old per-caller copies" fragment. I rewrote the death comment block in plain prose.
• Primitive Obsession and Middle Man. rendered_point is gone. stop_on_leg returns a Stop { at: Point, heading }.
• New tests: a stop that crosses a cell edge (grid address and cell), and next_spline_id within one millisecond.

Not changed:

• Shared position writer. place_stopped still writes x/y/z, grid and cell itself, as place in cycle/ctx.rs does. One shared writer would mean changing place, which the adapter pin covers, and the advance commit. That is a separate refactor.
• Durable test timing. The sleeps only pick a kill time. The expected point comes from the kill timestamp (despawn_at minus the decay), so the firing phase does not matter. The test passed on this head and failed on main.
• Wander risk. With no Character nearby, no cell is awake, so idle movement cannot replace the leg before the kill. creature_advance_persist relies on the same setup.

Ran locally: cargo test -p lyracore-module --lib -- creatures:: tripwire combat::death (433 passed), creature_death_stop -- --ignored (passed), module clippy with -D warnings, and the wasm check with and without debug_reducers.