Skip to content

Commit e57e82c

Browse files
johanzanderclaude
andauthored
docs: require plan-faithfulness test for DP/control-mapping fixes (#386)
PR #385 extended the shadow-price discharge gate to LOAD_SUPPORT with only a synthetic-input unit test on _apply_period_schedule -- no existing or new test actually exercises the branch via a real DP-produced schedule (docs/agents/simulator.md's R == P requirement). Wire that requirement into implement-issue's TDD step so it's the primary RED test for DP/intent/control-mapping fixes, not an optional supplement, with a Step 6 quality-gate backstop in case it's missed. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent b8ca5b7 commit e57e82c

1 file changed

Lines changed: 50 additions & 2 deletions

File tree

.claude/skills/implement-issue/SKILL.md

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,40 @@ the bug (from the diagnosis's evidence — the specific period/scenario/input)
7575
and watch it fail, then write the minimal fix. No refactors outside the bug
7676
— match `docs/agents/patterns.md`.
7777

78+
**Required test shape — checked against the diff, not optional:**
79+
80+
If the fix touches the DP (`dp_battery_algorithm.py`), intent classification
81+
(`decision_intelligence.py`), or control/rate mapping (`inverter_controller.py`
82+
/ `battery_system_manager.py`), the PRIMARY RED test — not an extra test
83+
alongside it, the one that proves the bug — is a plan-faithfulness scenario,
84+
not a unit test calling the changed function with hand-built arguments. Write
85+
it as:
86+
87+
```python
88+
from core.bess.tests.helpers import run_scenario_realized
89+
# scenario is a full DP-optimized schedule, not a hand-built period/decision
90+
result, realized_cost = run_scenario_realized(scenario)
91+
assert realized_cost == pytest.approx(result.total_cost, ...) # R == P
92+
```
93+
94+
A unit test on the changed function directly (e.g. calling
95+
`_apply_period_schedule` or `intra_period_discharge_gate` with stubbed
96+
arguments) can pass while the new branch is unreachable by any real
97+
DP-produced schedule — that is exactly the coverage gap that shipped
98+
undetected in PR #385 (`docs/agents/simulator.md`). Add such a unit test only
99+
as a supplement, never as the sole RED test, for this category of fix.
100+
101+
If the diagnosis's evidence is a user-supplied debug log/bundle, build the
102+
scenario from that real data instead of a hand-assembled fixture:
103+
104+
```bash
105+
python scripts/mock_ha/scenarios/from_debug_log.py <bundle.md>
106+
```
107+
108+
(`docs/agents/testing.md` → Bug Reproduction with Mock HA). Do this before
109+
writing the RED test — the bundle already contains the exact conditions that
110+
reproduced the bug.
111+
78112
### 6. Quality gate + code review (background)
79113

80114
Every PR must pass both the fast and slow suites, plus code review. This is
@@ -87,8 +121,16 @@ self-contained prompt covering:
87121
1. `./scripts/quality-check.sh` (fast suite) — if it fails, fix and re-run,
88122
do not proceed with failures.
89123
2. `.venv/bin/pytest -m slow` (slow suite) — same failure handling.
90-
3. Invoke the `code-review` skill on the diff.
91-
4. Report back: pass/fail on both suites, and any CONFIRMED code-review
124+
3. If the diff touches the DP, intent classification, or control/rate
125+
mapping (the Step 5 table): confirm the diff's new/changed tests include
126+
a `run_scenario_realized` / `verify_plan_faithfulness` call, not only a
127+
unit test on the changed function with hand-built arguments. If missing,
128+
this is a required-before-continuing gap, not a nice-to-have — report it
129+
as a blocking finding alongside the suite results, same severity as a
130+
failing test.
131+
4. Invoke the `code-review` skill on the diff.
132+
5. Report back: pass/fail on both suites, whether check 3 passed, and any
133+
CONFIRMED code-review
92134
findings verbatim (everything else goes to `TODO.md`).
93135

94136
Do not poll — you'll be notified on completion. This is a hard session
@@ -219,6 +261,8 @@ flow above, which stops at draft-PR-open per the Step 10 constraints.
219261
| "the user is in a hurry, just open the PR" | Time pressure from the user is not permission to skip Step 8 — it's the reason to say so explicitly and give a real ETA instead. |
220262
| "I'll just watch the background agent run" | Defeats the point — the whole reason it's backgrounded is so the session isn't held open through the slow suite. Let the notification bring you back. |
221263
| "the fix is small, docs don't need touching" | Small fixes are exactly what silently invalidates a one-line doc claim (a removed threshold, a renamed formula). Grep the two design docs before opening the PR, every time. |
264+
| "a unit test on the changed function is enough" | Not for DP/intent/control-mapping changes — a synthetic-input unit test can pass while the new branch is unreachable by any real optimizer-derived scenario. `docs/agents/simulator.md` requires `R == P` for exactly this class of change. |
265+
| "the existing suite still passes, so nothing broke" | Passing unchanged means the new code path may simply be untested, not unbroken — check whether any existing fixture actually reaches the new branch before treating a green suite as coverage. |
222266

223267
## Red Flags — Stop and Go Back
224268

@@ -233,6 +277,10 @@ flow above, which stops at draft-PR-open per the Step 10 constraints.
233277
dispatching the Step 6 background agent.
234278
- About to open the PR without checking whether the fix invalidates a claim
235279
in `docs/agents/bess-knowledge.md` or `docs/SOFTWARE_DESIGN.md`.
280+
- About to write only a synthetic-input unit test for a DP/intent/control-
281+
mapping change instead of a plan-faithfulness (`R == P`) scenario test.
282+
- About to write a repro test from hand-built data when a user debug log/
283+
bundle is available and `from_debug_log.py` could build it from real data.
236284

237285
## Quick Reference
238286

0 commit comments

Comments
 (0)