diff --git a/src/bcbench/agent/claude/agent.py b/src/bcbench/agent/claude/agent.py index 60f36cff9..8e4f42c70 100644 --- a/src/bcbench/agent/claude/agent.py +++ b/src/bcbench/agent/claude/agent.py @@ -17,7 +17,7 @@ from bcbench.dataset import BaseDatasetEntry from bcbench.exceptions import AgentError, AgentTimeoutError from bcbench.logger import get_logger -from bcbench.operations import setup_agent_skills, setup_custom_agent, setup_instructions_from_config +from bcbench.operations import setup_agent_playbooks, setup_agent_skills, setup_custom_agent, setup_instructions_from_config from bcbench.types import AgentHarness, AgentMetrics, AgentRuntimeConfig, EvaluationCategory, ExperimentConfiguration, PluginConfig logger = get_logger(__name__) @@ -65,6 +65,7 @@ def run_claude_code( instructions_enabled: bool = setup_instructions_from_config(claude_config, entry, repo_path, harness=AgentHarness.CLAUDE) skills_enabled: bool = setup_agent_skills(claude_config, entry, repo_path, harness=AgentHarness.CLAUDE) custom_agent: str | None = setup_custom_agent(claude_config, entry, repo_path, harness=AgentHarness.CLAUDE) + playbooks = setup_agent_playbooks(claude_config, entry, repo_path, harness=AgentHarness.CLAUDE, custom_agent=custom_agent) plugins: list[tuple[PluginConfig, Path]] = resolve_config_plugins(claude_config, allow_copilot_manifest=False) config = ExperimentConfiguration( @@ -73,6 +74,10 @@ def run_claude_code( custom_instructions=instructions_enabled, skills_enabled=skills_enabled, custom_agent=custom_agent, + playbooks_enabled=playbooks.enabled, + playbook_mode=playbooks.mode, + playbook_revision=playbooks.revision, + playbook_id=playbooks.playbook_id, plugins=[plugin.record for plugin, _ in plugins] or None, ) diff --git a/src/bcbench/agent/copilot/agent.py b/src/bcbench/agent/copilot/agent.py index 9ee557db0..598813383 100644 --- a/src/bcbench/agent/copilot/agent.py +++ b/src/bcbench/agent/copilot/agent.py @@ -18,7 +18,7 @@ from bcbench.dataset import BaseDatasetEntry from bcbench.exceptions import AgentError, AgentTimeoutError from bcbench.logger import get_logger -from bcbench.operations import setup_agent_skills, setup_custom_agent, setup_instructions_from_config +from bcbench.operations import setup_agent_playbooks, setup_agent_skills, setup_custom_agent, setup_instructions_from_config from bcbench.types import AgentHarness, AgentMetrics, AgentRuntimeConfig, EvaluationCategory, ExperimentConfiguration, PluginConfig logger = get_logger(__name__) @@ -62,6 +62,7 @@ def run_copilot_agent( instructions_enabled: bool = setup_instructions_from_config(copilot_config, entry, repo_path, harness=AgentHarness.COPILOT) skills_enabled: bool = setup_agent_skills(copilot_config, entry, repo_path, harness=AgentHarness.COPILOT) custom_agent: str | None = setup_custom_agent(copilot_config, entry, repo_path, harness=AgentHarness.COPILOT) + playbooks = setup_agent_playbooks(copilot_config, entry, repo_path, harness=AgentHarness.COPILOT, custom_agent=custom_agent) plugins: list[tuple[PluginConfig, Path]] = resolve_config_plugins(copilot_config, allow_copilot_manifest=True) config = ExperimentConfiguration( @@ -70,6 +71,10 @@ def run_copilot_agent( custom_instructions=instructions_enabled, skills_enabled=skills_enabled, custom_agent=custom_agent, + playbooks_enabled=playbooks.enabled, + playbook_mode=playbooks.mode, + playbook_revision=playbooks.revision, + playbook_id=playbooks.playbook_id, plugins=[plugin.record for plugin, _ in plugins] or None, ) diff --git a/src/bcbench/agent/shared/__init__.py b/src/bcbench/agent/shared/__init__.py index 5d0a5bbb4..d0bf04b5f 100644 --- a/src/bcbench/agent/shared/__init__.py +++ b/src/bcbench/agent/shared/__init__.py @@ -6,12 +6,20 @@ from bcbench.agent.shared.mcp_gateway import start_bc_mcp_gateway from bcbench.agent.shared.plugin import resolve_config_plugins from bcbench.agent.shared.prompt import build_prompt +from bcbench.playbooks import PlaybookDefinition, PlaybookManifest, PlaybookSetup, load_playbook_manifest, playbook_revision, resolve_playbook_for_area, resolve_playbook_for_paths __all__ = [ + "PlaybookDefinition", + "PlaybookManifest", + "PlaybookSetup", "agent_subprocess_env", "build_al_lsp_plugin", "build_mcp_config", "build_prompt", + "load_playbook_manifest", + "playbook_revision", "resolve_config_plugins", + "resolve_playbook_for_area", + "resolve_playbook_for_paths", "start_bc_mcp_gateway", ] diff --git a/src/bcbench/agent/shared/config.yaml b/src/bcbench/agent/shared/config.yaml index 210f64f56..009a52252 100644 --- a/src/bcbench/agent/shared/config.yaml +++ b/src/bcbench/agent/shared/config.yaml @@ -153,6 +153,13 @@ agents: enabled: true name: fix-bug +# Controls area-specific playbooks inside the selected custom agent. +# discover lets the agent select from confirmed source paths during investigation. +# selected lets the harness select from dataset metadata.area before investigation. +playbooks: + enabled: true + mode: discover + # controls loading agent plugins for the run, only enabled entries are validated and loaded. # Each plugin gets its own entry, and is passed to the CLI via `--plugin-dir` (session-scoped). # name: plugin name; also how it is recorded on the result as "@" diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug.agent.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug.agent.md index edb33977d..cb13c7aae 100644 --- a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug.agent.md +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug.agent.md @@ -13,6 +13,13 @@ the change, implement it, and validate it with the AL tools when they are availa does not fetch work items, does not create branches, does not commit, and does not open pull requests. Its only output is the change in the working tree plus a short report. +## Execution model + +Execute the workflow directly in this agent. Do not use the Agent tool. Do not delegate any part of +the task to a subagent. Do not start background work. This is an unattended, non-interactive +run: returning ends the session immediately, so all investigation, edits, and validation must finish +before the final response. + ## Step 1: Locate the support files and read the rules Set `AGENT_ROOT` from the harness running this agent: @@ -27,6 +34,10 @@ does not exist, stop and report the missing path. Read `AGENT_ROOT/rules.md` before acting. It defines the hard constraints, how to use the AL tools, and how to fail. +If `AGENT_ROOT/playbooks/selected.yaml` exists, read it and then read the playbook named by its +`file` field before extracting the task. Read no other area playbook. If the marker names a missing +file, stop and report that the agent package is incomplete. + ## Step 2: Extract the task From the user prompt, identify the issue description, the repository path, and any reproduction @@ -44,3 +55,5 @@ Read `AGENT_ROOT/workflow.md` and execute every step of it. | `AGENT_ROOT/rules.md` | Always, before acting | | `AGENT_ROOT/workflow.md` | Always, as Step 3 | | `AGENT_ROOT/troubleshooting.md` | When a build, publish, or test call behaves in a way the workflow does not cover | +| `AGENT_ROOT/playbooks/manifest.yaml` | During discover-mode routing | +| `AGENT_ROOT/playbooks/selected.yaml` | When present; identifies the selected-mode playbook | diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/costing.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/costing.md new file mode 100644 index 000000000..eb3780092 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/costing.md @@ -0,0 +1,399 @@ +# Costing / Inventory Valuation Bug-Fix Playbook (Business Central) +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + + +> Domain knowledge for the automated bug-fix agent working on **Costing / Inventory +> Valuation** bugs in `this repo` (base-app W1 + localized layer copies): +> item costing, cost adjustment, Value Entries, item application, exact-cost +> reversing, revaluation, item charges, additional-reporting-currency costing, +> manufacturing cost shares, and inventory valuation reports. +> +> This is **not** an AL language guide. It only carries costing/valuation-specific +> knowledge and concrete lessons — what worked and what did not — from past fixes +> and reviews. Generic AL rules, style advice, analyzer findings, CI hygiene, and +> anything the compiler/publisher catches on its own are deliberately left out. + +--- + +## §0 The area in one picture + +Costing is the ledger layer underneath inventory posting. The same defect often +appears as a purchase/posting bug, an undo bug, a report bug, or a G/L +reconciliation bug, but the invariant is the same: + +1. **Item Ledger Entry (ILE, table 32)** records quantity and application state. + It carries entry numbers, invoiced quantity, exact-cost fields, and summary + cost FlowFields. +2. **Value Entry (table 5802)** records the value attached to an ILE. It is the + first place to check when a user says inventory value, expected/actual cost, + item-charge cost, ACY amount, valuation date, or revaluation is wrong. +3. **Item Application Entry (table 339)** links inbound and outbound ILEs. FIFO, + average, specific costing, exact-cost reversing, and undo paths depend on this + chain staying correct. +4. **Cost adjustment** reopens the chain after posting. It marks entries to adjust, + calculates average/standard/FIFO effects, creates adjustment Value Entries, and + later posts the cost to G/L. +5. **Reports** (`Inventory Valuation`, `Cost Shares Breakdown`, Power BI report + entry points) should read the same Value Entry/ILE truth without losing filters, + event semantics, or amount formatting. + +**First classify the bug by ledger effect, not by UI entry point.** A purchase +invoice defect can be an item-charge Value Entry defect. A currency +posting defect can be an inventory valuation/G/L reconciliation defect. A report +modernization can silently bypass the old Value Entry filters that extensions +relied on. + +### Key objects you will keep meeting (current IDs) + +IDs below are current for the costing and inventory valuation area. Most objects are base app; several localized layers carry byte-for- +byte or near-byte-for-byte copies of posting code. + +| Object | Type | Current ID | Main path | Why it matters | +|---|---|---:|---|---| +| `Item` | table | **27** | W1 BaseApp Inventory/Item | Costing method, unit/standard cost, inventory value zero, FlowFields. | +| `Item Ledger Entry` | table | **32** | W1 + layer copies | Quantity, invoiced quantity, exact-cost/application state, ILE summary costs. | +| `Value Entry` | table | **5802** | W1 + layer copies | Actual/expected cost amounts, ACY amounts, valuation date, item-charge marker. | +| `Item Application Entry` | table | **339** | W1 + layer copies | Cost application chain, exact-cost reversing, unapply/undo. | +| `Item Application Entry History` | table | **343** | W1 | Historical application trace. | +| `Item Journal Line` | table | **83** | W1 + layer copies | Posting input for CU22; source currency and cost fields are read here. | +| `Item Jnl.-Post Line` | codeunit | **22** | W1 + APAC/CH/ES/IT/RU copies | Creates ILE/Value Entries; ACY costing fixes landed here. | +| `Purch.-Post` | codeunit | **90** | W1 + many layer copies | Posts item-charge Value Entries through receipt distribution paths. | +| `Undo Posting Management` | codeunit | **5817** | W1 | Shared undo posting; owns `PostItemJnlLineAppliedToList`. | +| `Undo Sales Shipment Line` | codeunit | **5815** | W1/RU | Drop-shipment item-application unapply seam. | +| `Undo Purchase Receipt Line` | codeunit | **5813** | W1 | Purchase exact-cost reversing/undo entry point. | +| `Undo Return Shipment Line` | codeunit | **5814** | W1 | Return-side undo. | +| `Undo Transfer Shipment` | codeunit | **9030** | W1 | Transfer undo path; review asked for direct coverage when errors changed. | +| `Inventory Adjustment` | codeunit | **5895** | W1 + APAC/RU copies | Cost-adjustment implementation behind the interface. | +| `Inventory Adjustment Handler` | codeunit | **5894** | W1 | Orchestrates adjustment runs. | +| `ItemCostManagement` | codeunit | **5804** | W1/APAC/RU copies | Average/precise cost calculation and open inbound ILE filtering. | +| `Cost Adjustment Params Mgt.` | codeunit | **5824** | W1 | Parameterization around adjustment runs. | +| `Inventory Posting To G/L` | codeunit | **5802** | W1 | Posts cost adjustment/value entries to G/L. | +| `Post Inventory Cost to G/L` | report | **1002** | W1 | Batch report for inventory cost to G/L. | +| `Post Inventory Cost to G/L` | codeunit | **2846** | W1 | Codeunit wrapper for the same posting. | +| `Adjust Cost - Item Entries` | report | **795** | W1 | User-facing cost adjustment run. | +| `Cost Adjustment Overview` | page | **5801** | W1 | Cost-adjustment page/actions. | +| `Avg. Cost Adjmt. Entry Point` | table | **5804** | W1 | Average-cost valuation-date entry points. | +| `Avg. Cost Adjmt. Entry Points` | page | **5815** | W1 | Diagnostic/user view of average-cost entry points. | +| `Average Cost Calc. Overview` | table/page | **5847** | W1 | Average-cost diagnostic buffer. | +| `Inventory Adjmt. Entry (Order)` | table | **5896** | W1 | Manufacturing/order cost adjustment buffer. | +| `Inventory Valuation` | report | **1001** | W1 | Main W1 valuation report; existing Value Entry events matter. | +| `Inventory Valuation` | report | **10139** | NA | NA copy with separate event parity needs. | +| `Cost Shares Breakdown` | report | **5848** | W1 | Manufacturing cost-share report; WIP/filter/event bugs. | +| `Standard Cost Worksheet` | table/page | **5841** | W1 | Standard-cost worksheet data and UI. | +| `Calculate Standard Cost` | codeunit | **5812** | W1 | Standard-cost calculation engine. | +| `Standard Cost Worksheet` | report | **5855** | W1 | Standard-cost calculation/update report. | +| `Calculate Inventory Value` | report | **5899** | W1 | Revaluation journal population. | +| `Revaluation Journal` | page | **5803** | W1 | Revaluation entry point. | +| `Item Charge` | table | **5800** | W1 | Item-charge master. | +| `Item Charge Assignment (Purch)` | table/page | **5805** | W1 | Purchase item-charge assignment and posted-cost distribution. | +| `G/L - Item Ledger Relation` | table/page | **5823** | W1 | Reconciliation link from inventory value to G/L. | +| `Invt. Posting Buffer` | table | **48** | W1 | Cost-to-G/L staging buffer. | +| `Inventory Setup` | table | **313** | W1 | Automatic/expected cost posting, average-cost setup. | +| `Stockkeeping Unit` | table | **5700** | W1 | SKU-level standard/unit costs. | +| `Capacity Ledger Entry` | table | **5832** | W1 | Manufacturing cost-share capacity cost source. | +| `Prod. Order Line` | table | **5406** | W1 | Production item/source for WIP and standard-cost cases. | +| `Purch. Rcpt. Line` | table | **121** | W1 | Receipt-line source for item-charge Value Entries. | +| `Purchase Line` | table | **39** | W1 | Charge-assignment target line and receipt state. | + +### Reliable markers and fields + +Use these fields to reason about the defect. Do not replace them with nearby +fields unless the current fix proves the nearby field is the correct one. + +| Table | Field(s) | Current IDs | Rule | +|---|---|---:|---| +| `Value Entry` | `"Item Ledger Entry No."`, `"Valued Quantity"`, `"Invoiced Quantity"` | 11, 12, 14 | A cost fix is not proved by existence of a Value Entry. Prove the entry is on the right ILE and has the right valued/invoiced quantity. An earlier fix caught this. | +| `Value Entry` | `"Cost Amount (Actual)"`, `"Cost Amount (Expected)"` | 43, 151 | Actual vs expected legs can need separate formulas and tests. An earlier fix landed only after both receipt and invoice legs were asserted. | +| `Value Entry` | `"Cost Amount (Actual) (ACY)"`, `"Cost Amount (Expected) (ACY)"` | 68, 156 | ACY inventory value must reconcile with G/L Additional-Currency Amount when document currency = ARC. We hit this before. | +| `Value Entry` | `"Expected Cost"`, `"Item Charge No."`, `"Partial Revaluation"`, `"Valuation Date"` | 98, 99, 102, 104 | These separate expected/actual, item-charge, partial-revaluation, and valuation-date paths. Test the branch you touch. | +| `Item Ledger Entry` | `"Invoiced Quantity"`, `"Applies-to Entry"`, `"Completely Invoiced"`, `"Applied Entry to Adjust"` | 14, 28, 5800, 5802 | Partial invoicing and exact-cost/application fixes should use state that remains valid until fully invoiced. An earlier fix changed the lookup from `Invoiced Quantity = 0` to `Completely Invoiced = false`. | +| `Item Ledger Entry` | `"Cost Amount (Expected)"`, `"Cost Amount (Actual)"`, ACY variants | 5803, 5804, 5806, 5807 | These are summary amounts. Trace back to Value Entries when the amount is wrong. | +| `Item` | `"Costing Method"`, `"Unit Cost"`, `"Standard Cost"`, `"Inventory Value Zero"` | 21, 22, 24, 5409 | Costing method drives which application/adjustment rule is in play. | +| `Inventory Setup` | `"Automatic Cost Adjustment"`, `"Expected Cost Posting to G/L"`, `"Average Cost Calc. Type"`, `"Average Cost Period"` | 30, 5800, 5804, 5805 | Average/expected cost behavior depends on setup; do not hard-code a single tenant shape. | +| `Item Charge Assignment (Purch)` | quantity/amount assignment fields | table 5805 | For item charges, the invariant includes both posted Value Entry and `Qty. Assigned = Quantity Invoiced` on the charge line, as shown by earlier fixes. | + +--- + +## §1 The loop that worked + +1. **Start from the posted ledger symptom.** Identify the exact ILE and Value Entry + that should change. For reports, identify the Value Entry filter/sum the report + used before the change. An earlier report change was risky because the new FlowField + path bypassed old `Value Entry` filter events. +2. **Classify the branch: actual vs expected, direct item vs item charge, W1 vs + localized layer, receipt vs invoice, single receipt vs partial receipts.** Most + bad fixes covered the easy branch and missed the sibling branch. +3. **Find the analogous correct path and mirror it.** `PostItemChargePerRcpt` was + the model for separately invoiced charges in an earlier fix. Existing W1 + `CalcPosShares()` was the model the APAC copy failed to reach in an earlier fix. +4. **When there are layer copies, patch and test the copies intentionally.** CU22 + ACY logic existed in APAC/CH/ES/IT/RU/W1; APAC had an extra source-currency + branch, so a mechanically identical helper call was still unreachable. +5. **For rounding/currency bugs, assert exact amounts, not abs/existence.** The + fix should prove `Value Entry` ACY equals the document/ARC amount and reconciles + to G/L; where signs matter, assert signed values. +6. **For application/undo bugs, prove the link, not the message.** Check the item + application or item-entry relation points to the ILE just posted/reversed. An earlier fix used `ItemJnlPostLine.GetItemLedgerEntryNo()` because the old field held + the wrong entry number for subcontracting undo. +7. **For extensibility fixes, place the event at the old calculation seam.** The + publisher must fire after standard filters are set and before `FindSet()`/`CalcSums()` + if the purpose is to let extensions refine the costing set. + +--- + +## §2 Adjust Cost / average-cost calculation bugs + +The corpus is thin on direct `Adjust Cost - Item Entries` product-code defects; +most lived findings are adjacent seams that cost adjustment later consumes. +Treat them as guardrails for the next real adjust-cost bug. + +- **Protect the entry-number allocation window, not just the caller:** CU22 `PostSplitJnlLine` allocates ILE and Value Entry numbers and + later inserts those entries. A `Commit()` inside that window can release locks + while cached entry numbers are still pending, causing duplicate ILE/Value Entry + numbers. The fix shape was `CommitBehavior::Ignore` around the split posting + loop with an opt-out event. The review pushback was about the test: a subscriber + after insertion does not prove the dangerous window. Put the regression `Commit()` call + into `OnBeforeInsertItemLedgEntry` or `OnBeforeInsertValueEntry` so the lock/no- + duplicate invariant is actually tested. +- **Average-cost filter hooks must pass the record by `var`:** + `OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntry` fires + after `OpenInbndItemLedgEntry` has item/open/positive/location/variant filters + and before `FindSet()`. Without `var`, subscribers cannot refine open inbound + ILEs, so the event is functionally useless for average-cost calculation. If the + next average-cost bug is an extension/filter bug, verify the event sits exactly + between standard filters and the read. +- **Cost Adjustment / Item Card action duplication is UI-only:** when a bug mentions Cost Adjustment actions, separate UI discoverability + from valuation logic. An earlier fix only hid base item data actions when Manufacturing + was enabled and left cost adjustment data processing unchanged. Do not infer an + adjust-cost engine bug from duplicated Export/Import actions. +- **Re-enable cost-adjustment tests when the product fix lands:** disabled SCM Inventory Costing IV tests covered ARC + posting and adjustment scenarios. When fixing a costing defect, check whether a + disabled-test entry exists for the exact costing batch/IV scenario and remove + only that entry after the underlying source-currency/costing issue is fixed. + +--- + +## §3 Value Entry ACY / rounding / expected-vs-actual bugs + +This was the densest valuation cluster. The repeated symptom: document currency +is the Additional Reporting Currency (ARC), but CU22 recalculates Value Entry ACY +from LCY using another exchange rate, so inventory valuation no longer reconciles +with G/L Additional-Currency Amount. + +- **Use the document amount only for the real ARC scenario:** the + special path should run when `ItemJnlLine."Source Currency Code"` equals the + non-empty Additional Reporting Currency and there are no cost add-ons. The APAC + copy initially called `ShouldUseDocumentAmountForACY()` only inside + `Source Currency Code = ''`, making the new branch unreachable for the exact bug + condition. In localized posting code, prove the predicate is reachable in each + layer, not just textually present. +- **Cover purchase posting where currency factor differs from posting-date rate:** the minimum test is a purchase posting with document currency = + ARC and a currency factor different from the posting-date exchange rate. Assert + the Value Entry ACY amount equals the source document amount and reconciles with + the G/L Entry Additional-Currency Amount. +- **Expected-cost and actual-cost legs need separate proof:** early + coverage proved expected and actual direct item costs, but later changes still + needed branch-specific proof. If `Expected Cost` can be true on receipt and false + on invoice, assert both legs. Do not assume invoice follows receipt because the + helper name is shared. +- **Item charges are not automatically part of the direct-item ARC shortcut:** adding `ItemJnlLine."Item Charge No." = ''` narrowed + `ShouldUseDocumentAmountForACY()`. That was plausible, but the review blocked + because no item-charge ARC purchase test proved item charges still reconcile. + Any change to this guard must include a purchase item-charge case where document + currency = ARC. +- **Do not drop the per-base-unit ACY rounding residual on the actual leg:** the accepted fix added `RoundingResidualAmountInvdACY`, computed as + invoiced quantity times the per-base-unit ACY unit-cost residual, and used it in + the invoiced/actual leg: `DirCostACY := "Unit Cost (ACY)" * "Invoiced Quantity" + + RoundingResidualAmountInvdACY`. This mirrors the expected leg, but scales by + `"Invoiced Quantity"` instead of `Quantity`. The proving test used a non-base + unit of measure so the per-base-unit ACY cost rounds and would otherwise drop a + residual; it asserted both receipt expected and invoice actual Value Entry ACY + amounts equal the exact document ACY amount. +- **Amount tests must assert exact cost fields, not existence:** for this class, assert `Value Entry."Cost Amount (Expected) + (ACY)"` and/or `"Cost Amount (Actual) (ACY)"`, and assert the G/L Entry + Additional-Currency Amount. A `RecordIsNotEmpty(ValueEntry)` assertion would miss + the whole bug. + +--- + +## §4 Item charges' cost effect + +Item charges are valuation entries. They are not just purchase-document metadata. +When a charge is assigned to an item line, the cost effect must land on the right +receipt ILE(s), with the right quantity and amount. + +- **Separately invoiced item charges must post a Value Entry:** the root defect was: receive the item line first; later post an item- + charge invoice with the target item line's `Qty. to Invoice = 0`; no charge + Value Entry is created, `Qty. Assigned` stays `0`, `Quantity Invoiced` becomes + `1`, and the purchase order cannot be deleted because + `TestField("Qty. Assigned", "Quantity Invoiced")` fails. The correct direction + is to reuse the existing receipt distribution helpers (`PostDistributeItemCharge` + / `PostItemCharge`) from `Purch.-Post` instead of inventing a parallel value- + entry writer. +- **Never `FindFirst()` one receipt for an order-line charge:** the + reviewed fix found `Purch. Rcpt. Line` by `Order No.` + `Order Line No.` and + posted the full charge against the first receipt. That corrupts cost when the + order line was received in multiple partial receipts. Loop all matching receipt + lines and split `Qty. to Assign` / `Amount to Assign` proportionally by each + receipt's `Quantity (Base)`, mirroring the path where a charge is posted per + receipt. +- **The item-charge regression needs two invariants:** assert the + charge Value Entry cost amount and valued quantity, and assert the purchase + charge line has `Qty. Assigned = Quantity Invoiced`. The original symptom was + both a missing valuation entry and an assignment-state mismatch. +- **Return/Credit Memo symmetry is a conscious follow-up, not accidental silence:** the new path intentionally targeted `Order`/`Invoice`. The review + called out that `Return Order`/`Credit Memo` can plausibly suffer the same + separate-invoice assignment bug. If the next bug is on the return side, resolve + return-shipment lines and mirror the sign/quantity handling there; do not reuse + purchase-receipt sign rules blindly. +- **ACY guard changes must include item-charge coverage:** if a CU22 + predicate excludes `"Item Charge No." <> ''`, prove an ARC item-charge purchase + still posts Value Entry ACY amounts that reconcile with G/L. + +--- + +## §5 Cost application / exact-cost reversing / undo + +Application bugs usually look like the wrong entry was chosen, not like no entry +was written. Check the ILE number and application relation before changing amounts. + +- **Partial invoicing must keep the Negative Adjmt. ILE eligible until fully + invoiced:** project consumption through Get Receipt Lines posted the + second partial invoice's Value Entry to the wrong ILE because the lookup only + found a Negative Adjmt. ILE with `Invoiced Quantity = 0`. After the first partial + invoice, that was false even though the entry was not fully invoiced. Use + `Completely Invoiced = false` for this lookup so later partial invoices continue + applying to the correct Negative Adjmt. ILE. +- **Undo relation keys must come from the ILE just posted:** in `Undo Posting Management`.`PostItemJnlLineAppliedToList`, + subcontracting undo filled `TempItemEntryRelation."Item Entry No."` from + `ItemJnlLine."Item Shpt. Entry No."`. For subcontracting, that value can be a + capacity ledger entry number, not the reversing output ILE. Use + `ItemJnlPostLine.GetItemLedgerEntryNo()` from the same global CU22 instance that + posted the line; guard it to the subcontracting/non-zero scenario so non- + subcontracting undo stays unchanged. The tests re-enabled a lot-tracking undo + case because the defect only showed on that sensitive path. +- **Drop-shipment unapply events belong before the application entry read:** `Undo Sales Shipment Line.UnApplyDropShipment` needed an event after + standard filters on `Item Application Entry` and before `FindFirst()`, so + extensions using load fields can add extension fields before the record is read + and later modified/deleted by item application unapply logic. If the next exact- + cost reversing bug is an extension-field/load-field bug, place the seam there. +- **Message-only undo fixes are not valuation fixes:** a + better `NoLinesToReverseErr` on empty Sales/Purchase/Transfer undo selections did + not change posting, application, or valuation state. If a bug is about wrong cost + reversal, do not stop at the selection/error path. + +--- + +## §6 Revaluation and standard-cost worksheet bugs + +The review corpus has little direct revaluation math. The useful lived lessons are +about standard cost and the revaluation entry points that feed Value Entries. + +- **`Calculate Inventory Value` / Revaluation Journal are the revaluation entry + point, but the Value Entry fields prove the fix.** Use report 5899 to populate + the journal and page 5803 to inspect it, but verify the posted result in Value + Entry fields `"Partial Revaluation"`, `"Valuation Date"`, and the actual/ACY + cost fields. Do not claim a revaluation fix from worksheet lines alone. +- **Standard-cost SKU updates must respect the setup/source of SKU costs:** the bug was that single-level capacity/material cost for SKU could be + calculated/overwritten when Manufacturing Setup says SKU manufacturing costs are + loaded separately. The final reviewed change was small (widening a message from + `Text[250]` to `Text`), but the root scenario is the useful rule: standard-cost + worksheet/report fixes must preserve whether SKU manufacturing costs are loaded + separately, and long explanatory messages must not fail the run. +- **Manufacturing cost calculation extension points need all overloads:** expected production-order cost had a normal overload and a non- + inventory-material overload. The first review found the new handled event only + on one overload. The final fix added a separate `OnBeforeCalcProdOrderLineExpCost` + shape for the non-inventory-cost path. If a standard/expected-cost bug has two + calculation overloads, cover both or explicitly prove one cannot run. +- **IT SKU cost events need the item context they actually use:** the + IT `CalcRtngLineCostSKU` path used `MainItem` to resolve subcontractor prices; + the W1 path did not. The event surface had to pass `MainItem` only in the IT + event. For localized standard-cost bugs, do not flatten W1 and IT signatures if + the localized calculation uses extra cost context. + +--- + +## §7 Inventory valuation and manufacturing cost reports + +Report bugs are still costing bugs when they change filters, sums, event seams, or +formatted financial amounts. Treat layouts and navigation as lower risk only when +they demonstrably do not change calculation. + +- **Inventory Valuation report 1001 must not depend on CH-only fields:** + the Excel-layout fixes moved calculation toward Item FlowFields such as `Opening + Bal. ILE Qty.`, `Increases ILE Qty.`, and `Cost Posted To G/L`, but those fields + were added only to the CH Item table while W1 report 1001 read them. If a report + calculation is in W1, the fields/events it reads must exist in W1, not only in a + localization layer. +- **Preserve `Value Entry` filter events when optimizing report sums:** + `OnItemOnAfterGetRecordOnAfterValueEntrySetInitialFilters` and + `OnCalculateItemOnBeforeAssignDecreaseAmounts` let subscribers refine Value + Entry filters before opening/increase/decrease/G/L sums. Replacing the sums with + FlowFields bypassed those subscriber changes. Any performance rewrite of + Inventory Valuation must either keep the old event behavior or add a compatible + replacement before the sums are calculated. +- **Excel financial layouts need amount/quantity formats:** Inventory + Valuation's visible Excel pivot tables and sheets cannot leave amount and + quantity cells as General. Add explicit number formats for LCY amounts and + quantities so decimal precision/separators do not vary by culture. +- **NA Inventory Valuation report 10139 needs event parity with W1:** + extensions could compile against W1 report 1001 events but not the NA report. + The accepted shape added a `SkipItem` event before child ILE processing and a + `Value Entry` filter event after initial filters and before `CalcSums()`. Default + behavior must remain unchanged with no subscriber. +- **Inventory Valuation Power BI placement is navigation-only if report objects do + not change:** moving actions from Finance Manager to Business + Manager did not alter report pages, setup records, posting, financial + calculations, permissions, or event contracts. Do not overfit a valuation engine + fix to a role-center action bug. +- **Cost Shares Breakdown WIP mode must filter before inserting capacity cost rows:** report 5848 already applied Item filters when printing + WIP buffer rows, but capacity ledger entries for unrelated production items were + inserted before that filter. Apply the temporary Item + `CopyFilters(Item)` + + `IsEmpty()` pattern before `InsertCapLedgEntryCostShare()` so an Item filter does + not show unrelated production orders. +- **Cost-share override events must sit before standard share application:** report 5848 needed an event that lets subscribers replace how cost + share applies to capacity and overhead amounts. The default path still adds the + same inventory adjustment order costs, calculates `ShareOfCost` when `OutputQty + <> 0`, and multiplies the same buffer fields. Additive event, no default behavior + change. + +--- + +## §8 Recurring agentic-review findings — fix these *before* opening review + +These are issues the automated reviews repeatedly caught on costing/valuation changes. +Pre-empting them saves review rounds. + +- **A posted Value Entry existing is not enough.** Assert the right ILE, `Valued + Quantity`, `Invoiced Quantity`, `Cost Amount (Actual/Expected)`, ACY fields, and + assignment state relevant to the defect. An earlier fix's first test would have + passed with a wrong amount and wrong receipt distribution. +- **Partial receipts and partial invoices are first-class costing cases.** If the + fix finds one receipt (`FindFirst()`) or only `Invoiced Quantity = 0`, add a multi- + receipt or second-partial-invoice test. Earlier fixes are the pattern. +- **Expected and actual cost legs are separate branches.** For receipt+invoice or + expected-cost posting bugs, assert both `Expected Cost = true` and actual entries + where the bug can hit both. An earlier fix only closed after the actual leg's residual + matched the expected leg. +- **ACY fixes require a reconciliation assertion.** When document currency equals + ARC, assert Value Entry ACY equals the document amount and G/L Additional- + Currency Amount. A currency factor different from the posting-date rate is what + exposes the double-conversion bug. +- **Layer copies are not behaviorally identical just because names match.** APAC's + extra source-currency branch made the new W1-style ACY helper unreachable. Check control flow in every changed layer copy. +- **If a guard excludes item charges, add an item-charge test.** `"Item Charge No." + = ''` in an ACY helper is a financial branch change, not a harmless narrowing. +- **Event requests must prove the subscriber can change the costing set.** Events + for filters need a `var` record and must fire after standard filters but before + `FindSet()`/`FindFirst()`/`CalcSums()`. +- **Report performance rewrites must preserve extension semantics.** Replacing + Value Entry loops with FlowFields can silently bypass old filter events. Extension parity is part of correctness for valuation reports. +- **Excel layouts for valuation/cost reports need explicit formats.** Financial + amount and quantity cells/pivots should not be General. +- **Undo/application fixes must assert the relation points to the entry just + posted or unapplied.** For sensitive undo paths, use the posting codeunit's + authoritative last ILE number or the filtered Item Application Entry, not a + nearby shipment/capacity entry field. +- **Costing test re-enablement should be surgical.** Remove disabled-test metadata + only for fixed ARC posting/adjustment scenarios; leave unrelated still-failing + costing batch tests disabled until their underlying defect is fixed. diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manifest.yaml b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manifest.yaml new file mode 100644 index 000000000..99bdfec95 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manifest.yaml @@ -0,0 +1,27 @@ +playbooks: + - id: warehouse + file: warehouse.md + areas: + - warehouse + paths: + - App/Layers/W1/BaseApp/Warehouse/** + - App/Layers/W1/BaseApp/Inventory/Tracking/** + + - id: manufacturing + file: manufacturing.md + areas: + - manufacturing + paths: + - App/Layers/W1/BaseApp/Manufacturing/** + + - id: costing + file: costing.md + paths: + - App/Layers/W1/BaseApp/Inventory/Costing/** + + - id: subscription-billing + file: subscription-billing.md + areas: + - subscription billing + paths: + - src/Apps/W1/Subscription Billing/App/** diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manufacturing.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manufacturing.md new file mode 100644 index 000000000..b9108ede3 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/manufacturing.md @@ -0,0 +1,426 @@ +# Manufacturing Bug-Fix Playbook (Business Central) +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Manufacturing +> (non-subcontracting)** bugs in this repo's base-app layers: production orders, routings, production BOMs, +> components, capacity, consumption/output, flushing, production journals, +> planning worksheet / MRP, and Assembly only where it overlaps manufacturing +> copy or availability behavior. +> +> The Manufacturing app folder is a shell; the code covered here lives in the +> base-app layer under `src/Layers/W1/BaseApp/Manufacturing/`. +> +> This is **not** an AL language guide. It only carries manufacturing-specific +> knowledge and the concrete lessons — what worked and what didn't — from past +> fixes. Generic AL rules, generic CI hygiene, and anything the compiler, +> publisher, or analyzer rulesets catch on their own are deliberately left out. +> +> **Subcontracting is excluded.** If the bug is about subcontracting purchase +> orders, WIP transfer orders to subcontractors, subcontractor pricing, or the +> legacy IT/W1 app migration seam, switch to the separate Subcontracting playbook. +> This file only cross-references overlap where a production-order/routing fact is +> reusable. + +--- + +## §0 The area in one picture + +Manufacturing is a chain of **definitions → planning → orders → journals/posting +→ ledgers/reports**. Most bugs in this corpus were not "math is hard" bugs; they +were bugs where one link in that chain used a narrower key, stale filter, or +wrong unit than its sibling path. + +1. **Definitions** — `Production BOM Header` / `Production BOM Line` / + `Production BOM Version`, plus `Routing Header` / `Routing Line` / + `Routing Version`. Certification gates bad combinations before they become + production-order refresh failures. +2. **Planning** — MRP / planning worksheet reads Item and SKU production BOMs, + routing cost, low-level codes, and requisition lines. SKU-level BOMs are real + manufacturing setup, not an afterthought. +3. **Production orders** — orders move through Simulated / Planned / Firm Planned + / Released / Finished. Lines carry item/BOM/routing; components carry material + demand; routing lines create capacity needs and capacity ledger entries. +4. **Posting** — consumption and output post through item journals / production + journals into Item Ledger, Value Entry, and Capacity Ledger. The WIP reports + read those posted entries and can be financially wrong even when posting was + correct. +5. **Capacity display** — Work/Machine Center calendars, load pages, Power BI, and + Capacity Ledger visuals all convert from capacity-unit codes to time factors. + A display-only bug can still hide or blank the real capacity picture. + +### Key objects you will keep meeting (current IDs) + +IDs below are current in this repo at playbook creation time. Prefer these +names over stale object IDs from bug text. + +| Object | Type | Current ID | Area | Notes | +|---|---:|---:|---|---| +| `Manufacturing Setup` | table/page | **99000765 / 99000768** | Setup | Order no. series, SKU cost loading, capacity UOM, flushing defaults, no-output finishing. | +| `Production Order` | table | **5405** | Orders | Header status/source; statuses use enum 5405. | +| `Production Order Status` | enum | **5405** | Orders | Simulated, Planned, Firm Planned, Released, Finished. | +| `Prod. Order Line` | table | **5406** | Orders | Item line; carries `Production BOM No.`, `Routing No.`, `Finished Qty. (Base)`, `Scrap %`. | +| `Prod. Order Component` | table | **5407** | Components | Material demand; key filters include Status, Prod. Order No., Prod. Order Line No. | +| `Prod. Order Routing Line` | table | **5409** | Routing/capacity | Operation demand; Previous/Next Operation No. matter for parallel routing. | +| `Prod. Order Capacity Need` | table/page | **5410 / 99000820** | Capacity | Planned operation capacity demand. | +| `Production BOM Header` | table | **99000771** | BOM | Header status and version no. series. | +| `Production BOM Line` | table | **99000772** | BOM/components | Version Code + Line No. filtering is easy to get wrong. | +| `Production BOM Version` | table/page | **99000779 / 99000809** | BOM versions | Certification must validate version lines too. | +| `BOM Status` | enum | **99000771** | BOM | Used by header/version certification gates. | +| `Routing Header` | table | **99000763** | Routing | Has Type Serial/Parallel and version no. series. | +| `Routing Line` | table | **99000764** | Routing | Has Standard Task Code and operation sequencing. | +| `Routing Version` | table/page | **99000786 / 99000810** | Routing versions | Version-line pages mirror routing-line page behavior. | +| `Work Center` | table/card/list | **99000754 / 99000754 / 99000755** | Capacity | Parent for machine centers; has calendars/load. | +| `Machine Center` | table/card/list | **99000758 / 99000760 / 99000761** | Capacity | Capacity unit often resolves through parent work center. | +| `Calendar Entry` | table | **99000757** | Capacity | Keyed by Capacity Type + No. + Date for "available until" style checks. | +| `Capacity Ledger Entry` | table | **5832** | Posting/capacity | Posted output/capacity cost source for WIP and Power BI. | +| `Capacity Unit of Measure` | table | **99000780** | Capacity | `Code` is not the same as `Type`; this caused Power BI blanks. | +| `Item Journal Line` | table | **83** | Posting | Consumption/output journal line carrier. | +| `Flushing Method` | enum | **5417** | Posting/flushing | Manual, Forward, Backward, Pick+ variants. | +| `Consumption Journal` | page | **99000846** | Posting | Manual component consumption entry. | +| `Output Journal` | page | **99000823** | Posting | Manual output/capacity posting entry. | +| `Production Journal` | page/codeunit | **5510 / 5510** | Posting | Combined consumption/output UI path. | +| `Requisition Line` | table | **246** | Planning | Planning worksheet/carry-out line. | +| `Planning Worksheet` | page | **99000852** | Planning/MRP | Carries action messages into orders. | +| `Planning Component` | table/list | **99000829 / 99000861** | Planning/MRP | Component demand before order creation. | +| `Stockkeeping Unit` | table/card | **5700 / 5700** | Planning/cost | SKU-level `Production BOM No.` and cost loading are separate paths. | +| `Calculate Low-Level Code` | codeunit | **99000793** | Planning/MRP | Must traverse Item and SKU BOM links. | +| `Mfg. Carry Out Action` | codeunit | **99000818** | Planning/MRP | Creates prod. orders from requisition/planning lines. | +| `Calculate Standard Cost` | codeunit | **5812** | Costing | SKU and non-inventory material paths differ. | +| `Mfg. Cost Calculation Mgt.` | codeunit | **99000758** | Costing | Routing/cost-time/expected-cost calculations. | +| `Copy Production Order Document` | report | **99003802** | Orders | Request-page lookup can keep stale filters. | +| `Inventory Valuation - WIP` | report | **5802** | WIP reporting | Production Order - WIP financial report. | +| `Inventory Valuation - WIP CZA` | report | **31133** | WIP reporting | CZ sibling with same stale-value trap. | +| `Calculate Work Center Calendar` | report | **99001046** | Capacity | Card action / request filter path. | +| `Calc. Machine Center Calendar` | report | **99001045** | Capacity | Card action / request filter path. | +| `Standard Cost Worksheet` | table/page | **5841 / 5841** | Costing | Single-level material/capacity cost calculations. | +| `Assembly Header` / `Assembly Line` | tables | **900 / 901** | Assembly overlap | Assemble-to-order copy path only. | +| `Assemble-to-Order Link` | table | **904** | Assembly overlap | Link that must survive sales-quote copy. | + +### Reliable manufacturing markers / fields + +- **Always filter production-order children by line when the child is line-scoped.** + `Prod. Order Component` and `Prod. Order Routing Line` bugs commonly appear when + code uses only Status + Prod. Order No. and accidentally crosses production + lines. An earlier WIP fix read the `Prod. Order Line` with + `Finished Qty. (Base) = 0`; a related subcontracting fix hit the same + `Prod. Order Line No.` trap. +- **Do not confuse capacity UOM `Code` with capacity UOM `Type`.** Manufacturing + Setup stores `Show Capacity In` as a code, while Power BI time factors are keyed + by type. Custom codes like `TIMER` must resolve through `Capacity Unit of + Measure.Type`. +- **SKU-level production definitions are first-class.** A valid SKU can carry a + `Production BOM No.` even when the Item path is blank or different. Low-level + code, standard cost, and planning must include SKU loops and permissions. +- **Routing line descriptions are operation data, not just work-center names.** If + a requisition/planning path is sourced from a `Prod. Order Routing Line`, prefer + the operation descriptions where the line/work center matches; only fall back to + the work center for the documented blank/mismatch cases. +- **Parallel routing is real UI state.** Previous/Next Operation No. are hidden for + Serial but must be visible on Routing and Routing Version lines when the header + Type is Parallel. + +--- + +## §1 The loop that worked + +1. **Identify the manufacturing link where the bug lives.** Definitions, + planning, order creation, journal posting, capacity display, and WIP reporting + each have different keys. Do not fix a report symptom by changing posting until + you have proven posting is wrong. +2. **Find the sibling path that already behaves correctly and mirror its filter.** + Event availability from Item Card/Requisition Line already cleared the date + filter; production-order line/component actions needed the same behavior. Sales/Purchase/Assembly copy-document reports already excluded the + current document; production order copy needed the same lookup pattern plus + stale-filter cleanup. +3. **Check W1 and localized copies before declaring a fix complete.** The WIP + stale-consumption reset had to be applied to W1 report 5802 and CZA report + 31133. Routing-description logic was correctly applied identically + to W1 and IT copies. +4. **For setup-driven defaults, preserve precedence.** New defaults belong in + Manufacturing Setup only when the document/header has no explicit value; never + backfill existing headers silently. +5. **Follow review rounds to final state.** An earlier fix looked acceptable until a + later round found a missing `Stockkeeping Unit` read permission; final accept + came only after the permission matched the new SKU traversal. The + production code was acceptable, but test rewrites temporarily removed negative + no-series and print assertions before most were restored. + +--- + +## §2 Production orders and order documents + +- **Self-copy belongs in the lookup, not as a late error.** `Copy Production Order + Document` showed the current order when source status equaled target status, so + users could select it and only fail later. Exclude the target `No.` in the lookup + like Sales/Purchase/Assembly/Inventory copy-document patterns — and clear that + filter when the source status changes, or a stale `No.` filter can hide a valid + order in another status. +- **Released production orders from planning are not just a new enum label.** The + carry-out flow must set status through `SetProdOrderStatus`, use the released + order no. series, include released orders in the print filter, and run forward + flushing after line/component creation. Tests that only prove the report handler + fired are weak; prove the released order reaches the print dataset. +- **No-series negative cases matter for planning carry-out.** An earlier change briefly + removed the released-order no-series regression; review asked to restore it + because a missing released no. series should fail before creating orders. +- **Production Order - WIP vs embedded Power BI WIP are different user surfaces.** + Renaming the embedded page caption to `Production Order WIP (Power BI)` fixed a + Tell Me ambiguity without changing the page object name or AL references. If a bug is search/discoverability, do not touch report 5802. + +--- + +## §3 Routings, routing versions, and standard tasks + +- **Parallel routing fields must be controlled by the parent routing type.** The + line subpages need a Boolean from the parent Routing/Routing Version page so + `Previous Operation No.` and `Next Operation No.` show for Parallel and stay + hidden for Serial. This is page-state logic; table data was not the problem. +- **Routing and Routing Version pages must be tested separately.** An earlier fix changed + both normal routing lines and routing version lines; the review blocked until + TestPage coverage could assert visibility for both Serial and Parallel. +- **Validating `Routing Line.Standard Task Code` copies more than a code.** It + deletes/inserts Routing Tool, Routing Personnel, Routing Quality Measure, and + Routing Comment Line records from Standard Task relations. Demo-data or repair + helpers that validate the field need permissions for both routing relation + tables and standard-task relation tables. +- **Rerunnable demo-data fixes must update existing routing lines.** An earlier fix first + inserted Standard Tasks but returned early when a routing line already existed, + leaving upgraded/rerun companies with blank Standard Task Code. The accepted fix + backfilled blank existing lines, temporarily reopened certified routings, + preserved descriptions, and then restored status. +- **Do not overrule documented operation mappings.** In an earlier fix, review questioned omitted Standard Task Codes for later parallel/subcontracting demo + operations; the author documented that those operations were outside the mapping, + and the suggestion was treated as disputed rather than blocking. For the next + bug, match the reported operation map, not every visually similar operation. +- **Routing description preservation has an intentional asymmetry.** In the + requisition-line update path, routing `Description` is copied even when blank; + routing `Description 2` falls back to work center `Name 2` when blank. Tests + were added that lock in that asymmetry. If a later agent "normalizes" both + fields, it may reintroduce the reported bug. + +--- + +## §4 Production BOMs, BOM versions, and components + +- **Certification checks must include Production BOM Versions, not just headers.** + A variant-mandatory item on a Production BOM Version line with blank Variant Code + must block certification before status is persisted. Scope the line loop to the + current Version Code, run before Modify/Commit, and mirror the header validation + pattern. +- **Event context should match the header event shape.** An earlier fix added + `OnBeforeCheckVariantIfMandatory`; the review asked to pass the old version + record by value because it is context only and should match the same event on + `Production BOM Header`. +- **Production BOM Comment Line relations must filter by Version Code.** The old + relation let a `BOM Line No.` from a different version validate. Tightening the + TableRelation is a data-integrity fix, but remember it can reject legacy comment + rows that only existed because the relation was buggy. +- **SKU-level BOMs must participate in low-level-code traversal.** The planning + worksheet needing a second regenerative run was caused by Calculate Low-Level + Code ignoring Production BOMs stored on SKUs. Add both upward and downward SKU + traversal, deduplicate multiple SKUs that point at the same BOM, and persist the + recalculated item low-level code from SKU `Production BOM No.` validation. +- **A missing SKU BOM path may be unreachable for domain reasons.** Review + initially flagged passing a blank BOM record to `SetRecursiveLevelsOnBOM`; that concern was + withdrawn because SKU `Production BOM No.` is table-relation validated and the + existing `Status = Certified` guard prevents writes. Before "fixing" a scary + branch, compare sibling procedures and validation gates. +- **Planning Component is a separate pre-order table.** If a BOM/component bug only + appears before carry-out, look for `Planning Component` propagation as well as + `Prod. Order Component`. The subcontracting playbook has the analogous field- + propagation trap; for non-subcontracting, use it as a reminder to enumerate all + BOM → planning → production-order transfer paths. + +--- + +## §5 Capacity, work centers, machine centers, calendars, and Power BI + +- **Calendar-entry "available until" FlowFields key on capacity type and center no.** + Work Center and Machine Center fields should filter `Calendar Entry` by matching + `Capacity Type` + `No.` and use the key that includes Date so MAX(Date) returns + the latest calendar date. This made stale calendars visible before scheduling + fails. +- **Card actions must pass a narrowed table view to the existing calendar reports.** + Work Center Card runs report 99001046 filtered by current `No.`; Machine Center + Card runs report 99001045 filtered by current `No.`. Request pages expose Work + Center Group Code for work centers and Work Center No. for machine centers. +- **Capacity display conversion is not a persistence change, but it can break public + page procedures.** An earlier fix added `Capacity Shown In` to Work/Machine Center + calendar/load pages. The old 3-parameter `Load` / `SetLines` overloads must stay + compatibility wrappers; they should not suddenly read Manufacturing Setup, + convert values, or `TestField("Show Capacity In")` for existing extension callers. +- **Load % is not a capacity quantity.** Conversion tests needed to + prove Work Center and Machine Center values are scaled by TimeFactor while Load % + stays unchanged. +- **Machine Center conversion may need the parent Work Center.** the accepted + direction resolved the capacity unit through the parent Work Center for Machine + Center display. Do not assume the machine center alone carries all conversion + context. +- **Power BI measures must use capacity UOM type, not setup code.** An earlier fix corrected + blanks when Manufacturing Setup `Show Capacity In` was a custom code like + `TIMER` by joining `Manufacturing Setup - PBI API` to `Capacity Unit of Measure` + and exposing `code` + `type`; all 21 measures across Work Center, Machine Center, + Capacity Ledger Entries, Prod Order Capacity Need, and Production Orders then + lookup time factors by type. +- **Power BI setup joins should degrade gracefully.** Review warned + that an inner dataitem join can hide the whole manufacturing setup row if `Show + Capacity In` is blank or points at a missing capacity UOM. Prefer a left-outer + shape when the visible setup row is still meaningful. + +--- + +## §6 Consumption, output, WIP, finished-without-output, and scrap + +- **Report variables that describe one Value Entry must be reset for every Value + Entry, including non-WIP rows.** Report 5802 reused stale `ValueOfMatConsump` + after a consumption entry when a non-WIP value entry followed, making reported + material consumption differ from Value Entries. Move resets before the WIP check; + apply the same pattern to CZA report 31133. +- **Do not reset production-order accumulators just because one variable was stale.** + In an earlier fix, `ValueOfRevalCostAct` and `ValueOfRevalCostPstd` were *not* the same + per-record reset problem; they accumulate for the production order and are used + by `ValueEntryOnPostDataItem`. Resetting them per value entry would change the + calculation. +- **Finished without output is a legitimate Manufacturing Setup scenario.** Report + 5802 had to move WIP cleared by finishing a production order without output into + an Expensed WIP column and remove it from ending WIP, Consumption, and Capacity + columns. Detection must be per production order line with `Finished Qty. (Base) = + 0`, not just per order header. +- **Capacity cost must follow no-output reclassification too.** An earlier fix first + handled material, but review found capacity still counted in both Capacity and + Expensed WIP. The accepted fix added capacity to `OrderExpensedCap` and + subtracted it from `ValueOfCapSum` / `TotalValueOfCap`. +- **Mixed orders are the dangerous test shape.** A production order with one line + that has output and one line finished without output catches order-level WIP + detection mistakes. An earlier fix added this mixed-order test after review. +- **The corpus is thin on scrap-specific bugs.** The reliable lived lesson is that + scrap affects both material and routing/capacity calculations through the same + manufacturing cost functions; when changing scrap behavior, look at the + production-order line, component, and routing/capacity paths together. No + standalone scrap fix in this corpus established a more specific rule. + +--- + +## §7 Flushing and production journals + +- **Forward flushing after planning carry-out must happen after line/component + creation.** The released-order carry-out path kept tests that assert the + component is consumed; if you create released orders directly and forget the + forward-flush timing, the order exists but material is not consumed. +- **Released & Print is still a production-order creation path.** Do not fork a + print-only path that skips flushing or status/no-series logic. The final + shape kept one status mapping and one carry-out production-order creation flow, + then verified print inclusion separately. +- **Production Journal / Output Journal / Consumption Journal have little direct + corpus coverage.** For the next bug in these pages, derive behavior from the + posted ledger/report symptom: earlier fixes prove report 5802 can be + wrong even when the journal posting flow is correct. Do not change journal + posting to fix a report-only stale-variable bug. + +--- + +## §8 Planning worksheet, MRP, SKU cost, and standard cost + +- **Event availability from production orders must clear the inherited due-date + filter.** Opening Item Availability by Event from prod. order lines/components + inherited `Item."Date Filter" = 0D..Due Date`, hiding demand after the production + order Due Date. Clear the date filter like the Period view and requisition-line + Event path; cover both line and component actions. +- **Planning worksheet low-level codes must see SKU-only multi-level BOM chains.** + The one-run MRP result depends on Calculate Low-Level Code assigning 0/1/2 levels + through SKU Production BOMs; otherwise a second regenerative run is needed before + dependent component supply appears. +- **New table reads in planning code need object permissions.** An earlier fix added SKU + traversal but initially missed `TableData "Stockkeeping Unit" = r` on the + codeunit. The fix was one line, but without it users could hit a runtime + permission error exactly in the fixed scenario. +- **SKU manufacturing cost loading is opt-in setup.** In Standard Cost Worksheet, + do not overwrite SKU costs when Manufacturing Setup says SKU manufacturing costs + are loaded separately. The visible review finding was only a message + `Text[250]` overflow, but the domain scenario was the SKU/material/capacity cost + split. +- **Manufacturing cost events must cover both inventory and non-inventory material + cost paths.** Handled events were added for SKU routing cost, direct unit + cost, cost-time inputs, and expected production-order costs. The first round + missed the overload with `ExpNonInvMatCost`; the accepted fix added + `OnBeforeCalcProdOrderLineExpCostWithNonInvMatCost`. +- **IT and W1 cost paths can need different context.** A localized path had to pass + `MainItem` through the IT `OnCalcRtngCostSKUOnBeforeCalcRtngLineCostSKU` event + because that IT SKU route uses `MainItem` to resolve subcontractor prices. W1 did + not need the parameter because its overload does not take it. This is a + cross-reference only; if the bug is actually subcontractor-price calculation, + use the Subcontracting playbook. + +--- + +## §9 Manufacturing setup, versions, demo data, and Assembly overlap + +- **Manufacturing version defaults are optional and non-backfilling.** An earlier fix + added Manufacturing Setup defaults for production BOM version and routing version + number series. New headers inherit only when their own version series is blank; + explicit header values win; blank setup preserves old behavior; existing headers + are not backfilled. +- **Production definition wizard and Contoso data are setup consumers.** An earlier fix + covered wizard-created headers and Contoso PV10/RV10 generation. If a setup + default changes header insert logic, update helper setup order so common/finance + setup exists before manufacturing setup seeds version numbers. +- **Produced-item demo data belongs in the Manufacturing module.** An earlier fix added a + `PRODUCED` item template through demo-data codeunit 5310 before manufactured + items are created. It reused the existing helper with a new overload and kept the + old signature unchanged. +- **Demo-data overloads must preserve old callers.** The final round called + out that the old `InsertItemTemplateData` signature stayed unchanged, while the + manufacturing overload supplied planning/manufacturing fields. Follow that + pattern for future demo-data additions. +- **Assembly overlap is copy-document permission/link preservation, not production + posting.** earlier fixes corrected Team Member copying of sales quotes with + assemble-to-order links by granting narrow inherent permissions on the copy paths + that read/recreate `Assembly Header`, `Assembly Line`, and + `Assemble-to-Order Link`. The tests had to include resource components, + item-component ATO, and archived quote copy. +- **Resource-only Assembly tests are not enough.** The first round used a + resource-only BOM; review asked for item-component ATO because availability and + component reads are the typical path. The later rounds added it and were accepted. + +--- + +## Recurring agentic-review findings — fix these before proposing a change + +- **Compatibility wrappers must remain compatibility wrappers.** If you add a new + overload for capacity display, demo data, or manufacturing setup defaults, keep + the old public signature behavior unchanged. The capacity-display change remained blocked because + old `Load` / `SetLines` overloads started reading setup and converting values; + the demo-data change was accepted after keeping the original demo-data overload intact. +- **Clear filters you add to request pages/lookups.** An earlier fix added a self-document + exclusion filter but review caught that changing status could leave a stale `No.` + filter. If a request page can be reused after a field change, remove the old + filter in the else path. +- **When you add a new table read to a codeunit, update its permissions.** SKU reads + in `Calculate Low-Level Code` needed `TableData "Stockkeeping Unit" = r`. Standard Task validation copied routing-relation rows and needed those + relation-table permissions. +- **Apply fixes to localized sibling objects with the same caption/logic.** WIP + stale-value fixes needed W1 report 5802 and CZA report 31133. Routing + description fixes needed W1 and IT copies. Capacity calendar actions + touched W1, IT, and CZ areas. +- **Tests must hit the branch you changed, not just the headline scenario.** An earlier fix changed line and component availability but initially tested only the line; + another changed two routing pages and needed UI tests for both; the API + test already created the custom-code capacity UOM but did not assert the new + `type` column. +- **Do not weaken tests while aligning a change.** An earlier change briefly removed negative + no-series coverage, print-dataset verification, and `AssertEmpty` in a + multi-order test. Most were restored after review; print still had a weaker + assertion. +- **Use the net change diff when reviewing later rounds.** the later + review explicitly checked that the test-only delta was in the net change diff before + treating removed assertions as authored changes. Avoid treating base-branch churn as authored changes. +- **For display conversions, test the unchanged values too.** Capacity quantities + should scale by TimeFactor, but Load % should not. Power BI visible + text changing from code to type must be confirmed, not assumed. +- **A data-integrity TableRelation fix can expose old bad data.** The BOM + Comment Line relation tightening was correct, but the review still called out + that legacy rows created under the buggy relation may fail revalidation. +- **For no-output/finished-order bugs, include mixed production lines.** Order-level + tests can pass while line-level WIP is wrong. An earlier fix needed a mixed output / + no-output order to prove the per-line logic. +- **Assembly copy fixes need resource, item-component, and archive variants.** The fix was only accepted after item-component and archived quote scenarios were + covered; the later fix started with resource + item-component coverage. diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/subscription-billing.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/subscription-billing.md new file mode 100644 index 000000000..adbd46f7f --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/subscription-billing.md @@ -0,0 +1,389 @@ +# Subscription Billing Bug-Fix Playbook (BC / NAV) + +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Subscription +> Billing** bugs in `this repo` (W1 `Subscription Billing` app: +> subscription contracts, service objects/commitments, recurring billing, +> usage-based billing, billing proposals/lines, contract deferrals, price +> updates, renewal and termination). +> +> This is **not** an AL language guide. It only carries Subscription +> Billing-specific knowledge and the concrete lessons — what worked and what +> didn't — from past fixes. Generic AL rules and anything the compiler, +> publisher/deployment, analyzer rules, or permission-set checks will flag on +> their own are deliberately left out. + +--- + +## 0. The area in one picture + +Subscription Billing is one W1 app, but most bugs sit on the seam between two +record generations: + +1. **Source/subscription generation** — the commercial state the user owns: + `Subscription Header`, `Subscription Line`, customer/vendor subscription + contracts and contract lines, service-object data, renewal/termination dates, + price-update templates, and usage-data imports. +2. **Generated billing/posting generation** — temporary or transactional state + produced from the source: `Billing Line`, `Usage Data Billing`, sales and + purchase documents, deferral schedules, posted-document extension fields, + overdue/analysis rows, and renewal quote buffers. + +**Decide which generation a bug is in before touching code.** A large share of +fixes were not "the amount formula is wrong"; they were "path X copied the +source value and path Y didn't", "the generated row was filtered differently", +"a page buffer overwrote the user's pending value", or "a cached/generated +answer leaked into the next line/document". + +### Key objects you will keep meeting (current IDs) + +IDs below are current in repo-relative `src/Apps/W1/Subscription Billing/App`. +The app owns object range **8000..8113** (`app.json`). Verify less-common objects. + +| Object | Type | Current ID | App area | Notes | +|---|---|---:|---|---| +| `Subscription Header` | table | **8057** | Service Objects | Source service-object header. | +| `Subscription Line` | table | **8059** | Service Commitments | Source commitment line: dates, price, quantity, discounts, billing base period, renewal/termination fields. | +| `Sales Subscription Line` | table | **8068** | Sales Service Commitments | Sales-document-side commitment line; BOM explosion and FCY bugs land here. | +| `Customer Subscription Contract` | table | **8052** | Customer Contracts | Customer contract header copied into sales billing documents. | +| `Cust. Sub. Contract Line` | table | **8062** | Customer Contracts | Customer contract line linked back to `Subscription Line`. | +| `Vendor Subscription Contract` | table | **8063** | Vendor Contracts | Vendor contract header; mirror sales fixes here when applicable. | +| `Vend. Sub. Contract Line` | table | **8065** | Vendor Contracts | Vendor-side contract line linked back to `Subscription Line`. | +| `Billing Line` | table | **8061** | Billing | Generated billing candidate; holds net amounts and source links. | +| `Billing Line Archive` | table | **8064** | Billing | Billing history; existence matters even when amount is zero. | +| `Billing Proposal` | codeunit | **8062** | Billing | Builds billing lines and calculates billing periods. | +| `Create Billing Documents` | codeunit | **8060** | Billing | Creates sales/purchase invoices/credit memos from billing lines. | +| `Billing Template` | table | **8060** | Billing | Automation/error context; interactive errors need record identity. | +| `Contract Billing Err. Log` | table | **8022** | Billing | Non-interactive/automated billing errors land here. | +| `Usage Data Billing` | table | **8006** | Usage Based Billing | Generated usage-billing rows; `Processing Status` is a correctness boundary. | +| `Usage Data Import` | table | **8013** | Usage Based Billing | Import header; status updates must stay scoped to one import. | +| `Create Usage Data Billing` | codeunit | **8023** | Usage Based Billing | Creates usage billing candidates. | +| `Process Usage Data Billing` | codeunit | **8026** | Usage Based Billing | Updates subscription quantity/price/cost from usage data. | +| `Usage Based Billing Mgmt.` | codeunit | **8029** | Usage Based Billing | Helper surface for connector and billing flows. | +| `Usage Based Pricing` | enum | **8007** | Usage Based Billing | Extensible boundary; `None` is the exclusion, not the upper bound. | +| `Processing Status` | enum | **8012** | Usage Based Billing | `Error` lines must not be processed or reset by unrelated imports. | +| `Price Update Template` | table | **8003** | Contract Price Update | Stores filters for price-update proposals. | +| `Contract Renewal Selection` | page | **8006** | Contract Renewal | Temporary-buffer page for renewal terms. | +| `Sub. Contr. Renewal Subcribers` | codeunit | **8001** | Contract Renewal | Sales-post subscribers; renewal detection can be expensive and cached. | +| `Contract Deferrals Release` | report | **8051** | Deferrals | Releases contract deferral schedules to G/L. | +| `Cust. Sub. Contract Deferral` | table | **8066** | Deferrals | Customer deferral schedule. | +| `Vend. Sub. Contract Deferral` | table | **8072** | Deferrals | Vendor deferral schedule; keep customer/vendor logic symmetric. | +| `Assign Service Commitments` | page | **8065** | Service Commitments | Dialog opened from subscriptions or sales lines; caption context matters. | +| `Extend Contract` | page | **8002** | Customer Contracts | Page parameter setters are the integration seam. | +| `Sales Line` | tableextension | **8054** | Sales Service Commitments | Holds subscription fields and `IsLineAttachedToBillingLine()`. | +| `Purchase Line` | tableextension | **8065** | Sales Service Commitments | Purchase mirror of billing-line attachment checks. | + +### Reliable Subscription Billing markers and seams + +- A sales/purchase line being "owned by billing" is represented through + `IsLineAttachedToBillingLine()` / the billing-line link, not by a generic item + or document-type guess. On `Sales Line`, that helper has caching history — make + the public result key-aware before exposing or reusing it. +- `Billing Line` amounts are net/VAT-exclusive. If the generated document has + `Prices Including VAT = true`, convert before assigning document line prices, + and do it in **all four** paths: sales, purchase, usage-sales, usage-purchase. +- `Usage Data Billing`.`Processing Status = Error` is a hard exclusion. Do not + process it, do not flip it back to OK through a broad status reset, and do not + let one import clean another import's errors. +- `Usage Based Pricing` is extensible. `None` is the lower boundary to exclude; + an upper bounded enum filter silently rejects partner values above the current + last base enum value. +- Billed/archived state is about **record existence**, not amount totals. A + zero-value archived billing line is still billing and can lock date edits. +- Renewal and extend-contract pages use temporary buffers. Page refresh triggers + can reload persistent `Subscription Line` and wipe pending user edits unless + the current buffered row is read first. + +--- + +## 1. The loop that worked + +1. **Read the bug context and identify the exact path.** Subscription + Billing usually has parallel customer/vendor, sales/purchase, standard/usage, + per-contract/per-customer, and page/API/import paths. The bug is often one + missing sibling path, not the shared helper. +2. **Find the mirror path and compare it line-for-line.** Good fixes mirror the + already-correct sibling: customer deferral → vendor deferral, + sales document header → purchase document header, standard billing + price assignment → usage-based billing price assignment. +3. **Keep generated-state filters scoped to the source row/import/document.** If + you touch status updates, billing-line links, or renewal caches, key them by + the stable source identity and clear them at document/import boundaries. +4. **When widening extensibility, prove the new surface is deterministic.** A + public helper with hidden cache requirements or an extensible enum with no + runtime branch is worse than no API. +5. **For amount fixes, trace source → billing line → document line → posted/deferral + release.** The same visible invoice amount can be set in standard billing, + usage billing, deferral release, or renewal quote calculation; cover the one + the bug actually reaches. + +--- + +## 2. Billing proposals and billing document creation + +- **Non-progress billing-period loop:** `BillingProposal.CalculateBillingPeriod` + could recalculate the same `BillingPeriodEnd` forever when a harmonized customer + contract's `Next Billing To` capped the period before the requested billing + date. Fix: keep the previous end date and only loop while the recalculation + actually moves forward. The capped billing line should still be created and + advance the subscription from that line. +- **Large-run performance is not just keys:** scale + fixes touched proposal creation, document creation, usage-data links, progress + tracking, field loading, transaction checkpoints, and cached reads. Review kept rejecting broad + speedups without functional proof because billing amounts, document links, + usage-data links, and extension-visible pricing events are financial behavior. + If you optimize this area, prove customer and vendor documents, usage lines, + links, and new transaction-checkpoint semantics. +- **Do not bypass pricing/UoM hooks silently:** the `Billing Price Calc. + Skip` idea set handled on sales/purchase price, cost, and UoM events while + billing lines were initialized. That can bypass subscriber adjustments to + price, cost, quantity, or UoM. Add an opt-out/integration point or prove the + supported billing result stays identical. +- **New transaction checkpoints change rollback semantics:** + adding a transaction checkpoint after each created billing document can intentionally preserve earlier + documents and billing-line updates after a later failure. Treat that as a + behavior change; test at least two documents where the second fails and assert + the first is intentionally preserved. +- **No-GUI/Job Queue is a separate billing path:** dialog/progress + changes must run under `GuiAllowed = false` and still create documents or log + errors. Do not validate only the interactive page path. +- **External Document No. has two sales-header paths:** per-contract + billing uses `CreateSalesHeaderFromContract`, where `TransferFields(CustomerContract, + false)` copies the new field and a subsequent `Validate("External Document No.")` + retriggers base validation. Per-customer grouped billing uses + `CreateSalesHeaderForCustomerNo`, where the contract must be looked up and the + value validated explicitly. If you add contract-header fields, cover both paths. +- **Payment discount belongs to the contract terms:** recurring + invoices were inheriting the customer/vendor payment discount instead of the + contract's payment terms. The sales side needed a temporary reassignment because + W1 `Sales Header` validation checks `xRec`; the purchase side only needed a + re-validate after `Document Date` was set. Run the recalculation after document + date validation and assert terms code, discount %, and discount date. +- **VAT-inclusive document prices require gross-up:** billing lines + store net values. When `Sales Header` / `Purchase Header`.`Prices Including VAT` + is true, gross up with VAT % and round to Currency `Unit-Amount Rounding + Precision` before assigning line price. Apply to standard and usage-based sales + and purchase paths; skip Full VAT. Usage-based VAT-inclusive coverage was the + review gap. +- **Interactive errors must raise populated `ErrorInfo`:** + `CreateBillingDocuments` already built error identity for Billing Template and + Billing Line failures but discarded it by calling `Error(ErrorText)`. Interactive + paths should raise the populated `ErrorInfo` so the client can navigate to the + record. Automated billing keeps logging to `Contract Billing Err. Log`. Cover + both `DisplayOrLogErrorFromBillingTemplate` and `DisplayOrLogErrorFromBillingLine`. +- **Configurable billing-period text is contract-type data:** the + Billing Period Description belongs on the subscription contract type and should + reuse the existing Field Translation pattern. Blank values stay on the standard + label path; when billing creates sales/purchase document lines, resolve text + using the contract type from the billing contract. +- **Day/week periods do not snap to month end:** explicit + `Subscription Line End Date` invoice amounts were wrong because day/week + formulas were treated like month rhythms. For `D` and `W` date formulas, use the + plain period end; keep month/quarter/year month-end alignment. Include leap-year + and month-end cases. + +--- + +## 3. Service commitments, sales lines, and assignment flows + +- **Assign dialog must know whether it was opened from a sales line:** + page **8065** `Assign Service Commitments` uses the sales line number and + description in `DataCaptionExpression = GetCaption()` only when + `OpenedFromSalesLine` is true; the subscription-header path falls back to the + package code. Do not make a caption fix that breaks the subscription-header + dialog. +- **BOM explosion prompt belongs after the component line exists:** + the correct event order is `Sales-Explode BOM`.`OnExplodeBOMCompLinesOnAfterAssignType` + before `No.` validation and `OnExplodeBOMCompLinesOnAfterToSalesLineInsert` + immediately after `Insert()`. Record the component line before `No.` validation, + skip the early validation path only for that line, clear state after insert, + then create subscription lines from the inserted sales line with quantity. +- **Foreign-currency BOM explosion must use the cached sales line date:** + a BOM component creating a `Sales Subscription Line` for an FCY customer failed + when `GetDate()` hard-read the sales line before it was safely persisted. Use + the same cached Sales Line helper as the other calculations; keep the FCY + exchange-rate formula and unit-amount rounding unchanged, and leave `Get()` as + the normal persisted-line fallback. +- **`IsLineAttachedToBillingLine()` cannot expose a stale Sales Line cache:** + Purchase Line was a direct lookup and safe. Sales Line returned a cached Boolean + until `InitCachedVar()` ran, so an external caller reusing one record variable + across lines could get the previous line's result. The accepted fix stored the + line identity (`Document Type`, `Document No.`, `Line No.`) with the cached + value and refreshed when the key changed. +- **Start-date enforcement belongs on the `Subscription Line` field:** page-only checks let imports, APIs, and code paths bypass the + rule. Validate in the table before `UpdateNextBillingDate`, read the persisted + line to know whether the old `Next Billing Date` was still the old start date, + block current billing lines and archived billing-line **existence** (including + zero amount), allow valid correction/unbilled cases, and exempt temporary + renewal buffers. +- **Temporary records are legitimate in this app:** + renewal and selection pages stage changes before applying them. A table-level + guard must explicitly leave temporary buffers usable; a page refresh must prefer + the buffer row over reloading the persistent subscription line. + +--- + +## 4. Usage-based billing and connector extensibility + +- **Exclude `Processing Status = Error` everywhere in processing:** + lines rejected for a currency mismatch must not update subscription quantity, + price, or cost. The robust pattern also changes `SetProcessedUsageDataBillingToOk` + so it rebuilt filters from the `Usage Data Import` entry number and excluded + error rows itself instead of trusting a filtered record passed by the caller. +- **Status reset is scoped by import:** a helper that marks processed + rows OK must be limited to the current import. Do not clear errored lines from + unrelated imports, and keep its filter shape aligned with the main processing + loop. +- **Open-ended usage-pricing filters are the extensibility pattern:** + `SetUsageDataBillingFilters` should include partner `Usage Based Pricing` enum + values above `Unit Cost Surcharge` while still excluding `None`. The accepted + proof used a test-only enum value at ordinal 100 and verified exactly one row: + the extension value was included, `None` was excluded. +- **Expose connector helpers without changing their semantics:** + custom Usage Data Connector apps need to call the same helper procedures used by + the generic connector path, especially around `Usage Data Processing.CreateBillingData`. + The safe change only removes `internal` from existing procedures; it did + not change filters, metadata creation, amount calculation, event timing, or + status handling. +- **Usage-based VAT follows the same gross-up rule as standard billing:** + if the fix touches `SetInvoicePriceFromUsageDataBilling` overloads, add direct + VAT-inclusive usage tests. Standard billing tests do not prove the usage path is + reached. + +--- + +## 5. Contract deferrals, G/L routing, dimensions, and price updates + +- **G/L Account contract lines use the selected line account:** without deferrals, `CustomerDeferralsMngmt` and + `VendorDeferralsMngmt` should early-exit so BaseApp's selected `Sales Line."No."` + / `Purchase Line."No."` remains the posting account. With deferrals, store the + selected account on the deferral and have `ContractDeferralsRelease` prefer it; + blank values on existing deferrals and non-G/L lines fall back to General + Posting Setup. +- **GPS validation must respect deferral-owned accounts:** + `CheckGenPostingSetup` should not demand a setup contract account when the + deferral row already carries its own G/L account. This is what lets a G/L + Account contract line post even when the generic customer/vendor subscription + contract account is blank. +- **Credit memo reversal is a separate G/L-account proof:** earlier review + called out missing explicit tests for credit memos + reversing G/L Account contract lines, with and without deferrals. If you change + deferral account routing, cover the reversal path. +- **Vendor single-period deferrals mirror the already-correct customer logic:** when the billing period stays inside one calendar month, vendor + deferrals must use the exact schedule length before falling through to partial + month or full-month branches. Multi-period schedules keep first/middle/last + period rules. The existing `OnBeforeInsertVendorContractDeferral` override point + still fires after calculated values are set. +- **Default Dimension Priorities must survive the Subscription Line merge:** resolve dimensions through the priority-aware path (`Source Code + Setup` + Dimension Management helpers) like BaseApp Sales/Purchase lines. Keep + `Subscription Line`.`OnAfterGetCombinedDimensionSetID(Rec)` firing immediately + after the combined dimension set is computed, or extensions lose the old hook. + End-to-end proof is a generated invoice line carrying the highest-priority Item + dimension. +- **Price Update Template filters are user input:** + a past bug was that proposal default filters overwrote `Price Update Template` + filters on Subscription Lines. If you touch price-update proposal generation, + verify template filters are applied after/with defaults rather than replaced by + them. + +--- + +## 6. Renewal, termination, and contract extension pages + +- **Sales-post renewal detection must be cached only inside one posting run:** repeated `SalesLine.IsContractRenewal()` and + `SalesHeader.HasOnlyContractRenewalLines()` calls caused O(N²) scans and about + 25-28% batch CPU in renewal posting. Cache wrappers are appropriate, keyed by + line/header `SystemId`, but clear them on both `OnBeforePostSalesDoc` and + `OnAfterPostSalesDoc`; temporary buffers without `SystemId` use the uncached + fallback. +- **Cache reset boundaries need multi-document proof:** post multiple + documents in one run and verify normal lines are still posted correctly after a + renewal document. A stale renewal cache can silently skip invoice/shipment line + insertion or header creation. +- **Renewal Term page edits live in a temp `Subscription Line` buffer:** `ContractRenewalSelection.OnOpenPage` fills the buffer, + `RenewalTermCtrl.OnValidate` writes to it, and `OnAfterGetRecord` must read the + current buffer row by `Subscription Line Entry No.` before loading the stored + line. Otherwise `CurrPage.Update` makes the field appear to reject the user's + value. +- **Use different values on different renewal rows:** a test that + enters the same Renewal Term on two lines does not prove per-line buffering. + Set different terms, move to the next line, then return and assert the first + line kept its own value. +- **Cancellation in days is not month-end rounding:** + a past bug rounded `Cancellation Possible Until` to end of month when `Notice + Period` was expressed in days. If you touch termination math, distinguish day + formulas from month-aligned formulas just as invoice-period math does. +- **Renewal quote totals must honor `Billing Base Period`:** a past bug showed the base-period amount instead of the renewal-term + amount on the Contract Renewal Quote. If you fix renewal totals, trace the + amount period from `Subscription Line`.`Billing Base Period` through planned + and quote lines, not just the visible price field. +- **Expose `Extend Contract` parameters as a page integration seam:** + dependent apps need to open page **8002** and initialize it the same way as the + existing usage-data flows. Widening the two parameter procedures is safe only if + signatures and bodies stay the same and `OnOpenPage` still copies parameters + into page state before validation. + +--- + +## 7. Extensibility lessons specific to Subscription Billing + +- **Do not make closed financial enums extensible unless runtime supports partner + values:** `Rec. Billing Document Type` and `Usage Based Billing Doc. + Type` model real invoice/credit-memo states; posting, deferral, filtering, and + conversion code only understood built-in values. Keep them closed unless the + full document flow handles custom values. +- **Grouping enums need an `else`/event path:** `Customer Rec. Billing + Grouping` and `Vendor Rec. Billing Grouping` feed `ProcessBillingLines()` case + statements. A partner value that compiles but creates no sales/purchase document + is a runtime bug. Add a deliberate `IsHandled` extension point or keep the enum + closed. +- **Do not expose temporary internal state:** helpers such as + `SetUnitPriceAndUnitCostFromExtendContract()` / `ResetCalledFromExtendContract()` + were called out because they reveal page-flow internals, not stable domain + concepts. Prefer exposing a domain operation or the existing page parameter seam. +- **Extensibility regression tests can be tiny but must consume the new contract:** use a test app to call one newly public procedure or add a + test-only enum value. The point is to catch accidental rollback of access and to + prove the runtime path handles partner input. +- **Public cached APIs need key-aware behavior:** once a helper is + callable by external apps, callers should not need to know to call a separate + cache initializer. Direct lookup or key-aware cache refresh is the safe pattern. + +--- + +## 8. Recurring agentic-review findings — fix these *before* handing off the fix + +These are issues automated reviews repeatedly caught in Subscription +Billing changes. Pre-empting them saves review rounds. + +- **Every parallel billing path you changed needs evidence.** Per-contract vs + per-customer grouped sales headers, sales vs purchase headers, standard vs usage-based price assignment, and customer vs + vendor deferrals each had separate code paths. Do not test only the + path that first reproduced the bug. +- **Performance changes must prove behavior, not just speed.** If you add keys, + caches, progress trackers, bulk updates, or transaction checkpoints in billing creation, prove + amounts, document links, usage-data links, pricing/UoM hook behavior, no-GUI + execution, and the intended rollback/transaction boundary. +- **A helper receiving a filtered record is a trap.** Rebuild critical filters + inside the helper from stable identity (`Usage Data Import` entry number, + document SystemId, billing-line key) when the helper changes status or cache + state. `SetProcessedUsageDataBillingToOk` was fixed this way. +- **Record existence beats amount totals for billed-state checks.** Zero-value + archived billing lines still count as billing and must lock the same start-date + edits as non-zero lines. +- **Page buffer tests must distinguish rows.** If the bug is "the current renewal + row overwrote another row," use different Renewal Terms and navigate back. If the test uses the same value everywhere, it proves too little. +- **Scope test cleanup to created subscription lines.** Broad `ModifyAll` on + `Subscription Line` can make renewal/contract tests order-dependent in shared + test companies; limit cleanup or assert the expected count first. +- **Interactive and automated billing errors are different contracts.** Raising + populated `ErrorInfo` is right for interactive Billing Template/Billing Line + paths; automated billing should keep logging behavior. Cover both sibling + interactive helpers when both are changed. +- **Extensible enums need real runtime behavior.** Do not let a partner enum value + compile and then skip document creation or filtering at runtime. Either add a + proper extension point or keep the enum closed. +- **Public access changes are API contracts.** Removing `internal` is acceptable + for stable domain helpers and page parameter seams, but not + for volatile temporary state. Once public, name/signature/semantics + become partner dependencies. diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/warehouse.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/warehouse.md new file mode 100644 index 000000000..5d5a88504 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/playbooks/warehouse.md @@ -0,0 +1,395 @@ +# Warehouse / Inventory / Item Tracking Bug-Fix Playbook (BC / NAV) + +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Warehouse / +> Inventory / Item Tracking** bugs in this repo's W1/base-app layers: warehouse receipts/shipments, +> put-away/pick (directed + basic), bins and bin content, warehouse +> activities/worksheets, inventory movements/reclass/transfers, item +> availability, and serial/lot/package tracking with reservations. +> +> This is **not** an AL language guide. It carries only warehouse/tracking +> knowledge and concrete lessons — what worked and what did not — from past +> fixes and reviews. Inventory costing/valuation and manufacturing-internal math +> are deliberately out of scope, except where warehouse handling touches them. + +--- + +## 0. The area in one picture + +The same item quantity can be represented in **three overlapping models** before it finally becomes a posted ledger fact: + +1. **Warehouse handling** — source documents create `Warehouse Request` rows, then worksheet/activity/document lines (`Pick`, + `Put-away`, `Movement`, `Invt. Pick`, `Invt. Put-away`, warehouse receipt/shipment) move quantities between bins and staging + bins. +2. **Inventory / transfer application** — item journals, transfer orders, and source posting create/apply `Item Ledger Entry` + rows. Transfers have outbound, in-transit, and inbound faces; inbound reservation and application are not the same as outbound + availability. +3. **Tracking and reservations** — `Tracking Specification` carries proposed serial/lot/package assignment; `Reservation Entry` + carries availability, source, and paired-reservation state. Warehouse pick lines can reserve or consume tracked stock before + anything is posted. + +**Fix the seam the bug lives on.** A lot of defects were not in the main posting routine but in the glue: source references copied +to warehouse requests, pick worksheet availability excluding the wrong bins, temporary availability entries for unregistered +picks, or tracking status copied as a real reservation when it was only a prospect. + +### One-step vs. two-step handling + +- **Basic warehouse / inventory documents** use inventory picks, inventory put-aways, and movements directly from source/inventory + needs. They often have bins but not directed put-away and pick. +- **Advanced / directed warehouse** uses warehouse receipts/shipments plus warehouse put-aways/picks. Location fields (`Require + Receive`, `Require Shipment`, `Require Put-away`, `Require Pick`, `Directed Put-away and Pick`) decide which documents are + legal. +- **Staging bins are not pickable stock.** Receipt bins and shipment bins are handling bins; pick worksheet and availability logic + must not offer their stock as ordinary available-to-take quantity. +- **FEFO changes bin choice.** In basic warehouse movements with `Pick According to FEFO`, a blank `From Bin` can be meaningful: + the engine chooses the lot/bin. Do not widen a FEFO-specific blank-bin fix to all movements. + +### Tracking model in one paragraph + +`Tracking Specification` (table 336) is the document-line working copy. `Reservation Entry` (table 337) is the +availability/reservation fact and is keyed by source fields plus serial/lot/package values. A copied tracking line is often +`Reservation Status::Prospect`; forcing `Reservation` without the paired entry corrupts the model. Warehouse +activity lines also carry serial/lot/package values and can represent allocated but unregistered demand; item tracking +availability must subtract them or the same lot/package can be chosen twice. + +### Key objects you will keep meeting (current IDs) + +IDs below are current for this area. + +| Object | Type | Current ID | Area | Notes | +|---|---|---:|---|---| +| `Location` | table | **14** | Inventory / Warehouse | Warehouse setup fields: `Require Pick`, `Require Shipment`, `Bin Mandatory`, `Directed Put-away and Pick`, `Pick According to FEFO`, receipt/shipment bins. | +| `Item Ledger Entry` | table | **32** | Inventory | Final posted inventory facts; transfer application must honor selected tracking. | +| `Tracking Specification` | table | **336** | Tracking | Working tracking lines; includes `Source Type`, `Source Subtype`, `Source ID`, `Source Ref. No.`, serial/lot/package fields. | +| `Reservation Entry` | table | **337** | Reservation | Real reservation/prospect rows; wrong status/source/quantity creates orphan or missing entries. | +| `Reservation Entries` | page | **497** | Reservation | Inspection page for table 337. | +| `Reservation` | page | **498** | Reservation | User-facing reservation flow; inbound transfer errors live here. | +| `Available - Transfer Lines` | page | **99000896** | Transfer / Reservation | Separate transfer availability page; do not assume `Reservation` page tests cover it. | +| `Available - Item Ledg. Entries` | page | **504** | Inventory | Selects open ILE application candidates. | +| `Item Tracking Lines` | page | **6510** | Tracking | Main tracking assignment page. | +| `Item Tracking Summary` | page | **6500** | Tracking | Lot/serial/package availability summary fed by codeunit 6501. | +| `Item Tracking Management` | codeunit | **6500** | Tracking | `CopyItemTracking`/tracking-copy status traps. | +| `Item Tracking Data Collection` | codeunit | **6501** | Availability / Tracking | Builds tracking availability; must account for unregistered picks. | +| `Transfer Header` | table | **5740** | Transfer | `Direct Transfer`, `In-Transit Code`; do not validate direct transfer false after route setup unless intended. | +| `Transfer Line` | table | **5741** | Transfer | Source type for transfer reservation/application; use `Database::"Transfer Line"`, not magic `5741`. | +| `Warehouse Request` | table | **5765** | Warehouse source | Source key rows; upgrade/rename source fields only under exact filters. | +| `Warehouse Activity Header` | table | **5766** | Activity | Stores header flags such as `Do Not Fill Qty. to Handle`. | +| `Warehouse Activity Line` | table | **5767** | Activity | Pick/put-away/movement lines; has Activity Type, Action Type, bin, source, tracking, quantity fields. | +| `Warehouse Pick` | page | **5779** | Activity | Advanced pick document. | +| `Warehouse Put-away` | page | **5770** | Activity | Advanced put-away document. | +| `Inventory Pick` | page | **7377** | Activity | Basic pick; `Activity Type = Invt. Pick` matters. | +| `Inventory Put-away` | page | **7375** | Activity | Basic put-away. | +| `Warehouse Movement` | page | **7315** | Activity | Movement activity page. | +| `Warehouse Receipt Header` / `Line` | tables | **7316 / 7317** | Document | Two-step receipt document. | +| `Warehouse Shipment Header` / `Line` | tables | **7320 / 7321** | Document | Two-step shipment document; posted/picked quantities affect availability. | +| `Warehouse Entry` | table | **7312** | Warehouse ledger | Bin quantities, cubage, weight, posted warehouse movements. | +| `Bin Content` | table | **7302** | Bins | Quantity, pick qty, ATO component pick qty, `Block Movement`; keyed by location/bin/item/variant/UOM. | +| `Bin Type` | table | **7303** | Bins | Flags `Receive`, `Ship`, `Pick`; directed locations use these for staging/pickability. | +| `Bin` | table | **7354** | Bins | `Bin Type Code`, capacity/ranking fields. | +| `Whse. Worksheet Line` | table | **7326** | Worksheet | Movement/pick/put-away worksheet rows; template creation bug in an earlier fix. | +| `Whse. Worksheet Name` / `Template` | tables | **7327 / 7328** | Worksheet setup | Template `Page ID` must honor custom caller page. | +| `Pick Worksheet` / `Movement Worksheet` / `Put-away Worksheet` | pages | **7345 / 7351 / 7352** | Worksheet | Worksheet UI over table 7326. | +| `Create Pick` | codeunit / report | **7312 / 5754** | Pick creation | Codeunit creates picks; report drives request/print flow. | +| `Create Inventory Pick/Movement` | codeunit | **7322** | Basic whse | Inventory pick/movement creation, FEFO movement, job availability aggregation. | +| `Warehouse Availability Mgt.` | codeunit | **7314** | Availability | Pick worksheet / shipment-bin availability calculations. | +| `WMS Management` | codeunit | **7302** | Validation | Shared warehouse journal validation including bin-content movement checks. | +| `Whse.-Activity-Post` | codeunit | **7324** | Posting/register | Inventory pick/warehouse activity posting checks. | +| `Whse. Jnl.-Register Line` | codeunit | **7301** | Warehouse journal | Registers warehouse journal lines; cubage/weight must be set before run. | +| `Whse.-Source - Create Document` | report | **7305** | Put-away / movement creation | Internal put-away event trap. | +| `Phys. Invt. Order-Post` | codeunit | **5884** | Physical inventory | Posting should mirror warehouse/transfer link-copy placement. | +| `Inventory Profile Offsetting` | codeunit | **99000854** | Planning / reservation | Requisition base-quantity rounding affects reservation cleanup. | +| `Matched Order Line Mgmt.` | codeunit | **5826** | Receipt matching | Receipt-to-order filters; only adjacent to this playbook, but useful analogy. | + +### Reliable markers and fields + +- Use `Location` setup to decide document legality: `Require Receive`, `Require Shipment`, `Require Put-away`, `Require Pick`, + `Bin Mandatory`, and `Directed Put-away and Pick`. Directed destinations usually should **not** receive a transfer-to bin from + custom workflow setup. +- Use `Bin Type.Receive/Ship/Pick`, not only `Location."Shipment Bin Code"`, when reasoning about directed staging bins. Multiple + Ship-type bins exist in real warehouses. +- Treat `Warehouse Activity Line."Activity Type"` values `Pick` and `Invt. Pick` separately. A blank `Action Type` does not turn + an inventory pick into a normal warehouse pick. +- Preserve the source key as a set: `Source Type`, `Source Subtype`, `Source ID`, `Source Batch Name`, `Source Prod. Order Line`, + `Source Ref. No.`. Partial migration of warehouse source references can corrupt availability or navigation. +- For transfer reservation direction, `Transfer Line` source subtype distinguishes outbound vs inbound; inbound quantities are not + reservable before receipt and must get an inbound-specific message. +- For tracking copies, `Prospect` is usually the right status when copying item tracking without creating the matching reservation + pair. `Reservation` status requires paired positive/negative entries. +- For transfer application, serial/lot/package values selected on the transfer entry must participate in the open-ILE filter + before the candidate entry is picked. + +--- + +## 1. The loop that worked + +1. **Classify the location first.** Basic bin-mandatory, directed put-away and pick, and no-bin locations have different valid + bin/document flows. Many fixes are just "apply this bin only for bin-mandatory non-directed locations". +2. **Find the sibling path and mirror it.** Non-FEFO already filtered Pick bins where FEFO did not; transfer-from bin + validation already had the guard that transfer-to needed; warehouse/transfer posting already showed where to copy + record links. +3. **Follow source references end to end.** A source-type fix must cover creation, item tracking, availability helpers, Show + Source Document, and upgrade rows that carry old keys. +4. **Separate "availability shown" from "posting/application done."** Unregistered picks, receipt bins, shipment bins, and + in-transit transfer lines affect what should be selectable long before ledger posting. +5. **When tracking is involved, test the exact tracked value that survives.** It is not enough that a document is created; assert + the selected lot/package/serial stayed on the purchase/transfer/pick line or consumed the right ILE. +6. **For multi-round bugs, carry reviewer objections forward until the final resolution.** Several good fixes had a correct first + idea but an unsafe scope or a weak test that only later got fixed. + +--- + +## 2. Receipts, shipments, and staging bins + +Symptoms: pick creation says "nothing to handle", pick worksheet overstates stock, serial/lot appendices mix between posted +shipments, or document posting loses header metadata. + +- **Ship-type bins are not pick bins:** a directed location with multiple Ship-type bins failed a second pick because + previous picked quantity sat in a non-default Ship bin. The fix had two parts: add the Pick-only bin type filter to the FEFO + branch in `Create Pick`, and change `Warehouse Availability Mgt.` to sum **all** Ship-type bins in `CalcQtyOnShipmentBins`, not + only `Location."Shipment Bin Code"`. Guard the default-bin fallback so the default shipment bin is not double-counted when it + already has a Ship bin type. The useful test assertion was not just quantity; it asserted the new Take line's `Bin Code` was the + pick bin, not the ship bin. +- **Pick worksheet availability must exclude receive and shipment handling stock:** + `CalcQtyAvailToTakeOnWhseWorksheetLine` needed to ignore current receipt and shipment bins, and to cap bin-content availability + for receive / put-away locations so received-not-put-away quantity stays unavailable even if the receipt bin code changes later. + The remaining review gap was the symmetric shipment-bin-lineage case: if the shipment bin code changes after a pick is + registered, picked-not-shipped quantity in the old shipment bin must still stay unavailable. +- **Batch-printed posted shipment tracking appendix is a layout-scope bug until proven otherwise:** the GB + `SalesShipment.rdlc` fix removed a tablix-level `Start` from the Serial/Lot Number appendix. + Because the symptom was lot rows appearing under the wrong document, review required manual verification with **at least two** + posted shipments in one batch and confirmation that the appendix is scoped under the outer per- document group + (`No_SalesShptHeader` + `OutputNo`). Pagination-only changes are not obviously enough when the symptom is cross-document mixing. +- **Record links on posted physical inventory should mirror posting flows:** `Phys. Invt. Order-Post` had to call + `RecordLinkManagement.CopyLinks` immediately after inserting the posted Phys. Invt. Order Header and each posted recording + header. The acceptable placement matched Sales, Purchase, Warehouse Receipt/Shipment, Transfer, Inventory Document, and + Assembly: copy inside the standard `if not IsHandled then` insert block, so subscribers that replace the insert own link copying + too. + +--- + +## 3. Pick / put-away / activity and worksheet bugs + +Symptoms: wrong quantity to handle, wrong source bin, worksheet rows that do not clear, custom worksheet pages opening with the +wrong template, or event subscribers receiving a stale warehouse record. + +- **Create Pick request flags must reach the header:** the Pick Worksheet path already put `Do Not Fill Qty. to Handle` + into `CreatePickParameters`, and line creation already used it to clear line qty. The missing piece was copying that flag onto + `Warehouse Activity Header` before insert, so registration can still see the user's request. Because the shared `Create Pick` + codeunit also serves Warehouse Shipment, Movement Worksheet, Internal Pick, Production, Assembly, and Job paths, keep the change + at the central parameter-to-header point and test the Pick Worksheet path that lost it. +- **FEFO pick reshuffle must consume picked quantity per lot, not per reservation row:** warehouse pick registration + for nonspecific reservations first tried to keep reservation quantity per lot, but with two reservation entries for the same lot + it subtracted the full picked quantity from each entry. The final fix used a per-lot remaining-quantity dictionary: calculate + picked quantity once per lot, then decrease the remaining amount as each `Reservation Entry` is processed. The regression + created the same lot through two item ledger entries, registered the FEFO pick, posted shipment, and proved the other orders + could still pick/ship. +- **Internal put-away after quality inspection must source current bin content, not stale receive bin:** after a WHITE + receipt is put away, a failed- quantity internal put-away cannot use the original receive bin. The resolver should use positive + `Bin Content` for non-tracked inventory at bin-mandatory locations, excluding receive/adjustment bins, while item-tracked + inventory keeps using `GetCurrentLocationOfTrackedInventory`. If multiple bins qualify, allocate the requested + Specific/Sample/Failed/Passed quantity **once across bins** and error on shortfall; do not copy the full quantity to every bin. + The accepted test asserted no line sourced from RECEIVE and total put-away quantity equaled the failed quantity. +- **Inventory Pick for ATO: skip ATO per line, not for the whole document:** + `Whse.-Activity-Post.CheckQuantityInBinContentForTracking` needed to skip activity lines marked `Assemble to Order`, because + assembly output bin content is created later by `Sales-Post`. A first-line document-level exit was unsafe: if the ATO line was + first, normal tracked lines skipped validation too. Remove the header/first-line exit and rely on the per-line ATO check; test a + mixed pick with the ATO line first. +- **Bin replenishment + FEFO blank `From Bin` needs two narrow fixes:** in codeunit 7322, when FEFO leaves worksheet + `From Bin` blank, availability must not count the destination bin's earliest lot because it cannot move onto itself; and the + handled-line buffer must be recorded under blank `From Bin` so it matches and clears the worksheet row. The final fix gated + destination-bin exclusion on `CurrLocation."Pick According to FEFO"` and only used the blank-bin buffer behavior for FEFO blank + inventory movements. The test reproduced earliest lot split between source and destination, then asserted full movement and + worksheet cleanup. +- **Warehouse worksheet template creation must persist the caller's page:** table 7326 `TemplateSelection` filtered by + the caller's `PageID` but, on first-time template creation, stored the standard page ID. The fix is to validate `"Page ID"` with + the input page in the zero-template path. Because this writes setup data, the scenario-specific regression should start with no + `Whse. Worksheet Template`, call `TemplateSelection(PageTemplate = Movement, custom PageID)`, assert the stored page ID, then + call again and prove no duplicate/failure. If similar warehouse journal/bin-creation template routines keep the old pattern, + state whether scope is intentional. +- **Report 7305 internal put-away event must expose the actual line:** `Whse.-Source - Create Document` raised + `OnBeforeProcessWhseMovWkshLines` inside the `Whse. Internal Put-away Line` dataitem but passed the sibling `Whse. Put-away + Worksheet Line`, which was stale and the wrong record type. The correct event is a dedicated + `OnAfterWhseInternalPutAwayLineOnPreDataItem` at the end of that `OnPreDataItem`, after filters are set. Be careful removing the + old event: even a bad event may have subscribers; keep or explicitly assess it. + +--- + +## 4. Bins, bin content, capacity, and movement blocking + +Symptoms: blocked bins can still be depleted, capacity is bypassed by split movements, or workflow-created transfers carry invalid +bin codes. + +- **`Bin Content.Block Movement` must be enforced on every outbound posting path that posts warehouse journal lines:** negative adjustments from bin-mandatory non-directed locations must check the `From Bin Code` bin content for + `Outbound` or `All`; sales shipment posting must call `WMS Management.CheckWhseJnlLine` before `WhseJnlPostLine.Run`. The lookup + must include Location, From Bin, Item, Variant, and Unit of Measure. Review also called out purchase return shipment as the + sibling outbound path to check; if it is in scope, add the same validation there. Add an inbound control scenario so `Block + Movement = Outbound` still allows positive inbound movement. +- **Capacity checks need cubage/weight on the posted warehouse entry:** `Prohibit More Than Max. Cap.` could be + bypassed by splitting Inventory Movement lines because the first partial registration posted a warehouse entry without + Cubage/Weight; the next capacity calculation did not see the occupied capacity. Fill cubage and weight on the warehouse journal + line with existing `WMSMgt.CalcCubageAndWeight` before `WhseJnlRegisterLine.Run`. The final test used + `WarehouseActivityLine.SplitLine`, registered the first split part, then verified the second split part was blocked. +- **Quality transfer destination bins are only valid for bin-mandatory non-directed destinations:** + the workflow bin flows from `QltyWorkflowResponse.GetWellKnownKeyBin` into the disposition buffer `"New Bin Code"`, then into + `Transfer Line."Transfer-To Bin Code"`. Apply it under a non-empty guard **and** a destination-location guard mirroring the + transfer-from side: destination is `Bin Mandatory` and not `Directed Put-away and Pick`. If the destination changes to a non-bin + or directed location, clear the persisted workflow bin, not only the page variable. Cover switching an already-configured + non-directed destination to a directed one. +- **Do not accidentally turn a routed transfer into a direct transfer:** for quality transfer dispositions, a + direct transfer is derived from an empty in-transit location. When an in-transit code has already been validated onto the + header, do not call `Validate("Direct Transfer", false)` just because the computed flag is false; leave the default false and + route intact. Only validate `Direct Transfer` when the disposition is actually direct. +- **Localized demo in-transit codes need one source of truth:** Inventory and Warehousing Contoso setup created + duplicate Italian own-logistics in-transit locations (`LOG PROP.` vs `LOG. PROP.`) because two translatable labels represented + the same logical code. Warehousing now uses `Create Location.OwnLogLocation` instead of a separate label. This is a forward + fix only; existing duplicates are not cleaned up. + +--- + +## 5. Transfers, reclass, and inventory document seams + +Symptoms: inbound transfer reservation gives a false "fully reserved", transfer receipt applies against the wrong lot/package, or +planning/drop-shipment flows create/delete bad reservation rows. + +- **Inbound transfer lines are not reservable before receipt:** the user-facing bug was a non-direct transfer shipped + but not received; reserving the inbound line showed generic `Fully reserved.` although no inbound reservation existed. Both + `Reservation.Page.al` and `AvailableTransferLines` need the inbound-specific message. Do not gate that message on `Qty. in + Transit (Base) <> 0`; unshipped inbound lines need the same clear explanation. If you add an Available Transfer Lines filter, + test the **page** (`SetSourceTableFilters`), not a direct `Transfer Line` table filter. +- **Use `Database::"Transfer Line"` and transfer direction, not raw source type numbers:** `5741` is table `Transfer + Line`, but raw literals made the reservation guard harder to review across rounds. This is one of the rare readability findings + worth carrying in the playbook because source-type mistakes change reservation behavior. +- **Transfer application must filter by selected tracking:** when a transfer receipt applies open item ledger entries, + the selected serial/lot and package values on the posted transfer entry must be marked as required before the open-entry search + applies tracking filters. The package behavior was added through the existing package extension subscriber before the existing + package filter hook ran. Tests verified the unselected first lot/package kept remaining quantity while the selected second + lot/package was consumed. +- **Transfer demand planning extensibility must protect the current profile:** an event before transfer demand + inventory profile creation is valid for split demand (cut-length) scenarios, but if the handled event receives the same + `SupplyInvtProfile` record that later continues through the procedure, a subscriber can leave the current record on the wrong + inserted profile. Save and restore the current profile or pass a copy when `IsHandled` can insert multiple transfer demand + profiles. +- **Project / job warehouse source references must align with direct reservation source references:** direct Job + Planning Line reservations used table 1003 / subtype Order while warehouse picks used `Database::Job` / subtype 0, causing + reservation availability to be double-counted or mis-keyed. The final fix changed warehouse activity, worksheet, request, + item-tracking, and creation paths to use Job Planning Line consistently, kept Show Source Document opening the Job Card, and + made `Create Inventory Pick/Movement.GetSourceLineNo` return `-1` for both `Database::Job` and `Database::"Job Planning Line"` + so multiple reserved planning lines still aggregate. +- **Warehouse Request upgrade code must filter before renaming key fields:** because `Warehouse Request` source fields + are key fields, migration used rename/delete-insert style logic. A round regressed by removing `Source Type = Database::Job` and + `Source Subtype = 0` filters before `FindSet`, which could rename Sales, Purchase, Transfer, or other requests to Job Planning + Line. Always add a non-job control row to upgrade tests. +- **Requisition-line base quantity rounding can orphan reservations:** when planning project demand to a + purchase order with a non-base purchase UoM, the purchase quantity rounds and recalculates base quantity. If `Inventory Profile + Offsetting` copies a slightly different `Quantity (Base)` than `SupplyInventoryProfile."Remaining Quantity (Base)"`, later + deletion leaves reservation entries and blocks deleting the project planning line. The fix used a UoM-scaled tolerance (`Qty. + per Unit of Measure`, not one fixed precision step) and then aligned base fields with the demand. The deterministic test used + Qty. per UoM = 12 and asserted no reservation entries remained. + +--- + +## 6. Item tracking / reservation interplay + +Symptoms: lots look available while already on picks, purchase-order creation corrupts table 337, Description lookup skips +auto-reservation, or drop shipment tracking is deleted as an illegal field change. + +- **Unregistered picks consume tracked availability:** `Item Tracking Data Collection` must add temporary demand for + outstanding warehouse pick and inventory pick lines with matching item, variant, location, serial/lot/package, positive + outstanding quantity, and a different source. Round 5 caught that the code comment claimed inventory picks were covered but the + filter still only had `Activity Type = Pick`; include `Invt. Pick` too. Later rounds added source- reservation netting so split + Take lines do not subtract the same source-line reservation more than once. This is the key pattern for "available lot" bugs: + group by source + tracking, skip the current source, subtract matching source reservation once, then insert only the remaining + temporary demand. +- **Nonspecific reservation reshuffle must preserve tracking truth:** for FEFO warehouse picks, deleting surplus + reservations and keeping picked lots must account for split reservation entries on the same lot. Use remaining qty per lot; + never recompute the full picked qty independently for each reservation row. +- **Copied tracking from planning should usually be `Prospect`, not `Reservation`:** `CopyItemTracking3` with + `Reservation Status::Reservation` created only one side of a pair, then later code looked for the missing counterpart and raised + `Reservation Entry does not exist`, leaving table 337 corrupted after Order Planning / Create Purchase Orders. Reverting to + Prospect status fixed the data model. If removing a public overload that accepted a status parameter, obsolete it first and + document that the status is ignored until the clean tag. +- **Do not delete the old serial-tracking test without replacing the scenario:** the removed test had encoded + the broken Reservation status, but it still represented the original bug scenario. Replace it with an end-to-end Order Planning test that + creates/cancels purchase orders for a lot/serial-tracked item and proves reservation entries remain valid and the flow can + re-run. +- **Sales Description lookup must capture `No.` changes before `SaveRecord`:** for `Reserve = Always` items selected + through Description lookup, `CurrPage.SaveRecord` resynced `xRec`, so `Rec."No." <> xRec."No."` became false and + `AutoReserve` was skipped. Capture `NoHasChanged` before save, return a `SelectionRestored` flag from + `RestoreLookupSelectionWithResult`, and use both in the auto-reserve guard. The accepted dispute: setting `CurrFieldNo` to + `FieldNo("No.")` was safe because `CheckWarehouse` is skipped on the restore path and item availability / credit checks are + `Type = Item` guarded. +- **Single-instance lookup state must be cleared before the forced-error point:** the negative test became meaningful + only when it asserted `Lookup State Manager.IsRecordSaved` true before the error and false after. The state is in-memory and + non-transactional, so `asserterror` rollback does not clear it; production must clear it in `RestoreLookupSelectionWithResult` + before `OnBeforeNoOnAfterValidate` fires. +- **Drop-shipment purchase creation with lot tracking must preserve both sales link and lot reservation:** Req. + Line-Reserve was treating valid drop-shipment fields (`Sales Order No.`, `Sales Order Line No.`, `Sell-to Customer No.`) as + illegal reservation changes and deleting tracking before PO creation. The fix is narrow, but the test must assert more than + "purchase line exists": set a location, verify the drop-shipment sales link survives, and verify expected lot + reservation/tracking survives. +- **Project planning line deletion bugs are reservation bugs too:** if a bug ends as "cannot delete source + line," inspect whether a planning or document conversion step created reservation entries with base qty not matching the source + demand. The correct assertion is often "no reservation entries remain" after deleting the downstream order and source line. + +--- + +## 7. Availability-specific traps + +Symptoms: an availability page hides future demand, availability overstates stock in staging bins, or warehouse availability +changes after source-key migration. + +- **Clear date filters when opening Item Availability by Event from production lines/components:** the Event view from + production order lines was capped at `0D..Due Date`, hiding later demand that Period view and Item Card showed. Clear + `Item."Date Filter"` before `ShowItemAvailabilityByEvent`, matching the Period path and the requisition-line Event path. If + changing both line and component actions, cover both; the original review accepted with a request to test the component branch + too. +- **Pick worksheet availability and Create Pick availability must agree:** if one path excludes Ship/Receive + bins and the other path counts them, users get either false pickability or "nothing to handle." When fixing one calculation, + trace the sibling calculation (`Create Pick`, `Warehouse Availability Mgt.`, worksheet line calc) for the same bin-type rule. +- **Source-line aggregation can be semantically meaningful:** changing job warehouse source type from Job to Job + Planning Line was correct for reservations, but availability calculation would have changed if `GetSourceLineNo` started + filtering one planning line at a time. Returning `-1` kept the old aggregate behavior for multiple reserved job planning lines. +- **Transfer inbound availability is not reservation permission:** a line can appear in a transfer availability page + and still be non-reservable from the inbound side until receipt. Keep page filtering, validation, and error messages aligned, + and do not let a table-filter-only test stand in for the page behavior. + +--- + +## 8. Recurring agentic-review findings — fix these *before* submitting changes + +These are the warehouse/tracking-specific issues reviewers repeatedly caught. Pre-empting them saves rounds. + +- **A test helper must not raise the production error itself.** An earlier fix initially had `AutoReserveTransferLine` throw + `InboundReservationErr` in the test helper, so the test passed without executing the `Reservation` page / Reservation Management + guard. Let the page/codeunit raise the error. +- **Tests must drive the UI/page object when the bug is in page filters.** An earlier fix kept a test that filtered `Transfer Line` + directly while the production behavior was `AvailableTransferLines.SetSourceTableFilters`. +- **Line-order exits are dangerous in mixed warehouse activity documents.** An earlier fix needed a per-line ATO skip; a first-line ATO + exit would skip validation for normal tracked lines if the ATO line sorted first. +- **Quantity allocation across multiple bins must sum to the requested quantity.** An earlier fix first risked giving the full + failed/specific quantity to each eligible bin. Allocate remaining quantity per bin and fail on shortfall. +- **FEFO-specific fixes need FEFO guards.** An earlier fix originally applied a destination-bin exclusion to any blank-`From Bin` + inventory movement; the final fix gated it on `Pick According to FEFO`. +- **When changing source-key models, include upgrade controls for unrelated rows.** An earlier fix regressed by renaming unfiltered + `Warehouse Request` rows; the final test seeded a non-job Sales Header request and verified it stayed unchanged. +- **When changing reservation quantities, test split entries for the same lot.** An earlier fix only became safe after testing a lot + split across two item ledger / reservation entries. +- **When fixing availability around picks, include both `Pick` and `Invt. Pick`.** An earlier fix missed inventory picks even though the + comment claimed blank action type covered them. +- **When adding handled events around mutable records, protect the current record.** An earlier fix exposed `SupplyInvtProfile` by var + before continuing the standard planning flow; a handled subscriber that inserts several profiles can leave the caller on the + wrong record unless you save/restore or pass a copy. +- **When applying a workflow bin, mirror both directions.** An earlier fix's transfer-to bin needed the same location guard shape as + the existing transfer-from bin. +- **When clearing invalid bin setup in a page, persist the clear.** An earlier fix's important behavior was clearing the stored + workflow argument, not just hiding or blanking the on-screen variable. +- **When fixing report tracking appendices, verify cross-document scope manually.** An earlier fix could not reasonably add an RDLC + rendering test, so the right evidence was a batch print with at least two tracked shipments. +- **When copy-item-tracking status changes, preserve compatibility and prove both old and new bugs.** An earlier fix needed an + obsolete wrapper for the removed public overload and a replacement test for the older serial-tracking scenario. +- **When using location helper setup in tests, assert the warehouse fact, not just that a document exists.** Examples: pick Take + line `Bin Code` is the pick bin, transfer line `Transfer-To Bin Code` is set/cleared correctly, + lot/package remaining quantity changes on the selected value only, and no reservation entries remain after deletion. + +--- diff --git a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/workflow.md b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/workflow.md index 341eadf9a..df1754b0e 100644 --- a/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/workflow.md +++ b/src/bcbench/agent/shared/instructions/microsoft-BCApps/agents/fix-bug/workflow.md @@ -43,7 +43,24 @@ contract. Read them only; never edit them (Rule 1). Issue images, when the task references them, are under `problem/` at the repository root. Read them if the described symptom is visual. -### Step 2: Write the plan +### Step 2: Load a discovered area playbook + +Skip this step when `AGENT_ROOT/playbooks/selected.yaml` exists. + +Use only source paths confirmed during Step 1. Do not inspect the benchmark dataset, gold patch, +hidden test patch, or benchmark answer files. + +Read `AGENT_ROOT/playbooks/manifest.yaml`. Normalize confirmed paths to repository-relative paths +with `/` separators and compare them case-insensitively with the manifest patterns: + +- Exactly one distinct playbook matches: read that playbook before writing the plan. +- No playbook matches: continue without one. +- Different confirmed paths match different playbooks: report the ambiguity in the plan and read + none of them. + +Read at most one area playbook. Do not browse unrelated playbooks. + +### Step 3: Write the plan Hold the plan in memory - do not write it to a file (Rule 4). It must cover: diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug.agent.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug.agent.md index edb33977d..cb13c7aae 100644 --- a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug.agent.md +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug.agent.md @@ -13,6 +13,13 @@ the change, implement it, and validate it with the AL tools when they are availa does not fetch work items, does not create branches, does not commit, and does not open pull requests. Its only output is the change in the working tree plus a short report. +## Execution model + +Execute the workflow directly in this agent. Do not use the Agent tool. Do not delegate any part of +the task to a subagent. Do not start background work. This is an unattended, non-interactive +run: returning ends the session immediately, so all investigation, edits, and validation must finish +before the final response. + ## Step 1: Locate the support files and read the rules Set `AGENT_ROOT` from the harness running this agent: @@ -27,6 +34,10 @@ does not exist, stop and report the missing path. Read `AGENT_ROOT/rules.md` before acting. It defines the hard constraints, how to use the AL tools, and how to fail. +If `AGENT_ROOT/playbooks/selected.yaml` exists, read it and then read the playbook named by its +`file` field before extracting the task. Read no other area playbook. If the marker names a missing +file, stop and report that the agent package is incomplete. + ## Step 2: Extract the task From the user prompt, identify the issue description, the repository path, and any reproduction @@ -44,3 +55,5 @@ Read `AGENT_ROOT/workflow.md` and execute every step of it. | `AGENT_ROOT/rules.md` | Always, before acting | | `AGENT_ROOT/workflow.md` | Always, as Step 3 | | `AGENT_ROOT/troubleshooting.md` | When a build, publish, or test call behaves in a way the workflow does not cover | +| `AGENT_ROOT/playbooks/manifest.yaml` | During discover-mode routing | +| `AGENT_ROOT/playbooks/selected.yaml` | When present; identifies the selected-mode playbook | diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/costing.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/costing.md new file mode 100644 index 000000000..eb3780092 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/costing.md @@ -0,0 +1,399 @@ +# Costing / Inventory Valuation Bug-Fix Playbook (Business Central) +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + + +> Domain knowledge for the automated bug-fix agent working on **Costing / Inventory +> Valuation** bugs in `this repo` (base-app W1 + localized layer copies): +> item costing, cost adjustment, Value Entries, item application, exact-cost +> reversing, revaluation, item charges, additional-reporting-currency costing, +> manufacturing cost shares, and inventory valuation reports. +> +> This is **not** an AL language guide. It only carries costing/valuation-specific +> knowledge and concrete lessons — what worked and what did not — from past fixes +> and reviews. Generic AL rules, style advice, analyzer findings, CI hygiene, and +> anything the compiler/publisher catches on its own are deliberately left out. + +--- + +## §0 The area in one picture + +Costing is the ledger layer underneath inventory posting. The same defect often +appears as a purchase/posting bug, an undo bug, a report bug, or a G/L +reconciliation bug, but the invariant is the same: + +1. **Item Ledger Entry (ILE, table 32)** records quantity and application state. + It carries entry numbers, invoiced quantity, exact-cost fields, and summary + cost FlowFields. +2. **Value Entry (table 5802)** records the value attached to an ILE. It is the + first place to check when a user says inventory value, expected/actual cost, + item-charge cost, ACY amount, valuation date, or revaluation is wrong. +3. **Item Application Entry (table 339)** links inbound and outbound ILEs. FIFO, + average, specific costing, exact-cost reversing, and undo paths depend on this + chain staying correct. +4. **Cost adjustment** reopens the chain after posting. It marks entries to adjust, + calculates average/standard/FIFO effects, creates adjustment Value Entries, and + later posts the cost to G/L. +5. **Reports** (`Inventory Valuation`, `Cost Shares Breakdown`, Power BI report + entry points) should read the same Value Entry/ILE truth without losing filters, + event semantics, or amount formatting. + +**First classify the bug by ledger effect, not by UI entry point.** A purchase +invoice defect can be an item-charge Value Entry defect. A currency +posting defect can be an inventory valuation/G/L reconciliation defect. A report +modernization can silently bypass the old Value Entry filters that extensions +relied on. + +### Key objects you will keep meeting (current IDs) + +IDs below are current for the costing and inventory valuation area. Most objects are base app; several localized layers carry byte-for- +byte or near-byte-for-byte copies of posting code. + +| Object | Type | Current ID | Main path | Why it matters | +|---|---|---:|---|---| +| `Item` | table | **27** | W1 BaseApp Inventory/Item | Costing method, unit/standard cost, inventory value zero, FlowFields. | +| `Item Ledger Entry` | table | **32** | W1 + layer copies | Quantity, invoiced quantity, exact-cost/application state, ILE summary costs. | +| `Value Entry` | table | **5802** | W1 + layer copies | Actual/expected cost amounts, ACY amounts, valuation date, item-charge marker. | +| `Item Application Entry` | table | **339** | W1 + layer copies | Cost application chain, exact-cost reversing, unapply/undo. | +| `Item Application Entry History` | table | **343** | W1 | Historical application trace. | +| `Item Journal Line` | table | **83** | W1 + layer copies | Posting input for CU22; source currency and cost fields are read here. | +| `Item Jnl.-Post Line` | codeunit | **22** | W1 + APAC/CH/ES/IT/RU copies | Creates ILE/Value Entries; ACY costing fixes landed here. | +| `Purch.-Post` | codeunit | **90** | W1 + many layer copies | Posts item-charge Value Entries through receipt distribution paths. | +| `Undo Posting Management` | codeunit | **5817** | W1 | Shared undo posting; owns `PostItemJnlLineAppliedToList`. | +| `Undo Sales Shipment Line` | codeunit | **5815** | W1/RU | Drop-shipment item-application unapply seam. | +| `Undo Purchase Receipt Line` | codeunit | **5813** | W1 | Purchase exact-cost reversing/undo entry point. | +| `Undo Return Shipment Line` | codeunit | **5814** | W1 | Return-side undo. | +| `Undo Transfer Shipment` | codeunit | **9030** | W1 | Transfer undo path; review asked for direct coverage when errors changed. | +| `Inventory Adjustment` | codeunit | **5895** | W1 + APAC/RU copies | Cost-adjustment implementation behind the interface. | +| `Inventory Adjustment Handler` | codeunit | **5894** | W1 | Orchestrates adjustment runs. | +| `ItemCostManagement` | codeunit | **5804** | W1/APAC/RU copies | Average/precise cost calculation and open inbound ILE filtering. | +| `Cost Adjustment Params Mgt.` | codeunit | **5824** | W1 | Parameterization around adjustment runs. | +| `Inventory Posting To G/L` | codeunit | **5802** | W1 | Posts cost adjustment/value entries to G/L. | +| `Post Inventory Cost to G/L` | report | **1002** | W1 | Batch report for inventory cost to G/L. | +| `Post Inventory Cost to G/L` | codeunit | **2846** | W1 | Codeunit wrapper for the same posting. | +| `Adjust Cost - Item Entries` | report | **795** | W1 | User-facing cost adjustment run. | +| `Cost Adjustment Overview` | page | **5801** | W1 | Cost-adjustment page/actions. | +| `Avg. Cost Adjmt. Entry Point` | table | **5804** | W1 | Average-cost valuation-date entry points. | +| `Avg. Cost Adjmt. Entry Points` | page | **5815** | W1 | Diagnostic/user view of average-cost entry points. | +| `Average Cost Calc. Overview` | table/page | **5847** | W1 | Average-cost diagnostic buffer. | +| `Inventory Adjmt. Entry (Order)` | table | **5896** | W1 | Manufacturing/order cost adjustment buffer. | +| `Inventory Valuation` | report | **1001** | W1 | Main W1 valuation report; existing Value Entry events matter. | +| `Inventory Valuation` | report | **10139** | NA | NA copy with separate event parity needs. | +| `Cost Shares Breakdown` | report | **5848** | W1 | Manufacturing cost-share report; WIP/filter/event bugs. | +| `Standard Cost Worksheet` | table/page | **5841** | W1 | Standard-cost worksheet data and UI. | +| `Calculate Standard Cost` | codeunit | **5812** | W1 | Standard-cost calculation engine. | +| `Standard Cost Worksheet` | report | **5855** | W1 | Standard-cost calculation/update report. | +| `Calculate Inventory Value` | report | **5899** | W1 | Revaluation journal population. | +| `Revaluation Journal` | page | **5803** | W1 | Revaluation entry point. | +| `Item Charge` | table | **5800** | W1 | Item-charge master. | +| `Item Charge Assignment (Purch)` | table/page | **5805** | W1 | Purchase item-charge assignment and posted-cost distribution. | +| `G/L - Item Ledger Relation` | table/page | **5823** | W1 | Reconciliation link from inventory value to G/L. | +| `Invt. Posting Buffer` | table | **48** | W1 | Cost-to-G/L staging buffer. | +| `Inventory Setup` | table | **313** | W1 | Automatic/expected cost posting, average-cost setup. | +| `Stockkeeping Unit` | table | **5700** | W1 | SKU-level standard/unit costs. | +| `Capacity Ledger Entry` | table | **5832** | W1 | Manufacturing cost-share capacity cost source. | +| `Prod. Order Line` | table | **5406** | W1 | Production item/source for WIP and standard-cost cases. | +| `Purch. Rcpt. Line` | table | **121** | W1 | Receipt-line source for item-charge Value Entries. | +| `Purchase Line` | table | **39** | W1 | Charge-assignment target line and receipt state. | + +### Reliable markers and fields + +Use these fields to reason about the defect. Do not replace them with nearby +fields unless the current fix proves the nearby field is the correct one. + +| Table | Field(s) | Current IDs | Rule | +|---|---|---:|---| +| `Value Entry` | `"Item Ledger Entry No."`, `"Valued Quantity"`, `"Invoiced Quantity"` | 11, 12, 14 | A cost fix is not proved by existence of a Value Entry. Prove the entry is on the right ILE and has the right valued/invoiced quantity. An earlier fix caught this. | +| `Value Entry` | `"Cost Amount (Actual)"`, `"Cost Amount (Expected)"` | 43, 151 | Actual vs expected legs can need separate formulas and tests. An earlier fix landed only after both receipt and invoice legs were asserted. | +| `Value Entry` | `"Cost Amount (Actual) (ACY)"`, `"Cost Amount (Expected) (ACY)"` | 68, 156 | ACY inventory value must reconcile with G/L Additional-Currency Amount when document currency = ARC. We hit this before. | +| `Value Entry` | `"Expected Cost"`, `"Item Charge No."`, `"Partial Revaluation"`, `"Valuation Date"` | 98, 99, 102, 104 | These separate expected/actual, item-charge, partial-revaluation, and valuation-date paths. Test the branch you touch. | +| `Item Ledger Entry` | `"Invoiced Quantity"`, `"Applies-to Entry"`, `"Completely Invoiced"`, `"Applied Entry to Adjust"` | 14, 28, 5800, 5802 | Partial invoicing and exact-cost/application fixes should use state that remains valid until fully invoiced. An earlier fix changed the lookup from `Invoiced Quantity = 0` to `Completely Invoiced = false`. | +| `Item Ledger Entry` | `"Cost Amount (Expected)"`, `"Cost Amount (Actual)"`, ACY variants | 5803, 5804, 5806, 5807 | These are summary amounts. Trace back to Value Entries when the amount is wrong. | +| `Item` | `"Costing Method"`, `"Unit Cost"`, `"Standard Cost"`, `"Inventory Value Zero"` | 21, 22, 24, 5409 | Costing method drives which application/adjustment rule is in play. | +| `Inventory Setup` | `"Automatic Cost Adjustment"`, `"Expected Cost Posting to G/L"`, `"Average Cost Calc. Type"`, `"Average Cost Period"` | 30, 5800, 5804, 5805 | Average/expected cost behavior depends on setup; do not hard-code a single tenant shape. | +| `Item Charge Assignment (Purch)` | quantity/amount assignment fields | table 5805 | For item charges, the invariant includes both posted Value Entry and `Qty. Assigned = Quantity Invoiced` on the charge line, as shown by earlier fixes. | + +--- + +## §1 The loop that worked + +1. **Start from the posted ledger symptom.** Identify the exact ILE and Value Entry + that should change. For reports, identify the Value Entry filter/sum the report + used before the change. An earlier report change was risky because the new FlowField + path bypassed old `Value Entry` filter events. +2. **Classify the branch: actual vs expected, direct item vs item charge, W1 vs + localized layer, receipt vs invoice, single receipt vs partial receipts.** Most + bad fixes covered the easy branch and missed the sibling branch. +3. **Find the analogous correct path and mirror it.** `PostItemChargePerRcpt` was + the model for separately invoiced charges in an earlier fix. Existing W1 + `CalcPosShares()` was the model the APAC copy failed to reach in an earlier fix. +4. **When there are layer copies, patch and test the copies intentionally.** CU22 + ACY logic existed in APAC/CH/ES/IT/RU/W1; APAC had an extra source-currency + branch, so a mechanically identical helper call was still unreachable. +5. **For rounding/currency bugs, assert exact amounts, not abs/existence.** The + fix should prove `Value Entry` ACY equals the document/ARC amount and reconciles + to G/L; where signs matter, assert signed values. +6. **For application/undo bugs, prove the link, not the message.** Check the item + application or item-entry relation points to the ILE just posted/reversed. An earlier fix used `ItemJnlPostLine.GetItemLedgerEntryNo()` because the old field held + the wrong entry number for subcontracting undo. +7. **For extensibility fixes, place the event at the old calculation seam.** The + publisher must fire after standard filters are set and before `FindSet()`/`CalcSums()` + if the purpose is to let extensions refine the costing set. + +--- + +## §2 Adjust Cost / average-cost calculation bugs + +The corpus is thin on direct `Adjust Cost - Item Entries` product-code defects; +most lived findings are adjacent seams that cost adjustment later consumes. +Treat them as guardrails for the next real adjust-cost bug. + +- **Protect the entry-number allocation window, not just the caller:** CU22 `PostSplitJnlLine` allocates ILE and Value Entry numbers and + later inserts those entries. A `Commit()` inside that window can release locks + while cached entry numbers are still pending, causing duplicate ILE/Value Entry + numbers. The fix shape was `CommitBehavior::Ignore` around the split posting + loop with an opt-out event. The review pushback was about the test: a subscriber + after insertion does not prove the dangerous window. Put the regression `Commit()` call + into `OnBeforeInsertItemLedgEntry` or `OnBeforeInsertValueEntry` so the lock/no- + duplicate invariant is actually tested. +- **Average-cost filter hooks must pass the record by `var`:** + `OnCalculatePreciseCostAmountsOnAfterFilterOpenInboundItemLedgerEntry` fires + after `OpenInbndItemLedgEntry` has item/open/positive/location/variant filters + and before `FindSet()`. Without `var`, subscribers cannot refine open inbound + ILEs, so the event is functionally useless for average-cost calculation. If the + next average-cost bug is an extension/filter bug, verify the event sits exactly + between standard filters and the read. +- **Cost Adjustment / Item Card action duplication is UI-only:** when a bug mentions Cost Adjustment actions, separate UI discoverability + from valuation logic. An earlier fix only hid base item data actions when Manufacturing + was enabled and left cost adjustment data processing unchanged. Do not infer an + adjust-cost engine bug from duplicated Export/Import actions. +- **Re-enable cost-adjustment tests when the product fix lands:** disabled SCM Inventory Costing IV tests covered ARC + posting and adjustment scenarios. When fixing a costing defect, check whether a + disabled-test entry exists for the exact costing batch/IV scenario and remove + only that entry after the underlying source-currency/costing issue is fixed. + +--- + +## §3 Value Entry ACY / rounding / expected-vs-actual bugs + +This was the densest valuation cluster. The repeated symptom: document currency +is the Additional Reporting Currency (ARC), but CU22 recalculates Value Entry ACY +from LCY using another exchange rate, so inventory valuation no longer reconciles +with G/L Additional-Currency Amount. + +- **Use the document amount only for the real ARC scenario:** the + special path should run when `ItemJnlLine."Source Currency Code"` equals the + non-empty Additional Reporting Currency and there are no cost add-ons. The APAC + copy initially called `ShouldUseDocumentAmountForACY()` only inside + `Source Currency Code = ''`, making the new branch unreachable for the exact bug + condition. In localized posting code, prove the predicate is reachable in each + layer, not just textually present. +- **Cover purchase posting where currency factor differs from posting-date rate:** the minimum test is a purchase posting with document currency = + ARC and a currency factor different from the posting-date exchange rate. Assert + the Value Entry ACY amount equals the source document amount and reconciles with + the G/L Entry Additional-Currency Amount. +- **Expected-cost and actual-cost legs need separate proof:** early + coverage proved expected and actual direct item costs, but later changes still + needed branch-specific proof. If `Expected Cost` can be true on receipt and false + on invoice, assert both legs. Do not assume invoice follows receipt because the + helper name is shared. +- **Item charges are not automatically part of the direct-item ARC shortcut:** adding `ItemJnlLine."Item Charge No." = ''` narrowed + `ShouldUseDocumentAmountForACY()`. That was plausible, but the review blocked + because no item-charge ARC purchase test proved item charges still reconcile. + Any change to this guard must include a purchase item-charge case where document + currency = ARC. +- **Do not drop the per-base-unit ACY rounding residual on the actual leg:** the accepted fix added `RoundingResidualAmountInvdACY`, computed as + invoiced quantity times the per-base-unit ACY unit-cost residual, and used it in + the invoiced/actual leg: `DirCostACY := "Unit Cost (ACY)" * "Invoiced Quantity" + + RoundingResidualAmountInvdACY`. This mirrors the expected leg, but scales by + `"Invoiced Quantity"` instead of `Quantity`. The proving test used a non-base + unit of measure so the per-base-unit ACY cost rounds and would otherwise drop a + residual; it asserted both receipt expected and invoice actual Value Entry ACY + amounts equal the exact document ACY amount. +- **Amount tests must assert exact cost fields, not existence:** for this class, assert `Value Entry."Cost Amount (Expected) + (ACY)"` and/or `"Cost Amount (Actual) (ACY)"`, and assert the G/L Entry + Additional-Currency Amount. A `RecordIsNotEmpty(ValueEntry)` assertion would miss + the whole bug. + +--- + +## §4 Item charges' cost effect + +Item charges are valuation entries. They are not just purchase-document metadata. +When a charge is assigned to an item line, the cost effect must land on the right +receipt ILE(s), with the right quantity and amount. + +- **Separately invoiced item charges must post a Value Entry:** the root defect was: receive the item line first; later post an item- + charge invoice with the target item line's `Qty. to Invoice = 0`; no charge + Value Entry is created, `Qty. Assigned` stays `0`, `Quantity Invoiced` becomes + `1`, and the purchase order cannot be deleted because + `TestField("Qty. Assigned", "Quantity Invoiced")` fails. The correct direction + is to reuse the existing receipt distribution helpers (`PostDistributeItemCharge` + / `PostItemCharge`) from `Purch.-Post` instead of inventing a parallel value- + entry writer. +- **Never `FindFirst()` one receipt for an order-line charge:** the + reviewed fix found `Purch. Rcpt. Line` by `Order No.` + `Order Line No.` and + posted the full charge against the first receipt. That corrupts cost when the + order line was received in multiple partial receipts. Loop all matching receipt + lines and split `Qty. to Assign` / `Amount to Assign` proportionally by each + receipt's `Quantity (Base)`, mirroring the path where a charge is posted per + receipt. +- **The item-charge regression needs two invariants:** assert the + charge Value Entry cost amount and valued quantity, and assert the purchase + charge line has `Qty. Assigned = Quantity Invoiced`. The original symptom was + both a missing valuation entry and an assignment-state mismatch. +- **Return/Credit Memo symmetry is a conscious follow-up, not accidental silence:** the new path intentionally targeted `Order`/`Invoice`. The review + called out that `Return Order`/`Credit Memo` can plausibly suffer the same + separate-invoice assignment bug. If the next bug is on the return side, resolve + return-shipment lines and mirror the sign/quantity handling there; do not reuse + purchase-receipt sign rules blindly. +- **ACY guard changes must include item-charge coverage:** if a CU22 + predicate excludes `"Item Charge No." <> ''`, prove an ARC item-charge purchase + still posts Value Entry ACY amounts that reconcile with G/L. + +--- + +## §5 Cost application / exact-cost reversing / undo + +Application bugs usually look like the wrong entry was chosen, not like no entry +was written. Check the ILE number and application relation before changing amounts. + +- **Partial invoicing must keep the Negative Adjmt. ILE eligible until fully + invoiced:** project consumption through Get Receipt Lines posted the + second partial invoice's Value Entry to the wrong ILE because the lookup only + found a Negative Adjmt. ILE with `Invoiced Quantity = 0`. After the first partial + invoice, that was false even though the entry was not fully invoiced. Use + `Completely Invoiced = false` for this lookup so later partial invoices continue + applying to the correct Negative Adjmt. ILE. +- **Undo relation keys must come from the ILE just posted:** in `Undo Posting Management`.`PostItemJnlLineAppliedToList`, + subcontracting undo filled `TempItemEntryRelation."Item Entry No."` from + `ItemJnlLine."Item Shpt. Entry No."`. For subcontracting, that value can be a + capacity ledger entry number, not the reversing output ILE. Use + `ItemJnlPostLine.GetItemLedgerEntryNo()` from the same global CU22 instance that + posted the line; guard it to the subcontracting/non-zero scenario so non- + subcontracting undo stays unchanged. The tests re-enabled a lot-tracking undo + case because the defect only showed on that sensitive path. +- **Drop-shipment unapply events belong before the application entry read:** `Undo Sales Shipment Line.UnApplyDropShipment` needed an event after + standard filters on `Item Application Entry` and before `FindFirst()`, so + extensions using load fields can add extension fields before the record is read + and later modified/deleted by item application unapply logic. If the next exact- + cost reversing bug is an extension-field/load-field bug, place the seam there. +- **Message-only undo fixes are not valuation fixes:** a + better `NoLinesToReverseErr` on empty Sales/Purchase/Transfer undo selections did + not change posting, application, or valuation state. If a bug is about wrong cost + reversal, do not stop at the selection/error path. + +--- + +## §6 Revaluation and standard-cost worksheet bugs + +The review corpus has little direct revaluation math. The useful lived lessons are +about standard cost and the revaluation entry points that feed Value Entries. + +- **`Calculate Inventory Value` / Revaluation Journal are the revaluation entry + point, but the Value Entry fields prove the fix.** Use report 5899 to populate + the journal and page 5803 to inspect it, but verify the posted result in Value + Entry fields `"Partial Revaluation"`, `"Valuation Date"`, and the actual/ACY + cost fields. Do not claim a revaluation fix from worksheet lines alone. +- **Standard-cost SKU updates must respect the setup/source of SKU costs:** the bug was that single-level capacity/material cost for SKU could be + calculated/overwritten when Manufacturing Setup says SKU manufacturing costs are + loaded separately. The final reviewed change was small (widening a message from + `Text[250]` to `Text`), but the root scenario is the useful rule: standard-cost + worksheet/report fixes must preserve whether SKU manufacturing costs are loaded + separately, and long explanatory messages must not fail the run. +- **Manufacturing cost calculation extension points need all overloads:** expected production-order cost had a normal overload and a non- + inventory-material overload. The first review found the new handled event only + on one overload. The final fix added a separate `OnBeforeCalcProdOrderLineExpCost` + shape for the non-inventory-cost path. If a standard/expected-cost bug has two + calculation overloads, cover both or explicitly prove one cannot run. +- **IT SKU cost events need the item context they actually use:** the + IT `CalcRtngLineCostSKU` path used `MainItem` to resolve subcontractor prices; + the W1 path did not. The event surface had to pass `MainItem` only in the IT + event. For localized standard-cost bugs, do not flatten W1 and IT signatures if + the localized calculation uses extra cost context. + +--- + +## §7 Inventory valuation and manufacturing cost reports + +Report bugs are still costing bugs when they change filters, sums, event seams, or +formatted financial amounts. Treat layouts and navigation as lower risk only when +they demonstrably do not change calculation. + +- **Inventory Valuation report 1001 must not depend on CH-only fields:** + the Excel-layout fixes moved calculation toward Item FlowFields such as `Opening + Bal. ILE Qty.`, `Increases ILE Qty.`, and `Cost Posted To G/L`, but those fields + were added only to the CH Item table while W1 report 1001 read them. If a report + calculation is in W1, the fields/events it reads must exist in W1, not only in a + localization layer. +- **Preserve `Value Entry` filter events when optimizing report sums:** + `OnItemOnAfterGetRecordOnAfterValueEntrySetInitialFilters` and + `OnCalculateItemOnBeforeAssignDecreaseAmounts` let subscribers refine Value + Entry filters before opening/increase/decrease/G/L sums. Replacing the sums with + FlowFields bypassed those subscriber changes. Any performance rewrite of + Inventory Valuation must either keep the old event behavior or add a compatible + replacement before the sums are calculated. +- **Excel financial layouts need amount/quantity formats:** Inventory + Valuation's visible Excel pivot tables and sheets cannot leave amount and + quantity cells as General. Add explicit number formats for LCY amounts and + quantities so decimal precision/separators do not vary by culture. +- **NA Inventory Valuation report 10139 needs event parity with W1:** + extensions could compile against W1 report 1001 events but not the NA report. + The accepted shape added a `SkipItem` event before child ILE processing and a + `Value Entry` filter event after initial filters and before `CalcSums()`. Default + behavior must remain unchanged with no subscriber. +- **Inventory Valuation Power BI placement is navigation-only if report objects do + not change:** moving actions from Finance Manager to Business + Manager did not alter report pages, setup records, posting, financial + calculations, permissions, or event contracts. Do not overfit a valuation engine + fix to a role-center action bug. +- **Cost Shares Breakdown WIP mode must filter before inserting capacity cost rows:** report 5848 already applied Item filters when printing + WIP buffer rows, but capacity ledger entries for unrelated production items were + inserted before that filter. Apply the temporary Item + `CopyFilters(Item)` + + `IsEmpty()` pattern before `InsertCapLedgEntryCostShare()` so an Item filter does + not show unrelated production orders. +- **Cost-share override events must sit before standard share application:** report 5848 needed an event that lets subscribers replace how cost + share applies to capacity and overhead amounts. The default path still adds the + same inventory adjustment order costs, calculates `ShareOfCost` when `OutputQty + <> 0`, and multiplies the same buffer fields. Additive event, no default behavior + change. + +--- + +## §8 Recurring agentic-review findings — fix these *before* opening review + +These are issues the automated reviews repeatedly caught on costing/valuation changes. +Pre-empting them saves review rounds. + +- **A posted Value Entry existing is not enough.** Assert the right ILE, `Valued + Quantity`, `Invoiced Quantity`, `Cost Amount (Actual/Expected)`, ACY fields, and + assignment state relevant to the defect. An earlier fix's first test would have + passed with a wrong amount and wrong receipt distribution. +- **Partial receipts and partial invoices are first-class costing cases.** If the + fix finds one receipt (`FindFirst()`) or only `Invoiced Quantity = 0`, add a multi- + receipt or second-partial-invoice test. Earlier fixes are the pattern. +- **Expected and actual cost legs are separate branches.** For receipt+invoice or + expected-cost posting bugs, assert both `Expected Cost = true` and actual entries + where the bug can hit both. An earlier fix only closed after the actual leg's residual + matched the expected leg. +- **ACY fixes require a reconciliation assertion.** When document currency equals + ARC, assert Value Entry ACY equals the document amount and G/L Additional- + Currency Amount. A currency factor different from the posting-date rate is what + exposes the double-conversion bug. +- **Layer copies are not behaviorally identical just because names match.** APAC's + extra source-currency branch made the new W1-style ACY helper unreachable. Check control flow in every changed layer copy. +- **If a guard excludes item charges, add an item-charge test.** `"Item Charge No." + = ''` in an ACY helper is a financial branch change, not a harmless narrowing. +- **Event requests must prove the subscriber can change the costing set.** Events + for filters need a `var` record and must fire after standard filters but before + `FindSet()`/`FindFirst()`/`CalcSums()`. +- **Report performance rewrites must preserve extension semantics.** Replacing + Value Entry loops with FlowFields can silently bypass old filter events. Extension parity is part of correctness for valuation reports. +- **Excel layouts for valuation/cost reports need explicit formats.** Financial + amount and quantity cells/pivots should not be General. +- **Undo/application fixes must assert the relation points to the entry just + posted or unapplied.** For sensitive undo paths, use the posting codeunit's + authoritative last ILE number or the filtered Item Application Entry, not a + nearby shipment/capacity entry field. +- **Costing test re-enablement should be surgical.** Remove disabled-test metadata + only for fixed ARC posting/adjustment scenarios; leave unrelated still-failing + costing batch tests disabled until their underlying defect is fixed. diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manifest.yaml b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manifest.yaml new file mode 100644 index 000000000..99bdfec95 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manifest.yaml @@ -0,0 +1,27 @@ +playbooks: + - id: warehouse + file: warehouse.md + areas: + - warehouse + paths: + - App/Layers/W1/BaseApp/Warehouse/** + - App/Layers/W1/BaseApp/Inventory/Tracking/** + + - id: manufacturing + file: manufacturing.md + areas: + - manufacturing + paths: + - App/Layers/W1/BaseApp/Manufacturing/** + + - id: costing + file: costing.md + paths: + - App/Layers/W1/BaseApp/Inventory/Costing/** + + - id: subscription-billing + file: subscription-billing.md + areas: + - subscription billing + paths: + - src/Apps/W1/Subscription Billing/App/** diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manufacturing.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manufacturing.md new file mode 100644 index 000000000..b9108ede3 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/manufacturing.md @@ -0,0 +1,426 @@ +# Manufacturing Bug-Fix Playbook (Business Central) +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Manufacturing +> (non-subcontracting)** bugs in this repo's base-app layers: production orders, routings, production BOMs, +> components, capacity, consumption/output, flushing, production journals, +> planning worksheet / MRP, and Assembly only where it overlaps manufacturing +> copy or availability behavior. +> +> The Manufacturing app folder is a shell; the code covered here lives in the +> base-app layer under `src/Layers/W1/BaseApp/Manufacturing/`. +> +> This is **not** an AL language guide. It only carries manufacturing-specific +> knowledge and the concrete lessons — what worked and what didn't — from past +> fixes. Generic AL rules, generic CI hygiene, and anything the compiler, +> publisher, or analyzer rulesets catch on their own are deliberately left out. +> +> **Subcontracting is excluded.** If the bug is about subcontracting purchase +> orders, WIP transfer orders to subcontractors, subcontractor pricing, or the +> legacy IT/W1 app migration seam, switch to the separate Subcontracting playbook. +> This file only cross-references overlap where a production-order/routing fact is +> reusable. + +--- + +## §0 The area in one picture + +Manufacturing is a chain of **definitions → planning → orders → journals/posting +→ ledgers/reports**. Most bugs in this corpus were not "math is hard" bugs; they +were bugs where one link in that chain used a narrower key, stale filter, or +wrong unit than its sibling path. + +1. **Definitions** — `Production BOM Header` / `Production BOM Line` / + `Production BOM Version`, plus `Routing Header` / `Routing Line` / + `Routing Version`. Certification gates bad combinations before they become + production-order refresh failures. +2. **Planning** — MRP / planning worksheet reads Item and SKU production BOMs, + routing cost, low-level codes, and requisition lines. SKU-level BOMs are real + manufacturing setup, not an afterthought. +3. **Production orders** — orders move through Simulated / Planned / Firm Planned + / Released / Finished. Lines carry item/BOM/routing; components carry material + demand; routing lines create capacity needs and capacity ledger entries. +4. **Posting** — consumption and output post through item journals / production + journals into Item Ledger, Value Entry, and Capacity Ledger. The WIP reports + read those posted entries and can be financially wrong even when posting was + correct. +5. **Capacity display** — Work/Machine Center calendars, load pages, Power BI, and + Capacity Ledger visuals all convert from capacity-unit codes to time factors. + A display-only bug can still hide or blank the real capacity picture. + +### Key objects you will keep meeting (current IDs) + +IDs below are current in this repo at playbook creation time. Prefer these +names over stale object IDs from bug text. + +| Object | Type | Current ID | Area | Notes | +|---|---:|---:|---|---| +| `Manufacturing Setup` | table/page | **99000765 / 99000768** | Setup | Order no. series, SKU cost loading, capacity UOM, flushing defaults, no-output finishing. | +| `Production Order` | table | **5405** | Orders | Header status/source; statuses use enum 5405. | +| `Production Order Status` | enum | **5405** | Orders | Simulated, Planned, Firm Planned, Released, Finished. | +| `Prod. Order Line` | table | **5406** | Orders | Item line; carries `Production BOM No.`, `Routing No.`, `Finished Qty. (Base)`, `Scrap %`. | +| `Prod. Order Component` | table | **5407** | Components | Material demand; key filters include Status, Prod. Order No., Prod. Order Line No. | +| `Prod. Order Routing Line` | table | **5409** | Routing/capacity | Operation demand; Previous/Next Operation No. matter for parallel routing. | +| `Prod. Order Capacity Need` | table/page | **5410 / 99000820** | Capacity | Planned operation capacity demand. | +| `Production BOM Header` | table | **99000771** | BOM | Header status and version no. series. | +| `Production BOM Line` | table | **99000772** | BOM/components | Version Code + Line No. filtering is easy to get wrong. | +| `Production BOM Version` | table/page | **99000779 / 99000809** | BOM versions | Certification must validate version lines too. | +| `BOM Status` | enum | **99000771** | BOM | Used by header/version certification gates. | +| `Routing Header` | table | **99000763** | Routing | Has Type Serial/Parallel and version no. series. | +| `Routing Line` | table | **99000764** | Routing | Has Standard Task Code and operation sequencing. | +| `Routing Version` | table/page | **99000786 / 99000810** | Routing versions | Version-line pages mirror routing-line page behavior. | +| `Work Center` | table/card/list | **99000754 / 99000754 / 99000755** | Capacity | Parent for machine centers; has calendars/load. | +| `Machine Center` | table/card/list | **99000758 / 99000760 / 99000761** | Capacity | Capacity unit often resolves through parent work center. | +| `Calendar Entry` | table | **99000757** | Capacity | Keyed by Capacity Type + No. + Date for "available until" style checks. | +| `Capacity Ledger Entry` | table | **5832** | Posting/capacity | Posted output/capacity cost source for WIP and Power BI. | +| `Capacity Unit of Measure` | table | **99000780** | Capacity | `Code` is not the same as `Type`; this caused Power BI blanks. | +| `Item Journal Line` | table | **83** | Posting | Consumption/output journal line carrier. | +| `Flushing Method` | enum | **5417** | Posting/flushing | Manual, Forward, Backward, Pick+ variants. | +| `Consumption Journal` | page | **99000846** | Posting | Manual component consumption entry. | +| `Output Journal` | page | **99000823** | Posting | Manual output/capacity posting entry. | +| `Production Journal` | page/codeunit | **5510 / 5510** | Posting | Combined consumption/output UI path. | +| `Requisition Line` | table | **246** | Planning | Planning worksheet/carry-out line. | +| `Planning Worksheet` | page | **99000852** | Planning/MRP | Carries action messages into orders. | +| `Planning Component` | table/list | **99000829 / 99000861** | Planning/MRP | Component demand before order creation. | +| `Stockkeeping Unit` | table/card | **5700 / 5700** | Planning/cost | SKU-level `Production BOM No.` and cost loading are separate paths. | +| `Calculate Low-Level Code` | codeunit | **99000793** | Planning/MRP | Must traverse Item and SKU BOM links. | +| `Mfg. Carry Out Action` | codeunit | **99000818** | Planning/MRP | Creates prod. orders from requisition/planning lines. | +| `Calculate Standard Cost` | codeunit | **5812** | Costing | SKU and non-inventory material paths differ. | +| `Mfg. Cost Calculation Mgt.` | codeunit | **99000758** | Costing | Routing/cost-time/expected-cost calculations. | +| `Copy Production Order Document` | report | **99003802** | Orders | Request-page lookup can keep stale filters. | +| `Inventory Valuation - WIP` | report | **5802** | WIP reporting | Production Order - WIP financial report. | +| `Inventory Valuation - WIP CZA` | report | **31133** | WIP reporting | CZ sibling with same stale-value trap. | +| `Calculate Work Center Calendar` | report | **99001046** | Capacity | Card action / request filter path. | +| `Calc. Machine Center Calendar` | report | **99001045** | Capacity | Card action / request filter path. | +| `Standard Cost Worksheet` | table/page | **5841 / 5841** | Costing | Single-level material/capacity cost calculations. | +| `Assembly Header` / `Assembly Line` | tables | **900 / 901** | Assembly overlap | Assemble-to-order copy path only. | +| `Assemble-to-Order Link` | table | **904** | Assembly overlap | Link that must survive sales-quote copy. | + +### Reliable manufacturing markers / fields + +- **Always filter production-order children by line when the child is line-scoped.** + `Prod. Order Component` and `Prod. Order Routing Line` bugs commonly appear when + code uses only Status + Prod. Order No. and accidentally crosses production + lines. An earlier WIP fix read the `Prod. Order Line` with + `Finished Qty. (Base) = 0`; a related subcontracting fix hit the same + `Prod. Order Line No.` trap. +- **Do not confuse capacity UOM `Code` with capacity UOM `Type`.** Manufacturing + Setup stores `Show Capacity In` as a code, while Power BI time factors are keyed + by type. Custom codes like `TIMER` must resolve through `Capacity Unit of + Measure.Type`. +- **SKU-level production definitions are first-class.** A valid SKU can carry a + `Production BOM No.` even when the Item path is blank or different. Low-level + code, standard cost, and planning must include SKU loops and permissions. +- **Routing line descriptions are operation data, not just work-center names.** If + a requisition/planning path is sourced from a `Prod. Order Routing Line`, prefer + the operation descriptions where the line/work center matches; only fall back to + the work center for the documented blank/mismatch cases. +- **Parallel routing is real UI state.** Previous/Next Operation No. are hidden for + Serial but must be visible on Routing and Routing Version lines when the header + Type is Parallel. + +--- + +## §1 The loop that worked + +1. **Identify the manufacturing link where the bug lives.** Definitions, + planning, order creation, journal posting, capacity display, and WIP reporting + each have different keys. Do not fix a report symptom by changing posting until + you have proven posting is wrong. +2. **Find the sibling path that already behaves correctly and mirror its filter.** + Event availability from Item Card/Requisition Line already cleared the date + filter; production-order line/component actions needed the same behavior. Sales/Purchase/Assembly copy-document reports already excluded the + current document; production order copy needed the same lookup pattern plus + stale-filter cleanup. +3. **Check W1 and localized copies before declaring a fix complete.** The WIP + stale-consumption reset had to be applied to W1 report 5802 and CZA report + 31133. Routing-description logic was correctly applied identically + to W1 and IT copies. +4. **For setup-driven defaults, preserve precedence.** New defaults belong in + Manufacturing Setup only when the document/header has no explicit value; never + backfill existing headers silently. +5. **Follow review rounds to final state.** An earlier fix looked acceptable until a + later round found a missing `Stockkeeping Unit` read permission; final accept + came only after the permission matched the new SKU traversal. The + production code was acceptable, but test rewrites temporarily removed negative + no-series and print assertions before most were restored. + +--- + +## §2 Production orders and order documents + +- **Self-copy belongs in the lookup, not as a late error.** `Copy Production Order + Document` showed the current order when source status equaled target status, so + users could select it and only fail later. Exclude the target `No.` in the lookup + like Sales/Purchase/Assembly/Inventory copy-document patterns — and clear that + filter when the source status changes, or a stale `No.` filter can hide a valid + order in another status. +- **Released production orders from planning are not just a new enum label.** The + carry-out flow must set status through `SetProdOrderStatus`, use the released + order no. series, include released orders in the print filter, and run forward + flushing after line/component creation. Tests that only prove the report handler + fired are weak; prove the released order reaches the print dataset. +- **No-series negative cases matter for planning carry-out.** An earlier change briefly + removed the released-order no-series regression; review asked to restore it + because a missing released no. series should fail before creating orders. +- **Production Order - WIP vs embedded Power BI WIP are different user surfaces.** + Renaming the embedded page caption to `Production Order WIP (Power BI)` fixed a + Tell Me ambiguity without changing the page object name or AL references. If a bug is search/discoverability, do not touch report 5802. + +--- + +## §3 Routings, routing versions, and standard tasks + +- **Parallel routing fields must be controlled by the parent routing type.** The + line subpages need a Boolean from the parent Routing/Routing Version page so + `Previous Operation No.` and `Next Operation No.` show for Parallel and stay + hidden for Serial. This is page-state logic; table data was not the problem. +- **Routing and Routing Version pages must be tested separately.** An earlier fix changed + both normal routing lines and routing version lines; the review blocked until + TestPage coverage could assert visibility for both Serial and Parallel. +- **Validating `Routing Line.Standard Task Code` copies more than a code.** It + deletes/inserts Routing Tool, Routing Personnel, Routing Quality Measure, and + Routing Comment Line records from Standard Task relations. Demo-data or repair + helpers that validate the field need permissions for both routing relation + tables and standard-task relation tables. +- **Rerunnable demo-data fixes must update existing routing lines.** An earlier fix first + inserted Standard Tasks but returned early when a routing line already existed, + leaving upgraded/rerun companies with blank Standard Task Code. The accepted fix + backfilled blank existing lines, temporarily reopened certified routings, + preserved descriptions, and then restored status. +- **Do not overrule documented operation mappings.** In an earlier fix, review questioned omitted Standard Task Codes for later parallel/subcontracting demo + operations; the author documented that those operations were outside the mapping, + and the suggestion was treated as disputed rather than blocking. For the next + bug, match the reported operation map, not every visually similar operation. +- **Routing description preservation has an intentional asymmetry.** In the + requisition-line update path, routing `Description` is copied even when blank; + routing `Description 2` falls back to work center `Name 2` when blank. Tests + were added that lock in that asymmetry. If a later agent "normalizes" both + fields, it may reintroduce the reported bug. + +--- + +## §4 Production BOMs, BOM versions, and components + +- **Certification checks must include Production BOM Versions, not just headers.** + A variant-mandatory item on a Production BOM Version line with blank Variant Code + must block certification before status is persisted. Scope the line loop to the + current Version Code, run before Modify/Commit, and mirror the header validation + pattern. +- **Event context should match the header event shape.** An earlier fix added + `OnBeforeCheckVariantIfMandatory`; the review asked to pass the old version + record by value because it is context only and should match the same event on + `Production BOM Header`. +- **Production BOM Comment Line relations must filter by Version Code.** The old + relation let a `BOM Line No.` from a different version validate. Tightening the + TableRelation is a data-integrity fix, but remember it can reject legacy comment + rows that only existed because the relation was buggy. +- **SKU-level BOMs must participate in low-level-code traversal.** The planning + worksheet needing a second regenerative run was caused by Calculate Low-Level + Code ignoring Production BOMs stored on SKUs. Add both upward and downward SKU + traversal, deduplicate multiple SKUs that point at the same BOM, and persist the + recalculated item low-level code from SKU `Production BOM No.` validation. +- **A missing SKU BOM path may be unreachable for domain reasons.** Review + initially flagged passing a blank BOM record to `SetRecursiveLevelsOnBOM`; that concern was + withdrawn because SKU `Production BOM No.` is table-relation validated and the + existing `Status = Certified` guard prevents writes. Before "fixing" a scary + branch, compare sibling procedures and validation gates. +- **Planning Component is a separate pre-order table.** If a BOM/component bug only + appears before carry-out, look for `Planning Component` propagation as well as + `Prod. Order Component`. The subcontracting playbook has the analogous field- + propagation trap; for non-subcontracting, use it as a reminder to enumerate all + BOM → planning → production-order transfer paths. + +--- + +## §5 Capacity, work centers, machine centers, calendars, and Power BI + +- **Calendar-entry "available until" FlowFields key on capacity type and center no.** + Work Center and Machine Center fields should filter `Calendar Entry` by matching + `Capacity Type` + `No.` and use the key that includes Date so MAX(Date) returns + the latest calendar date. This made stale calendars visible before scheduling + fails. +- **Card actions must pass a narrowed table view to the existing calendar reports.** + Work Center Card runs report 99001046 filtered by current `No.`; Machine Center + Card runs report 99001045 filtered by current `No.`. Request pages expose Work + Center Group Code for work centers and Work Center No. for machine centers. +- **Capacity display conversion is not a persistence change, but it can break public + page procedures.** An earlier fix added `Capacity Shown In` to Work/Machine Center + calendar/load pages. The old 3-parameter `Load` / `SetLines` overloads must stay + compatibility wrappers; they should not suddenly read Manufacturing Setup, + convert values, or `TestField("Show Capacity In")` for existing extension callers. +- **Load % is not a capacity quantity.** Conversion tests needed to + prove Work Center and Machine Center values are scaled by TimeFactor while Load % + stays unchanged. +- **Machine Center conversion may need the parent Work Center.** the accepted + direction resolved the capacity unit through the parent Work Center for Machine + Center display. Do not assume the machine center alone carries all conversion + context. +- **Power BI measures must use capacity UOM type, not setup code.** An earlier fix corrected + blanks when Manufacturing Setup `Show Capacity In` was a custom code like + `TIMER` by joining `Manufacturing Setup - PBI API` to `Capacity Unit of Measure` + and exposing `code` + `type`; all 21 measures across Work Center, Machine Center, + Capacity Ledger Entries, Prod Order Capacity Need, and Production Orders then + lookup time factors by type. +- **Power BI setup joins should degrade gracefully.** Review warned + that an inner dataitem join can hide the whole manufacturing setup row if `Show + Capacity In` is blank or points at a missing capacity UOM. Prefer a left-outer + shape when the visible setup row is still meaningful. + +--- + +## §6 Consumption, output, WIP, finished-without-output, and scrap + +- **Report variables that describe one Value Entry must be reset for every Value + Entry, including non-WIP rows.** Report 5802 reused stale `ValueOfMatConsump` + after a consumption entry when a non-WIP value entry followed, making reported + material consumption differ from Value Entries. Move resets before the WIP check; + apply the same pattern to CZA report 31133. +- **Do not reset production-order accumulators just because one variable was stale.** + In an earlier fix, `ValueOfRevalCostAct` and `ValueOfRevalCostPstd` were *not* the same + per-record reset problem; they accumulate for the production order and are used + by `ValueEntryOnPostDataItem`. Resetting them per value entry would change the + calculation. +- **Finished without output is a legitimate Manufacturing Setup scenario.** Report + 5802 had to move WIP cleared by finishing a production order without output into + an Expensed WIP column and remove it from ending WIP, Consumption, and Capacity + columns. Detection must be per production order line with `Finished Qty. (Base) = + 0`, not just per order header. +- **Capacity cost must follow no-output reclassification too.** An earlier fix first + handled material, but review found capacity still counted in both Capacity and + Expensed WIP. The accepted fix added capacity to `OrderExpensedCap` and + subtracted it from `ValueOfCapSum` / `TotalValueOfCap`. +- **Mixed orders are the dangerous test shape.** A production order with one line + that has output and one line finished without output catches order-level WIP + detection mistakes. An earlier fix added this mixed-order test after review. +- **The corpus is thin on scrap-specific bugs.** The reliable lived lesson is that + scrap affects both material and routing/capacity calculations through the same + manufacturing cost functions; when changing scrap behavior, look at the + production-order line, component, and routing/capacity paths together. No + standalone scrap fix in this corpus established a more specific rule. + +--- + +## §7 Flushing and production journals + +- **Forward flushing after planning carry-out must happen after line/component + creation.** The released-order carry-out path kept tests that assert the + component is consumed; if you create released orders directly and forget the + forward-flush timing, the order exists but material is not consumed. +- **Released & Print is still a production-order creation path.** Do not fork a + print-only path that skips flushing or status/no-series logic. The final + shape kept one status mapping and one carry-out production-order creation flow, + then verified print inclusion separately. +- **Production Journal / Output Journal / Consumption Journal have little direct + corpus coverage.** For the next bug in these pages, derive behavior from the + posted ledger/report symptom: earlier fixes prove report 5802 can be + wrong even when the journal posting flow is correct. Do not change journal + posting to fix a report-only stale-variable bug. + +--- + +## §8 Planning worksheet, MRP, SKU cost, and standard cost + +- **Event availability from production orders must clear the inherited due-date + filter.** Opening Item Availability by Event from prod. order lines/components + inherited `Item."Date Filter" = 0D..Due Date`, hiding demand after the production + order Due Date. Clear the date filter like the Period view and requisition-line + Event path; cover both line and component actions. +- **Planning worksheet low-level codes must see SKU-only multi-level BOM chains.** + The one-run MRP result depends on Calculate Low-Level Code assigning 0/1/2 levels + through SKU Production BOMs; otherwise a second regenerative run is needed before + dependent component supply appears. +- **New table reads in planning code need object permissions.** An earlier fix added SKU + traversal but initially missed `TableData "Stockkeeping Unit" = r` on the + codeunit. The fix was one line, but without it users could hit a runtime + permission error exactly in the fixed scenario. +- **SKU manufacturing cost loading is opt-in setup.** In Standard Cost Worksheet, + do not overwrite SKU costs when Manufacturing Setup says SKU manufacturing costs + are loaded separately. The visible review finding was only a message + `Text[250]` overflow, but the domain scenario was the SKU/material/capacity cost + split. +- **Manufacturing cost events must cover both inventory and non-inventory material + cost paths.** Handled events were added for SKU routing cost, direct unit + cost, cost-time inputs, and expected production-order costs. The first round + missed the overload with `ExpNonInvMatCost`; the accepted fix added + `OnBeforeCalcProdOrderLineExpCostWithNonInvMatCost`. +- **IT and W1 cost paths can need different context.** A localized path had to pass + `MainItem` through the IT `OnCalcRtngCostSKUOnBeforeCalcRtngLineCostSKU` event + because that IT SKU route uses `MainItem` to resolve subcontractor prices. W1 did + not need the parameter because its overload does not take it. This is a + cross-reference only; if the bug is actually subcontractor-price calculation, + use the Subcontracting playbook. + +--- + +## §9 Manufacturing setup, versions, demo data, and Assembly overlap + +- **Manufacturing version defaults are optional and non-backfilling.** An earlier fix + added Manufacturing Setup defaults for production BOM version and routing version + number series. New headers inherit only when their own version series is blank; + explicit header values win; blank setup preserves old behavior; existing headers + are not backfilled. +- **Production definition wizard and Contoso data are setup consumers.** An earlier fix + covered wizard-created headers and Contoso PV10/RV10 generation. If a setup + default changes header insert logic, update helper setup order so common/finance + setup exists before manufacturing setup seeds version numbers. +- **Produced-item demo data belongs in the Manufacturing module.** An earlier fix added a + `PRODUCED` item template through demo-data codeunit 5310 before manufactured + items are created. It reused the existing helper with a new overload and kept the + old signature unchanged. +- **Demo-data overloads must preserve old callers.** The final round called + out that the old `InsertItemTemplateData` signature stayed unchanged, while the + manufacturing overload supplied planning/manufacturing fields. Follow that + pattern for future demo-data additions. +- **Assembly overlap is copy-document permission/link preservation, not production + posting.** earlier fixes corrected Team Member copying of sales quotes with + assemble-to-order links by granting narrow inherent permissions on the copy paths + that read/recreate `Assembly Header`, `Assembly Line`, and + `Assemble-to-Order Link`. The tests had to include resource components, + item-component ATO, and archived quote copy. +- **Resource-only Assembly tests are not enough.** The first round used a + resource-only BOM; review asked for item-component ATO because availability and + component reads are the typical path. The later rounds added it and were accepted. + +--- + +## Recurring agentic-review findings — fix these before proposing a change + +- **Compatibility wrappers must remain compatibility wrappers.** If you add a new + overload for capacity display, demo data, or manufacturing setup defaults, keep + the old public signature behavior unchanged. The capacity-display change remained blocked because + old `Load` / `SetLines` overloads started reading setup and converting values; + the demo-data change was accepted after keeping the original demo-data overload intact. +- **Clear filters you add to request pages/lookups.** An earlier fix added a self-document + exclusion filter but review caught that changing status could leave a stale `No.` + filter. If a request page can be reused after a field change, remove the old + filter in the else path. +- **When you add a new table read to a codeunit, update its permissions.** SKU reads + in `Calculate Low-Level Code` needed `TableData "Stockkeeping Unit" = r`. Standard Task validation copied routing-relation rows and needed those + relation-table permissions. +- **Apply fixes to localized sibling objects with the same caption/logic.** WIP + stale-value fixes needed W1 report 5802 and CZA report 31133. Routing + description fixes needed W1 and IT copies. Capacity calendar actions + touched W1, IT, and CZ areas. +- **Tests must hit the branch you changed, not just the headline scenario.** An earlier fix changed line and component availability but initially tested only the line; + another changed two routing pages and needed UI tests for both; the API + test already created the custom-code capacity UOM but did not assert the new + `type` column. +- **Do not weaken tests while aligning a change.** An earlier change briefly removed negative + no-series coverage, print-dataset verification, and `AssertEmpty` in a + multi-order test. Most were restored after review; print still had a weaker + assertion. +- **Use the net change diff when reviewing later rounds.** the later + review explicitly checked that the test-only delta was in the net change diff before + treating removed assertions as authored changes. Avoid treating base-branch churn as authored changes. +- **For display conversions, test the unchanged values too.** Capacity quantities + should scale by TimeFactor, but Load % should not. Power BI visible + text changing from code to type must be confirmed, not assumed. +- **A data-integrity TableRelation fix can expose old bad data.** The BOM + Comment Line relation tightening was correct, but the review still called out + that legacy rows created under the buggy relation may fail revalidation. +- **For no-output/finished-order bugs, include mixed production lines.** Order-level + tests can pass while line-level WIP is wrong. An earlier fix needed a mixed output / + no-output order to prove the per-line logic. +- **Assembly copy fixes need resource, item-component, and archive variants.** The fix was only accepted after item-component and archived quote scenarios were + covered; the later fix started with resource + item-component coverage. diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/subscription-billing.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/subscription-billing.md new file mode 100644 index 000000000..adbd46f7f --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/subscription-billing.md @@ -0,0 +1,389 @@ +# Subscription Billing Bug-Fix Playbook (BC / NAV) + +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Subscription +> Billing** bugs in `this repo` (W1 `Subscription Billing` app: +> subscription contracts, service objects/commitments, recurring billing, +> usage-based billing, billing proposals/lines, contract deferrals, price +> updates, renewal and termination). +> +> This is **not** an AL language guide. It only carries Subscription +> Billing-specific knowledge and the concrete lessons — what worked and what +> didn't — from past fixes. Generic AL rules and anything the compiler, +> publisher/deployment, analyzer rules, or permission-set checks will flag on +> their own are deliberately left out. + +--- + +## 0. The area in one picture + +Subscription Billing is one W1 app, but most bugs sit on the seam between two +record generations: + +1. **Source/subscription generation** — the commercial state the user owns: + `Subscription Header`, `Subscription Line`, customer/vendor subscription + contracts and contract lines, service-object data, renewal/termination dates, + price-update templates, and usage-data imports. +2. **Generated billing/posting generation** — temporary or transactional state + produced from the source: `Billing Line`, `Usage Data Billing`, sales and + purchase documents, deferral schedules, posted-document extension fields, + overdue/analysis rows, and renewal quote buffers. + +**Decide which generation a bug is in before touching code.** A large share of +fixes were not "the amount formula is wrong"; they were "path X copied the +source value and path Y didn't", "the generated row was filtered differently", +"a page buffer overwrote the user's pending value", or "a cached/generated +answer leaked into the next line/document". + +### Key objects you will keep meeting (current IDs) + +IDs below are current in repo-relative `src/Apps/W1/Subscription Billing/App`. +The app owns object range **8000..8113** (`app.json`). Verify less-common objects. + +| Object | Type | Current ID | App area | Notes | +|---|---|---:|---|---| +| `Subscription Header` | table | **8057** | Service Objects | Source service-object header. | +| `Subscription Line` | table | **8059** | Service Commitments | Source commitment line: dates, price, quantity, discounts, billing base period, renewal/termination fields. | +| `Sales Subscription Line` | table | **8068** | Sales Service Commitments | Sales-document-side commitment line; BOM explosion and FCY bugs land here. | +| `Customer Subscription Contract` | table | **8052** | Customer Contracts | Customer contract header copied into sales billing documents. | +| `Cust. Sub. Contract Line` | table | **8062** | Customer Contracts | Customer contract line linked back to `Subscription Line`. | +| `Vendor Subscription Contract` | table | **8063** | Vendor Contracts | Vendor contract header; mirror sales fixes here when applicable. | +| `Vend. Sub. Contract Line` | table | **8065** | Vendor Contracts | Vendor-side contract line linked back to `Subscription Line`. | +| `Billing Line` | table | **8061** | Billing | Generated billing candidate; holds net amounts and source links. | +| `Billing Line Archive` | table | **8064** | Billing | Billing history; existence matters even when amount is zero. | +| `Billing Proposal` | codeunit | **8062** | Billing | Builds billing lines and calculates billing periods. | +| `Create Billing Documents` | codeunit | **8060** | Billing | Creates sales/purchase invoices/credit memos from billing lines. | +| `Billing Template` | table | **8060** | Billing | Automation/error context; interactive errors need record identity. | +| `Contract Billing Err. Log` | table | **8022** | Billing | Non-interactive/automated billing errors land here. | +| `Usage Data Billing` | table | **8006** | Usage Based Billing | Generated usage-billing rows; `Processing Status` is a correctness boundary. | +| `Usage Data Import` | table | **8013** | Usage Based Billing | Import header; status updates must stay scoped to one import. | +| `Create Usage Data Billing` | codeunit | **8023** | Usage Based Billing | Creates usage billing candidates. | +| `Process Usage Data Billing` | codeunit | **8026** | Usage Based Billing | Updates subscription quantity/price/cost from usage data. | +| `Usage Based Billing Mgmt.` | codeunit | **8029** | Usage Based Billing | Helper surface for connector and billing flows. | +| `Usage Based Pricing` | enum | **8007** | Usage Based Billing | Extensible boundary; `None` is the exclusion, not the upper bound. | +| `Processing Status` | enum | **8012** | Usage Based Billing | `Error` lines must not be processed or reset by unrelated imports. | +| `Price Update Template` | table | **8003** | Contract Price Update | Stores filters for price-update proposals. | +| `Contract Renewal Selection` | page | **8006** | Contract Renewal | Temporary-buffer page for renewal terms. | +| `Sub. Contr. Renewal Subcribers` | codeunit | **8001** | Contract Renewal | Sales-post subscribers; renewal detection can be expensive and cached. | +| `Contract Deferrals Release` | report | **8051** | Deferrals | Releases contract deferral schedules to G/L. | +| `Cust. Sub. Contract Deferral` | table | **8066** | Deferrals | Customer deferral schedule. | +| `Vend. Sub. Contract Deferral` | table | **8072** | Deferrals | Vendor deferral schedule; keep customer/vendor logic symmetric. | +| `Assign Service Commitments` | page | **8065** | Service Commitments | Dialog opened from subscriptions or sales lines; caption context matters. | +| `Extend Contract` | page | **8002** | Customer Contracts | Page parameter setters are the integration seam. | +| `Sales Line` | tableextension | **8054** | Sales Service Commitments | Holds subscription fields and `IsLineAttachedToBillingLine()`. | +| `Purchase Line` | tableextension | **8065** | Sales Service Commitments | Purchase mirror of billing-line attachment checks. | + +### Reliable Subscription Billing markers and seams + +- A sales/purchase line being "owned by billing" is represented through + `IsLineAttachedToBillingLine()` / the billing-line link, not by a generic item + or document-type guess. On `Sales Line`, that helper has caching history — make + the public result key-aware before exposing or reusing it. +- `Billing Line` amounts are net/VAT-exclusive. If the generated document has + `Prices Including VAT = true`, convert before assigning document line prices, + and do it in **all four** paths: sales, purchase, usage-sales, usage-purchase. +- `Usage Data Billing`.`Processing Status = Error` is a hard exclusion. Do not + process it, do not flip it back to OK through a broad status reset, and do not + let one import clean another import's errors. +- `Usage Based Pricing` is extensible. `None` is the lower boundary to exclude; + an upper bounded enum filter silently rejects partner values above the current + last base enum value. +- Billed/archived state is about **record existence**, not amount totals. A + zero-value archived billing line is still billing and can lock date edits. +- Renewal and extend-contract pages use temporary buffers. Page refresh triggers + can reload persistent `Subscription Line` and wipe pending user edits unless + the current buffered row is read first. + +--- + +## 1. The loop that worked + +1. **Read the bug context and identify the exact path.** Subscription + Billing usually has parallel customer/vendor, sales/purchase, standard/usage, + per-contract/per-customer, and page/API/import paths. The bug is often one + missing sibling path, not the shared helper. +2. **Find the mirror path and compare it line-for-line.** Good fixes mirror the + already-correct sibling: customer deferral → vendor deferral, + sales document header → purchase document header, standard billing + price assignment → usage-based billing price assignment. +3. **Keep generated-state filters scoped to the source row/import/document.** If + you touch status updates, billing-line links, or renewal caches, key them by + the stable source identity and clear them at document/import boundaries. +4. **When widening extensibility, prove the new surface is deterministic.** A + public helper with hidden cache requirements or an extensible enum with no + runtime branch is worse than no API. +5. **For amount fixes, trace source → billing line → document line → posted/deferral + release.** The same visible invoice amount can be set in standard billing, + usage billing, deferral release, or renewal quote calculation; cover the one + the bug actually reaches. + +--- + +## 2. Billing proposals and billing document creation + +- **Non-progress billing-period loop:** `BillingProposal.CalculateBillingPeriod` + could recalculate the same `BillingPeriodEnd` forever when a harmonized customer + contract's `Next Billing To` capped the period before the requested billing + date. Fix: keep the previous end date and only loop while the recalculation + actually moves forward. The capped billing line should still be created and + advance the subscription from that line. +- **Large-run performance is not just keys:** scale + fixes touched proposal creation, document creation, usage-data links, progress + tracking, field loading, transaction checkpoints, and cached reads. Review kept rejecting broad + speedups without functional proof because billing amounts, document links, + usage-data links, and extension-visible pricing events are financial behavior. + If you optimize this area, prove customer and vendor documents, usage lines, + links, and new transaction-checkpoint semantics. +- **Do not bypass pricing/UoM hooks silently:** the `Billing Price Calc. + Skip` idea set handled on sales/purchase price, cost, and UoM events while + billing lines were initialized. That can bypass subscriber adjustments to + price, cost, quantity, or UoM. Add an opt-out/integration point or prove the + supported billing result stays identical. +- **New transaction checkpoints change rollback semantics:** + adding a transaction checkpoint after each created billing document can intentionally preserve earlier + documents and billing-line updates after a later failure. Treat that as a + behavior change; test at least two documents where the second fails and assert + the first is intentionally preserved. +- **No-GUI/Job Queue is a separate billing path:** dialog/progress + changes must run under `GuiAllowed = false` and still create documents or log + errors. Do not validate only the interactive page path. +- **External Document No. has two sales-header paths:** per-contract + billing uses `CreateSalesHeaderFromContract`, where `TransferFields(CustomerContract, + false)` copies the new field and a subsequent `Validate("External Document No.")` + retriggers base validation. Per-customer grouped billing uses + `CreateSalesHeaderForCustomerNo`, where the contract must be looked up and the + value validated explicitly. If you add contract-header fields, cover both paths. +- **Payment discount belongs to the contract terms:** recurring + invoices were inheriting the customer/vendor payment discount instead of the + contract's payment terms. The sales side needed a temporary reassignment because + W1 `Sales Header` validation checks `xRec`; the purchase side only needed a + re-validate after `Document Date` was set. Run the recalculation after document + date validation and assert terms code, discount %, and discount date. +- **VAT-inclusive document prices require gross-up:** billing lines + store net values. When `Sales Header` / `Purchase Header`.`Prices Including VAT` + is true, gross up with VAT % and round to Currency `Unit-Amount Rounding + Precision` before assigning line price. Apply to standard and usage-based sales + and purchase paths; skip Full VAT. Usage-based VAT-inclusive coverage was the + review gap. +- **Interactive errors must raise populated `ErrorInfo`:** + `CreateBillingDocuments` already built error identity for Billing Template and + Billing Line failures but discarded it by calling `Error(ErrorText)`. Interactive + paths should raise the populated `ErrorInfo` so the client can navigate to the + record. Automated billing keeps logging to `Contract Billing Err. Log`. Cover + both `DisplayOrLogErrorFromBillingTemplate` and `DisplayOrLogErrorFromBillingLine`. +- **Configurable billing-period text is contract-type data:** the + Billing Period Description belongs on the subscription contract type and should + reuse the existing Field Translation pattern. Blank values stay on the standard + label path; when billing creates sales/purchase document lines, resolve text + using the contract type from the billing contract. +- **Day/week periods do not snap to month end:** explicit + `Subscription Line End Date` invoice amounts were wrong because day/week + formulas were treated like month rhythms. For `D` and `W` date formulas, use the + plain period end; keep month/quarter/year month-end alignment. Include leap-year + and month-end cases. + +--- + +## 3. Service commitments, sales lines, and assignment flows + +- **Assign dialog must know whether it was opened from a sales line:** + page **8065** `Assign Service Commitments` uses the sales line number and + description in `DataCaptionExpression = GetCaption()` only when + `OpenedFromSalesLine` is true; the subscription-header path falls back to the + package code. Do not make a caption fix that breaks the subscription-header + dialog. +- **BOM explosion prompt belongs after the component line exists:** + the correct event order is `Sales-Explode BOM`.`OnExplodeBOMCompLinesOnAfterAssignType` + before `No.` validation and `OnExplodeBOMCompLinesOnAfterToSalesLineInsert` + immediately after `Insert()`. Record the component line before `No.` validation, + skip the early validation path only for that line, clear state after insert, + then create subscription lines from the inserted sales line with quantity. +- **Foreign-currency BOM explosion must use the cached sales line date:** + a BOM component creating a `Sales Subscription Line` for an FCY customer failed + when `GetDate()` hard-read the sales line before it was safely persisted. Use + the same cached Sales Line helper as the other calculations; keep the FCY + exchange-rate formula and unit-amount rounding unchanged, and leave `Get()` as + the normal persisted-line fallback. +- **`IsLineAttachedToBillingLine()` cannot expose a stale Sales Line cache:** + Purchase Line was a direct lookup and safe. Sales Line returned a cached Boolean + until `InitCachedVar()` ran, so an external caller reusing one record variable + across lines could get the previous line's result. The accepted fix stored the + line identity (`Document Type`, `Document No.`, `Line No.`) with the cached + value and refreshed when the key changed. +- **Start-date enforcement belongs on the `Subscription Line` field:** page-only checks let imports, APIs, and code paths bypass the + rule. Validate in the table before `UpdateNextBillingDate`, read the persisted + line to know whether the old `Next Billing Date` was still the old start date, + block current billing lines and archived billing-line **existence** (including + zero amount), allow valid correction/unbilled cases, and exempt temporary + renewal buffers. +- **Temporary records are legitimate in this app:** + renewal and selection pages stage changes before applying them. A table-level + guard must explicitly leave temporary buffers usable; a page refresh must prefer + the buffer row over reloading the persistent subscription line. + +--- + +## 4. Usage-based billing and connector extensibility + +- **Exclude `Processing Status = Error` everywhere in processing:** + lines rejected for a currency mismatch must not update subscription quantity, + price, or cost. The robust pattern also changes `SetProcessedUsageDataBillingToOk` + so it rebuilt filters from the `Usage Data Import` entry number and excluded + error rows itself instead of trusting a filtered record passed by the caller. +- **Status reset is scoped by import:** a helper that marks processed + rows OK must be limited to the current import. Do not clear errored lines from + unrelated imports, and keep its filter shape aligned with the main processing + loop. +- **Open-ended usage-pricing filters are the extensibility pattern:** + `SetUsageDataBillingFilters` should include partner `Usage Based Pricing` enum + values above `Unit Cost Surcharge` while still excluding `None`. The accepted + proof used a test-only enum value at ordinal 100 and verified exactly one row: + the extension value was included, `None` was excluded. +- **Expose connector helpers without changing their semantics:** + custom Usage Data Connector apps need to call the same helper procedures used by + the generic connector path, especially around `Usage Data Processing.CreateBillingData`. + The safe change only removes `internal` from existing procedures; it did + not change filters, metadata creation, amount calculation, event timing, or + status handling. +- **Usage-based VAT follows the same gross-up rule as standard billing:** + if the fix touches `SetInvoicePriceFromUsageDataBilling` overloads, add direct + VAT-inclusive usage tests. Standard billing tests do not prove the usage path is + reached. + +--- + +## 5. Contract deferrals, G/L routing, dimensions, and price updates + +- **G/L Account contract lines use the selected line account:** without deferrals, `CustomerDeferralsMngmt` and + `VendorDeferralsMngmt` should early-exit so BaseApp's selected `Sales Line."No."` + / `Purchase Line."No."` remains the posting account. With deferrals, store the + selected account on the deferral and have `ContractDeferralsRelease` prefer it; + blank values on existing deferrals and non-G/L lines fall back to General + Posting Setup. +- **GPS validation must respect deferral-owned accounts:** + `CheckGenPostingSetup` should not demand a setup contract account when the + deferral row already carries its own G/L account. This is what lets a G/L + Account contract line post even when the generic customer/vendor subscription + contract account is blank. +- **Credit memo reversal is a separate G/L-account proof:** earlier review + called out missing explicit tests for credit memos + reversing G/L Account contract lines, with and without deferrals. If you change + deferral account routing, cover the reversal path. +- **Vendor single-period deferrals mirror the already-correct customer logic:** when the billing period stays inside one calendar month, vendor + deferrals must use the exact schedule length before falling through to partial + month or full-month branches. Multi-period schedules keep first/middle/last + period rules. The existing `OnBeforeInsertVendorContractDeferral` override point + still fires after calculated values are set. +- **Default Dimension Priorities must survive the Subscription Line merge:** resolve dimensions through the priority-aware path (`Source Code + Setup` + Dimension Management helpers) like BaseApp Sales/Purchase lines. Keep + `Subscription Line`.`OnAfterGetCombinedDimensionSetID(Rec)` firing immediately + after the combined dimension set is computed, or extensions lose the old hook. + End-to-end proof is a generated invoice line carrying the highest-priority Item + dimension. +- **Price Update Template filters are user input:** + a past bug was that proposal default filters overwrote `Price Update Template` + filters on Subscription Lines. If you touch price-update proposal generation, + verify template filters are applied after/with defaults rather than replaced by + them. + +--- + +## 6. Renewal, termination, and contract extension pages + +- **Sales-post renewal detection must be cached only inside one posting run:** repeated `SalesLine.IsContractRenewal()` and + `SalesHeader.HasOnlyContractRenewalLines()` calls caused O(N²) scans and about + 25-28% batch CPU in renewal posting. Cache wrappers are appropriate, keyed by + line/header `SystemId`, but clear them on both `OnBeforePostSalesDoc` and + `OnAfterPostSalesDoc`; temporary buffers without `SystemId` use the uncached + fallback. +- **Cache reset boundaries need multi-document proof:** post multiple + documents in one run and verify normal lines are still posted correctly after a + renewal document. A stale renewal cache can silently skip invoice/shipment line + insertion or header creation. +- **Renewal Term page edits live in a temp `Subscription Line` buffer:** `ContractRenewalSelection.OnOpenPage` fills the buffer, + `RenewalTermCtrl.OnValidate` writes to it, and `OnAfterGetRecord` must read the + current buffer row by `Subscription Line Entry No.` before loading the stored + line. Otherwise `CurrPage.Update` makes the field appear to reject the user's + value. +- **Use different values on different renewal rows:** a test that + enters the same Renewal Term on two lines does not prove per-line buffering. + Set different terms, move to the next line, then return and assert the first + line kept its own value. +- **Cancellation in days is not month-end rounding:** + a past bug rounded `Cancellation Possible Until` to end of month when `Notice + Period` was expressed in days. If you touch termination math, distinguish day + formulas from month-aligned formulas just as invoice-period math does. +- **Renewal quote totals must honor `Billing Base Period`:** a past bug showed the base-period amount instead of the renewal-term + amount on the Contract Renewal Quote. If you fix renewal totals, trace the + amount period from `Subscription Line`.`Billing Base Period` through planned + and quote lines, not just the visible price field. +- **Expose `Extend Contract` parameters as a page integration seam:** + dependent apps need to open page **8002** and initialize it the same way as the + existing usage-data flows. Widening the two parameter procedures is safe only if + signatures and bodies stay the same and `OnOpenPage` still copies parameters + into page state before validation. + +--- + +## 7. Extensibility lessons specific to Subscription Billing + +- **Do not make closed financial enums extensible unless runtime supports partner + values:** `Rec. Billing Document Type` and `Usage Based Billing Doc. + Type` model real invoice/credit-memo states; posting, deferral, filtering, and + conversion code only understood built-in values. Keep them closed unless the + full document flow handles custom values. +- **Grouping enums need an `else`/event path:** `Customer Rec. Billing + Grouping` and `Vendor Rec. Billing Grouping` feed `ProcessBillingLines()` case + statements. A partner value that compiles but creates no sales/purchase document + is a runtime bug. Add a deliberate `IsHandled` extension point or keep the enum + closed. +- **Do not expose temporary internal state:** helpers such as + `SetUnitPriceAndUnitCostFromExtendContract()` / `ResetCalledFromExtendContract()` + were called out because they reveal page-flow internals, not stable domain + concepts. Prefer exposing a domain operation or the existing page parameter seam. +- **Extensibility regression tests can be tiny but must consume the new contract:** use a test app to call one newly public procedure or add a + test-only enum value. The point is to catch accidental rollback of access and to + prove the runtime path handles partner input. +- **Public cached APIs need key-aware behavior:** once a helper is + callable by external apps, callers should not need to know to call a separate + cache initializer. Direct lookup or key-aware cache refresh is the safe pattern. + +--- + +## 8. Recurring agentic-review findings — fix these *before* handing off the fix + +These are issues automated reviews repeatedly caught in Subscription +Billing changes. Pre-empting them saves review rounds. + +- **Every parallel billing path you changed needs evidence.** Per-contract vs + per-customer grouped sales headers, sales vs purchase headers, standard vs usage-based price assignment, and customer vs + vendor deferrals each had separate code paths. Do not test only the + path that first reproduced the bug. +- **Performance changes must prove behavior, not just speed.** If you add keys, + caches, progress trackers, bulk updates, or transaction checkpoints in billing creation, prove + amounts, document links, usage-data links, pricing/UoM hook behavior, no-GUI + execution, and the intended rollback/transaction boundary. +- **A helper receiving a filtered record is a trap.** Rebuild critical filters + inside the helper from stable identity (`Usage Data Import` entry number, + document SystemId, billing-line key) when the helper changes status or cache + state. `SetProcessedUsageDataBillingToOk` was fixed this way. +- **Record existence beats amount totals for billed-state checks.** Zero-value + archived billing lines still count as billing and must lock the same start-date + edits as non-zero lines. +- **Page buffer tests must distinguish rows.** If the bug is "the current renewal + row overwrote another row," use different Renewal Terms and navigate back. If the test uses the same value everywhere, it proves too little. +- **Scope test cleanup to created subscription lines.** Broad `ModifyAll` on + `Subscription Line` can make renewal/contract tests order-dependent in shared + test companies; limit cleanup or assert the expected count first. +- **Interactive and automated billing errors are different contracts.** Raising + populated `ErrorInfo` is right for interactive Billing Template/Billing Line + paths; automated billing should keep logging behavior. Cover both sibling + interactive helpers when both are changed. +- **Extensible enums need real runtime behavior.** Do not let a partner enum value + compile and then skip document creation or filtering at runtime. Either add a + proper extension point or keep the enum closed. +- **Public access changes are API contracts.** Removing `internal` is acceptable + for stable domain helpers and page parameter seams, but not + for volatile temporary state. Once public, name/signature/semantics + become partner dependencies. diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/warehouse.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/warehouse.md new file mode 100644 index 000000000..5d5a88504 --- /dev/null +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/playbooks/warehouse.md @@ -0,0 +1,395 @@ +# Warehouse / Inventory / Item Tracking Bug-Fix Playbook (BC / NAV) + +> **How to use this guide — read first.** This is a playbook of practical hints, not a source of indisputable truth. Don't spend time confirming or refuting the statements here, and don't go looking for a pull request, work item, commit, author, or date behind any recommendation. Use it as a helper — apply what fits the bug in front of you, and use your own judgment for the specifics. + +> Domain knowledge for the automated bug-fix agent working on **Warehouse / +> Inventory / Item Tracking** bugs in this repo's W1/base-app layers: warehouse receipts/shipments, +> put-away/pick (directed + basic), bins and bin content, warehouse +> activities/worksheets, inventory movements/reclass/transfers, item +> availability, and serial/lot/package tracking with reservations. +> +> This is **not** an AL language guide. It carries only warehouse/tracking +> knowledge and concrete lessons — what worked and what did not — from past +> fixes and reviews. Inventory costing/valuation and manufacturing-internal math +> are deliberately out of scope, except where warehouse handling touches them. + +--- + +## 0. The area in one picture + +The same item quantity can be represented in **three overlapping models** before it finally becomes a posted ledger fact: + +1. **Warehouse handling** — source documents create `Warehouse Request` rows, then worksheet/activity/document lines (`Pick`, + `Put-away`, `Movement`, `Invt. Pick`, `Invt. Put-away`, warehouse receipt/shipment) move quantities between bins and staging + bins. +2. **Inventory / transfer application** — item journals, transfer orders, and source posting create/apply `Item Ledger Entry` + rows. Transfers have outbound, in-transit, and inbound faces; inbound reservation and application are not the same as outbound + availability. +3. **Tracking and reservations** — `Tracking Specification` carries proposed serial/lot/package assignment; `Reservation Entry` + carries availability, source, and paired-reservation state. Warehouse pick lines can reserve or consume tracked stock before + anything is posted. + +**Fix the seam the bug lives on.** A lot of defects were not in the main posting routine but in the glue: source references copied +to warehouse requests, pick worksheet availability excluding the wrong bins, temporary availability entries for unregistered +picks, or tracking status copied as a real reservation when it was only a prospect. + +### One-step vs. two-step handling + +- **Basic warehouse / inventory documents** use inventory picks, inventory put-aways, and movements directly from source/inventory + needs. They often have bins but not directed put-away and pick. +- **Advanced / directed warehouse** uses warehouse receipts/shipments plus warehouse put-aways/picks. Location fields (`Require + Receive`, `Require Shipment`, `Require Put-away`, `Require Pick`, `Directed Put-away and Pick`) decide which documents are + legal. +- **Staging bins are not pickable stock.** Receipt bins and shipment bins are handling bins; pick worksheet and availability logic + must not offer their stock as ordinary available-to-take quantity. +- **FEFO changes bin choice.** In basic warehouse movements with `Pick According to FEFO`, a blank `From Bin` can be meaningful: + the engine chooses the lot/bin. Do not widen a FEFO-specific blank-bin fix to all movements. + +### Tracking model in one paragraph + +`Tracking Specification` (table 336) is the document-line working copy. `Reservation Entry` (table 337) is the +availability/reservation fact and is keyed by source fields plus serial/lot/package values. A copied tracking line is often +`Reservation Status::Prospect`; forcing `Reservation` without the paired entry corrupts the model. Warehouse +activity lines also carry serial/lot/package values and can represent allocated but unregistered demand; item tracking +availability must subtract them or the same lot/package can be chosen twice. + +### Key objects you will keep meeting (current IDs) + +IDs below are current for this area. + +| Object | Type | Current ID | Area | Notes | +|---|---|---:|---|---| +| `Location` | table | **14** | Inventory / Warehouse | Warehouse setup fields: `Require Pick`, `Require Shipment`, `Bin Mandatory`, `Directed Put-away and Pick`, `Pick According to FEFO`, receipt/shipment bins. | +| `Item Ledger Entry` | table | **32** | Inventory | Final posted inventory facts; transfer application must honor selected tracking. | +| `Tracking Specification` | table | **336** | Tracking | Working tracking lines; includes `Source Type`, `Source Subtype`, `Source ID`, `Source Ref. No.`, serial/lot/package fields. | +| `Reservation Entry` | table | **337** | Reservation | Real reservation/prospect rows; wrong status/source/quantity creates orphan or missing entries. | +| `Reservation Entries` | page | **497** | Reservation | Inspection page for table 337. | +| `Reservation` | page | **498** | Reservation | User-facing reservation flow; inbound transfer errors live here. | +| `Available - Transfer Lines` | page | **99000896** | Transfer / Reservation | Separate transfer availability page; do not assume `Reservation` page tests cover it. | +| `Available - Item Ledg. Entries` | page | **504** | Inventory | Selects open ILE application candidates. | +| `Item Tracking Lines` | page | **6510** | Tracking | Main tracking assignment page. | +| `Item Tracking Summary` | page | **6500** | Tracking | Lot/serial/package availability summary fed by codeunit 6501. | +| `Item Tracking Management` | codeunit | **6500** | Tracking | `CopyItemTracking`/tracking-copy status traps. | +| `Item Tracking Data Collection` | codeunit | **6501** | Availability / Tracking | Builds tracking availability; must account for unregistered picks. | +| `Transfer Header` | table | **5740** | Transfer | `Direct Transfer`, `In-Transit Code`; do not validate direct transfer false after route setup unless intended. | +| `Transfer Line` | table | **5741** | Transfer | Source type for transfer reservation/application; use `Database::"Transfer Line"`, not magic `5741`. | +| `Warehouse Request` | table | **5765** | Warehouse source | Source key rows; upgrade/rename source fields only under exact filters. | +| `Warehouse Activity Header` | table | **5766** | Activity | Stores header flags such as `Do Not Fill Qty. to Handle`. | +| `Warehouse Activity Line` | table | **5767** | Activity | Pick/put-away/movement lines; has Activity Type, Action Type, bin, source, tracking, quantity fields. | +| `Warehouse Pick` | page | **5779** | Activity | Advanced pick document. | +| `Warehouse Put-away` | page | **5770** | Activity | Advanced put-away document. | +| `Inventory Pick` | page | **7377** | Activity | Basic pick; `Activity Type = Invt. Pick` matters. | +| `Inventory Put-away` | page | **7375** | Activity | Basic put-away. | +| `Warehouse Movement` | page | **7315** | Activity | Movement activity page. | +| `Warehouse Receipt Header` / `Line` | tables | **7316 / 7317** | Document | Two-step receipt document. | +| `Warehouse Shipment Header` / `Line` | tables | **7320 / 7321** | Document | Two-step shipment document; posted/picked quantities affect availability. | +| `Warehouse Entry` | table | **7312** | Warehouse ledger | Bin quantities, cubage, weight, posted warehouse movements. | +| `Bin Content` | table | **7302** | Bins | Quantity, pick qty, ATO component pick qty, `Block Movement`; keyed by location/bin/item/variant/UOM. | +| `Bin Type` | table | **7303** | Bins | Flags `Receive`, `Ship`, `Pick`; directed locations use these for staging/pickability. | +| `Bin` | table | **7354** | Bins | `Bin Type Code`, capacity/ranking fields. | +| `Whse. Worksheet Line` | table | **7326** | Worksheet | Movement/pick/put-away worksheet rows; template creation bug in an earlier fix. | +| `Whse. Worksheet Name` / `Template` | tables | **7327 / 7328** | Worksheet setup | Template `Page ID` must honor custom caller page. | +| `Pick Worksheet` / `Movement Worksheet` / `Put-away Worksheet` | pages | **7345 / 7351 / 7352** | Worksheet | Worksheet UI over table 7326. | +| `Create Pick` | codeunit / report | **7312 / 5754** | Pick creation | Codeunit creates picks; report drives request/print flow. | +| `Create Inventory Pick/Movement` | codeunit | **7322** | Basic whse | Inventory pick/movement creation, FEFO movement, job availability aggregation. | +| `Warehouse Availability Mgt.` | codeunit | **7314** | Availability | Pick worksheet / shipment-bin availability calculations. | +| `WMS Management` | codeunit | **7302** | Validation | Shared warehouse journal validation including bin-content movement checks. | +| `Whse.-Activity-Post` | codeunit | **7324** | Posting/register | Inventory pick/warehouse activity posting checks. | +| `Whse. Jnl.-Register Line` | codeunit | **7301** | Warehouse journal | Registers warehouse journal lines; cubage/weight must be set before run. | +| `Whse.-Source - Create Document` | report | **7305** | Put-away / movement creation | Internal put-away event trap. | +| `Phys. Invt. Order-Post` | codeunit | **5884** | Physical inventory | Posting should mirror warehouse/transfer link-copy placement. | +| `Inventory Profile Offsetting` | codeunit | **99000854** | Planning / reservation | Requisition base-quantity rounding affects reservation cleanup. | +| `Matched Order Line Mgmt.` | codeunit | **5826** | Receipt matching | Receipt-to-order filters; only adjacent to this playbook, but useful analogy. | + +### Reliable markers and fields + +- Use `Location` setup to decide document legality: `Require Receive`, `Require Shipment`, `Require Put-away`, `Require Pick`, + `Bin Mandatory`, and `Directed Put-away and Pick`. Directed destinations usually should **not** receive a transfer-to bin from + custom workflow setup. +- Use `Bin Type.Receive/Ship/Pick`, not only `Location."Shipment Bin Code"`, when reasoning about directed staging bins. Multiple + Ship-type bins exist in real warehouses. +- Treat `Warehouse Activity Line."Activity Type"` values `Pick` and `Invt. Pick` separately. A blank `Action Type` does not turn + an inventory pick into a normal warehouse pick. +- Preserve the source key as a set: `Source Type`, `Source Subtype`, `Source ID`, `Source Batch Name`, `Source Prod. Order Line`, + `Source Ref. No.`. Partial migration of warehouse source references can corrupt availability or navigation. +- For transfer reservation direction, `Transfer Line` source subtype distinguishes outbound vs inbound; inbound quantities are not + reservable before receipt and must get an inbound-specific message. +- For tracking copies, `Prospect` is usually the right status when copying item tracking without creating the matching reservation + pair. `Reservation` status requires paired positive/negative entries. +- For transfer application, serial/lot/package values selected on the transfer entry must participate in the open-ILE filter + before the candidate entry is picked. + +--- + +## 1. The loop that worked + +1. **Classify the location first.** Basic bin-mandatory, directed put-away and pick, and no-bin locations have different valid + bin/document flows. Many fixes are just "apply this bin only for bin-mandatory non-directed locations". +2. **Find the sibling path and mirror it.** Non-FEFO already filtered Pick bins where FEFO did not; transfer-from bin + validation already had the guard that transfer-to needed; warehouse/transfer posting already showed where to copy + record links. +3. **Follow source references end to end.** A source-type fix must cover creation, item tracking, availability helpers, Show + Source Document, and upgrade rows that carry old keys. +4. **Separate "availability shown" from "posting/application done."** Unregistered picks, receipt bins, shipment bins, and + in-transit transfer lines affect what should be selectable long before ledger posting. +5. **When tracking is involved, test the exact tracked value that survives.** It is not enough that a document is created; assert + the selected lot/package/serial stayed on the purchase/transfer/pick line or consumed the right ILE. +6. **For multi-round bugs, carry reviewer objections forward until the final resolution.** Several good fixes had a correct first + idea but an unsafe scope or a weak test that only later got fixed. + +--- + +## 2. Receipts, shipments, and staging bins + +Symptoms: pick creation says "nothing to handle", pick worksheet overstates stock, serial/lot appendices mix between posted +shipments, or document posting loses header metadata. + +- **Ship-type bins are not pick bins:** a directed location with multiple Ship-type bins failed a second pick because + previous picked quantity sat in a non-default Ship bin. The fix had two parts: add the Pick-only bin type filter to the FEFO + branch in `Create Pick`, and change `Warehouse Availability Mgt.` to sum **all** Ship-type bins in `CalcQtyOnShipmentBins`, not + only `Location."Shipment Bin Code"`. Guard the default-bin fallback so the default shipment bin is not double-counted when it + already has a Ship bin type. The useful test assertion was not just quantity; it asserted the new Take line's `Bin Code` was the + pick bin, not the ship bin. +- **Pick worksheet availability must exclude receive and shipment handling stock:** + `CalcQtyAvailToTakeOnWhseWorksheetLine` needed to ignore current receipt and shipment bins, and to cap bin-content availability + for receive / put-away locations so received-not-put-away quantity stays unavailable even if the receipt bin code changes later. + The remaining review gap was the symmetric shipment-bin-lineage case: if the shipment bin code changes after a pick is + registered, picked-not-shipped quantity in the old shipment bin must still stay unavailable. +- **Batch-printed posted shipment tracking appendix is a layout-scope bug until proven otherwise:** the GB + `SalesShipment.rdlc` fix removed a tablix-level `Start` from the Serial/Lot Number appendix. + Because the symptom was lot rows appearing under the wrong document, review required manual verification with **at least two** + posted shipments in one batch and confirmation that the appendix is scoped under the outer per- document group + (`No_SalesShptHeader` + `OutputNo`). Pagination-only changes are not obviously enough when the symptom is cross-document mixing. +- **Record links on posted physical inventory should mirror posting flows:** `Phys. Invt. Order-Post` had to call + `RecordLinkManagement.CopyLinks` immediately after inserting the posted Phys. Invt. Order Header and each posted recording + header. The acceptable placement matched Sales, Purchase, Warehouse Receipt/Shipment, Transfer, Inventory Document, and + Assembly: copy inside the standard `if not IsHandled then` insert block, so subscribers that replace the insert own link copying + too. + +--- + +## 3. Pick / put-away / activity and worksheet bugs + +Symptoms: wrong quantity to handle, wrong source bin, worksheet rows that do not clear, custom worksheet pages opening with the +wrong template, or event subscribers receiving a stale warehouse record. + +- **Create Pick request flags must reach the header:** the Pick Worksheet path already put `Do Not Fill Qty. to Handle` + into `CreatePickParameters`, and line creation already used it to clear line qty. The missing piece was copying that flag onto + `Warehouse Activity Header` before insert, so registration can still see the user's request. Because the shared `Create Pick` + codeunit also serves Warehouse Shipment, Movement Worksheet, Internal Pick, Production, Assembly, and Job paths, keep the change + at the central parameter-to-header point and test the Pick Worksheet path that lost it. +- **FEFO pick reshuffle must consume picked quantity per lot, not per reservation row:** warehouse pick registration + for nonspecific reservations first tried to keep reservation quantity per lot, but with two reservation entries for the same lot + it subtracted the full picked quantity from each entry. The final fix used a per-lot remaining-quantity dictionary: calculate + picked quantity once per lot, then decrease the remaining amount as each `Reservation Entry` is processed. The regression + created the same lot through two item ledger entries, registered the FEFO pick, posted shipment, and proved the other orders + could still pick/ship. +- **Internal put-away after quality inspection must source current bin content, not stale receive bin:** after a WHITE + receipt is put away, a failed- quantity internal put-away cannot use the original receive bin. The resolver should use positive + `Bin Content` for non-tracked inventory at bin-mandatory locations, excluding receive/adjustment bins, while item-tracked + inventory keeps using `GetCurrentLocationOfTrackedInventory`. If multiple bins qualify, allocate the requested + Specific/Sample/Failed/Passed quantity **once across bins** and error on shortfall; do not copy the full quantity to every bin. + The accepted test asserted no line sourced from RECEIVE and total put-away quantity equaled the failed quantity. +- **Inventory Pick for ATO: skip ATO per line, not for the whole document:** + `Whse.-Activity-Post.CheckQuantityInBinContentForTracking` needed to skip activity lines marked `Assemble to Order`, because + assembly output bin content is created later by `Sales-Post`. A first-line document-level exit was unsafe: if the ATO line was + first, normal tracked lines skipped validation too. Remove the header/first-line exit and rely on the per-line ATO check; test a + mixed pick with the ATO line first. +- **Bin replenishment + FEFO blank `From Bin` needs two narrow fixes:** in codeunit 7322, when FEFO leaves worksheet + `From Bin` blank, availability must not count the destination bin's earliest lot because it cannot move onto itself; and the + handled-line buffer must be recorded under blank `From Bin` so it matches and clears the worksheet row. The final fix gated + destination-bin exclusion on `CurrLocation."Pick According to FEFO"` and only used the blank-bin buffer behavior for FEFO blank + inventory movements. The test reproduced earliest lot split between source and destination, then asserted full movement and + worksheet cleanup. +- **Warehouse worksheet template creation must persist the caller's page:** table 7326 `TemplateSelection` filtered by + the caller's `PageID` but, on first-time template creation, stored the standard page ID. The fix is to validate `"Page ID"` with + the input page in the zero-template path. Because this writes setup data, the scenario-specific regression should start with no + `Whse. Worksheet Template`, call `TemplateSelection(PageTemplate = Movement, custom PageID)`, assert the stored page ID, then + call again and prove no duplicate/failure. If similar warehouse journal/bin-creation template routines keep the old pattern, + state whether scope is intentional. +- **Report 7305 internal put-away event must expose the actual line:** `Whse.-Source - Create Document` raised + `OnBeforeProcessWhseMovWkshLines` inside the `Whse. Internal Put-away Line` dataitem but passed the sibling `Whse. Put-away + Worksheet Line`, which was stale and the wrong record type. The correct event is a dedicated + `OnAfterWhseInternalPutAwayLineOnPreDataItem` at the end of that `OnPreDataItem`, after filters are set. Be careful removing the + old event: even a bad event may have subscribers; keep or explicitly assess it. + +--- + +## 4. Bins, bin content, capacity, and movement blocking + +Symptoms: blocked bins can still be depleted, capacity is bypassed by split movements, or workflow-created transfers carry invalid +bin codes. + +- **`Bin Content.Block Movement` must be enforced on every outbound posting path that posts warehouse journal lines:** negative adjustments from bin-mandatory non-directed locations must check the `From Bin Code` bin content for + `Outbound` or `All`; sales shipment posting must call `WMS Management.CheckWhseJnlLine` before `WhseJnlPostLine.Run`. The lookup + must include Location, From Bin, Item, Variant, and Unit of Measure. Review also called out purchase return shipment as the + sibling outbound path to check; if it is in scope, add the same validation there. Add an inbound control scenario so `Block + Movement = Outbound` still allows positive inbound movement. +- **Capacity checks need cubage/weight on the posted warehouse entry:** `Prohibit More Than Max. Cap.` could be + bypassed by splitting Inventory Movement lines because the first partial registration posted a warehouse entry without + Cubage/Weight; the next capacity calculation did not see the occupied capacity. Fill cubage and weight on the warehouse journal + line with existing `WMSMgt.CalcCubageAndWeight` before `WhseJnlRegisterLine.Run`. The final test used + `WarehouseActivityLine.SplitLine`, registered the first split part, then verified the second split part was blocked. +- **Quality transfer destination bins are only valid for bin-mandatory non-directed destinations:** + the workflow bin flows from `QltyWorkflowResponse.GetWellKnownKeyBin` into the disposition buffer `"New Bin Code"`, then into + `Transfer Line."Transfer-To Bin Code"`. Apply it under a non-empty guard **and** a destination-location guard mirroring the + transfer-from side: destination is `Bin Mandatory` and not `Directed Put-away and Pick`. If the destination changes to a non-bin + or directed location, clear the persisted workflow bin, not only the page variable. Cover switching an already-configured + non-directed destination to a directed one. +- **Do not accidentally turn a routed transfer into a direct transfer:** for quality transfer dispositions, a + direct transfer is derived from an empty in-transit location. When an in-transit code has already been validated onto the + header, do not call `Validate("Direct Transfer", false)` just because the computed flag is false; leave the default false and + route intact. Only validate `Direct Transfer` when the disposition is actually direct. +- **Localized demo in-transit codes need one source of truth:** Inventory and Warehousing Contoso setup created + duplicate Italian own-logistics in-transit locations (`LOG PROP.` vs `LOG. PROP.`) because two translatable labels represented + the same logical code. Warehousing now uses `Create Location.OwnLogLocation` instead of a separate label. This is a forward + fix only; existing duplicates are not cleaned up. + +--- + +## 5. Transfers, reclass, and inventory document seams + +Symptoms: inbound transfer reservation gives a false "fully reserved", transfer receipt applies against the wrong lot/package, or +planning/drop-shipment flows create/delete bad reservation rows. + +- **Inbound transfer lines are not reservable before receipt:** the user-facing bug was a non-direct transfer shipped + but not received; reserving the inbound line showed generic `Fully reserved.` although no inbound reservation existed. Both + `Reservation.Page.al` and `AvailableTransferLines` need the inbound-specific message. Do not gate that message on `Qty. in + Transit (Base) <> 0`; unshipped inbound lines need the same clear explanation. If you add an Available Transfer Lines filter, + test the **page** (`SetSourceTableFilters`), not a direct `Transfer Line` table filter. +- **Use `Database::"Transfer Line"` and transfer direction, not raw source type numbers:** `5741` is table `Transfer + Line`, but raw literals made the reservation guard harder to review across rounds. This is one of the rare readability findings + worth carrying in the playbook because source-type mistakes change reservation behavior. +- **Transfer application must filter by selected tracking:** when a transfer receipt applies open item ledger entries, + the selected serial/lot and package values on the posted transfer entry must be marked as required before the open-entry search + applies tracking filters. The package behavior was added through the existing package extension subscriber before the existing + package filter hook ran. Tests verified the unselected first lot/package kept remaining quantity while the selected second + lot/package was consumed. +- **Transfer demand planning extensibility must protect the current profile:** an event before transfer demand + inventory profile creation is valid for split demand (cut-length) scenarios, but if the handled event receives the same + `SupplyInvtProfile` record that later continues through the procedure, a subscriber can leave the current record on the wrong + inserted profile. Save and restore the current profile or pass a copy when `IsHandled` can insert multiple transfer demand + profiles. +- **Project / job warehouse source references must align with direct reservation source references:** direct Job + Planning Line reservations used table 1003 / subtype Order while warehouse picks used `Database::Job` / subtype 0, causing + reservation availability to be double-counted or mis-keyed. The final fix changed warehouse activity, worksheet, request, + item-tracking, and creation paths to use Job Planning Line consistently, kept Show Source Document opening the Job Card, and + made `Create Inventory Pick/Movement.GetSourceLineNo` return `-1` for both `Database::Job` and `Database::"Job Planning Line"` + so multiple reserved planning lines still aggregate. +- **Warehouse Request upgrade code must filter before renaming key fields:** because `Warehouse Request` source fields + are key fields, migration used rename/delete-insert style logic. A round regressed by removing `Source Type = Database::Job` and + `Source Subtype = 0` filters before `FindSet`, which could rename Sales, Purchase, Transfer, or other requests to Job Planning + Line. Always add a non-job control row to upgrade tests. +- **Requisition-line base quantity rounding can orphan reservations:** when planning project demand to a + purchase order with a non-base purchase UoM, the purchase quantity rounds and recalculates base quantity. If `Inventory Profile + Offsetting` copies a slightly different `Quantity (Base)` than `SupplyInventoryProfile."Remaining Quantity (Base)"`, later + deletion leaves reservation entries and blocks deleting the project planning line. The fix used a UoM-scaled tolerance (`Qty. + per Unit of Measure`, not one fixed precision step) and then aligned base fields with the demand. The deterministic test used + Qty. per UoM = 12 and asserted no reservation entries remained. + +--- + +## 6. Item tracking / reservation interplay + +Symptoms: lots look available while already on picks, purchase-order creation corrupts table 337, Description lookup skips +auto-reservation, or drop shipment tracking is deleted as an illegal field change. + +- **Unregistered picks consume tracked availability:** `Item Tracking Data Collection` must add temporary demand for + outstanding warehouse pick and inventory pick lines with matching item, variant, location, serial/lot/package, positive + outstanding quantity, and a different source. Round 5 caught that the code comment claimed inventory picks were covered but the + filter still only had `Activity Type = Pick`; include `Invt. Pick` too. Later rounds added source- reservation netting so split + Take lines do not subtract the same source-line reservation more than once. This is the key pattern for "available lot" bugs: + group by source + tracking, skip the current source, subtract matching source reservation once, then insert only the remaining + temporary demand. +- **Nonspecific reservation reshuffle must preserve tracking truth:** for FEFO warehouse picks, deleting surplus + reservations and keeping picked lots must account for split reservation entries on the same lot. Use remaining qty per lot; + never recompute the full picked qty independently for each reservation row. +- **Copied tracking from planning should usually be `Prospect`, not `Reservation`:** `CopyItemTracking3` with + `Reservation Status::Reservation` created only one side of a pair, then later code looked for the missing counterpart and raised + `Reservation Entry does not exist`, leaving table 337 corrupted after Order Planning / Create Purchase Orders. Reverting to + Prospect status fixed the data model. If removing a public overload that accepted a status parameter, obsolete it first and + document that the status is ignored until the clean tag. +- **Do not delete the old serial-tracking test without replacing the scenario:** the removed test had encoded + the broken Reservation status, but it still represented the original bug scenario. Replace it with an end-to-end Order Planning test that + creates/cancels purchase orders for a lot/serial-tracked item and proves reservation entries remain valid and the flow can + re-run. +- **Sales Description lookup must capture `No.` changes before `SaveRecord`:** for `Reserve = Always` items selected + through Description lookup, `CurrPage.SaveRecord` resynced `xRec`, so `Rec."No." <> xRec."No."` became false and + `AutoReserve` was skipped. Capture `NoHasChanged` before save, return a `SelectionRestored` flag from + `RestoreLookupSelectionWithResult`, and use both in the auto-reserve guard. The accepted dispute: setting `CurrFieldNo` to + `FieldNo("No.")` was safe because `CheckWarehouse` is skipped on the restore path and item availability / credit checks are + `Type = Item` guarded. +- **Single-instance lookup state must be cleared before the forced-error point:** the negative test became meaningful + only when it asserted `Lookup State Manager.IsRecordSaved` true before the error and false after. The state is in-memory and + non-transactional, so `asserterror` rollback does not clear it; production must clear it in `RestoreLookupSelectionWithResult` + before `OnBeforeNoOnAfterValidate` fires. +- **Drop-shipment purchase creation with lot tracking must preserve both sales link and lot reservation:** Req. + Line-Reserve was treating valid drop-shipment fields (`Sales Order No.`, `Sales Order Line No.`, `Sell-to Customer No.`) as + illegal reservation changes and deleting tracking before PO creation. The fix is narrow, but the test must assert more than + "purchase line exists": set a location, verify the drop-shipment sales link survives, and verify expected lot + reservation/tracking survives. +- **Project planning line deletion bugs are reservation bugs too:** if a bug ends as "cannot delete source + line," inspect whether a planning or document conversion step created reservation entries with base qty not matching the source + demand. The correct assertion is often "no reservation entries remain" after deleting the downstream order and source line. + +--- + +## 7. Availability-specific traps + +Symptoms: an availability page hides future demand, availability overstates stock in staging bins, or warehouse availability +changes after source-key migration. + +- **Clear date filters when opening Item Availability by Event from production lines/components:** the Event view from + production order lines was capped at `0D..Due Date`, hiding later demand that Period view and Item Card showed. Clear + `Item."Date Filter"` before `ShowItemAvailabilityByEvent`, matching the Period path and the requisition-line Event path. If + changing both line and component actions, cover both; the original review accepted with a request to test the component branch + too. +- **Pick worksheet availability and Create Pick availability must agree:** if one path excludes Ship/Receive + bins and the other path counts them, users get either false pickability or "nothing to handle." When fixing one calculation, + trace the sibling calculation (`Create Pick`, `Warehouse Availability Mgt.`, worksheet line calc) for the same bin-type rule. +- **Source-line aggregation can be semantically meaningful:** changing job warehouse source type from Job to Job + Planning Line was correct for reservations, but availability calculation would have changed if `GetSourceLineNo` started + filtering one planning line at a time. Returning `-1` kept the old aggregate behavior for multiple reserved job planning lines. +- **Transfer inbound availability is not reservation permission:** a line can appear in a transfer availability page + and still be non-reservable from the inbound side until receipt. Keep page filtering, validation, and error messages aligned, + and do not let a table-filter-only test stand in for the page behavior. + +--- + +## 8. Recurring agentic-review findings — fix these *before* submitting changes + +These are the warehouse/tracking-specific issues reviewers repeatedly caught. Pre-empting them saves rounds. + +- **A test helper must not raise the production error itself.** An earlier fix initially had `AutoReserveTransferLine` throw + `InboundReservationErr` in the test helper, so the test passed without executing the `Reservation` page / Reservation Management + guard. Let the page/codeunit raise the error. +- **Tests must drive the UI/page object when the bug is in page filters.** An earlier fix kept a test that filtered `Transfer Line` + directly while the production behavior was `AvailableTransferLines.SetSourceTableFilters`. +- **Line-order exits are dangerous in mixed warehouse activity documents.** An earlier fix needed a per-line ATO skip; a first-line ATO + exit would skip validation for normal tracked lines if the ATO line sorted first. +- **Quantity allocation across multiple bins must sum to the requested quantity.** An earlier fix first risked giving the full + failed/specific quantity to each eligible bin. Allocate remaining quantity per bin and fail on shortfall. +- **FEFO-specific fixes need FEFO guards.** An earlier fix originally applied a destination-bin exclusion to any blank-`From Bin` + inventory movement; the final fix gated it on `Pick According to FEFO`. +- **When changing source-key models, include upgrade controls for unrelated rows.** An earlier fix regressed by renaming unfiltered + `Warehouse Request` rows; the final test seeded a non-job Sales Header request and verified it stayed unchanged. +- **When changing reservation quantities, test split entries for the same lot.** An earlier fix only became safe after testing a lot + split across two item ledger / reservation entries. +- **When fixing availability around picks, include both `Pick` and `Invt. Pick`.** An earlier fix missed inventory picks even though the + comment claimed blank action type covered them. +- **When adding handled events around mutable records, protect the current record.** An earlier fix exposed `SupplyInvtProfile` by var + before continuing the standard planning flow; a handled subscriber that inserts several profiles can leave the caller on the + wrong record unless you save/restore or pass a copy. +- **When applying a workflow bin, mirror both directions.** An earlier fix's transfer-to bin needed the same location guard shape as + the existing transfer-from bin. +- **When clearing invalid bin setup in a page, persist the clear.** An earlier fix's important behavior was clearing the stored + workflow argument, not just hiding or blanking the on-screen variable. +- **When fixing report tracking appendices, verify cross-document scope manually.** An earlier fix could not reasonably add an RDLC + rendering test, so the right evidence was a batch print with at least two tracked shipments. +- **When copy-item-tracking status changes, preserve compatibility and prove both old and new bugs.** An earlier fix needed an + obsolete wrapper for the removed public overload and a replacement test for the older serial-tracking scenario. +- **When using location helper setup in tests, assert the warehouse fact, not just that a document exists.** Examples: pick Take + line `Bin Code` is the pick bin, transfer line `Transfer-To Bin Code` is set/cleared correctly, + lot/package remaining quantity changes on the selected value only, and no reservation entries remain after deletion. + +--- diff --git a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/workflow.md b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/workflow.md index 341eadf9a..df1754b0e 100644 --- a/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/workflow.md +++ b/src/bcbench/agent/shared/instructions/microsoftInternal-NAV/agents/fix-bug/workflow.md @@ -43,7 +43,24 @@ contract. Read them only; never edit them (Rule 1). Issue images, when the task references them, are under `problem/` at the repository root. Read them if the described symptom is visual. -### Step 2: Write the plan +### Step 2: Load a discovered area playbook + +Skip this step when `AGENT_ROOT/playbooks/selected.yaml` exists. + +Use only source paths confirmed during Step 1. Do not inspect the benchmark dataset, gold patch, +hidden test patch, or benchmark answer files. + +Read `AGENT_ROOT/playbooks/manifest.yaml`. Normalize confirmed paths to repository-relative paths +with `/` separators and compare them case-insensitively with the manifest patterns: + +- Exactly one distinct playbook matches: read that playbook before writing the plan. +- No playbook matches: continue without one. +- Different confirmed paths match different playbooks: report the ambiguity in the plan and read + none of them. + +Read at most one area playbook. Do not browse unrelated playbooks. + +### Step 3: Write the plan Hold the plan in memory - do not write it to a file (Rule 4). It must cover: diff --git a/src/bcbench/evaluate/bugfix.py b/src/bcbench/evaluate/bugfix.py index acc1c9838..b1f0b9c01 100644 --- a/src/bcbench/evaluate/bugfix.py +++ b/src/bcbench/evaluate/bugfix.py @@ -2,7 +2,7 @@ from bcbench.dataset import BugFixEntry from bcbench.evaluate.base import AgentRunner, EvaluationPipeline -from bcbench.exceptions import BuildError, TestExecutionError +from bcbench.exceptions import BuildError, EmptyDiffError, TestExecutionError from bcbench.github_actions import github_log_group from bcbench.logger import get_logger from bcbench.operations import ( @@ -56,7 +56,20 @@ def evaluate(self, context: EvaluationContext[BugFixEntry]) -> None: # Clean test projects to revert any unintended agent changes before capturing diff clean_project_paths(context.repo_path, test_projects) - generated_patch = stage_and_get_diff(context.repo_path) + try: + generated_patch = stage_and_get_diff(context.repo_path) + except EmptyDiffError as error: + logger.warning(f"Agent produced no AL changes for {context.entry.instance_id}") + result = BugFixResult.create_result( + context, + "", + build=False, + resolved=False, + error_message=str(error), + ) + self.save_result(context, result) + return + result: BugFixResult | None = None try: diff --git a/src/bcbench/operations/__init__.py b/src/bcbench/operations/__init__.py index 00a67989a..025b2117a 100644 --- a/src/bcbench/operations/__init__.py +++ b/src/bcbench/operations/__init__.py @@ -24,7 +24,7 @@ init_repo, stage_and_get_diff, ) -from bcbench.operations.instruction_operations import copy_problem_statement_folder, setup_custom_agent, setup_instructions_from_config +from bcbench.operations.instruction_operations import copy_problem_statement_folder, setup_agent_playbooks, setup_custom_agent, setup_instructions_from_config from bcbench.operations.project_operations import categorize_projects from bcbench.operations.setup_operations import bootstrap_app_json, set_runtime_version, setup_repo_prebuild from bcbench.operations.skills_operations import setup_agent_skills @@ -56,6 +56,7 @@ "resolve_artifact_version_root", "run_tests", "set_runtime_version", + "setup_agent_playbooks", "setup_agent_skills", "setup_custom_agent", "setup_instructions_from_config", diff --git a/src/bcbench/operations/instruction_operations.py b/src/bcbench/operations/instruction_operations.py index 774849ba3..5168e0a79 100644 --- a/src/bcbench/operations/instruction_operations.py +++ b/src/bcbench/operations/instruction_operations.py @@ -1,11 +1,15 @@ from pathlib import Path from shutil import copytree, rmtree +from typing import cast + +import yaml from bcbench.config import get_config from bcbench.dataset import BaseDatasetEntry from bcbench.dataset.dataset_entry import RepoGroundedEntry from bcbench.logger import get_logger -from bcbench.types import AgentHarness +from bcbench.playbooks import PlaybookSetup, load_playbook_manifest, playbook_revision, resolve_playbook_for_area +from bcbench.types import AgentHarness, PlaybookMode logger = get_logger(__name__) _config = get_config() @@ -66,6 +70,44 @@ def setup_custom_agent(agent_config: dict, entry: BaseDatasetEntry, repo_path: P return None +def setup_agent_playbooks( + agent_config: dict, + entry: BaseDatasetEntry, + repo_path: Path, + harness: AgentHarness, + custom_agent: str | None, +) -> PlaybookSetup: + playbook_config: dict = agent_config.get("playbooks", {}) + if not playbook_config.get("enabled", False): + return PlaybookSetup() + + if custom_agent is None: + raise ValueError("playbooks require a custom agent") + + mode = playbook_config.get("mode") + if mode not in ("discover", "selected"): + raise ValueError(f"Invalid playbook mode: {mode!r}") + + playbook_dir = harness.get_target_dir(repo_path) / "agents" / custom_agent / "playbooks" + manifest = load_playbook_manifest(playbook_dir) + marker = playbook_dir / "selected.yaml" + marker.unlink(missing_ok=True) + + selected = resolve_playbook_for_area(manifest, entry.metadata.area) if mode == "selected" else None + if selected is not None: + marker.write_text( + yaml.safe_dump({"id": selected.id, "file": selected.file}, sort_keys=False), + encoding="utf-8", + ) + + return PlaybookSetup( + enabled=True, + mode=cast(PlaybookMode, mode), + revision=playbook_revision(playbook_dir, manifest), + playbook_id=selected.id if selected else None, + ) + + def _get_source_instructions_path(profile: str) -> Path: """ Get path to the source instruction folder for an instruction profile. diff --git a/src/bcbench/playbooks.py b/src/bcbench/playbooks.py new file mode 100644 index 000000000..f96ad994c --- /dev/null +++ b/src/bcbench/playbooks.py @@ -0,0 +1,111 @@ +from __future__ import annotations + +import hashlib +from pathlib import Path, PurePosixPath +from typing import Annotated + +import yaml +from pydantic import BaseModel, ConfigDict, Field, model_validator + +from bcbench.types import PlaybookMode + +__all__ = [ + "PlaybookDefinition", + "PlaybookManifest", + "PlaybookSetup", + "load_playbook_manifest", + "playbook_revision", + "resolve_playbook_for_area", + "resolve_playbook_for_paths", +] + + +class PlaybookDefinition(BaseModel): + model_config = ConfigDict(frozen=True, extra="forbid") + + id: Annotated[str, Field(pattern=r"^[a-z][a-z0-9-]*$")] + file: Annotated[str, Field(pattern=r"^[a-z][a-z0-9-]*\.md$")] + areas: list[Annotated[str, Field(min_length=1)]] = Field(default_factory=list) + paths: Annotated[list[str], Field(min_length=1)] + + +class PlaybookManifest(BaseModel): + model_config = ConfigDict(frozen=True, extra="forbid") + + playbooks: Annotated[list[PlaybookDefinition], Field(min_length=1)] + + @model_validator(mode="after") + def validate_uniqueness(self) -> PlaybookManifest: + ids = [playbook.id for playbook in self.playbooks] + if len(ids) != len(set(ids)): + raise ValueError("duplicate playbook id") + + aliases = [area.casefold() for playbook in self.playbooks for area in playbook.areas] + if len(aliases) != len(set(aliases)): + raise ValueError("duplicate area alias") + + roots = [(playbook.id, _path_root(pattern)) for playbook in self.playbooks for pattern in playbook.paths] + for index, (left_id, left_root) in enumerate(roots): + for right_id, right_root in roots[index + 1 :]: + if left_id != right_id and (left_root == right_root or left_root.startswith(f"{right_root}/") or right_root.startswith(f"{left_root}/")): + raise ValueError(f"overlapping playbook paths: {left_id} and {right_id}") + return self + + +class PlaybookSetup(BaseModel): + model_config = ConfigDict(frozen=True) + + enabled: bool = False + mode: PlaybookMode | None = None + revision: str | None = None + playbook_id: str | None = None + + +def _normalize_path(value: str) -> str: + return value.replace("\\", "/").strip("/").casefold() + + +def _path_root(pattern: str) -> str: + normalized = _normalize_path(pattern) + if not normalized.endswith("/**"): + raise ValueError(f"playbook path must end with '/**': {pattern}") + return normalized.removesuffix("/**") + + +def load_playbook_manifest(playbook_dir: Path) -> PlaybookManifest: + payload = yaml.safe_load((playbook_dir / "manifest.yaml").read_text(encoding="utf-8")) + manifest = PlaybookManifest.model_validate(payload) + for playbook in manifest.playbooks: + if not (playbook_dir / playbook.file).is_file(): + raise ValueError(f"missing playbook file: {playbook.file}") + return manifest + + +def resolve_playbook_for_area(manifest: PlaybookManifest, area: str | None) -> PlaybookDefinition | None: + if not area: + return None + + normalized = area.casefold() + return next( + (playbook for playbook in manifest.playbooks if normalized in {alias.casefold() for alias in playbook.areas}), + None, + ) + + +def resolve_playbook_for_paths(manifest: PlaybookManifest, paths: list[str]) -> PlaybookDefinition | None: + matches = { + playbook.id: playbook for path in paths for playbook in manifest.playbooks if any(PurePosixPath(_normalize_path(path)).full_match(_normalize_path(pattern)) for pattern in playbook.paths) + } + return next(iter(matches.values())) if len(matches) == 1 else None + + +def playbook_revision(playbook_dir: Path, manifest: PlaybookManifest) -> str: + digest = hashlib.sha256() + paths = [ + playbook_dir / "manifest.yaml", + *(playbook_dir / playbook.file for playbook in sorted(manifest.playbooks, key=lambda item: item.id)), + ] + for path in paths: + digest.update(path.name.encode()) + digest.update(path.read_bytes()) + return digest.hexdigest()[:12] diff --git a/src/bcbench/results/display.py b/src/bcbench/results/display.py index 97499e587..b51df307a 100644 --- a/src/bcbench/results/display.py +++ b/src/bcbench/results/display.py @@ -31,6 +31,9 @@ def create_console_summary(results: Sequence[BaseEvaluationResult], summary: Eva console.print(f"Custom Instructions: [bold]{'Yes' if results[0].experiment and results[0].experiment.custom_instructions else 'No'}[/bold]") console.print(f"Skills: [bold]{'Yes' if results[0].experiment and results[0].experiment.skills_enabled else 'No'}[/bold]") console.print(f"Custom Agent: [bold]{results[0].experiment.custom_agent if results[0].experiment and results[0].experiment.custom_agent else 'N/A'}[/bold]") + console.print(f"Playbooks: [bold]{'Yes' if results[0].experiment and results[0].experiment.playbooks_enabled else 'No'}[/bold]") + console.print(f"Playbook Mode: [bold]{results[0].experiment.playbook_mode if results[0].experiment and results[0].experiment.playbook_mode else 'N/A'}[/bold]") + console.print(f"Playbook Revision: [bold]{results[0].experiment.playbook_revision if results[0].experiment and results[0].experiment.playbook_revision else 'N/A'}[/bold]") console.print(f"Plugins: [bold]{', '.join(results[0].experiment.plugins) if results[0].experiment and results[0].experiment.plugins else 'None'}[/bold]") metrics = summary.render_console_metrics() @@ -99,6 +102,9 @@ def create_github_job_summary(results: Sequence[BaseEvaluationResult], summary: f"- Custom Instructions: {'Yes' if results[0].experiment and results[0].experiment.custom_instructions else 'No'}", f"- Skills: {'Yes' if results[0].experiment and results[0].experiment.skills_enabled else 'No'}", f"- Custom Agent: {results[0].experiment.custom_agent if results[0].experiment and results[0].experiment.custom_agent else 'N/A'}", + f"- Playbooks: {'Yes' if results[0].experiment and results[0].experiment.playbooks_enabled else 'No'}", + f"- Playbook Mode: {results[0].experiment.playbook_mode if results[0].experiment and results[0].experiment.playbook_mode else 'N/A'}", + f"- Playbook Revision: {results[0].experiment.playbook_revision if results[0].experiment and results[0].experiment.playbook_revision else 'N/A'}", f"- Plugins: {', '.join(results[0].experiment.plugins) if results[0].experiment and results[0].experiment.plugins else 'None'}", ] ) diff --git a/src/bcbench/results/summary.py b/src/bcbench/results/summary.py index 3c5d31e27..82e0a7033 100644 --- a/src/bcbench/results/summary.py +++ b/src/bcbench/results/summary.py @@ -83,7 +83,7 @@ def _base_fields(cls, results: Sequence[BaseEvaluationResult], run_id: str) -> d tool_usages: list[dict[str, int]] = [r.metrics.tool_usage for r in results if r.metrics and r.metrics.tool_usage is not None] first_result = results[0] - experiment = first_result.experiment if first_result.experiment and not first_result.experiment.is_empty() else None + experiment = first_result.experiment.for_aggregate() if first_result.experiment and not first_result.experiment.is_empty() else None return { "total": len(results), diff --git a/src/bcbench/types.py b/src/bcbench/types.py index 1b71678e6..621261f92 100644 --- a/src/bcbench/types.py +++ b/src/bcbench/types.py @@ -30,12 +30,14 @@ "ExpectedOutput", "ExperimentConfiguration", "JudgeCalibrationReport", + "PlaybookMode", "PluginConfig", "RepoSlug", ] type ChecklistLevel = Literal["critical", "expected", "aspirational"] +type PlaybookMode = Literal["discover", "selected"] class ChecklistAssertion(TypedDict): @@ -110,6 +112,12 @@ class ExperimentConfiguration(BaseModel): # Custom agent name used in experiment (if any) custom_agent: str | None = None + # Area-specific bug-fix playbook configuration + playbooks_enabled: bool = False + playbook_mode: PlaybookMode | None = None + playbook_revision: str | None = None + playbook_id: str | None = None + # Plugins loaded for this experiment: "@" (github) or "@local" plugins: list[str] | None = None @@ -119,7 +127,21 @@ def is_empty(self) -> bool: An empty configuration means no special experiment settings were used. This is useful for comparing with None (no experiment) vs default experiment. """ - return self.mcp_servers is None and self.al_lsp_enabled is False and self.custom_instructions is False and self.skills_enabled is False and self.custom_agent is None and self.plugins is None + return ( + self.mcp_servers is None + and self.al_lsp_enabled is False + and self.custom_instructions is False + and self.skills_enabled is False + and self.custom_agent is None + and self.playbooks_enabled is False + and self.playbook_mode is None + and self.playbook_revision is None + and self.playbook_id is None + and self.plugins is None + ) + + def for_aggregate(self) -> ExperimentConfiguration: + return self.model_copy(update={"playbook_id": None}) # Where an agent plugin comes from: local, or cloned from GitHub diff --git a/tests/test_bugfix_pipeline.py b/tests/test_bugfix_pipeline.py new file mode 100644 index 000000000..77dd4708c --- /dev/null +++ b/tests/test_bugfix_pipeline.py @@ -0,0 +1,20 @@ +from bcbench.config import get_config +from bcbench.evaluate.bugfix import BugFixPipeline +from bcbench.exceptions import EmptyDiffError +from bcbench.results.bugfix import BugFixResult +from tests.conftest import create_evaluation_context + + +def test_empty_diff_is_persisted_as_failed_result(tmp_path, monkeypatch): + context = create_evaluation_context(tmp_path) + monkeypatch.setattr("bcbench.evaluate.bugfix.clean_project_paths", lambda *_args: None) + monkeypatch.setattr("bcbench.evaluate.bugfix.stage_and_get_diff", lambda _repo_path: (_ for _ in ()).throw(EmptyDiffError())) + + BugFixPipeline().evaluate(context) + + result_file = context.result_dir / f"{context.entry.instance_id}{get_config().file_patterns.result_pattern}" + result = BugFixResult.model_validate_json(result_file.read_text(encoding="utf-8")) + assert result.output == "" + assert result.resolved is False + assert result.build is False + assert result.error_message == "Generated diff is empty. Agent did not make any changes." diff --git a/tests/test_claude_code_agent.py b/tests/test_claude_code_agent.py index cb1095f16..1958cae92 100644 --- a/tests/test_claude_code_agent.py +++ b/tests/test_claude_code_agent.py @@ -4,6 +4,7 @@ from unittest.mock import patch from bcbench.agent.claude.agent import run_claude_code +from bcbench.playbooks import PlaybookSetup from bcbench.types import EvaluationCategory from tests.conftest import create_dataset_entry @@ -28,6 +29,7 @@ def test_claude_code_excludes_user_settings_and_auto_memory(tmp_path: Path, monk ), patch("bcbench.agent.claude.agent.setup_agent_skills", return_value=False), patch("bcbench.agent.claude.agent.setup_custom_agent", return_value=None), + patch("bcbench.agent.claude.agent.setup_agent_playbooks", return_value=PlaybookSetup()), patch("bcbench.agent.claude.agent.resolve_config_plugins", return_value=[]), patch( "bcbench.agent.claude.agent.subprocess.run", diff --git a/tests/test_copilot_agent.py b/tests/test_copilot_agent.py index 55b00ffaa..76e99ae09 100644 --- a/tests/test_copilot_agent.py +++ b/tests/test_copilot_agent.py @@ -4,6 +4,7 @@ from bcbench.agent.copilot.agent import run_copilot_agent from bcbench.agent.copilot.cli import invoke_copilot +from bcbench.playbooks import PlaybookSetup from bcbench.types import EvaluationCategory from tests.conftest import create_dataset_entry @@ -82,6 +83,7 @@ def test_copilot_does_not_enable_hooks_memory_or_unrestricted_urls(tmp_path: Pat patch("bcbench.agent.copilot.agent.setup_instructions_from_config", return_value=False), patch("bcbench.agent.copilot.agent.setup_agent_skills", return_value=False), patch("bcbench.agent.copilot.agent.setup_custom_agent", return_value=None), + patch("bcbench.agent.copilot.agent.setup_agent_playbooks", return_value=PlaybookSetup()), patch("bcbench.agent.copilot.agent.resolve_config_plugins", return_value=[]), patch("bcbench.agent.copilot.cli.parse_output", return_value=(None, None)) as mock_parse_output, patch( diff --git a/tests/test_playbooks.py b/tests/test_playbooks.py new file mode 100644 index 000000000..8f605ea5c --- /dev/null +++ b/tests/test_playbooks.py @@ -0,0 +1,242 @@ +from pathlib import Path +from shutil import copytree +from unittest.mock import MagicMock + +import pytest +import yaml + +from bcbench.operations.instruction_operations import setup_agent_playbooks +from bcbench.playbooks import ( + PlaybookManifest, + PlaybookSetup, + load_playbook_manifest, + playbook_revision, + resolve_playbook_for_area, + resolve_playbook_for_paths, +) +from bcbench.types import AgentHarness + +MANIFEST = """\ +playbooks: + - id: warehouse + file: warehouse.md + areas: [warehouse] + paths: + - App/Layers/W1/BaseApp/Warehouse/** + - id: project + file: project.md + areas: [project] + paths: + - App/Layers/W1/BaseApp/Projects/** +""" + + +def write_package(tmp_path: Path, manifest: str = MANIFEST) -> Path: + playbook_dir = tmp_path / "playbooks" + playbook_dir.mkdir() + (playbook_dir / "manifest.yaml").write_text(manifest, encoding="utf-8") + (playbook_dir / "warehouse.md").write_text("# Warehouse\n", encoding="utf-8") + (playbook_dir / "project.md").write_text("# Project\n", encoding="utf-8") + return playbook_dir + + +def test_loads_valid_manifest(tmp_path: Path): + manifest = load_playbook_manifest(write_package(tmp_path)) + + assert isinstance(manifest, PlaybookManifest) + assert [playbook.id for playbook in manifest.playbooks] == ["warehouse", "project"] + + +@pytest.mark.parametrize( + ("area", "expected"), + [ + ("warehouse", "warehouse"), + ("WAREHOUSE", "warehouse"), + ("project", "project"), + ("sales", None), + (None, None), + ], +) +def test_resolves_area_case_insensitively(tmp_path: Path, area: str | None, expected: str | None): + manifest = load_playbook_manifest(write_package(tmp_path)) + + selected = resolve_playbook_for_area(manifest, area) + + assert (selected.id if selected else None) == expected + + +@pytest.mark.parametrize( + ("paths", "expected"), + [ + (["App/Layers/W1/BaseApp/Warehouse/Activity/Foo.Codeunit.al"], "warehouse"), + (["app/layers/w1/baseapp/projects/project/posting/foo.codeunit.al"], "project"), + (["App/Layers/W1/BaseApp/Sales/Foo.Codeunit.al"], None), + ( + [ + "App/Layers/W1/BaseApp/Warehouse/Activity/Foo.Codeunit.al", + "App/Layers/W1/BaseApp/Projects/Project/Foo.Codeunit.al", + ], + None, + ), + ], +) +def test_resolves_paths_only_for_one_distinct_playbook(tmp_path: Path, paths: list[str], expected: str | None): + manifest = load_playbook_manifest(write_package(tmp_path)) + + selected = resolve_playbook_for_paths(manifest, paths) + + assert (selected.id if selected else None) == expected + + +@pytest.mark.parametrize( + ("manifest", "message"), + [ + (MANIFEST.replace("id: project", "id: warehouse"), "duplicate playbook id"), + (MANIFEST.replace("areas: [project]", "areas: [warehouse]"), "duplicate area alias"), + (MANIFEST.replace("project.md", "missing.md"), "missing playbook file"), + (MANIFEST.replace("App/Layers/W1/BaseApp/Projects/**", "App/Layers/W1/BaseApp/Warehouse/Activity/**"), "overlapping playbook paths"), + ], +) +def test_rejects_invalid_package(tmp_path: Path, manifest: str, message: str): + with pytest.raises(ValueError, match=message): + load_playbook_manifest(write_package(tmp_path, manifest)) + + +def test_rejects_path_without_recursive_suffix(tmp_path: Path): + manifest = MANIFEST.replace("App/Layers/W1/BaseApp/Projects/**", "App/Layers/W1/BaseApp/Projects/*") + + with pytest.raises(ValueError, match="must end with '/\\*\\*'"): + load_playbook_manifest(write_package(tmp_path, manifest)) + + +def test_revision_changes_with_playbook_content(tmp_path: Path): + playbook_dir = write_package(tmp_path) + manifest = load_playbook_manifest(playbook_dir) + original = playbook_revision(playbook_dir, manifest) + + (playbook_dir / "warehouse.md").write_text("# Warehouse changed\n", encoding="utf-8") + + assert playbook_revision(playbook_dir, manifest) != original + + +def install_package(tmp_path: Path, harness: AgentHarness) -> Path: + source_root = tmp_path / "source" + source_root.mkdir() + source = write_package(source_root) + target = harness.get_target_dir(tmp_path) / "agents" / "fix-bug" / "playbooks" + copytree(source, target) + return target + + +def entry_with_area(area: str | None) -> MagicMock: + entry = MagicMock() + entry.metadata.area = area + return entry + + +@pytest.mark.parametrize( + ("harness", "target_dir"), + [ + (AgentHarness.COPILOT, ".github"), + (AgentHarness.CLAUDE, ".claude"), + ], +) +def test_selected_mode_writes_harness_specific_marker(tmp_path: Path, harness: AgentHarness, target_dir: str): + install_package(tmp_path, harness) + + setup = setup_agent_playbooks( + {"playbooks": {"enabled": True, "mode": "selected"}}, + entry_with_area("warehouse"), + tmp_path, + harness=harness, + custom_agent="fix-bug", + ) + + marker = tmp_path / target_dir / "agents" / "fix-bug" / "playbooks" / "selected.yaml" + assert setup == PlaybookSetup( + enabled=True, + mode="selected", + revision=setup.revision, + playbook_id="warehouse", + ) + assert setup.revision + assert yaml.safe_load(marker.read_text(encoding="utf-8")) == {"id": "warehouse", "file": "warehouse.md"} + + +def test_discover_mode_validates_package_without_marker(tmp_path: Path): + playbook_dir = install_package(tmp_path, AgentHarness.COPILOT) + + setup = setup_agent_playbooks( + {"playbooks": {"enabled": True, "mode": "discover"}}, + entry_with_area("warehouse"), + tmp_path, + harness=AgentHarness.COPILOT, + custom_agent="fix-bug", + ) + + assert setup.enabled is True + assert setup.mode == "discover" + assert setup.playbook_id is None + assert not (playbook_dir / "selected.yaml").exists() + + +def test_disabled_playbooks_do_not_require_custom_agent(tmp_path: Path): + setup = setup_agent_playbooks( + {"playbooks": {"enabled": False, "mode": "discover"}}, + entry_with_area("warehouse"), + tmp_path, + harness=AgentHarness.COPILOT, + custom_agent=None, + ) + + assert setup == PlaybookSetup() + + +def test_selected_mode_with_unmapped_area_removes_stale_marker(tmp_path: Path): + playbook_dir = install_package(tmp_path, AgentHarness.COPILOT) + marker = playbook_dir / "selected.yaml" + marker.write_text("id: stale\nfile: stale.md\n", encoding="utf-8") + + setup = setup_agent_playbooks( + {"playbooks": {"enabled": True, "mode": "selected"}}, + entry_with_area("sales"), + tmp_path, + harness=AgentHarness.COPILOT, + custom_agent="fix-bug", + ) + + assert setup.playbook_id is None + assert not marker.exists() + + +def test_enabled_playbooks_require_custom_agent(tmp_path: Path): + with pytest.raises(ValueError, match="playbooks require a custom agent"): + setup_agent_playbooks( + {"playbooks": {"enabled": True, "mode": "discover"}}, + entry_with_area("warehouse"), + tmp_path, + harness=AgentHarness.COPILOT, + custom_agent=None, + ) + + +def test_invalid_playbook_mode_fails(tmp_path: Path): + with pytest.raises(ValueError, match="Invalid playbook mode"): + setup_agent_playbooks( + {"playbooks": {"enabled": True, "mode": "automatic"}}, + entry_with_area("warehouse"), + tmp_path, + harness=AgentHarness.COPILOT, + custom_agent="fix-bug", + ) + + +@pytest.mark.parametrize("profile", ["microsoft-BCApps", "microsoftInternal-NAV"]) +def test_fix_bug_agent_forbids_delegation_and_background_work(profile: str): + agent_file = Path("src/bcbench/agent/shared/instructions") / profile / "agents" / "fix-bug.agent.md" + content = agent_file.read_text(encoding="utf-8") + + assert "Do not use the Agent tool" in content + assert "Do not delegate" in content + assert "Do not start background work" in content + assert "Execute the workflow directly in this agent" in content