Skip to content

feat: load review methodology from factory-mono builtin skill#64

Open
factory-nizar wants to merge 4 commits intodevfrom
feat/review-skill-migration
Open

feat: load review methodology from factory-mono builtin skill#64
factory-nizar wants to merge 4 commits intodevfrom
feat/review-skill-migration

Conversation

@factory-nizar
Copy link
Copy Markdown
Contributor

@factory-nizar factory-nizar commented Mar 23, 2026

Summary

Migrates review methodology from inline prompts to the review builtin skill in factory-mono (PR #11498).

Changes

  • Candidate and validator prompts are now thin wrappers that invoke the review skill for Pass 1 (candidate generation) and Pass 2 (validation)
  • All review methodology, triage, parallel review, validation rules, dedup, and confidence filtering come from the skill
  • Removed load-skill.ts — no GitHub fetch needed, CLI bundles the skill as a builtin
  • Templates retain session framing (senior staff software engineer / validating candidate review comments) for session querying
  • Suggestion block rules toggled via include_suggestions (defaults true for CI)

Depends on

  • factory-mono #11498 (adds review builtin skill)

Verified

@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch 5 times, most recently from a3eefc9 to 0d493c4 Compare March 23, 2026 23:07
@factory-nizar factory-nizar changed the title Migrate review to skill-based architecture with shallow default Migrate review to skill-based architecture Mar 23, 2026
@factory-nizar factory-nizar changed the base branch from feat/sb-v3-balanced to dev March 23, 2026 23:34
@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch 4 times, most recently from 07f09c6 to 7e4fc66 Compare March 24, 2026 00:32
@factory-nizar factory-nizar marked this pull request as ready for review March 24, 2026 00:38
@factory-droid
Copy link
Copy Markdown
Contributor

factory-droid bot commented Mar 24, 2026

Droid finished @factory-nizar's task —— View job


Droid reviewed this PR and found 3 issues (3 P1).

Summary: The skill-loader integration has a genuine path-resolution bug (HOME fallback to ~ doesn't expand in Node/Bun) and both prompt templates can silently lose their review methodology when skill loading fails, since the previously hardcoded guidance was removed without a fallback. Consider using os.homedir() and either failing fast on missing skills or keeping a minimal inlined methodology.

Copy link
Copy Markdown
Contributor

@factory-droid factory-droid bot left a comment

Choose a reason for hiding this comment

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

The skill-loader integration has a genuine path-resolution bug (HOME fallback to ~ doesn't expand in Node/Bun) and both prompt templates can silently lose their review methodology when skill loading fails, since the previously hardcoded guidance was removed without a fallback. Consider using os.homedir() and either failing fast on missing skills or keeping a minimal inlined methodology.

@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch from 7e4fc66 to 7c007f8 Compare March 24, 2026 00:53
Copy link
Copy Markdown

@jonathan-factory jonathan-factory left a comment

Choose a reason for hiding this comment

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

Looks good, might be good to confirm that the plugin gets properly loaded before the action runs

Possible that it only gets installed in interactive mode

@factory-nizar
Copy link
Copy Markdown
Contributor Author

i'll test on the mono-repo now that the plugin is merged!

@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch from dba8f95 to cd082dd Compare March 27, 2026 20:01
@factory-nizar factory-nizar changed the title Migrate review to skill-based architecture feat: load review methodology from factory-mono builtin skill Mar 27, 2026
@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch from cd082dd to bcc5b7a Compare March 27, 2026 21:46
Load the review skill's shared methodology from factory-mono's
builtin-skills/review/SKILL.md instead of keeping it inline in the
CI prompt templates. The skill is loaded at runtime via local plugin
cache or GitHub fallback.

The shared methodology (bug patterns, reporting gate, confidence
calibration, deduplication, analysis discipline) is extracted via
BEGIN_SHARED_METHODOLOGY / END_SHARED_METHODOLOGY markers and injected
into both candidate and validator prompts. Suggestion block rules
remain controlled by the include_suggestions toggle at the CI template
level.

Depends on: Factory-AI/factory-mono#11498

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch from bcc5b7a to 6465fba Compare March 27, 2026 21:55
The Droid CLI bundles the review skill as a builtin, so the agent can
invoke it via the Skill tool at runtime. Remove the load-skill.ts
GitHub fetch and instead instruct the agent to invoke the 'review'
skill directly.

This eliminates the network dependency during the prepare step and
uses the CLI as the single source of truth for the review methodology.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-nizar factory-nizar force-pushed the feat/review-skill-migration branch from 61e6660 to 03cb28b Compare March 27, 2026 22:01
@factory-nizar
Copy link
Copy Markdown
Contributor Author

CI Verification

Tested end-to-end on factory-mono PR #11503 which points droid-review.yml at this branch.

Results

  • Droid Auto Review workflow: Passed
  • Two-pass pipeline: Both candidate generation and validation completed successfully
  • Skill tool: Included in enabled tools (--enabled-tools ...Skill) for candidate pass
  • Prompt instructions: Both passes include invoke the 'review' skill to load the review methodology
  • Review posted: [P1] finding about unpinned action reference — follows the skill's security bug pattern (supply-chain risk)

Log evidence

Candidate pass prompt:

Before starting, invoke the 'review' skill to load the review methodology. 
Follow its bug patterns, analysis discipline, reporting gate, and priority levels throughout your review.

Validator pass prompt:

Before starting, invoke the 'review' skill to load the review methodology. 
Apply its reporting gate, confidence calibration, and deduplication rules.

CLI exec command:

droid exec --output-format stream-json --skip-permissions-unsafe   --enabled-tools Read,Grep,Glob,LS,Execute,Edit,Create,ApplyPatch,...,Task,FetchUrl,Skill   --tag code-review --model gpt-5.2 --reasoning-effort high   -f /home/runner/_work/_temp/droid-prompts/droid-prompt.txt

…skill

Both candidate and validator prompts now only provide:
- Session framing (senior engineer / validating candidates)
- PR context and file paths
- Skill invocation instruction (Pass 1 or Pass 2)
- CI-specific output schema and posting constraints

All review methodology, triage, parallel review, validation rules,
deduplication, and confidence filtering come from the skill.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
## CRITICAL REQUIREMENTS
### Critical Requirements

1. You MUST read and validate **every** candidate before posting anything.
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.

These are ci specfic

@factory-nizar
Copy link
Copy Markdown
Contributor Author

factory-nizar commented Mar 27, 2026

CI Verification (Final — With Code Smell Test)

Tested end-to-end on factory-mono PR #11503 with a deliberate code smell file to verify skill-driven findings.

Workflow Run: Passed

Log Evidence

Candidate pass:

You are a senior staff software engineer and expert code reviewer.
Invoke the 'review' skill to load the review methodology, then execute its
**Pass 1: Candidate Generation** procedure — including suggestion block rules.

Validator pass:

You are validating candidate review comments for PR #11503 in Factory-AI/factory-mono.
Invoke the 'review' skill to load the review methodology, then execute its
**Pass 2: Validation** procedure — including suggestion block rules.

What This Proves

  1. Skill loaded: The review builtin skill was invoked via the Skill tool at runtime
  2. Two-pass pipeline: Both candidate generation (Pass 1) and validation (Pass 2) completed
  3. Methodology applied: Findings follow skill's priority levels, reporting gate, and bug patterns
  4. Suggestions enabled: Skill invocation includes suggestion block rules
  5. Thin wrappers work: Templates only provide context + output schema; all review logic comes from the skill

Allows using a custom CLI build for testing builtin skills from
unreleased branches.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-nizar
Copy link
Copy Markdown
Contributor Author

Bundle Verification — Builtin Review Skill

Built CLI from feat/review-builtin-skill branch in CI to confirm the review SKILL.md is correctly bundled.

CI Run — Passed

Build step output:

Bundle created
1
Review skill found in bundle

grep -c "BEGIN_SHARED_METHODOLOGY" bundle/droid.js returned 1 — the builtin review skill content (methodology, two-pass pipeline, bug patterns, reporting gate) is compiled into the CLI bundle.

Review step output:

You are a senior staff software engineer and expert code reviewer.
Invoke the 'review' skill to load the review methodology, then execute its
**Pass 1: Candidate Generation** procedure — including suggestion block rules.

You are validating candidate review comments for PR #11503 in Factory-AI/factory-mono.
Invoke the 'review' skill to load the review methodology, then execute its
**Pass 2: Validation** procedure — including suggestion block rules.

Both passes completed successfully. Once factory-mono PR #11498 merges, the released CLI will include the builtin skill and the Skill tool will load it directly instead of from factory-plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants