8000 fix(inventory): add authoritative light extinguish by Mnehmos · Pull Request #105 · Mnehmos/mnehmos.rpg.mcp · GitHub
[go: up one dir, main page]

Skip to content

fix(inventory): add authoritative light extinguish - #105

Merged
Mnehmos merged 1 commit into
mainfrom
codex/light-source-extinguish
Aug 25, 2026
Merged

Mnehmos merged 1 commit into
mainfrom
codex/light-source-extinguish

Conversation

@Mnehmos
@Mnehmos Mnehmos commented Aug 25, 2026 •
Copy link
Copy Markdown
Owner

Summary\n- add inventory_manage action �xtinguish (aliases put_out and douse) for recognized torch/lantern sources\n- deactivate the authoritative custom effect while preserving source item provenance and inventory\n- make repeated extinguish calls idempotent and explicit\n\n## Evidence\n-

pm test (2,275 passed, 7 skipped)\n-
pm run build passes\n- focused inventory suite: 34 passed\n\nCloses #104\nRefs Mnehmos/rpg-mcp-live#268

Summary by CodeRabbit

  • New Features
    • Added an inventory action to extinguish active light sources, such as torches.
    • Reports when a light source is already extinguished.
    • Repeated extinguish requests are handled safely without changing the result.

@Mnehmos
Mnehmos merged commit 494f649 into main Aug 25, 2026
3 of 4 checks passed
@Mnehmos
Mnehmos deleted the codex/light-source-extinguish branch August 25, 2026 04:19
@repowise-bot
repowise-bot Bot commented Aug 25, 2026
Copy link
Copy Markdown

✅ Health of changed files: 5.3 (unchanged)
⚠️ Change risk: moderate, riskier than 50% of this repo's commits.

📋 At a glance
2 hotspots touched · 1 new finding introduced · 2 files with recent fix history · 10 dead-code findings.

Files & modules (2)
  • src (1 file)
    • .../consolidated/inventory-manage.ts
  • tests (1 file)
    • .../consolidated/inventory-manage.test.ts

✅ Health gate: passed

📌 Before you merge

  • Run .../consolidated/open5e-authority.test.ts: they import the changed files
🔎 More signals (3)

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (1 with dependents)"]
    f_src_server_consolidated_inventory_manage_ts[".../consolidated/inventory-manage.ts 🔥"]:::changed
  end
  f_src_server_consolidated_batch_manage_ts[".../consolidated/batch-manage.ts"]
  f_src_server_consolidated_inventory_manage_ts --> f_src_server_consolidated_batch_manage_ts
  f_src_server_consolidated_index_ts[".../consolidated/index.ts"]
  f_src_server_consolidated_inventory_manage_ts --> f_src_server_consolidated_index_ts
  t_tests_server_consolidated_open5e_authority_test_ts(["✅ .../consolidated/open5e-authority.test.ts"]):::guard
  t_tests_server_consolidated_open5e_authority_test_ts -.-> f_src_server_consolidated_inventory_manage_ts
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Loading

Solid arrows: code that imports the changed files (2 direct dependents, from the last indexed snapshot). Dashed: history/tests.

🔥 Hotspots touched (2)

  • .../consolidated/inventory-manage.test.ts: 4 commits/90d, 0 dependents
  • .../consolidated/inventory-manage.ts: 7 commits/90d, 4 dependents

💀 Dead code (10 findings)

  • 💀 .../consolidated/inventory-manage.ts ACTIONS (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts GiveSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts RemoveSchema (confidence 0.65)
7 more
  • 💀 .../consolidated/inventory-manage.ts TransferSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts UseSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts EquipSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts UnequipSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts GetSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts GetDetailedSchema (confidence 0.65)
  • 💀 .../consolidated/inventory-manage.ts definitions (confidence 0.65)

📊 See the full report for this PR
Your repo map with this PR's blast radius lit up, every caller of the contracts it changes, and health before and after. No sign-in. · ⭐ Star Repowise · 📥 Install bot · Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot · Updated 2026-08-25 04:19 UTC

@coderabbitai
coderabbitai Bot commented Aug 25, 2026 •
Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ecdaa5f8-dd38-425f-b8c0-5ff5962b293e

📥 Commits

Reviewing files that changed from the base of the PR and between ce4c9f7 and d44dc11.

📒 Files selected for processing (2)
  • src/server/consolidated/inventory-manage.ts
  • tests/server/consolidated/inventory-manage.test.ts

📝 Walkthrough

Walkthrough

The consolidated inventory tool adds an extinguish action for active torches and lanterns. It validates the item, deactivates its active light-source effect, reports repeated calls as already extinguished, and preserves effect metadata.

Changes

Inventory extinguish action

Layer / File(s) Summary
Action contract and registration
src/server/consolidated/inventory-manage.ts
The tool adds the extinguish action, required characterId and itemId inputs, aliases, and description text.
Extinguish execution and validation
src/server/consolidated/inventory-manage.ts, tests/server/consolidated/inventory-manage.test.ts
The handler validates light-source items, deactivates active effects through effectsRepo.deactivate, formats the result, and tests idempotency, metadata, provenance, and inventory quantity.

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

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant InventoryManage
  participant effectsRepo
  participant ResponseFormatter
  Caller->>InventoryManage: request extinguish with characterId and itemId
  InventoryManage->>InventoryManage: validate light-source item
  InventoryManage->>effectsRepo: find active effect by target and name
  effectsRepo-->>InventoryManage: active effect or no effect
  InventoryManage->>effectsRepo: deactivate active effect
  InventoryManage->>ResponseFormatter: return extinguish result
  ResponseFormatter-->>Caller: render extinguished or already-extinguished status
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/light-source-extinguish

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d44dc1134b

ℹ️ 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".

Comment on lines +463 to +464
const effectName = `Light source: ${item.name}`;
const activeEffect = effectsRepo.findByTargetAndName(params.characterId, 'character', effectName);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Match the persisted light by source item ID

When two light-source templates share a name, extinguishing the never-lit template will find and deactivate the other template's effect; similarly, item_manage.update can rename a lit template, after which this lookup reports it as already extinguished while the authoritative effect remains active. The effect already persists the stable item identity in source_entity_id, so the lookup should match the target and that ID rather than the mutable, non-unique display name.

Useful? React with 👍 / 👎.

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

0