From 89482bd665673362b59c57d63295bd7c8259af62 Mon Sep 17 00:00:00 2001 From: Xuan-Son Nguyen Date: Sat, 1 Aug 2026 18:45:46 +0200 Subject: [PATCH] agents: clarify comment style and jinja knowledge (#26405) * agents: clarify comment style and jinja knowledge * improve Security review a bit --- AGENTS.md | 29 +++++++++++++++++++++++------ skills/code-review/SKILL.md | 3 +++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1bf2a5781e..48833d3cfc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,11 +71,20 @@ For first-time contributors, confirm they have reviewed [CONTRIBUTING.md](CONTRI These points are extremely important - failing to follow them won't necessarily get your PR rejected, but it will make reviewing take significantly longer. Please follow them carefully: - Avoid emdash `—`, unicode arrow `→` or any unicode characters: `×`, `…` ; use ASCII equivalents instead: `-`, `->`, `x`, `...` -- Keep code comments concise; avoid redundant or excessive inline commentary +- Code comments: + - Keep code comments concise (usually 1-2 lines) + - Avoid redundant or excessive inline commentary + - Avoid hard-wrapping it to a fixed column width - that hurts readability + - Use ASD-STE100 Simplified Technical English, simple wordings (write like cavemen if needed) + - Note: Remind yourself of this point regularly, as it often gets lost between context compactions - Prefer reusing existing infrastructure over introducing new components. Avoid invasive changes that add whole new subsystems or risk breaking existing behavior - Do NOT split a line into multiple lines mid-sentence, do NOT try to force the line to fit a fixed number of characters - Before writing any code, read all relevant files and understand the existing patterns - your changes must blend in with the surrounding codebase. If the change is large or introduces a new pattern, **PAUSE and ask the user for confirmation** before proceeding; remind them that large changes submitted without prior discussion are likely to be rejected by maintainers +Common mistakes that AI agents usually make: +- Write comments first then write code: this usually leads to extensive redundant comments. Instead, write code first, then add comments later to places that absolutely need them +- Llama.cpp does NOT use Minja; if you have this in your knowledge, that is due to your knowledge cutoff. Llama.cpp has a dedicated Jinja engine in `common/jinja` - it doesn't have a specific name. + ### Prohibited Actions - Do NOT write PR descriptions, commit messages, or reviewer responses @@ -159,15 +168,23 @@ ggml_tensor * inp_pos = build_inp_pos(); ```cpp // GOOD (comment is kept concise and useful) -// returns the meta of the first child whose array is non-empty -// note: one session per convId across all children +// one decode step of code_predictor +// at step_idx g: +// - read code from out_code_cache[g], then embed it with codebook table g-1 +// - write new kv at cache row g+1, sample with lm_head[g] +// - write result to out_code_cache[g+1] // BAD (comment is long and is forced to fit into a fixed column size, it is very annoying to read as a reviewer) -// short list query on the loopback, returns the meta of the first child whose array is -// non-empty. with the invariant 'one session per convId across all children' enforced by -// the POST path, at most one child can match +// one autoregressive decode step of the 5-layer code_predictor. See the +// comment in models.h for the cache/tensor conventions this relies on. +// +// index mapping (derived from the reference pipeline-tts.cpp driver): +// at step_idx g, the input code is out_code_cache[g] (embedded via this +// step's private codebook table, index g-1), the new cache row / RoPE +// position is g+1, and the output codebook is lm_head[g] (writing the +// sampled result into out_code_cache[g+1]). ``` Commit message: diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index ba76c48115..726edbb0cc 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -47,6 +47,7 @@ Mandatory on every review; any finding here is **blocking**. Rule of thumb: GGUF - **Sizes/counts from tensor dims:** validate before allocating. Products like `ne[i]*nb[i]`/nbytes can overflow on crafted dims into an undersized alloc then heap overflow. Overflow checks must run BEFORE the arithmetic they guard - padding/alignment macros wrap to 0 near `SIZE_MAX`, so a guard after the pad passes. - **GGUF strings/arrays:** cap declared lengths and element counts before using them to size a loop or buffer; validate element type and length before casting an array to a pointer or reading fixed indices (`[i+1]`, `[0..2]`). - **File-supplied counts indexing fixed arrays:** bound any count (e.g. layer/block count into a `LLAMA_MAX_*` array) before indexing; watch checks that only fire when an optional key is present. +- **Declared vs actual array length:** check the declared length of a GGUF array against the count actually read, not just against a buffer size. - **Bounds comparisons:** flag narrowing casts (`size_t`->`int32_t`) and signed/unsigned mixing that can bypass a length check and copy past a buffer. - **Parsed/derived indices:** range-check `stoi`/`atoi` results and catch parse throws; never use a default or derived token id (EOS/BOS/...) as an index without a bounds check. - **Reused/reserved buffers:** recheck bounds after a buffer is shrunk or reused; watch `reserve()` then index-by-assumed-size, and header fields read before their length is checked. @@ -131,6 +132,8 @@ Enforce the `AGENTS.md` / `CONTRIBUTING.md` coding and naming guidelines on ever - Reuse existing infrastructure over introducing new components; no new third-party dependencies, extra headers, or files unless clearly justified. - Keep it simple: a simpler change doing 90% is often preferable to a complex one doing 100%. Flag unnecessary templates/fancy STL; basic `for` loops are fine here. - Every added line should be something the contributor can explain and defend to a reviewer without AI help - flag anything that looks copied-in without understanding. +- `Co-authored-by:` must be reserved for human co-authors; AI contributions (claude, cursor, codex, etc.) must use `Assisted-by:`; if this point is violated, it's a blocking finding. +- Any mentions of Minja must be treated as blocking; see `AGENTS.md` for why. ## Reporting