Skip to content

feat(alchemy): interactive cauldron brewing, heat control, and potion purity engine - #192

Open
angelTomo9 wants to merge 2 commits into
Bitcoindefi:mainfrom
angelTomo9:feat-alchemy-cauldron-1787767259634
Open

feat(alchemy): interactive cauldron brewing, heat control, and potion purity engine#192
angelTomo9 wants to merge 2 commits into
Bitcoindefi:mainfrom
angelTomo9:feat-alchemy-cauldron-1787767259634

Conversation

@angelTomo9

Copy link
Copy Markdown

Summary

Implements an interactive alchemy cauldron brewing engine for OpenAO MMORPG.

Features

  • Stepwise reagent mixing into cauldron sessions
  • Temperature level monitoring (Underheated, Optimal, Overheated)
  • Directional stirring cycles to stabilize brewing reactions
  • Dynamic output potion purity tiers (Perfect Elixir down to Ruined Sludge)
  • Full unit test coverage under Vitest

Comment thread api/src/lib/alchemyBrewingCauldron.ts Outdated
Comment thread api/src/lib/alchemyBrewingCauldron.ts
Comment thread api/src/tests/alchemyBrewingCauldron.test.ts
@leocagli

Copy link
Copy Markdown
Collaborator

Freno acá antes de que sigas invirtiendo tiempo, porque el problema no es el código.

El estado del repositorio

100 PR abiertos
 42 de Rodrigoue9
 41 tuyos

Ochenta y tres de dos personas. Y hay siete títulos que existen dos veces con autores
distintos: los dos escribieron lo mismo sin saberlo. Ese trabajo duplicado no lo puede
recuperar nadie.

Ninguno cita una issue

Revisé los 26 que abriste hoy y ninguno referencia una issue del repositorio. Y las
funcionalidades que traen (pesca, minería, durabilidad, clima, ciclo día y noche, guild
wars, alquimia) no tienen issue abierta. Busqué: cero resultados para stamina,
fishing, durability, mining, weather, guild y spell.

Las 29 issues abiertas son de otra cosa: el editor de mapas por etapas, soporte mobile,
reconexión de WebSocket, el endpoint de ranking, y migraciones de schema.

O sea que esto es trabajo real sobre cosas que nadie pidió, y por lo tanto no se puede
acreditar a ninguna issue ni a ninguna campaña.

Lo que dice el CONTRIBUTING

Se subió hoy a las 03:02, unas horas antes de tu primera PR de esta tanda, así que es
probable que no lo hayas visto:

Comentá la issue que querés con un plan concreto, esperá a que te la asignen, y recién
ahí abrí la PR con Closes #N.

No es burocracia. Es exactamente lo que evita que dos personas escriban lo mismo, que es
lo que pasó siete veces acá.

Qué te propongo

Elegí una issue abierta que te interese, comentala con qué archivo vas a tocar y cómo
lo vas a verificar, y te la asigno. Con una PR enfocada sobre una issue asignada vas a
llegar más lejos que con cuarenta sin dueño.

Y decime qué querés hacer con las que ya están abiertas. Si hay alguna que sí corresponde
a una issue existente, decime cuál y la miro primero.

Comment on lines +32 to +46

it("rejects brewing when player lacks required alchemy skill", () => {
const session = AlchemyBrewingCauldronEngine.startSession("elixir_of_strength")!; // Requires 60
AlchemyBrewingCauldronEngine.addIngredient(session, "ogre_blood");
AlchemyBrewingCauldronEngine.addIngredient(session, "mountain_sage");
AlchemyBrewingCauldronEngine.addIngredient(session, "dragon_scale_dust");

const result = AlchemyBrewingCauldronEngine.finishBrew(session, 30); // Skill only 30
expect(result.success).toBe(false);
expect(result.purityPercent).toBe(10);
expect(result.reason).toContain("Insufficient Alchemy skill");
});

it("handles null session for unknown recipes and finishBrew invalid session", () => {
const nullSession = AlchemyBrewingCauldronEngine.startSession("non_existent_recipe");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Quality: Commit drops test coverage for heat/stir-direction penalties

The removed test ("downgrades purity if cauldron overheats and stirring direction is reversed") was the only case exercising the overheated/underheated badHeatTicks penalty (L163-164) and the wrong-requiredStirDirection -25 penalty (L152-154). Its replacements all use the correct stir direction and either optimal or zero heat, so those two penalty branches are now completely untested and regressions in the core purity math would pass CI. Add a test that overheats/underheats and reverses stir direction to assert the expected purity reduction.

Was this helpful? React with 👍 / 👎

Comment on lines +147 to +149
// Skill gives up to +10 bonus purity for master alchemists (skill 100) vs novice
const skillBonus = Math.floor((skill - recipe.minAlchemySkill) / 5);
let purity = 100 + skillBonus;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: Skill bonus is uncapped and comment overstates the max

The comment claims "up to +10 bonus purity for master alchemists (skill 100)", but Math.floor((skill - recipe.minAlchemySkill) / 5) yields +13 for greater_mana_potion (minSkill 35) and +8 for elixir_of_strength (minSkill 60) at skill 100, so the bonus is neither capped at 10 nor consistent across recipes. Either clamp the bonus (e.g. Math.min(10, ...)) or fix the comment to match the actual formula.

Was this helpful? React with 👍 / 👎

Comment on lines +158 to +160
// Heat penalty: If optimal heat was NEVER applied, heavily penalize cold brew (-50)
if (session.heatExposureTicks.optimal === 0) {
purity -= 50;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: finishBrew ignores recipe.targetHeat, hardcodes OPTIMAL check

The unheated-brew penalty checks heatExposureTicks.optimal === 0 regardless of the recipe's targetHeat field, which is otherwise never read. Both current recipes target OPTIMAL so there is no visible bug, but any future recipe with a non-OPTIMAL targetHeat would be scored against the wrong heat band. Drive the penalty off recipe.targetHeat to keep the field meaningful.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 27, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 3 resolved / 6 findings

Adds an interactive alchemy cauldron brewing and purity engine, but drops test coverage for heat and stir-direction penalties, contains an uncapped skill bonus that diverges from its documentation, and hardcodes the optimal heat check in finishBrew.

⚠️ Quality: Commit drops test coverage for heat/stir-direction penalties

📄 api/src/tests/alchemyBrewingCauldron.test.ts:32-46 📄 api/src/lib/alchemyBrewingCauldron.ts:152-164

The removed test ("downgrades purity if cauldron overheats and stirring direction is reversed") was the only case exercising the overheated/underheated badHeatTicks penalty (L163-164) and the wrong-requiredStirDirection -25 penalty (L152-154). Its replacements all use the correct stir direction and either optimal or zero heat, so those two penalty branches are now completely untested and regressions in the core purity math would pass CI. Add a test that overheats/underheats and reverses stir direction to assert the expected purity reduction.

💡 Quality: Skill bonus is uncapped and comment overstates the max

📄 api/src/lib/alchemyBrewingCauldron.ts:147-149

The comment claims "up to +10 bonus purity for master alchemists (skill 100)", but Math.floor((skill - recipe.minAlchemySkill) / 5) yields +13 for greater_mana_potion (minSkill 35) and +8 for elixir_of_strength (minSkill 60) at skill 100, so the bonus is neither capped at 10 nor consistent across recipes. Either clamp the bonus (e.g. Math.min(10, ...)) or fix the comment to match the actual formula.

💡 Quality: finishBrew ignores recipe.targetHeat, hardcodes OPTIMAL check

📄 api/src/lib/alchemyBrewingCauldron.ts:158-160

The unheated-brew penalty checks heatExposureTicks.optimal === 0 regardless of the recipe's targetHeat field, which is otherwise never read. Both current recipes target OPTIMAL so there is no visible bug, but any future recipe with a non-OPTIMAL targetHeat would be scored against the wrong heat band. Drive the penalty off recipe.targetHeat to keep the field meaningful.

✅ 3 resolved
Edge Case: Brewing with no heat applied still yields a Perfect Elixir

📄 api/src/lib/alchemyBrewingCauldron.ts:146-160
finishBrew only subtracts purity for recorded underheated/overheated ticks (session.heatExposureTicks). A player who never calls adjustHeat leaves all tick counters at 0, so a brew that was never heated at all scores 100 purity and returns PERFECT_ELIXIR — despite the recipe requiring targetHeat OPTIMAL. The engine never rewards reaching the target heat or penalizes the total absence of optimal exposure. Fix by requiring a minimum number of optimal ticks (e.g. penalize when heatExposureTicks.optimal is below a threshold) or by validating currentHeat/target-heat exposure before granting high purity.

Quality: Clamped skill value computed but never affects purity

📄 api/src/lib/alchemyBrewingCauldron.ts:120-134
In finishBrew, skill is clamped to [1,100] and used only for the minAlchemySkill gate; it never influences purityPercent or qualityTier. Two players above the skill threshold produce identical results regardless of skill. If skill is meant to scale output quality, factor it into the purity calculation; otherwise the Math.min/Math.max clamp is dead work and could be simplified.

Quality: Missing tests for skill gate, invalid recipe, and elixir recipe

📄 api/src/tests/alchemyBrewingCauldron.test.ts:1-15 📄 api/src/lib/alchemyBrewingCauldron.ts:120-128
The test file covers only greater_mana_potion happy path, wrong ingredients, and overheat. Untested branches include the insufficient-skill path (skill < minAlchemySkill returning purity 10), startSession returning null for an unknown recipeId, finishBrew with an invalid recipeId, and the elixir_of_strength recipe (COUNTER_CLOCKWISE, 5 cycles). Add cases for these to lock in the scoring/gating logic.

🤖 Prompt for agents
Code Review: Adds an interactive alchemy cauldron brewing and purity engine, but drops test coverage for heat and stir-direction penalties, contains an uncapped skill bonus that diverges from its documentation, and hardcodes the optimal heat check in `finishBrew`.

1. ⚠️ Quality: Commit drops test coverage for heat/stir-direction penalties
   Files: api/src/tests/alchemyBrewingCauldron.test.ts:32-46, api/src/lib/alchemyBrewingCauldron.ts:152-164

   The removed test ("downgrades purity if cauldron overheats and stirring direction is reversed") was the only case exercising the overheated/underheated `badHeatTicks` penalty (L163-164) and the wrong-`requiredStirDirection` -25 penalty (L152-154). Its replacements all use the correct stir direction and either optimal or zero heat, so those two penalty branches are now completely untested and regressions in the core purity math would pass CI. Add a test that overheats/underheats and reverses stir direction to assert the expected purity reduction.

2. 💡 Quality: Skill bonus is uncapped and comment overstates the max
   Files: api/src/lib/alchemyBrewingCauldron.ts:147-149

   The comment claims "up to +10 bonus purity for master alchemists (skill 100)", but `Math.floor((skill - recipe.minAlchemySkill) / 5)` yields +13 for greater_mana_potion (minSkill 35) and +8 for elixir_of_strength (minSkill 60) at skill 100, so the bonus is neither capped at 10 nor consistent across recipes. Either clamp the bonus (e.g. `Math.min(10, ...)`) or fix the comment to match the actual formula.

3. 💡 Quality: finishBrew ignores recipe.targetHeat, hardcodes OPTIMAL check
   Files: api/src/lib/alchemyBrewingCauldron.ts:158-160

   The unheated-brew penalty checks `heatExposureTicks.optimal === 0` regardless of the recipe's `targetHeat` field, which is otherwise never read. Both current recipes target OPTIMAL so there is no visible bug, but any future recipe with a non-OPTIMAL `targetHeat` would be scored against the wrong heat band. Drive the penalty off `recipe.targetHeat` to keep the field meaningful.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Important

Your trial ends in 5 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

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.

2 participants