feat(claude): two-agent post-commit review pair for lfx-self-serve#707
Open
josep-reyero wants to merge 29 commits into
Open
feat(claude): two-agent post-commit review pair for lfx-self-serve#707josep-reyero wants to merge 29 commits into
josep-reyero wants to merge 29 commits into
Conversation
LFXV2-1827 — Phase 0 of the LFX Skills Refactor plan. - Add .claude/skills/lfx-self-serve-self-review/SKILL.md: a forked-context pre-PR self-review skill (context: fork, agent: code-standards-enforcer). Seven phases: parse base + compute local diff, load rules/hooks/arch docs/checklists, validate local PR-shape sanity (branch name, JIRA, conventional commits, rebased, DCO + GPG signing), flag protected files, run code-standards enforcement, validate upstream API contracts, emit a structured findings report with verdict NOT READY / READY WITH CHANGES / READY. Autonomous — no /review chaining, no gh pr mutation. - Symlink the four shared checklists (backend, frontend, shared-and-sql, docs) from lfx-review-pr/references/ so they stay single-source. - CLAUDE.md: replace "Pre-PR validation" with "Work cycle — mandatory before opening a PR". Leads with a CRITICAL callout that running the self-review is non-negotiable; positions /preflight as the downstream mechanical check. Add a non-negotiable entry to "What NOT to do". - skill-guidance.md: route "ready for PR" / "is my branch ready?" intents to /lfx-self-serve-self-review. Disambiguate /lfx-review-pr as the existing-PR-by-number skill. /lfx-review-pr is intentionally left unchanged — it is invoked by a reviewer in a fresh session against an open PR, so the cold-context property comes for free from session boundaries and the existing in-main orchestration preserves enforcer/metadata parallelism. Plan: lfx-architecture-scratch/2026-05-DevX-Time-to-Merge/lfx-builder-skills-refactor.md §2 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
LFXV2-1827 — preflight format step added blank lines after markdown headings in the report-structure code block. Whitespace only, no content change. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds parallel post-commit reviewers, a pre-PR "shape" readiness skill and checklist, rewrites the PR-level review skill to gate mutating actions behind explicit approval, and expands the reviewers' knowledge-base documents. ChangesRestructured Review Architecture: Post-Commit + Pre-PR + PR-Level
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CodeRev as lfx-self-serve-code-reviewer
participant LearnRev as lfx-self-serve-learnings-reviewer
participant Readiness as lfx-self-serve-pr-readiness
participant CLAUDE as CLAUDE.md / Skill Guidance
Dev->>CodeRev: invoke post-commit (mode:local)
Dev->>LearnRev: invoke post-commit (mode:local)
CodeRev->>LearnRev: (independent) load KB/checklists
CodeRev-->>Dev: findings
LearnRev-->>Dev: findings
Dev->>Readiness: run pre-PR readiness before PR
Readiness->>CLAUDE: consult `pr-shape.md` and rules
Readiness-->>Dev: verdict (READY / NOT READY / READY WITH CHANGES)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProtected file modified — requires code-owner review.
CLAUDE.mdis listed as a protected file per.claude/rules/development-rules.mdand theguard-protected-files.shhook. This change adds the new mandatory/lfx-self-serve-self-reviewworkflow, which is appropriate, but requires extra review scrutiny and code-owner sign-off before merge. Surface this modification in the PR description and tag a code owner.Note: The PR objectives already mention this is flagged for CODEOWNERS review, so this comment serves as formal documentation in the review.
Based on learnings: "Do not edit CLAUDE.md or other lfx-preflight protected files without code-owner review."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 1, This change modifies the protected file CLAUDE.md and therefore needs a code-owner review: update the PR description to explicitly call out the modification to CLAUDE.md and that you added the new /lfx-self-serve-self-review workflow, then add/mention the appropriate CODEOWNER(s) so they are notified (per .claude/rules/development-rules.md and guard-protected-files.sh) and wait for their approval before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/lfx-self-serve-self-review/SKILL.md:
- Around line 37-47: Add explicit error handling around each git command shown
(git fetch, git rev-parse --abbrev-ref HEAD, git merge-base <base> HEAD, git
diff, git log): after running each command check its exit code/throw and abort
the phase with a clear, specific message mapping failures to causes (e.g.,
"Failed to fetch from origin — check network/credentials" for git fetch; "Failed
to determine current branch" for git rev-parse; "Invalid base branch `<base>` or
not reachable from HEAD" for git merge-base; "Failed to compute diff between
`<base>` and HEAD" for git diff/log). Ensure the skill stops further processing
on any non-zero exit and surfaces the original command error output in the
message for debugging.
- Around line 211-227: The Projects table entry points to
`lfx-v2-project-service` which does not contain gen/http/openapi3.yaml; update
the service table row referencing `lfx-v2-project-service` so it either points
to the correct OpenAPI path in that repo or remove the Projects row entirely;
search the repository for the actual OpenAPI spec filename/path and replace the
table value and any related gh api example (the gh api
repos/linuxfoundation/<repo>/contents/gen/http/openapi3.yaml snippet) with the
correct repo/path if found, and verify the gh api command returns the file (or
delete the entry if no spec exists).
---
Outside diff comments:
In `@CLAUDE.md`:
- Line 1: This change modifies the protected file CLAUDE.md and therefore needs
a code-owner review: update the PR description to explicitly call out the
modification to CLAUDE.md and that you added the new /lfx-self-serve-self-review
workflow, then add/mention the appropriate CODEOWNER(s) so they are notified
(per .claude/rules/development-rules.md and guard-protected-files.sh) and wait
for their approval before merging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f123876b-ef02-4186-9490-e4b51f1e4d1f
📒 Files selected for processing (7)
.claude/rules/skill-guidance.md.claude/skills/lfx-self-serve-self-review/SKILL.md.claude/skills/lfx-self-serve-self-review/references/backend-checklist.md.claude/skills/lfx-self-serve-self-review/references/docs-checklist.md.claude/skills/lfx-self-serve-self-review/references/frontend-checklist.md.claude/skills/lfx-self-serve-self-review/references/shared-and-sql-checklist.mdCLAUDE.md
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new guided Claude skill to perform a local (pre-PR) self-review against repo standards, then updates contributor guidance so this self-review becomes a mandatory step before opening a PR.
Changes:
- Introduces
/lfx-self-serve-self-reviewskill that computes a local diff vs a base branch, runs standards checks, and emits a structured findings report with a verdict. - Updates
CLAUDE.mdworkflow to require the self-review before/preflightand before opening any PR. - Updates skill suggestion guidance to route “ready for PR?” intents to the new self-review skill (and positions
/preflightas the mechanical follow-up).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
CLAUDE.md |
Makes /lfx-self-serve-self-review a mandatory pre-PR step and reorders the “before PR” work cycle. |
.claude/skills/lfx-self-serve-self-review/SKILL.md |
Adds the new self-review skill definition and report/verdict structure. |
.claude/rules/skill-guidance.md |
Updates skill routing so local readiness checks suggest /lfx-self-serve-self-review before /preflight. |
Comments suppressed due to low confidence (2)
.claude/skills/lfx-self-serve-self-review/SKILL.md:239
- Phase 6 says an upstream spec fetch failure should be reported as a WARNING and that “an unverified contract is not a passing check”, but the report structure in Phase 7 only has Critical/Should fix/Nit sections. Please define where these warnings go (new section vs. down-level to SHOULD_FIX) so they don’t get dropped from the structured output.
For direct Snowflake SQL (not proxy calls): verify every `?` placeholder has a corresponding value in the binds array, in the correct order. Bind mismatch → **CRITICAL**.
If `gh api` fails (404, auth, network), include a **WARNING** in the report: "Upstream API contract for `<service>` could not be verified — manual validation required." An unverified contract is **not** a passing check.
.claude/skills/lfx-self-serve-self-review/SKILL.md:288
- Verdict rules don’t currently account for the “unverified upstream contract is not a passing check” case (Phase 6): since WARNING isn’t part of the verdict gating, the skill could still emit
READYeven when upstream validation could not be performed. Consider explicitly stating how unverified upstream contracts affect the verdict (e.g., forceREADY WITH CHANGESorNOT READY).
### Verdict rules
- **NOT READY** — any CRITICAL finding, any unsigned commit, or any confirmed upstream contract mismatch (not just unverified).
- **READY WITH CHANGES** — zero CRITICAL; SHOULD_FIX findings present. Address them or explicitly document the trade-off in the PR description.
- **READY** — zero CRITICAL, zero SHOULD_FIX. NITs are fine to carry forward.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reshape the pre-PR review lifecycle into two specialised skills with a shared code-review subagent. Triad: - /lfx-self-serve-self-review — pre-PR, forked, code-convention audit via the shared code-standards-enforcer agent. - /lfx-self-serve-pr-readiness — pre-PR, forked, no subagent. PR-shape sanity + bot-reviewer simulation against bundled empirical checklists. - /lfx-review-pr — post-PR, main thread. Uses the same agent for code review and owns the post-PR layer (verify prior comments, draft-then-approve, /review invocation). The agent absorbs everything shared between the two code-review consumers: rule library, severity vocab, false-positives, findings JSON shape, mode-dispatched diff computation, upstream API procedure, protected-file flagging via live hook parsing. A mode flag (local|pr) in the first line of the user prompt selects the fetch path; the rest of the playbook is identical. Pre-PR scope split: pr-readiness is the bot-feedback parity layer (catch what CodeRabbit / Copilot would flag before the PR opens); self-review is the project-convention audit. The post-PR reviewer does NOT replicate the bot pass — CodeRabbit and Copilot run their own analysis once the PR is open. Code checklists moved from .claude/skills/lfx-review-pr/references/ to docs/reviews/ — they are project documentation, not skill internals. The agent's system prompt marks them mandatory. Bot-finds: 16 .md files under .claude/skills/lfx-self-serve-pr-readiness/references/bot-finds/, authored from ~30 PRs sampled (2026-03-15 to 2026-05-15) of CodeRabbit + GitHub Copilot inline comments. 55 patterns total, each traced to a specific PR-comment citation or a documented CodeRabbit / Copilot category. Categories: security, type safety, async correctness, accessibility, docs-comments drift, testing, log-metric correctness, cookie trust, RxJS / signals timing, guards / interceptors ordering, route auth surface, query-param hardening, Snowflake row-shape, template binding traps, sanitizer & public URLs, OTEL & rate-limit drift. CLAUDE.md work-cycle gate updated to mandate both pre-PR skills before /preflight and push. LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
josep-reyero
changed the title
…rve-code-reviewer Renames the shared review subagent for clearer scoping and matches the repo-prefixed naming used by the consuming skills. Updates all references across CLAUDE.md, skill bodies, and skill reference files. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 30 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
.claude/skills/lfx-self-serve-self-review/SKILL.md:53
- Phase 2/3 assumes the agent runs PR-shape checks (“Step 8”) and returns
pr-shapefindings, but the agent spec in.claude/agents/lfx-self-serve-code-reviewer.mdstates PR-shape is handled by/lfx-self-serve-pr-readinessand/lfx-review-pr, not the agent. Align this section (and the report template) with the agent’s actual JSON output so the skill doesn’t reference a non-existent category.
The playbook will:
1. Compute the local diff (Step 1, `mode: local` branch).
2. Load CLAUDE.md, `.claude/rules/*.md`, the protected-files hook, conditionally relevant architecture docs, and the four review checklists (Step 2).
3. Audit each changed file against all applicable rules and checklists (Steps 3–5).
4. Validate upstream API contracts for any backend changes (Step 6).
5. Flag protected files (Step 7).
6. Run PR-shape sanity — branch name, JIRA, conventional commits, rebase, diff size, DCO+GPG signing (Step 8, both-modes section only).
7. Return a JSON array of findings with categories `code`, `upstream-api`, `protected-files`, `pr-shape` (Step 9).
.claude/skills/lfx-review-pr/SKILL.md:75
- Phase 3 requires
headRefNametogit show origin/<headRefName>:<file>but also instructs running Phase 3 in parallel with the background agent and says to getheadRefNamefrom the agent’s metadata fetch. The agent output is just findings JSON (no headRefName), so Phase 3 can’t reliably run. Fix by fetching PR metadata (at leastheadRefName/baseRefName) in Phase 1 withgh pr view ..., or by deferring file reads until after the metadata is available from a skill-side fetch.
```bash
gh api repos/{owner}/{repo}/pulls/<N>/comments --paginate
gh api repos/{owner}/{repo}/pulls/<N>/reviews --paginate
- Skip trivial comments: nits, acknowledgments, "+1", bot auto-comments (CodeRabbit / Copilot — these run their own review and don't need verification), and purely informational remarks.
- For every CRITICAL or SHOULD_FIX comment from a human reviewer:
- Read the file on the PR branch:
git show origin/<headRefName>:<file>(use theheadRefNamefrom the agent's PR metadata fetch). - Compare the current code against what the comment requested.
- Read the file on the PR branch:
**.claude/skills/lfx-review-pr/SKILL.md:107**
* Phase 4 references PR title/body coming from `gh pr view --json title,body` in Phase 1, and later checks also need `headRefName`, `baseRefName`, `author`, and additions/deletions. But Phase 1 currently only runs `gh repo view ...` and never fetches PR metadata. Add a Phase 1 `gh pr view <N> --json title,body,headRefName,baseRefName,author,additions,deletions` (or equivalent) so PR-shape checks and the Phase 7 summary have the required inputs.
Walk every check in .claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md against the PR data. This is the same checklist /lfx-self-serve-pr-readiness walks pre-PR, plus the two PR-only items (PR title format and external-refs check) that only apply once a PR exists.
Fetch the additional PR-level data this needs:
gh api repos/{owner}/{repo}/pulls/<N>/commits --paginate --jq '.[].commit.message'
# (PR title and body come from `gh pr view --json title,body` in Phase 1)Then check each item from .claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md:
- Branch name format → from
gh pr view --json headRefName. Match<type>/LFXV2-<number>. - JIRA ticket reference → grep
LFXV2-[0-9]+over commit subjects + bodies + PR body. None → SHOULD_FIX. - Conventional commit on every commit → each commit subject against the regex in
pr-shape.md.choreis invalid. - PR title format (PR-only) →
type(scope): description, lowercase, noLFXV2-XXXin the title,choreinvalid. → SHOULD_FIX. - Branch rebased on base →
git merge-base --is-ancestor origin/<baseRefName> origin/<headRefName>. Non-zero → SHOULD_FIX. - Diff size → if
additions > 1000→ NIT (plus a review-body note — see Additional Rules). - DCO + GPG signing → from
git log --format='%G? %h %s' origin/<baseRefName>..origin/<headRefName>and commit bodies.N/B/Ecodes → CRITICAL. MissingSigned-off-by:→ CRITICAL.
**.claude/skills/lfx-review-pr/SKILL.md:193**
* This note says the diff-size PR-shape NIT “still emits from the agent”, but PR-shape is handled in this skill’s Phase 4 and the `lfx-self-serve-code-reviewer` agent explicitly does not emit PR-shape findings. Update this text to avoid pointing reviewers to a non-existent agent finding source.
If the PR's additions exceed 1000 lines, include in the proposed review body:
Note: This PR has {additions} additions, which exceeds the recommended 1000-line target per
commit-workflow.md. Consider splitting into smaller, independently reviewable PRs.
(The PR-shape NIT for diff size still emits from the agent — this is the human-readable echo in the review body.)
Extra instructions
</details>
Extracts the bot-rubric / knowledge-base portion of /lfx-self-serve-pr-readiness into a new /lfx-self-serve-learnings-review skill, and shifts the work-cycle gate so: - Pre-commit (every commit, parallel): /lfx-self-serve-self-review AND /lfx-self-serve-learnings-review run as forked subagents. - Pre-PR (once): /lfx-self-serve-pr-readiness audits PR shape only (branch, JIRA, commits, DCO + GPG, rebase, diff size). The rename better matches the actual lifecycle — KB review is code-level and benefits from running often on small diffs; PR-shape is a once-per-PR check that doesn't change mid-development. The "learnings" framing leaves room for human-flagged patterns and codebase gotchas alongside the empirical bot patterns. References moved via git mv from pr-readiness/references/ to learnings-review/references/. CLAUDE.md work-cycle section and skill-guidance.md updated to reflect the new triad shape. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…bot-finds → pr-knowledge Consolidates the 16-file bot-finds knowledge base into 8 semantically coherent buckets and renames the directory to `pr-knowledge/` to reflect that the KB now spans both bot-flagged and human-flagged patterns. The 16 → 8 mapping: - security.md ← security + sanitizer-and-public-urls + cookie-trust - typescript-correctness.md ← type-safety + async-correctness - templates-and-accessibility.md ← accessibility + template-binding-traps - frontend-state-and-timing.md ← rxjs-signals-timing - server-request-handling.md ← route-auth-surface + guards-interceptors-ordering + query-param-hardening - observability-and-logging.md ← otel-and-rate-limit-drift + log-metric-correctness - data-and-snowflake.md ← snowflake-rowshape-schema - code-truthiness.md ← docs-comments-drift + testing Dedup merges (2 patterns collapsed; net 55 → 53 patterns): - error-message-identity-leak (security) + error-reveals-auth-state (route-auth-surface) → single entry in security.md (both cite PR #636). - raw-query-param-string-cast (type-safety) + raw-query-string-cast (query-param-hardening) → single entry in server-request-handling.md (both cite PR #665 project.controller.ts:774). Updates SKILL.md Phase 3 routing table, bot-rubric.md category index, and rule-ID prefixes throughout (bot-finds/<file>/<pattern> → pr-knowledge/<file>/<pattern>). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…dge subdir) Moves the 8 pattern files from references/pr-knowledge/ up to references/ to keep the skill structure flat. Anthropic's published skill structure docs only show flat-or-nested-with-scripts/ examples for references/ — no explicit support for nested category folders. The 8 files now sit peer-to-peer with bot-rubric.md and known-false-positives.md. Rule-ID prefixes simplified from pr-knowledge/<file>/<pattern> → <file>/<pattern>; routing-table column header renamed "Bot-find file" → "Pattern file"; SKILL.md, bot-rubric.md, known-false-positives.md, and the architecture-scratch team-review doc all updated. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 23 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
.claude/skills/lfx-self-serve-self-review/SKILL.md:53
- This skill claims the
lfx-self-serve-code-revieweragent performs PR-shape sanity and returnscategory: pr-shapefindings, but the new agent definition explicitly says PR-shape is not the agent’s concern (it’s handled by/lfx-self-serve-pr-readinessand/lfx-review-pr). As a result, the report rendering here will be missing PR-shape results (or will incorrectly assume PASS). Align the skill’s Phase 2 description/output schema with the agent’s actual responsibilities (code/upstream/protected-files only), or move PR-shape into this skill body explicitly.
The playbook will:
1. Compute the local diff (Step 1, `mode: local` branch).
2. Load CLAUDE.md, `.claude/rules/*.md`, the protected-files hook, conditionally relevant architecture docs, and the four review checklists (Step 2).
3. Audit each changed file against all applicable rules and checklists (Steps 3–5).
4. Validate upstream API contracts for any backend changes (Step 6).
5. Flag protected files (Step 7).
6. Run PR-shape sanity — branch name, JIRA, conventional commits, rebase, diff size, DCO+GPG signing (Step 8, both-modes section only).
7. Return a JSON array of findings with categories `code`, `upstream-api`, `protected-files`, `pr-shape` (Step 9).
.claude/skills/lfx-review-pr/SKILL.md:97
- Phase 3/4 require PR metadata (headRefName/baseRefName/author/title/body/additions) to run in parallel with the background agent, but Phase 1 no longer fetches it. As written, Phase 4 references
gh pr view --json title,body“in Phase 1” and Phase 3 needsheadRefName, yet neither is actually gathered until the agent finishes (and the skill doesn’t describe how to extract it from the agent output early). Consider reinstating a lightweightgh pr view <N> --json title,body,headRefName,baseRefName,author,additions,deletionsin Phase 1 (or change Phase 3/4 to wait for the agent result before proceeding).
## Phase 4 — PR-shape pass (parallel with Phase 2)
Walk every check in `.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md` against the PR data. This is the same checklist `/lfx-self-serve-pr-readiness` walks pre-PR, plus the two PR-only items (PR title format and external-refs check) that only apply once a PR exists.
Fetch the additional PR-level data this needs:
```bash
gh api repos/{owner}/{repo}/pulls/<N>/commits --paginate --jq '.[].commit.message'
# (PR title and body come from `gh pr view --json title,body` in Phase 1)
</details>
Renames bot-rubric.md → review-rubric.md and rewrites residual bot framing throughout the KB and skill body: "Bot-flagged patterns" → "Automated review patterns"; "the bots flag" → "CodeRabbit + Copilot flag"; "Bot-quirk noise" → "Review-automation quirks"; etc. The KB now consistently uses pr-learnings / pr-knowledge framing for its own concept, and CodeRabbit + Copilot by name when referring to the upstream tooling whose published rubrics we union. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Resolves five valid bot review comments from CodeRabbit + Copilot:
1. Remove stale "PR-shape sanity" claims from self-review and
lfx-review-pr skill descriptions and bodies — the
lfx-self-serve-code-reviewer agent only returns
code | upstream-api | protected-files categories. PR-shape lives in
/lfx-self-serve-pr-readiness (pre-PR) and is walked by /lfx-review-pr
in its own Phase 4. Self-review report drops the now-empty PR-shape
table.
2. Fix wrong docs/reviews/pr-shape.md path in
.claude/agents/lfx-self-serve-code-reviewer.md — actual location is
.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md.
3. Add staged-but-uncommitted handling to the agent's mode:local — the
pre-commit framing was incompatible with the previous "abort if no
commits between base and HEAD" behaviour. Now audits the union of
committed and staged diffs.
4. Replace ambiguous `$MB` shell-variable pattern with three-dot
`<base>...HEAD` syntax in git diff commands (lets git compute the
merge-base internally — no shared shell variable required). Applied
to the agent, learnings-review Phase 2, and pr-readiness Phase 2.
5. Add explicit base-name normalization step ("if <base> has no slash,
prefix with origin/") so plain `main` doesn't resolve to a possibly-
stale local branch after `git fetch origin`.
Also fixes the failing markdown-lint and Code Quality (prettier) checks
in CI: MD029 ordered-list prefix, MD032 lists-around-blanks, MD040
fenced-code-block language hints, and prettier formatting across the
modified files.
Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…-review Resolves four additional Copilot findings on PR #707: - lfx-review-pr/SKILL.md: Phase 1 now fetches PR metadata (title / body / headRefName / baseRefName / author / additions / deletions) and captures BASE_REF, HEAD_REF, AUTHOR variables. Phases 3–5 reference these consistently instead of pulling unresolved <headRefName> / <author> placeholders that would never be populated when those phases run in parallel with the background agent. (Phase 4 path-references also switched to /tmp/pr-<N>-meta.json captured in Phase 1.) - self-review/SKILL.md: H1 heading renamed from "Pre-PR Self-Review" to "Pre-commit Self-Review" so it matches the frontmatter description and the CLAUDE.md work-cycle positioning. Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Back-fills 2 months of older PR history (2026-01-15 → 2026-03-14) on top of the existing 2-month sample, bringing total coverage to ~4 months (~68 PRs, ~754 inline CodeRabbit + Copilot review comments). Pattern count: 53 → 67. New patterns added to 7 of the 8 category files; 2 new entries also added to known-false-positives.md. Most additions land in security.md (recurring auth-state / public-route patterns) and server-request-handling.md (controller-level findings). Files stay under the ~150-line guidance. Patterns admitted only when they appeared in ≥2 PRs OR were single-PR but high-severity (security / data-integrity / runtime-crash). Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…ta-talk
Moves the unioned CodeRabbit + Copilot review rubric out of
references/review-rubric.md (deleted in the prior commit) into a
dedicated bot-rubric-agent that backs /lfx-self-serve-learnings-review.
The agent's system prompt now carries the severity mapping, 8-bucket
category index, behavioural guidance, what-it-doesn't-cover boundary,
and source provenance — loaded ambiently rather than re-read from a
reference file on every invocation. Mirrors how /lfx-self-serve-self-review
uses lfx-self-serve-code-reviewer.
The learnings-review skill body keeps all its phases (parse args,
compute diff, load references, KB pass, cross-check, false-positives,
render report) — only the rubric content moves out.
Mandatory section rewritten to be explicit that the audit reads ONLY
the pattern files whose "Read when" condition matches the diff, not
every file. Removes the "if you emit findings without reading every
reference relevant to the diff" wording that read as "read everything
just to be safe".
Also drops harness meta-talk ("this skill runs context: fork with no
subagent", "this skill runs in a forked context using the X subagent
type") from learnings-review, self-review, and pr-readiness — the
skill body doesn't need to know its own execution mechanics; that's
the harness's concern.
Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 21 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md:56
- Diff-size check is described as “additions ≤ 1000 lines (per commit-workflow.md)”, but
commit-workflow.mdstates a target of “1000 lines of diff” (which usually means additions+deletions). If you intend to measure only additions, the reference to commit-workflow.md is misleading; otherwise, adjust the check to use total changed lines to match the documented policy.
## 5. `pr-shape/diff-size` — NIT
**Check:** additions ≤ 1000 lines (per `.claude/rules/commit-workflow.md`).
**Failure message:** `This change has <N> additions, exceeding the 1000-line target. Consider splitting into smaller, independently-reviewable PRs.`
…knowledge-base/
The empirical pattern knowledge base lived at .claude/pr-knowledge/ —
buried under Claude-machinery. Moving to docs/reviews/knowledge-base/
puts it where the existing code-convention checklists live (peer to
docs/reviews/{frontend,backend,shared-and-sql,docs}-checklist.md), so
both review surfaces sit together as discoverable team-facing review
documentation rather than agent-internal config.
The lfx-self-serve-learnings-reviewer agent's Step 2 routing table
updated to read from the new path. CLAUDE.md work-cycle, skill-guidance,
and the team-review snapshot all updated to match.
9 KB files renamed via git mv; only path strings changed in the
referencing files.
Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Flip the reviewer pair (lfx-self-serve-code-reviewer + lfx-self-serve-learnings-reviewer) from a pre-commit gate that synchronously blocks the commit window to a post-commit pair spawned in the background via the Agent tool. Devs commit first, fire the pair off with run_in_background: true, and keep building. Findings from a returned pair are rolled into the next commit; the safety bound shifts to the PR boundary, where the queue is drained and any remaining CRITICAL / reasonable SHOULD_FIX must be addressed before opening for review. Both agents already diff cumulatively against origin/main, so a later review pair sees the full state and supersedes any earlier in-flight one — no coordination logic needed. Multiple concurrent pairs are normal during fast iteration. Updates: CLAUDE.md work cycle and "What NOT to do" bullets, .claude/agents/* descriptions and bodies, .claude/rules/skill-guidance.md routing + trigger phrases + cowork section, and .claude/skills/lfx-self-serve-pr-readiness SKILL.md description, body, and companion-skills note. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Both post-commit review subagents had three stale references missed in the pre→post-commit flip (commit e520209): - code-reviewer body line 49 still framed `mode: local` as pre-commit; rewritten to describe the post-commit invocation while preserving mid-edit staged-diff handling. - code-reviewer report-template H1 still printed "(pre-commit)"; flipped to "(post-commit)". - learnings-reviewer report-template H1 still printed "Pre-commit code review"; flipped to "Post-commit code review". Net effect of the drift: every report either agent emitted under the new workflow was self-labeled as pre-commit, contradicting CLAUDE.md and the frontmatter descriptions. Caught by the post-commit review pair spawned against e520209. Also picks up a prettier reflow on .claude/rules/skill-guidance.md table alignment (header row trailing-whitespace) flagged in the same review. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/rules/skill-guidance.md (1)
1-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier formatting check failed — fix required before merge.
The GitHub Actions pipeline reported that this file has code style issues detected by Prettier. Run
yarn format(orprettier --write .claude/rules/skill-guidance.md) to fix the formatting violations, then commit the result.Based on learnings: Pre-commit runs
npx lint-staged(prettier + lint on staged files), and you should not commit before the checks pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/rules/skill-guidance.md around lines 1 - 88, Prettier formatting errors were detected in .claude/rules/skill-guidance.md; run the formatter and commit the fix: execute yarn format (or prettier --write .claude/rules/skill-guidance.md) to rewrite the file, verify npx lint-staged / pre-commit hooks pass locally, then stage and commit the formatted file (do not bypass pre-commit checks).
🧹 Nitpick comments (2)
docs/reviews/knowledge-base/server-request-handling.md (1)
65-76: ⚡ Quick winStandardize spelling variant: "normalization" vs "normalisation".
Line 71 mixes both "normalization" (American English) and "normalisation" (British English) in the same sentence within the empirical citation. For consistency, use one variant throughout.
📝 Suggested fix
-**Empirical citation:** PR `#272` `apps/lfx-one/src/server/services/meeting.service.ts:696` — "Email matching against query-service tags is case-sensitive. Here `email` is taken directly from the OIDC claims and passed into `getMeetingRegistrantsByEmail` without normalization, while other code paths lower-case emails before comparing/filtering." Also flagged in PR `#249` `public-meeting.controller.ts:66` — "The email should be lowercased before passing to `addInvitedStatusToMeeting` for consistent tag matching." +**Empirical citation:** PR `#272` `apps/lfx-one/src/server/services/meeting.service.ts:696` — "Email matching against query-service tags is case-sensitive. Here `email` is taken directly from the OIDC claims and passed into `getMeetingRegistrantsByEmail` without normalisation, while other code paths lower-case emails before comparing/filtering." Also flagged in PR `#249` `public-meeting.controller.ts:66` — "The email should be lowercased before passing to `addInvitedStatusToMeeting` for consistent tag matching."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reviews/knowledge-base/server-request-handling.md` around lines 65 - 76, The document mixes "normalization" and "normalisation"; pick one variant and make all occurrences consistent (e.g., change every "normalisation" to "normalization" or vice versa) within this review file and related sentences that reference functions like getMeetingRegistrantsByEmail and addInvitedStatusToMeeting and the user.controller.ts citation so the empirical citation and the Fix paragraph use the same spelling throughout.docs/reviews/knowledge-base/code-truthiness.md (1)
51-62: ⚡ Quick winStandardize spelling variant: "optimization" vs "optimisation".
Line 57 uses "optimization" (American English) while line 61 uses "optimisation" (British English). For consistency within the document, pick one variant and use it throughout.
📝 Suggested fix
-**Empirical citation:** PR `#247` `apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts:27` (also `:42`) — "The PR description mentions adding an `isPlatformBrowser` guard to prevent wasted SSR calls, and `PLATFORM_ID` is injected on line 27, but it's never actually used in the code. The `initProjects` method should check if it's running in the browser before making the HTTP call to properly implement the SSR optimization mentioned in the PR summary." +**Empirical citation:** PR `#247` `apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.ts:27` (also `:42`) — "The PR description mentions adding an `isPlatformBrowser` guard to prevent wasted SSR calls, and `PLATFORM_ID` is injected on line 27, but it's never actually used in the code. The `initProjects` method should check if it's running in the browser before making the HTTP call to properly implement the SSR optimisation mentioned in the PR summary." -**Failure message:** `PLATFORM_ID` injected but `isPlatformBrowser` guard never called — SSR optimisation claimed but missing. +**Failure message:** `PLATFORM_ID` injected but `isPlatformBrowser` guard never called — SSR optimisation claimed but missing. -**Fix:** either add the guard (`if (isPlatformBrowser(this.platformId)) { this.initProjects(); }`) or update the comment / PR description to accurately reflect what's optimised (e.g., `shareReplay(1)` caching only). +**Fix:** either add the guard (`if (isPlatformBrowser(this.platformId)) { this.initProjects(); }`) or update the comment / PR description to accurately reflect what's optimised (e.g., `shareReplay(1)` caching only).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reviews/knowledge-base/code-truthiness.md` around lines 51 - 62, Choose one spelling variant ("optimization" or "optimisation") and make it consistent throughout the document `code-truthiness/ssr-guard-claimed-but-not-implemented`; update every occurrence (e.g., the Pattern/Failure message that currently reads "SSR optimisation" and the Empirical citation / Fix text that uses "optimization") to the chosen variant so the pattern name, failure message, and explanatory text all use the same spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/lfx-self-serve-code-reviewer.md:
- Line 49: Replace the incorrect phrase "This mode runs **pre-commit**" with
"post-commit" in the sentence that begins "This mode runs ..." inside
.claude/agents/lfx-self-serve-code-reviewer.md; also adjust the following clause
to clarify that the staged diff is included to cover the first commit on a new
branch (where there is no prior commit) rather than implying the agent runs
before commit—which aligns the sentence with the agent description ("post-commit
in the background") and the mode description ("post-commit review").
In `@CLAUDE.md`:
- Around line 258-297: The change touches a protected file (CLAUDE.md) which per
the repo policy requires explicit code-owner review; update the PR to either
remove the edit or explicitly surface this modification by adding a clear note
in the PR description and requesting review from the CLAUDE.md code-owners
(follow the lfx-preflight/code-owner workflow), and ensure the PR includes the
code-owner approval before merging so the modification to CLAUDE.md is accepted.
---
Outside diff comments:
In @.claude/rules/skill-guidance.md:
- Around line 1-88: Prettier formatting errors were detected in
.claude/rules/skill-guidance.md; run the formatter and commit the fix: execute
yarn format (or prettier --write .claude/rules/skill-guidance.md) to rewrite the
file, verify npx lint-staged / pre-commit hooks pass locally, then stage and
commit the formatted file (do not bypass pre-commit checks).
---
Nitpick comments:
In `@docs/reviews/knowledge-base/code-truthiness.md`:
- Around line 51-62: Choose one spelling variant ("optimization" or
"optimisation") and make it consistent throughout the document
`code-truthiness/ssr-guard-claimed-but-not-implemented`; update every occurrence
(e.g., the Pattern/Failure message that currently reads "SSR optimisation" and
the Empirical citation / Fix text that uses "optimization") to the chosen
variant so the pattern name, failure message, and explanatory text all use the
same spelling.
In `@docs/reviews/knowledge-base/server-request-handling.md`:
- Around line 65-76: The document mixes "normalization" and "normalisation";
pick one variant and make all occurrences consistent (e.g., change every
"normalisation" to "normalization" or vice versa) within this review file and
related sentences that reference functions like getMeetingRegistrantsByEmail and
addInvitedStatusToMeeting and the user.controller.ts citation so the empirical
citation and the Fix paragraph use the same spelling throughout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3ceea08-12c8-4cf3-b037-c9a046382a60
📒 Files selected for processing (21)
.claude/agents/code-standards-enforcer.md.claude/agents/lfx-self-serve-code-reviewer.md.claude/agents/lfx-self-serve-learnings-reviewer.md.claude/rules/skill-guidance.md.claude/skills/lfx-review-pr/SKILL.md.claude/skills/lfx-self-serve-pr-readiness/SKILL.md.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.mdCLAUDE.mddocs/reviews/backend-checklist.mddocs/reviews/docs-checklist.mddocs/reviews/frontend-checklist.mddocs/reviews/knowledge-base/code-truthiness.mddocs/reviews/knowledge-base/data-and-snowflake.mddocs/reviews/knowledge-base/frontend-state-and-timing.mddocs/reviews/knowledge-base/known-false-positives.mddocs/reviews/knowledge-base/observability-and-logging.mddocs/reviews/knowledge-base/security.mddocs/reviews/knowledge-base/server-request-handling.mddocs/reviews/knowledge-base/templates-and-accessibility.mddocs/reviews/knowledge-base/typescript-correctness.mddocs/reviews/shared-and-sql-checklist.md
💤 Files with no reviewable changes (1)
- .claude/agents/code-standards-enforcer.md
✅ Files skipped from review due to trivial changes (5)
- docs/reviews/knowledge-base/observability-and-logging.md
- docs/reviews/knowledge-base/data-and-snowflake.md
- docs/reviews/knowledge-base/templates-and-accessibility.md
- docs/reviews/knowledge-base/typescript-correctness.md
- docs/reviews/knowledge-base/frontend-state-and-timing.md
josep-reyero
changed the title
Add a new SHOULD_FIX entry to docs/reviews/knowledge-base/security.md: `security/prototype-pollution-via-dynamic-key-assignment`. Captures the pattern that PR #673 surfaced via Copilot — a recursive plain-object walker assigning keys via `result[key] = ...` over `Object.entries` can be coerced into `Object.prototype` pollution if the input carries an own-property literally named `__proto__`, `constructor`, or `prototype`. A plain-object guard on the input does NOT close the hole; dangerous keys still get copied through. Surfaced during the calibration test of the new post-commit reviewer pair against PR #673 — bots on the real PR caught this, our pair did not, so the KB needs the pattern entry for future cross-check pickup. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reviews/knowledge-base/security.md (1)
202-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix markdown formatting to clear pipeline warnings.
Quality Checks report Prettier style issues for this file; please run Prettier and commit the formatting-only update (likely EOF/newline/wrapping cleanup).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reviews/knowledge-base/security.md` at line 202, Run Prettier on the affected markdown file and commit a formatting-only update to resolve the Prettier style warnings: ensure EOF newline, fix wrapping/line length issues, and apply consistent Markdown formatting (e.g., headers, lists, code fences) so the Quality Checks pass; make this a standalone commit titled something like "chore: format markdown (Prettier)" and do not change content other than whitespace/formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reviews/knowledge-base/security.md`:
- Line 159: The guidance incorrectly advises binding a
DomSanitizer.bypassSecurityTrustUrl result via [innerHTML]; change the text to
remove any reference to using [innerHTML] with bypassSecurityTrustUrl and
instead instruct to bind the trusted value to [href] or [attr.href] (not
[innerHTML]), mention that [innerHTML] is for bypassSecurityTrustHtml, and be
explicit about the security trade-off—only use
DomSanitizer.bypassSecurityTrustUrl for known-safe URLs you constructed (never
untrusted user input).
- Around line 67-68: Update the paragraph about [innerHTML] to state Angular's
actual default behavior: Angular automatically sanitizes bindings in the HTML
security context (e.g., [innerHTML]) and strips dangerous constructs like
<script> and executable attributes, so unsafe payloads do not survive by
default; clarify that the real vulnerability is when code explicitly bypasses
the sanitizer (e.g., using DomSanitizer.bypassSecurityTrustHtml or similar APIs)
with untrusted data, and adjust the wording to replace claims that Angular
"doesn't auto-sanitize" or that scripts "can survive" with this corrected
explanation.
---
Outside diff comments:
In `@docs/reviews/knowledge-base/security.md`:
- Line 202: Run Prettier on the affected markdown file and commit a
formatting-only update to resolve the Prettier style warnings: ensure EOF
newline, fix wrapping/line length issues, and apply consistent Markdown
formatting (e.g., headers, lists, code fences) so the Quality Checks pass; make
this a standalone commit titled something like "chore: format markdown
(Prettier)" and do not change content other than whitespace/formatting.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a612a8f2-5b90-4b7f-9b50-58b536a878a5
📒 Files selected for processing (1)
docs/reviews/knowledge-base/security.md
…urce Three evidence-fidelity fixes flagged by the post-commit code-reviewer on 71f6659: - Drop the fabricated `:32` line anchor. The Copilot comment on PR #673 has `line: null` (file-level after rebase) with `original_line: 32` — citing it as `object.utils.ts:32` is half-true and inconsistent with the rest of the KB's anchoring convention. Cite the file only. - Replace the silently-paraphrased Copilot quote tail with the verbatim text. The original ends with the prototype-pollution rationale and the `Object.create(null)` / "safely defining dangerous keys" recommendations; the paraphrase mangled both inside quote marks. - Reorder the Fix to lead with `Object.defineProperty(result, key, ...)` — this is what PR #673 actually merged, and the in-code comment ("Use defineProperty to bypass setters (e.g. __proto__) and prevent prototype pollution when keys come from untrusted sources like JSON.parse") is now quoted as the empirical rationale. Explicit-skip and `Object.create(null)` stay as alternatives. JIRA: LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/reviews/knowledge-base/security.md (2)
67-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrect the Angular [innerHTML] sanitization claim (per prior review).
This text still incorrectly states that Angular doesn't auto-sanitize
[innerHTML]and that dangerous payloads "can survive." As confirmed by the prior review's web search, Angular automatically sanitizes[innerHTML]bindings in the HTML security context, explicitly removing<script>tags and executable attributes by default. The XSS risk arises only when code explicitly bypasses sanitization (e.g., usingbypassSecurityTrustHtmlwith untrusted data), not from Angular's default behavior. This inaccuracy will mislead reviewers using this KB.📝 Suggested correction
-**Pattern:** `[innerHTML]="userField"` binding where `userField` is user-provided content (vote description, meeting description, comment body) without sanitization. Angular doesn't auto-sanitize `[innerHTML]` for trusted-HTML; it strips at runtime but `<script>` tags / `onerror=` attributes / encoded JS payloads can survive. +**Pattern:** `[innerHTML]="userField"` binding where `userField` is user-provided content (vote description, meeting description, comment body) and the code explicitly bypasses Angular's sanitizer. Angular auto-sanitizes `[innerHTML]` by default in the HTML security context, removing `<script>` tags and dangerous executable attributes. The XSS risk occurs when developers use `DomSanitizer.bypassSecurityTrustHtml` (or similar bypass APIs) with untrusted user content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reviews/knowledge-base/security.md` around lines 67 - 68, Update the paragraph describing the `[innerHTML]` pattern to state that Angular automatically sanitizes `[innerHTML]` in the HTML security context (removing `<script>` tags and executable attributes) and that XSS only becomes a risk when sanitization is intentionally bypassed (for example via DomSanitizer.bypassSecurityTrustHtml or similar APIs) or when unsafe content is otherwise inserted into a trusted DOM context; replace the claim that Angular “doesn't auto-sanitize” and that payloads “can survive” with this corrected explanation and add a short note linking the risky exception (bypassSecurityTrustHtml) so reviewers know when to be concerned.
159-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
[innerHTML]from the bypassSecurityTrustUrl guidance (per prior review).This line still incorrectly suggests binding
bypassSecurityTrustUrlvia[innerHTML]. As confirmed by the prior review's web search of Angular documentation,bypassSecurityTrustUrlis specifically for the URL security context and must bind to[href]or[attr.href], never[innerHTML]. The[innerHTML]binding is for the HTML security context and requiresbypassSecurityTrustHtmlinstead. This incorrect guidance will cause implementation errors.📝 Suggested correction
-**Fix:** use `DomSanitizer.bypassSecurityTrustUrl(url)` to mark the URL as safe, bind via `[innerHTML]` or `[attr.href]`. Be explicit about the security trade-off; only do this for known-safe URLs you constructed (not user input). +**Fix:** use `DomSanitizer.bypassSecurityTrustUrl(url)` to mark the URL as safe, then bind to `[href]` or `[attr.href]`. (Note: `[innerHTML]` requires `bypassSecurityTrustHtml`, not `bypassSecurityTrustUrl`.) Be explicit about the security trade-off; only do this for known-safe URLs you constructed (not user input).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reviews/knowledge-base/security.md` at line 159, The guidance incorrectly recommends binding DomSanitizer.bypassSecurityTrustUrl via [innerHTML]; update the text to remove any suggestion of using [innerHTML] with bypassSecurityTrustUrl and explicitly state that bypassSecurityTrustUrl must be bound to [href] or [attr.href] (or used in URL contexts) while bypassSecurityTrustHtml is the correct sanitizer for HTML content and [innerHTML]; keep the security note that these bypasses should only be used for known-safe, constructed URLs (not user input).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reviews/knowledge-base/security.md`:
- Around line 1-201: The markdown has many lines exceeding Prettier's
88-character limit (e.g., the "Security" header and the `security/*` pattern
sections like `security/secrets-in-diff`,
`security/public-meeting-visibility-filter`,
`security/href-from-untrusted-source`), so reformat the file with Prettier to
wrap long lines and fix markdown formatting; run the project formatter (yarn
prettier --write) on the file, re-check that all long lines in the `Security`
section and each `security/...` pattern paragraph are wrapped to <=88 chars, and
commit the formatted result.
---
Duplicate comments:
In `@docs/reviews/knowledge-base/security.md`:
- Around line 67-68: Update the paragraph describing the `[innerHTML]` pattern
to state that Angular automatically sanitizes `[innerHTML]` in the HTML security
context (removing `<script>` tags and executable attributes) and that XSS only
becomes a risk when sanitization is intentionally bypassed (for example via
DomSanitizer.bypassSecurityTrustHtml or similar APIs) or when unsafe content is
otherwise inserted into a trusted DOM context; replace the claim that Angular
“doesn't auto-sanitize” and that payloads “can survive” with this corrected
explanation and add a short note linking the risky exception
(bypassSecurityTrustHtml) so reviewers know when to be concerned.
- Line 159: The guidance incorrectly recommends binding
DomSanitizer.bypassSecurityTrustUrl via [innerHTML]; update the text to remove
any suggestion of using [innerHTML] with bypassSecurityTrustUrl and explicitly
state that bypassSecurityTrustUrl must be bound to [href] or [attr.href] (or
used in URL contexts) while bypassSecurityTrustHtml is the correct sanitizer for
HTML content and [innerHTML]; keep the security note that these bypasses should
only be used for known-safe, constructed URLs (not user input).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9f3966a-dae7-4f08-b641-afa5fdc97c1a
📒 Files selected for processing (1)
docs/reviews/knowledge-base/security.md
Verified each finding against the source before accepting. Six fixes,
one removal, two stale-reply candidates (replies handled separately,
not in this commit).
1. `.claude/skills/lfx-review-pr/SKILL.md` — three fixes Copilot flagged:
- Phase 1 now adds `files` to `gh pr view --json` so Phase 4's
external-refs check has the changed-file list it needs.
- Document explicitly that Bash tool invocations are isolated — env
vars do NOT survive across calls. Each later phase that uses
BASE_REF/HEAD_REF/AUTHOR re-derives them via `jq -r` from
/tmp/pr-<N>-meta.json at the top of its own Bash invocation.
- Phase 4 commit-subject extraction now uses
`git log --format='%H %s' "origin/$BASE_REF..origin/$HEAD_REF"`
instead of `gh api .../commits --jq '.[].commit.message'`. The
latter returns the full message (subject + body) which false-fails
the conventional-commit regex on body content.
2. `.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md`
+ `CLAUDE.md` — Copilot caught a real factual error: both files
claimed commitlint enforces a 72-character header cap, but
commitlint.config.mjs only extends @commitlint/config-angular and
doesn't override `header-max-length` (config-angular default is 100).
Empirical confirmation: 20 of the last 50 commits on main have
subjects > 72 chars (up to 79) — they all merged. The 72 is a team
style guideline, not a commitlint enforcement. Both files now say so.
3. `docs/reviews/knowledge-base/known-false-positives.md` — remove the
"Generated with Claude Code attribution missing" entry. Copilot
flagged it as contradicting `.claude/rules/development-rules.md`
("Always prepend 'Generated with [Claude Code]' if you assisted with
the code"). Empirical confirmation: 29 source files under
`apps/lfx-one/src/` currently carry the attribution. The
false-positive filter would have suppressed legitimate findings on
AI-assisted files. Drop the entry; let the bots flag it; humans judge
per-file based on whether the file was AI-assisted.
4. `docs/reviews/knowledge-base/security.md` — two CodeRabbit Major
findings on KB accuracy plus a CI prettier failure:
- `[innerHTML]` pattern wording rewritten. Old text said "Angular
doesn't auto-sanitize `[innerHTML]`" — wrong. Angular's default
HTML sanitizer (SecurityContext.HTML, active on `[innerHTML]`)
removes `<script>` and dangerous attributes. The real risk surface
the entry captures is (a) `DomSanitizer.bypassSecurityTrustHtml`
disabling sanitization, or (b) user content flowing into a code
path where bypass is later added. Reworded accordingly; severity
unchanged.
- `bypassSecurityTrustUrl` fix wording corrected. Old text said to
"bind via `[innerHTML]` or `[attr.href]`" — `[innerHTML]` is wrong;
it pairs with `bypassSecurityTrustHtml`, not the URL variant. Now
bind via `[href]` or `[attr.href]`, never `[innerHTML]`.
- Prettier reformatted four lines (two semantic edits + two
unrelated `*...*` → `_..._` italic-style conversions). File now
passes `prettier --check`.
5. `docs/reviews/knowledge-base/typescript-correctness.md` — Copilot
caught the KB recommending a non-existent helper (`parseLocalDate(s)`
in `@lfx-one/shared/utils/date`). The shared package already exposes
`parseLocalDateString(dateStr): Date` in
`packages/shared/src/utils/date-time.utils.ts` (re-exported via
`@lfx-one/shared/utils`). Fix now points to the existing helper.
Two threads (CLAUDE.md outdated KB path, learnings-reviewer.md:56 KB
path) refer to claims about old paths (`.claude/pr-knowledge/`) that
were already updated in commit 11d5ba8 — those are stale and will be
replied/resolved without a code change.
JIRA: LFXV2-1827
Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
Phase 1 of `/lfx-review-pr` previously did `git fetch origin "$BASE_REF" "$HEAD_REF"` and downstream phases read files via `git show "origin/$HEAD_REF:<file>"`. That assumes the PR's head ref is reachable under `origin/*`, which is false for PRs opened from forks — `headRefName` points to a branch on the fork's remote, not the upstream `origin`. Switched to GitHub's `pull/<N>/head` refspec, which mirrors the PR head into the upstream repo regardless of whether the PR came from a fork. The fetch creates a local ref at `refs/pr/<N>/head`, and all downstream phases (3, 4, including merge-base / git log / git show) reference it uniformly. `HEAD_REF` is kept in the shell-state caveat only because the branch-name regex check (Phase 4) still needs the literal branch name. Resolves the final Copilot finding on PR #707 (.claude/skills/lfx-review-pr/SKILL.md:48). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…hape The previous commit (38e586a) switched `/lfx-review-pr`'s skill body to use GitHub's `pull/<N>/head` refspec so the PR-head fetch works for fork PRs. Two downstream consumers still carried the old `origin/<headRefName>` pattern and would re-introduce the same fork-PR bug: - `.claude/agents/lfx-self-serve-code-reviewer.md` `mode: pr` — `/lfx-review-pr` Phase 2 spawns this exact agent, so on a fork PR the agent's diff fetch + `git show` reads would fail (or silently audit a stale local ref). Now uses the same refspec + `refs/pr/<N>/head`. - `.claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md:43` post-PR rebase recipe — still showed the legacy form. Updated to use `refs/pr/<N>/head` and pointed contributors at the skill that mediates the fetch. Also dropped the unused `isCrossRepository` field from the skill's metadata fetch (the refspec approach deliberately works uniformly so nothing branches on the fork flag), and added a note explaining why the local destination namespace is `refs/pr/<N>/head` (no collision with `refs/heads/` or `refs/remotes/origin/`). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
The CRITICAL banner above already states what the rationale subsection explained (post-commit + drain at PR boundary, work cycle bound by the PR not the commit). The "Post-commit (after every commit, parallel, asynchronous)" and "Pre-PR (drain the queue, then open)" subsections below restate the same operating model in actionable form. Removing the rationale tightens the work-cycle block to two purely-operational subsections without losing any guidance. Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…Service API Two new Copilot findings on PR #707 pointed out stale identifiers in the post-commit reviewer's knowledge base: - `server-request-handling.md` referenced `apps/lfx-one/src/server/helpers/validation.ts` in two places (the Fix for `raw-query-string-cast` at line 131 and the Detect for `missing-typeof-string-validation` at line 153). The file is actually `validation.helper.ts`. Updated both occurrences. - `templates-and-accessibility.md` recommended `LensService.getCurrentLens()` to fetch the active lens, but that method doesn't exist. `LensService` exposes `activeLens: Signal<Lens>` (at `apps/lfx-one/src/app/shared/services/lens.service.ts:23`). Rewrote the Fix to point at the signal call. The post-commit reviewer pair would have cited these files in future reviews and sent contributors searching for non-existent paths / APIs. Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
…t guidance The post-commit reviewer pair (`lfx-self-serve-code-reviewer` + `lfx-self-serve-learnings-reviewer`) is now scoped explicitly to the pre-PR phase. Once the PR is open and the bot-iteration loop is live, CodeRabbit + Copilot auto-trigger on every push and become the audit surface — stacking the subagent pair on top adds latency without proportional signal and makes the iteration loop too slow. Changes: - CLAUDE.md "Work cycle" CRITICAL banner now has two paragraphs: the pre-PR mandate (unchanged in spirit) and the post-PR carve-out. - New "Post-PR iteration" subsection captures the bot-driven loop: wait for bots, triage, fix(review) commit, reply + resolve, push, repeat. - "What NOT to do" bullets re-scoped to make the pre-PR/post-PR boundary explicit. - `.claude/rules/skill-guidance.md` mirrors the carve-out in the subagent table, trigger phrases, and Cowork-sessions guidance. - New "Lean on subagents" paragraph in Working mode points at the full delegation surface (Explore for searches, parallel Agent calls for independent investigations, context-heavy reads, the LFX reviewer pair). - Tightened the additions to keep total CLAUDE.md line count flat: banner rephrased, redundant Post-commit step 6 folded into step 5, closing paragraph dropped (its content lives in the banner now). Refs LFXV2-1827 Signed-off-by: Josep Garcia-Reyero Sais <josepreyero@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reshape the lfx-self-serve review lifecycle around a two-subagent post-commit pair (spawned in the background via the Agent tool with
run_in_background: true) plus a single PR-shape skill at pre-PR time. Goal: catch what reviewers (humans + CodeRabbit + Copilot) flag before the PR opens, so reviewer time goes to architecture and product judgement rather than mechanical pattern enforcement — and do it without parking the developer in a synchronous wait.The shape
lfx-self-serve-code-reviewersubagent_type:+mode: local,run_in_background: true. Audits.claude/rules/, the fourdocs/reviews/checklists, architecture docs, upstream API contracts, protected files.lfx-self-serve-learnings-reviewersubagent_type:+base/extra,run_in_background: true. Comprehensive code-review rubric (security / performance / code quality / architecture / testing) cross-checked against thedocs/reviews/knowledge-base/empirical pattern KB./lfx-self-serve-pr-readiness/lfx-review-prlfx-self-serve-code-reviewerinmode: pr(returns JSON); composes a draft review with prior-comment verification, PR-shape, new-contributor tone; posts only after explicit user approval. Fetches the PR head viapull/<N>/headso it works uniformly for fork PRs.Why agents and not skills for the review pair? Skills run synchronously and can't be invoked in parallel — the Agent tool's
run_in_background: trueis the only way to get genuinely concurrent reviews that don't block the dev session. Each agent runs in its own fresh forked context (no dev-thread bias) and they execute side-by-side instead of one-after-another.Why post-commit? Both agents diff cumulatively against
origin/main, so a later review pair sees the full state and supersedes any earlier in-flight one. Commit first, fire the pair off in the background, keep building. The safety bound is the PR boundary, not the commit boundary — nothing leaves the branch without a clean final review, but the branch can hold transient reviewed-and-pending-fix state. Background execution actually overlaps with dev work that way.Why pre-PR only? Once the PR is open, CodeRabbit + Copilot auto-trigger on every push and become the live audit surface. Stacking subagent audits on top of bot audits adds latency without proportional signal — the pair is pre-PR insurance, the bots own the post-PR loop.
Why split the dev side from the review side
CLAUDE.md +
.claude/rules/+/develop's instructions already feed every dev session ~1.3K lines, plus the architecture and external API docs they cross-reference. The pattern knowledge base adds another ~1K (68 patterns across 8 categories + the senior-engineer review rubric), and it'll keep growing as we fold in more human-flagged patterns and codebase gotchas. Bolting all of that into the dev prompt would make it much heavier and harder for Claude to actually follow.The two post-commit subagents instead spawn in parallel in their own forked contexts — so it's not tedious for the developer, and the KB lives where it's actually used.
Knowledge base — 4-month back-filled, 68 patterns
The
lfx-self-serve-learnings-revieweragent cross-checks findings againstdocs/reviews/knowledge-base/— eight per-category pattern files (security.md,typescript-correctness.md,templates-and-accessibility.md,frontend-state-and-timing.md,server-request-handling.md,observability-and-logging.md,data-and-snowflake.md,code-truthiness.md) plusknown-false-positives.md.The 68 patterns were canonicalised from ~68 PRs sampled over 2026-01-15 → 2026-05-15 (~4 months) and ~754 inline CodeRabbit + GitHub Copilot review comments. Promotion rule: appeared in ≥2 PRs, OR single-PR with high severity (security / data-integrity / runtime-crash / SSR-crash). Cross-check discipline drops any finding the agent emits that can't be traced back to a quotable pattern entry.
What lives where
.claude/agents/lfx-self-serve-code-reviewer.md— repo-convention code-review playbook withmode: local/mode: prdispatch..claude/agents/lfx-self-serve-learnings-reviewer.md— senior-engineer review rubric + KB routing + cross-check + markdown rendering. Self-contained.docs/reviews/knowledge-base/— 8 pattern files +known-false-positives.md(the empirical KB). Peer to the convention checklists, team-discoverable.docs/reviews/{frontend,backend,shared-and-sql,docs}-checklist.md— the four code-convention checklists (canonical location, code-reviewer agent reads from here)..claude/skills/lfx-self-serve-pr-readiness/references/pr-shape.md— PR-shape checklist (walked by pr-readiness pre-PR and/lfx-review-prpost-PR).CLAUDE.mdwork-cycle gate — mandates the post-commit subagent pair on every pre-PR commit, drains the queue clean before opening, requires/lfx-self-serve-pr-readiness+/preflightto pass, and switches to the bot-iteration loop (no subagent pair) once the PR is open.Team-review snapshot
Human-readable snapshot of the KB at
lfx-architecture-scratch/2026-05-DevX-Time-to-Merge/lfx-self-serve-pr-learnings.md— open for team review (flag patterns to add, drop, or revise).Work cycle (CLAUDE.md)
Pre-PR (asynchronous, while branch is local). After every commit, issue two
Agenttool calls in a single message, both withrun_in_background: true:subagent_type: lfx-self-serve-code-reviewer, prompt:"mode: local\nbase: origin/main\nextra: <focus>"→ renders a code-convention review.subagent_type: lfx-self-serve-learnings-reviewer, prompt:"base: origin/main\nextra: <focus>"→ renders a comprehensive rubric review with KB cross-check.Both run concurrently in fresh forked contexts. Keep working on the next commit while they run. When the pair returns, roll every CRITICAL and reasonable SHOULD_FIX into the next commit. Multiple in-flight pairs are normal; the latest returned pair is authoritative (cumulative diff against
origin/main).Pre-PR drain: when the work is done, wait for the latest in-flight pair, fix any remaining findings, rerun until clean, then
/lfx-self-serve-pr-readiness→/preflight→ push → open.Post-PR iteration: CodeRabbit + Copilot auto-trigger on every push. Wait for bot comments, triage CRITICAL / reasonable SHOULD_FIX, push a
fix(review): ...commit, reply + resolve threads, repeat until clean. No subagent pair on these commits — the bots own the audit surface once the PR exists.Test plan
lfx-self-serve-code-reviewer(mode: local) against a branch with a fresh commit — confirm it diffs cumulatively againstorigin/mainand renders a code-only verdict.lfx-self-serve-learnings-revieweragainst the same branch — confirm it loads only the pattern files whose Read-when condition matches the changed-file paths, applies the rubric, and drops known false positives.run_in_background: true— confirm the dev session is not blocked: it can continue tool calls while the agents run./lfx-self-serve-pr-readiness— verify it walkspr-shape.mdand emits PR-shape findings only (no code findings)./lfx-review-pragainst an open PR — verify it spawnslfx-self-serve-code-reviewerinmode: prin background, prior-comment verification works, draft-then-approve flow blocks auto-posting./lfx-review-pragainst a PR opened from a fork — verify thepull/<N>/headrefspec creates the localrefs/pr/<N>/headref and every downstreamgit show/git log/git merge-basereads from it cleanly.CLAUDE.mdwork-cycle references all components in correct order: pre-PR commits (subagent pair, background) → pre-PR drain →/lfx-self-serve-pr-readiness→/preflight→ push → post-PR bot loop (no subagent pair).Protected files
CLAUDE.mdmodified —guard-protected-files.shflags this. Code-owner review requested on the updated work-cycle section.JIRA: LFXV2-1827