fix: add fork PR support, bounded diff, and explicit validation skip telemetry by factory-nizar · Pull Request #123 · Factory-AI/droid-action

factory-nizar · 2026-08-12T23:13:07Z

Summary

Implements three levers identified in the investigation of P0 signal metabase-232-2026-07-28-overall-rate-drop (overall 7d success rate fell to 91.0%).

Root Cause

The investigation revealed that:

Fork PRs fail deterministically - gh pr checkout fails when the token is scoped only to the base repo (VALIS 10.5%, aytunc 6.5%, Kanini 5.6%, Florian 4.1%)
Large diffs crash Pass 1 - unbounded diff computation causes buffer overflows on monorepo PRs
Pass 2 validation skip is silent - treats lost reviews as successes, laundering Pass 1 failures into the success metric

Changes

Lever A: Fork-aware checkout

Add isCrossRepository and headRepository { nameWithOwner } to PR GraphQL query
Detect fork PRs and fall back to refs/pull/N/head fetch + detached HEAD checkout when gh pr checkout fails
Apply consistently in both review.ts and generate-review-prompt.ts
Add telemetry: is_fork_pr, checkout_strategy, fork_repository

Lever B: Bounded diff with size cap

Introduce REVIEW_DIFF_MAX_BYTES env var (default 10MB, overridable)
Filter out noise paths (lockfiles, dist/, vendor/, generated files) before size check
Gracefully truncate with explicit marker instead of failing when diff exceeds max
Add telemetry: diff_original_bytes, diff_final_bytes, diff_was_truncated

Lever C: Make Pass 2 validation skip loud

Retry candidates file validation once after 500ms delay (guards against partially-flushed writes)
Attempt to repair malformed JSON by filtering valid comment objects
Emit core.warning instead of core.notice when skipping Pass 2
Add explicit telemetry: validation_skip_reason, review_outcome
Update action.yml DROID_SUCCESS to NOT treat validator_should_run=false as success
Apply consistently to both GitHub and GitLab prepare-validator entrypoints

Expected Impact

Lever A + B: Remove deterministic Pass 1 failures concentrated in fork-heavy and monorepo orgs (estimated ~920 of 1002 failures, 92% of the drop)
Lever C: Stop treating skipped reviews as successes, making dashboard 232 trustworthy at the 99.9% target

Testing

✅ Typecheck passes
✅ All existing tests pass
✅ Mock data updated for new PRBranchData fields
✅ Backward compatible: same-repo PRs and valid candidates files unchanged

Telemetry

New outputs for tracking:

Fork detection: is_fork_pr, fork_repository, checkout_strategy
Diff filtering: diff_original_bytes, diff_final_bytes, diff_was_truncated
Validation skip: validation_skip_reason, review_outcome

…telemetry Implements three levers to address P0 overall 7d success rate drop to 91.0%: **Lever A - Fork-aware checkout**: - Add isCrossRepository and headRepository fields to PR GraphQL query - Detect fork PRs and fall back to refs/pull/N/head fetch + detached HEAD checkout when gh pr checkout fails (token scoped only to base repo) - Apply consistently in both review.ts and generate-review-prompt.ts - Add telemetry: is_fork_pr, checkout_strategy, fork_repository **Lever B - Bounded diff with size cap and truncation**: - Introduce REVIEW_DIFF_MAX_BYTES env var (default 10MB) - Filter out noise paths (lockfiles, dist/, vendor/, generated files) - Gracefully truncate with explicit marker instead of failing when diff exceeds max size - Add telemetry: diff_original_bytes, diff_final_bytes, diff_was_truncated **Lever C - Make Pass 2 validation skip loud and rare**: - Retry candidates file validation once after 500ms delay (guards against partially-flushed writes) - Attempt to repair malformed JSON by filtering valid comment objects - Emit core.warning instead of core.notice when skipping Pass 2 - Add explicit telemetry: validation_skip_reason, review_outcome - Update action.yml DROID_SUCCESS to NOT treat validator_should_run=false as success (treats skipped reviews as distinct degraded outcome) - Apply consistently to both GitHub and GitLab prepare-validator entrypoints All changes preserve backward compatibility for same-repo PRs and valid candidates files. Tests pass, typecheck passes. Relates to: bfdbcf49-701c-4e80-9ddf-32719c1f0ec0 Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

factory-droid · 2026-08-12T23:13:19Z

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

Good reliability improvements for fork PR checkouts, diff size caps, and validator skip telemetry, but a few regressions need fixing before merge. The new GitLab contract tests currently fail due to a missing yaml dependency and existing self-mapping snippets in GitLab docs/examples, and there are also correctness gaps in diff truncation and GitLab skip-to-success signaling.

factory-droid · 2026-08-12T23:29:54Z

+import { describe, expect, it } from "bun:test";
+import * as fs from "fs";
+import * as path from "path";
+import { parseAllDocuments, parse } from "yaml";


[P0] Add yaml dependency for new GitLab contract tests

test/gitlab/component-contract.test.ts imports the yaml package, but it is not declared in package.json, so bun test will fail at module resolution before running any assertions; add yaml as a devDependency (and keep lockfiles in sync) or switch the test to a parser dependency already present.

factory-droid · 2026-08-12T23:29:54Z

+        for (const [name, value] of Object.entries(
+          (config["droid-review"]?.variables ?? {}) as Record<string, string>,
+        )) {
+          expect(value, `${name} maps to itself`).not.toBe(`$${name}`);


[P0] Fix self-referential CI variable mappings in GitLab examples/docs

This PR adds tests that forbid copy-pasteable YAML mappings like GITLAB_TOKEN: $GITLAB_TOKEN and FACTORY_API_KEY: $FACTORY_API_KEY because GitLab treats them as circular references and the job receives the literal $NAME; right now gitlab/examples/factory/droid-review.yml and the GitLab snippets in README.md and docs/gitlab-setup.md still include these mappings, so once YAML parsing works the new tests will fail and readers will copy a broken configuration.

factory-droid · 2026-08-12T23:29:54Z

+  }
+
+  // Still too large, truncate with a marker
+  const truncationMarker = `\n\n[DIFF TRUNCATED: Original size ${originalBytes} bytes, filtered size ${filteredBytes} bytes, max allowed ${maxBytes} bytes. Review is based on the first ${maxBytes} bytes of the filtered diff. Large generated files, lockfiles, and vendor directories were excluded.]\n`;


[P1] Clamp diff truncation to respect REVIEW_DIFF_MAX_BYTES

If REVIEW_DIFF_MAX_BYTES is set smaller than the truncation marker length, maxBytes - truncationMarker.length becomes negative, so the current substring(0, ...) + truncationMarker can produce a diff larger than maxBytes (defeating the cap and potentially reintroducing large-artifact failures); slicing by bytes and handling the marker>=max case keeps the hard size guarantee.

Suggested change

const truncationMarker = `\n\n[DIFF TRUNCATED: Original size ${originalBytes} bytes, filtered size ${filteredBytes} bytes, max allowed ${maxBytes} bytes. Review is based on the first ${maxBytes} bytes of the filtered diff. Large generated files, lockfiles, and vendor directories were excluded.]\n`;

// Still too large, truncate with a marker

const truncationMarker = `\n\n[DIFF TRUNCATED: Original size ${originalBytes} bytes, filtered size ${filteredBytes} bytes, max allowed ${maxBytes} bytes. Review is based on the first ${maxBytes} bytes of the filtered diff. Large generated files, lockfiles, and vendor directories were excluded.]\n`;

const markerBytes = Buffer.byteLength(truncationMarker, "utf8");

if (markerBytes >= maxBytes) {

return {

diff: Buffer.from(truncationMarker, "utf8")

.subarray(0, maxBytes)

.toString("utf8"),

wasTruncated: true,

originalBytes,

};

}

const availableBytes = maxBytes - markerBytes;

const truncatedDiff =

Buffer.from(filteredDiff, "utf8")

.subarray(0, availableBytes)

.toString("utf8") +

truncationMarker;

factory-droid · 2026-08-12T23:29:54Z

+        ...state,
+        validatorSkippedReason: validationError,
+        reviewOutcome: "skipped_invalid_candidates",
+      });


[P1] Mark invalid Pass 1 output as a failed review in the GitLab tracking note

On the retry-failed path this step writes a no-op prompt and exits 0 while recording validatorSkippedReason, but the GitLab template only flips DROID_SUCCESS to false when /tmp/droid-error.txt exists, so gitlab-update-comment-link will still render a green tracking note even though the review was not completed; writing the error file on this path preserves the pipeline's soft-landing behavior while making the lost review visible.

Suggested change

});

});

await fs.writeFile(

"/tmp/droid-error.txt",

`Pass 1 candidates validation failed - review was not completed. Reason: ${validationError}\n`,

);

factory-droid · 2026-08-12T23:29:54Z

+    console.log("   - Token doesn't have 'repo' scope for this repo");
+    console.log();
+    console.log("Alternative: Configure via GitHub UI:");
+    console.log("  1. Go to https://github.com/${owner}/${repo}/settings");


[P1] Fix user-facing URLs/commands to interpolate owner/repo

These guidance logs use ${owner}/${repo} inside normal quoted strings, so the script prints the literal ${owner}/${repo} instead of a usable URL/command, which makes the failure path instructions incorrect for users.

Suggested change

console.log(" 1. Go to https://github.com/${owner}/${repo}/settings");

console.log();

console.log("Alternative: Configure via GitHub UI:");

console.log(` 1. Go to https://github.com/${owner}/${repo}/settings`);

console.log(" 2. Navigate to Settings > Branches");

console.log(" 3. Click 'Add rule' for 'main'");

console.log(" 4. Uncheck 'Allow squash merging'");

console.log(

" 5. Add another rule for 'dev' with all merge methods enabled",

);

console.log();

console.log("Or use the gh CLI directly:");

console.log(` gh repo edit ${owner}/${repo} --disable-squash-merge`);

console.log(" (Note: gh may also fail with 404 if org restricts this)");

factory-droid Bot reviewed Aug 12, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: add fork PR support, bounded diff, and explicit validation skip telemetry - #123

fix: add fork PR support, bounded diff, and explicit validation skip telemetry#123
factory-nizar wants to merge 1 commit into
mainfrom
fix/fork-diff-skip-levers

factory-nizar commented Aug 12, 2026

Uh oh!

factory-droid Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

factory-droid Bot Aug 12, 2026

Uh oh!

factory-droid Bot Aug 12, 2026

Uh oh!

factory-droid Bot Aug 12, 2026

Uh oh!

factory-droid Bot Aug 12, 2026

Uh oh!

factory-droid Bot Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-  const truncationMarker = `\n\n[DIFF TRUNCATED: Original size ${originalBytes} bytes, filtered size ${filteredBytes} bytes, max allowed ${maxBytes} bytes. Review is based on the first ${maxBytes} bytes of the filtered diff. Large generated files, lockfiles, and vendor directories were excluded.]\n`;
+  // Still too large, truncate with a marker
+  const truncationMarker = `\n\n[DIFF TRUNCATED: Original size ${originalBytes} bytes, filtered size ${filteredBytes} bytes, max allowed ${maxBytes} bytes. Review is based on the first ${maxBytes} bytes of the filtered diff. Large generated files, lockfiles, and vendor directories were excluded.]\n`;
+  const markerBytes = Buffer.byteLength(truncationMarker, "utf8");
+  if (markerBytes >= maxBytes) {
+    return {
+      diff: Buffer.from(truncationMarker, "utf8")
+        .subarray(0, maxBytes)
+        .toString("utf8"),
+      wasTruncated: true,
+      originalBytes,
+    };
+  }
+  const availableBytes = maxBytes - markerBytes;
+  const truncatedDiff =
+    Buffer.from(filteredDiff, "utf8")
+      .subarray(0, availableBytes)
+      .toString("utf8") +
+    truncationMarker;

-      });
+      });
+      await fs.writeFile(
+        "/tmp/droid-error.txt",
+        `Pass 1 candidates validation failed - review was not completed. Reason: ${validationError}\n`,
+      );

-    console.log("  1. Go to https://github.com/${owner}/${repo}/settings");
+    console.log();
+    console.log("Alternative: Configure via GitHub UI:");
+    console.log(`  1. Go to https://github.com/${owner}/${repo}/settings`);
+    console.log("  2. Navigate to Settings > Branches");
+    console.log("  3. Click 'Add rule' for 'main'");
+    console.log("  4. Uncheck 'Allow squash merging'");
+    console.log(
+      "  5. Add another rule for 'dev' with all merge methods enabled",
+    );
+    console.log();
+    console.log("Or use the gh CLI directly:");
+    console.log(`  gh repo edit ${owner}/${repo} --disable-squash-merge`);
+    console.log("  (Note: gh may also fail with 404 if org restricts this)");

Uh oh!

Conversation

factory-nizar commented Aug 12, 2026

Summary

Root Cause

Changes

Lever A: Fork-aware checkout

Lever B: Bounded diff with size cap

Lever C: Make Pass 2 validation skip loud

Expected Impact

Testing

Telemetry

Related

Uh oh!

factory-droid Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

factory-droid Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

factory-droid Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

factory-droid Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

factory-droid Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

factory-droid Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

factory-droid Bot commented Aug 12, 2026 •

edited

Loading