Finish BuilderSync publication and preview-safe authorization#2130
Finish BuilderSync publication and preview-safe authorization#21303mdistal wants to merge 19 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — not generatedThe recap was suppressed because the diff matched a secret/credential pattern. No plan was published. Reason: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
PR #2130 substantially expands the Content → Builder workflow with body-aware review and reconciliation, rich-content fidelity, stable hydration/timing, Slack artifact continuity, preview database isolation, and a signed hosted authorization relay. The overall architecture is strong: Builder writes are scoped to selected rows, access checks and owner scoping are preserved, relay secrets are validated, and the test coverage is extensive. This is high risk because it includes credential authorization, external writes, and preview database provisioning.
Key Findings
- 🔴 HIGH — Ambiguous Builder transport failures are marked retryable without verifying whether the remote write already succeeded, so a retry can duplicate an externally applied write.
- 🟡 MEDIUM — The selected-row preview path does not perform the authoritative Builder target-row backfill used by prepare, so partial snapshots can show an existing update as
create_draft. - 🟡 MEDIUM — Neon action outputs are not validated before empty/invalid database URLs are written to Netlify preview configuration.
A prior relay-replay concern was not resubmitted because its relevant callback lines are unchanged and are not commentable in this PR diff; the current review is limited to actionable positions introduced by the latest PR head.
🧪 Browser testing: Planned 17 grounded UI test cases, but execution was blocked because the browser-test executor had no browser-control tools; the dev server was healthy.
| `Builder write request failed with HTTP ${writeResult.status}.`; | ||
| await deps.markExecutionFailed({ | ||
| executionId: execution.id, | ||
| state: writeResult.ambiguity ? "reconciliation_required" : "failed", |
There was a problem hiding this comment.
🔴 Verify ambiguous Builder writes before allowing retries
Timeout/transport ambiguity only means the response was lost; the Builder write may already have been applied. Marking this execution retryable without reconciling the target can allow a subsequent retry to repeat an externally successful write and leave local/remote state inconsistent. Query or reconcile the target after ambiguous dispatch before permitting a retry.
There was a problem hiding this comment.
Addressed in the current branch: ambiguous transport/write results now persist the reconciliation_required state and throw a conflict that explicitly prevents retry. The execution path performs read-only intent lookup and reconciliation before any later attempt can proceed, and only marks success after remote evidence is reconciled locally.
| .slice(0, BUILDER_SOURCE_REVIEW_PREPARE_LIMIT); | ||
| const review = | ||
| changeSets.length > 0 | ||
| ? buildBuilderSourceReviewPayload({ source, changeSets }) |
There was a problem hiding this comment.
🟡 Backfill authoritative Builder rows in selected previews
This preview path builds the review directly from the current source snapshot, unlike prepare, which first backfills authoritative Builder target rows. For a partial snapshot, an existing selected remote entry can therefore be shown as create_draft with no target ID, causing the approval UI to describe a different operation from the later prepare/execute path. Apply the same authoritative-row reconciliation before constructing the preview payload.
There was a problem hiding this comment.
Fixed in fbf8b9e. Read-only previews now call the same withAuthoritativeBuilderTargetRows backfill used by prepare before building the review payload. Added a database regression test covering a selected preview whose snapshot window omits the linked Builder row.
| -d '{"value":"'"$VAL"'","context":"deploy-preview"}' \ | ||
| -o /dev/null -w '' \ | ||
| || true | ||
| set_branch_value "$KEY" "$VAL" |
There was a problem hiding this comment.
🟡 Validate Neon database URL outputs before configuring Netlify
The Netlify environment step consumes the Neon action outputs without checking that both pooled and unpooled URLs are non-empty. If branch creation returns empty outputs while the action still exits successfully, the preview can be configured with blank database URLs and fail later with little diagnostic signal; fail the workflow before writing invalid values.
There was a problem hiding this comment.
Resolved by scope removal in c1156ea. The BuilderSync PR no longer changes the Neon preview-branch workflow or Netlify database provisioning; that infrastructure work is being kept separate from the production BuilderSync path.
Summary
agent-native-blog-article-testmodelLocal QA evidence
Published the rich QA article through the real Content UI to exactly one existing safe Builder entry (
75972cb0094f4568a81bf80789505838). Final reconciliation proved:publishedServer timings on the successful pass:
The production
blog-articlemodel was never used. Notion OAuth and Notion Sync are explicitly out of scope.Verification
Hosted rollout note
End-to-end hosted Builder authorization requires the same
AGENT_NATIVE_BUILDER_RELAY_SECRETon the Content preview and the approved corporate callback deployment. The corporate workspace must consume the published core patch before its first-hop callback can relay.The corporate callback now also requires
AGENT_NATIVE_BUILDER_RELAY_TARGET_ORIGINS, a comma-separated exact-origin allowlist for trusted preview receivers. Wildcards are rejected, and the shared relay secret is enforced at 32 or more characters.For Netlify, callback trust is additionally restricted to the immutable deploy-specific permalink (
https://<24-hex-deploy-id>--<site>.netlify.app); mutabledeploy-preview-*aliases are rejected even if configured.The final hosted-preview check also found and fixed an independent connection gate: Builder account authorization remains available when branch-creation capability is not configured.
Hosted QA evidence
Hosted timings on the final pass: