refactor(workflow): strengthen contracts and zod boundaries - #100
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/workflow-types.ts (1)
13-44: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsolidate the duplicate provider ID aliases.
AgentProviderIdis defined here andWorkflowProviderIdis defined inworkflow-contracts.ts, both asLocalAgentProvider. Either import/export the consolidatedWorkflowProviderIdhere and reuse it atWorkflowAgentCallRecord.provider,AgentCacheKeyInput.provider, andbuildAgentCacheKeyInput(provider: ...), or exportAgentProviderIdfromworkflow-contracts.tsif callers should depend on the single contract alias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-types.ts` around lines 13 - 44, Consolidate the provider ID aliases by using a single contract-level alias: import and re-export WorkflowProviderId from workflow-contracts.ts, then update WorkflowAgentCallRecord.provider, AgentCacheKeyInput.provider, and the buildAgentCacheKeyInput(provider: ...) parameter to use it consistently. Remove the duplicate AgentProviderId definition rather than maintaining parallel aliases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/json-types.ts`:
- Around line 40-42: Update parseJsonText to catch JSON.parse failures and
convert malformed input into the same clear validation-error type used by
parseJsonValue, while preserving normal parsing and validation behavior for
valid JSON.
In `@src/workflow-script.ts`:
- Around line 186-201: Update validateMeta to enable Zod's reportInput option
when calling workflowMetaSchema.safeParse, so issue.input is available for
distinguishing missing values from present values with invalid types. Preserve
the existing required-field handling for undefined input and allow present
wrong-type fields such as meta.concurrency to use the normal validation error
path.
In `@src/workflow-store.ts`:
- Around line 566-572: Update the bulk mapping paths in listRuns() and
listAgentCalls(), including rowToRun, rowToEvent, and rowToAgentCall, so invalid
persisted enum-like values are handled per row with safe parsing and corrupted
rows are skipped or flagged rather than aborting the entire list operation.
Preserve strict .parse() behavior for single-record getRun() and getAgentCall()
lookups.
---
Nitpick comments:
In `@src/workflow-types.ts`:
- Around line 13-44: Consolidate the provider ID aliases by using a single
contract-level alias: import and re-export WorkflowProviderId from
workflow-contracts.ts, then update WorkflowAgentCallRecord.provider,
AgentCacheKeyInput.provider, and the buildAgentCacheKeyInput(provider: ...)
parameter to use it consistently. Remove the duplicate AgentProviderId
definition rather than maintaining parallel aliases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0546af15-0cb2-4c3c-ad46-41c0315e5417
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
package.jsonsrc/json-types.tssrc/local-agent-adapters.test.tssrc/local-agent-adapters.tssrc/local-agent-capabilities.tssrc/local-agent-profiles.tssrc/local-agent-runtime.tssrc/workflow-api.tssrc/workflow-cli.tssrc/workflow-contracts.test.tssrc/workflow-contracts.tssrc/workflow-engine.tssrc/workflow-files.tssrc/workflow-replay.tssrc/workflow-sandbox.test.tssrc/workflow-sandbox.tssrc/workflow-schema.test.tssrc/workflow-schema.tssrc/workflow-script.test.tssrc/workflow-script.tssrc/workflow-store.test.tssrc/workflow-store.tssrc/workflow-tools.tssrc/workflow-types.ts
| export function parseJsonText(text: string): JsonValue { | ||
| return parseJsonValue(JSON.parse(text) as unknown); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
parseJsonText lets raw JSON.parse errors escape uncaught.
JSON.parse throws a native SyntaxError (not a Zod validation error) on malformed text. Since this helper is used to reload persisted state (e.g. workflow run args/JSON blobs), a corrupted persisted value would surface as an unstructured SyntaxError rather than a clear validation failure at the call site.
🛡️ Proposed fix
export function parseJsonText(text: string): JsonValue {
- return parseJsonValue(JSON.parse(text) as unknown);
+ let parsed: unknown;
+ try {
+ parsed = JSON.parse(text);
+ } catch (error) {
+ throw new Error(`Invalid JSON text: ${error instanceof Error ? error.message : String(error)}`);
+ }
+ return parseJsonValue(parsed);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parseJsonText(text: string): JsonValue { | |
| return parseJsonValue(JSON.parse(text) as unknown); | |
| } | |
| export function parseJsonText(text: string): JsonValue { | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(text); | |
| } catch (error) { | |
| throw new Error(`Invalid JSON text: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| return parseJsonValue(parsed); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/json-types.ts` around lines 40 - 42, Update parseJsonText to catch
JSON.parse failures and convert malformed input into the same clear
validation-error type used by parseJsonValue, while preserving normal parsing
and validation behavior for valid JSON.
There was a problem hiding this comment.
Not included in the finalized hardening stack. Persisted-state call sites already wrap malformed JSON with contextual WorkflowStoredDataError or explicitly treat it as a replay miss; this comment concerns the generic helper’s standalone error wording. It remains reasonable cleanup, but is not a blocker for cross-harness workflow orchestration.
There was a problem hiding this comment.
@Waishnav, acknowledged. Given the existing call-site handling, this is non-blocking for the workflow hardening scope. I’ll treat helper-level parse-error normalization as optional follow-up cleanup rather than a required change for this PR.
Would you like me to open a follow-up issue for the standalone parseJsonText error consistency improvement?
✏️ Learnings added
Learnt from: Waishnav
Repo: Waishnav/devspace PR: 100
File: src/json-types.ts:40-42
Timestamp: 2026-07-26T21:01:08.221Z
Learning: In `src/json-types.ts`, `parseJsonText` may expose native `JSON.parse` errors, but persisted workflow-state consumers already either wrap malformed JSON with contextual `WorkflowStoredDataError` or treat it as a replay miss. Helper-level error normalization is optional cleanup, not a blocker for cross-harness workflow orchestration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
refactor(agents): unify provider resolution and subagent guidance
feat(workflow): support agent profiles and project workflows
fix(workflow): replay deterministic call prefixes
fix(workflow): supervise cancellation and terminal lifecycle
feat(workflow): isolate workflow script execution
feat(ui): live workspace and workflow dashboards
feat(workflow): read-only project workflow TUI
feat(workflow): script iteration, replay provenance, and call inspection
refactor(workflow): model operational failures with better-result
1 participant
Summary
agent()output typing viajson-schema-to-ts.parallel()and add typed pipeline overloads.Stack
pr/dw-6-native-schema(PR feat(agents): native schema for codex/claude agent({ schema }) #99)pr/dw-7-typed-contracts-zodTest plan
npm run typechecknpm testnpm run buildSummary by CodeRabbit