refactor: move review guidelines from prompt injection to skill invocation#60
refactor: move review guidelines from prompt injection to skill invocation#60factory-nizar wants to merge 2 commits intodevfrom
Conversation
|
Droid finished @factory-nizar's task —— View job Code Review Summary — PR #60 The refactor correctly moves guideline consumption into 2 issues found (2 approved, 1 rejected out of 3 candidates) |
factory-nizar
force-pushed
the
feat/review-guidelines-to-skill
branch
from
dcd681f to
fe4c3c6
Compare
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| @@ -24,7 +23,7 @@ export function generateReviewPrompt(context: PreparedContext): string { | |||
|
|
|||
| return `You are performing an automated code review for PR #${prNumber} in ${repoFullName}. | |||
| The gh CLI is installed and authenticated via GH_TOKEN. | |||
There was a problem hiding this comment.
[P1] Non-validator review path silently drops custom review-guidelines
When REVIEW_USE_VALIDATOR is false, this prompt drives the entire review directly — file-group-reviewer subagents are never spawned (the Task tool isn't even included in allowedTools). Since the file-group-reviewer is the only component that now invokes the review-guidelines skill, any repo-specific guidelines from .factory/skills/review-guidelines/SKILL.md are silently ignored in this code path. Either re-inject guidelines into this prompt, or enable the Skill tool and add an explicit invocation step here.
There was a problem hiding this comment.
this is fine since we are deleting this flow
| @@ -23,7 +22,7 @@ export function generateSecurityReviewPrompt(context: PreparedContext): string { | |||
|
|
|||
| return `You are performing a security-focused code review for PR #${prNumber} in ${repoFullName}. | |||
| The gh CLI is installed and authenticated via GH_TOKEN. | |||
There was a problem hiding this comment.
[P1] Security review silently drops custom review-guidelines
Security review uses generateSecurityReviewPrompt directly (selected when reviewType === "security") and never spawns file-group-reviewer subagents. Since the skill-based guideline loading was only added to the file-group-reviewer droid, any repo-specific review rules from .factory/skills/review-guidelines/SKILL.md are now silently skipped during security reviews. Either keep injecting guidelines for this prompt or enable the Skill tool and add an explicit invocation step.
There was a problem hiding this comment.
I think this is also fine? but @varin-nair-factory maybe you can confirm. I view the security review as a separate non generic review that would have it's own instructions
There was a problem hiding this comment.
The refactor correctly moves guideline consumption into file-group-reviewer via the Skill tool, but two review flows that don't use that subagent — non-validator code review (REVIEW_USE_VALIDATOR=false) and security review (reviewType=security) — silently lose access to custom review guidelines.
…ation Remove direct injection of review-guidelines file content into orchestrator prompts. Instead, the file-group-reviewer subagent now invokes the review-guidelines skill via the Skill tool, ensuring the guidelines are consumed where they're actually needed (by the reviewer, not the orchestrator). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ines The --enabled-tools allowlist passed to droid exec did not include Skill, which meant file-group-reviewer subagents could not invoke the review-guidelines skill even though their droid config listed it. Add Skill to the candidateGenerationTools array in both generate-review-prompt and review command. Also strengthen file-group-reviewer instructions to treat custom guideline violations as mandatory (not stylistic) and make skill invocation the required first step. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
factory-nizar
force-pushed
the
feat/review-guidelines-to-skill
branch
from
5754b25 to
756ac20
Compare
|
Droid finished @factory-nizar's task —— View job Code Review Summary — PR #60 All three candidates were rejected after validation. The TS2454 claim does not reproduce (TypeScript relaxes definite assignment for closure-captured variables). The security-review Skill gap is a pre-existing condition unrelated to this PR. The validator-guidelines concern is speculative since the validator only validates Phase 1 candidates — guidelines are enforced at candidate generation by file-group-reviewer subagents. 0 issues found (0 approved, 3 rejected out of 3 candidates) |
There was a problem hiding this comment.
Code Review Summary — PR #60
All three candidates were rejected after validation. The TS2454 claim does not reproduce (TypeScript relaxes definite assignment for closure-captured variables). The security-review Skill gap is a pre-existing condition unrelated to this PR. The validator-guidelines concern is speculative since the validator only validates Phase 1 candidates — guidelines are enforced at candidate generation by file-group-reviewer subagents. No actionable issues found.
1 participant
Summary
Moves review guidelines from being directly injected into orchestrator prompts to being invoked as a skill by the
file-group-reviewersubagent.Closes FAC-16997
Changes
file-group-reviewer.md: AddedSkilltool and a new workflow step to invoke thereview-guidelinesskill before starting reviewsrc/utils/review-guidelines.ts: DeletedloadReviewGuidelines()andformatGuidelinesSection()utilitiesreviewGuidelinesfield fromPreparedContext,PromptCreationOptions, and all prompt templates (review-candidates, review-validator, review-prompt, security-review)loadReviewGuidelines()calls fromreview-validator.tsandgenerate-review-prompt.tsreview-guidelines.test.tsRationale
The review guidelines were being injected into the orchestrator prompt, but the orchestrator doesn't perform the actual file review -- the
file-group-reviewersubagents do. This change ensures the guidelines are consumed where they're actually needed by having the subagent invoke thereview-guidelinesskill directly.Testing
Tested on: https://github.com/Factory-AI/factory-mono/pull/10782