Skip to content

support the workflow status for cli and mcp and node sdk users#22

Merged
jessehanley merged 4 commits into
bentonow:mainfrom
ziptied:main
Apr 2, 2026
Merged

support the workflow status for cli and mcp and node sdk users#22
jessehanley merged 4 commits into
bentonow:mainfrom
ziptied:main

Conversation

@ziptied

@ziptied ziptied commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Add status field to WorkflowAttributes type to track
workflow state as either 'live' or 'draft'. Update all
workflow test fixtures to include the new status field
and add assertions to verify status values are correctly
returned from the API.

there is also work to flag the project as a ESM package type and keep CJS configs this should allow ingestion into vanilla js ecosystem projects.

Add status field to WorkflowAttributes type to track
workflow state as either 'live' or 'draft'. Update all
workflow test fixtures to include the new status field
and add assertions to verify status values are correctly
returned from the API.
feat: add status field to workflow attributes
…module

Add ESM package type and keep tooling configs in CJS
@greptile-apps

greptile-apps Bot commented Mar 17, 2026

Copy link
Copy Markdown

Confidence Score: 2/5

  • Not safe to merge — adding "type": "module" without a CJS build output is a breaking change for CJS consumers.
  • The status type addition is safe, but the ESM migration is incomplete: no exports field, no CJS output, meaning require() breaks. The stated goal of improving vanilla JS compatibility is undermined.
  • package.json — the "type": "module" change needs a corresponding dual-format build and exports field before this is safe to publish.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: package.json
Line: 7-9

Comment:
**ESM-only package breaks CJS consumers**

Adding `"type": "module"` without a CJS build output or an `exports` field with a `require` condition means any consumer using `require('@bentonow/bento-node-sdk')` will get a runtime error: `ERR_REQUIRE_ESM`. The PR description mentions improving vanilla JS compatibility, but this change has the opposite effect for CJS environments.

To support both, add an `exports` field with dual entry points:
```json
"exports": {
  ".": {
    "import": "./dist/index.js",
    "require": "./dist/index.cjs"
  }
}
```
And update the build script to emit both ESM and CJS outputs.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/sdk/workflows/types.ts
Line: 17

Comment:
**`status` as required field is a breaking change**

Making `status` required means any existing cached/persisted API response that doesn't include this field will fail TypeScript type checks. If there's any chance the API returns workflows without `status` (e.g., older records), this should be optional:

```suggestion
  status?: 'live' | 'draft';
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: d5484a0

Comment thread package.json
Comment on lines +7 to 9
"type": "module",
"main": "dist/index.js",
"module": "dist/index.js",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 ESM-only package breaks CJS consumers

Adding "type": "module" without a CJS build output or an exports field with a require condition means any consumer using require('@bentonow/bento-node-sdk') will get a runtime error: ERR_REQUIRE_ESM. The PR description mentions improving vanilla JS compatibility, but this change has the opposite effect for CJS environments.

To support both, add an exports field with dual entry points:

"exports": {
  ".": {
    "import": "./dist/index.js",
    "require": "./dist/index.cjs"
  }
}

And update the build script to emit both ESM and CJS outputs.

Prompt To Fix With AI
This is a comment left during a code review.
Path: package.json
Line: 7-9

Comment:
**ESM-only package breaks CJS consumers**

Adding `"type": "module"` without a CJS build output or an `exports` field with a `require` condition means any consumer using `require('@bentonow/bento-node-sdk')` will get a runtime error: `ERR_REQUIRE_ESM`. The PR description mentions improving vanilla JS compatibility, but this change has the opposite effect for CJS environments.

To support both, add an `exports` field with dual entry points:
```json
"exports": {
  ".": {
    "import": "./dist/index.js",
    "require": "./dist/index.cjs"
  }
}
```
And update the build script to emit both ESM and CJS outputs.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDK is ESM only anyway, this allows others to use it as a module with proper imports, we don't have any CJS in here so its a non issue.

*/
export type WorkflowAttributes = {
name: string;
status: 'live' | 'draft';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 status as required field is a breaking change

Making status required means any existing cached/persisted API response that doesn't include this field will fail TypeScript type checks. If there's any chance the API returns workflows without status (e.g., older records), this should be optional:

Suggested change
status: 'live' | 'draft';
status?: 'live' | 'draft';
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/sdk/workflows/types.ts
Line: 17

Comment:
**`status` as required field is a breaking change**

Making `status` required means any existing cached/persisted API response that doesn't include this field will fail TypeScript type checks. If there's any chance the API returns workflows without `status` (e.g., older records), this should be optional:

```suggestion
  status?: 'live' | 'draft';
```

How can I resolve this? If you propose a fix, please make it concise.

@jessehanley jessehanley merged commit 02750bf into bentonow:main Apr 2, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants