Files
Alexandre d60f17685e skill(hassio-addon-workflow): fix the stage-3 trap's actual mechanism (#3058)
* skill(hassio-addon-workflow): fix the stage-3 trap's actual mechanism

traps.md attributed the shipped svc-xorg failure to that service's own shebang
("starts #!/usr/bin/env bashio, not with-contenv"). Verified against
.templates/ha_entrypoint.sh: that isn't what decides it. ha_entrypoint.sh
rewrites the first line of every cont-init.d script and every services.d/s6-rc.d
run script to one shebang chosen by probing candidate_shebangs in order. The
first candidate is /command/with-contenv bashio; in add-ons that override the
base ENTRYPOINT ["/init"] (confirmed live in ente, wger, free_games_claimer),
s6 stage 1 never runs, that candidate fails, and the probe falls through to
/usr/bin/env bashio for every service in the image, regardless of what shebang
it shipped with. So no service reads stage 3 in that case, not just svc-xorg.

Surfaced independently by two agents in an unrelated eval run for PR #3057 (both
named the rewrite; neither had the conditionality right), then verified against
the script and confirmed the ENTRYPOINT-override path is real rather than
hypothetical before writing this.

No add-on touched; references/ only. markdownlint clean.

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

* skill(hassio-addon-workflow): scope the stage-3 trap to the PID-1-entrypoint path

Flagged by both Codex and Copilot, reproduced against .templates/ha_entrypoint.sh
before accepting.

The previous commit's fix still overclaimed: it said ha_entrypoint.sh rewrites
every service's shebang, universally. It doesn't. The services.d/s6-rc.d rewrite
loop is gated on `if $PID1` (line 430) — it only runs when ha_entrypoint.sh
itself is the container's PID 1, i.e. when the add-on replaces the base
ENTRYPOINT ["/init"]. Under the normal /init path (the overwhelming majority of
add-ons), ha_entrypoint.sh runs as the stage-2 hook with PID1=false, never
touches service run files, s6's own stage 1 already created
container_environment before any cont-init script ran, and a service's shipped
with-contenv shebang reads it normally — the opposite of what the previous
wording implied. cont-init.d scripts are unconditional either way
(run_one_script has no $PID1 gate), so a cont-init script's own shebang was
never informative and that half stands.

Also fixed: the previous commit named wger as a live example of the
ENTRYPOINT override. Copilot caught that wger's override is written into its
Dockerfile but commented out — confirmed (`grep '^#ENTRYPOINT' wger/Dockerfile`).
Only ente and free_games_claimer currently have it active; wger dropped from
the list.

markdownlint clean; no add-on touched.

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

---------

Co-authored-by: BirdNET-Go Addon Builder <addon-builder@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-11 08:05:29 +02:00

307 lines
19 KiB
Markdown

# Repo-specific traps
Things that look correct and are not. Each cost real time or shipped broken. Read this before
implementing; skim the headings, read the ones you're about to touch.
The repo's own `CLAUDE.md` documents structure, Dockerfile conventions, `updater.json`, CI
workflows and lint rules — that is not repeated here.
## Contents
- [Environment and workspace](#environment-and-workspace)
- [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)
- [Chromium / Electron under Xvfb](#chromium--electron-under-xvfb)
- [CI and review bots](#ci-and-review-bots)
---
## Environment and workspace
**The checkout is probably on the wrong branch.** Checkouts under `/data/claude` are shared and
persistent; another session leaves them wherever it finished. A stale branch looks entirely
normal. Compare the add-on's `config.yaml` `version` against the running `$BUILD_VERSION` before
trusting anything you read. `scripts/preflight.sh` does this.
**Never run `git stash` under `/data/claude`.** `refs/stash` is shared across every worktree and
concurrent session, so it is *not* isolated even in your own worktree. A bare `stash` / `stash
pop` pair in a clean worktree once restored another session's stash, producing conflict markers
in six untouched files. To compare a file against another revision use
`git show <rev>:<path> > /tmp/x`. If a pop does go wrong: a conflicted pop **keeps** the stash
entry, so nothing is lost — confirm `git rev-parse HEAD` matches what you pushed, then
`git reset --hard HEAD`.
**Work in a worktree under `/data`, not `/tmp`**`/tmp` is `noexec`, so scripts there won't run.
```bash
git worktree add --detach /data/claude/.work/<task> origin/master
```
**You cannot test the Docker build.** dockerd does not start in this environment. CI is the only
gate. One observed run took ~3 hours, with 20+ runs queued against 2 executing — that was account
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.
**`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>`.
## Passing values into base-image services
The plumbing has four stages. `scripts/env_trace.sh <VAR> <process>` walks all four and tells
you which one drops the value — use it rather than reasoning about this from memory.
1. `/data/options.json` — the user's saved options.
2. **Injected export block**`.templates/00-global_var.sh` writes a literal
`export <option>='<value>'` block into *every* service `run` script, using the option name
**verbatim**. So `max_resolution` *is* injected; it just isn't a name any service reads.
`MAX_RES` would be both injected and read.
3. `container_environment` — s6's envdir, read **only** by services whose shebang is
`#!/usr/bin/with-contenv`.
4. The running process — the only stage that decides behaviour.
**Name the option exactly as the env var the service reads** (uppercase), the way `DRINODE`,
`KEYBOARD` and `TZ` already do. Verified live: `DRINODE` appears as `export DRINODE=…` in all 16
service run scripts including `svc-xorg`, and Xvfb runs with `-vfbdevice /dev/dri/renderD128`.
**Two consequences that are easy to get wrong:**
- `00-global_var.sh` is cont-init **00**. Any cont-init script numbered higher runs *after* the
injection, so it cannot change what a service will see through stage 2.
- **Whether a service's own shebang still decides stage 3 depends on whether `ha_entrypoint.sh`
runs as PID 1** — i.e. whether the add-on replaces the base `ENTRYPOINT ["/init"]` with one that
makes `ha_entrypoint.sh` itself the container's entrypoint (live today in `ente` and
`free_games_claimer`; `wger` has the override written into its Dockerfile but commented out, so
it is not currently one of these — check the Dockerfile, not this list). Under the normal
`/init` path this script runs as the stage-2 hook, `$PID1` is false, and it never touches
`services.d/*/run` or `s6-overlay/s6-rc.d/*/run` at all (`.templates/ha_entrypoint.sh:430`,
gated on `if $PID1`) — s6's own stage 1 already created `/run/s6/container_environment` before
any cont-init script ran, so a service's shipped `with-contenv` shebang reads it normally.
Only the `ENTRYPOINT`-override path breaks this, and it breaks it twice over: s6 stage 1 never
runs, so nothing ever creates the envdir; and because `ha_entrypoint.sh` is now PID 1, it
rewrites the first line of every service `run` file to whichever shebang its own
`candidate_shebangs` probe landed on. That probe's first candidate,
`/command/with-contenv bashio`, fails precisely because the envdir was never created, so it
falls through to `/usr/bin/env bashio` for every service — the real mechanism behind the shipped
`svc-xorg` failure (its envdir file was written 6 seconds before Xvfb started, and Xvfb still
came up at the base-image default). `cont-init.d` scripts are a separate case: `run_one_script`
rewrites their shebang unconditionally, with no `$PID1` gate, so a cont-init script's own
shebang is never informative either way. `ha_entrypoint.sh` dumps the environment itself to
compensate for the missing envdir, and the envdir writes in `00-global_var.sh` /
`01-config_yaml.sh` are `if [ -d ]` guarded, so they take effect only once something has created
that directory.
**Renaming an option to match a base-image env var moves validation out of your script and into
the schema.** `00-global_var.sh` exports empty strings (only objects/arrays/nulls are dropped),
and base-image scripts typically test `${VAR+x}`*set*-ness, not emptiness. So an empty
`MAX_RES` becomes `Xvfb -screen 0 "x24"` and the X server does not start. If you make this move,
constrain the value in `config.yaml` (`match(^[0-9]{1,5}x[0-9]{1,5}$)?`) in the same commit, or
keep a guard script.
**Open question, unresolved:** what Supervisor does with a stored `options.json` key that no
longer exists in the new schema — error, warn, or silently drop. This decides whether renaming an
option is safe on upgrade. The `monica` add-on shipped exactly such a rename
(`MEILISEARCH_KEY``meilisearch_key`) with no migration, which is weak evidence it is
tolerated. The base image has an `init-migrations` oneshot reading `/migrations` if a migration
is needed. Confirm before renaming a shipped option.
Whichever mechanism you use, verify the service actually received it:
```bash
tr '\0' '\n' < /proc/<pid>/environ | grep <VAR>
```
**`cont-init.d` runs as root before s6 services start** — that part is true and is the right
place for filesystem and permission setup.
**Anything needing an X display must not run in `cont-init.d`** — Xvfb isn't up yet. Put it in
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
list under errexit — bashio's internals return non-zero and process substitution inherits the
failure. Capture with `$(...)` first, then feed a here-string.
**Scripts shared by symlink**: `80-configuration.sh` and friends are shared with the webtop
add-ons. Put add-on-specific logic in a new numbered script instead of editing them.
**`grep -E '^…$'` anchors per line.** A multi-line config value passes validation on its first
line and is then used verbatim. Use bash's `[[ =~ ]]`, which anchors the whole string.
## Dockerfile and architecture
**Prefer `BUILD_ARCH` over `TARGETARCH`** — the repo's builder passes `BUILD_ARCH` explicitly,
while `TARGETARCH` is BuildKit-provided and may or may not be populated.
Either way the variable must be declared with `ARG <NAME>` **in the build stage that uses it**;
without that it expands empty, the guard never matches, and the block silently does nothing —
which is the same dead-`if` failure described just below, and the usual cause of it.
**Verify a guarded block actually ran** rather than assuming. Check whether its payload exists in
the running image (`command -v <tool>`), and cross-check `/var/log/apt/history.log` for the
matching `apt-get install` line. An `if` block whose condition never matched leaves no trace and
no error — one such block sat dead for weeks while appearing to guarantee driver verification.
**Adding a build stage above `ARG BUILD_FROM` breaks the final `FROM`.** Global build args must
be declared *before the first* `FROM` in the file; an `ARG` that follows one belongs to that stage
only. Inserting a tools stage at the top of an add-on Dockerfile therefore demotes the
`ARG BUILD_FROM` below it, and the final `FROM ${BUILD_FROM}` expands empty:
`failed to solve: base name (${BUILD_FROM}) should not be blank`. Move `ARG BUILD_FROM` (and
`ARG BUILD_VERSION`) above the new stage. `netalertx` does not hit this only because it hardcodes
its base image instead of using `${BUILD_FROM}` — do not copy its ordering blindly
(PR #3024).
**A base image can lose its package manager between upstream releases.** Zoraxy v3.3.4 added
`/sbin/apk` to the upstream cleanup step, so `ha_automodules.sh` failed with
`apt-get: not found / apk: not found` (exit 127) on both architectures. The removal deleted only
the binary — `/etc/apk` (repositories, keys, world) and `/lib/apk/db` survived, confirmed by a
single `sbin/.wh.apk` whiteout in the layer — so copying `apk.static` from an
`apk-tools-static` build stage restores package management in one line. Diff the upstream image
configs across the two tags (`.history[].created_by` from the registry config blob) before
theorising; it names the changed step exactly. Note `build_from` is often a floating `:latest`
tag, so the builder's revert-on-failure does **not** restore a working build — the next rebuild
fails identically until the Dockerfile is fixed (PR #3024).
**Don't test for distro-specific filenames.** A guard on
`/usr/share/vulkan/icd.d/intel_icd.x86_64.json` named a file Debian does not ship (it installs
`intel_icd.json`), so fixing the arch variable alone would have turned dead code into a failing
build.
## Versioning
`CLAUDE.md` owns the format (`X.Y.Z.N`, never `X.Y.Z-N`, and why). The one thing it does not say:
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.
## Chromium / Electron under Xvfb
**Xvfb offers only indirect/software GLX**, so Chromium probes it, fails, and falls back to CPU
rendering — the GPU process runs `--use-gl=disabled` and the renderer `--disable-gpu-compositing`.
**Passing ANGLE flags is not sufficient.** `--ozone-platform=x11 --use-gl=angle
--use-angle=gl-egl` reached Chromium's command line exactly as intended and the GPU process
*still* reported `--use-gl=disabled`, having overridden the flag after its own init failed.
**A standalone ANGLE probe proves less than it appears to.** Loading Claude Desktop's bundled
`libEGL.so`, initializing the OpenGL backend and reading back
`ANGLE (Intel, Mesa Intel(R) Graphics (ADL-N), OpenGL 4.6)` proves the driver and device work —
not that Chromium's GPU process, sandbox, dmabuf import and X11 presentation path work. Codex
flagged this distinction during review and was right.
`--use-angle=gles-egl` is rejected outright by Mesa ("Intel or NVIDIA OpenGL ES drivers are not
supported").
**`--disable-dev-shm-usage`** is a workaround for Docker's 64 MB default `/dev/shm`. Home
Assistant **ignores** the add-on's `shm_size`, so the real size varies per install — it was 7.7 GB
on one host. Detect at runtime rather than assuming either way; keep the flag when the size
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.
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.
**"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.
**Reviewers**: CodeRabbit (deepest — often runs scripts to prove a claim; reviews ~9 minutes
after the PR opens, or on `@coderabbitai review`), chatgpt-codex-connector, Copilot, Codacy.
**Codacy is red on essentially every add-on PR and gates nothing.** `gh pr checks` reports it as
`fail` (older runs showed `action_required`); #3019, #3044 and #3050 all merged with it failing,
and `master` carries no branch protection, so no check is required in the GitHub sense. It exposes
no annotations via the API, so its findings are only visible in the maintainer's Codacy account.
Note it and move on rather than guessing. `pr_review.sh watch` therefore prints it every poll but
keeps it out of the verdict — the one check on that list, which is a denylist of known noise, not
an allowlist of gates, so a job added to CI later counts as blocking until someone exempts it.
**Resolving a review thread requires GraphQL** (`resolveReviewThread`); the REST API cannot do it.
`scripts/pr_review.sh` wraps fetch / reply / resolve.
**`gh pr checks` output is TAB-separated, and every blocking gate here has spaces in its name.**
Parsing it with awk's default field splitting truncates each check to its first word and reads the
wrong column as the state: `Codacy Static Code Analysis<TAB>fail` becomes `Codacy=Static`, and
`Test addon build (wger)<TAB>pending` becomes `Test=addon`. A `case` over that string then matches
neither `*fail*` nor `*pending*` and falls through to the "all passing" branch — the failure mode
that makes a CI-reporting command lie. `pr_review.sh watch` called #3044 green while Codacy was
red, and on #3042 printed "settled — all passing" while the HA add-on linter was failing; it would
also have called a build that had not started a pass. Use `awk -F'\t'`, judge the state column
alone (never the joined `name=state` text, or a check named `flaky-fail-detector` reads as a
failure), and treat an unrecognised state as a failure instead of letting it reach the passing
branch. Fixed in #3052.
The TSV is gh's *non-TTY* renderer, which is what `$(gh pr checks ... | awk)` always gets; attached
to a terminal the same command prints a coloured, aligned table with a summary line, so never
sanity-check the format by eye in a shell and assume the script sees that. `gh pr checks --json`
would be sturdier, and Copilot recommends it (#3052), but it does not exist before gh 2.36 and the
add-on ships 2.23 — it fails with `unknown flag: --json`. The parse is therefore built to fail
safe instead: states are allowlisted, so a header row would land in the failure branch and a
space-aligned table would parse to zero rows and keep `watch` waiting. Either way it cannot
return a false pass.
**CHANGELOG heading dates are ISO, whatever the bots' defaults say.** Match the format already in
the add-on's file. Repo-wide that is `## <version> (YYYY-MM-DD)`: 7705 dated headings against 363
in `DD-MM-YYYY`, and the newest entry is ISO in 125 of 135 add-ons. Copilot flags an ISO file that
gets a `DD-MM-YYYY` entry (#3019). `DD-MM-YYYY` is not invented — it is what `onpush_builder.yaml`
writes with `date '+%d-%m-%Y'` when it has to insert a heading you forgot, and what the
addons_updater bot writes when its `date_iso8601` option is off (`99-run.sh`; it is on in
production here) — but neither is a reason to write it yourself. The builder's duplicate check is
`grep -q "^## ${version} ("`, keyed on the exact `config.yaml` version and blind to the date, so
an ISO heading you wrote yourself still suppresses the bot's insertion.
**The repo's `.markdownlint.yaml` does not disable MD022/MD032**, so a CHANGELOG will show
dozens of pre-existing heading/list findings. They are noise because lint is `continue-on-error`,
not because the config exempts them — don't cite the config as a reason to ignore a finding.
**Separate new lint findings from pre-existing ones** by linting the same file at `origin/master`
and diffing the result sets — otherwise you chase warnings that were already there.
`scripts/validate.sh --vs-master` does this.