Revise copilot-instructions.md: consolidate, shorten, and extract language-specific rules, take 2#353
Conversation
…ct language-specific rules - Reduced main file from 10,007 chars / 188 lines to 3,745 chars / 87 lines - Merged Build Timing, Development Workflow, Key Files, Important Notes into relevant sections - Created cpp.instructions.md for C++ conventions (applyTo: **/*.cpp,**/*.h,**/*.ino) - Created web.instructions.md for web UI conventions (applyTo: wled00/data/**) - Added C++ code examples for header guards and WLEDMM markers - Preserved all essential meaning and instructions Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/efbf6aab-bf3e-4a26-9987-4a23df01a663 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/efbf6aab-bf3e-4a26-9987-4a23df01a663 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
… codebase analysis Analyzed the actual pixel pipeline (Segment → Strip → BusManager → BusDigital → PolyBus) and documented the optimization patterns found in FX_fcn.cpp, FX_2Dfcn.cpp, bus_manager.cpp, colorTools.hpp. Added sections for: function attributes, uint_fast types, member caching, unsigned range checks, early returns, pre-computation, parallel channel processing, bit shifts, static caching, inlining strategy, and color handling. Also added general sections for preprocessor flags, error handling, strings, and memory/PSRAM from CONTRIBUTING.md baseline. Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/0f020127-abbe-46ab-895f-60d9a2b6550f Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/0f020127-abbe-46ab-895f-60d9a2b6550f Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Move detailed build timeouts, development workflows, troubleshooting, manual testing scenarios, and CI validation steps from copilot-instructions.md into agent-build-instructions.md. The main copilot-instructions.md now contains the concise project overview and links to the detailed agent instructions. Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/b97ca18e-bdf2-4a20-b631-ffebb2d3db72 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/b97ca18e-bdf2-4a20-b631-ffebb2d3db72 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/a603c522-9f87-4ac9-b257-776811d1e27e Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
…ead it Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/a603c522-9f87-4ac9-b257-776811d1e27e Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
…ation Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/34c8abd3-7360-449a-9831-a590b4f597fe Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/673fed5c-912d-4b1b-9a12-cd0bd20a8d04 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
…n in esp-idf.instructions.md Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/d96831e9-2088-4a18-8b07-040fd66cd133 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
…e chip phrasing Agent-Logs-Url: https://github.com/MoonModules/WLED-MM/sessions/26473aa2-9477-4f37-8900-1c095889a29c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
|
@coderabbitai it's time for your final review :-) |
🧩 Analysis chain🏁 Script executed: Length of output: 78736
✅ All Previously Flagged Issues — Resolved
🟡 One Small Remaining Issue
Everything else — correctness, consistency, internal cross-references, 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Line 35: In the "Common firmware environments" list the comma before the token
esp32dev_compat is missing a trailing space; edit that line so there is a space
after the comma (i.e., change "...esp32_16MB_V4_M_eth,`esp32dev_compat`..." to
include a space after the comma), updating the environment list string that
contains esp32dev_compat for consistent formatting and readability.
In @.github/esp-idf.instructions.md:
- Around line 696-697: Fix the typos in the xTaskCreateUniversal() tip: change
"fuction" to "function", "automaticially" to "automatically", and "sigle-core"
to "single-core", and rephrase the sentence to read clearly that
xTaskCreateUniversal() has the same signature as xTaskCreatePinnedToCore() but
automatically falls back to xTaskCreate() on single-core MCUs. Ensure the
corrected symbols xTaskCreateUniversal(), xTaskCreatePinnedToCore(), and
xTaskCreate() remain exact.
- Around line 718-719: Line 718 incorrectly attributes software-timer execution
to the FreeRTOS IDLE task; update the text to state that FreeRTOS software
timers are serviced by the dedicated timer service/daemon task (enabled via
configUSE_TIMERS = 1 in FreeRTOSConfig.h) which runs separately from the IDLE
task and processes timer callbacks from its own queue, while also keeping the
note that starving IDLE still affects entry to light sleep/power management.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ff722fe-e218-4f7c-a1be-e772afdfbeed
📒 Files selected for processing (5)
.coderabbit.yaml.github/agent-build-instructions.md.github/copilot-instructions.md.github/cpp.instructions.md.github/esp-idf.instructions.md
✅ Files skipped from review due to trivial changes (2)
- .coderabbit.yaml
- .github/cpp.instructions.md
Corrected typos and formatting in the usage tip for xTaskCreateUniversal.
|
engage 😁 |
readability Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Clarified the role of the FreeRTOS IDLE task in memory management, timer execution, and low-power sleep functionality.
|
@coderabbitai, a bit of a critical question, to better understand the benefits of all the .md files to use by AI. A smaller side note: I see also formatting guidelines in the .md files, but isn't it better to just define clang-format to enforce code style? Looks redundant to me to try to teach you how formatting should look like. So please give an overview of general advantages using the files defined here, but focus on the general drawbacks. Does it cost more AI thinking time? More tokens, can it distract you and so on and so on. Conclude with guidelines how to setup multiple .md files efficiently. I am not asking this for this WLED-MM repository specifically, just asking for a friend, so keep your response general |
|
Tip For best results, initiate chat on the files or code changes.
What Problem Do Instruction Files Actually Solve?You're right that large language models already have extensive general knowledge about C++, ESP-IDF, GitHub Actions, etc. The genuine value of instruction files is narrow but real:
The key insight: instruction files are most valuable when they prevent false positives (AI flagging intentional patterns as bugs) and encode decisions that can't be inferred from the code alone. Real Drawbacks1. Token cost — yes, this is real. 2. Distraction and noise — yes, also real. 3. Maintenance burden — often underestimated. 4. False confidence. 5. Redundancy with general training. Formatting Guidelines vs.
|
|
@coderabbitai, regarding
Can you then review the files included in this PR using the remarks you just gave and give a list of recommendations how to fine tune this |
|
Tip For best results, initiate chat on the files or code changes.
Applying the Lean Principle: File-by-File Audit
|
| Section | Recommendation | Rationale |
|---|---|---|
| Formatting (indentation, brace style, spacing) | ✂️ Remove entirely | clang-format enforces this; documenting it is redundant tokens every review |
| Naming conventions (camelCase, PascalCase, UPPER_CASE) | ✂️ Remove or collapse to 2 lines | LLMs know standard C++ naming conventions |
General const/constexpr rationale (the long "why" paragraphs) |
✂️ Keep rules only, remove explanations | The decision-tree at the end ("if it doesn't write members → static; if it only reads → const") is all that's needed for review |
| Template specialization guidance (general C++ patterns) | ✂️ Remove | General LLM knowledge |
Header guard pattern (#ifndef/#define/#pragma once) |
✂️ Remove | General knowledge |
// WLEDMM marker, WLEDMM_FASTPATH semantics, d_malloc/p_malloc, float→int wrapping suppressor, VLA policy |
✅ Keep — these are the genuine false-positive suppressors and project-specific decisions | |
| Hot-path optimization section | WLED_O2_ATTR, IRAM_ATTR) and the "pixel get/set is the hot path" scoping note; remove generic optimization advice |
|
| FreeRTOS mutex macros | ✅ Keep — project-specific macro names can't be inferred |
Estimated savings: ~200–250 lines, keeping the 50–80 lines that are genuinely project-specific.
esp-idf.instructions.md (~756 lines) — Largest file, highest redundancy ratio
ESP-IDF is thoroughly covered in LLM training. The project-specific value is narrow:
| Section | Recommendation | Rationale |
|---|---|---|
General CONFIG_IDF_TARGET_* patterns |
✂️ Remove | Well-known |
Generic heap_caps_* usage |
✂️ Remove | Standard knowledge |
| General I2S, RMT, ADC patterns | ✂️ Remove | Well-known; only keep the IDF v4→v5 rename table |
| General FreeRTOS task creation/pinning | ✂️ Remove | Well-known |
| GPIO33–37 reserved on ESP32-S3 with octal PSRAM | ✅ Keep — catches a real, non-obvious hardware constraint | |
SOC_ADC_MAX_BITWIDTH → CONFIG_SOC_ADC_RTC_MAX_BITWIDTH rename |
✅ Keep — an IDF v5 breaking change | |
| IDF v4→v5 migration table | ✅ Keep — project-specific version transition | |
| PSRAM 80/120 MHz caveat + experimental flag | ✅ Keep — prevents a real misunderstanding | |
xTaskCreateUniversal() tip |
✅ Keep — project-specific helper | |
IDLE task / vTaskDelay(0) guidance |
✅ Keep — prevents a specific false-positive pattern |
Estimated savings: ~450–500 lines. The file could realistically be ~200–250 lines.
cicd.instructions.md (~153 lines) — Moderate redundancy
| Section | Recommendation | Rationale |
|---|---|---|
| YAML formatting, descriptive names, "add comments" | ✂️ Remove | General LLM knowledge |
Caching with actions/cache@v4, artifact naming |
✂️ Remove | Well-known patterns |
General permissions: + secrets: scoping |
✂️ Trim to 2–3 lines | Keep only the project-specific note that GITHUB_TOKEN should be at step level |
@v2 mutable tag nuance, pull_request_target restriction |
✅ Keep — concrete false-positive suppressors | |
| Project-specific action version preferences | ✅ Keep if you have them |
Estimated savings: ~60–80 lines.
web.instructions.md (~28 lines) — Already lean ✅
No significant cuts needed. The only marginal item is camelCase for JS functions (general knowledge), but removing it saves almost nothing.
agent-build-instructions.md (~109 lines) — Correct as-is
This file isn't injected by .coderabbit.yaml during code review — it's purely for Copilot agent mode. It doesn't affect review token costs. No action needed.
Summary of Recommended Impact
| File | Current | After trim | Primary reason to cut |
|---|---|---|---|
cpp.instructions.md |
~505 lines | ~200–250 lines | General C++ knowledge + formatting (→ clang-format) |
esp-idf.instructions.md |
~756 lines | ~200–250 lines | General ESP-IDF knowledge |
cicd.instructions.md |
~153 lines | ~75–90 lines | General GitHub Actions knowledge |
web.instructions.md |
~28 lines | ~25 lines | Already lean |
| Total injected per C++ review | ~1,414 lines | ~475–590 lines |
The lean version would stay well within the ~500-word target per review context, concentrate on actual false-positive suppressors, and reduce noise — which is what makes reviews more focused, not less.
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 353
File: .github/cpp.instructions.md:418-425
Timestamp: 2026-04-03T13:44:26.400Z
Learning: In MoonModules/WLED-MM, the guidance for float-to-unsigned conversion is: casting a potentially-negative float through `int` first (e.g., `uint8_t x = int(someFloat)`) is defined and intentional when the float value is known to be within `int`'s representable range. This achieves consistent, repeatable modulo-256 wrapping across Xtensa (ESP32) and RISC-V (ESP32-C3/C6) MCUs — avoiding the UB that occurs when a negative float is cast directly to an unsigned type. The comment `// float→int (defined), int→uint8_t (defined)` in cpp.instructions.md is correct for expressions like `40.74f * atan2f(dy, dx)` whose result is bounded within int's range. Do not suggest clamping as a "fix" for this pattern, as it would change the intended wrapping semantics.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-02T22:23:41.534Z
Learning: In MoonModules/WLED-MM, the approved VLA guidance for cpp.instructions.md reads: "Avoid Variable Length Arrays (VLAs): GCC/Clang support VLAs as an extension (not part of the C++ standard), so they look like a legitimate feature — but they are allocated on the stack at runtime. On ESP32/ESP8266, FreeRTOS task stacks are typically only 2–8 KB; a VLA whose size depends on a runtime parameter (segment dimensions, pixel counts, etc.) can silently exhaust the stack and cause the program to behave in unexpected ways or crash. Prefer a fixed-size array with a compile-time bound, or heap allocation (d_malloc / p_malloc) for dynamically sized buffers. Any VLA must be explicitly justified in the PR."
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2026-04-04T00:29:49.113Z
Learning: In MoonModules/WLED-MM, function-level doc comment blocks (e.g. /* ***** */ style headers) are NOT required during code reviews, especially not for previously existing functions. Do not flag missing function-level comments as review issues in this repository.
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-03T23:41:27.774Z
Learning: Applies to wled00/src/**/*.{cpp,h,hpp} : Match existing code style in C++ files you edit. Use clang-format for formatting when needed. See `cpp.instructions.md` for language-specific conventions
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-03T23:41:27.774Z
Learning: Applies to wled00/**/*.{cpp,h,hpp} : Document attribution of inspiration, knowledge, and sources used in code. Use `// AI:` comments for larger blocks of AI-generated code and ensure AI-generated code is well documented
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/json.cpp:295-304
Timestamp: 2025-10-22T22:24:46.163Z
Learning: When deriving an upper-bound index from a count in WLED-MM PR `#270`, use a safe guard: last = count ? count - 1 : 0. Avoid min/max with 0 on unsigned types and never pass (count-1) directly to helpers (e.g., updateVal16, relativeChange, random8) without guarding. Effect IDs are 16-bit; palette IDs remain 8-bit but should use the same guard when count can be 0.
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 240
File: wled00/image_loader.cpp:0-0
Timestamp: 2025-11-14T12:42:22.059Z
Learning: In MoonModules/WLED-MM and embedded systems code, the pattern `unsigned timePassed = millis() - last_time;` is rollover-safe and proven to work correctly even when millis() wraps around at ~49.7 days uptime. This works due to modular arithmetic in unsigned integers. The subtraction yields the correct elapsed time regardless of rollover. This pattern should not be flagged as problematic or refactored to use signed arithmetic.
@ewowi This was intentionally - currently we can't use auto-formatting via .clang-format, because that will cause lots of merge conflicts (different formatting) whenever MM tries to cherry-pick features from upstream WLED. That's why I added a few basic "soft" rules for MM here, instead of enforcing them with clang-format. |
|
@ewowi thanks for your ideas to trim-down instructions files and only keep ruless that are not "common AI knowledge". Question @coderabbitai the new instructions will also be valuable for human contributors. Is there a good way to "mark" or "tag" paragraphs / sections, so they will be ignored for AI reviews? The idea is to slim down the "AI relevant" parts, but still keep things like the CONFIG_IDF_TARGET_* patterns in the file, as additional context for contributors. |
|
Tip For best results, initiate chat on the files or code changes.
Here are the practical options, roughly ordered by effectiveness: Option 1 — File-level split (cleanest, most reliable)Keep the AI-facing instruction files lean (only false-positive suppressors + project-specific decisions), and move the comprehensive human-readable documentation to a separate location that is not referenced in
Option 2 — Convention-based marker (informational only, no enforcement)Agree on a marker convention and manually ensure AI-facing files don't include those sections. For example: <!-- [human-only]
The following section is background for human contributors and is not part of the AI review rules.
### CONFIG_IDF_TARGET_* patterns
...detailed examples...
[/human-only] -->This does not prevent the AI from reading the content — but it signals to future maintainers what can be trimmed if token budget becomes a concern, and keeps the editorial intent visible in version history. Option 3 — Structured AI review instructions in
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
What the Tools Natively SupportNeither CodeRabbit nor GitHub Copilot has a built-in "skip this section" syntax within an instruction file. Exclusion controls exist only at the file level, not the section level:
Practical Workarounds (Best to Worst)1. 🥇 Split into two files (most reliable)Keep a lean AI-facing file referenced in Both can cross-link each other. Downside: two files to maintain. 2. 🥈 Convention-based section marker (soft, but readable by humans AND AI)Define a custom HTML comment convention and teach the AI about it in the file's own preamble: <!-- HUMAN_ONLY_START -->
## Reference: CONFIG_IDF_TARGET_* patterns
The following table is informational reference for contributors...
<!-- HUMAN_ONLY_END -->Then add to the file header: > **Note for AI tools**: Sections enclosed in `HUMAN_ONLY_START / HUMAN_ONLY_END`
> HTML comments are contributor reference material and should NOT be used as
> actionable review criteria.HTML comments are invisible in rendered Markdown (contributors see clean docs), while the header instruction tells AI tools to skip those blocks. This is soft — AI compliance isn't guaranteed — but it's practical, readable, and costs nothing extra to implement. 3. 🥉 "AI rules first" layout (lightest-weight, least reliable)Put the lean AI-relevant rules at the TOP of the file and push human-only reference content to a clearly-labeled section at the bottom (e.g., Recommendation for This PRGiven the context of
Would you like me to plan the addition of a 🌐 Web query: 💡 Result: There isn’t an “exclude section” / “ignore block” syntax inside a Copilot custom instructions markdown file that tells Copilot to ignore parts of the file. What you can do instead:
Sources
🌐 Web query: 💡 Result: CodeRabbit’s Exclude an entire Markdown file (or all
|
|
@coderabbitai i think i'll start with option 2 - "soft" conventional markers. The problem with option1 is divergence - we might be able to ask for an AI review of the "human-facing" to ensure that they still reflect the AI-facing docs that will evolve faster. But experience tells me that "educational only" docs are eroding quickly and will be forgotten. |
|
Tip For best results, initiate chat on the files or code changes.
Here's a ready-to-use template for the soft marker convention you can drop into the file preamble: > **Note for AI review tools**: sections enclosed in
> `<!-- HUMAN_ONLY_START -->` / `<!-- HUMAN_ONLY_END -->` HTML comments contain
> contributor reference material. Do **not** use that content as actionable review
> criteria — treat it as background context only.And then wrap the pure-reference sections: <!-- HUMAN_ONLY_START -->
### Reference: CONFIG_IDF_TARGET_* patterns
| Macro | Chips |
|---|---|
| `CONFIG_IDF_TARGET_ESP32` | original ESP32 |
...
<!-- HUMAN_ONLY_END -->HTML comments are invisible in rendered Markdown, so contributors see clean documentation with no visual clutter. On your mitigation idea — using AI review to keep the human-facing sections aligned with the evolving AI-facing rules — that's actually the strongest part of the approach. A periodic comment like The combination of soft markers + occasional AI alignment checks is more resilient than either file splitting or no markers at all. 🧠 Learnings used |
|
@coderabbitai good point - please suggest a path instruction for .coderabbit.yaml that would trigger a |
|
Tip For best results, initiate chat on the files or code changes.
- path: ".github/*.instructions.md"
instructions: |
This file contains both AI-facing rules and human-only reference sections.
Human-only sections are enclosed in `<!-- HUMAN_ONLY_START -->` /
`<!-- HUMAN_ONLY_END -->` HTML comment markers and should not be used as
actionable review criteria.
When this file is modified in a PR, perform the following alignment check:
1. For each `<!-- HUMAN_ONLY_START --> ... <!-- HUMAN_ONLY_END -->` block,
verify that its examples and guidance are consistent with (and do not
contradict) the AI-facing rules stated in the same file.
2. Flag any HUMAN_ONLY section whose content has drifted from the surrounding
AI-facing rules due to edits introduced in this PR.
3. If new AI-facing rules were added without updating a related HUMAN_ONLY
reference section, note this as a suggestion (not a required fix).Drop this as a new entry in the The three-point instruction structure keeps it concise while covering the three failure modes worth catching: contradiction, drift, and missing coverage. 🧠 Learnings used |
… background information for contributors (#354) Revising AI review instructions: * Reduce context window use for AI review tools, by avoiding repeating common knowledge and API information that is (usually) part of the AI training datasets any way. * Introduce a mechanism to maintain both parts in single files, to avoid "silent diversion" over time * Adding a coderabbit path instruction that ensures cross-checking of both parts whenever a PR modifies instruction files Objectives: * Primary Goal: only inject content in AI-visible areas that are WLED-MM–specific or which deviate from general knowledge (the context window "token cost" of true false-positive suppressors is always worth it). * Soft goal: keep each file's AI-facing section lean enough that the signal-to-noise ratio in the attention layer stays high — around 1,500–2,000 words per file type is a reasonable practical ceiling for current models. * Aspirational: 500 words per file if achievable without sacrificing review quality. This is an evolution of #353, based on the discussion in #353 (comment) --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Documentation