diff --git a/.claude/skills/issue-triage/references/debug-journal.md b/.claude/skills/issue-triage/references/debug-journal.md index 60d5978f5..6080c0752 100644 --- a/.claude/skills/issue-triage/references/debug-journal.md +++ b/.claude/skills/issue-triage/references/debug-journal.md @@ -21,11 +21,22 @@ What you get out of it: Committed examples of the same format live in `coverage/cases/*.yaml`; they run as golden regressions under `./run_all --test debug_cases`. +`--debug_file` **without** `--redo` can crash on an older dump with `KeyError: 0` at `prediction.py:769` (GH#4766) — retry with `--redo` before concluding the dump itself is unusable. + Two lines in that output are normal and are not the reporter's bug: - `Prediction kernel stale binary ... - using Python engine` — the local C++ kernel is older than the Python side expects, so the pure-Python engine runs instead. The plan is still correct, just slower. - `Config item ... is below the minimum ... - clamping to ...` — routine clamping of an out-of-range setting. +## Debug yaml can contain real secrets + +A `predbat_debug.yaml` or debug-history snapshot attached to a bug report can carry live, unredacted credentials. Read it locally; never quote a raw block from one in a public comment without checking it first. + +- **`create_debug_yaml()` (`userinterface.py:754`) only masks the `args` key.** Since v8.48.1 (`73eeb330`) a second full copy, `self.args_from_apps_yaml` (`predbat.py:1786`), is stored under its own key, is not in `DEBUG_EXCLUDE_LIST`, and doesn't match `is_secret_key()` by name — so it is written out completely unmasked, with every `!secret` reference already resolved to its real value. **Confirmed still present on `main` (checked 2026-08-27).** GH#4765 is this bug catching a real reporter: their debug yaml carried a plaintext `mcp_secret`, and the triage session had to ask them to rotate it after the fact rather than being able to prevent the leak. +- The masker itself (`mask_secret_args()` / `is_secret_key()`, `utils.py:141-159`) matches `_key`/`password`/`secret`/`token` substrings on top-level keys. Narrower gaps flagged in GH#4757/#4770 (credential-like keys with none of those substrings) and GH#4768 (the MCP `get_apps` tool returning `args` unmasked) have since been closed — confirmed fixed on `main`. +- `debug_history_enable` defaults **True** and captures a snapshot roughly every 3 hours **independently of `switch.predbat_debug_enable`** (`predbat.py:807-820`) — a user who has never turned debug on still has credentials sitting in the rotating `predbat_debug_history.tgz` buffer. +- `annual.py:60`'s `scrub_secrets()` is a properly recursive masker; its docstring's claim to mirror `create_debug_yaml()`'s behaviour is itself wrong (GH#4770) — they mask differently, and `create_debug_yaml()` is the leakier of the two. + ## Check configuration before code "The plan is wrong" has repeatedly turned out to be settings rather than a defect: @@ -42,10 +53,11 @@ Grep for the named symbol rather than trusting a line number. | Area | What past debugging found | Targeted test | |------|---------------------------|---------------| | Fox (`fox.py`) | The cloud API returns errno 42015/44096 for settings a given device does not support (`FOX_SETTINGS_UNSUPPORTED_ERRNO`); those are marked unavailable and never polled or written again. Entity type matters — WorkMode is a select, ExportLimit a number. | `fox_api`, `fox_oauth` | -| Solis (`solis.py`) | `SOLIS_CID_STORAGE_MODE = 636` is Modbus 43110, a bit mask. On firmware "4B and above" (what `is_tou_v2_mode()` detects: CID 6798 reads 43605) the timed charge/discharge enable moved to the per-slot registers — `SOLIS_CID_CHARGE_ENABLE_BASE`/`..._DISCHARGE_ENABLE_BASE`, Modbus 43707, six slots not three — and every mode value carrying TOU bit 1 (3/35/43/51/98) was dropped; 35 became 33, 98 became 96. Such an inverter answers a CID 636 write with code 0 and reads back without bit 1, so a log full of CID 636 verification warnings is a refused bit, not a failed write (GH#4707). `set_storage_mode_if_needed()` decides the bit from `is_tou_v2_mode()`: never asked for on V2, always asked for and retried on V1, where bit 1 still *is* the timed charge/discharge enable. Do not try to learn it from the read-back instead — GH#4710 did, and GH#4774 showed why that cannot work: on six inverters the same write of 179 verifies minutes before and minutes after the one that reads back 177, so a single post-write read is not evidence of a firmware property. Latching it was also silent rather than noisy: the verify read refreshes `cached_values`, so after a stripped write the cache held the stripped value, the suppressed computed value matched it, and `set_storage_mode_if_needed()` stopped writing CID 636 at all for the whole 8-hour verdict — across an overnight charge window. GH#4239's "only retained while a window is configured" is not the whole story — it was refused with slot 1 enabled and its window in force. `read_and_write_cid()` re-reads once after `verify_settle_seconds` before calling a mismatch a failure, because the immediate verify read is taken about half a second after the write. Do not "stop the failed verify poisoning the cache" — `write_cid()` caches the value it *requested* and the post-write `read_cid()` deliberately overwrites it with what the inverter actually reports, so the cache mirrors the inverter rather than Predbat's intent; skipping that update would leave the cache agreeing with the write that just failed, and change detection would then never retry it. The two control paths choose the storage mode differently and it matters: V1 picks from `in_charge_slot`/`in_discharge_slot`, which are clock tests, so CID 636 changes value and gets written the moment a window opens; V2 picks from `slot1_active`, which is only "slot 1 has a window configured", so the mode is written when the slot is *programmed* and nothing at all happens at the window boundary. On the GH#4774 night the last CID 636 write was 22 minutes before the window opened, and the comparison night that worked had one near the window - so `claim_window_mode_assertion()` now asserts the mode once per window on both paths. Slot registers are polled hourly, plus every 5 minutes while `is_inside_active_window()` is true, so drift during the window that matters is caught in the same cycle it happens. GH#4774 also left a useful negative result: that inverter grid-charged from 20% to 91% overnight with bit 1 clear throughout, confirming the per-slot enables alone drive timed charging on V2 — so a "charge window never started" report on V2 firmware is not a CID 636 problem and needs looking at elsewhere. Separately, an inverter reporting `batteryType 'No Battery'` stays in `self.inverter_sn` for polling; `is_battery_inverter()` keeps control writes off it, which was most of the warning volume in GH#4707. That explanation doesn't cover every CID 636 report, though: a later GH#4707 comment showed zero `setting storage mode to` idle-mode log lines, and both V1 mode-decision idle paths always log that line — so a report with neither line cannot be coming from `write_time_windows_if_changed()` at all. The only remaining write path is the HA select handler, `set_storage_mode_value()` at `solis.py:2574`. Check which write path actually ran before assuming this entry's explanation applies. | `solis` | +| Solis (`solis.py`) | `SOLIS_CID_STORAGE_MODE = 636` is Modbus 43110, a bit mask. On firmware "4B and above" (what `is_tou_v2_mode()` detects: CID 6798 reads 43605) the timed charge/discharge enable moved to the per-slot registers — `SOLIS_CID_CHARGE_ENABLE_BASE`/`..._DISCHARGE_ENABLE_BASE`, Modbus 43707, six slots not three — and every mode value carrying TOU bit 1 (3/35/43/51/98) was dropped; 35 became 33, 98 became 96. Such an inverter answers a CID 636 write with code 0 and reads back without bit 1, so a log full of CID 636 verification warnings is a refused bit, not a failed write (GH#4707). `set_storage_mode_if_needed()` decides the bit from `is_tou_v2_mode()`: never asked for on V2, always asked for and retried on V1, where bit 1 still *is* the timed charge/discharge enable. Do not try to learn it from the read-back instead — GH#4710 did, and GH#4774 showed why that cannot work: on six inverters the same write of 179 verifies minutes before and minutes after the one that reads back 177, so a single post-write read is not evidence of a firmware property. Latching it was also silent rather than noisy: the verify read refreshes `cached_values`, so after a stripped write the cache held the stripped value, the suppressed computed value matched it, and `set_storage_mode_if_needed()` stopped writing CID 636 at all for the whole 8-hour verdict — across an overnight charge window. GH#4239's "only retained while a window is configured" is not the whole story — it was refused with slot 1 enabled and its window in force. `read_and_write_cid()` re-reads once after `verify_settle_seconds` before calling a mismatch a failure, because the immediate verify read is taken about half a second after the write. Do not "stop the failed verify poisoning the cache" — `write_cid()` caches the value it *requested* and the post-write `read_cid()` deliberately overwrites it with what the inverter actually reports, so the cache mirrors the inverter rather than Predbat's intent; skipping that update would leave the cache agreeing with the write that just failed, and change detection would then never retry it. The two control paths choose the storage mode differently and it matters: V1 picks from `in_charge_slot`/`in_discharge_slot`, which are clock tests, so CID 636 changes value and gets written the moment a window opens; V2 picks from `slot1_active`, which is only "slot 1 has a window configured", so the mode is written when the slot is *programmed* and nothing at all happens at the window boundary. On the GH#4774 night the last CID 636 write was 22 minutes before the window opened, and the comparison night that worked had one near the window - so `claim_window_mode_assertion()` now asserts the mode once per window on both paths. Slot registers are polled hourly, plus every 5 minutes while `is_inside_active_window()` is true, so drift during the window that matters is caught in the same cycle it happens. GH#4774 also left a useful negative result: that inverter grid-charged from 20% to 91% overnight with bit 1 clear throughout, confirming the per-slot enables alone drive timed charging on V2 — so a "charge window never started" report on V2 firmware is not a CID 636 problem and needs looking at elsewhere. Separately, an inverter reporting `batteryType 'No Battery'` stays in `self.inverter_sn` for polling; `is_battery_inverter()` keeps control writes off it, which was most of the warning volume in GH#4707. That explanation doesn't cover every CID 636 report, though: a later GH#4707 comment showed zero `setting storage mode to` idle-mode log lines, and both V1 mode-decision idle paths always log that line — so a report with neither line cannot be coming from `write_time_windows_if_changed()` at all. The only remaining write path is the HA select handler, `set_storage_mode_value()` at `solis.py:2574`. Check which write path actually ran before assuming this entry's explanation applies. Separately, on inverters using `H M` time format (`GS_fb00`, most cloud subtypes) `adjust_force_export()`'s guard (`inverter.py:2632`/`:2664`) is `new_start != old_start or is_hm_format` — `is_hm_format` alone makes it always true, so `changed_start_end` is set and `press_and_poll_button()` fires every cycle inside an export window regardless of whether anything actually changed (confirmed live on main, GH#4709). The idle-cycle guard for the charge side at `inverter.py:3222` has the same shape, so plain `GS` presses on every idle cycle too via `is_hm_format`, not just `GS_fb00` (GH#4712) — `SX4` presses once then converges, `GE` never presses. `press_and_poll_button()` also presses both buttons on a split-button config, which is why an unrelated register (`timed_charge_current`) gets cleared alongside the intended one. Fixes are open but unmerged as of 2026-08-27 (PR #4711, #4713). | `solis` | | SolaX (`solax.py`) | Code `10402` is a token/auth failure, retried in-request rather than waiting for the next cycle. SolaX clamps battery minimum SOC at 10% (`SOLAX_MIN_RESERVE_PERCENT`), so `battery_min_soc` is auto-configured to stop Predbat writing limits the inverter will reject. | `solax` | | Sigenergy (`sigenergy.py`) | Lifetime/history totals are cumulative server-side and reset around EU midnight, which showed up as an overnight dip; `fetch_history_totals` applies a monotonic clamp. "Energy totals went backwards overnight" starts here. Separately, a user `apps.yaml` override of `inverter.has_reserve_soc: true` (stock `SIG` default is `False`, deliberately — reverted in `5bfc5c80` after #2873/#3124) bypasses two guards that normally make reserve writes inert for Sigenergy (`execute.py:947-950`, `inverter.py:547-549`), so `adjust_reserve()` writes straight to `number.sigen_plant_ess_discharge_cut_off_state_of_charge`. Confirmed from a reporter's log: zero occurrences of the "Inverter does not support reserve" disable message the first guard would normally log, plus 56 reserve writes landing on that register (GH#4728). The stock template's `reserve:` line also points at that same entity rather than `ess_backup_state_of_charge`, which is what makes the override immediately destructive rather than merely inert. | `sigenergy` | -| AlphaESS (`alphaess.py`) | Once any discharge window is configured (`ctrDis=1`), AlphaESS firmware allows discharge *only* inside that window — outside it the battery is charge-from-PV-only, with no self-consumption fallback (AlphaESS's own documented behaviour, per GH#4701). Predbat's generic handling of what happens outside a configured discharge window (`execute.py`) has no AlphaESS-specific case and assumes normal demand mode resumes there, which holds for other brands but not this one — the next best export window Predbat schedules can be hours away, leaving the battery locked out of discharge until then (GH#4723). | `alphaess_control` | +| AlphaESS (`alphaess.py`) | Once any discharge window is configured (`ctrDis=1`), AlphaESS firmware allows discharge *only* inside that window — outside it the battery is charge-from-PV-only, with no self-consumption fallback (AlphaESS's own documented behaviour, per GH#4701). Predbat's generic handling of what happens outside a configured discharge window (`execute.py`) has no AlphaESS-specific case and assumes normal demand mode resumes there, which holds for other brands but not this one — the next best export window Predbat schedules can be hours away, leaving the battery locked out of discharge until then (GH#4723). A separate write-ordering bug, **fixed in PR #4776 (merged 2026-08-27)**: the periodic API commits a schedule in stages — window, then enable switch, then target SoC — pressing the write button after each, so the *first* commit of a cycle always carried the *previous* cycle's target while `alphaess_min_write_interval` (default 300s) held the correcting write back for the rest of that pacing window; a manual charge on an already-live slot could run at the stale target for up to five minutes (GH#4769, confirmed from the reporter's log, not inferred). Also: the legacy `/updateDisChargeConfigInfo` endpoint's `batUseCap` field (the reserve/export-SoC value) has no floor on the Predbat side (`alphaess.py:1103`/`:1143`), while the periodic path clamps it to AlphaESS's documented `[10,100]` range — a reserve/export-SoC entity set below 10 gets every legacy write rejected with an undocumented `10001` (GH#4748); that errno is the signature to recognise this class by. | `alphaess_control` | +| Ohme (`ohme.py`) | A vendored, version-pinned copy of `dan-r/ohmepy` (`ohme.py:33`, `VERSION`). GH#4719 (2026-08-25) found it several minor versions behind upstream with the control routes (max-charge, session rule) pointing at a withdrawn `/v1/chargeSessions/{id}/rule` — a 404 with Spring's `NoResourceFoundException` — while GET/pause/resume/approve kept working since those stayed on v1 upstream. **Fixed on `main` since** (confirmed 2026-08-27): the vendored client now calls the v2 routes upstream moved those endpoints to. A reporter on an older Predbat version seeing control-only failures (GETs/pause/resume fine, target time/percent/preconditioning silently not applying) is hitting this, not a new bug. | `ohme` | | Solcast (`solcast.py`) | `max_kwh` — the array-capacity ceiling used to sanity-check the forecast — is initialised to `9999` (`solcast.py:1339`) and is only ever reassigned on the Forecast.Solar and Open-Meteo branches. On the direct-API and HA-sensor Solcast paths it stays `9999`, so the "Raw forecast exceeds the array ceiling" warning (`solcast.py:1191`) can never fire for Solcast users — confirmed by reading the assignments (GH#4730). That warning is exactly the diagnostic an "impossible PV predicted" report needs, so its absence isn't evidence the ceiling wasn't exceeded. Branch order at `solcast.py:1368` also means a configured `solcast_api_key` always wins over `pv_forecast_*` HA sensors, so comparing Predbat's number against the HA entity compares against the wrong source when both are set. | `solcast` | | HA write/verify (`ha.py`, `inverter.py`) | `get_state(refresh=True)` is a no-op in a normal HA add-on install: it only re-reads when `not self.ha_key`, and `ha_key` is set in that case, so verification reads the websocket cache, never a fresh value (`ha.py:798`, since PR #2342). Separately, `write_and_poll_value()` treats `domain == "sensor"` as Predbat-owned and POSTs `/api/states/` directly (`inverter.py:2155`, `ha.py:1071-1077`) instead of calling the integration. Probed empirically across all four write-path/domain combinations (GH#4738): a control entity that resolves to a `sensor.*` domain by mistake gets a direct state overwrite that reads back as success, with zero corresponding lines in the real integration's log. `call_service_wrapper()`'s return value is also discarded at every `inverter.py` write site, so a rejected HA service call (e.g. a Modbus "Illegal Function" error) never reaches the verify logic. | `inverter` | | GE Cloud (`gecloud.py`) | Gateway fields can come back null. `merge_non_null` stops nulls overwriting good values, and the publish path guards null containers — GH#4656 was a null crash in that area. | `ge_cloud` | @@ -53,13 +65,16 @@ Grep for the named symbol rather than trusting a line number. | Sunsynk / DEYE (`sunsynk.py`, `deye.py`) | Freeze export is gated by the per-slot power register (`sellTime{n}Pac` on Sunsynk), confirmed on live hardware — setting the energy mode alone had no effect, and with slot power at zero the battery still charged. `read_only` was ignored by the reconcile loops until `_is_read_only()` gated `_reconcile_control()` (GH#4436). | `sunsynk_control`, `deye_control` | | Grid sign / arrow direction | `grid_power_invert` is owned by some integrations and not others. With two systems configured, one integration setting it `True` bleeds into the other's entities and inverts the arrows. The fix is an explicit `False` in the automatic config of both. | `sunsynk_config`, `teslemetry` | | Enphase (`enphase.py`) | Unofficial Enlighten endpoints. Accounts with MFA cannot log in at all. Discharge-to-grid schedules are required for export control. Writes need a double-submit CSRF token or return 403. Using the Enphase app at the same time can trip session limits. | `enphase_api` | -| Octopus (`octopus.py`, `fetch.py`) | Intelligent Go tariffs are detected via `is_intelligent_go_tariff()`, and IOG-prefixed tariffs must be skipped when updating intelligent devices. Saving-session auto-join rebinding regressed when `joined_events` was empty (GH#4573). `octopus_slots_signature()` deliberately omits the time-drifting fields of active dispatch slots so a replan is not forced every cycle. | `octopus_*`, `saving_session*` | +| Octopus (`octopus.py`, `fetch.py`) | Intelligent Go tariffs are detected via `is_intelligent_go_tariff()`, and IOG-prefixed tariffs must be skipped when updating intelligent devices. Saving-session auto-join rebinding regressed when `joined_events` was empty (GH#4573). `octopus_slots_signature()` deliberately omits the time-drifting fields of active dispatch slots so a replan is not forced every cycle. `car_charging_threshold` is a strict fallback gated on `not self.car_charging_energy` (`fetch.py:228`, and `load_ml_component.py:348-361`) — it never runs as a second filter alongside a real `car_charging_energy` sensor (GH#4717). Saving-session reporting credits the full saving rate to every minute of the session on both rate tables (`load_saving_slot()`, `octopus.py:2786`); the slot dict has no baseline field to subtract (GH#2090) — a complaint that a saving session's reported total looks inflated starts here, not in a rate-fetch bug. | `octopus_*`, `saving_session*` | | Axle (`axle.py`) | Export sessions have to boost the import rate as well as the export rate. State is published unconditionally from `run()` so a fetch failure does not freeze the sensor at a stale value. | `axle` | | History fetch / memory (`ha.py`) | History is fetched in `HISTORY_CHUNK_DAYS`-sized chunks with boundary dedup — records landing exactly on a chunk start inside a data gap corrupted smoothing before that was fixed. The largest memory peak in a run is ML load-predictor training (`load_predictor.py`), not the plan. | `history_chunking` | | Charge/discharge curve (`inverter.py`) | The curve is evaluated per target minute; tapering near ~93% SOC is expected behaviour, not a fault. | `find_charge_curve`, `battery_curve_keys` | | Standalone / Docker (non-HA) | GH#4601: a callback returning `None` instead of `True` broke the Octopus saving-session fallback in standalone mode. Anything that works under HA but not standalone is worth checking along the `ha.py` websocket and `userinterface.py` callback paths. | `trigger_callback_success_signal` | | Holiday mode (`fetch.py`) | GH#4732: under `days_previous_auto` (the default) holiday mode is handled entirely inside `compute_load_forecast_history()`, not by the `days_previous = [1]` branch, which is only reachable with `days_previous_auto: False`. Days whose holiday state does not match the *forecast day's* are now excluded outright rather than halved, and `get_holiday_minutes()` must span `num_days + 1` to cover the `minutes_now` overhang. A slot with no matching history falls back to `holiday_load_scaling` (default 0.7), which is what makes holiday mode act on day one - a normalised weighted mean cannot otherwise express "all of my data is wrong". | `holiday_mode` | | Predheat (`predheat.py`) | GH#4670: with `predheat_enable` set, Predheat still did not activate after startup because of lazy flag initialisation. There is no registered Predheat test module, so there is nothing to run here — investigate by reading. | none | +| Load ML CPU spikes (`load_ml_component.py`, `load_predictor.py`) | The 2-hourly retrain isn't one pass. `_do_training()` hardcodes `ml_curriculum_step_days = 1` / `ml_curriculum_max_passes = 4` (`load_ml_component.py:94-95` — plain instance attributes, no `config.py` entry, not user-configurable), and `train_curriculum()` caps to the largest N windows (`load_predictor.py:1560-1564`); on ~80 days of history that's roughly five near-full-scale passes back to back, confirmed against a reporter's log as a 16-minute CPU spike every 2 hours (GH#3896). Separately, `threads` only ever reaches the C++ prediction kernel (`plan.py:1497`, `resolve_batch_threads`) — never wired to ML — and there is no `OMP_NUM_THREADS`/`threadpoolctl` anywhere in the repo (confirmed absent, 2026-08-27), so NumPy's BLAS backend is free to fan out across every core on its own during that training window. Whether that actually saturates a host depends on which BLAS backend is linked — worth confirming the platform before assuming a code fix is the right lever. | none | +| Savings & metrics (`output.py`, `predbat.py`) | `savings_total_predbat` accumulates the **unadjusted** `saving` (`predbat.py:1183`, fed from `self.savings_today_predbat = saving` in `output.py`) while `savings_yesterday_predbat` publishes `saving_adjusted` (`output.py:3380`) — the two sensors answer different questions and can disagree in sign on the same day (confirmed from a reporter's dump: `saving_real: +86.14p` vs `saving_adjusted: -25.66p`, GH#3894). Both are still current on `main`. Separately, the battery-value adjustment bills the SoC **level** at day-end against the counterfactual baseline rather than the **change** in SoC over the day — a day that force-exports through midnight can be energy-neutral yet still get charged the full baseline-vs-actual SoC gap as if it were lost value. | none | +| Component auto-config gate (`components.py`) | `components.py:705-708` decides whether to print a `Warn: Skipping interface, missing required configuration` by checking whether *any* of the component's declared args appear anywhere in `args_from_apps_yaml` — not whether that component's own identifying args are set. `gecloud_data` lists `days_previous` among its constructor args, a core scheduling setting that ships uncommented in the default `config/apps.yaml`, so the gate is truthy and the warning fires on installs with no GE Cloud configuration at all (confirmed reproduced and still live on `main`, GH#4786). The `solar` component (`pv_scaling`, `pv_forecast_tomorrow`) has the same latent shape, unreproduced. A "component seems half-configured" report is worth checking against this gate before trusting the warning at face value. | none | ## Symptom → first place to look @@ -93,6 +108,8 @@ Grep for the named symbol rather than trusting a line number. - **Log noise.** `predbat.log` carries routine `Warn:` lines (config clamps, kernel status, unsupported settings). Don't quote a warning as the root cause unless it lines up with the time the reporter describes. - **Hardware questions.** A unit test settles what the code does, not what an inverter did. Several findings here were only confirmed on live hardware. If the question is hardware behaviour, say the maintainer needs to confirm it rather than running a test to look thorough. - **`gh issue list --search` without `--repo` searches all of GitHub.** Confident-looking hits can be from unrelated projects (GH#4705). Always pass `--repo springfall2008/batpred` on a duplicate search. +- **A flat SoC reading isn't necessarily a stuck charge.** `soc_kw` is derived from `soc_percent × soc_max / 100` (`inverter.py:1422-1428`); on a large pack, one percentage point is a meaningful chunk of energy — e.g. ~0.32 kWh on a 31.6 kWh pack, ~9.5 minutes flat at a 2 kW rate. Before treating a flat run as evidence of a stall, check whether its duration is fully explained by the pack's percent-to-kWh granularity at the charge/discharge rate in play (GH#4784). +- **The HA Companion app's embedded webview ignores `Content-Disposition: attachment`** and renders any download link inline instead, even when the server sends correct headers — confirmed for both the debug-snapshot and bulk-archive routes (GH#4720). A "download doesn't work" report from the Companion app specifically is a client limitation, not a server bug. Debug-history snapshots are already saved as plain-text `.txt` YAML in `/cache/` (`debug_history.py:132`, `format="text"`), so they're readable via File Editor/Samba without going through the download route at all. ## Adding to this file diff --git a/.cspell/custom-dictionary-workspace.txt b/.cspell/custom-dictionary-workspace.txt index c6045eea6..19813807e 100644 --- a/.cspell/custom-dictionary-workspace.txt +++ b/.cspell/custom-dictionary-workspace.txt @@ -54,6 +54,7 @@ batteryb batusecap beforeunload bierner +BLAS boostable brickatius byok @@ -575,6 +576,7 @@ Teslemetry testname thirdparty threadpool +threadpoolctl timea timeb timec @@ -617,6 +619,8 @@ unparseable unpickled unpickles unpushed +unredacted +unreproduced unsmoothed unstaged untriaged diff --git a/tools/test_triage_daemon.py b/tools/test_triage_daemon.py index 681b8641e..07777ec3c 100644 --- a/tools/test_triage_daemon.py +++ b/tools/test_triage_daemon.py @@ -7,6 +7,7 @@ mocked - nothing here touches a real repo, GitHub, or Claude Code session. """ +import fnmatch import json import subprocess import tempfile @@ -17,6 +18,12 @@ import triage_daemon +def bash_rule_matches(rule, command): + """Simulate Claude Code's Bash(...) permission-rule prefix-glob matching against a command.""" + pattern = rule.removeprefix("Bash(").removesuffix(")") + return fnmatch.fnmatchcase(command, pattern) + + class DaemonPathsTestCase(unittest.TestCase): """Base class that points the daemon's module-level paths at a scratch temp dir.""" @@ -382,6 +389,39 @@ def test_pr_disallowed_tools_still_blocks_force_push_variants(self): self.assertTrue(any("force" in entry for entry in pr_denied), pr_denied) self.assertTrue(any("-f" in entry for entry in pr_denied), pr_denied) + def test_force_push_denials_do_not_false_positive_on_a_branch_name_containing_f(self): + """Regression test for issue #4788: the force-push heuristic used to be + "Bash(git push*-f*)", an unanchored substring match that read the "-f" inside + an ordinary branch name as the force-push flag and denied a completely normal + push. "fix/power-flow-car-outside-ct-clamp-4788" contains "-flow", which tripped + it - the PR flow's own push step got silently denied and the branch never made + it to origin from that run. Anchoring "-f" to its own token (spaces on both + sides, or the end of the command) must not regress back to matching substrings + like "-flow", "-fix" or "-format".""" + ordinary_pushes = [ + "git push -u origin fix/power-flow-car-outside-ct-clamp-4788", + "git push -u origin fix/auto-format-thing-1234", + "git push -u origin feat/prefix-field-support-9001", + ] + force_push_denials = [rule for rule in triage_daemon.DISALLOWED_TOOLS_PR.split(",") if "force" in rule or "-f" in rule] + for command in ordinary_pushes: + matched = [rule for rule in force_push_denials if bash_rule_matches(rule, command)] + self.assertEqual(matched, [], f"{command!r} was falsely denied by {matched}") + + def test_force_push_denials_still_catch_real_force_push_spellings(self): + """The tightened force-push rules must still deny every spelling a maintainer + or a prompt-injected instruction would actually use.""" + real_force_pushes = [ + "git push -f origin main", + "git push origin main -f", + "git push --force origin main", + "git push --force-with-lease origin main", + ] + force_push_denials = [rule for rule in triage_daemon.DISALLOWED_TOOLS_PR.split(",") if "force" in rule or "-f" in rule] + for command in real_force_pushes: + matched = any(bash_rule_matches(rule, command) for rule in force_push_denials) + self.assertTrue(matched, f"{command!r} should still be denied, but no rule in {force_push_denials} matched") + def test_review_disallowed_tools_still_blocks_dangerous_gh_subcommands(self): """The BOT_REVIEW-on-PR flow keeps every dangerous gh subcommand denied, including generic gh api calls against another repo.""" diff --git a/tools/triage_daemon.py b/tools/triage_daemon.py index 1011e94fb..0fec2ffc8 100644 --- a/tools/triage_daemon.py +++ b/tools/triage_daemon.py @@ -188,7 +188,16 @@ # Even though the PR flow can push, force-push variants stay denied - defense in depth # against a prompt-injected instruction attempting to rewrite history. Prefix-glob # matching can't parse flags, so this is a heuristic, not a guarantee. -_PR_FORCE_PUSH_DENIALS = ["Bash(git push*--force*)", "Bash(git push*-f*)"] +# +# The "-f" entry needs a space on both sides of the flag, not "git push*-f*" - that +# unanchored form matches "-f" as a substring anywhere in the command, including +# inside a perfectly ordinary branch name. "git push -u origin +# fix/power-flow-car-outside-ct-clamp-4788" contains "-flow", which the old pattern +# read as a force-push flag and denied outright (issue #4788: the branch never made +# it past a manual push). Anchoring "-f" to its own token still catches "git push -f +# ...", "git push ... -f" and "--force"/"--force-with-lease", without also catching +# "-flow", "-fix", "-format" or any other word that merely contains "-f". +_PR_FORCE_PUSH_DENIALS = ["Bash(git push* --force*)", "Bash(git push* -f)", "Bash(git push* -f *)"] DISALLOWED_TOOLS_PR = ",".join([item for item in _DISALLOWED_TOOLS_BASE if item not in _PR_REMOVED_DENIALS] + _PR_FORCE_PUSH_DENIALS) # The review and cleanup flows do NOT inherit the broad "Bash(gh *)" grant: with it # present, carving a scoped exception out of the gh api denial below would do nothing,