docs(skill): fold recent-issue lessons and the simplify-after-review pass into hassio-addon-workflow (#3015)

* docs(skill): simplify again after the code review, and demand a trigger for defensive branches

The full loop ran simplify (step 5) before Codex's code review (step 6) and never
again, so nothing walked back what the review added. Adversarial review is asked to
find what could go wrong, so its output is a list of arguments for more code and it
is never asked whether the branch it wants is reachable — accepting objections only
ratchets the diff upward. Step 6 now ends by re-running step 5's checks over the
hunks the review touched.

The other half was earlier than the review. The standing rule already said complexity
is bought only by a measurement, but it said it about performance, so a branch added
for robustness did not visibly fall under it. It now covers hypothetical hosts as
well as hypothetical performance: name the input that reaches a defensive branch and
the image it happens on, or delete it and let the case fail visibly. Step 3's
attack-your-own-plan list asks the same question before any code exists, which is
where it is cheapest to answer.

The case study in references/simplify.md is #3013: 25 lines of code at review, 10
merged. A pure-bash fallback written at implement time for images shipping
with-contenv but not s6-dumpenv — reasoned from the two binaries living in different
s6 packages, never demonstrated on a real image, and defending a case that would have
degraded to the pre-fix behaviour anyway — plus the helper function and second reset
that existed only to serve it. Deleting the fallback deleted all of it. The review's
own objections were correct and cost two tokens on an existing line.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(skill): fold recent-issue lessons into hassio-addon-workflow

Distilled from the calibre-web trusted-ips saga (#3004/#3009/#3010) and the
seerr builder revert (#2993/#2997):

- traps.md: 'merged is not on master' (builder revert-on-failure); new
  section on writing into an app's own config (user-editable fields,
  prefer boot-constant values, dual-stack mapped ranges)
- simplify.md: case study — stateful merge machinery (+34 lines, closed)
  vs trusting the static supervisor range (net -6 lines, shipped)
- SKILL.md: post-merge survival check in step 9; a one-line feedback
  loop in step 10 so follow-up PRs feed lessons back into references/

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mbqutqkh7yTj4EnhWKFBQx

* docs(skill): address Codex review — auth blast radius, fetch before post-merge check

- traps.md: trusting a whole range for an auth header is an impersonation
  trade-off needing the maintainer's explicit call, not a neutral
  simplification (Codex P1)
- SKILL.md: fetch origin master before the post-merge survival check, the
  tracking ref is stale otherwise (Codex P2)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mbqutqkh7yTj4EnhWKFBQx

* docs(skill): fetch before the traps.md post-merge check too (Copilot review)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mbqutqkh7yTj4EnhWKFBQx

* docs(skill): name the post-merge check explicitly — tree, not ancestry

CodeRabbit's merge-risk note on #3015: the post-merge step said to confirm
the commit 'survived', which reads as an ancestry check. A revert leaves the
commit in history and undoes its tree, so --contains reports success on
exactly the case the step exists to catch. Names the diff check instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mbqutqkh7yTj4EnhWKFBQx

* docs(skill): scope the post-merge check to your paths, not the whole tree

CodeRabbit was right that traps.md's 'your commit's tree is still what
origin/master holds' is invalid on a moving master — unrelated commits break
whole-tree equality. Scoped to the touched paths, matching SKILL.md.

Its other half, an ancestry check with merge-base --is-ancestor, does not
hold here: the repo squash-merges, so a merged PR head is never an ancestor.
Verified on #3010 — --is-ancestor reports NOT an ancestor while its fix is
live on master, i.e. a false failure on exactly the case the step must pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mbqutqkh7yTj4EnhWKFBQx

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

.claude/skills/hassio-addon-workflow/SKILL.md

@@ -29,8 +29,8 @@ Triage first, then one of two paths:
   resolve bot comments.
 - **Full loop** — performance/RAM/CPU work, diagnosis, anything changing a shipped default,
   ladder levels 4-6, or an explicit Codex-check request: scope → measure → plan → Codex reviews
-  the plan → implement → simplify → Codex reviews the code → PR → resolve comments → verify in
-  production → report.
+  the plan → implement → simplify → Codex reviews the code → **simplify again** → PR → resolve
+  comments → verify in production → report.
 
 Escalate mid-flight if a light task grows — touches a default, needs a new script or service, or
 reveals a deeper problem.
@@ -42,7 +42,10 @@ where every add-on solves a problem the same way is worth more than a locally ni
 design. Prefer reusing or extending over adding a parallel implementation, and when you must add
 something new, spell it the way the rest of the repo spells it (naming, option names, script
 numbering, file layout). Complexity is bought only by a **measurement** showing a concrete,
-user-visible cost on a real host — never by reasoning about hypothetical performance.
+user-visible cost on a real host — never by reasoning about hypothetical performance, and never by
+reasoning about a hypothetical *host* either. A defensive branch is complexity like any other: name
+the input that reaches it and the image or host where that happens, or delete it and let the case
+fail visibly instead.
 
 **Repo layout.** `alexbelgium/hassio-addons`; each add-on is a top-level directory. This skill is
 checked in at `.claude/skills/hassio-addon-workflow/` (canonical copy). Set the skill root once,
@@ -113,6 +116,10 @@ Attack your own plan before implementing:
 - What is the **blast radius** if the assumption underneath it is wrong?
 - What am I **inferring** that I could instead **detect at runtime** or **record explicitly**?
   Highest-yield question here — see `references/evidence.md`'s failure-mode section.
+- For every branch that exists **only to survive something going wrong**: name the image or host
+  where that input actually arrives, and go and look. Naming is the bar, not reproducing it here —
+  `references/simplify.md` works the `/dev/shm` guard and the `s6-dumpenv` fallback through that
+  distinction.
 
 Full loop only, before writing code: get Codex's independent read on the plan. Spawn a subagent
 whose prompt includes the path `references/codex-review.md` and tells it to follow that file's
@@ -140,11 +147,22 @@ justify the divergence in the PR body — but never at the cost of an isolation
 numbered script, not an edit. Case studies of what happens when this check is skipped:
 `references/simplify.md`.
 
-## 6. Codex attacks the code (full loop only)
+## 6. Codex attacks the code, then simplify what the review added (full loop only)
 
 Same delegated invocation, pointed at `git diff origin/master...HEAD` plus your reasoning per
 hunk. Details in `references/codex-review.md`.
 
+Then run step 5's checks again over the hunks the review changed. Adversarial review only ever
+argues *for* another branch — that is its job — so accepting objections ratchets the diff upward,
+and nothing else in the loop walks it back down. For each accepted objection: is the case it
+defends one you have now demonstrated, or one you have merely been told about? Taking a
+correctness objection often deletes the code that made it necessary, and a fix that collapses back
+to fewer lines than you started the review with is the normal outcome, not a suspicious one.
+
+These edits land after step 4's checks already ran, so re-run them: `scripts/validate.sh <addon>
+--vs-master` plus the behavioural tests, over the final diff. Deleting a branch is exactly the
+kind of edit that leaves a stray `fi` behind.
+
 ## 7. Open the PR
 
 CI gates on a PR: **`CHANGELOG.md` updated** (hard fail), the **HA add-on linter**
@@ -172,7 +190,12 @@ work" — either it was exercised, or say plainly it wasn't.
 
 Light path: verification is `validate.sh` plus CI; anything beyond that is Assumed. Full loop: CI
 passing proves the build works, not that the change does anything — re-run the measurement that
-motivated the work once the rebuilt add-on is running. Real "merged and inert" examples, and what
+motivated the work once the rebuilt add-on is running. After merge, `git fetch origin master`
+(the tracking ref is stale otherwise), then confirm the *changes* survived — `git diff
+origin/master -- <the paths you touched>` comes back empty. Ancestry is not the check: a revert
+leaves your commit in history and undoes its tree, so `--contains` reports success either way. The
+builder's revert-on-failure job can revert a merge for reasons unrelated to your diff (see
+`references/traps.md#ci-and-review-bots`). Real "merged and inert" examples, and what
 to do when a fix can't be self-verified: `references/evidence.md`.
 
 ## 10. Calibrate and report
@@ -191,5 +214,9 @@ Risk + rollback                 — the riskiest hunk and how to revert it alone
 Lead with anything that did not work — a merged PR that achieved nothing is the single most
 important sentence in the report. Give confidence per claim, not one blanket number.
 
+**Feed the skill.** When a shipped fix needed a follow-up PR, or a reviewer caught something this
+skill should have, add the distilled lesson to the matching `references/` file in that follow-up
+PR — one entry, with the PR numbers. That loop is what keeps this file short and the traps real.
+
 Scripts are meant to be **run, not read** — each is cited at its point of use above; read one
 only if its output surprises you.

.claude/skills/hassio-addon-workflow/references/codex-review.md

@@ -39,3 +39,10 @@ codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
 **Codex agrees with confident premises.** It has confirmed a wrong conclusion stated too
 confidently, and separately caught a genuine methodology error in the same review. Treat its
 confirmations with the same scepticism as its objections — especially about the build.
+
+**Its objections ratchet complexity upward.** An adversarial reviewer is asked to find what could
+go wrong, so its output is a list of arguments for more code; it is never asked whether the branch
+it wants is reachable. Separate "this is wrong" from "this is undefended" before you write
+anything: the first is a bug and you fix it, the second is a claim about some host, and it needs
+the same demonstration you would demand of a measurement. That is what step 6's second simplify
+pass is for.

.claude/skills/hassio-addon-workflow/references/simplify.md

@@ -1,7 +1,7 @@
 # Simplify — case studies
 
 Evidence for why the mechanism ladder in SKILL.md step 3 exists, and why levels 4-6 need a reason
-that survives being said out loud. In all three cases the simpler option existed and was skipped.
+that survives being said out loud. In each case the simpler option existed and was skipped.
 Being able to build the complicated thing is not a reason to.
 
 - A rejected PR spent a **388-line TCP proxy plus a 142-line monkeypatch of a private upstream
@@ -13,6 +13,22 @@ Being able to build the complicated thing is not a reason to.
 - A resolution cap shipped as a **new init script writing an s6 envdir** — the wrong mechanism
   entirely (ladder level 4). Renaming the option to the env var the service already reads
   (level 1) would have worked, and the new script did not.
+- A calibre-web trusted-ips fix injected the add-on's **per-boot IP**, which forced a
+  rewrite-every-boot design that erased user entries; preserving them then needed merge logic
+  plus a state file recording what was injected (+34 lines, PR #3009 — closed unmerged). Asking
+  "is there a constant that makes the rewrite unnecessary?" gave the shipped fix: trust the
+  static supervisor range `172.30.32.0/23`, one idempotent statement, **net −6 lines**
+  (PR #3010). Complexity spent working around a changing value is a sign to hunt for the
+  constant instead.
+- A `.templates/ha_entrypoint.sh` fix went to review at 25 lines of code and merged at 10. Two
+  sources of the excess, and neither was caught by the loop: a **pure-bash fallback** written at
+  implement time for images shipping `with-contenv` but not `s6-dumpenv` — reasoned from the two
+  binaries living in different s6 packages, never demonstrated on any real image, and the case it
+  defended would have degraded to the pre-fix behaviour anyway — and a **helper function plus a
+  second reset** that existed only to serve that fallback. Deleting the fallback deleted all of
+  it. The rest of the review's objections were correct and cost two tokens on an existing line.
+  It took the maintainer asking "is this the simplest way possible?" to run the pass that step 6
+  now requires.
 
 ## Checks worth running against your own diff
 
@@ -25,6 +41,17 @@ Being able to build the complicated thing is not a reason to.
   A removal that depends on a host default still needs the same verification as an addition.
 - **Is the fix bigger than the thing it fixes?** That is a smell, not a rule — but it usually
   means the problem was framed one level too deep.
+- **For each defensive branch: what input reaches it, on which image or host?** Go and check,
+  the way you would check a measurement. The bar is being able to **name** the case, not to
+  reproduce it here: Docker's 64 MB `/dev/shm` default is documented behaviour that HA does not
+  override, so the bullet above keeps that guard even though this host measured 7.7 GB. Nobody
+  could name a single image shipping `with-contenv` without `s6-dumpenv`, so that fallback went.
+  If you cannot name the case, delete the branch — the situation then fails the way it already
+  fails today, visibly, instead of through a second path that is never exercised and silently
+  rots as the base images move. Weigh the cost too: a one-flag guard against a crash you cannot
+  rule out is cheap, a second code path that degrades to the pre-fix behaviour anyway is not.
+  Write down in the PR body what you cut and why, so the next person does not re-add it from the
+  same reasoning.
 - **How does this fail in three years**, when the base image, Electron, or upstream has moved?
   Code that reads a documented knob keeps working. Code that reaches into private internals
   does not.

.claude/skills/hassio-addon-workflow/references/traps.md

@@ -11,6 +11,7 @@ workflows and lint rules — that is not repeated here.
 - [Environment and workspace](#environment-and-workspace)
 - [Measurement](#measurement)
 - [Passing values into base-image services](#passing-values-into-base-image-services)
+- [Writing into an app's own config](#writing-into-an-apps-own-config)
 - [Shell and bashio](#shell-and-bashio)
 - [Dockerfile and architecture](#dockerfile-and-architecture)
 - [Versioning](#versioning)
@@ -116,6 +117,26 @@ place for filesystem and permission setup.
 the openbox autostart. (ANGLE's OpenGL backend, for instance, fails with "Could not open the
 default X display".)
 
+## Writing into an app's own config
+
+**A field your cont-init script writes may also be user-editable in the app's UI.** An
+unconditional `UPDATE`/overwrite on every boot silently erases whatever the user added there,
+and containers are recreated on restart so it re-erases forever (calibre-web
+`config_reverse_proxy_trusted_ips`, #3004 — flagged by two review bots, fixed in #3010).
+Prepend/merge with an idempotence guard instead of assigning.
+
+**Prefer values that are constant across boots.** The add-on's own IP changes every restart,
+so injecting it forces a rewrite-every-boot design plus stale-entry cleanup (a stale trusted IP
+can be handed to a *different* add-on later). Trusting the whole supervisor range
+`172.30.32.0/23` is constant, written once. For dual-stack listeners the IPv4 form never
+matches IPv4-mapped addresses — also list the mapped form (`::ffff:172.30.32.0/119`).
+
+Constant is not free when the value gates **authentication**: trusting the whole range means any
+add-on on the supervisor network can send the auth header and impersonate a user. #3010 shipped
+that as an explicit, stated trade-off with the maintainer's sign-off. State the blast radius in
+the PR body and get the maintainer's call before widening trust — never present it as a neutral
+simplification.
+
 ## Shell and bashio
 
 **`bashio::config` for lists**: `while read ... < <(bashio::config ...)` silently yields an empty
@@ -195,6 +216,17 @@ All three hard gates below are matrixed over `check-addon-changes.outputs.change
 - **Version bump** — no workflow checks it. It is repo convention, and required for Supervisor to
   offer the rebuild, but it will not fail CI.
 
+**"Merged" is not "on master".** The push builder's revert-on-failure job reverts the merge
+commit when its prebuild step fails — including failures unrelated to your diff. A seerr fix
+merged at 05:15 and was reverted one minute later because `EndBug/add-and-commit`'s floating
+`v11` tag had moved to a broken release (#2993, reapplied verbatim in #2997). After merge,
+`git fetch origin master` first — the remote-tracking ref is stale otherwise and would "confirm"
+against pre-merge state — then check that `git diff origin/master -- <the paths you touched>` is
+empty before declaring done. Scope it to your paths: master moves under you, so whole-tree
+equality fails on unrelated commits. Ancestry is not the check either — this repo squash-merges,
+so a merged PR head is never an ancestor of `master` (verified on #3010, whose fix is live), and
+a revert leaves the original commit an ancestor anyway.
+
 **CI rewrites your shell scripts.** `lint.yml` runs `shfmt -w -i 4 -ci -bn -sr` over every `*.sh`
 and `run`, plus a `chmod +x` pass, on schedule. Repo-wide reformatting commits land on master
 without your involvement — another reason a shared checkout goes stale mid-task.