mirror of
https://github.com/alexbelgium/hassio-addons.git
synced 2026-09-14 05:39:09 +02:00
docs(skill): ask "is this the simplest way possible" before implementing (#3062)
* docs(skill): ask "is this the simplest way possible" before implementing Every question the loop asks — of me and of Codex — is about what could go wrong, and each answer argues for more code. Nothing pushed the other way until the maintainer asked, and by then the code existed. On PR #3061 that cost a 309-line Python registry client to read one OCI label, in a script whose neighbouring branch does the same kind of job in inline curl and jq. The plan review and the code review both passed it. Asked outright, Codex said to delete all 309 lines; the inline version came to 21 and returned byte-identical output on every image tested. It is the second time the question has had to come from the maintainer. Step 3 now asks it before any code exists, of myself in writing and of Codex as a question of its own with the smaller alternative sketched out to argue for. Step 5 gains an Idiom check, on whether a hunk looks like the code it sits beside. Adding a new file or a new mechanism now means the full loop however small the diff looks, and codex-review.md records that Codex will never raise proportionality unprompted but answers it bluntly when asked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(skill): give the proportionality self-question a home on the light path Three independent reviewers on #3062 (CodeRabbit, Codex, Copilot) all caught the same gap: the self-question and its written answer lived only inside step 3, marked full-loop-only, while the light path went straight from scope to implement. A light task following the documented sequence never reached the instruction telling it to ask "is this the simplest way?" at all. Moved the self-question (and the written answer it requires) into the shared triage section, before the light/full-loop split, so both paths ask it before implementing. Step 3 now only adds Codex to that question, for full loop. Also: clarified that a version bump plus its CHANGELOG entry is release bookkeeping, not a "new file", so it does not trigger the light-to-full-loop escalation on its own (Codex's second finding on #3062, which read the one-file threshold as counting every file the PR touches rather than whether a new mechanism was added). Fixed step 5's stale "six questions" to seven, after Idiom was added in the prior commit. Fixed simplify.md's case study to say the 309-line client was "initially proposed", not shipped, matching the prior PR's own body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(skill): split "test the smaller candidate" by whether the diff exists yet CodeRabbit on #3062 caught that step 3's "build the smaller one and run it" never says to compare it against the larger candidate, only to run it. As written it applied at plan time, before either candidate has code — where the larger candidate is a plan, not a diff, so there is nothing to compare against without building both in full, which defeats the point of picking the cheaper one first. Testing the smaller sketch against the actual requirement is the correct and complete check there. The comparison CodeRabbit is describing is real, but it happens at step 6, not step 3: by the time code review runs, the larger candidate already exists as the diff, so sketching the smaller one and diffing its output against the shipped code costs nothing extra and is exactly what settled #3061. Moved the build-and-compare instruction there instead of applying it, unchanged, to a step where nothing exists to compare against yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -33,8 +33,20 @@ Triage first, then one of two paths:
|
||||
the plan → implement → simplify → Codex reviews the code → **simplify again** → PR → resolve
|
||||
comments → verify in production → report.
|
||||
|
||||
Escalate mid-flight if a light task grows — touches a default, needs a new script or service, or
|
||||
reveals a deeper problem.
|
||||
Before implementing, on **either** path, ask yourself the proportionality question and answer
|
||||
it in the plan (a one-line plan, for a light task) — in writing, naming the smaller version you
|
||||
rejected and why:
|
||||
|
||||
> What is the smallest thing that solves this? What would you delete from what I just proposed?
|
||||
> If it had to be written in ten lines, in the idiom of the code it sits beside, what would be
|
||||
> lost — and is any of that a case I have actually demonstrated?
|
||||
|
||||
Escalate mid-flight if a light task grows — touches a default, needs a new script or service,
|
||||
reveals a deeper problem, or turns out to need more than the one file once you have honestly
|
||||
answered that question. A light task never skips it; it skips **Codex** only while the answer
|
||||
stays inside one file. Adding a **new file or a new mechanism** puts it on the full loop, however
|
||||
small the diff looks — a version bump and its CHANGELOG entry are release bookkeeping, not a new
|
||||
file for this purpose, and never trigger it on their own.
|
||||
|
||||
**Standing rule:** ship the simplest solution that works, and build it out of what already
|
||||
exists — a `.templates/` module, an existing cont-init script, the pattern a sibling add-on
|
||||
@@ -113,6 +125,16 @@ plan:
|
||||
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.
|
||||
|
||||
**Full loop adds Codex to the proportionality question already asked above** — put it to Codex as
|
||||
a numbered question of its own, with the smaller alternative sketched out for it to argue for.
|
||||
Every other question in the loop asks what could go wrong, which only ever argues for more code;
|
||||
this is the one place that pushes the other way, and it is worthless unless asked outright — see
|
||||
`references/codex-review.md`. Nothing is built yet at this step, so "test" means against the real
|
||||
requirement, not against the other candidate — sketch the smaller one and check it against what
|
||||
the add-on actually needs; that sketch is what you go on to implement if it holds up, not one of
|
||||
two things you build. A sketch costs minutes and settles the argument with a measurement instead
|
||||
of a preference.
|
||||
|
||||
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?
|
||||
@@ -123,7 +145,8 @@ Attack your own plan before implementing:
|
||||
where that input arrives (the standing rule above) and go and look, before you write it.
|
||||
|
||||
Full loop only, before writing code: get Codex's independent read on the plan, delegated as above —
|
||||
`references/codex-review.md` has the invocation and how to write the prompt.
|
||||
`references/codex-review.md` has the invocation and how to write the prompt. The proportionality
|
||||
question above goes in that prompt every time, and in the step 6 prompt too.
|
||||
|
||||
## 4. Implement
|
||||
|
||||
@@ -145,7 +168,7 @@ path.
|
||||
|
||||
## 5. Simplify
|
||||
|
||||
Six questions over your own diff, before anyone else reads it:
|
||||
Seven questions over your own diff, before anyone else reads it:
|
||||
|
||||
- **Level** — did the diff stay at the ladder level chosen in step 3, or creep up one?
|
||||
- **Deletion** — can this be solved by deleting instead of adding?
|
||||
@@ -153,6 +176,10 @@ Six questions over your own diff, before anyone else reads it:
|
||||
- **Reuse** — does any hunk reimplement what `.templates/`, another script in this add-on, or a
|
||||
sibling add-on already does? If a future add-on hits this problem, will it find one way to solve
|
||||
it or two? Fold near-duplicates in, or justify the divergence in the PR body.
|
||||
- **Idiom** — does this look like the code it sits beside? The same kind of job done two ways in
|
||||
one file costs every later reader more than the nicer of the two ways saves. Reuse asks whether
|
||||
the mechanism already exists; Idiom asks whether you wrote yours the way the neighbours are
|
||||
written.
|
||||
- **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.
|
||||
@@ -177,6 +204,12 @@ you have now demonstrated, or one you have merely been told about? Taking a
|
||||
correctness objection often deletes the code that made it necessary, and a fix that collapses back
|
||||
to fewer lines than you started the review with is the normal outcome, not a suspicious one.
|
||||
|
||||
Then ask the proportionality question again, of the diff this time — it is cheap, and by now
|
||||
there is real code to point at rather than a plan. This is where "build the smaller one and test
|
||||
both" is actually free: the larger candidate already exists as the diff, so sketch the smaller
|
||||
alternative and diff its output against the shipped code on the same inputs, the way #3061 was
|
||||
settled (`references/simplify.md`) — rather than against the requirement alone.
|
||||
|
||||
These edits land after step 4's checks already ran, so re-run them: `scripts/validate.sh <addon>
|
||||
--vs-master` plus the behavioural tests, over the final diff. Deleting a branch is exactly the
|
||||
kind of edit that leaves a stray `fi` behind.
|
||||
|
||||
@@ -27,6 +27,10 @@ codex exec --model gpt-5.6-sol --sandbox read-only --skip-git-repo-check \
|
||||
- **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".
|
||||
- **The proportionality question, in both reviews, as a numbered question of its own:** "is this
|
||||
the simplest way possible, and what exactly would you delete?" Sketch the smaller alternative in
|
||||
the prompt and ask it to argue for that, and quote the repo's standing simplicity rule so it has
|
||||
the bar to hold you to. Ask for lines and functions to delete, not a direction.
|
||||
- **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
|
||||
@@ -41,3 +45,8 @@ confirmations with the same scepticism as its objections — especially about th
|
||||
|
||||
**Its objections only ever argue for more code** — it is asked what could go wrong, never whether
|
||||
the branch it wants is reachable. Sort them before writing anything; SKILL.md step 6 is that pass.
|
||||
|
||||
**Unprompted it will never say "this is too much".** Asked outright it will, bluntly and usefully:
|
||||
on PR #3061 it answered "delete all 309 lines" of a helper both its earlier reviews had passed
|
||||
without comment. Expect it to reverse an earlier position when you ask it to re-examine one — a
|
||||
stance it drops that easily was never strongly held, which is itself the answer.
|
||||
|
||||
@@ -30,6 +30,17 @@ 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
|
||||
now requires.
|
||||
|
||||
- The immich updater fix (issue #3060, PR #3061) initially proposed a **309-line Python registry
|
||||
client** to read one OCI label, into a script whose neighbouring `dockerhub` branch does the same kind of
|
||||
job in a handful of inline `curl | jq` lines. Both the plan review and the code review passed
|
||||
it, because both were asked what could break rather than what it should cost. Asked "is this the
|
||||
simplest way possible?", Codex answered "delete all 309 lines"; the inline version came to 21
|
||||
and was measured to return byte-identical output on all five images. About a third of what went
|
||||
was defending cases never observed in this repo — an architecture mismatch, a hostile label, an
|
||||
unsupported registry — each of which already failed closed on its own. Second time the
|
||||
maintainer has had to ask the question, which is why step 3 now asks it before any code exists
|
||||
and step 5 asks whether the hunk looks like its neighbours.
|
||||
|
||||
## Where SKILL.md step 5's questions get hard
|
||||
|
||||
**Deletion is not automatically the safe direction.** The `/dev/shm` case in `evidence.md` is a
|
||||
|
||||
Reference in New Issue
Block a user