Skip to content

Commit 37e2479

Browse files
committed
docs: codify code-review process + recurring-pattern checklist in CLAUDE.md
Two additions to project CLAUDE.md so future sessions surface the same opportunities consistently: 1. A "Code Review & Improvement Process" section documenting the four-angle parallel-agent survey (quality/debt/coverage/ops), the recurring patterns that actually show up in this codebase (magic numbers belonging in defaultSettings, backoff reinvented per file, labelSnapshot-style threading, async-parse state-loss races, GHA floating tags, secrets in run: commands, version-sync drift), and the multi-item execution recipe (one commit per item, risky last, version bump separately, source-repo release backfill). 2. A note in the existing release section that version-sync is now CI- enforced (so you can't silently drift package.json and config.yaml apart anymore), and that the source-repo GitHub Release needs to be backfilled manually since hacs-distribution.yml only releases on the distribution repo.
1 parent 125f8c1 commit 37e2479

1 file changed

Lines changed: 45 additions & 1 deletion

File tree

CLAUDE.md

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ When bumping the version (e.g., for a bug fix or feature release), you MUST:
3636

3737
If you skip step 3, the distribution repo will NOT be updated, Home Assistant will not see the new version, and the add-on will not auto-update on user devices. This has caused stale deployments in the past.
3838

39+
CI now enforces step 1: the `version-sync` job in `.github/workflows/ci.yml` fails the build if `package.json` and `homeassistant-addon/config.yaml` disagree. Do not edit one without the other.
40+
41+
After tagging, backfill a GitHub Release on the source repo (`gh release create vX.Y.Z --notes "..."`) so the source-repo release page stays in lockstep with the distribution. The `hacs-distribution.yml` workflow only creates a Release on the **distribution** repo, not the source.
42+
3943
### Home Assistant Add-on config.yaml Rules
4044

4145
When modifying `homeassistant-addon/config.yaml`, follow these rules to prevent upgrade failures:
@@ -131,4 +135,44 @@ Settings are loaded from `settings.js` with fallback to defaults in `index.js`.
131135
- C-Gate error code parsing and logging
132136
- MQTT connection resilience with LWT
133137
- Settings validation on startup
134-
- Graceful degradation when services are unavailable
138+
- Graceful degradation when services are unavailable
139+
140+
## Code Review & Improvement Process
141+
142+
When the user asks to "review the codebase", "look for improvement opportunities", "what should we work on next", or anything similarly open-ended, do a **parallel-agent survey** rather than a sequential scan. The codebase is large enough (40+ test files, 30+ src modules) that a single-pass review either misses things or blows the context window.
143+
144+
### How to run a codebase review
145+
146+
Dispatch four Explore agents in parallel covering these angles:
147+
148+
1. **Code quality and refactor candidates**: largest/most complex files, oversized methods (>80 lines or >3 nesting levels), duplicated patterns across files, magic numbers/timeouts not pulled from `defaultSettings.js`, error-handling gaps, long parameter lists, architectural smells (e.g. parameters threaded through many methods that could be instance properties).
149+
2. **Technical debt and security**: TODO/FIXME/HACK comments (note which are stale vs justified), dead code, debug leftovers (`console.log` in `src/`), security smells in `webServer.js` specifically (path traversal, unsafe deserialization, missing input validation, CORS gaps, rate-limit gaps), hardcoded credentials/paths, unused/transitive deps.
150+
3. **Test coverage and quality**: per-module coverage, mock staleness (mocks of methods that no longer exist), test smells (mock-only assertions, multi-expect tests, skipped tests, long setups), gaps in critical paths (connection-pool exhaustion, CORS rejection, rate-limit enforcement, label hot-reload).
151+
4. **Ops and build hygiene**: Dockerfile reproducibility (pinned digests, HEALTHCHECK, layer caching, root vs non-root), `cont-init.d` script strict-mode and ordering, GitHub Actions version pinning (SHA vs `@vX`), secrets exposure, version-sync enforcement, integration-test gating on releases, translation drift across 17 languages.
152+
153+
Tell each agent to return a **prioritized list (HIGH/MEDIUM/LOW) with file:line references**, and to **say so explicitly** if a category is clean rather than padding.
154+
155+
Synthesize their reports into 5-7 top opportunities ranked by (value / effort). Don't dump all findings - the user is looking for a steerable shortlist, not an audit. Lead with the items that close documented pain points or fix latent bugs; defer big architectural refactors for dedicated sessions unless the user explicitly opts in.
156+
157+
### Recurring patterns to look for in this codebase
158+
159+
These are the patterns that have actually shown up in past reviews. If you spot a new instance, flag it:
160+
161+
- **Magic numbers that should be in `src/defaultSettings.js`**. The pattern is hardcoded timeouts/limits/retry counts that ops on slow hardware or fragile networks would want to tune. New tunables should be additive (default = current value) so existing users see no behaviour change. The `haDiscoveryMaxTreeRetryAttempts`/`haDiscoveryTreeRetry*Ms`/`haDiscoveryTreeRequestTimeoutMs`/`webMaxBodySizeBytes` settings (added in 1.9.0) are the template.
162+
- **Exponential-backoff formulas reinvented per file**. Use `src/backoff.js`'s `backoffDelay(retryNumber, { initialMs, maxMs, jitter })` rather than rolling another `Math.min(initial * Math.pow(2, n), max)`.
163+
- **Long positional parameter lists threading state through helper chains** (the `labelSnapshot` smell). If the parameter is conceptually a property of the operation rather than a per-call input, lift it to an instance property scoped to the operation and clear it at the end. Safe because the run is synchronous and JavaScript is single-threaded.
164+
- **Async parse callbacks where state is cleared before the callback fires**. If `parseString` or similar can fail, the failure path needs a recovery route (e.g. via `_handleTreeRequestFailure`); don't just log and return, or the surrounding state machine will get stuck.
165+
- **GitHub Actions pinned to floating tags** (`@v5`, `@v2`). Pin to commit SHA with a version comment. First-party `actions/*` and third-party both.
166+
- **Secrets substituted into rendered `run:` commands**. Move them to `env:` so they cannot leak via verbose-mode logs.
167+
- **`package.json` and `homeassistant-addon/config.yaml` version drift**. CI now enforces this, but if you see the check pass on a release that shouldn't be passing, double-check the version-sync logic in `ci.yml`.
168+
169+
### Process for executing on a multi-item improvement plan
170+
171+
When the user approves a batch of improvements:
172+
173+
1. Create one task per item via `TaskCreate` and track them.
174+
2. Make **one commit per item** with a focused, narrative commit message (the "why", not just the "what"). Easier to review and easier to revert.
175+
3. Order commits so risky/big items go **last** - small infrastructure changes first build confidence in the test suite and CI.
176+
4. After all items are committed locally, push them as a batch and watch CI. The version bump goes in a separate `chore: release vX.Y.Z` commit at the end, with the CHANGELOG entry summarizing the batch.
177+
5. Tag and push the tag - that triggers the HACS distribution workflow.
178+
6. Backfill the source-repo GitHub Release with the CHANGELOG section.

0 commit comments

Comments
 (0)