Harden secret store and resolve catalog overrides#1891
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 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing type guard in
getcauses unsafe property access- Added the missing
isPlatformError(cause)guard before accessingcause.reason._tagin thegetmethod, consistent with the guards used inremoveandgetOrCreateRandom.
- Added the missing
Or push these changes by commenting:
@cursor push a0aa6a4824
Preview (a0aa6a4824)
diff --git a/apps/server/src/auth/Layers/ServerSecretStore.ts b/apps/server/src/auth/Layers/ServerSecretStore.ts
--- a/apps/server/src/auth/Layers/ServerSecretStore.ts
+++ b/apps/server/src/auth/Layers/ServerSecretStore.ts
@@ -35,7 +35,7 @@
fileSystem.readFile(resolveSecretPath(name)).pipe(
Effect.map((bytes) => Uint8Array.from(bytes)),
Effect.catch((cause) =>
- cause.reason._tag === "NotFound"
+ isPlatformError(cause) && cause.reason._tag === "NotFound"
? Effect.succeed(null)
: Effect.fail(
new SecretStoreError({You can send follow-ups to the cloud agent here.
ApprovabilityVerdict: Approved This is a type-hardening and refactoring PR that improves type safety (narrowing You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Potentially incorrect
isPlatformErrortag check breaks race handling- Replaced
Predicate.isTagged(u, "PlatformError")(which checks_tag === "PlatformError"— a value neitherBadArgumentnorSystemErrorcarries) withinstanceof PlatformError.PlatformError, consistent with the existing check in theremovefunction.
- Replaced
Or push these changes by commenting:
@cursor push e2bb5af8e5
Preview (e2bb5af8e5)
diff --git a/apps/server/src/auth/Layers/ServerSecretStore.ts b/apps/server/src/auth/Layers/ServerSecretStore.ts
--- a/apps/server/src/auth/Layers/ServerSecretStore.ts
+++ b/apps/server/src/auth/Layers/ServerSecretStore.ts
@@ -1,6 +1,6 @@
import * as Crypto from "node:crypto";
-import { Effect, FileSystem, Layer, Path, Predicate } from "effect";
+import { Effect, FileSystem, Layer, Path } from "effect";
import * as PlatformError from "effect/PlatformError";
import { ServerConfig } from "../../config.ts";
@@ -28,9 +28,6 @@
const resolveSecretPath = (name: string) => path.join(serverConfig.secretsDir, `${name}.bin`);
- const isPlatformError = (u: unknown): u is PlatformError.PlatformError =>
- Predicate.isTagged(u, "PlatformError");
-
const get: ServerSecretStoreShape["get"] = (name) =>
fileSystem.readFile(resolveSecretPath(name)).pipe(
Effect.map((bytes) => Uint8Array.from(bytes)),
@@ -105,7 +102,8 @@
return create(name, generated).pipe(
Effect.as(Uint8Array.from(generated)),
Effect.catchTag("SecretStoreError", (error) =>
- isPlatformError(error.cause) && error.cause.reason._tag === "AlreadyExists"
+ error.cause instanceof PlatformError.PlatformError &&
+ error.cause.reason._tag === "AlreadyExists"
? get(name).pipe(
Effect.flatMap((created) =>
created !== nullYou can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Inconsistent
instanceofcheck inremovevs rest of file- Replaced
cause instanceof PlatformError.PlatformErrorwithisPlatformError(cause)in theremovemethod to match the pattern used ingetOrCreateRandomand avoid instanceof fragility across module boundaries.
- Replaced
Or push these changes by commenting:
@cursor push d9c794b0ad
Preview (d9c794b0ad)
diff --git a/apps/server/src/auth/Layers/ServerSecretStore.ts b/apps/server/src/auth/Layers/ServerSecretStore.ts
--- a/apps/server/src/auth/Layers/ServerSecretStore.ts
+++ b/apps/server/src/auth/Layers/ServerSecretStore.ts
@@ -126,7 +126,7 @@
const remove: ServerSecretStoreShape["remove"] = (name) =>
fileSystem.remove(resolveSecretPath(name)).pipe(
Effect.catch((cause) =>
- cause instanceof PlatformError.PlatformError && cause.reason._tag === "NotFound"
+ isPlatformError(cause) && cause.reason._tag === "NotFound"
? Effect.void
: Effect.fail(
new SecretStoreError({You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit e016fe7. Configure here.
- harden server secret store file handling and publish-time catalog resolution - tighten desktop artifact typing - remove redundant instanceof checks from shared provider and runtime paths Co-authored-by: codex <codex@users.noreply.github.com>
384c33e to
5aced13
Compare


Closes #1887
Summary
instanceofchecks from the non-Claude typed error paths touched by this PRTesting
bun fmtbun lintbun typecheckbun run testNote
Medium Risk
Touches secret storage and several error-mapping paths; while mostly defensive/error-message changes, regressions could affect secret reads/writes and provider/bootstrap resiliency.
Overview
Publishing now includes workspace overrides. The server
publishCLI writes a temporarypackage.jsonthat resolves bothdependenciesand root-leveloverridesfrom the workspace catalog, with tighter typing around the generated metadata.Error handling is standardized and more informative. The server secret store tightens concurrent/missing-file handling using tagged
PlatformErrordetection; several layers (GitHubCli,GitCore,CodexAdapter/CodexProvider, terminal manager, workspace entries, websocket bootstrap logging) simplifyinstanceofbranches and improve propagated error detail/log payloads. On the web side,BootstrapHttpErrorbecomes aneffectData.TaggedErrorand transient-retry detection switches to tag-based predicates.Dependency/catalog typing is tightened.
resolveCatalogDependencies(and callers like desktop artifact build) now operate onRecord<string, string>instead ofunknown.Reviewed by Cursor Bugbot for commit 83abc76. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden secret store error handling and resolve catalog overrides in publish flow
ServerSecretStoreerror handling to branch oncause.reason._tagdirectly instead of using removedisMissingSecretFileError/isAlreadyExistsSecretFileErrorhelpers, usingPredicate.isTaggedforPlatformErrordetection.publishCmdhandler in cli.ts to write a resolvedoverridesfield (from the root workspace catalog) alongside dependencies in the temporarypackage.jsonused for npm publish.instanceof-basedBootstrapHttpErrorwith aData.TaggedErrorandPredicate.isTaggedguard across auth.ts and context.ts.error.messagedirectly rather thanString(error)fallbacks.resolveCatalogDependenciesparameter and return types fromRecord<string, unknown>toRecord<string, string>.Macroscope summarized 83abc76.