Uh oh!
There was an error while loading. Please reload this page.
Preserve asset access failure causes - #3342
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved This PR improves error handling by preserving structured error causes rather than silently swallowing all filesystem errors. The changes are well-tested, improve debuggability, and the author is the primary owner of the modified files. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Bad icon href blocks later sources
- Changed findExistingFile to skip candidates where resolveRelativePathWithinRoot fails (using Effect.orElseSucceed with Option.none) instead of propagating the error, restoring the scan-order behavior so later source files are still consulted.
Or push these changes by commenting:
@cursor push 7ace30344c
Preview (7ace30344c)
diff --git a/apps/server/src/project/ProjectFaviconResolver.test.ts b/apps/server/src/project/ProjectFaviconResolver.test.ts--- a/apps/server/src/project/ProjectFaviconResolver.test.ts+++ b/apps/server/src/project/ProjectFaviconResolver.test.ts@@ -167,22 +167,31 @@
}),
);
- it.effect("rejects icon metadata paths outside the workspace", () =>+ it.effect("skips icon metadata paths outside the workspace", () =>
Effect.gen(function* () {
const resolver = yield* ProjectFaviconResolver.ProjectFaviconResolver;
const cwd = yield* makeTempDir;
yield* writeTextFile(cwd, "index.html", '<link rel="icon" href="../../secret.svg">');
- const error = yield* resolver.resolvePath(cwd).pipe(Effect.flip);+ const resolved = yield* resolver.resolvePath(cwd);- expect(error).toMatchObject({- _tag: "ProjectFaviconResolutionError",- operation: "resolve-path",- workspaceRoot: cwd,- relativePath: "../secret.svg",- });- expect(error.cause).toBeInstanceOf(WorkspacePaths.WorkspacePathOutsideRootError);+ expect(resolved).toBeNull();
}),
);
++ it.effect("skips outside-root href and finds valid icon from later source", () =>+ Effect.gen(function* () {+ const resolver = yield* ProjectFaviconResolver.ProjectFaviconResolver;+ const cwd = yield* makeTempDir;+ yield* writeTextFile(cwd, "index.html", '<link rel="icon" href="../../secret.svg">');+ yield* writeTextFile(cwd, "public/index.html", '<link rel="icon" href="/brand/logo.svg">');+ yield* writeTextFile(cwd, "public/brand/logo.svg", "<svg>brand</svg>");++ const resolved = yield* resolver.resolvePath(cwd);++ expect(resolved).not.toBeNull();+ expect(resolved).toContain("public/brand/logo.svg");+ }),+ );
});
});
diff --git a/apps/server/src/project/ProjectFaviconResolver.ts b/apps/server/src/project/ProjectFaviconResolver.ts--- a/apps/server/src/project/ProjectFaviconResolver.ts+++ b/apps/server/src/project/ProjectFaviconResolver.ts@@ -134,30 +134,26 @@
relativePath,
})
.pipe(
- Effect.mapError(- (cause) =>- new ProjectFaviconResolutionError({- operation: "resolve-path",- workspaceRoot: projectCwd,- relativePath,- cause,- }),- ),+ Effect.map(Option.some),+ Effect.orElseSucceed(() => Option.none<{ absolutePath: string; relativePath: string }>()),
);
- const stats = yield* optionOnNotFound(fileSystem.stat(candidate.absolutePath)).pipe(+ if (Option.isNone(candidate)) {+ continue;+ }+ const stats = yield* optionOnNotFound(fileSystem.stat(candidate.value.absolutePath)).pipe(
Effect.mapError(
(cause) =>
new ProjectFaviconResolutionError({
operation: "stat-candidate",
workspaceRoot: projectCwd,
relativePath,
- absolutePath: candidate.absolutePath,+ absolutePath: candidate.value.absolutePath,
cause,
}),
),
);
if (Option.isSome(stats) && stats.value.type === "File") {
- return candidate.absolutePath;+ return candidate.value.absolutePath;
}
}
return null;You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 26f8074f1ebce58181f24b3acea9e6b29ad846c9. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
26f8074 to
5d5fb30CompareCo-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
5d5fb30 to
09ead13CompareUh oh!
There was an error while loading. Please reload this page.
Co-authored-by: codex <codex@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Summary
Verification
vp test apps/server/src/assets/AssetAccess.test.tsvp check(passes with 20 pre-existing warnings)vp run typecheckNote
Medium Risk
Changes error semantics for asset URL creation and favicon resolution (real FS failures now fail the Effect with structured errors instead of null), which affects RPC clients and observability paths around signed asset access.
Overview
Asset URL issuance and resolution now return
AssetAccessErrorvalues with requiredoperationandresourcefields (contract inpackages/contracts/src/assets.ts), plus stable messages and optionalcause, instead of a generic helper. Workspace-file issuance maps path validation, canonical inspection, favicon, signing key, and related steps to named operations;ws.tsaligns workspace-context failures with the same shape.Filesystem handling adds
optionOnNotFoundso onlyNotFoundis treated as “missing”; permission and other **PlatformError**s propagate.issueAssetUrlsurfaces non-missing failures during canonical workspace inspection (e.g.realPath) asinspect-workspace-asseterrors with the original cause.resolveAssetstill returnsnullat the HTTP boundary but logs causes viaresolveCanonicalWorkspaceFileForRequestand attachment stat failures.Project favicon introduces
ProjectFaviconResolutionErrorwith stage-specificoperationvalues;resolvePathfails on bad workspace normalization, stat/read errors, and path resolution failures instead of collapsing them tonull, while outside-root icon hrefs are skipped and search continues. Tests cover structured errors and preserved causes for asset issuance and favicon resolution.Reviewed by Cursor Bugbot for commit 639917f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add structured
operationandresourcefields toAssetAccessErrorand preserve failure causesAssetAccessErrorin assets.ts with requiredoperation(typed union of all operation labels) andresourcefields, replacing generic failures with structured context.ProjectFaviconResolutionErrorin ProjectFaviconResolver.ts with anoperationfield covering normalization, path resolution, stat, and read failures; previously these cases returnednullsilently.optionOnNotFoundhelpers in bothAssetAccess.tsandProjectFaviconResolver.tsto convertPlatformError.NotFoundintoOption.nonewhile re-throwing otherPlatformErrorcases as structured errors.operationandresourcewhen constructingAssetAccessErrorfor workspace context resolution failures.NotFoundfilesystem errors that were previously swallowed or returned asnullnow propagate as typed errors with contextual fields.Macroscope summarized 639917f.