docs(claude/memory): capture PR-review comment-style feedback
CI / flake (pull_request) Successful in 58s
CI / flake (pull_request) Successful in 58s
Add two memories and correct one existing, from a review of PR comments across multicluster and unified-helm over the past two months: - code_comment_style: no Jira/ticket IDs in code comments by default, keep comments concise and about the non-obvious why, and use # (not Helm template) comments where they must reach the rendered manifest. - copilot_review_false_positives: verify Copilot blocking claims against the spec and live config before acting; records two Terraform FPs. - workflow_review_and_comments: drop the now-contradicted 'one-liner + WSP ticket reference' guidance, which reviewers repeatedly strip.
This commit is contained in:
@@ -0,0 +1,19 @@
|
||||
---
|
||||
name: copilot_review_false_positives
|
||||
description: "Verify Copilot PR-review 'this breaks X' claims against spec/live config before acting; two recorded false positives"
|
||||
metadata:
|
||||
node_type: memory
|
||||
type: feedback
|
||||
originSessionId: 59e09a3f-1429-4f68-a5fb-9af4390e9b0d
|
||||
---
|
||||
|
||||
The Copilot reviewer on the `multicluster` / `unified-helm` repos raises blocking-sounding "this will fail" claims that are sometimes wrong. Verify against the language spec and the live/`master` config before treating one as real or applying its fix.
|
||||
|
||||
Recorded false positives (both Terraform, both Emma-flagged "for future reference"):
|
||||
|
||||
- **PR #1742** — claimed `var.map.hyphenated-key` dot access is parsed as subtraction and breaks `terraform plan`. False: HCL2 identifiers may contain hyphens (`ID_Start (ID_Continue | "-")*`), and the same pattern is already live on `master` in prod. Bracket indexing was adopted anyway as marginally clearer, not as a fix.
|
||||
- **PR #1745** — claimed the `aks_pools` per-pool `max_surge` lookup was off-by-one and should use `count.index + 1`. False: every config attribute on that resource indexes with `count.index`; only the cosmetic `name`/`az_nodepool` label uses `+1`. Applying `+1` would have introduced a real bug (wrong pool, and out-of-bounds on the last pool).
|
||||
|
||||
**Why:** Blindly applying a plausible-but-wrong Copilot suggestion can introduce a real defect or waste review cycles.
|
||||
|
||||
**How to apply:** For any Copilot claim that code is broken or unsafe, confirm it against the relevant spec and the existing working config first; if it's wrong, say so plainly on the PR and leave the code. Genuine Copilot catches (over-broad WAF `@beginsWith`, missing input validation, doc/behaviour drift) still get fixed. Relates to [[workflow-review-and-comments]] and [[code_comment_style]].
|
||||
Reference in New Issue
Block a user