compat: add Integrated Dynamics + Integrated Crafting deterministic recipe integration - #51
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds compatibility support for Integrated Dynamics and Integrated Crafting, including new compatibility modules, contracts, audits, documentation, and GameTest fixtures. To handle Integrated Dynamics' multi-mod jar, the Compat Kit is updated with a --mod-id option, and a build task is added to strip an embedded Refined Storage GameTest class to avoid classloading issues. Feedback on the changes points out that calling .distinct() on a stream of ItemStack in IntegrateddynamicsCompat.java is ineffective because ItemStack does not implement value-based equality, and should be removed.
| private static List<ItemStack> representatives(Ingredient ingredient) { | ||
| return Arrays.stream(ingredient.getItems()) | ||
| .filter(stack -> !stack.isEmpty()) | ||
| .map(stack -> stack.copyWithCount(1)) | ||
| .distinct() | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
Calling .distinct() on a stream of ItemStack is ineffective because ItemStack does not implement value-based equals and hashCode. Since copyWithCount(1) creates a new ItemStack instance for each element, reference-equality comparison will never match, making .distinct() redundant.
Since itemKeys already correctly deduplicates the items by mapping them to StorageResourceKey (which implements value-based equality) and calling .distinct(), we can safely remove the redundant .distinct() call here to improve clarity and avoid misleading code.
private static List<ItemStack> representatives(Ingredient ingredient) {
return Arrays.stream(ingredient.getItems())
.filter(stack -> !stack.isEmpty())
.map(stack -> stack.copyWithCount(1))
.toList();
}There was a problem hiding this comment.
Validated as false positive for Integrated Dynamics production behavior (no production change).
- Factually correct: NeoForge 1.21.1
ItemStackhas no valueequals/hashCode, so.distinct()aftercopyWithCount(1)is identity-only. - Callers of
representativesare onlyexact(...)(non-empty) andconsumed(...)→itemKeys(...), which maps toStorageResourceKey.item(...)then.distinct()under value equality.TypedRecipeInputalso rejects duplicate alternatives. - Deterministic consume-any uniqueness is therefore already enforced by
StorageResourceKeydistinct, not by the ItemStack stream distinct. - Removing the redundant
.distinct()would be clarity-only and would not change craft plans. Farmers Delight-styleItemStackKeyvalue dedupe is needed only when a module indexes remainders by stack identity; ID does not.
Evidence recorded in docs/notes.md on head 18453eb195697d603d3c3b9a0f06dd4232998cd2.
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
Continuation: Gemini
|
| Check | Result |
|---|---|
Gemini claim (identity-only ItemStack.distinct) |
Correct observation |
| Observable ID craft-plan uniqueness | Already via itemKeys → StorageResourceKey.distinct |
| Production change | None |
| Docs | docs/notes.md records the evidence |
runIntegratedDynamicsGameTestServer |
8/8 |
runIntegratedCraftingGameTestServer |
4/4 |
| Python unit tests | 409 OK |
git diff --check |
clean |
New head: 18453eb195697d603d3c3b9a0f06dd4232998cd2. No review bot triggered.
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
18453eb to
a5c2f30
Compare
|
Rebased onto current Head: Evidence
Notes
No merge / no review-bot trigger from this update. |
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
a5c2f30 to
629b33c
Compare
Rebase evidence (post-#73/#70/#69/#61/#58)Head: Rebased through Advanced AE, Create Aquatic Ambitions, Draconic Evolution, Productive Bees, and Railcraft. Conflict resolutions kept both sides (main fail-closed modules + ID/IC). Remeasured coexistence digests under the full matrix:
Local verification (heavy_gradle / exclusive matrix slots)
No merge / no review-bot trigger. |
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
629b33c to
34eb2a3
Compare
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
34eb2a3 to
7086f8d
Compare
Rebase evidence (post-#78 isolation + Oritech #47)Head: Conflict policy kept main’s isolation architecture and this PR’s ID/IC module surface. Peer coexistence digests were not restored into descriptors. ID isolated digest is now Local gates (heavy_gradle where applicable)
Gemini |
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-authored-by: Cursor <cursoragent@cursor.com>
7086f8d to
03a11de
Compare
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
03a11de to
9e93919
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e93919c94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boolean hasItem = item.isPresent() && exact(item.get()); | ||
| boolean hasFluid = fluid.isPresent() && exact(fluid.get()); | ||
| return hasItem || hasFluid; |
There was a problem hiding this comment.
When a datapack supplies a valid fluid input together with a non-simple item ingredient, this hasItem || hasFluid check accepts the recipe even though the item input failed validation. dryingPlan nevertheless passes that ingredient's representative stacks to consumeAny, which can craft against an approximation of custom matching semantics—or throw if the ingredient has no representatives—instead of failing closed. Require every present input to be exact in addition to requiring at least one input.
Useful? React with 👍 / 👎.
| private static boolean exactDryingOutput(RecipeDryingBasin recipe) { | ||
| Optional<ItemStack> item = dryingItemOutput(recipe); | ||
| Optional<FluidStack> fluid = recipe.getOutputFluid().filter(IntegrateddynamicsCompat::exact); | ||
| return item.isPresent() || fluid.isPresent(); |
There was a problem hiding this comment.
When a drying recipe has an ItemStackFromIngredient output plus a valid fluid output, dryingItemOutput deliberately filters out the derived item, but this OR still marks the recipe supported. The resulting plan commits the craft with only the fluid output, silently dropping an output that the real machine would produce; reject the recipe whenever a present item output is not an exact left-side ItemStack.
Useful? React with 👍 / 👎.
| if (entry.name == integratedDynamicsRsGameTestClass) { | ||
| dropped = true | ||
| zin.closeEntry() | ||
| entry = zin.nextEntry | ||
| continue |
There was a problem hiding this comment.
The Integrated Dynamics fixture and compatibility-matrix runtimes are wired to the jar produced here after this class is deleted, while the SHA verifier checks a separate pristine artifact. Consequently, these release gates never load the exact third-party jar players receive and cannot detect registration or classpath conflicts involving the removed holder; use the raw SHA-verified Maven bytes and resolve the missing test dependency without mutating them. The repository explicitly requires third-party Maven artifacts to remain raw exact bytes.
AGENTS.md reference: AGENTS.md:L176-L176
Useful? React with 👍 / 👎.
| var holder = helper.getLevel().getRecipeManager().byKey(recipeId).orElse(null); | ||
| if (holder != null && CraftingTerminalMenu.supportsRecipeHolder(holder)) { |
There was a problem hiding this comment.
The committed representative artifact inventory contains only three minecraft:crafting_shaped recipes and no integratedcrafting:special/minecraft_dead_bush, so this lookup returns empty and the conditional lets the GameTest pass without ever calling supportsRecipeHolder. This test is nevertheless used as the contract's DeadBush/remainder verification evidence; load or construct a real RecipeDeadBush holder so the required present-mod gate performs the claimed behavior assertion.
AGENTS.md reference: AGENTS.md:L173-L173
Useful? React with 👍 / 👎.
| recipeFamilies.register(mechanicalDryingBasin.getPath(), () -> | ||
| RecipeFamilyFactories.deterministicResources( |
There was a problem hiding this comment.
When either Integrated Dynamics mechanical consumption rate changes after a recipe has first been listed, deterministicResources retains the old per-recipe plan and final contract, so previews and commits can continue charging the previous FE amount and shared Craftable results have no dynamic token that invalidates them. Both mechanical registrations read mutable loaded config from their plan functions, so they need dynamicDeterministicResources with the corresponding complete config token, as used elsewhere for reloadable server rates.
Useful? React with 👍 / 👎.
| if (energyPerTick == 0) { | ||
| return OptionalLong.of(0L); |
There was a problem hiding this comment.
When either mechanical machine's loaded consumptionRate is zero, this helper returns a present zero value, so both mechanical eligibility predicates accept the recipe and their plans omit the FE input entirely. That turns the accepted mechanical families into free crafts even though the reviewed contract and active compatibility documentation require a finite positive FE total and say non-positive totals fail closed; reserve zero for the manual Drying Basin path and reject it for mechanical families.
Useful? React with 👍 / 👎.
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9e93919 to
791b4f3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 791b4f3c86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!craft(context, SQUEEZER_RECIPE) | ||
| || context.core().getResourceAmount( | ||
| StorageResourceKey.neoforgeEnergy()) | ||
| != Long.MAX_VALUE - expectedSqueezerEnergy(15)) { |
There was a problem hiding this comment.
The contract cites this method as its checked_overflow_atomic evidence, but starting with Long.MAX_VALUE energy and subtracting the finite recipe cost is valid arithmetic, and the assertion explicitly requires the craft to succeed. An unchecked-overflow regression would therefore remain undetected; provoke an overflowing addition or destination write and assert that every input, energy, output, and station-work value remains unchanged.
AGENTS.md reference: AGENTS.md:L176-L176
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d0fe0d. The fixture now fills the existing Menril Resin fluid destination to Long.MAX_VALUE, attempts the Mechanical Squeezer craft, requires rejection, and asserts the input item, item output, fluid amount, finite FE, and station work all remain unchanged. Added a focused static regression first (RED for the missing real-overflow test), then verified it GREEN; runIntegratedDynamicsGameTestServer passed 8/8, the full Python suite passed 597/597, and git diff --check is clean.
PR #51 review note: ItemStack stream distinct is identity-only, but ID consume-any uniqueness already comes from StorageResourceKey.distinct. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2d0fe0d to
b62c0cc
Compare
Remove glued conflict leftovers from create digest JSON and docs, restore main CAA/AA notes body with ID lead paragraphs, and keep Create digest at main's CAA baseline pending full-matrix remeasure. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restore the truncated matrix fixture, assert isolated recipe digests in ID/IC GameTests, and pin the measured Integrated Dynamics inventory SHA. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase onto current main requires format-17 evidence and passing Compat Kit verify; migrate both modules from exact Modrinth jars, clean sources, and full ancestry rather than accepting format-7 fail-closed verify. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document scanner format 17 identities, Outcome A/C, and that full Compat Kit verify must pass; remove the obsolete follow-up claim for failing verify. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase onto #87 requires shared exact-SHA transforms instead of the Integrated Dynamics-specific strip task; both contracts now declare the same GameTestHolder removal for fixture and matrix runtimes. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Capture exclusive-matrix coexistence digests and the declarative runtime transform migration after rebasing onto exact fixture transforms. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Declared ItemStackFromIngredient drying outputs must fail closed even when an exact fluid is also present, so Auto Storage never promotes fluid-only plans by treating an unmodelable item as absent. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Zero-family Integrated Crafting present_target_load_once must assert the compat module registered exactly once, not only ModList plus empty registries. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Preserve Immersive Engineering/core Craftable fast-path evidence alongside Integrated Dynamics/Crafting gates after rebasing onto d8314fc. via [HAPI](https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the dead ITEM.get null check and require auto_storage namespace plus integratedcrafting path prefix on the IC unsafe-registry GameTest. Co-authored-by: Cursor <cursoragent@cursor.com>
Reject present non-exact drying inputs even when the other input is exact, and omit Mechanical Squeezer FE when the loaded cost is zero so plans stay valid. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
09064d3 to
6775b69
Compare
|
/gemini review |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6775b69. Configure here.
There was a problem hiding this comment.
Code Review
This pull request introduces compatibility support for Integrated Dynamics and Integrated Crafting. It registers three deterministic recipe families for Integrated Dynamics (Drying Basin, Mechanical Drying Basin, and Mechanical Squeezer) with localized station labels and atomic transaction contracts. Integrated Crafting is integrated as a fail-closed module contributing zero production recipe families. The PR includes comprehensive documentation, JSON audits, contracts, isolated test fixtures, GameTests, and static regression tests to ensure robust, fail-closed behavior. There are no review comments provided, and I have no additional feedback to offer as the implementation is thorough and well-tested.
Unfiltered AE2 terraformersmc re-registration made Gradle probe Flywheel there; a 502 from that host failed unrelated Create/CEI resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds compatibility support for Integrated Dynamics and Integrated Crafting. It introduces new compatibility modules, station descriptors, localized language keys, and comprehensive integration GameTests to verify deterministic recipe handling (such as Drying Basin and Mechanical Squeezer) while ensuring non-deterministic or unsafe recipes remain fail-closed. The review feedback points out a redundant .distinct() call on an ItemStack stream in IntegrateddynamicsCompat.java that can be safely removed to improve code clarity.
| private static List<ItemStack> representatives(Ingredient ingredient) { | ||
| return Arrays.stream(ingredient.getItems()) | ||
| .filter(stack -> !stack.isEmpty()) | ||
| .map(stack -> stack.copyWithCount(1)) | ||
| .distinct() | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
The .distinct() call on the ItemStack stream is redundant and ineffective. In Minecraft/NeoForge, ItemStack does not override equals() or hashCode(), meaning it defaults to reference identity (==). Since stack.copyWithCount(1) is called right before .distinct(), every element in the stream is a newly instantiated ItemStack object, making reference-based deduplication a no-op. To avoid confusion and improve clarity, consider removing the redundant .distinct() call.
private static List<ItemStack> representatives(Ingredient ingredient) {
return Arrays.stream(ingredient.getItems())
.filter(stack -> !stack.isEmpty())
.map(stack -> stack.copyWithCount(1))
.toList();
}There was a problem hiding this comment.
Controller classification: non-actionable / false positive for ID production behavior (same finding class as #51 (comment)).
ItemStack has no value equals/hashCode on this baseline, so stream .distinct() is identity-only. Callers map through StorageResourceKey.item(...).distinct() under value equality; removing this call is clarity-only and does not change craft plans. No production change.
Drop cherry-pick conflict spill of PR #51 history from notes.md. Co-authored-by: Cursor <cursoragent@cursor.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5743405. Configure here.
Drop cherry-pick conflict spill of PR #51 history from notes.md. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Closes #44.
Keeps both targets in one PR:
Rebased onto current
origin/main(d8314fc5a3c9f5244eb6a50269b5590ef90a8ca6) after PR #62 Immersive Engineering / core typed-plan fast-path. Additive docs keep IE Outcome C + listing-local typed-plan reuse from main together with ID/IC compatibility, the three P1 corrections, and the ICregistrationCount()load-once assertion. Preserved main's declarative transform pipeline and shared contract-ownedruntime_artifact_transformsfor both ID and IC. Descriptors own only isolated recipe-inventory digests; coexistence/unclaimed digests are matrix-report-only.Note: issue #44 body text was mis-templated as Ender IO; title + coordinator scope are authoritative (ID + IC).
Artifacts
1.33.31.4.6tG3ZKTepse5g7ewqmaven.modrinth:integrated-dynamics:tG3ZKTepmaven.modrinth:integrated-crafting:se5g7ewq7c508ebd4048a589812562740132d39802ea0034e11a011fbfd53188b39fdba21c49c774bc8fa28d344592b65ae5d0082c497b21dd5250fcc2aadefedb964cec1.21.1-1.33.3/b232bc068c31b7ad98e437775e15b76b98dda6f71.21.1-1.4.6/352bce1cfd1d57aef79aeb17b38895aaac0cac392382ea29…eb5f(56,279,815 B)vEjxRv40, Common Capsc50bCinZtG3ZKTep+ Cyclops + Common CapsGameTestsAspectsRefinedStorageID Modrinth jar embeds a second
[[mods]]entry (integrateddynamicscompat); Compat Kit selects--mod-id integrateddynamicswhile hashing full jar bytes. Fixture/matrix runtimes use the shared declarative transform output; audit SHA verification still resolves pristine Modrinth bytes.Families
Integrated Dynamics (Outcome A)
RecipeDryingBasin,RecipeMechanicalDryingBasin,RecipeMechanicalSqueezer.22990962ac2a4058027e307c1f38bace89abc07336b71b1a2ac812ab0789da9b/ 252 recipes.Integrated Crafting (Outcome C)
RecipeDeadBush(special crafting / entity mutation); network automation surfaces already covered by vanilla exact recipes.3593986292d1f9d2cf093b3a9d16b0828c3410817eb57cb1d6b4b70b23719aa7.Format-17 + #87 transform migration
Legacy format-7 audits/contracts are gone. Both modules were migrated with repository Compat Kit commands against exact jars, clean official sources, and staged ancestry. After rebasing onto #87, the ID-specific Gradle strip task was removed and both contracts declare the same
target.runtime_artifact_transformsrecord. Full Compat Kit CLIverifymust pass.Tests and exact local results (head
b9a778386f4159bfaca32c9a5d599399e3ce36a6)verify(ID + staged ancestry)verify(IC + staged ancestry)runIntegratedDynamicsGameTestServerrunIntegratedCraftingGameTestServerrunCompatibilityMatrixGameTestServer(exclusive 4-slot)6f5ef3a5…c460); unclaimed 1,589 (6680f6c7…c11d); craftable prepare 49.481 ms; interaction p95 15.753 ms; first open 3.901 ms; first/switch p95 1.036 / 0.620 ms; per-menu 116,989 B; shared index 4,134,776 B./gradlew build./gradlew runDatapython3 -m unittest discover scriptsgit diff --checkDocs
docs/integrated-dynamics-compatibility.md,docs/integrated-crafting-compatibility.mddocs/notes.md,docs/structure.mdOutcome
Remaining risks
BlockMechanical*Config.consumptionRate(default 80).@GameTestHolderbefore namespace filtering; fixture/matrix use the shared exact-SHA transform that removes onlyGameTestsAspectsRefinedStorage.Test plan
d8314fcpreserving IE typed-plan / ID-IC / 9 MiB gateb9a778386f4159bfaca32c9a5d599399e3ce36a6