Skip to content

Add supply chain security checks to review protocol #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ItzikEzra-rh wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add supply chain security checks to review protocol #59

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7611894
Add supply chain security checks to review protocol
ItzikEzra-rh Jun 9, 2026
cc381d8
Address review: improve actionability of author mismatch guidance
ItzikEzra-rh Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion _shared/review-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ Evaluate changes against these categories, prioritized by impact:
2. **Error handling** — Are errors caught, propagated, and reported
appropriately? Are failure modes handled?
3. **Security** — Are there injection risks, unsafe operations, exposed secrets,
or other OWASP-category concerns?
or other OWASP-category concerns? Reject any PR introducing `.claude/` or
`.vscode/` directories (active supply chain attack vectors). Flag changes to
Comment on lines +44 to +45

@coderabbitai coderabbitai Bot Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

.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

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

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

@ItzikEzra-rh ItzikEzra-rh Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coderabbitai coderabbitai Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

CI/CD configuration (Actions workflows, pipeline configs) for extra scrutiny.
If the most recent commit author differs from the PR author, flag for
manual review before approving.
4. **Design** — Does each new abstraction earn its complexity? Are
responsibilities clearly divided — no god functions, no single type
accumulating unrelated concerns? Do interfaces hide implementation details
Expand Down
Loading