Skip to content

Revise copilot-instructions.md: consolidate, shorten, and extract language-specific rules, take 2#353

Merged
softhack007 merged 78 commits intomdevfrom
copilot/revise-copilot-instructions
Apr 3, 2026
Merged

Revise copilot-instructions.md: consolidate, shorten, and extract language-specific rules, take 2#353
softhack007 merged 78 commits intomdevfrom
copilot/revise-copilot-instructions

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 2, 2026

  • Fix ADC bit-width portability section (SOC_ADC_MAX_BITWIDTH → CONFIG_SOC_ADC_RTC_MAX_BITWIDTH in IDF v5)
  • Add "Why the IDLE task is not optional" explanation
  • Update cpp.instructions.md cross-reference
  • Review fixes: code block lead-in sentence; dual-core chip phrasing (ESP32 + ESP32-S3, vs. single-core chips)

Summary by CodeRabbit

Documentation

  • Added comprehensive internal documentation files establishing coding standards and development workflows for C++, Web UI, CI/CD, and ESP-IDF components.
  • Implemented CodeRabbit configuration to enforce repository-specific code review guidance and quality standards across pull requests.
  • Documented development workflow procedures and contribution expectations to streamline the development process and improve code consistency.

Copilot AI and others added 2 commits April 2, 2026 10:52
…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>
Copilot AI and others added 2 commits April 2, 2026 11:12
… 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>
Copilot AI and others added 2 commits April 2, 2026 11:23
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>
Copilot AI and others added 2 commits April 2, 2026 11:32
@softhack007

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

…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>
@softhack007
Copy link
Copy Markdown
Collaborator

@coderabbitai it's time for your final review :-)

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 3, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== Recent commits ==="
git log --oneline -8

echo ""
echo "======================================"
echo "=== .github/esp-idf.instructions.md ==="
cat .github/esp-idf.instructions.md 2>/dev/null || echo "FILE NOT FOUND"

echo ""
echo "======================================"
echo "=== .github/cpp.instructions.md (tail - static/const sections) ==="
# Show the full file but focus on recent additions
cat .github/cpp.instructions.md

echo ""
echo "======================================"
echo "=== .github/web.instructions.md ==="
cat .github/web.instructions.md

echo ""
echo "======================================"
echo "=== .github/agent-build-instructions.md ==="
cat .github/agent-build-instructions.md

echo ""
echo "======================================"
echo "=== .github/cicd.instructions.md (tail) ==="
tail -60 .github/cicd.instructions.md

echo ""
echo "======================================"
echo "=== .coderabbit.yaml ==="
cat .coderabbit.yaml

echo ""
echo "======================================"
echo "=== git diff HEAD~2 HEAD -- .github/ .coderabbit.yaml ==="
git diff HEAD~2 HEAD -- .github/ .coderabbit.yaml 2>/dev/null || git diff HEAD~1 HEAD -- .github/ .coderabbit.yaml 2>/dev/null

Length of output: 78736


@softhack007 — Final check complete. Here's the summary:

✅ All Previously Flagged Issues — Resolved

# Item Status
PSRAM speed "120 MHz" without experimental caveat ✅ Now correctly states "up to 80 MHz (120 MHz possible with CONFIG_SPIRAM_SPEED_120M, requires enabling experimental ESP-IDF features)"
ADC bit-width macro version-awareness SOC_ADC_MAX_BITWIDTHCONFIG_SOC_ADC_RTC_MAX_BITWIDTH rename documented with version-aware guard pattern
IDLE task explanation ✅ New "Why the IDLE task is not optional" section in esp-idf.instructions.md — accurate and well-structured
xTaskCreateUniversal() tip ✅ Added
// WLEDMM spacing, "common Common", "phased out" ✅ All fixed
@v2 mutable tag nuance in cicd.instructions.md ✅ Explicitly called out
All 5 instruction files linked in .coderabbit.yaml ✅ Confirmed

🟡 One Small Remaining Issue

esp-idf.instructions.mdxTaskCreateUniversal() tip block has 4 typos (introduced in the recent "Improve tip for xTaskCreateUniversal usage" commit):

Typo Fix
"fuction" "function"
"automaticially" "automatically"
"sigle-core" "single-core"
"MCU's" "MCUs"
"``xTaskCreatePinnedToCore()`` ," remove the space before the comma

Everything else — correctness, consistency, internal cross-references, .coderabbit.yaml linkages, and alignment with official GitHub Copilot and CodeRabbit recommendations — is solid. The instruction set is in excellent shape overall.


🧠 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: 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.
✅ Actions performed

Review triggered.

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a469b42 and 0c0a1cd.

📒 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.
@softhack007
Copy link
Copy Markdown
Collaborator

engage

😁

softhack007 and others added 3 commits April 4, 2026 01:28
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.
@softhack007 softhack007 merged commit 44a96c3 into mdev Apr 3, 2026
36 checks passed
@ewowi
Copy link
Copy Markdown
Collaborator

ewowi commented Apr 4, 2026

@coderabbitai, a bit of a critical question, to better understand the benefits of all the .md files to use by AI.
I see above all the AI agents are very positive about it but exactly what problem is this a solution for? It almost looks this is to support a local LLM, but you agent people have all the knowledge already isn't it? I am happy to give you people the really repo specific instructions but general things (esp-idf, c++ etc) I expect you to know and also to digest how that is used from the current code.
I also have been told having too much in AGENTS.md is causing unnecessary distractions.

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

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

@ewowi — Great questions, and a healthy dose of skepticism is warranted here. Let me give you an honest, general-purpose assessment.


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:

What AI genuinely doesn't know without you telling it What AI already knows well enough
Your project's intentional deviations from general best practice General C++ / ESP-IDF / GitHub Actions patterns
Decisions that look like bugs but aren't (e.g. a float→int→uint8_t wrapping pattern) Standard style rules
Which files/patterns to ignore or treat as deprecated Common security practices
Your PR process expectations How delay() works in FreeRTOS
Project-specific macro semantics (WLEDMM_FASTPATH, d_malloc) Memory allocation tradeoffs
Which reviewer comments are out of scope for this PR Naming conventions

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 Drawbacks

1. Token cost — yes, this is real.
Every instruction file is prepended to (or injected into) the context for each review. A 500-line cpp.instructions.md costs the same tokens every single time, on every file, even when reviewing a YAML workflow that has nothing to do with C++. Path-based scoping (like .coderabbit.yaml's path_instructions) mitigates this significantly, but the overhead is still real.

2. Distraction and noise — yes, also real.
There's a well-documented phenomenon where large instruction sets cause models to partially "ignore" instructions later in the context, or to over-apply rules from one domain to another. A model that has been told "flag all VLAs" will flag VLAs even in a file where VLAs are explicitly documented as intentional, unless the instruction is carefully scoped.

3. Maintenance burden — often underestimated.
Instruction files drift from the actual codebase. A rule written today may become incorrect after a refactor. Unlike code, instruction files have no compiler, no tests, and no automated freshness checks.

4. False confidence.
An AI that has been given detailed instructions may appear more authoritative in its review comments, even when those instructions are wrong or outdated. Reviewers may defer to the AI precisely when they should be questioning it.

5. Redundancy with general training.
Documenting that "always put a space after //" or "use camelCase for function names" in an instruction file mostly just adds tokens — the model already applies these rules from its training. You're teaching the model something it learned years ago.


Formatting Guidelines vs. clang-format — Your Point Is Valid

Automated enforcement via clang-format is strictly superior to documented style rules for anything that a formatter can handle. If clang-format is configured and runs in CI, there is no value in also writing "use 2-space indentation" in an instruction file. The instruction file version will:

  • Cost tokens on every review
  • Generate comments that should have been caught by the tool
  • Potentially conflict with the formatter's output

The only style-related things worth documenting for AI are rules that can't be automated: e.g., naming conventions for domain-specific identifiers, or "prefer early return over deeply nested conditions" (which clang-format can't enforce).


Guidelines for Efficient Instruction File Setup

  1. Only document what can't be inferred from the code or from general training.
    If the AI would get it right without instruction 95% of the time, the instruction probably isn't worth the token cost.

  2. Prefer short, actionable rules over explanations.
    "Do not flag uint8_t x = int(f) as a bug when the comment says // float→int wrapping" is more useful than a paragraph explaining why. The paragraph is for humans; the rule is for AI.

  3. Use path-based scoping aggressively.
    A C++ rule should never be injected into a YAML review. A CI/CD rule should never appear in an embedded firmware review. Unscoped instructions are the fastest path to distraction.

  4. Keep the total injected instruction size per file type under ~500 words.
    Beyond that, the marginal value of additional instructions drops rapidly and the distraction cost rises.

  5. Encode "false positive suppressors" first, feature guidance second.
    "Don't flag pattern X as a bug" is more impactful than "always do Y" because it directly prevents noise.

  6. Treat instruction files like tests — they need maintenance when the code changes.
    If you wouldn't update the instruction file when refactoring the relevant code, don't write the instruction.

  7. Prefer automation over documentation.
    clang-format + cppcheck + ESLint enforce more reliably and cheaply than any instruction file.

  8. One general instruction file + small path-scoped supplements tends to work better than many large domain-specific files. The general file covers cross-cutting concerns; the path files suppress known false positives in specific areas.


Bottom line: The sweet spot is a short general file covering project identity + a handful of false-positive suppressors for your project's intentional deviations, with automated tooling doing the heavy lifting for everything else. More is not better.


🧠 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
Copy link
Copy Markdown
Collaborator

ewowi commented Apr 4, 2026

@coderabbitai, regarding

Bottom line: The sweet spot is a short general file covering project identity + a handful of false-positive suppressors for your project's intentional deviations, with automated tooling doing the heavy lifting for everything else. More is not better.

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

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

@ewowi — Fair challenge. Applying my own advice to these files, here are my concrete recommendations:


Applying the Lean Principle: File-by-File Audit

cpp.instructions.md (~505 lines) — Significantly over budget

This is the biggest target. Suggested cuts:

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 ⚠️ Trim — keep the WLED-specific attributes (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_BITWIDTHCONFIG_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.

@softhack007 softhack007 changed the title Revise copilot-instructions.md: consolidate, shorten, and extract language-specific rules Revise copilot-instructions.md: consolidate, shorten, and extract language-specific rules, take 2 Apr 4, 2026
@softhack007
Copy link
Copy Markdown
Collaborator

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.

@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.

@softhack007
Copy link
Copy Markdown
Collaborator

softhack007 commented Apr 4, 2026

@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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

@softhack007 — Great question. The honest answer is: neither CodeRabbit nor GitHub Copilot currently has a built-in directive to skip sections within a file. Once a file is referenced in .coderabbit.yaml's path_instructions or picked up as a *.instructions.md, the full text is consumed. HTML comments (<!-- -->) are invisible in rendered Markdown but are still present in the raw text the AI reads — so they don't help either.

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 .coderabbit.yaml:

.github/
  cpp.instructions.md          ← AI-facing: ~150 lines of suppressors + WLED-MM specifics
  esp-idf.instructions.md      ← AI-facing: ~200 lines of project-specific notes

docs/contributing/
  cpp-guide.md                 ← Human-facing: full CONFIG_IDF_TARGET_* patterns, examples, etc.
  esp-idf-guide.md             ← Human-facing: comprehensive reference

.coderabbit.yaml only references the .github/ files. The docs/ files are linked from a CONTRIBUTING.md for human contributors.


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 .coderabbit.yaml directly

For content that must stay short and strictly AI-scoped, put the rule inline in .coderabbit.yaml's instructions: field rather than in a file. This way the comprehensive .md file can be entirely human-facing:

path_instructions:
  - path: "**/*.{cpp,h,hpp,ino}"
    instructions: |
      Apply cpp.instructions.md conventions.
      Key suppressors: float→int→uint8_t wrapping is intentional; WLEDMM_FASTPATH is the default path; VLAs require PR justification.

Practical Recommendation for This PR

Given the effort already invested in the current files, the lowest-cost path is Option 2 as a convention now, with a note in CONTRIBUTING.md pointing human contributors to the full files. If token budget becomes a measured problem in future reviews, a proper split (Option 1) can be done incrementally — the markers will make it easy to identify what to move.


🧠 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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@softhack007 — Great question. Here's what the official tooling actually supports, and the best practical workarounds:


What the Tools Natively Support

Neither 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:

  • GitHub Copilot: You can prevent an entire file from being used by Copilot code review or Copilot coding agent by adding the excludeAgent keyword to the YAML frontmatter block, using either "code-review" or "coding-agent". This is an all-or-nothing switch per file, not per section.

  • CodeRabbit: path_instructions adds path-specific guidance for code review using file path glob patterns. The instructions field provides additional review guidance for matching paths. Again, scoping is at the file/path level, not the section level within a file.


Practical Workarounds (Best to Worst)

1. 🥇 Split into two files (most reliable)

Keep a lean AI-facing file referenced in .coderabbit.yaml and a full human reference file that is NOT referenced by any AI config:

.github/
  esp-idf.instructions.md        ← referenced in .coderabbit.yaml (lean, AI-facing)
  docs/esp-idf-reference.md      ← human reference only, ignored by AI

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., ## Appendix: Reference Tables). Models weight earlier context more heavily when their "instruction budget" is full. This is the lowest-maintenance option but the least reliable.


Recommendation for This PR

Given the context of esp-idf.instructions.md (~756 lines), the most practical approach is option 2 (convention-based markers) combined with moving the longest pure-reference tables (like the full CONFIG_IDF_TARGET_* pattern table) under <!-- HUMAN_ONLY_START --> markers. That way:

  • The file remains a single source of truth for contributors
  • The AI preamble instructs AI tools to treat those sections as reference-only
  • No duplicate maintenance burden
  • The rendered Markdown stays clean (HTML comments are invisible)

Would you like me to plan the addition of a HUMAN_ONLY marker convention to the file preamble as a concrete edit?


🌐 Web query:

GitHub Copilot custom instructions file exclude section ignore block

💡 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:

  • Scope an instructions file to only certain paths (so the instructions effectively don’t apply elsewhere) using frontmatter like applyTo in .github/instructions/*.instructions.md. The applyTo value supports glob patterns. [1]
  • Prevent specific Copilot agents from using a given instructions file with frontmatter excludeAgent (e.g., exclude "code-review" or "coding-agent"). [1]
  • Exclude files/directories from being used by Copilot at all using Copilot Content Exclusion (configured in GitHub repo/org/enterprise settings, using path/pattern rules). This is the supported mechanism for “ignore these files,” not something embedded in an instructions markdown file. [2], [3]
  • There is no official .copilot-ignore / copilot.exclude file you drop into a repo to manage exclusions. [4]

Sources

  1. GitHub Changelog: agent-specific instructions (applyTo, excludeAgent) [1]
  2. GitHub Docs: Copilot Content Exclusion configuration and pattern syntax [2]
  3. GitHub Changelog: Content Exclusions on github.com [3]
  4. GitHub Community discussion noting no official .copilot-ignore/copilot.exclude repo file [4]

🌐 Web query:

CodeRabbit path_instructions exclude section markdown file

💡 Result:

CodeRabbit’s reviews.path_instructions can only target files/paths via glob patterns—it can’t exclude (or apply different rules to) a specific section inside a Markdown file. (docs.coderabbit.ai)

Exclude an entire Markdown file (or all .md) instead

Use reviews.path_filters to exclude paths (prefix with !). (docs.coderabbit.ai)

# .coderabbit.yaml
reviews:
  path_filters:
    - "!docs/README.md"   # exclude one file
    # or:
    - "!**/*.md"          # exclude all Markdown files

If you only want “different review behavior” for Markdown (not exclusion)

Keep the files included, and add a Markdown-specific instruction block:

reviews:
  path_instructions:
    - path: "docs/**.md"
      instructions: |
        Check for clarity/accuracy only; avoid style nitpicks.

Workarounds to “exclude a section”

  • Move that section into a separate file (then exclude that file with path_filters).
  • Or move the content to a generated/ignored location you exclude with path_filters. (docs.coderabbit.ai)

Citations:


@softhack007 — The web search confirms the picture. Here's a precise, citation-backed answer:


Native Tool Support: File-Level Only, No Section-Level

Neither CodeRabbit nor GitHub Copilot has a built-in "skip this section" syntax within an instruction file.

  • 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.

  • CodeRabbit's reviews.path_instructions can only target files/paths via glob patterns — it can't exclude (or apply different rules to) a specific section inside a Markdown file.

What the tools do support is file-level scoping:

  • GitHub Copilot: You can scope an instructions file to only certain paths using applyTo frontmatter in .github/instructions/*.instructions.md, or prevent specific Copilot agents from using a given file with excludeAgent (e.g., exclude "code-review" or "coding-agent").

  • CodeRabbit: Use reviews.path_filters (prefix with !) to exclude an entire file from review.


Practical Workarounds

🥇 Split into two files (the only reliably enforced approach)

The official workarounds to "exclude a section" are: move that section into a separate file (then exclude that file with path_filters), or move the content to a generated/ignored location you exclude with path_filters.

For your case, this would look like:

.github/
  esp-idf.instructions.md     ← lean AI-facing file, referenced in .coderabbit.yaml
  docs/esp-idf-reference.md   ← full human reference, NOT referenced by any AI config

You can cross-link them: add a line at the bottom of the AI-facing file: > Human reference: see [esp-idf-reference.md](docs/esp-idf-reference.md).

For GitHub Copilot, add frontmatter to the human-reference file so Copilot's code-review agent skips it entirely:

---
applyTo: "none"
excludeAgent: "code-review"
---

🥈 Convention-based section marker (soft, but keeps one file)

There is no official support for this, but you can define a convention and instruct the AI in the file's own preamble:

> **Note for AI review 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.

<!-- HUMAN_ONLY_START -->
## Reference: CONFIG_IDF_TARGET_* patterns
...detailed tables for contributors...
<!-- HUMAN_ONLY_END -->

HTML comments are invisible in rendered Markdown, so contributors see clean docs. The AI preamble instruction provides guidance to skip those blocks. This is soft — AI compliance is not guaranteed and depends on context length — but it's zero maintenance overhead and works reasonably well in practice for large, clearly labelled blocks.


Recommendation: For esp-idf.instructions.md specifically (the largest file at ~756 lines), the split-file approach gives the cleanest result. The convention-based marker is a good fit for cpp.instructions.md where the reference content is interspersed and harder to cleanly separate.


🧠 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.

@softhack007
Copy link
Copy Markdown
Collaborator

@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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

@softhack007 — The divergence concern is well-founded. Standalone educational docs are exactly the kind of content that gets silently stale — no CI failure, no test break, just quiet drift.

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 @coderabbitai compare the HUMAN_ONLY sections of esp-idf.instructions.md against the AI-facing rules above and flag any drift would catch erosion before it becomes invisible. It turns the "educational docs erode" problem into a reviewable, flaggable condition rather than silent drift.

The combination of soft markers + occasional AI alignment checks is more resilient than either file splitting or no markers at all.


🧠 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.

@softhack007
Copy link
Copy Markdown
Collaborator

@coderabbitai good point - please suggest a path instruction for .coderabbit.yaml that would trigger a compare the HUMAN_ONLY sections whenever a PR modifies the .github/*instructions.md`files.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

Tip

For best results, initiate chat on the files or code changes.

@softhack007 — Here is a ready-to-use path_instructions entry for .coderabbit.yaml:

    - 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 reviews.path_instructions list in .coderabbit.yaml, alongside the existing **/*.{cpp,h,hpp,ino} entry. The glob .github/*.instructions.md will match all current instruction files (cpp.instructions.md, esp-idf.instructions.md, cicd.instructions.md, web.instructions.md) and any future ones added to .github/.

The three-point instruction structure keeps it concise while covering the three failure modes worth catching: contradiction, drift, and missing coverage.


🧠 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.

softhack007 added a commit that referenced this pull request Apr 4, 2026
… 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants