fix(core): replace browser/public enums with list-derived unions#690
fix(core): replace browser/public enums with list-derived unions#690rahulkarajgikar wants to merge 1 commit intotruffle-ai:mainfrom
Conversation
|
@rahulkarajgikar is attempting to deploy a commit to the Shaunak's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughReplaces exported enums Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/errors/types.test.ts (1)
1-18: Add one public-entrypoint/type-value smoke test.These assertions cover the leaf module's runtime shape, but the compatibility contract here is broader: consumers should still be able to import from a public barrel and use the same symbol in both type and value positions. A small check like
const scope: ErrorScope = ErrorScope.SYSTEM_PROMPTfromindex.browser.ts(and the storage equivalent) would catch regressions that the current direct-import tests won't.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/errors/types.test.ts` around lines 1 - 18, Add a smoke test that imports the public barrel entrypoint and verifies the symbols work in both type and value positions: import ErrorScope and ErrorType from the public entrypoint (index.browser) and add lines like "const scope: ErrorScope = ErrorScope.SYSTEM_PROMPT" and "const type: ErrorType = ErrorType.PAYMENT_REQUIRED" (and assert they equal the expected values) to ensure consumers can use the same symbol as a type and a value; mirror the same check for the storage/public-node barrel if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/errors/types.test.ts`:
- Around line 1-18: Add a smoke test that imports the public barrel entrypoint
and verifies the symbols work in both type and value positions: import
ErrorScope and ErrorType from the public entrypoint (index.browser) and add
lines like "const scope: ErrorScope = ErrorScope.SYSTEM_PROMPT" and "const type:
ErrorType = ErrorType.PAYMENT_REQUIRED" (and assert they equal the expected
values) to ensure consumers can use the same symbol as a type and a value;
mirror the same check for the storage/public-node barrel if applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6077ca6d-5aae-467c-850e-0889f23cb22c
📒 Files selected for processing (8)
.changeset/tough-countries-sing.mdpackages/core/src/errors/index.tspackages/core/src/errors/types.test.tspackages/core/src/errors/types.tspackages/core/src/index.browser.tspackages/core/src/storage/error-codes.test.tspackages/core/src/storage/error-codes.tspackages/core/src/storage/index.ts
babe8ca to
dc1d3d0
Compare
dc1d3d0 to
0f62817
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/storage/errors.ts (1)
12-356: 🛠️ Refactor suggestion | 🟠 MajorKeep
storage/errors.tsconsuming the canonical storage code definitions.This PR is trying to make
STORAGE_ERROR_CODES/StorageErrorCodethe source of truth, but this file now repeats every emittedcodeliteral inline in bothDextoRuntimeErrorandDextoValidationErrorpayloads. That makespackages/core/src/storage/error-codes.tsnon-canonical again for this module and makes drift easy the next time a code is renamed or added. Please route these factories back through the compatibility namespace or a helper derived from the exported list instead of hardcoding the strings here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/storage/errors.ts` around lines 12 - 356, The factories in errors.ts (e.g., connectionFailed, notConnected, managerNotInitialized, blobInvalidConfig, blobSizeExceeded, etc.) hardcode error code string literals instead of using the canonical STORAGE_ERROR_CODES / StorageErrorCode; update each DextoRuntimeError and DextoValidationError construction to import and reference the canonical code from the storage/error-codes.ts compatibility export (or a small helper that maps enum keys to strings) so the code values come from that single source of truth rather than inline literals; ensure both runtime factories (connectionFailed, readFailed, blobOperationFailed, etc.) and validation factories (databaseInvalidConfig, blobInvalidConfig, cacheInvalidConfig) use the canonical symbol when constructing errors.
🧹 Nitpick comments (2)
packages/core/src/utils/result.ts (1)
257-258: Prefer canonical constants for fallback scope/type to avoid drift.Line 257, Line 258, Line 270, and Line 271 hardcode
'agent'/'user'. Since this PR establishes list-derived values as canonical, using exported compatibility constants here would keep defaults tied to the single source of truth.♻️ Suggested refactor
- scope: params.scope ?? 'agent', - type: params.type ?? 'user', + scope: params.scope ?? ErrorScope.AGENT, + type: params.type ?? ErrorType.USER, ... - scope: params.scope ?? 'agent', - type: params.type ?? 'user', + scope: params.scope ?? ErrorScope.AGENT, + type: params.type ?? ErrorType.USER,// Add/import the runtime compatibility namespace from your errors module.Also applies to: 270-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/result.ts` around lines 257 - 258, Replace the hardcoded fallback literals for params.scope and params.type with the canonical runtime compatibility constants exported from your errors module: import the runtime compatibility namespace (e.g., runtimeCompatibility or RUNTIME_COMPATIBILITY) and use its scope and type constants instead of 'agent' and 'user' in the assignments for scope: params.scope ?? ... and type: params.type ?? ... (also update the same replacements at the other occurrences around the second pair of defaults). Ensure you reference the exported constant names from the errors module so defaults stay tied to the single source of truth.packages/core/src/systemPrompt/errors.ts (1)
13-72: Keep core error classifiers on the canonical runtime exports.These raw
'system_prompt'/'user'/'system'literals duplicate the canonical values and, forscope, drop compiler protection because core still acceptsErrorScope | string. I’d keeppackages/coreonErrorScope.SYSTEM_PROMPT/ErrorType.USER/ErrorType.SYSTEM(or module-level constants derived from them) and reserve free-form strings for out-of-core packages that actually need custom scopes.♻️ Suggested shape
+import { ErrorScope, ErrorType } from '../errors/types.js'; import { DextoRuntimeError } from '../errors/DextoRuntimeError.js'; import { SystemPromptErrorCode } from './error-codes.js'; import { safeStringify } from '../utils/safe-stringify.js'; + +const SCOPE = ErrorScope.SYSTEM_PROMPT; static invalidFileType(filePath: string, allowedExtensions: string[]) { return new DextoRuntimeError( SystemPromptErrorCode.FILE_INVALID_TYPE, - 'system_prompt', - 'user', + SCOPE, + ErrorType.USER, `File ${filePath} is not a ${allowedExtensions.join(' or ')} file`, { filePath, allowedExtensions } ); } static fileReadFailed(filePath: string, reason: string) { return new DextoRuntimeError( SystemPromptErrorCode.FILE_READ_FAILED, - 'system_prompt', - 'system', + SCOPE, + ErrorType.SYSTEM, `Failed to read file ${filePath}: ${reason}`, { filePath, reason } ); }Based on learnings: error scopes were broadened to
ErrorScope | stringso packages outside of core can define their own scopes without modifying the coreErrorScopeenum.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/systemPrompt/errors.ts` around lines 13 - 72, The error factory functions (invalidFileType, fileTooLarge, fileReadFailed, unknownContributorSource, invalidContributorConfig) are using raw strings for scope/type ('system_prompt', 'user', 'system'); change those to the canonical core enums (e.g. ErrorScope.SYSTEM_PROMPT and ErrorType.USER / ErrorType.SYSTEM) to preserve compiler-checked values and avoid free-form strings. Update the top of the file to import the enums (or module-level constants) and replace the literal occurrences in each DextoRuntimeError constructor call with the corresponding enum members, leaving the rest of each returned error object intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/storage/errors.ts`:
- Around line 12-356: The factories in errors.ts (e.g., connectionFailed,
notConnected, managerNotInitialized, blobInvalidConfig, blobSizeExceeded, etc.)
hardcode error code string literals instead of using the canonical
STORAGE_ERROR_CODES / StorageErrorCode; update each DextoRuntimeError and
DextoValidationError construction to import and reference the canonical code
from the storage/error-codes.ts compatibility export (or a small helper that
maps enum keys to strings) so the code values come from that single source of
truth rather than inline literals; ensure both runtime factories
(connectionFailed, readFailed, blobOperationFailed, etc.) and validation
factories (databaseInvalidConfig, blobInvalidConfig, cacheInvalidConfig) use the
canonical symbol when constructing errors.
---
Nitpick comments:
In `@packages/core/src/systemPrompt/errors.ts`:
- Around line 13-72: The error factory functions (invalidFileType, fileTooLarge,
fileReadFailed, unknownContributorSource, invalidContributorConfig) are using
raw strings for scope/type ('system_prompt', 'user', 'system'); change those to
the canonical core enums (e.g. ErrorScope.SYSTEM_PROMPT and ErrorType.USER /
ErrorType.SYSTEM) to preserve compiler-checked values and avoid free-form
strings. Update the top of the file to import the enums (or module-level
constants) and replace the literal occurrences in each DextoRuntimeError
constructor call with the corresponding enum members, leaving the rest of each
returned error object intact.
In `@packages/core/src/utils/result.ts`:
- Around line 257-258: Replace the hardcoded fallback literals for params.scope
and params.type with the canonical runtime compatibility constants exported from
your errors module: import the runtime compatibility namespace (e.g.,
runtimeCompatibility or RUNTIME_COMPATIBILITY) and use its scope and type
constants instead of 'agent' and 'user' in the assignments for scope:
params.scope ?? ... and type: params.type ?? ... (also update the same
replacements at the other occurrences around the second pair of defaults).
Ensure you reference the exported constant names from the errors module so
defaults stay tied to the single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e073f3c-6e1b-40d3-a4d7-8864dfe86364
📒 Files selected for processing (76)
.changeset/tough-countries-sing.mdpackages/agent-management/src/config/errors.tspackages/agent-management/src/config/loader.test.tspackages/agent-management/src/plugins/errors.tspackages/agent-management/src/plugins/marketplace/errors.tspackages/agent-management/src/preferences/errors.tspackages/agent-management/src/preferences/loader.test.tspackages/agent-management/src/preferences/schemas.tspackages/agent-management/src/registry/errors.tspackages/agent-management/src/registry/registry.test.tspackages/agent-management/src/resolver.test.tspackages/agent-management/src/runtime/errors.tspackages/agent-management/src/tool-factories/agent-spawner/errors.tspackages/agent-management/src/tool-factories/agent-spawner/runtime.tspackages/agent-management/src/writer.test.tspackages/core/src/agent/DextoAgent.lifecycle.test.tspackages/core/src/agent/errors.tspackages/core/src/approval/errors.tspackages/core/src/context/errors.tspackages/core/src/errors/DextoRuntimeError.tspackages/core/src/errors/index.tspackages/core/src/errors/types.tspackages/core/src/hooks/manager.tspackages/core/src/index.browser.tspackages/core/src/llm/errors.tspackages/core/src/llm/executor/turn-executor.tspackages/core/src/llm/providers/codex-app-server.tspackages/core/src/llm/providers/local/errors.tspackages/core/src/llm/registry/index.test.tspackages/core/src/llm/registry/sync.tspackages/core/src/llm/resolver.tspackages/core/src/llm/schemas.tspackages/core/src/llm/services/vercel.integration.test.tspackages/core/src/llm/validation.tspackages/core/src/logger/v2/errors.tspackages/core/src/mcp/errors.tspackages/core/src/mcp/manager.test.tspackages/core/src/mcp/resolver.tspackages/core/src/mcp/schemas.tspackages/core/src/memory/errors.tspackages/core/src/prompts/errors.tspackages/core/src/resources/errors.tspackages/core/src/session/chat-session.tspackages/core/src/session/errors.tspackages/core/src/session/history/database.test.tspackages/core/src/session/session-manager.test.tspackages/core/src/storage/error-codes.tspackages/core/src/storage/errors.tspackages/core/src/storage/index.tspackages/core/src/systemPrompt/contributors.test.tspackages/core/src/systemPrompt/errors.tspackages/core/src/systemPrompt/manager.test.tspackages/core/src/telemetry/errors.tspackages/core/src/tools/errors.tspackages/core/src/tools/tool-manager.integration.test.tspackages/core/src/tools/tool-manager.test.tspackages/core/src/tools/tool-manager.tspackages/core/src/utils/result.test.tspackages/core/src/utils/result.tspackages/core/src/workspace/errors.tspackages/server/src/hono/middleware/error.tspackages/server/src/hono/routes/agents.tspackages/server/src/hono/routes/llm.tspackages/server/src/hono/routes/schedules.tspackages/server/src/hono/routes/sessions.tspackages/server/src/hono/routes/system-prompt.tspackages/storage/src/cache/schemas.tspackages/storage/src/database/schemas.tspackages/tools-builtins/src/implementations/delegate-to-url-tool.test.tspackages/tools-builtins/src/implementations/delegate-to-url-tool.tspackages/tools-builtins/src/implementations/http-request-tool.tspackages/tools-filesystem/src/errors.tspackages/tools-plan/src/errors.tspackages/tools-process/src/errors.tspackages/tools-scheduler/src/errors.tspackages/tools-todo/src/errors.ts
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/errors/DextoRuntimeError.ts
- .changeset/tough-countries-sing.md
- packages/core/src/llm/resolver.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/errors/index.ts
- packages/core/src/storage/index.ts
- packages/core/src/storage/error-codes.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/llm/schemas.ts (1)
135-136: Optional: centralize repeated issue metadata literals to reduce typo risk.The same
{ scope: 'llm', type: 'user' }pair is repeated in many branches. A small shared constant/helper would improve maintainability.♻️ Optional refactor sketch
+const LLM_USER_ISSUE_META = { scope: 'llm', type: 'user' } as const; +const LLM_SYSTEM_ISSUE_META = { scope: 'llm', type: 'system' } as const; ... - params: { - code: LLMErrorCode.MODEL_INCOMPATIBLE, - scope: 'llm', - type: 'user', - }, + params: { + code: LLMErrorCode.MODEL_INCOMPATIBLE, + ...LLM_USER_ISSUE_META, + }, ... - params: { - code: LLMErrorCode.REQUEST_INVALID_SCHEMA, - scope: 'llm', - type: 'system', - }, + params: { + code: LLMErrorCode.REQUEST_INVALID_SCHEMA, + ...LLM_SYSTEM_ISSUE_META, + },Also applies to: 151-152, 172-173, 196-197, 227-228, 250-251, 265-266, 320-321, 335-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/llm/schemas.ts` around lines 135 - 136, The repeated literal { scope: 'llm', type: 'user' } should be centralized: add a single shared constant (e.g., LLM_USER_METADATA) in this module and replace each inline occurrence (the object literals shown in the branches) with that constant to avoid typos and improve maintainability; if these metadata objects are used outside the file, export the constant (name it clearly like LLM_USER_ISSUE_METADATA) and update all references in this file (and imports elsewhere if exported) so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/llm/schemas.ts`:
- Around line 135-136: The repeated literal { scope: 'llm', type: 'user' }
should be centralized: add a single shared constant (e.g., LLM_USER_METADATA) in
this module and replace each inline occurrence (the object literals shown in the
branches) with that constant to avoid typos and improve maintainability; if
these metadata objects are used outside the file, export the constant (name it
clearly like LLM_USER_ISSUE_METADATA) and update all references in this file
(and imports elsewhere if exported) so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aca46cd3-2429-4f13-a5d1-cf16c0e2c817
📒 Files selected for processing (77)
.changeset/tough-countries-sing.mddocs/static/openapi/openapi.jsonpackages/agent-management/src/config/errors.tspackages/agent-management/src/config/loader.test.tspackages/agent-management/src/plugins/errors.tspackages/agent-management/src/plugins/marketplace/errors.tspackages/agent-management/src/preferences/errors.tspackages/agent-management/src/preferences/loader.test.tspackages/agent-management/src/preferences/schemas.tspackages/agent-management/src/registry/errors.tspackages/agent-management/src/registry/registry.test.tspackages/agent-management/src/resolver.test.tspackages/agent-management/src/runtime/errors.tspackages/agent-management/src/tool-factories/agent-spawner/errors.tspackages/agent-management/src/tool-factories/agent-spawner/runtime.tspackages/agent-management/src/writer.test.tspackages/core/src/agent/DextoAgent.lifecycle.test.tspackages/core/src/agent/errors.tspackages/core/src/approval/errors.tspackages/core/src/context/errors.tspackages/core/src/errors/DextoRuntimeError.tspackages/core/src/errors/index.tspackages/core/src/errors/types.tspackages/core/src/hooks/manager.tspackages/core/src/index.browser.tspackages/core/src/llm/errors.tspackages/core/src/llm/executor/turn-executor.tspackages/core/src/llm/providers/codex-app-server.tspackages/core/src/llm/providers/local/errors.tspackages/core/src/llm/registry/index.test.tspackages/core/src/llm/registry/sync.tspackages/core/src/llm/resolver.tspackages/core/src/llm/schemas.tspackages/core/src/llm/services/vercel.integration.test.tspackages/core/src/llm/validation.tspackages/core/src/logger/v2/errors.tspackages/core/src/mcp/errors.tspackages/core/src/mcp/manager.test.tspackages/core/src/mcp/resolver.tspackages/core/src/mcp/schemas.tspackages/core/src/memory/errors.tspackages/core/src/prompts/errors.tspackages/core/src/resources/errors.tspackages/core/src/session/chat-session.tspackages/core/src/session/errors.tspackages/core/src/session/history/database.test.tspackages/core/src/session/session-manager.test.tspackages/core/src/storage/error-codes.tspackages/core/src/storage/errors.tspackages/core/src/storage/index.tspackages/core/src/systemPrompt/contributors.test.tspackages/core/src/systemPrompt/errors.tspackages/core/src/systemPrompt/manager.test.tspackages/core/src/telemetry/errors.tspackages/core/src/tools/errors.tspackages/core/src/tools/tool-manager.integration.test.tspackages/core/src/tools/tool-manager.test.tspackages/core/src/tools/tool-manager.tspackages/core/src/utils/result.test.tspackages/core/src/utils/result.tspackages/core/src/workspace/errors.tspackages/server/src/hono/middleware/error.tspackages/server/src/hono/routes/agents.tspackages/server/src/hono/routes/llm.tspackages/server/src/hono/routes/schedules.tspackages/server/src/hono/routes/sessions.tspackages/server/src/hono/routes/system-prompt.tspackages/storage/src/cache/schemas.tspackages/storage/src/database/schemas.tspackages/tools-builtins/src/implementations/delegate-to-url-tool.test.tspackages/tools-builtins/src/implementations/delegate-to-url-tool.tspackages/tools-builtins/src/implementations/http-request-tool.tspackages/tools-filesystem/src/errors.tspackages/tools-plan/src/errors.tspackages/tools-process/src/errors.tspackages/tools-scheduler/src/errors.tspackages/tools-todo/src/errors.ts
✅ Files skipped from review due to trivial changes (27)
- packages/core/src/mcp/schemas.ts
- packages/core/src/systemPrompt/contributors.test.ts
- .changeset/tough-countries-sing.md
- packages/core/src/memory/errors.ts
- packages/core/src/mcp/resolver.ts
- packages/core/src/workspace/errors.ts
- packages/server/src/hono/routes/agents.ts
- packages/agent-management/src/preferences/loader.test.ts
- packages/agent-management/src/config/loader.test.ts
- packages/core/src/errors/DextoRuntimeError.ts
- packages/core/src/llm/registry/sync.ts
- packages/core/src/prompts/errors.ts
- packages/storage/src/database/schemas.ts
- packages/tools-todo/src/errors.ts
- packages/core/src/agent/DextoAgent.lifecycle.test.ts
- packages/agent-management/src/resolver.test.ts
- packages/tools-plan/src/errors.ts
- packages/core/src/systemPrompt/errors.ts
- packages/core/src/utils/result.ts
- packages/agent-management/src/tool-factories/agent-spawner/errors.ts
- packages/tools-builtins/src/implementations/delegate-to-url-tool.test.ts
- packages/core/src/mcp/errors.ts
- packages/core/src/telemetry/errors.ts
- packages/core/src/resources/errors.ts
- docs/static/openapi/openapi.json
- packages/core/src/llm/errors.ts
- packages/tools-builtins/src/implementations/delegate-to-url-tool.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/core/src/mcp/manager.test.ts
- packages/agent-management/src/writer.test.ts
- packages/core/src/llm/validation.ts
- packages/core/src/llm/registry/index.test.ts
- packages/agent-management/src/plugins/errors.ts
- packages/core/src/session/history/database.test.ts
- packages/core/src/session/errors.ts
- packages/core/src/systemPrompt/manager.test.ts
- packages/core/src/storage/index.ts
- packages/core/src/llm/services/vercel.integration.test.ts
- packages/server/src/hono/routes/llm.ts
- packages/core/src/llm/resolver.ts
- packages/server/src/hono/middleware/error.ts
- packages/core/src/llm/providers/local/errors.ts
- packages/core/src/hooks/manager.ts
- packages/core/src/llm/executor/turn-executor.ts
- packages/server/src/hono/routes/schedules.ts
- packages/tools-scheduler/src/errors.ts
- packages/core/src/logger/v2/errors.ts
- packages/core/src/errors/index.ts
- packages/core/src/approval/errors.ts
- packages/core/src/session/chat-session.ts
- packages/core/src/utils/result.test.ts
- packages/core/src/errors/types.ts
- packages/core/src/tools/errors.ts
- packages/agent-management/src/registry/errors.ts
- packages/tools-builtins/src/implementations/http-request-tool.ts
- packages/core/src/index.browser.ts
- packages/core/src/llm/providers/codex-app-server.ts
- packages/core/src/storage/errors.ts
- packages/core/src/storage/error-codes.ts
|
Closing this one. After reviewing the actual consumer value, replacing ErrorScope / ErrorType in this form creates more churn than benefit, and the real follow-up is tightening the error-layer typing without the current string escape hatch in #691. |
Summary
Validation
Closes #688
Summary by CodeRabbit