Add supply chain security checks to review protocol#59
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)**/*.md📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
_shared/**⚙️ CodeRabbit configuration file
Files:
⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
WalkthroughExpanded the Security checklist in ChangesSecurity Evaluation Guidance
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@_shared/review-protocol.md`:
- Around line 44-45: Replace the blanket rejection of ".vscode/" with a policy
to flag IDE-specific directories (e.g., ".vscode/", ".idea/") for extra
scrutiny: allow legitimate non-executable workspace files such as
"settings.json" and "extensions.json" but require reviewers to inspect any
executable configurations (e.g., "tasks.json", "launch.json", preLaunchTask
entries, or scripts referenced from those files) for malicious behavior before
approving; update the text that currently rejects ".vscode/" to this new
guidance and call out "tasks.json" and "launch.json" as high-risk files to
verify.
- Around line 44-45: Update the guidance in review-protocol.md so it no longer
unconditionally rejects `.claude/` (which this repo uses); instead change the
rule to flag introduction of IDE/agent dirs (e.g., `.claude/`, `.vscode/`) only
when the repository does not already document or use them—check for existing
references in CONTRIBUTING.md or AGENTS.md and, if none exist, require reviewer
verification of legitimate use versus potential supply-chain injection; adjust
the sentence that currently reads "Reject any PR introducing `.claude/` or
`.vscode/` directories" to the narrower, conditional wording described.
- Around line 47-48: The rule "If the most recent commit author differs from the
PR author, treat as potentially compromised" is non-actionable; update that rule
text to require flagging such PRs for manual review with MEDIUM severity and
enumerate exceptions (maintainer fixup commits, known bot accounts like
Dependabot/renovate, and explicit Co-authored-by trailers) and add an escalation
to HIGH severity when no valid explanation is found; specifically edit the line
containing that rule in _shared/review-protocol.md to replace the current
sentence with the recommended phrasing that defines the manual-review flow, the
three exception checks, and the MEDIUM/HIGH severity guidance so reviewers have
concrete steps to follow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 58f2a55c-9a03-44a8-954a-2cb76769731b
📒 Files selected for processing (1)
_shared/review-protocol.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Workflow content must be plain markdown with no IDE-specific syntax
**/*.md: All file references must use relative paths (not absolute) for symlink compatibility across different install scopes
No IDE-specific syntax is permitted - all workflow content must be plain markdown
Run Vale validation before applying documentation changes to repository files
Files:
_shared/review-protocol.md
⚙️ CodeRabbit configuration file
**/*.md: Cross-workflow consistency (ai-workflows conventions):
- All file references must be relative paths (never absolute) —
this is critical for symlink compatibility- No IDE-specific syntax (Cursor-specific, VS Code-specific, etc.)
- Consistent terminology within a workflow: pick one term, stick
with it- Schema field names and types must match between producer and
consumer files (e.g., if a field is defined in one phase skill
and consumed in another, names and types must agree)- No verbatim duplication of multi-line instruction blocks
across SKILL.md, guidelines.md, and controller.md — each has
a distinct role (shared phase names and brief references are
expected cross-referencing, not duplication)
Files:
_shared/review-protocol.md
_shared/**
⚙️ CodeRabbit configuration file
_shared/**: Shared resource review (ai-workflows conventions):
- Shared resources are referenced by multiple workflows —
changes here have cross-cutting impact. Verify that all
consuming workflows are identified- Recipes must be self-contained and parameterized (using
uppercase PLACEHOLDER names for caller-provided values)- References TO shared resources from workflow skills must use
correct relative depth (../../_shared/ from skills/ directories)- No workflow-specific logic — shared resources must be generic
enough for all consumers
Files:
_shared/review-protocol.md
⚙️ CodeRabbit configuration file
_shared/**: # Code Review ProtocolShared evaluation criteria, finding format, and assessment model used by all
code review activities in this repository — both the standalonecode-review
workflow and the self-review gates embedded in other workflows.Core Principles
- Challenge decisions, not just implementation. Don't just verify that
the code does what it intends — question whether the intent is right.
Should this abstraction exist? Is this the right boundary between
components? Would a different approach avoid the complexity being managed
here? A review that only checks correctness is a linter with extra steps.- Review what the developer wrote, not what you would have written. The
above principle challenges decisions; this one constrains how. Evaluate
the approach on its merits — does it work, is it clear, is it safe? Do
not impose stylistic preferences or rewrite working code in your preferred
idiom. Challenge the design, not the taste.- Findings must be actionable. Every finding must include a concrete
suggestion the developer can evaluate and apply. Vague observations
("consider improving this") are not findings.- Severity must be honest. CRITICAL and HIGH are blockers that would cause
bugs, security issues, or maintenance problems. MEDIUM and LOW are suggestions
that improve quality but are not blocking. Inflating severity erodes trust;
deflating it hides real problems.- Assess on value, not severity. A LOW finding that genuinely improves the
code (clearer naming, better readability, a small correctness improvement) is
worth implementing. A MEDIUM finding that doesn't add real value (stylistic
preference, churn without improvement) is not. Severity indicates impact;
value determines whether the finding is worth acting on.- Discover, don't assume. The project's tech stack, conventions, and quality
standards are discovered from the codebase, not hardcoded. Every project gets
a review ...
Files:
_shared/review-protocol.md
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdThis file provides guidance to AI coding assistants when working with this repository.
Project Overview
This repository contains reusable AI coding workflows that can be installed globally or per-project in any environment (Cursor, Claude Code, Gemini). Each workflow is a self-contained directory with structured markdown files that AI agents can read and execute.
Current workflows:
- ai-ready — Codebase scanning and AGENTS.md generation (update)
- bugfix — Systematic bug resolution (assess, reproduce, diagnose, fix, test, review, document, pr)
- code-review — AI-driven code review with human-in-the-loop decisions (start, continue, clean)
- cve-fix — Automated CVE remediation from Jira tickets (start, scan, patch, validate, pr, backport, close)
- design — Design-and-decompose workflow (ingest, research, draft, decompose, revise, publish, respond, sync)
- docs-writer — Documentation creation workflow (gather, plan, draft, validate, apply, mr)
- e2e — Story-to-tests workflow for [QE] stories (ingest, plan, revise, code, validate, publish, respond)
- implement — Story-to-code workflow (ingest, plan, revise, code, validate, publish, respond)
- kcs — KCS Solution article workflow (gather, draft, validate, handoff)
- prd — Requirements-to-PRD workflow (ingest, clarify, draft, revise, publish, respond)
- sizing — Pre-cycle Feature sizing with T-shirt sizes and team effort breakdowns (ingest, assess, apply)
- skill-reviewer — Meta-workflow that audits AI skill directories
- triage — Bulk Jira bug triage with AI-driven categorization and HTML reports
Architecture
Workflow Structure
Every workflow follows this canonical structure:
workflow-name/ SKILL.md # Entry point with YAML frontmatter (name, description) guidelines.md # Behavioral rules: principles, hard limits, safety, quality README.md # Human-readable documentation (prerequisites, a...
Files:
_shared/review-protocol.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Challenge decisions, not just implementation - verify that the code does what it intends and question whether the intent is right
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Review what the developer wrote, not what you would have written - evaluate approach on merits (does it work, is it clear, is it safe) without imposing stylistic preferences
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Findings must be actionable with concrete suggestions that developers can evaluate and apply
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Severity must be honest - CRITICAL and HIGH are blockers for bugs/security/maintenance; MEDIUM and LOW are non-blocking suggestions
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Assess findings on value, not severity - prioritize findings that genuinely improve code clarity, readability, or correctness
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Discover project conventions from the codebase (AGENTS.md, CLAUDE.md, linting configs, contribution guidelines) rather than applying hardcoded standards
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Verify correctness - check for logic errors, off-by-one mistakes, and unhandled edge cases
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Verify error handling - ensure errors are caught, propagated, and reported appropriately with all failure modes handled
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Flag security concerns including injection risks, unsafe operations, exposed secrets, and OWASP-category issues
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Treat commits where the most recent commit author differs from the PR author as potentially compromised and flag for extra scrutiny
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Evaluate design quality - verify each new abstraction earns its complexity and responsibilities are clearly divided with no god functions
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Avoid single types accumulating unrelated concerns and ensure interfaces hide implementation details while exposing coherent contracts
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Make implicit assumptions between components explicit (shared constants, expected call order, assumed preconditions)
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Watch for design problems: functions with many parameters (missing intermediate type), types importing from unrelated packages (mixed concerns), repeated error-handling blocks (missing shared helper), interfaces exposing internal types
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Check for performance issues including unnecessary allocations, N+1 queries, unbounded operations, and other efficiency concerns
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Verify naming is descriptive and intent is clear from reading the code
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Verify test coverage - ensure changes are tested with edge cases covered and tests verify contracts, not just implementation
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Verify changes follow project conventions discovered from AGENTS.md, CLAUDE.md, linting configs, and contribution guidelines
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Every finding must include: File path, Location (line range or function name), Severity (CRITICAL/HIGH/MEDIUM/LOW), Category, Issue description, and concrete actionable Suggestion
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Discard findings that cannot cite a specific file and location in the actual diff - they indicate hallucinated references
Learnt from: CR
Repo: flightctl/ai-workflows
Timestamp: 2026-06-09T08:23:43.837Z
Learning: Validate all findings by confirming cited files were part of changes, confirming line ranges/functions exist in current version, and reading surrounding context
| or other OWASP-category concerns? Reject any PR introducing `.claude/` or | ||
| `.vscode/` directories (active supply chain attack vectors). Flag changes to |
There was a problem hiding this comment.
.vscode/ rejection is overly broad and doesn't distinguish legitimate use from attack vectors.
Many projects legitimately use .vscode/ for:
- Workspace settings (
settings.json) - Recommended extensions (
extensions.json) - Debug configurations (
launch.json,tasks.json) - Shared team configuration
A blanket rejection doesn't help reviewers distinguish between these legitimate uses and actual supply chain injection attempts (e.g., .vscode/tasks.json with malicious preLaunchTask scripts).
Suggestion: Replace the blanket rejection with: "Flag introduction of IDE-specific directories (.vscode/, .idea/, etc.) for extra scrutiny. Verify that any executable configuration (tasks, launch scripts) is reviewed for malicious behavior. Legitimate workspace settings are acceptable if they don't execute code."
🤖 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 `@_shared/review-protocol.md` around lines 44 - 45, Replace the blanket
rejection of ".vscode/" with a policy to flag IDE-specific directories (e.g.,
".vscode/", ".idea/") for extra scrutiny: allow legitimate non-executable
workspace files such as "settings.json" and "extensions.json" but require
reviewers to inspect any executable configurations (e.g., "tasks.json",
"launch.json", preLaunchTask entries, or scripts referenced from those files)
for malicious behavior before approving; update the text that currently rejects
".vscode/" to this new guidance and call out "tasks.json" and "launch.json" as
high-risk files to verify.
Source: Coding guidelines
Rejecting .claude/ conflicts with this repository's documented installation practices.
The instruction to "Reject any PR introducing .claude/ or .vscode/ directories" creates a direct conflict: this repository's own CONTRIBUTING.md and AGENTS.md document .claude/ as the legitimate project-level installation location for Claude Code workflows. The installer creates .claude/skills/, .claude/commands/, and .claude/CLAUDE.md as part of normal operation.
When consuming workflows (code-review, self-review gates) read this protocol, they would flag the repository's own installation method as a supply chain attack. This violates the coding guidelines requirement that shared resources must be "generic enough for all consumers."
Suggestion: Clarify the threat model. If the concern is unexpected .claude/ directories in repositories that don't use this workflow system, narrow the guidance to: "Flag introduction of IDE-specific directories (.claude/, .vscode/) in repositories that don't already use them. Verify legitimate use versus potential supply chain injection." This allows reviewers to distinguish between documented tooling and unexpected directory injection.
🤖 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 `@_shared/review-protocol.md` around lines 44 - 45, Update the guidance in
review-protocol.md so it no longer unconditionally rejects `.claude/` (which
this repo uses); instead change the rule to flag introduction of IDE/agent dirs
(e.g., `.claude/`, `.vscode/`) only when the repository does not already
document or use them—check for existing references in CONTRIBUTING.md or
AGENTS.md and, if none exist, require reviewer verification of legitimate use
versus potential supply-chain injection; adjust the sentence that currently
reads "Reject any PR introducing `.claude/` or `.vscode/` directories" to the
narrower, conditional wording described.
Source: Coding guidelines
There was a problem hiding this comment.
On .vscode/ — this is an intentionally strict posture. .vscode/ can carry executable configs that are actively exploited as attack vectors. Keeping the blanket rejection.
On .claude/ — the protocol evaluates diffs, not existing repo structure. A PR introducing a new .claude/ directory where one doesn't exist is the suspicious case. For projects that already have one, changes to .claude/ should require human review — not LLM-only — given the current risk landscape.
There was a problem hiding this comment.
@ItzikEzra-rh — understood on both points.
.vscode/: Noted — intentionally strict, keeping the blanket rejection. Acknowledged.
.claude/: The "introducing" wording does carry the intent for the new-directory case. One gap worth noting: your second point — "For projects that already have one, changes to .claude/ should require human review" — isn't reflected in the current protocol text. The current sentence only rejects introductions of .claude/; it's silent on subsequent changes. If you want that human-escalation policy enforced, it would need an additional sentence, e.g.:
"Changes to an existing
.claude/directory must be escalated to human review and not approved by an LLM reviewer alone."
Happy to leave it as-is if that's intentional scope for this PR.
✏️ Learnings added
Learnt from: ItzikEzra-rh
Repo: flightctl/ai-workflows PR: 59
File: _shared/review-protocol.md:44-45
Timestamp: 2026-06-09T10:39:01.516Z
Learning: In `_shared/review-protocol.md` (flightctl/ai-workflows), the blanket rejection of `.vscode/` directories is an intentional, strict security posture. It is NOT an oversight — `.vscode/` can carry executable configs (tasks.json, launch.json with preLaunchTask) that are actively exploited as supply chain attack vectors. Do not suggest relaxing this to a nuanced "flag for scrutiny" policy.
Learnt from: ItzikEzra-rh
Repo: flightctl/ai-workflows PR: 59
File: _shared/review-protocol.md:44-45
Timestamp: 2026-06-09T10:39:01.516Z
Learning: In `_shared/review-protocol.md` (flightctl/ai-workflows), "Reject any PR introducing `.claude/`" means rejecting PRs that add a new `.claude/` directory where one did not previously exist. The protocol evaluates diffs, not existing repo structure, so existing `.claude/` directories in a repo are not affected by this rule. Additionally, changes to an *existing* `.claude/` directory should require human review (not LLM-only), though this second nuance is not yet reflected in the protocol text as of PR `#59`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| If the most recent commit author differs from the PR author, treat as | ||
| potentially compromised. |
There was a problem hiding this comment.
"Treat as potentially compromised" lacks actionable guidance.
The instruction states "If the most recent commit author differs from the PR author, treat as potentially compromised" but does not specify what action to take. The protocol's Core Principles require that "Findings must be actionable" with "a concrete suggestion the developer can evaluate and apply."
Additionally, the check doesn't account for legitimate scenarios:
- Maintainers making fixup commits during review
- Automated bot commits (Dependabot, renovate, etc.)
- Co-authored commits (
Co-authored-by:trailers) - Squash-and-merge workflows where the final committer differs from the original author
Suggestion: Replace with: "If the most recent commit author differs from the PR author, flag for manual review with MEDIUM severity unless the mismatch is explained by: (1) maintainer fixup commits, (2) known bot accounts, or (3) explicit Co-authored-by trailers. Escalate to HIGH severity if no explanation is found." This provides actionable severity guidance and acknowledges legitimate patterns.
🤖 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 `@_shared/review-protocol.md` around lines 47 - 48, The rule "If the most
recent commit author differs from the PR author, treat as potentially
compromised" is non-actionable; update that rule text to require flagging such
PRs for manual review with MEDIUM severity and enumerate exceptions (maintainer
fixup commits, known bot accounts like Dependabot/renovate, and explicit
Co-authored-by trailers) and add an escalation to HIGH severity when no valid
explanation is found; specifically edit the line containing that rule in
_shared/review-protocol.md to replace the current sentence with the recommended
phrasing that defines the manual-review flow, the three exception checks, and
the MEDIUM/HIGH severity guidance so reviewers have concrete steps to follow.
Source: Coding guidelines
There was a problem hiding this comment.
Fair point on actionability. Updating the wording to 'flag for manual review before approving' — same intent, more concrete. Not adding exception lists though — that's too granular for a shared protocol, individual projects can refine in their own conventions.
There was a problem hiding this comment.
@ItzikEzra-rh, that's a reasonable call — "flag for manual review before approving" is concrete enough to be actionable without overloading the shared protocol. The design philosophy of keeping shared resources generic and delegating project-specific refinements to per-project conventions is exactly right for a cross-cutting resource like this.
Once the wording update lands, feel free to tag me and I'll resolve this comment.
✏️ Learnings added
Learnt from: ItzikEzra-rh
Repo: flightctl/ai-workflows PR: 59
File: _shared/review-protocol.md:47-48
Timestamp: 2026-06-09T10:38:42.113Z
Learning: In the `flightctl/ai-workflows` repository, the `_shared/review-protocol.md` intentionally stays at a high level of generality. Project-specific exception lists, edge-case handling, and fine-grained rule refinements are expected to live in individual project conventions files, not in the shared protocol. Avoid suggesting overly granular additions to shared resources in this repo.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary
.claude//.vscode/directory injection, CI/CD config tampering, and commit author mismatchesTest plan
Workflows Affected
Note: the change is intended to be surfaced as an additional checkbox in the code-review and self-review-gate flows (currently present but not yet completed).
Structural and Behavioral Changes to Shared Resources
Cross-Workflow Convention Impact
Other Notes