Skip to content

docs: name the phase-engine package in every cargo invocation - #7205

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
lgray:fix/cargo-p-engine-package-name
Aug 10, 2026
Merged

docs: name the phase-engine package in every cargo invocation#7205
matthewevans merged 2 commits into
phase-rs:mainfrom
lgray:fix/cargo-p-engine-package-name

Conversation

@lgray

@lgray lgray commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Summary

Eleven comments and script strings instruct a reader to run cargo ... -p engine ..., which cannot work: the workspace package is phase-engine and engine is only the lib target name. Each one costs its reader a failed command and a detour. This renames the package selector at all eleven live instruction sites; no behaviour changes.

Files changed

  • crates/engine/src/game/combat.rs — doc comment on combat_tax_profile_gate_go_wide_board
  • crates/engine/src/game/stack.rs — doc block for real_scute_board_resolution_is_not_full_eval_per_token
  • crates/engine/src/game/token_presets.rs — doc comment on catalog_loads_and_validates
  • crates/engine/src/game/triggers.rs#[ignore] reason on scute_swarm_throughput
  • crates/engine/src/parser/oracle_ir/feature.rs — doc comment on mod pool_structure_census
  • crates/engine/src/types/game_state_size.rs — GameState layout measurement command
  • crates/engine/src/bin/interaction_bindings.rs — generated-file header authority
  • crates/engine/tests/integration/squirrel_perf_probe.rs//! module header
  • client/src/adapter/generated/interaction/index.ts — generated header synchronized with its authority
  • scripts/migrate-mana-target-roles.mjs — "HOW TO USE IT" comment and the closing console.log

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

CR references

None. No game logic is touched — the diff is doc comments, one #[ignore] reason string, one block comment and one console.log.

Verification

  • Required checks ran clean, or the exact CI-owned alternative is stated below.
  • Gate A output below predates the maintainer documentation-only follow-up; CI refresh is pending.
  • Final review-impl below is clean for the current committed head.
  • Both anchors cite existing analogous code at the same seam.

The contributor ran these at 0f50602f8613f9d4931b8446eed70ab089d72c4c:

  • cargo fmt -p phase-engine -- --check — exit 0
  • cargo clippy --workspace --all-targets -- -D warnings — exit 0
  • cargo test -p phase-engine --lib — 18830 passed; 0 failed; 6 ignored
  • cargo test -p phase-engine --test integration — 4794 passed; 0 failed; 2 ignored
  • node --check scripts/migrate-mana-target-roles.mjs — exit 0

The defect, measured:

$ cargo pkgid -p engine
error: package ID specification `engine` did not match any packages
$ cargo pkgid -p phase-engine
path+file:///.../crates/engine#phase-engine@0.50.0

Authority: crates/engine/Cargo.toml[package] name = "phase-engine" against [lib] name = "engine".

The fix, measured — discriminating in both directions. The old form dies at package resolution before any build; the new form resolves and its filter selects a nonzero count of the intended tests (a corrected command selecting zero would mean the filter was wrong too):

$ cargo test -p engine --lib pool_structure_census -- --list
error: package ID specification `engine` did not match any packages     (exit 101)
$ cargo test -p phase-engine --lib pool_structure_census -- --list
parser::oracle_ir::feature::pool_structure_census::census: test
1 test, 0 benchmarks                                                    (exit 0)

Five corrected commands were probed in review; all resolve, all select nonzero (1, 2, 1, 1, 1). Running the fully corrected --ignored form reaches the test body and stops on the environment variable the comment itself tells you to set (ORACLE_POOL_DIR) — the expected outcome, and the point: selector, target filter and test filter all resolve. That is deliberately not claimed as a green test run. Cargo's own rerun hint prints -p phase-engine --lib.

Completeness. 18 word-exact hits at base, 11 live instruction sites fixed here. The 7 remaining hits are in .claude/wf/*.md historical planning documents, left for maintainer discretion.

Maintainer follow-up a812b75e7215a30f09925b53e6e54ca634f496e0 completed the three live selector sites that the original body had mistakenly described as covered by another PR. It is documentation-only; fresh CI for that head is pending.

--package engine and -p=engine spellings were also swept: no hits.

Reviewer note for anyone running their own grep: a naive grep -rn -- '-p engine' also matches .cargo/config.toml's -p engine-inventory-gen. That is a substring match, not a defect — engine-inventory-gen is a real package and cargo pkgid resolves it. A word-boundary pattern finds only the 7 historical planning-document references.

Gate A

Gate A PASS head=0f50602f8613f9d4931b8446eed70ab089d72c4c base=ee76bc54aee4941f20c126b49c5d6b54a36d86c5. The documentation-only maintainer follow-up a812b75e7215a30f09925b53e6e54ca634f496e0 is awaiting its fresh CI run.

Anchored on

  • Tiltfile:160 — cargo nextest run -p phase-engine: the working form of exactly this command, in the resource that actually runs the engine tests
  • scripts/check-interaction-bindings.sh:12 — cargo run -p phase-engine --features interaction-bindings ...: existing correct package selection at the same seam

Final review-impl

Final review-impl PASS head=a812b75e7215a30f09925b53e6e54ca634f496e0 (documentation-only follow-up; fresh CI pending)

Claimed parse impact

None. crates/engine/src/parser/oracle_ir/feature.rs appears in the diff, but the change is one word inside a doc comment; no parser behaviour, card data, or coverage output is affected.

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

Summary by CodeRabbit

  • Chores
    • Updated benchmark, test, performance-probe, and validation commands to reference the correct engine package.
    • Updated migration script guidance and output to use the corrected package name.
    • Updated generated-file and measurement instructions to reflect the renamed package.
    • No user-facing functionality changed.

Eight comments and script strings tell a reader to run `cargo ... -p engine
...`. None of them can work. The workspace package is `phase-engine`; `engine`
is only the lib TARGET name, so `-p engine` names a package that does not
exist:

    $ cargo test -p engine --lib pool_structure_census -- --list
    error: package ID specification `engine` did not match any packages
    $ cargo pkgid -p engine
    error: package ID specification `engine` did not match any packages
    $ cargo pkgid -p phase-engine
    path+file:///.../crates/engine#phase-engine@0.50.0

The authority is `crates/engine/Cargo.toml`: `[package] name = "phase-engine"`
against `[lib] name = "engine"`.

Every one of these strings is an instruction someone is meant to follow -- a
perf benchmark's `#[ignore]` reason, a census test's reproduce line, a
migration script's closing "Verify with:" -- so each one costs its reader a
failed command and a detour before they work out that the package is named
something else. The corrected forms resolve, and resolve to the intended
tests rather than to nothing:

    $ cargo test -p phase-engine --lib pool_structure_census -- --list
    parser::oracle_ir::feature::pool_structure_census::census: test
    1 test, 0 benchmarks

Running the fully corrected `--ignored` form reaches the test body and stops
on the environment variable the comment itself tells you to set
(`ORACLE_POOL_DIR`), which is the expected outcome and the point: the package
selector, target filter and test filter all resolve, and only the documented
setup step remains. Cargo agrees -- its own rerun hint prints
`-p phase-engine --lib`.

Three further instances of this defect exist in `game_state_size.rs`,
`bin/interaction_bindings.rs` and the generated `client/src/adapter/generated/
interaction/index.ts`; they are fixed by a separate in-flight PR and are
deliberately untouched here so the two changes cannot conflict in either merge
order. Seven more live in `.claude/wf/*.md` planning documents and are left for
maintainer discretion.

A naive `grep -rn -- '-p engine'` also matches `.cargo/config.toml`'s
`-p engine-inventory-gen`. That is a substring hit, not a defect --
`engine-inventory-gen` is a real package and `cargo pkgid` resolves it.

Assisted-by: ClaudeCode:claude-opus-5
@lgray
lgray requested a review from matthewevans as a code owner August 10, 2026 13:16
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request replaces outdated engine package references with phase-engine in Rust test documentation, benchmark commands, migration-script verification instructions, generated-file instructions, and build documentation.

Changes

Package name updates

Layer / File(s) Summary
Update package command references
crates/engine/src/game/*.rs, crates/engine/src/parser/oracle_ir/feature.rs, crates/engine/tests/integration/squirrel_perf_probe.rs, crates/engine/src/bin/interaction_bindings.rs, crates/engine/src/types/game_state_size.rs, scripts/migrate-mana-target-roles.mjs
Test commands, benchmark instructions, migration-script verification messages, generated-file instructions, and build documentation now target phase-engine instead of engine.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

  • phase-rs/phase#6739: This PR continues the package rename by updating remaining Rust, script, generated-file, and build-documentation references.

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 describes the main change: updating Cargo invocations to use the phase-engine package name.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Generated for head a812b75e7215a30f09925b53e6e54ca634f496e0.

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans matthewevans self-assigned this Aug 10, 2026
Completes the live documentation selector migration for the generated interaction bindings and GameState size measurement instructions.

Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
@matthewevans

Copy link
Copy Markdown
Member

Held pending current-head CI. The maintainer follow-up a812b75e7215a30f09925b53e6e54ca634f496e0 completes the three remaining live -p engine instruction strings and synchronizes the generated interaction header with its Rust authority. Manual review found no behavior change; the current required Rust and frontend checks, refreshed Gate A/parse-diff artifact, and CodeRabbit pass are still pending for this exact head.

Once those checks settle green, this will resume directly to approval and merge-queue handling; no contributor action is needed.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved — documentation-only command correction.

✅ Clean

  • crates/engine/Cargo.toml:1 defines the package as phase-engine, while :11 keeps engine as the library target; every changed command therefore now selects the correct Cargo package without changing production behavior.
  • The updated generated interaction header matches its source authority in crates/engine/src/bin/interaction_bindings.rs:24.
  • The current-head parse-diff artifact for a812b75e7215a30f09925b53e6e54ca634f496e0 reports no card-parse changes, and the refreshed required CI checks are green.

Recommendation: merge via the queue.

@matthewevans matthewevans added the documentation Improvements or additions to documentation label Aug 10, 2026
@matthewevans
matthewevans added this pull request to the merge queue Aug 10, 2026
@matthewevans matthewevans removed their assignment Aug 10, 2026
Merged via the queue into phase-rs:main with commit 6bad30e Aug 10, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants