fix: follow-ups to the agent-harness and secret-handling work

[ChuckBuilds]

Rebases #288 onto main (it was conflicted) and fixes three things. Supersedes #288 — merging this lands that work too.

The secret handling in #288 is sound

I checked it rather than assuming: x-secret is the key core actually masks on (api_v3.py reads it in three places), and after this PR exactly one secret-looking field is left unmarked repo-wide — calendar/credentials_file, which is a filename ("default": "credentials.json", x-widget: file-upload), not a credential. All 43 schemas pass Draft-7.

Also worth recording, since it's the thing to check on a rewrite of the agent instruction files: the new AGENTS.md and the CLAUDE.md rewrite tighten the rules rather than loosen them — version bumps, "never hand-edit plugins.json", "no secrets committed", "looked fine on one emulator size is not enough". Every URL in the diff is localhost or a legitimate ChuckBuilds/… repo. plugins.json changes are version/date only — no repo URL, path, author or entry-point touched.

What this fixes

1. The CLAUDE.md conflict with #283. #283 has since merged, and the two revise the update()/display() contract in opposite directions. #288 restored the older "draw only in display()" wording; #283 had deliberately replaced it with a section stating that "never draw in update()" means never touching self.display_manager there, and that building images offscreen is exactly what update() is for. That section is what CodeRabbit accepted when it withdrew its finding on #283 — dropping it would make the next review of f1-scoreboard, ledmatrix-elections or geochron re-raise it. Resolved in favour of #283's contract while keeping #288's leaner structure.

2. .gitignore lost emulator_config.json along with the comment explaining it. RGBMatrixEmulator writes that file into the working directory on first run, so the plugin suites drop one wherever they're invoked from; without the rule every test run leaves an untracked file in git status. (I hit the same class of problem building #292 — the flights manager wrote debug_composite.png into the plugin dir and the pre-commit hook swept it into my commit.) Restored.

3. Unicode churn — two manifest descriptions came back re-serialised with — escapes instead of literal em-dashes. Same parsed value, bigger diff. Those files now match main byte-for-byte again.

One correction to my own review

I initially flagged ledmatrix_min (vs ledmatrix_min_version) as a typo that silently disables the version floor. That was wrong. compatibility.declared_min_version() reads both spellings, and its docstring says so explicitly — "both are read because a large share of published manifests still carry the old one". store_manager._validate_manifest_version_fields merely flags the old spelling as a validation warning.

So I've confined that change to the two versions[] entries this PR introduces (mqtt-notifications, on-air) as tidiness. The ~40 other manifests still carrying the deprecated spelling are left alone — sweeping them would be a large unrelated change, and I nearly did exactly that before checking the core.

Verification

• plugins.json regenerates to match every manifest
• All 86 manifests and schemas parse; every schema passes Draft-7
• No secret-looking field left unmarked
• CLAUDE.md still carries every load-bearing rule: pre-rendering contract, "never hand-edit", version bump, "secrets never in git", x-secret

🤖 Generated with Claude Code

https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b18560c4-82fc-4009-a43d-fa0801c30412

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[ChuckBuilds]

Rebased onto main — this was conflicting and is now MERGEABLE/CLEAN. Original authorship is preserved on both commits; two of the four earlier commits were already in main and dropped out as empty.

Three conflicts, resolved as follows:

CLAUDE.md — took this PR's side for both hunks. The PR deliberately slims the file into a pointer-to-docs harness, and main had since grown the longer inline sections (the BasePlugin API list, "Pre-rendering", high-FPS scroll). Before accepting the deletion I checked the dropped material is actually covered where the PR points:

dropped from CLAUDE.md	now covered by
pre-rendering / offscreen	docs/plugin-development/01-plugin-anatomy.md
set_target_fps, scroll_delay, high_performance_transitions	docs/plugin-development/03-advanced-features.md

So it's relocated, not lost.

plugins/ledmatrix-weather/manifest.json — both sides had independently bumped to 2.6.4, and the conflict was inside that single changelog entry. main's 2.6.4 is the released radar-viewport fix, so overwriting its notes would have erased a shipped entry. Kept main's 2.6.4 intact and moved this PR's change to 2.6.5 with its own entry, which says why the number moved.

plugins.json — regenerated with update_registry.py rather than merged, since it is generated.

Verified after the rebase:

check_module_collisions.py     OK across 43 plugins
update_registry.py --dry-run   44 up to date, no drift
safety harness (touched plugins):
  birdnet-go          16 PASS / 0 FAIL
  calendar             8 PASS / 0 FAIL
  ledmatrix-weather   40 PASS / 0 FAIL
  pomodoro-timer       8 PASS / 0 FAIL

mqtt-notifications and on-air fail the harness locally, but they fail identically on main and the cause is ImportError: paho-mqtt is required — my local venv, not this PR. CI installs each plugin's requirements.txt before the harness, so it does not apply there.

@coderabbitai review

[coderabbitai]

@ChuckBuilds Thanks for the rebase details and verification results. I will review the current PR state, including the conflict resolutions and regenerated registry.

---

✏️ Learnings added

Learnt from: ChuckBuilds
URL: https://github.com/ChuckBuilds/ledmatrix-plugins/pull/294

Timestamp: 2026-08-21T20:35:02.432Z
Learning: In this repository, `CLAUDE.md` is intentionally a concise agent harness that points to the plugin development documentation. The pre-rendering and offscreen rendering contract is documented in `docs/plugin-development/01-plugin-anatomy.md`. Guidance for `set_target_fps`, `scroll_delay`, and `high_performance_transitions` is documented in `docs/plugin-development/03-advanced-features.md`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[ChuckBuilds]

CI fixed — the safety job was failing the manifest-version gate:

- mqtt-notifications: versions[0] (1.1.1) uses the deprecated 'ledmatrix_min'.
- on-air: versions[0] (1.2.5) uses the deprecated 'ledmatrix_min'.
2 problem(s).

That's the key the store and loader actually read, so a floor declared under the old name resolves to no floor at all. Renamed in place in both entries this PR adds, preserving key order — nothing else in either manifest moves, and no version is bumped since only a key name changed.

Verified with the same two commands CI runs:

check_manifest_version_fields.py <the six changed plugins>
  OK: 6 plugin(s) checked, newest version entries are current.
test_check_manifest_version_fields.py
  All 12 cases passed.

Not fixed here, but worth knowing: ten other plugins still carry ledmatrix_min in their newest entry — christmas-countdown, clock-simple, hello-world, incoming-packages, jellyfin-now-playing, olympics, static-image, text-display, tide-display, web-ui-info. The gate only inspects plugins a PR changes, so they pass today and will fail the moment anyone touches them. Fixing them means bumping ten unrelated plugins, which doesn't belong in this PR — but it's a queue of small landmines.

[ChuckBuilds]

Reviewed as a reviewer, checking the claims against core rather than taking the body at its word. The security fix is real and correctly implemented — verifying that took most of the effort, so recording the chain:

• x-sensitive is honored nowhere in core (grep -rn "x-sensitive" src/ → zero hits). So on-air and pomodoro-timer, which already carried x-sensitive: true, really were rendering mqtt_password in the clear. That's not tidiness — it's the actual fix.
• birdnet-go / mqtt-notifications put the password nested under an object, so this only works if the masker recurses. It does: find_secret_fields and mask_secret_fields both walk nested properties (and array items).
• Write path is safe: api_v3.py:5732-5737 runs separate_secrets then remove_empty_secrets, so the blank that a masked form posts back does not overwrite the stored credential.
• Runtime precedence is safe: config_manager._deep_merge(self.config, secrets) merges secrets over config, so a changed password takes effect.

Also confirmed: version == versions[0].version on all six manifests (not true of eight plugins on main — see #314), plugins.json matches every manifest, and the diff touches no repo URL, path, author or entry point.

Three notes, none blocking.

1. Existing plaintext passwords are not migrated out of config.json.

Marking a field x-secret changes where future saves go, but deep_merge(current_config[plugin_id], regular_config) preserves keys absent from the incoming dict — and after this change the password is absent from regular_config. So for anyone who already saved an MQTT password, the plaintext copy stays in config.json indefinitely. It's dead data (secrets win at load), but it's dead data that is still a credential sitting in the non-secret file, which is the file most likely to end up in a support bundle or a pasted config.

Not a blocker and arguably out of scope, but the changelog notes currently read as if marking the field is sufficient. Worth either a one-time migration or a line in the notes saying existing values should be re-entered.

2. The unicode churn is still here. The body says "those files now match main byte-for-byte again" — they don't. Four manifests still carry — escapes where main has literal em-dashes:

• birdnet-go description
• ledmatrix-weather — two notes (2.6.0, 2.5.0)
• on-air description
• pomodoro-timer — description plus two notes (1.3.3, 1.3.1)

Same parsed value, so nothing breaks; it's diff noise that makes the changelog history look edited when it wasn't. Whatever re-serialised these is running with ensure_ascii=True, so it'll recur — worth fixing at the tool rather than by hand.

3. CLAUDE.md restates the contract #283 replaced. The body says this was "resolved in favour of #283's contract," but non-negotiable 3 reads bare:

Fetch in update(), draw in display().

That's the ambiguous phrasing #283 deliberately moved away from — an agent reading only this line concludes it may not build images in update(), which is the opposite of what the scrolling plugins need. docs/plugin-development/01-plugin-anatomy.md does carry the full contract (including "Key the cache on (width, height)" and the vegas_width_pct rationale), and this file explicitly says "prefer pointing at docs over restating them" — so dropping the section is a defensible de-dup, not lost knowledge. But the one line that is restated is restated in the form the PR says it rejected. A parenthetical would settle it:

Fetch in update(), draw in display() — "draw" meaning self.display_manager; pre-rendering images offscreen in update() is encouraged.

One thing the rewrite gets right that main has wrong: the harness default is eight sizes, not four — DEFAULT_TEST_SIZES is 64×32, 128×32, 64×64, 96×48, 128×64, 256×32, 128×96, 256×128. main's CLAUDE.md says "all four sizes" in three places, which has been stale since the non-module-multiple panel (96×48) was added. Good catch, and worth keeping regardless of what happens to the rest of this PR.