Finish BuilderSync publication and preview-safe authorization#2130
Finish BuilderSync publication and preview-safe authorization#21303mdistal wants to merge 13 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.
| .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.
| -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.
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.The Slack correction flow has a frozen post-deploy test in private
#test-content-app: create one disposable Design Ask, rename it in Content, correct it in the same Slack thread, and verify the original Content ID is updated with Requester preserved and Assignee resolved. This cannot run against the PR before merge because the workspace has no preview Slack bot or webhook environment; it remains an explicit post-deploy gate.Verification
Hosted rollout note
The same-repository PR should provision the Content Netlify Deploy Preview and its Neon branch. End-to-end hosted Builder authorization additionally 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: