Skip to content

[codex] fix autopilot poison triggers#148

Draft
loothero wants to merge 1 commit into
mainfrom
codex/fix-autopilot-poison
Draft

[codex] fix autopilot poison triggers#148
loothero wants to merge 1 commit into
mainfrom
codex/fix-autopilot-poison

Conversation

@loothero
Copy link
Copy Markdown
Member

@loothero loothero commented May 4, 2026

Summary

  • Make autopilot poison eligibility re-run when balances, caps, thresholds, poison state, or applying/attacking state change.
  • Clear poison attempt guards and potion-applying state on failed poison/extra-life actions.
  • Add smart poison waiting so autopilot does not immediately attack after applying poison before poison damage reaches the 1 HP / 0 extra-life floor.
  • Add tests for poison floor projection and smart poison orchestration.

Root Cause

Autopilot poison effects only depended on a small subset of the state they used, so a beast could become eligible while the effect stayed idle and the UI still showed “Waiting for trigger...”. There was also no failure cleanup for potion actions, and poison could be followed by attack logic in the same evaluation pass.

Validation

  • pnpm exec tsc --noEmit
  • pnpm exec eslint src/hooks/useAutopilotOrchestrator.ts src/hooks/useAutopilotOrchestrator.test.tsx src/contexts/GameDirector.tsx src/utils/beasts.ts src/utils/beasts.test.ts --quiet
  • pnpm exec vitest run src/hooks/useAutopilotOrchestrator.test.tsx src/utils/beasts.test.ts src/contexts/GameDirector.test.tsx
  • pnpm build

@vercel
Copy link
Copy Markdown

vercel Bot commented May 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
summit Ready Ready Preview, Comment May 4, 2026 1:25am

Request Review

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 4, 2026

🚅 Deployed to the summit-pr-148 environment in Summit

3 services not affected by this PR
  • summit-db
  • summit-indexer
  • summit-api

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 446bb886-a9a9-4f76-8ad1-21bc52256f58

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-autopilot-poison

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements "Smart Poison" functionality within the autopilot orchestrator, enabling the system to project poison damage over time and delay attacks until the target reaches a minimum health threshold (1 HP and 0 extra lives). Key changes include the addition of damage projection utilities, updated state management in the orchestrator hook to handle poison wait states, and comprehensive test coverage. Feedback was provided regarding an inefficiency in the poison check timer, which currently polls every second; it is recommended to use the actual projected duration to reduce unnecessary orchestrator re-evaluations.

const scheduleSmartPoisonCheck = (secondsUntilReady: number) => {
clearSmartPoisonTimer();

const delayMs = Math.max(250, Math.min(Math.ceil(secondsUntilReady), 1) * 1000);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation caps the delay at 1 second (Math.min(..., 1) * 1000), which results in a polling loop that re-triggers the entire orchestrator logic every second while waiting for poison. Given the complexity of this hook (numerous useMemo and useEffect blocks), this is inefficient.

It is recommended to wait for the actual projected duration. React state updates (e.g., from new blocks) will naturally trigger re-evaluations if the summit state changes, and the existing cleanup logic will handle timer cancellation if the wait is no longer valid. If a per-second UI countdown is required in the log, it should ideally be handled by a more lightweight mechanism than re-triggering the full orchestrator.

Suggested change
const delayMs = Math.max(250, Math.min(Math.ceil(secondsUntilReady), 1) * 1000);
const delayMs = Math.max(250, Math.ceil(secondsUntilReady) * 1000);

@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 77.43191% with 58 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
client/src/hooks/useAutopilotOrchestrator.ts 73.05% 52 Missing ⚠️
client/src/contexts/GameDirector.tsx 0.00% 3 Missing ⚠️
client/src/utils/beasts.ts 95.08% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Claude Review - React/Frontend Review

run=25296585853 attempt=1 sha=7ab89609173965c337e1d12053a2ff8de8aaf584 scope=client

[MEDIUM] client/src/hooks/useAutopilotOrchestrator.ts:132 - scheduleSmartPoisonCheck delay formula is inverted/buggy.
Impact: For any secondsUntilReady >= 1 the expression Math.max(250, Math.min(Math.ceil(secondsUntilReady), 1) * 1000) always evaluates to 1000, because Math.min(N, 1) clamps the upper bound to 1 second before the *1000. The result is that smart poison waits poll every 1s and re-run the entire main attack effect (including all memo recomputation and selector evaluation) — e.g. ~102 effect re-runs over a 102s wait in the included test. Even if the 1s cadence is intentional for the countdown UI, the code reads like a typo for Math.max(250, Math.ceil(secondsUntilReady) * 1000) and an upper cap.
Fix: Either (a) commit to a single timeout: Math.max(250, Math.ceil(secondsUntilReady) * 1000); or (b) if a 1s countdown tick is desired, write it intentionally: Math.max(250, Math.min(Math.ceil(secondsUntilReady) * 1000, 1000)) and add a brief comment so it isn't read as a mistake.

[LOW] client/src/hooks/useAutopilotOrchestrator.ts:258, 268 - poisonedThisSequence.add(currentSummit.beast.token_id) is dead code.
Impact: The new setAttackInProgress(false); return; immediately after the add exits the function before another batch can run, so the per-sequence dedupe set is never consulted again. Harmless today but misleading for future readers who may believe targeted poison is being deduped across batches in this loop.
Fix: Remove the two poisonedThisSequence.add(...) calls (and reconsider whether the Set declaration at line 225 still pays for itself — it is now only ever queried in the !poisonedThisSequence.has(...) guard that always passes on first iteration).

[LOW] client/src/hooks/useAutopilotOrchestrator.ts:139-156 - startSmartPoisonWait snapshots a synthesized summit, ignoring later WebSocket truth.
Impact: Once the smart wait is set, getPoisonFloorProjection(smartPoisonWait.summit) always evaluates against the snapshot taken just after the local executeGameAction resolved. If the indexer-driven WS later reports a higher poison_count (another player poisoned the same beast) or a smaller current_health/extra_lives (somebody else attacked), the projection will be stale and autopilot will keep waiting after the floor has actually been reached. Trigger conditions: PvP overlap on a single summit beast. The token-id-change effect at line 331 only refreshes when the beast itself swaps.
Fix: Recompute the projection each evaluation against the live summit instead of the snapshot — e.g. fold the just-applied amount into the live values via max(snapshot.poison_count, summit.poison_count + delta) and use summit.poison_timestamp when it is newer than the snapshot. Alternatively re-snapshot inside the main effect when summit.poison_count/poison_timestamp advance.

[LOW] client/src/hooks/useAutopilotOrchestrator.ts:178-208 - handleApplyPoison returns true synchronously regardless of the eventual promise outcome.
Impact: Callers that gate on the boolean (if (... && handleApplyPoison(...))) set targetedPoisonKeyRef.current = poisonKey or poisonedTokenIdRef.current = summit.beast.token_id immediately, even though the promise may later reject and the .catch clears those refs back to null. There is no observable correctness bug today (the .catch resets, the .then(false) resets, and applyingPotions blocks re-entry while in flight), but the return value lies about success and complicates reasoning. With setApplyingPotions(false) only called in .catch and not on .then(false) (relying on GameDirector line 668 to handle that branch), the two failure paths are also subtly different.
Fix: Have handleApplyPoison return the dispatched boolean (initiation success) and consolidate failure cleanup into one helper invoked from both .then(!result) and .catch, including setApplyingPotions(false) in both — don't depend on GameDirector to do half of it.

[INFO] client/src/hooks/useAutopilotOrchestrator.ts:331-336 - Token-id reset effect runs on initial mount.
Impact: On first render with a summit already present, this effect runs its body once, clearing refs that were already null and calling clearSmartPoisonWait() while no wait could possibly exist. Harmless, but worth knowing — combined with React 18 StrictMode double-invocation in dev it can clear a wait that was scheduled in a previous mount cycle. Confirm via manual test that toggling autopilot in dev doesn't strand UI in "Waiting for poison" without a pending timer.

[INFO] client/src/hooks/useAutopilotOrchestrator.test.tsx:128-137 - Test bypasses the real applyingPotions lifecycle.
Impact: Because the mocked executeGameAction doesn't replicate GameDirector's setApplyingPotions(false) on success, the test manually calls it. This means the test does not exercise the actual cleanup path that the PR is trying to fix (the new setApplyingPotions(false) for add_extra_life/apply_poison failure at GameDirector.tsx:668). Consider an additional test that mocks executeGameAction to resolve false and asserts both applyingPotions clears and the smart-wait/poison refs are cleared — that is the scenario the PR description explicitly claims to fix.

[INFO] client/src/hooks/useAutopilotOrchestrator.ts:540-543 - Attack payload still uses raw summit poison-floor amounts (only summitForAttack is used for the damage check).
Impact: The "ready" branch synthesizes summitForAttack with floored health/extra_lives only to compare against totalEstimatedDamage. The actual executeGameAction({ type: 'attack', ... }) call doesn't take the summit pool as a parameter, so this is fine — but if the contract still applies poison damage at execution time and you were relying on that, no action is needed. Confirm that the 'guaranteed' damage check passing on the floored projection cannot let through a batch that fails at the contract because real poison damage hasn't yet been applied.

Summary: 0 CRITICAL, 0 HIGH, 1 MEDIUM, 3 LOW, 3 INFO

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Codex Review - React/Frontend Review

[INFO] client/:1 - Review blocked: the local command runner failed before git diff could execute (bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted), and remote PR diff access did not return the changed files.
Impact: I could not inspect the PR’s client diff, so I cannot certify the assumptions, exceptions, workarounds, or changed-line correctness.
Fix: Re-run the review where git diff 90301c58564ebac000d30f2692d409db8eec6cde...7ab89609173965c337e1d12053a2ff8de8aaf584 -- client/ works, or provide the diff contents.

Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 1 INFO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant