mirror of
https://github.com/alexbelgium/hassio-addons.git
synced 2026-09-14 13:49:09 +02:00
skill(hassio-addon-workflow): deduplicate, route traps.md by section, fix preflight path (#3057)
* skill(hassio-addon-workflow): deduplicate, route traps.md by section, fix preflight path Reviewed the skill against Anthropic's skill-authoring guidance (progressive disclosure, single ownership of each rule, explain the why rather than issue rigid rules) with a second, independent pass from Codex (gpt-5.6-sol). Real defect fixed: preflight.sh defaulted its repo argument to the hardcoded /data/claude/hassio-addons, so when run from a worktree it inspected the main checkout and reported that branch — the exact stale-checkout trap the script exists to catch. It now defaults to `git rev-parse --show-toplevel`. Deduplicated, one owner each: - measurement methodology: evidence.md owns it; traps.md's Measurement section is gone (its rtk note moved to Environment and workspace) - defensive-branch reachability: the standing rule owns it; step 3 points back - subagent delegation: SKILL.md's delegation note owns it; codex-review.md points - review-ratchet handling: SKILL.md step 6 owns it, absorbing codex-review.md's "this is wrong" vs "this is undefended" split - post-merge revert mechanics: traps.md owns them; step 9 states the check only - simplify checklist: step 5 owns the questions; simplify.md keeps the cases - version format: CLAUDE.md owns it; traps.md keeps only what it does not say Corrections found by review and verified against the workflows and scripts: - Super-Linter runs on every PR (lint.yml `pull_request`), not weekly; it is continue-on-error at both call sites, which is the part that matters - the three hard gates are matrixed over changed add-ons, so they skip rather than pass on a PR touching no top-level config.* - `--sandbox read-only` blocks writes, not reads or command execution; Codex can and does fall back to fetching the repo from GitHub - env_trace.sh's process argument is optional; pr_review.sh's reply and resolve take more than a PR number Also: step 4 now routes to the traps.md section by anchor instead of asking for the whole ~290-line file, and step 5 gains a Depth check (fix the shared mechanism once more than one add-on hits it) adapted from the built-in /simplify skill's altitude pass. No new markdownlint findings; shellcheck clean; preflight.sh verified reporting this worktree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * skill(hassio-addon-workflow): make step 4's traps.md routing actually save the tokens The routing table added in the previous commit was advisory only: a markdown anchor cannot be loaded on its own, so an agent told to read references/traps.md#shell-and-bashio reads the file and pays for all 18 KB. Measured overpay per edit, against what each section actually needs: shell / bashio 642 B needed, 18,070 B paid (28x) Dockerfile / arch 2,655 B needed, 18,070 B paid (7x) base-image env option 3,070 B needed, 18,070 B paid (6x) "CI and review bots" is 6,302 B of that — 35% of the file — and is needed at steps 7-8, never at step 4. scripts/traps.sh prints one section. Scripts are run rather than read, so the helper itself costs no prompt tokens and the table now buys what it claims. Kept traps.md whole rather than splitting it into six files: the split would save the same bytes but fragments a file with a working table of contents and breaks the references/traps.md#anchor cross-references that steps 5, 7 and 9 still use. Ambiguous or unmatched keywords list the sections and exit non-zero rather than silently printing the wrong one. Verified: every keyword in the step 4 table resolves to exactly one section, headings containing '/' work, and the last section reaches EOF. shellcheck clean; no new markdownlint findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * skill(hassio-addon-workflow): fix review findings — PATH form, preflight fallback, config.json claim All three reproduced before accepting. Step 4's routing table used a bare `traps.sh <keyword>`, which is not on PATH: every row failed with "traps.sh: command not found" from the repo root. Flagged independently by Codex, Copilot and CodeRabbit. Now uses the `bash "$SKILL/scripts/traps.sh"` form already established above it; all five keywords verified to run and resolve to exactly one section. preflight.sh fell back to the hardcoded /data/claude/hassio-addons when `git rev-parse` could not answer, recreating the stale-checkout bug this commit series set out to fix — and worse than CodeRabbit described: from a non-git directory it reported an unrelated branch of the main checkout as though it were the caller's own. It now refuses and asks for an explicit path (exit 1). Explicit-argument and in-worktree behaviour unchanged, both re-verified. Codex correctly noted the description advertised config.json while preflight.sh and validate.sh parse only config.yaml. Exactly one add-on uses config.json (zzz_archived_tor, archived) against 136 using config.yaml, so the proportionate fix is Codex's own alternative — narrow the description — rather than teaching two scripts JSON for an archived add-on. config.json dropped from the description. shellcheck clean; no new markdownlint findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: BirdNET-Go Addon Builder <addon-builder@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -4,29 +4,30 @@ description: >-
|
|||||||
Workflow for alexbelgium/hassio-addons Home Assistant add-on work: diagnose with real
|
Workflow for alexbelgium/hassio-addons Home Assistant add-on work: diagnose with real
|
||||||
measurements, independent Codex review, implement, open a PR, resolve CodeRabbit / Copilot /
|
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
|
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,
|
repo — bugs, RAM/CPU/performance tuning, Dockerfile, config.yaml, build.json,
|
||||||
version bumps, opening or iterating PRs — and when asked to "check with codex", "verify with
|
updater.json, cont-init.d, services.d or s6 changes, version and CHANGELOG bumps, a failing
|
||||||
chatgpt", or resolve bot comments. Cheap for small asks: a light path skips the heavy steps.
|
add-on CI check, opening or iterating PRs — and, on an add-on task, 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
|
# Home Assistant add-on workflow
|
||||||
|
|
||||||
**Answer style.** Chat replies are terse: no pleasantries, no tool-call narration, no decorative
|
**Answer style.** Chat replies are terse — no pleasantries, tool-call narration, decorative tables,
|
||||||
tables or emoji, no dumped logs — quote the shortest decisive line, and don't re-read or re-print
|
emoji or dumped logs; quote the shortest decisive line and don't re-print what is already in
|
||||||
what is already in context. Fragments and dropped articles are fine. Never compressed: uncertainty
|
context. Telegraphic fragments are fine *there*. Two things outrank brevity, because dropping a
|
||||||
markers ("likely", "assumed", "not verified"), negations (`not`/`never`/`no`/`only`), numbers,
|
word from either changes the meaning rather than shortening it: never compress uncertainty markers
|
||||||
units, technical terms, code blocks, error strings — step 9's Verified/Checked/Assumed distinction
|
("likely", "assumed", "not verified"), negations, numbers, units, technical terms, code or error
|
||||||
outranks brevity every time. Write in full prose, not fragments, for security warnings,
|
strings — step 9's Verified/Checked/Assumed distinction wins every time; and write full prose
|
||||||
irreversible-action confirmations, and any multi-step sequence a fragment could make ambiguous.
|
wherever a fragment could be read two ways — security warnings, irreversible-action confirmations,
|
||||||
Persisted text is prose too: commits, CHANGELOG entries, PR bodies, review-thread replies, the
|
multi-step sequences, and everything persisted (commits, CHANGELOG entries, PR bodies, review-thread
|
||||||
step 10 report.
|
replies, the step 10 report).
|
||||||
|
|
||||||
Triage first, then one of two paths:
|
Triage first, then one of two paths:
|
||||||
|
|
||||||
- **Light** — typo/doc fixes, CHANGELOG edits, version bumps, one-file edits at ladder levels
|
- **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
|
1-3 (below), simple questions: scope → implement → validate (step 4) → PR (version bump +
|
||||||
<addon> --vs-master`; `$SKILL` defined below) → PR (version bump + CHANGELOG still required) →
|
CHANGELOG still required) → resolve bot comments.
|
||||||
resolve bot comments.
|
|
||||||
- **Full loop** — performance/RAM/CPU work, diagnosis, anything changing a shipped default,
|
- **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
|
ladder levels 4-6, or an explicit Codex-check request: scope → measure → plan → Codex reviews
|
||||||
the plan → implement → simplify → Codex reviews the code → **simplify again** → PR → resolve
|
the plan → implement → simplify → Codex reviews the code → **simplify again** → PR → resolve
|
||||||
@@ -47,9 +48,8 @@ reasoning about a hypothetical *host* either. A defensive branch is complexity l
|
|||||||
the input that reaches it and the image or host where that happens, or delete it and let the case
|
the input that reaches it and the image or host where that happens, or delete it and let the case
|
||||||
fail visibly instead.
|
fail visibly instead.
|
||||||
|
|
||||||
**Repo layout.** `alexbelgium/hassio-addons`; each add-on is a top-level directory. This skill is
|
**Skill root.** The canonical copy lives at `.claude/skills/hassio-addon-workflow/`. Set it once;
|
||||||
checked in at `.claude/skills/hassio-addon-workflow/` (canonical copy). Set the skill root once,
|
every `scripts/…` and `references/…` path below is relative to it:
|
||||||
then every `scripts/…` and `references/…` path below is relative to it:
|
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
SKILL="$(git rev-parse --show-toplevel)/.claude/skills/hassio-addon-workflow"
|
SKILL="$(git rev-parse --show-toplevel)/.claude/skills/hassio-addon-workflow"
|
||||||
@@ -63,9 +63,10 @@ bash "$SKILL/scripts/preflight.sh" # and likewise for the other scripts
|
|||||||
|
|
||||||
**Delegate heavy output to a subagent.** Codex reviews and multi-thread PR triage produce output
|
**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
|
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
|
review (step 6), and PR-comment listing when there are more than ~5 threads (step 8): if subagents
|
||||||
subagent to run the command and report back only the objections/findings and your assessment of
|
are available, launch one to run the command and report back only the objections/findings and your
|
||||||
each, not the raw transcript.
|
assessment of each, not the raw transcript. Otherwise redirect the output to a file and read the
|
||||||
|
parts you need.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -85,9 +86,11 @@ Measure the running add-on rather than reasoning from source (`$BUILD_VERSION` s
|
|||||||
- Is this flag/driver/package actually present? → inspect the artifact: `/proc/<pid>/cmdline`,
|
- Is this flag/driver/package actually present? → inspect the artifact: `/proc/<pid>/cmdline`,
|
||||||
`command -v`, `/var/log/apt/history.log`
|
`command -v`, `/var/log/apt/history.log`
|
||||||
|
|
||||||
Verify you're reading the right revision first — `scripts/preflight.sh` catches a stale branch
|
Verify you're reading the right revision first — `scripts/preflight.sh` compares the checkout
|
||||||
before it costs a full analysis pass. Measurement methodology, gotchas, and real failure examples:
|
against the running `$BUILD_VERSION`, which catches the usual stale branch before it costs a full
|
||||||
`references/evidence.md`.
|
analysis pass (a version match does not prove the source is identical). Read
|
||||||
|
`references/evidence.md` before interpreting any number you did not get straight from
|
||||||
|
`measure.sh`, and for the failure modes this step exists to prevent.
|
||||||
|
|
||||||
## 3. Plan — choose the mechanism level, then Codex reviews it (full loop)
|
## 3. Plan — choose the mechanism level, then Codex reviews it (full loop)
|
||||||
|
|
||||||
@@ -117,45 +120,60 @@ Attack your own plan before implementing:
|
|||||||
- What am I **inferring** that I could instead **detect at runtime** or **record explicitly**?
|
- 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.
|
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
|
- 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 —
|
where that input arrives (the standing rule above) and go and look, before you write it.
|
||||||
`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
|
Full loop only, before writing code: get Codex's independent read on the plan, delegated as above —
|
||||||
whose prompt includes the path `references/codex-review.md` and tells it to follow that file's
|
`references/codex-review.md` has the invocation and how to write the prompt.
|
||||||
invocation, then report back only Codex's objections and an assessment of each — not the raw
|
|
||||||
transcript.
|
|
||||||
|
|
||||||
## 4. Implement
|
## 4. Implement
|
||||||
|
|
||||||
Touching a shell script, Dockerfile, or env option? Read `references/traps.md` first — skim the
|
Read the `references/traps.md` section matching what you're about to touch. It is ~18 KB and all
|
||||||
headings, read the sections you're about to touch; the bashio, s6-env, arch-guard and versioning
|
but one section is irrelevant to any given edit, so print the one you need rather than reading the
|
||||||
traps are all live. (The light-path facts it holds — versioning format, CHANGELOG heading — are
|
file — run it with no argument to list the sections:
|
||||||
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**,
|
| Touching | Run |
|
||||||
not just the happy path.
|
| --- | --- |
|
||||||
|
| an option or anything a base-image service reads | `bash "$SKILL/scripts/traps.sh" passing` |
|
||||||
|
| a file the app also writes itself | `bash "$SKILL/scripts/traps.sh" "app's own"` |
|
||||||
|
| shell, bashio, a symlinked script | `bash "$SKILL/scripts/traps.sh" bashio` |
|
||||||
|
| `Dockerfile`, `build.json`, an arch guard | `bash "$SKILL/scripts/traps.sh" dockerfile` |
|
||||||
|
| Chromium, Electron, Xvfb | `bash "$SKILL/scripts/traps.sh" chromium` |
|
||||||
|
|
||||||
|
Then validate with `scripts/validate.sh <addon> --vs-master`, and write behavioural tests for
|
||||||
|
anything with branches, targeting **the regression a reviewer described**, not just the happy
|
||||||
|
path.
|
||||||
|
|
||||||
## 5. Simplify
|
## 5. Simplify
|
||||||
|
|
||||||
Before requesting review, check: did the diff stay at the ladder level chosen in step 3? Can this
|
Six questions over your own diff, before anyone else reads it:
|
||||||
be solved by deleting instead of adding? Is the fix bigger than what it fixes? How does it fail in
|
|
||||||
three years? And on reuse: does any hunk reimplement something `.templates/`, another script in
|
- **Level** — did the diff stay at the ladder level chosen in step 3, or creep up one?
|
||||||
this add-on, or a sibling add-on already does — and if a future add-on hits this same problem,
|
- **Deletion** — can this be solved by deleting instead of adding?
|
||||||
will it find one way to solve it or two? Fold a near-duplicate into the existing mechanism, or
|
- **Size** — is the fix bigger than the thing it fixes?
|
||||||
justify the divergence in the PR body — but never at the cost of an isolation rule
|
- **Reuse** — does any hunk reimplement what `.templates/`, another script in this add-on, or a
|
||||||
`references/traps.md` documents: scripts shared by symlink with the webtop add-ons take a new
|
sibling add-on already does? If a future add-on hits this problem, will it find one way to solve
|
||||||
numbered script, not an edit. Case studies of what happens when this check is skipped:
|
it or two? Fold near-duplicates in, or justify the divergence in the PR body.
|
||||||
`references/simplify.md`.
|
- **Depth** — is this a special case bolted onto shared infrastructure? Fix the shared mechanism
|
||||||
|
instead once more than one add-on hits it; generalising from a single case is how bespoke
|
||||||
|
designs get built, so below that bar the special case is the right call.
|
||||||
|
- **Longevity** — how does this fail in three years, when the base image or upstream has moved?
|
||||||
|
|
||||||
|
The standing exception to Reuse and Depth: scripts shared by symlink with the webtop add-ons take a
|
||||||
|
new numbered script, never an edit (`references/traps.md#shell-and-bashio`). Case studies for the rest, including
|
||||||
|
what shipped when this pass was skipped: `references/simplify.md`.
|
||||||
|
|
||||||
## 6. Codex attacks the code, then simplify what the review added (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
|
Same delegated invocation, pointed at `git diff origin/master...HEAD` plus your reasoning per
|
||||||
hunk. Details in `references/codex-review.md`.
|
hunk. Details in `references/codex-review.md`.
|
||||||
|
|
||||||
Then run step 5's checks again over the hunks the review changed. Adversarial review only ever
|
Then run step 5's checks again over the hunks the review changed. Adversarial review mostly argues
|
||||||
argues *for* another branch — that is its job — so accepting objections ratchets the diff upward,
|
*for* another branch — that is what it is asked to do — so accepting objections tends to ratchet
|
||||||
and nothing else in the loop walks it back down. For each accepted objection: is the case it
|
the diff upward, and nothing else in the loop walks it back down. Sort each objection before you
|
||||||
defends one you have now demonstrated, or one you have merely been told about? Taking a
|
write anything:
|
||||||
|
**"this is wrong"** is a bug and you fix it; **"this is undefended"** is a claim about some host,
|
||||||
|
and it needs the same demonstration you would demand of a measurement — 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
|
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.
|
to fewer lines than you started the review with is the normal outcome, not a suspicious one.
|
||||||
|
|
||||||
@@ -165,20 +183,23 @@ kind of edit that leaves a stray `fi` behind.
|
|||||||
|
|
||||||
## 7. Open the PR
|
## 7. Open the PR
|
||||||
|
|
||||||
CI gates on a PR: **`CHANGELOG.md` updated** (hard fail), the **HA add-on linter**
|
Three hard gates — **`CHANGELOG.md` updated**, the **HA add-on linter**
|
||||||
(`frenck/action-addon-linter`, blocking — not the weekly Super-Linter, which is non-blocking), and
|
(`frenck/action-addon-linter`), and the **add-on image build** — but only on a PR that changes a
|
||||||
the **add-on image build**. Bump `version` anyway (`X.Y.Z.N`, never `X.Y.Z-N`, see
|
top-level `config.*`. On a PR that doesn't (docs, `.github/`, `.claude/`) they *skip*, which is not
|
||||||
`references/traps.md#versioning`) — Supervisor won't offer a rebuild without it. Update
|
the same as passing. Super-Linter runs on every PR and is `continue-on-error`, so it never blocks;
|
||||||
`README.md` if you added options; write the CHANGELOG heading as `## <version> (<date>)`,
|
fix its real findings anyway. Nothing checks the version bump, so bump it yourself — Supervisor
|
||||||
matching the date format already in that file — almost always ISO `YYYY-MM-DD`, see
|
won't offer a rebuild without one, and `CLAUDE.md` has the format. Update `README.md` if you added
|
||||||
`references/traps.md#ci-and-review-bots`.
|
options; write the CHANGELOG heading as `## <version> (<date>)`, matching the date format already
|
||||||
|
in that file — almost always ISO `YYYY-MM-DD`, see `references/traps.md#ci-and-review-bots`.
|
||||||
|
|
||||||
Write the body to a file, `gh pr create --body-file`: state what was measured, what changed,
|
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.
|
**what is not verified**, and how to roll back the riskiest hunk alone.
|
||||||
|
|
||||||
## 8. Resolve review comments
|
## 8. Resolve review comments
|
||||||
|
|
||||||
`scripts/pr_review.sh list|reply|resolve|status|watch <PR>`. For every comment, **reproduce the
|
`scripts/pr_review.sh list|status|watch <PR>` to read, `reply <PR> <COMMENT_ID> <text|@file>` and
|
||||||
|
`resolve <PR> <THREAD_ID…|--all>` to answer; run it with no arguments for the full usage. For every
|
||||||
|
comment, **reproduce the
|
||||||
claim before agreeing or disagreeing** — reviewers are frequently right and occasionally
|
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
|
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
|
evidence, then resolve. **Push back when you're right**, on the thread — a resolved-but-wrong
|
||||||
@@ -192,13 +213,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
|
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
|
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. After merge, `git fetch origin master`
|
motivated the work once the rebuilt add-on is running. Real "merged and inert" examples, and what
|
||||||
(the tracking ref is stale otherwise), then confirm the *changes* survived — `git diff
|
to do when a fix cannot be self-verified: `references/evidence.md`. Then confirm the change
|
||||||
origin/master -- <the paths you touched>` comes back empty. Ancestry is not the check: a revert
|
survived the merge: `git fetch origin master` first (the tracking ref is stale otherwise), then
|
||||||
leaves your commit in history and undoes its tree, so `--contains` reports success either way. The
|
`git diff origin/master -- <the paths you touched>` must come back empty. Ancestry is not the
|
||||||
builder's revert-on-failure job can revert a merge for reasons unrelated to your diff (see
|
check, and the builder reverts merges for reasons unrelated to your diff — both explained in
|
||||||
`references/traps.md#ci-and-review-bots`). Real "merged and inert" examples, and what
|
`references/traps.md#ci-and-review-bots`.
|
||||||
to do when a fix can't be self-verified: `references/evidence.md`.
|
|
||||||
|
|
||||||
## 10. Calibrate and report
|
## 10. Calibrate and report
|
||||||
|
|
||||||
|
|||||||
@@ -2,9 +2,7 @@
|
|||||||
|
|
||||||
Used for step 3 (plan review) and step 6 (code review), full loop only. Codex is a genuinely
|
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
|
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
|
minutes. Run it through a subagent, per SKILL.md's delegation note.
|
||||||
output doesn't land verbatim in your context — have the subagent return only Codex's objections
|
|
||||||
and your assessment of each.
|
|
||||||
|
|
||||||
## Invocation
|
## Invocation
|
||||||
|
|
||||||
@@ -12,11 +10,12 @@ and your assessment of each.
|
|||||||
on ~4 KB prompts (2026-08-03); the CLI with the same content succeeded. The MCP tool is still fine
|
on ~4 KB prompts (2026-08-03); the CLI with the same content succeeded. The MCP tool is still fine
|
||||||
for short questions.
|
for short questions.
|
||||||
|
|
||||||
`--sandbox read-only` lets Codex read files but blocks writes and command execution, and
|
`--sandbox read-only` blocks writes, not reads, and `approval_policy=never` stops it asking for
|
||||||
`approval_policy=never` means it will not be prompted for permission to run anything either — so
|
permission rather than stopping it acting — so it can still run read-only commands and often
|
||||||
paste every number into the prompt rather than expecting Codex to gather it. `- <` feeds the
|
falls back to fetching the repo from GitHub instead of reading your worktree. Paste every number
|
||||||
prompt file on stdin. Run it in the background so you are not blocked for the several minutes it
|
into the prompt rather than expecting it to gather them, and treat what it reports about *local*
|
||||||
takes (`&` here, or your harness's background-task mechanism):
|
state as unverified. `- <` 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
|
```bash
|
||||||
codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
||||||
@@ -40,9 +39,5 @@ codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
|||||||
confidently, and separately caught a genuine methodology error in the same review. Treat its
|
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.
|
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
|
**Its objections only ever argue for more code** — it is asked what could go wrong, never whether
|
||||||
go wrong, so its output is a list of arguments for more code; it is never asked whether the branch
|
the branch it wants is reachable. Sort them before writing anything; SKILL.md step 6 is that pass.
|
||||||
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.
|
|
||||||
|
|||||||
@@ -1,16 +1,24 @@
|
|||||||
# Evidence — measurement methodology and case studies
|
# Evidence — measurement methodology and case studies
|
||||||
|
|
||||||
## Why summed RSS and reserved-vs-resident both matter
|
## How to read the numbers
|
||||||
|
|
||||||
- **Summed RSS double-counts shared pages.** Removing a duplicate process frees its *private*
|
**Summed RSS overstates savings.** Shared library pages are counted once per process, so removing
|
||||||
memory, not its RSS. `scripts/measure.sh` reports PSS and private alongside RSS — quote
|
a duplicate frees its *private* memory, not its RSS. Measured example: four MCP shims summed to
|
||||||
**private** when arguing "removing this saves N MB".
|
882 MB RSS but 643 MB PSS / 564 MB private, and per-process private ranged 54 MB down to 2 MB —
|
||||||
- **A big mapping is not necessarily resident.** Large SysV/tmpfs segments are lazily populated;
|
which completely changes which duplicate is worth removing. Quote private when arguing "removing
|
||||||
reserved size is reported separately from resident for this reason.
|
this saves N MB".
|
||||||
- `/proc/meminfo` and `free` show **host** figures (no memory cgroup namespace here) — never
|
|
||||||
attribute those to the add-on.
|
**A large mapping is often not resident.** SysV/tmpfs segments are lazily populated. Xvfb's
|
||||||
- Sample duration matters: a 3 s CPU sample measured 2.3% where a 20 s sample measured 21.6% for
|
506 MB framebuffer shows `Rss: 0` in `/proc/<pid>/smaps`. Check before calling anything a leak.
|
||||||
the same process. Use ≥20 s for anything you report.
|
|
||||||
|
**`/proc/meminfo` and `free` show host figures** — there is no memory cgroup namespace here.
|
||||||
|
Never attribute those totals to the add-on.
|
||||||
|
|
||||||
|
**A short CPU sample is not a CPU measurement.** A 3 s sample measured 2.3% where a 20 s sample
|
||||||
|
measured 21.6% for the same process. Use >= 20 s for anything you report.
|
||||||
|
|
||||||
|
`scripts/measure.sh` already reports PSS and private alongside RSS, and resident separately from
|
||||||
|
reserved, so these three only bite when you compute a figure yourself or quote one from `ps`.
|
||||||
|
|
||||||
## Before asserting anything, ask what would show it false
|
## Before asserting anything, ask what would show it false
|
||||||
|
|
||||||
|
|||||||
@@ -30,28 +30,24 @@ Being able to build the complicated thing is not a reason to.
|
|||||||
It took the maintainer asking "is this the simplest way possible?" to run the pass that step 6
|
It took the maintainer asking "is this the simplest way possible?" to run the pass that step 6
|
||||||
now requires.
|
now requires.
|
||||||
|
|
||||||
## Checks worth running against your own diff
|
## Where SKILL.md step 5's questions get hard
|
||||||
|
|
||||||
- **Did the diff stay at the ladder level chosen in step 3?** If it crept up a level, either
|
**Deletion is not automatically the safe direction.** The `/dev/shm` case in `evidence.md` is a
|
||||||
justify that out loud or redo it at the level you chose.
|
removal that reintroduced a crash loop on hosts unlike this one. A removal that depends on a host
|
||||||
- **Can this be solved by deleting instead of adding?** A flag that shouldn't be passed, a
|
default needs the same verification as an addition.
|
||||||
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
|
**Naming the case is the bar for a defensive branch, not reproducing it.** Docker's 64 MB
|
||||||
`references/evidence.md` is a removal that reintroduced a crash loop on hosts unlike this one.
|
`/dev/shm` default is documented behaviour that Home Assistant does not override, so that guard
|
||||||
A removal that depends on a host default still needs the same verification as an addition.
|
stays even though this host measured 7.7 GB. Nobody could name a single image shipping
|
||||||
- **Is the fix bigger than the thing it fixes?** That is a smell, not a rule — but it usually
|
`with-contenv` without `s6-dumpenv`, so that fallback went. If you cannot name the case, delete the
|
||||||
means the problem was framed one level too deep.
|
branch: the situation then fails the way it already fails today, visibly, instead of through a
|
||||||
- **For each defensive branch: what input reaches it, on which image or host?** Go and check,
|
second path that is never exercised and silently rots as the base images move. Weigh the cost both
|
||||||
the way you would check a measurement. The bar is being able to **name** the case, not to
|
ways — a one-flag guard against a crash you cannot rule out is cheap; a second code path that
|
||||||
reproduce it here: Docker's 64 MB `/dev/shm` default is documented behaviour that HA does not
|
degrades to the pre-fix behaviour anyway is not. Write in the PR body what you cut and why, so the
|
||||||
override, so the bullet above keeps that guard even though this host measured 7.7 GB. Nobody
|
next person does not re-add it from the same reasoning.
|
||||||
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
|
**"Bigger than the thing it fixes" is a smell, not a rule** — but it usually means the problem was
|
||||||
fails today, visibly, instead of through a second path that is never exercised and silently
|
framed one level too deep.
|
||||||
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.
|
**Three-year failure** favours code that reads a documented knob. Code that reaches into private
|
||||||
Write down in the PR body what you cut and why, so the next person does not re-add it from the
|
internals does not survive the base image moving.
|
||||||
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.
|
|
||||||
|
|||||||
@@ -9,7 +9,6 @@ workflows and lint rules — that is not repeated here.
|
|||||||
## Contents
|
## Contents
|
||||||
|
|
||||||
- [Environment and workspace](#environment-and-workspace)
|
- [Environment and workspace](#environment-and-workspace)
|
||||||
- [Measurement](#measurement)
|
|
||||||
- [Passing values into base-image services](#passing-values-into-base-image-services)
|
- [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)
|
- [Writing into an app's own config](#writing-into-an-apps-own-config)
|
||||||
- [Shell and bashio](#shell-and-bashio)
|
- [Shell and bashio](#shell-and-bashio)
|
||||||
@@ -46,20 +45,6 @@ gate. One observed run took ~3 hours, with 20+ runs queued against 2 executing
|
|||||||
runner contention, not the diff. Check `gh run list` before concluding your PR is stuck. Poll in
|
runner contention, not the diff. Check `gh run list` before concluding your PR is stuck. Poll in
|
||||||
a background task, and never claim the build is verified when it hasn't run.
|
a background task, and never claim the build is verified when it hasn't run.
|
||||||
|
|
||||||
## Measurement
|
|
||||||
|
|
||||||
**Summed RSS overstates savings.** Shared library pages are counted once per process, so removing
|
|
||||||
a duplicate frees its *private* memory, not its RSS. Measured example: four MCP shims summed to
|
|
||||||
882 MB RSS but 643 MB PSS / 564 MB private, and per-process private ranged 54 MB down to 2 MB —
|
|
||||||
which completely changes which duplicate is worth removing. Quote private when arguing "removing
|
|
||||||
this saves N MB".
|
|
||||||
|
|
||||||
**A large mapping is often not resident.** SysV/tmpfs segments are lazily populated. Xvfb's
|
|
||||||
506 MB framebuffer shows `Rss: 0` in `/proc/<pid>/smaps`. Check before calling anything a leak.
|
|
||||||
|
|
||||||
**`/proc/meminfo` and `free` show host figures** — there is no memory cgroup namespace here.
|
|
||||||
Never attribute those totals to the add-on.
|
|
||||||
|
|
||||||
**`rtk` filters some command output.** For a complete listing, redirect to a file and read that
|
**`rtk` filters some command output.** For a complete listing, redirect to a file and read that
|
||||||
(`ps ... > $SP/ps.txt`), or use `rtk proxy <cmd>`.
|
(`ps ... > $SP/ps.txt`), or use `rtk proxy <cmd>`.
|
||||||
|
|
||||||
@@ -190,11 +175,9 @@ build.
|
|||||||
|
|
||||||
## Versioning
|
## Versioning
|
||||||
|
|
||||||
**`X.Y.Z.N`, never `X.Y.Z-N`.** A hyphen parses as a semver pre-release, which Supervisor treats
|
`CLAUDE.md` owns the format (`X.Y.Z.N`, never `X.Y.Z-N`, and why). The one thing it does not say:
|
||||||
as *older* than `X.Y.Z` — the update is never offered.
|
date-based versions (`2026.08.03`) are common here, so check whether master has already moved to
|
||||||
|
the version you were about to use before you pick it.
|
||||||
Date-based versions (`2026.08.03`) are common here. Check whether master has already moved to the
|
|
||||||
version you were about to use.
|
|
||||||
|
|
||||||
## Chromium / Electron under Xvfb
|
## Chromium / Electron under Xvfb
|
||||||
|
|
||||||
|
|||||||
@@ -5,7 +5,14 @@
|
|||||||
# Usage: preflight.sh [repo-path] [addon-slug]
|
# Usage: preflight.sh [repo-path] [addon-slug]
|
||||||
set -uo pipefail
|
set -uo pipefail
|
||||||
|
|
||||||
REPO="${1:-/data/claude/hassio-addons}"
|
# Default to the checkout this is run from, never a fixed path: a fixed path silently inspected
|
||||||
|
# the main checkout while the caller worked in a worktree, reporting a branch nobody was editing —
|
||||||
|
# the exact stale-checkout trap this script exists to catch. Falling back to one when git cannot
|
||||||
|
# answer would recreate it, so refuse instead and make the caller say which repo they mean.
|
||||||
|
REPO="${1:-}"
|
||||||
|
if [ -z "$REPO" ]; then
|
||||||
|
REPO=$(git rev-parse --show-toplevel 2> /dev/null) || REPO=""
|
||||||
|
fi
|
||||||
SLUG="${2:-}"
|
SLUG="${2:-}"
|
||||||
|
|
||||||
echo "== tools =="
|
echo "== tools =="
|
||||||
@@ -26,9 +33,14 @@ fi
|
|||||||
echo
|
echo
|
||||||
echo "== repo =="
|
echo "== repo =="
|
||||||
# git-aware check: in a worktree .git is a file, not a directory
|
# git-aware check: in a worktree .git is a file, not a directory
|
||||||
|
if [ -z "$REPO" ]; then
|
||||||
|
echo " not inside a git checkout, and no repo path given"
|
||||||
|
echo " -> pass one explicitly: preflight.sh <repo-path> [addon-slug]"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
if ! git -C "$REPO" rev-parse --git-dir > /dev/null 2>&1; then
|
if ! git -C "$REPO" rev-parse --git-dir > /dev/null 2>&1; then
|
||||||
echo " no git repo at $REPO"
|
echo " no git repo at $REPO"
|
||||||
exit 0
|
exit 1
|
||||||
fi
|
fi
|
||||||
cd "$REPO" || exit 0
|
cd "$REPO" || exit 0
|
||||||
branch=$(git branch --show-current 2> /dev/null || echo "(detached)")
|
branch=$(git branch --show-current 2> /dev/null || echo "(detached)")
|
||||||
|
|||||||
49
.claude/skills/hassio-addon-workflow/scripts/traps.sh
Executable file
49
.claude/skills/hassio-addon-workflow/scripts/traps.sh
Executable file
@@ -0,0 +1,49 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
# Print one section of references/traps.md.
|
||||||
|
#
|
||||||
|
# traps.md is ~18 KB and every section but one is irrelevant to any given edit: a shell fix needs
|
||||||
|
# 642 bytes of it, a Dockerfile fix 2.6 KB, and "CI and review bots" — 35% of the file — is needed
|
||||||
|
# at steps 7-8 and never at step 4. A markdown anchor cannot be loaded on its own, so reading the
|
||||||
|
# file to reach one section pays for all of them. This prints just the section, so the routing
|
||||||
|
# table in SKILL.md step 4 costs what it claims to.
|
||||||
|
#
|
||||||
|
# Usage: traps.sh <keyword> # substring of a section heading, case-insensitive
|
||||||
|
# traps.sh # list the sections
|
||||||
|
set -uo pipefail
|
||||||
|
|
||||||
|
TRAPS="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/references/traps.md"
|
||||||
|
[ -f "$TRAPS" ] || { echo "not found: $TRAPS" >&2; exit 1; }
|
||||||
|
|
||||||
|
# The Contents list duplicates the headings; grep the headings themselves so the list cannot drift.
|
||||||
|
list() {
|
||||||
|
echo "sections (pass any substring):"
|
||||||
|
grep '^## ' "$TRAPS" | grep -v '^## Contents' | sed 's/^## / /'
|
||||||
|
}
|
||||||
|
|
||||||
|
[ $# -eq 0 ] && { list; exit 0; }
|
||||||
|
|
||||||
|
# awk over exact heading text: section names contain '/' and other characters that would need
|
||||||
|
# escaping in a sed address, and a keyword matching several headings should be an error, not a
|
||||||
|
# silent pick of the first.
|
||||||
|
mapfile -t matches < <(grep '^## ' "$TRAPS" | grep -v '^## Contents' |
|
||||||
|
grep -iF -- "$1" | sed 's/^## //')
|
||||||
|
|
||||||
|
case "${#matches[@]}" in
|
||||||
|
0)
|
||||||
|
echo "no section matching '$1'" >&2
|
||||||
|
list >&2
|
||||||
|
exit 1
|
||||||
|
;;
|
||||||
|
1) ;;
|
||||||
|
*)
|
||||||
|
echo "'$1' matches ${#matches[@]} sections — be more specific:" >&2
|
||||||
|
printf ' %s\n' "${matches[@]}" >&2
|
||||||
|
exit 1
|
||||||
|
;;
|
||||||
|
esac
|
||||||
|
|
||||||
|
awk -v want="## ${matches[0]}" '
|
||||||
|
$0 == want { inside = 1; print; next }
|
||||||
|
inside && /^## / { exit }
|
||||||
|
inside { print }
|
||||||
|
' "$TRAPS"
|
||||||
Reference in New Issue
Block a user