Fallback to deployment manifest when workspace registry denies read - #5039
builder-io-integration[bot] wants to merge 5 commits into
Conversation
… read A hosted workspace-registry 401/403 was rethrown out of listWorkspaceApps(), aborting the whole discovery chain. Because listCuratedWorkspaceTemplates() calls the same function only to compute an installed flag, one denial lit up two error cards on /apps and hid every workspace app. A denial answers for the gateway hop, not for what the caller may see: the receiver resolves org membership against its own database, so a cross-deployment trust gap reads as 403 for every user at once. Fall back to the deployment-owned manifest, which is still access-filtered per caller in finalize(), and rethrow only when no source can answer the registry at all.
builder-io-integration
Bot
changed the title
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@builderio-bot look at the latest PR feedback and fix anything you agree with. Be skeptical. Reply in each open inline thread with exactly one of:
Or resolve the thread in GitHub. Outdated threads after new commits do not need a new reply. Get CI green and keep the branch mergeable. |
…14ffd3d0782c4ded89a5
A 401/403 cannot distinguish a deployment trust failure from a caller the gateway refused, so the manifest served in its place is unverified. Persisting it minted the org row that filterWorkspaceAppsByAccess then accepted, and reconciling deleted workspace_apps rows and their shares on the word of a manifest no authoritative registry confirmed. Annotate from existing rows only on that path: no insert, no update, and no reconcile. Reconciliation now requires persistence structurally rather than by caller discipline. The unavailable-gateway paths keep reconciling as before.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit addresses both previously reported issues: gateway-denied fallbacks are now read-only (persist: false), and reconciliation is disabled when the fallback source is unverified. The added SQL-observation test confirms that denied fallback discovery does not insert, update, or delete registry state, while ordinary unavailable-gateway behavior continues to reconcile. The two prior review threads were verified fixed and resolved.
New Finding
- 🟡 MEDIUM: Read-only fallback discovery can hide newly deployed manifest apps because access filtering still requires an existing
workspace_appsrow; the current regression test's permissiveresolveAccessmock does not cover an absent-row result.
The gateway-first flow, error propagation, and non-denied fallback behavior remain sound. This is a standard-risk incremental review.
🧪 Browser testing: Will run after this review (PR touches UI code)
| return finalize(localFilesystemApps, { | ||
| reconcile: !unverified, | ||
| persist: !unverified, |
There was a problem hiding this comment.
🟡 Preserve visibility for manifest apps without existing registry rows
When a gateway 401/403 selects this read-only fallback, persist: false skips creating rows for manifest apps, but filterWorkspaceAppsByAccess() still resolves access through workspace-app records. For a newly deployed app with no existing row, that lookup returns no access and filters the app out, so the fallback can show only Dispatch and fail to keep the Apps page readable. Add a read-only access projection/decision for manifest entries (without minting rows), and cover the absent-row case with resolveAccess returning null.
Additional Info
Found by 1 of 2 parallel reviewers; the other found no new issues. Focused app-creation-store.spec.ts run reported 60/60 passing, but its resolveAccess mock grants all candidates and does not exercise absent rows.
There was a problem hiding this comment.
Required — not fixing the visibility, fixed the silence.
I don't agree with restoring visibility here. The two options that would do it are both worse than the current behaviour:
- Mint the row — that is exactly the circular authorization the 🔴 thread above asked me to remove. We would be creating the access record that the access filter then accepts.
- Exempt rowless apps from the access check — that grants visibility with no access record at all, for precisely the apps we know least about, on a read we could not authenticate.
Dropping them is the fail-closed answer, and it matches what filterWorkspaceAppsByAccess already does when a per-app lookup fails (allowed: false). Scope is also narrower than it reads: this only affects an app deployed since the last successful registry read, during an active denial. Every app with an existing row still renders, which is the reported beta case.
Where you're right is the silence. Access for those apps is unknown, not denied, and dropping them without a word is the same coercion this PR is fixing in the other direction. ensureWorkspaceAppRecords now collects them and logs the count and the ids, so an operator can tell a benign degraded read from one hiding the whole workspace. Covered by names the apps an unverified read cannot resolve access for, mutation-checked.
Good catch on the mock, too — resolveAccess granting every candidate did mean the focused run never exercised absent rows. The new assertions key off the warning rather than the access filter, so they don't depend on that mock. While fixing this I also found the spec's afterEach was re-installing a second, drifted copy of the database mock; that's now a single shared factory.
|
There was a problem with your request, please try again later. Error id: |
…olve Skipping rowless manifest apps on the denied path is the right fail-closed answer, but access for them is unknown rather than denied, and dropping them silently is the coercion this hides behind. Name them and their count so an operator can tell a benign degraded read from one hiding the whole workspace.
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commit adds unresolved-row tracking and warning logs for read-only gateway-denied fallbacks. The previously reported absent-registry-row visibility issue remains open and was intentionally not reposted. The new tests improve observability, but one test harness issue means the mutation-safety assertion is currently ineffective after afterEach resets the DB mock.
New Findings
- 🟡 MEDIUM: A local gateway 401/403 is ignored instead of being classified as a denial, allowing trusted fallback persistence/reconciliation after an explicit authorization failure.
- 🟡 MEDIUM: The no-mutation regression test does not actually record SQL after teardown resets the mock, so it can pass even if the implementation mutates registry state.
The earlier fallback authorization and destructive-reconciliation findings remain resolved. This is a standard-risk incremental review.
🧪 Browser testing: Will run after this review (PR touches UI code)
| gatewayApps = await readWorkspaceAppsFromGateway(); | ||
| } catch (error) { | ||
| if (!(error instanceof WorkspaceAppsGatewayAuthorizationError)) throw error; | ||
| // A denial answers for the gateway hop, not for what this caller may see. |
There was a problem hiding this comment.
🟡 Classify local gateway authorization failures as denials
The local gateway branch only handles localResponse.ok; a local 401/403 falls through and returns null, so gatewayDenial is never set. The caller then treats the deployment manifest as authoritative and may persist/reconcile rows despite an explicit authorization failure. Propagate local 401/403 through WorkspaceAppsGatewayAuthorizationError, while retaining trusted fallback behavior for unavailable/404 responses.
Additional Info
Found by 1 of 2 parallel reviewers; verified in readWorkspaceAppsFromGateway local branch at lines 1747-1765.
There was a problem hiding this comment.
Required — not fixing. The mechanics are as you describe: a local 401/403 falls through, and with no Authorization header or a same-origin gateway the function returns null, so the manifest is treated as authoritative. But I don't think the denial downgrade should extend there.
That branch is gated on isLocalWorkspaceGateway, which is loopback only — localhost, 127.0.0.1, 0.0.0.0, ::1. The reason a denial downgrades trust in the hosted path is specifically a cross-deployment trust gap: the receiver resolves org membership against its own database, so its refusal tells us nothing reliable about this caller. A loopback gateway is the developer's own dev server in the same process group; there is no tenant boundary being crossed and nothing to be misled about. The local branch is also normally called unauthenticated, so a 401 there means the dev gateway is misconfigured rather than that a trust relationship failed.
The change would also carry a real cost. Downgrading to persist: false in local dev means registry rows never get created, filterWorkspaceAppsByAccess then drops every app, and the local Apps page goes empty — reintroducing exactly the symptom this PR exists to fix, on the one path where it is most likely to be hit by accident.
This behaviour is also unchanged by this PR; the local branch predates it. If we do want loopback denials classified, it deserves its own change with local-dev verification rather than riding along here.
The afterEach reset reinstalled a second, drifted copy of the database mock that did not record statements, so the no-mutation assertion saw an empty list for every test after the first and could pass vacuously. Reset to the shared recording factory and assert the recorder observed a read before trusting an empty mutation list.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Incremental Code Review Summary
The latest commit fixes the previously reported mutation-test issue: the reset DB mock now uses the SQL-tracking implementation, and the test proves the recorder is live before asserting no mutations. That review thread was resolved. The two other prior findings remain unresolved and were not reposted.
New Finding
- 🟡 MEDIUM: Non-authorization gateway failures such as 500, 429, timeouts, and malformed responses are collapsed to
null, after which the deployment manifest is treated as authoritative and may persist/reconcile stale data. The fallback should distinguish an explicitly absent route from an operational failure and keep outage fallbacks read-only or fail closed.
The standard-risk incremental review otherwise found no additional confirmed issues. Focused unit coverage reported 61 passing tests.
🧪 Browser testing: Will run after this review (PR touches UI code)
| warnWorkspaceAppsGatewayDenial(gatewayDenial, "deployment manifest"); | ||
| return finalize(manifestApps, { | ||
| reconcile: !unverified, | ||
| persist: !unverified, |
There was a problem hiding this comment.
🟡 Do not reconcile manifests after transient gateway failures
readWorkspaceAppsFromGateway() returns null for non-401/403 failures such as 5xx, 429, timeouts, and malformed responses. This branch then treats gatewayDenial === null as authoritative and calls finalize with persistence and reconciliation enabled, so a temporary registry outage can make a stale manifest insert/update records or delete org-owned rows and shares. Distinguish route absence from operational failure and keep outage fallbacks read-only or fail closed; add coverage for 500/429/timeout cases.
Additional Info
Found by 1 of 2 parallel reviewers; focused suite reported 61 passing tests.
There was a problem hiding this comment.
Required — not fixing here, but I agree it's a real gap and it should be the follow-up.
You're right on the mechanics, and right that the line I drew is not the fully principled one. The honest taxonomy is:
- 404 / no gateway configured — the registry genuinely is not there, so the deployment manifest is the authority and reconciling is correct.
- 401/403 — the registry exists and refused us.
- 5xx, 429, timeout, malformed body — the registry exists and failed to answer us.
The last two are the same epistemic state: we have no authoritative list, so deleting rows and shares on the manifest's word is unjustified in both. I'm not going to argue otherwise.
The reason I'm not extending it in this PR is scope and shape, not disagreement. readWorkspaceAppsFromGateway() collapses seven distinct outcomes into a single null — unset URL, malformed URL, local non-ok, same-origin self-fetch, missing authorization, 404, 5xx/throw. Telling "never configured" apart from "configured and failed" means turning that into a discriminated return and re-testing each exit, and every one of those paths needs real coverage rather than the vacuous kind this spec just taught me to distrust. That is a bigger change than I want riding on a hotfix for a regression that is breaking beta right now (#5009, 4.5 hours between merge and the user report).
The 401/403 case was in scope precisely because it is the one this PR introduced. The transient-failure case is pre-existing on main and unchanged by this diff, so landing this does not make it worse.
Happy to open the follow-up for the discriminated-outcome refactor if you'd like it tracked.
|
DO NOT MERGE |
3 participants
Summary
Fixes the Dispatch
/appspage showing two "Couldn't load data" error cards by falling back to the deployment-owned app manifest when the hosted workspace registry denies a read with HTTP 401/403.Problem
Factory item
dfff98617755815c719dd69df620d5a88ab93d242e145cd980a443d48edac00f(Slack thread: https://slack.com/app_redirect?team=T0GCV21GE&channel=C0ATH3CCZT4&message_ts=1789454640.836049) reported that the Dispatch/appsroute (beta.dispatch.agent-native.com/apps) failed to load both the "Your apps" and "Other apps" sections, each showing a red "Couldn't load data" card, while individual app tiles below rendered fine.Root cause:
listWorkspaceAppspreviously treated any 401/403 from the hosted workspace apps gateway as a hard failure and rethrew it, even though a deployment-owned manifest describing the same mounted apps was available and still access-filtered per caller. Because the curated template catalog (listCuratedWorkspaceTemplates) reads the same registry just to mark apps as installed, a single gateway denial produced two independent error cards on the page. The gateway denial itself stems from the receiver resolving org membership against its own database — a cross-deployment trust gap reads as 403 for every user at once, not just misconfigured ones.Solution
Catch
WorkspaceAppsGatewayAuthorizationErrorfrom the gateway read and fall back to local filesystem apps or the deployment manifest (env/manifest file) instead of immediately rethrowing. The fallback sources are still access-filtered per caller infinalize(), so security is preserved. Only when no fallback source can answer at all does the original denial get rethrown, and aconsole.warnis logged whenever a fallback is used so the denial stays diagnosable.Key Changes
listWorkspaceAppsinapp-creation-store.tsnow wraps the gateway read in try/catch, capturing aWorkspaceAppsGatewayAuthorizationErrorasgatewayDenialinstead of throwing immediately.warnWorkspaceAppsGatewayDenialhelper to log which fallback source (local filesystem or deployment manifest) served the request after a gateway denial.gatewayDenialis rethrown so a true "nothing can answer" case still fails loudly.listCuratedWorkspaceTemplatesalso stays readable and correctly reflects installed apps when the registry denies the read, since it shares the same underlying registry call.dispatch-registry-denial-fallback.mddocumenting the patch for@agent-native/dispatch.To clone this PR locally use the Github CLI with command
gh pr checkout 5039You can tag me at @BuilderIO for anything you want me to fix or change