mirror of
https://github.com/alexbelgium/hassio-addons.git
synced 2026-08-06 21:22:29 +02:00
refactor(skill): shorten hassio-addon-workflow via progressive disclosure (#2942)
* refactor(skill): shorten hassio-addon-workflow via progressive disclosure SKILL.md was 365 lines, loaded in full on every add-on task. Split it per Anthropic's Agent Skills best practices: keep steps, completion criteria, and the mechanism ladder inline; push rationale, war-story examples, and Codex CLI invocation details into reference files loaded only when that branch is taken. - SKILL.md: 365 -> 171 lines. The "ship the simplest solution" rule was stated three times; now once. Steps 3/6/9 keep their load-bearing checklist but point to detail files instead of inlining it. - references/evidence.md (new): measurement methodology, the host-generalization failure examples, the merged-and-inert case studies - references/codex-review.md (new): CLI invocation, prompt guidance, plan-attack checklist - references/simplify.md (new): mechanism-ladder case studies Also replaces the old "Token efficiency" section (which named this maintainer's personal MCP tools - rtk/headroom/tokensave - not guaranteed present for CI agents or other collaborators using the checked-in copy) with a subagent-delegation instruction: Codex's plan/code review and PR-comment triage on >5 threads should run in a subagent that returns a condensed summary, not raw output, into the calling session. * fix(skill): address PR review feedback - SKILL.md: define $SKILL once and state that all scripts/ and references/ shorthand paths are relative to it — they read as repo-root-relative otherwise and don't resolve from an add-on directory - codex-review.md: drop the reference to a "global CLAUDE.md note" that isn't in this repo's CLAUDE.md; describe --sandbox read-only accurately (reads allowed, writes/exec blocked, approvals disabled) instead of "cannot run anything"; add the missing & so the example is actually backgrounded as the prose claims - evidence.md: use smaps_rollup for process RSS — plain smaps prints one Rss: line per mapping (47 for a trivial process here), not a total - simplify.md: drop the absolute "removals cannot regress" claim, which contradicted the /dev/shm case study in evidence.md * refactor(skill): apply independent quality-review findings From an independent model review of the skill against the Agent Skills best-practice guides: - traps.md: CHANGELOG was described as "the only hard gate", contradicting SKILL.md — the HA add-on linter and the image build also block PRs (verified against onpr_check-pr.yaml). Gates list corrected. - codex-review.md: the "attack your own plan" checklist is run by the main agent, but lived in the file whose stated consumer is the delegated subagent — an agent that delegates correctly would never see it. Moved inline into SKILL.md step 3. - SKILL.md: delegation instruction now says exactly what to do (spawn a subagent whose prompt includes references/codex-review.md and follows its invocation) instead of "delegated per the rule above". - SKILL.md: traps.md is no longer mandatory reading on the light path — the light-path facts it holds (versioning, CHANGELOG format) are inline in step 7; it stays required when touching scripts/Dockerfiles/env. - SKILL.md: dropped the bundled-files table (every row already cited at point of use) and the "read the script when you use it" anti-instruction — scripts are run, not read. 177 -> 167 lines. - description: 982 -> 550 chars; removed workflow narrative that does nothing for skill selection and the stale-prone model name, kept all trigger terms. * fix(skill): note that CI hard gates skip on non-addon PRs The three hard gates in onpr_check-pr.yaml (changelog check, addon-linter, check-build) are each matrixed over check-addon-changes.outputs.changedAddons and if:-skipped when it is '[]'. A PR touching only docs, .github/ or .claude/ therefore shows them as "skipping" rather than passing — which should not be read as a green build. Verified against onpr_check-pr.yaml lines 64, 83, 101 and against this PR's own check output.
This commit is contained in:
Alexandre
@@ -1,314 +1,155 @@
|
||||
---
|
||||
name: hassio-addon-workflow
|
||||
description: >-
|
||||
End-to-end workflow for alexbelgium/hassio-addons add-on work — scope the change, diagnose
|
||||
against the live add-on with real measurements, get an independent Codex (gpt-5.6-sol) review
|
||||
of the plan, implement, have Codex review the code adversarially, open a PR, resolve the
|
||||
CodeRabbit / Copilot / Codex-connector review comments, and verify the merged result actually
|
||||
works. Use this whenever the task touches a Home Assistant add-on in this repo — fixing a bug
|
||||
or reported issue, tuning RAM, CPU or performance, editing a Dockerfile, config.yaml,
|
||||
cont-init.d script or s6 service, bumping an add-on version, or opening and iterating a PR
|
||||
against hassio-addons. Also use it when asked to "check with codex", "verify with chatgpt", or
|
||||
to resolve bot review comments on an add-on PR. Small tasks (typo fixes, version bumps,
|
||||
one-file tweaks, simple coding questions) route through a light path that skips measurement
|
||||
and Codex reviews — invoking this skill is cheap for small asks too.
|
||||
Workflow for alexbelgium/hassio-addons Home Assistant add-on work: diagnose with real
|
||||
measurements, independent Codex review, implement, open a PR, resolve CodeRabbit / Copilot /
|
||||
Codex bot review comments, verify in production. Use for any task touching an add-on in this
|
||||
repo — bugs, RAM/CPU/performance tuning, Dockerfile, config.yaml, cont-init.d or s6 changes,
|
||||
version bumps, opening or iterating PRs — and when asked to "check with codex", "verify with
|
||||
chatgpt", or resolve bot comments. Cheap for small asks: a light path skips the heavy steps.
|
||||
---
|
||||
|
||||
# Home Assistant add-on workflow
|
||||
|
||||
Triage first, then one of two paths:
|
||||
|
||||
- **Light path** (small, contained tasks): scope → implement the simplest mechanism →
|
||||
validate → PR → resolve bot comments → report honestly.
|
||||
- **Full loop** (diagnosis, performance, defaults, new mechanisms): scope → measure → plan →
|
||||
Codex attacks the plan → implement → simplify → Codex attacks the code → PR → resolve bot
|
||||
comments → verify in production → report with calibrated confidence.
|
||||
- **Light** — typo/doc fixes, CHANGELOG edits, version bumps, one-file edits at ladder levels
|
||||
1-3 (below), simple questions: scope → implement → validate (`$SKILL/scripts/validate.sh
|
||||
<addon> --vs-master`; `$SKILL` defined below) → PR (version bump + CHANGELOG still required) →
|
||||
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 standing rule on both paths:** when a simple solution and a more efficient-but-more-complex
|
||||
one both work, ship the simple one. Complexity is only paid for by a **measurement** showing the
|
||||
simple version has a concrete, user-visible cost on a real host — never by reasoning about
|
||||
hypothetical performance. Slightly less efficient and obviously correct beats faster and harder
|
||||
to review.
|
||||
Escalate mid-flight if a light task grows — touches a default, needs a new script or service, or
|
||||
reveals a deeper problem.
|
||||
|
||||
The disciplines below exist because each one, when skipped, produced a specific failure in this
|
||||
repo — the examples throughout are real, not illustrative.
|
||||
**Standing rule:** ship the simplest solution that works. Complexity is bought only by a
|
||||
**measurement** showing a concrete, user-visible cost on a real host — never by reasoning about
|
||||
hypothetical performance.
|
||||
|
||||
| Discipline | The failure it prevents |
|
||||
|---|---|
|
||||
| **Triage before you start** | Full ceremony spent on a one-line fix |
|
||||
| **Scope before you work** | Solving the wrong problem, elegantly |
|
||||
| **Evidence before reasoning** | Confident claims that measurement contradicts |
|
||||
| **Reason adversarially** | Shipping something that only works on your host |
|
||||
| **Simplify to the smallest thing that works** | 500 lines of machinery where an option would do |
|
||||
| **Verify before declaring done** | "This should work" — and it doesn't |
|
||||
| **Calibrate and report** | Verified and assumed presented as the same thing |
|
||||
**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,
|
||||
then every `scripts/…` and `references/…` path below is relative to it:
|
||||
|
||||
**Where things are.** The repo is `alexbelgium/hassio-addons`; each add-on is a top-level
|
||||
directory (`claude_desktop/`, `birdnet-go/`, …). This skill and its scripts are checked into
|
||||
the repo at `.claude/skills/hassio-addon-workflow/` — invoke scripts from the repo root, e.g.
|
||||
`bash "$(git rev-parse --show-toplevel)/.claude/skills/hassio-addon-workflow/scripts/preflight.sh"`.
|
||||
(In the claude_desktop add-on environment the checkout lives at `/data/claude/hassio-addons`;
|
||||
a copy of this skill may also exist under `~/.claude/skills/` — the checked-in copy is
|
||||
canonical.)
|
||||
```bash
|
||||
SKILL="$(git rev-parse --show-toplevel)/.claude/skills/hassio-addon-workflow"
|
||||
bash "$SKILL/scripts/preflight.sh" # and likewise for the other scripts
|
||||
```
|
||||
|
||||
**Three facts to know before you touch anything**, because each is silent when violated:
|
||||
**Non-negotiables:**
|
||||
- Docker build cannot be tested locally (no dockerd) — CI is the only gate.
|
||||
- Never `git stash` under `/data/claude` — `refs/stash` is shared across worktrees.
|
||||
- Work in a worktree under `/data`, not `/tmp` (`/tmp` is noexec).
|
||||
|
||||
- **You cannot test the Docker build** — dockerd does not start here. CI is the only gate.
|
||||
- **Never `git stash` under `/data/claude`** — `refs/stash` is shared across worktrees and
|
||||
sessions, so it is not isolated even in your own worktree.
|
||||
- **Work in a worktree under `/data`, not `/tmp`** (`/tmp` is `noexec`).
|
||||
|
||||
Repo-specific traps live in `references/traps.md`. Read it before implementing — it is the
|
||||
accumulated cost of previous sessions. The repo's own `CLAUDE.md` covers structure, Dockerfile
|
||||
conventions, CI and lint rules.
|
||||
**Delegate heavy output to a subagent.** Codex reviews and multi-thread PR triage produce output
|
||||
you don't need verbatim in your own context. For Codex's plan review (step 3), Codex's code
|
||||
review (step 6), and PR-comment listing when there are more than ~5 threads (step 8): launch a
|
||||
subagent to run the command and report back only the objections/findings and your assessment of
|
||||
each, not the raw transcript.
|
||||
|
||||
---
|
||||
|
||||
## 0. Triage — pick the path
|
||||
## 1. Scope
|
||||
|
||||
Classify the task before doing anything else.
|
||||
State goal, non-goals, constraints, and definition of done — two sentences, explicit. A diagnosis
|
||||
ask ("why is it slow?") is not automatically a fix ask. Changing a shipped default is the user's
|
||||
call, not yours — ask before implementing.
|
||||
|
||||
**Light path** — typo/doc fixes, CHANGELOG edits, version bumps, one-file small edits at
|
||||
mechanism levels 1–3 (the ladder in step 3), simple coding questions. Flow: one-sentence scope →
|
||||
pick the simplest mechanism → implement → `scripts/validate.sh <addon> --vs-master` → PR
|
||||
(version bump + CHANGELOG still required) → resolve bot comments. Skip measurement, both Codex
|
||||
reviews, and post-deploy measurement — but still label claims Verified / Checked / Assumed
|
||||
honestly in the report.
|
||||
## 2. Evidence before reasoning (full loop)
|
||||
|
||||
**Full loop** — performance/RAM/CPU work, diagnosis ("why is X slow/broken"), anything changing
|
||||
a shipped default, changes spanning several scripts or mechanisms (a version bump's
|
||||
config.yaml + CHANGELOG + Dockerfile touch is still light), anything at ladder levels 4–6, or
|
||||
when the user asks for a Codex check.
|
||||
Measure the running add-on rather than reasoning from source (`$BUILD_VERSION` set,
|
||||
`HOME=/data/data`) — reviewers hold you to the numbers. Tool per question:
|
||||
|
||||
**Escalation rule** — if a light task grows mid-flight (touches a default, needs a new script or
|
||||
service, reveals a deeper problem), stop and upgrade to the full loop rather than continuing
|
||||
light.
|
||||
- RAM/CPU → `scripts/measure.sh` (≥20 s sample)
|
||||
- "I set an option and nothing happened" → `scripts/env_trace.sh <VAR> <process>`
|
||||
- Is this flag/driver/package actually present? → inspect the artifact: `/proc/<pid>/cmdline`,
|
||||
`command -v`, `/var/log/apt/history.log`
|
||||
|
||||
`references/traps.md` is required reading on **both** paths — traps bite one-liners too.
|
||||
Verify you're reading the right revision first — `scripts/preflight.sh` catches a stale branch
|
||||
before it costs a full analysis pass. Measurement methodology, gotchas, and real failure examples:
|
||||
`references/evidence.md`.
|
||||
|
||||
## 1. Scope before you work
|
||||
## 3. Plan — choose the mechanism level, then Codex reviews it (full loop)
|
||||
|
||||
Write this down before generating anything. Two sentences is enough, but they must be explicit:
|
||||
Rank mechanisms, pick the lowest (simplest) one that solves it, and state the choice in the plan:
|
||||
|
||||
- **Goal** — the observable outcome, in the user's terms.
|
||||
- **Non-goals** — what you are deliberately not touching. This is the load-bearing half.
|
||||
- **Constraints** — what cannot change (defaults for other users, upgrade paths, arch support).
|
||||
- **Definition of done** — what evidence will demonstrate success.
|
||||
1. A config value — an option, a schema constraint, an existing env var.
|
||||
2. An existing knob the base image already reads (`MAX_RES`, `DRINODE`, `SELKIES_*`).
|
||||
3. A few lines in an existing script, at the point that already runs.
|
||||
4. A new init script.
|
||||
5. A new service, wrapper, or long-running process.
|
||||
6. Custom protocol code, or patching someone else's internals.
|
||||
|
||||
A diagnosis task and a fix task have different scopes. "Why is it slow?" is answered by
|
||||
measurement and a written finding; it does not automatically authorize a rewrite. When the user's
|
||||
ask genuinely spans several changes, list them and say which you are doing now.
|
||||
|
||||
Ask about defaults when the answer changes who is affected. Changing a shipped default touches
|
||||
every user of a 120-add-on repo; making it opt-in touches nobody until they choose. That is the
|
||||
user's call, not yours, and it is cheap to ask before implementing rather than after.
|
||||
|
||||
## 2. Evidence before reasoning
|
||||
|
||||
State no cause you have not observed. On a live add-on (`$BUILD_VERSION` set, `HOME=/data/data`)
|
||||
measure the running system rather than reasoning from source — reviewers will hold you to the
|
||||
numbers, and source-derived guesses are where wrong plans come from.
|
||||
|
||||
Pick the tool to the question:
|
||||
|
||||
- **"what is consuming RAM/CPU?"** → `scripts/measure.sh` (PSS and private, not summed RSS; keep
|
||||
the sample at ≥20 s — a 3 s sample measured 2.3% where 20 s measured 21.6%).
|
||||
- **"I set an option and nothing happened"** → `scripts/env_trace.sh <VAR> <process>`, which
|
||||
walks all four stages of the option plumbing and names the one that dropped the value.
|
||||
- **"is this flag/driver/package actually present?"** → look at the artifact itself:
|
||||
`/proc/<pid>/cmdline`, `command -v`, `/var/log/apt/history.log`.
|
||||
|
||||
`references/traps.md#measurement` explains why summed RSS and reserved-vs-resident both matter.
|
||||
|
||||
Before asserting anything, ask what would show it false, then go look:
|
||||
|
||||
- "This process is duplicated" → is it? `ps -ef --forest`, compare parents and start times.
|
||||
- "This costs 500 MB" → is it resident? `grep Rss /proc/<pid>/smaps`.
|
||||
- "This block never runs" → is its payload in the image? `command -v`, `apt` history.
|
||||
- "The flag isn't set" → `tr '\0' '\n' < /proc/<pid>/cmdline`.
|
||||
|
||||
**Verify the revision you are reading.** `scripts/preflight.sh` compares the checkout's
|
||||
`config.yaml` version against the running `$BUILD_VERSION`. A stale branch reads as completely
|
||||
normal and has already cost one full analysis pass.
|
||||
|
||||
When you correct yourself mid-analysis, keep the correction visible in your notes and in what you
|
||||
report. A retracted claim that stays retracted is worth more than one quietly dropped.
|
||||
|
||||
## 3. Plan, then have Codex attack it
|
||||
|
||||
### Choose the mechanism level first (both paths)
|
||||
|
||||
Rank mechanisms and start from the top — choose the level **before writing code** and state it
|
||||
in the plan. Each step down costs more to write, more to review, and more to keep working across
|
||||
base-image upgrades:
|
||||
|
||||
1. **A config value** — an option, a schema constraint, an existing env var.
|
||||
2. **An existing knob** the base image already reads (`MAX_RES`, `DRINODE`, `SELKIES_*`).
|
||||
3. **A few lines in an existing script**, at the point that already runs.
|
||||
4. **A new init script.**
|
||||
5. **A new service, wrapper, or long-running process.**
|
||||
6. **Custom protocol code, or patching someone else's internals.**
|
||||
|
||||
Levels 4–6 automatically mean full loop, and need a reason that survives being said out loud.
|
||||
"Upstream has no knob for this, and I checked" is a reason. "It felt cleaner" is not. If two
|
||||
levels both solve it, the higher (simpler) level wins even when the lower one would be more
|
||||
efficient — see the standing rule at the top.
|
||||
|
||||
### Codex review of the plan (full loop only — skip on the light path)
|
||||
|
||||
Write the plan around the measurements — each proposed change tied to a number — then get an
|
||||
independent read **before** writing code. Codex is a genuinely different model reading the files
|
||||
itself; on this workload it has repeatedly been worth the minutes.
|
||||
|
||||
**Use the CLI, not the MCP tool, for prompts of this size.** `mcp__codex__codex` timed out
|
||||
twice on ~4 KB prompts (2026-08-03); the CLI with the same content succeeded. This overrides the
|
||||
global CLAUDE.md note recommending the MCP tool — that guidance still holds for short questions.
|
||||
Run it backgrounded (`--sandbox read-only` means Codex cannot run anything, so paste every number
|
||||
into the prompt; `- <` feeds the prompt file on stdin):
|
||||
|
||||
```bash
|
||||
codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
||||
-c approval_policy='"never"' - < prompt.md > codex_out.txt 2>&1
|
||||
```
|
||||
|
||||
Write the prompt to a file. Include the files to read, your measurements **with numbers**, the
|
||||
proposed changes, and explicit instructions to challenge you. Ask direct questions ("is this
|
||||
really add-on-fixable?", "give the precise flag set") rather than "review this". Codex's sandbox
|
||||
often cannot run local commands and falls back to reading GitHub, so paste the evidence in rather
|
||||
than assuming it will find it.
|
||||
|
||||
**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.
|
||||
|
||||
### Attack your own plan too
|
||||
|
||||
Before implementing, spend a moment actively trying to break it:
|
||||
Levels 4-6 need a reason that survives being said out loud ("upstream has no knob for this, and I
|
||||
checked" is one; "it felt cleaner" is not) and mean full loop.
|
||||
|
||||
Attack your own plan before implementing:
|
||||
- What does this do on a host **unlike this one** — no GPU, small `/dev/shm`, aarch64, a VM?
|
||||
- What happens on **upgrade** to someone who configured this by hand?
|
||||
- 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.
|
||||
|
||||
That last question is the highest-yield one here; see [the recurring failure
|
||||
mode](#the-failure-mode-this-loop-keeps-producing).
|
||||
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
|
||||
invocation, then report back only Codex's objections and an assessment of each — not the raw
|
||||
transcript.
|
||||
|
||||
## 4. Implement
|
||||
|
||||
Read `references/traps.md` first; the bashio, s6-env, arch-guard and versioning traps are all
|
||||
live and each has shipped a bug.
|
||||
Touching a shell script, Dockerfile, or env option? Read `references/traps.md` first — skim the
|
||||
headings, read the sections you're about to touch; the bashio, s6-env, arch-guard and versioning
|
||||
traps are all live. (The light-path facts it holds — versioning format, CHANGELOG heading — are
|
||||
already inline in step 7.) Validate with `scripts/validate.sh <addon> --vs-master`. Write
|
||||
behavioural tests for anything with branches, targeting **the regression a reviewer described**,
|
||||
not just the happy path.
|
||||
|
||||
Validate with `scripts/validate.sh <addon> --vs-master`.
|
||||
## 5. Simplify
|
||||
|
||||
Write behavioural tests for anything with branches. Extract an embedded Python heredoc and drive
|
||||
it against fixtures with stubbed env vars; stub `bashio::*` and `df` to exercise shell paths. Test
|
||||
**the regression a reviewer described**, not just the happy path — a test that only covers the
|
||||
case you were already thinking about adds little.
|
||||
|
||||
## 5. Simplify — is this the simplest thing that works?
|
||||
|
||||
Do this once you have something working and before you ask anyone to review it. The question is
|
||||
not "is this good code" but **"what is the smallest change that makes the symptom go away, and
|
||||
why isn't that enough?"** If you cannot answer the second half, the smaller change is the answer.
|
||||
And restating the standing rule: obviously correct and slightly less efficient beats faster and
|
||||
harder to review — efficiency only buys complexity when a measurement shows it matters.
|
||||
|
||||
Checks worth running against your own diff:
|
||||
|
||||
- **Did the diff stay at the ladder level chosen in step 3?** If it crept up a level, either
|
||||
justify that out loud or redo it at the level you chose.
|
||||
- **Can this be solved by deleting instead of adding?** A flag that shouldn't be passed, a
|
||||
process that shouldn't start, a registration that shouldn't be duplicated. Removals cannot
|
||||
regress on hosts you can't test.
|
||||
- **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.
|
||||
- **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.
|
||||
|
||||
The evidence from this repo is blunt:
|
||||
|
||||
- A rejected PR spent a **388-line TCP proxy plus a 142-line monkeypatch of a private upstream
|
||||
method** to reclaim 159 MB — placing custom transport code in the path of every API request.
|
||||
Both independent reviewers said close it rather than iterate on it.
|
||||
- A ~180-line `ctypes` probe was written to decide whether to enable GPU flags. It worked
|
||||
perfectly, proved the driver was fine, and the change **still did nothing**, because the
|
||||
question it answered was not the question that mattered.
|
||||
- A resolution cap shipped as a **new init script writing an s6 envdir** — the wrong mechanism
|
||||
entirely. Renaming the option to the env var the service already reads (level 1) would have
|
||||
worked, and the new script did not.
|
||||
|
||||
In all three cases the simpler option existed and was skipped. Being able to build the
|
||||
complicated thing is not a reason to.
|
||||
Before requesting review, check: did the diff stay at the ladder level chosen in step 3? Can this
|
||||
be solved by deleting instead of adding? Is the fix bigger than what it fixes? How does it fail in
|
||||
three years? Case studies of what happens when this check is skipped: `references/simplify.md`.
|
||||
|
||||
## 6. Codex attacks the code (full loop only)
|
||||
|
||||
Same invocation, pointed at `git diff origin/master...HEAD` plus the reasoning behind each hunk.
|
||||
Ask specifically what breaks: upgrade paths, hosts unlike this one, users who configured things
|
||||
by hand. Ask it directly whether a simpler mechanism would achieve the same thing — an outside
|
||||
reader spots one-level-too-deep framing far more easily than the person who just built it.
|
||||
Same delegated invocation, pointed at `git diff origin/master...HEAD` plus your reasoning per
|
||||
hunk. Details in `references/codex-review.md`.
|
||||
|
||||
## 7. Open the PR
|
||||
|
||||
What CI actually gates on a PR: **`CHANGELOG.md` updated** (hard `exit 1`), the **HA add-on
|
||||
linter** (`frenck/action-addon-linter` in `onpr_check-pr.yaml`, no `continue-on-error` — a
|
||||
config.yaml schema error blocks the PR), and the **add-on image build**. The non-blocking lint
|
||||
is the *weekly Super-Linter*, not the PR checks — don't confuse the two. Nothing checks the
|
||||
version bump — but bump it
|
||||
anyway (`X.Y.Z.N`, never `X.Y.Z-N`, see `references/traps.md#versioning`), because Supervisor will
|
||||
not offer a rebuild without it, and update `README.md` if you added options. Match the existing
|
||||
CHANGELOG heading format, `## X.Y (DD-MM-YYYY)`.
|
||||
CI gates on a PR: **`CHANGELOG.md` updated** (hard fail), the **HA add-on linter**
|
||||
(`frenck/action-addon-linter`, blocking — not the weekly Super-Linter, which is non-blocking), and
|
||||
the **add-on image build**. Bump `version` anyway (`X.Y.Z.N`, never `X.Y.Z-N`, see
|
||||
`references/traps.md#versioning`) — Supervisor won't offer a rebuild without it. Update
|
||||
`README.md` if you added options; match the CHANGELOG heading format `## X.Y (DD-MM-YYYY)`.
|
||||
|
||||
Write the body to a file and use `gh pr create --body-file`. State what was measured, what
|
||||
changed, **what is not verified**, and how to roll back the riskiest hunk on its own.
|
||||
Write the body to a file, `gh pr create --body-file`: state what was measured, what changed,
|
||||
**what is not verified**, and how to roll back the riskiest hunk alone.
|
||||
|
||||
## 8. Resolve review comments
|
||||
|
||||
`scripts/pr_review.sh list|reply|resolve|status|watch <PR>`.
|
||||
|
||||
For every comment, **reproduce the claim before agreeing or disagreeing.** A CodeRabbit finding
|
||||
that `grep -E '^…$'` anchors per line — letting a multi-line value pass validation — was real and
|
||||
provable in one command. A finding that a changelog heading needed a blank line was a false
|
||||
positive against this repo's `.markdownlint.yaml`.
|
||||
|
||||
Reply with the evidence, then resolve the thread. **Push back when you are right**, on the thread,
|
||||
so the maintainer can overrule you — a resolved-but-wrong thread is worse than an open one.
|
||||
Equally, when a reviewer is right, fix the cause rather than papering over the symptom.
|
||||
`scripts/pr_review.sh list|reply|resolve|status|watch <PR>`. For every comment, **reproduce the
|
||||
claim before agreeing or disagreeing** — reviewers are frequently right and occasionally
|
||||
confidently wrong; a reproduction takes a minute and decides it either way. Reply with the
|
||||
evidence, then resolve. **Push back when you're right**, on the thread — a resolved-but-wrong
|
||||
thread is worse than an open one.
|
||||
|
||||
## 9. Verify before declaring done
|
||||
|
||||
Do not write "this should work". Either it was exercised, or say plainly that it wasn't.
|
||||
Never blur these three: **Verified** (you ran it and observed the result), **Checked** (parses,
|
||||
lints, type-checks), **Assumed** (reasoning only — name the assumption). Do not write "this should
|
||||
work" — either it was exercised, or say plainly it wasn't.
|
||||
|
||||
On the light path, verification is `validate.sh` plus CI — anything beyond that is **Assumed**,
|
||||
and the report must say so plainly.
|
||||
|
||||
Distinguish three states and never let them blur:
|
||||
|
||||
- **Verified** — you ran it and observed the result.
|
||||
- **Checked but not exercised** — it parses, lints, type-checks.
|
||||
- **Assumed** — reasoning only. Name the assumption.
|
||||
|
||||
**CI passing and the PR merging prove the build works, not that the change does anything.** Once
|
||||
the rebuilt add-on is running, re-run the measurement that motivated the work. Both changes in the
|
||||
session that produced this skill passed CI, merged, and were **inert**:
|
||||
|
||||
- The Xvfb resolution cap wrote its env file correctly and Xvfb still started at the base-image
|
||||
default — wrong env mechanism for that service.
|
||||
- The GPU flags reached Chromium's command line exactly as intended, and the GPU process still
|
||||
reported `--use-gl=disabled`, having overridden them after its own init failed.
|
||||
|
||||
Cheap post-deploy checks: `tr '\0' '\n' < /proc/<pid>/cmdline` for flags, `/proc/<pid>/environ`
|
||||
for env vars, `scripts/env_trace.sh <VAR> <process>` for the whole option plumbing, and a repeat
|
||||
CPU/PSS sample against the pre-change numbers.
|
||||
|
||||
Some fixes cannot be self-verified. A service reads its environment only at start, so an env-var
|
||||
fix is unproven until the add-on restarts — which needs the user, or `ha-cli` with their
|
||||
agreement. If you cannot restart, the change is **Assumed**, not Verified, and must be reported
|
||||
that way.
|
||||
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
|
||||
to do when a fix can't be self-verified: `references/evidence.md`.
|
||||
|
||||
## 10. Calibrate and report
|
||||
|
||||
Close against the scope from step 1, not against what you ended up doing. Structure:
|
||||
Close against the scope from step 1, not against what you ended up doing:
|
||||
|
||||
```
|
||||
What was asked / what shipped — mapped to the original scope
|
||||
@@ -319,46 +160,8 @@ Known broken / left out — explicitly, including anything descoped
|
||||
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, and it must not appear after the summary of what went well.
|
||||
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.
|
||||
|
||||
Give confidence per claim, not one blanket number, and make it mean something: "measured", "CI
|
||||
verified", "unverified — reasoning only". If a number came from one host, say so.
|
||||
|
||||
---
|
||||
|
||||
## The failure mode this loop keeps producing
|
||||
|
||||
Every bug shipped from the source session came from one move: **measuring this host correctly,
|
||||
then generalising it to all hosts.**
|
||||
|
||||
- `/dev/shm` was 7.7 GB here, so a flag looked useless — but Home Assistant ignores `shm_size`, so
|
||||
elsewhere it is Docker's 64 MB default and removing the flag reintroduces a crash loop.
|
||||
- An MCP entry was identified by its URL — but that URL is the documented default, so the rule
|
||||
would have deleted a user's hand-written configuration.
|
||||
- A GPU probe created a hardware context — but that proved the driver worked, not that Chromium's
|
||||
GPU path did.
|
||||
|
||||
The pattern is always *inference standing in for detection*. Before changing a default, ask what
|
||||
this is like on a host unlike yours. Prefer detecting the condition at runtime over asserting it.
|
||||
When ownership matters, **record it rather than infer it**.
|
||||
|
||||
## Token efficiency
|
||||
|
||||
`rtk` wraps commands via hook automatically. Compress large structured output you will re-read
|
||||
with `mcp__headroom__headroom_compress` (skip error/stack output). Use
|
||||
`mcp__tokensave__tokensave_context` for code exploration. Redirect big output to a file and read
|
||||
only what you need, and poll CI in a **background** task rather than blocking.
|
||||
|
||||
## Bundled files
|
||||
|
||||
| File | Use |
|
||||
|---|---|
|
||||
| `scripts/preflight.sh` | Tools, live-add-on check, revision-vs-running-image check. Exits 2 on mismatch |
|
||||
| `scripts/measure.sh` | RAM (PSS/private) + CPU snapshot; reserved vs resident. Sample ≥20 s |
|
||||
| `scripts/env_trace.sh` | Trace one env var through all four plumbing stages — for "my option did nothing" |
|
||||
| `scripts/validate.sh` | Local linters + CI gates; `--vs-master` shows only findings your diff added |
|
||||
| `scripts/pr_review.sh` | Fetch / reply to / resolve PR review threads; watch checks |
|
||||
| `references/traps.md` | Repo-specific traps — read before implementing |
|
||||
|
||||
Each script's header explains its reasoning; read the script when you use it.
|
||||
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.
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
# Codex review — invocation and prompt guidance
|
||||
|
||||
Used for step 3 (plan review) and step 6 (code review), full loop only. Codex is a genuinely
|
||||
different model reading the files itself; on this workload it has repeatedly been worth the
|
||||
minutes. Delegate the invocation to a subagent (see SKILL.md's subagent-delegation note) so its
|
||||
output doesn't land verbatim in your context — have the subagent return only Codex's objections
|
||||
and your assessment of each.
|
||||
|
||||
## Invocation
|
||||
|
||||
**Use the CLI, not the MCP tool, for prompts of this size.** The `codex` MCP tool timed out twice
|
||||
on ~4 KB prompts (2026-08-03); the CLI with the same content succeeded. The MCP tool is still fine
|
||||
for short questions.
|
||||
|
||||
`--sandbox read-only` lets Codex read files but blocks writes and command execution, and
|
||||
`approval_policy=never` means it will not be prompted for permission to run anything either — so
|
||||
paste every number into the prompt rather than expecting Codex to gather it. `- <` feeds the
|
||||
prompt file on stdin. Run it in the background so you are not blocked for the several minutes it
|
||||
takes (`&` here, or your harness's background-task mechanism):
|
||||
|
||||
```bash
|
||||
codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
||||
-c approval_policy='"never"' - < prompt.md > codex_out.txt 2>&1 &
|
||||
```
|
||||
|
||||
## Writing the prompt
|
||||
|
||||
- **Plan review (step 3):** include the files to read, your measurements **with numbers**, the
|
||||
proposed changes, and explicit instructions to challenge you. Ask direct questions ("is this
|
||||
really add-on-fixable?", "give the precise flag set") rather than "review this".
|
||||
- **Code review (step 6):** point it at `git diff origin/master...HEAD` plus the reasoning behind
|
||||
each hunk. Ask specifically what breaks: upgrade paths, hosts unlike this one, users who
|
||||
configured things by hand. Ask directly whether a simpler mechanism would achieve the same
|
||||
thing — an outside reader spots one-level-too-deep framing far more easily than the person who
|
||||
just built it.
|
||||
- Codex's sandbox often cannot run local commands and falls back to reading GitHub, so paste the
|
||||
evidence in rather than assuming it will find it.
|
||||
|
||||
**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.
|
||||
54
.claude/skills/hassio-addon-workflow/references/evidence.md
Normal file
54
.claude/skills/hassio-addon-workflow/references/evidence.md
Normal file
@@ -0,0 +1,54 @@
|
||||
# Evidence — measurement methodology and case studies
|
||||
|
||||
## Why summed RSS and reserved-vs-resident both matter
|
||||
|
||||
- **Summed RSS double-counts shared pages.** Removing a duplicate process frees its *private*
|
||||
memory, not its RSS. `scripts/measure.sh` reports PSS and private alongside RSS — quote
|
||||
**private** when arguing "removing this saves N MB".
|
||||
- **A big mapping is not necessarily resident.** Large SysV/tmpfs segments are lazily populated;
|
||||
reserved size is reported separately from resident for this reason.
|
||||
- `/proc/meminfo` and `free` show **host** figures (no memory cgroup namespace here) — never
|
||||
attribute those to the add-on.
|
||||
- Sample duration matters: a 3 s CPU sample measured 2.3% where a 20 s sample measured 21.6% for
|
||||
the same process. Use ≥20 s for anything you report.
|
||||
|
||||
## Before asserting anything, ask what would show it false
|
||||
|
||||
- "This process is duplicated" → is it? `ps -ef --forest`, compare parents and start times.
|
||||
- "This costs 500 MB" → is it resident? `grep Rss /proc/<pid>/smaps_rollup` — plain `smaps` prints
|
||||
one `Rss:` line per mapping (dozens of them), not a process total.
|
||||
- "This block never runs" → is its payload in the image? `command -v`, `apt` history.
|
||||
- "The flag isn't set" → `tr '\0' '\n' < /proc/<pid>/cmdline`.
|
||||
|
||||
When you correct yourself mid-analysis, keep the correction visible in your notes and in what you
|
||||
report — a retracted claim that stays retracted is worth more than one quietly dropped.
|
||||
|
||||
## The failure mode this loop keeps producing
|
||||
|
||||
Every bug shipped from the source session came from one move: **measuring this host correctly,
|
||||
then generalising it to all hosts.**
|
||||
|
||||
- `/dev/shm` was 7.7 GB here, so a flag looked useless — but Home Assistant ignores `shm_size`, so
|
||||
elsewhere it is Docker's 64 MB default and removing the flag reintroduces a crash loop.
|
||||
- An MCP entry was identified by its URL — but that URL is the documented default, so the rule
|
||||
would have deleted a user's hand-written configuration.
|
||||
- A GPU probe created a hardware context — but that proved the driver worked, not that Chromium's
|
||||
GPU path did.
|
||||
|
||||
The pattern is always *inference standing in for detection*. Before changing a default, ask what
|
||||
this is like on a host unlike yours. Prefer detecting the condition at runtime over asserting it.
|
||||
When ownership matters, **record it rather than infer it**.
|
||||
|
||||
## "Merged and inert" — CI passing proves the build works, not that the change does anything
|
||||
|
||||
Both changes in the session that produced this skill passed CI, merged, and were **inert**:
|
||||
|
||||
- The Xvfb resolution cap wrote its env file correctly and Xvfb still started at the base-image
|
||||
default — wrong env mechanism for that service.
|
||||
- The GPU flags reached Chromium's command line exactly as intended, and the GPU process still
|
||||
reported `--use-gl=disabled`, having overridden them after its own init failed.
|
||||
|
||||
Once the rebuilt add-on is running, re-run the measurement that motivated the work. Some fixes
|
||||
cannot be self-verified — a service that reads its environment only at start makes an env-var fix
|
||||
unproven until the add-on restarts, which needs the user or `ha-cli` with their agreement. If you
|
||||
cannot restart, the change is **Assumed**, not Verified, and must be reported that way.
|
||||
30
.claude/skills/hassio-addon-workflow/references/simplify.md
Normal file
30
.claude/skills/hassio-addon-workflow/references/simplify.md
Normal file
@@ -0,0 +1,30 @@
|
||||
# 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.
|
||||
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
|
||||
method** to reclaim 159 MB — placing custom transport code in the path of every API request.
|
||||
Both independent reviewers said close it rather than iterate on it.
|
||||
- A ~180-line `ctypes` probe was written to decide whether to enable GPU flags. It worked
|
||||
perfectly, proved the driver was fine, and the change **still did nothing**, because the
|
||||
question it answered was not the question that mattered.
|
||||
- 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.
|
||||
|
||||
## Checks worth running against your own diff
|
||||
|
||||
- **Did the diff stay at the ladder level chosen in step 3?** If it crept up a level, either
|
||||
justify that out loud or redo it at the level you chose.
|
||||
- **Can this be solved by deleting instead of adding?** A flag that shouldn't be passed, a
|
||||
process that shouldn't start, a registration that shouldn't be duplicated. Deleting usually
|
||||
shrinks the regression surface — but not always: the `/dev/shm` case in
|
||||
`references/evidence.md` is a removal that reintroduced a crash loop on hosts unlike this one.
|
||||
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.
|
||||
- **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.
|
||||
@@ -180,12 +180,18 @@ cannot be determined, because the crash it prevents is worse than its overhead.
|
||||
|
||||
## CI and review bots
|
||||
|
||||
**What CI actually gates** — checked against the workflows, because assuming costs a cycle:
|
||||
**What CI actually gates** — checked against the workflows, because assuming costs a cycle.
|
||||
|
||||
- **CHANGELOG updated** — the only hard gate (`onpr_check-pr.yaml`, its single `exit 1`).
|
||||
- **Add-on image build** — real, and slow; one run took ~3 h.
|
||||
- **Lint** — `lint.yml` runs super-linter with `continue-on-error: true` at both call sites, so
|
||||
it *cannot* fail a PR. Fix real findings anyway, but do not treat lint as a blocker.
|
||||
All three hard gates below are matrixed over `check-addon-changes.outputs.changedAddons` and
|
||||
`if:`-skipped when it is `[]`, so a PR that touches no add-on directory (docs, `.github/`,
|
||||
`.claude/`) shows them as *skipping*, not passing — do not read that as a green build.
|
||||
|
||||
- **CHANGELOG updated** — hard gate (`onpr_check-pr.yaml` exits 1 without it).
|
||||
- **HA add-on linter** — hard gate: `frenck/action-addon-linter` in `onpr_check-pr.yaml` has no
|
||||
`continue-on-error`, so a config.yaml schema error fails the PR.
|
||||
- **Add-on image build** — hard gate, and slow; one run took ~3 h.
|
||||
- **Weekly super-linter** — `lint.yml` runs with `continue-on-error: true` at both call sites, so
|
||||
it *cannot* fail a PR. Fix real findings anyway, but do not treat it as a blocker.
|
||||
- **Version bump** — no workflow checks it. It is repo convention, and required for Supervisor to
|
||||
offer the rebuild, but it will not fail CI.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user