Merge dev into main: GitLab Phase 1, review refactor, and README auth docs#105
Merged
Conversation
…core/review/
Both Pass 1 (candidate generation) and Pass 2 (validator) prompts were
~95% identical between GitHub and GitLab — same skill invocation, same
output schema, same critical constraints. Differences were limited to
labels (PR/MR, Repo/Project), JSON meta keys (prNumber/mrIid,
baseRef/targetBranch), MCP tool names, and a handful of platform-specific
instruction lines.
Extract the shared structure into platform-agnostic builders under
src/core/review/prompts/ that take a ReviewTerminology shape, and convert
the four existing prompt files into thin adapters:
src/core/review/prompts/
types.ts ReviewTerminology + ReviewPromptContext
candidates.ts generateCandidatesPrompt(ctx)
validator.ts generateValidatorPrompt(ctx)
src/create-prompt/terminology.ts GITHUB_TERMINOLOGY constant
src/gitlab/prompts/terminology.ts GITLAB_TERMINOLOGY + factory
Both platform-specific wrappers now just map their context shape onto
ReviewPromptContext and delegate. GitLab's submit_review needs the MR IID
re-asserted in the tool call, so we expose a gitlabTerminologyFor(mrIid)
helper that bakes it into the submit_review hint.
Net LOC is roughly even on this commit alone (+61), but ~280 LOC of
literal duplication is gone, and any future shared prompt (security
review/report when GitLab adds it) gets the shared scaffolding for free.
All 29 prompt-specific tests still pass on both platforms; full suite
445/445; tsc clean.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…l disk-write helper
The two platforms fetch review artifacts with fundamentally different
mechanics (GitHub shells out to git/gh CLI with shallow-clone unshallow
and a 50MB-buffered fallback; GitLab uses parallel REST calls), so the
fetchers stay platform-specific. But the on-disk shape is identical, so:
src/core/review/artifacts/types.ts ReviewArtifactPaths +
ReviewArtifactContents shapes
src/core/review/artifacts/write.ts writeReviewArtifacts(outDir,
contents, names) — mkdir + parallel
writeFile×3
GitLab's computeReviewArtifacts now delegates the disk-write through the
shared helper. GitHub keeps its 3-helper public API (each does its own
write so tests pin those signatures); it just re-exports the shared type
via the existing ReviewArtifacts alias.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…ters to core
The two platforms render the sticky review-tracking comment with very
different mechanics today — GitHub builds it from inside the agent via
the github-comment-server MCP tool, while GitLab writes a rich state-
machine body from CI — so we can't share a single renderer. What we
*can* share is the contract:
src/core/review/tracking/types.ts ReviewTrackingState +
ReviewTrackingTelemetry +
ReviewTrackingFields
src/core/review/tracking/format.ts formatDurationMs(ms) +
formatCostUsd(usd) — keeps the
"1m 23s • $0.0042" rendering
identical across platforms
GitLab's tracking-note.ts now imports those and re-exports the GitLab
aliases (TrackingNoteState, TrackingNoteTelemetry, TrackingNoteOptions)
so existing call sites stay untouched.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…core The actual string-matching for `@droid fill`, `@droid review`, `@droid security`, `@droid security --full`, and bare `@droid` is identical between platforms. Move parseDroidCommand + DroidCommand + ParsedCommand into: src/core/review/triggers/parse-command.ts GitHub's command-parser keeps extractCommandFromContext (which scans GitHub-payload-shaped events) and re-exports the shared types/parser so existing imports from `../utils/command-parser` stay valid. When the GitLab trigger path is ported (currently a pending task on the GitLab side), it can reuse parseDroidCommand directly instead of duplicating the regex set. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…L security badge Two pre-existing bugs surfaced by the bot review on PR #96 — both ship on dev today, this just brings the fixes along with the refactor. 1. formatDurationMs returned "1m 60s" for ms values whose remainder seconds rounded up to 60 (e.g. ms=119600 → seconds=119.6 → minutes=1, remSec=round(59.6)=60). Round to whole seconds first, then split into minutes+seconds so the carry happens cleanly. Add regression coverage in test/core/review/tracking/format.test.ts covering the rollover boundary (119600/179600/239600 ms) plus the normal sub-second / sub-minute / multi-minute cases. 2. The GitLab security-badge URL in the prompt instruction (`security%20review-ran-blue`) didn't match the badge actually rendered by buildTrackingNoteBody (`security%20review-enabled-blue`, plus shields.io style params + logo). If the validator pass followed the prompt literally and the CI-prepended badge also fired, the MR could end up with two distinct security badges. Export the SECURITY_BADGE constant from tracking-note.ts and reference it from GITLAB_TERMINOLOGY.securityBadgeInstruction so the renderer is the single source of truth. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Three small issues caught by a thorough re-review of the refactor diff:
1. `securityBadgeInstruction` wrapped the badge markdown in single
backticks (``````), so an LLM literally following the
prompt would paste the badge inside an inline-code span and produce
`` rendered as code instead of an image. Rephrased
to emit the raw markdown with an explicit "no surrounding backticks
or code fences" caveat.
2. The security-subagent prompt block interpolated
`${t.baseRefLabel.toLowerCase()}` which produced awkward bare
phrases ("pr base ref" / "mr target branch") instead of the
original prompts' "base ref" / "target branch". Added a dedicated
`baseRefShortLabel` field on ReviewTerminology and use it in the
narrative prose; the noun-prefixed `baseRefLabel` is still used in
the structured <context> block where it matches today's wording.
3. format.ts docstring claimed the helpers are "shared between the
GitHub MCP comment server and the GitLab tracking-note builder" but
the GH side doesn't consume them yet. Softened the wording to
reflect actual usage today plus the unification intent.
Tests: 451/451 still pass; tsc clean.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Document the two kinds of access the action needs: the FACTORY_API_KEY secret to run Droid, and GitHub access via the Factory Droid GitHub App (default) or a custom github_token (override). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
docs(readme): add Authentication section
… rate to 99.9% Code review success rate is currently 96% (per Metabase dashboard 232). Failures are concentrated in the Pass 1 to Pass 2 transition for CI reviews (~476/24484 eligible reviews fail at Pass 2 over 30 days). Root causes identified: - No retry on Droid Exec failures (transient model/API errors kill the run) - No retry on MCP server registration (single registration failure kills action) - No retry on gh pr checkout (transient git/GitHub API failures kill run) - No retry on gh pr diff fallback (transient failures in diff computation) - No validation of Pass 1 candidates JSON before Pass 2 (malformed/missing candidates cause Pass 2 to fail with no graceful degradation) Changes: - run-droid.ts: Add retry (3 attempts, exponential backoff) around Droid Exec spawn+wait loop. Transient model timeouts and API errors now retry instead of immediately failing the pipeline. - run-droid.ts: Add retry (3 attempts, 2s initial delay) around individual MCP server registrations. - review.ts: Wrap gh pr checkout in retryWithBackoff (3 attempts, 3s delay). - generate-review-prompt.ts: Same retry for the standalone review action path. - review-artifacts.ts: Wrap gh pr diff fallback in retryWithBackoff (3 attempts). - prepare-validator.ts: Validate Pass 1 candidates JSON exists and has valid schema before running Pass 2. If invalid, skip Pass 2 gracefully instead of failing the entire pipeline. - action.yml: Add conditional to skip validator Droid Exec step when prepare-validator outputs validator_should_run=false. - run-droid-mcp.test.ts: Update MCP failure test to account for retry delays. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Mirror the GitHub validation in gitlab-prepare-validator.ts: check that the candidates JSON file exists and has a valid 'comments' array before writing the Pass 2 prompt. If invalid, write a no-op prompt so droid exec exits cleanly instead of failing the pipeline. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
When validator_should_run is false (Pass 1 candidates invalid), base DROID_SUCCESS on Pass 1's conclusion instead of the skipped validator step's conclusion. This prevents the tracking comment from showing an error when the pipeline intentionally skipped Pass 2. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
GitHub Actions expression syntax has no ternary (? :) operator, so the DROID_SUCCESS expression threw 'Unexpected symbol' at runtime and broke the comment/status-update step on nearly every triggered review. Rewrite the validator-skip branch using &&/|| equivalents. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- prepare-validator: drop dead contains_trigger output, narrow catch to instanceof Error - gitlab-prepare-validator: narrow catch to instanceof Error - artifacts: co-locate ReviewArtifactNames with sibling types in types.ts - base-action/run-droid: extract shared retryWithBackoff into utils/retry, remove duplicated inline copy and redundant pre-retry mcp remove, redact inline --env secrets before logging/rethrow, unify Droid Exec retry to the shared backoff helper - add base-action retry unit test Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
fix(review): add retry logic and Pass 1 validation for 99.9% success rate
Contributor
|
Droid encountered an error —— View job Droid is reviewing code and running a security check… |
Contributor
|
Droid finished @factory-nizar's task —— View job No inline comments were posted because GitHub returned 422 "Line could not be resolved" when attempting to submit the batched review (the provided PR diff artifact was empty). The candidates appear actionable, but cannot be safely anchored to the current PR diff. |
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
3 participants
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.
Brings the latest changes from
devintomain.Highlights
src/core/review/(refactor(review): hoist GH+GL shared review code into src/core/review/ #96)Commits included
Please review and merge once CI passes.