Conversation
Contributor
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/plans/2026-02-20-operation-type-suffix.md (1)
3-3: Remove the AI-agent implementation instruction from committed documentation.The directive
> **For Claude:** REQUIRED SUB-SKILL: ...is an artifact of the AI-assisted authoring workflow and doesn't belong in version-controlled docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-02-20-operation-type-suffix.md` at line 3, Remove the AI-agent authoring directive line starting with "**For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans..." from the committed document; locate the exact string in docs/plans/2026-02-20-operation-type-suffix.md (or any other files) and delete that line, scan the file for any other agent-specific directives like "For Claude" or "REQUIRED SUB-SKILL" and remove them as well so the repository contains only human-facing documentation.packages/zenko/src/zenko.ts (1)
539-548:throwin library code conflicts with the Result-style guideline.
normalizeTypesConfigthrowsnew Error(...)for an invalidoperationTypeSuffix. The coding guideline forpackages/zenko/src/**/*.{ts,tsx}states "Prefer Result-style returns for recoverable errors in library code / Avoid throwing in library code when possible". Adopting this here would require propagating aResultthroughgenerateWithMetadata/generate, which is a non-trivial API change, but it should be considered as part of a broader error-handling pass on the public surface.As per coding guidelines: "Prefer Result-style returns for recoverable errors in library code. Avoid throwing in library code when possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zenko/src/zenko.ts` around lines 539 - 548, normalizeTypesConfig currently throws an Error when operationTypeSuffix is invalid; per the library guideline we should avoid throwing and instead return a Result-style error and propagate it through the public call chain. Change normalizeTypesConfig to return a Result<T, E>-like object (e.g., { ok: true, value } | { ok: false, error }) instead of throwing for invalid operationTypeSuffix, and update its callers—specifically generateWithMetadata and generate—to accept and propagate that Result (unpack success or return the error Result up the stack) so the public API surfaces errors via Result rather than exceptions; ensure you reference the operationTypeSuffix validation logic and preserve the original error message text in the returned error.packages/zenko/zenko-config.schema.json (1)
60-64: Consider adding apatternconstraint to match runtime validation.Without it, IDEs that use the
$schemareference for validation will accept invalid values like"my-op"or"123Op"at config-authoring time; the error only surfaces at generation time.♻️ Proposed addition
"operationTypeSuffix": { "type": "string", "default": "Operation", + "pattern": "^$|^[a-zA-Z_$][a-zA-Z0-9_$]*$", "description": "Suffix appended to operation type names ..." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zenko/zenko-config.schema.json` around lines 60 - 64, operationTypeSuffix accepts arbitrary strings now, causing IDE/schema validation to allow invalid values like "my-op" or "123Op"; add a JSON Schema "pattern" constraint to the operationTypeSuffix schema entry (operationTypeSuffix) to enforce a valid identifier format used at runtime—for example add "pattern": "^[A-Za-z][A-Za-z0-9_]*$" (start with a letter, then letters/digits/underscores) and update the description if needed to mention the identifier requirement so IDE validation matches runtime checks.packages/zenko/src/__tests__/operation-suffix.test.ts (1)
1-157: Consider adding snapshot assertions for at least the core cases.The tests use focused
toContainassertions, which are valuable for targeted checks but don't guard against broader regressions in generated output format. The coding guideline requires heavy snapshots for output verification. Adding one snapshot per top-level scenario (default, custom suffix, empty suffix) would complement the existing assertions.As per coding guidelines: "Use heavy snapshots for output verification in tests. Name snapshots descriptively with format like
expect(output).toMatchSnapshot('cli-petstore-output')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zenko/src/__tests__/operation-suffix.test.ts` around lines 1 - 157, Add snapshot assertions for the core scenarios to guard generated output format: in the "default behavior" test (uses 'Operation' suffix by default) add expect(result).toMatchSnapshot('operation-suffix-default') after the existing toContain checks; in the "custom suffix" test (uses custom suffix when provided) add expect(result).toMatchSnapshot('operation-suffix-custom') after its assertions; and in the "empty suffix removes suffix entirely" test add expect(result).toMatchSnapshot('operation-suffix-empty'). Keep the current focused toContain assertions and ensure snapshots are named descriptively; locate these tests around the generate(simpleSpec, ...) calls and add the snapshot assertions immediately after obtaining result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-02-20-operation-type-suffix.md`:
- Line 13: The markdown skips from an H1 to H3 causing markdownlint MD001; fix
by either promoting the existing "### Task 1: Add Fireblocks spec export to
`@zenko/specs`" (and any other task headings) to H2 (change "###" to "##") or
insert an intermediate H2 such as "## Tasks" above the current task headings so
the document hierarchy is sequential; update the heading tokens for the "Task 1:
Add Fireblocks spec export to `@zenko/specs`" heading (and sibling task headings,
if any) accordingly.
---
Nitpick comments:
In `@docs/plans/2026-02-20-operation-type-suffix.md`:
- Line 3: Remove the AI-agent authoring directive line starting with "**For
Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans..." from the
committed document; locate the exact string in
docs/plans/2026-02-20-operation-type-suffix.md (or any other files) and delete
that line, scan the file for any other agent-specific directives like "For
Claude" or "REQUIRED SUB-SKILL" and remove them as well so the repository
contains only human-facing documentation.
In `@packages/zenko/src/__tests__/operation-suffix.test.ts`:
- Around line 1-157: Add snapshot assertions for the core scenarios to guard
generated output format: in the "default behavior" test (uses 'Operation' suffix
by default) add expect(result).toMatchSnapshot('operation-suffix-default') after
the existing toContain checks; in the "custom suffix" test (uses custom suffix
when provided) add expect(result).toMatchSnapshot('operation-suffix-custom')
after its assertions; and in the "empty suffix removes suffix entirely" test add
expect(result).toMatchSnapshot('operation-suffix-empty'). Keep the current
focused toContain assertions and ensure snapshots are named descriptively;
locate these tests around the generate(simpleSpec, ...) calls and add the
snapshot assertions immediately after obtaining result.
In `@packages/zenko/src/zenko.ts`:
- Around line 539-548: normalizeTypesConfig currently throws an Error when
operationTypeSuffix is invalid; per the library guideline we should avoid
throwing and instead return a Result-style error and propagate it through the
public call chain. Change normalizeTypesConfig to return a Result<T, E>-like
object (e.g., { ok: true, value } | { ok: false, error }) instead of throwing
for invalid operationTypeSuffix, and update its callers—specifically
generateWithMetadata and generate—to accept and propagate that Result (unpack
success or return the error Result up the stack) so the public API surfaces
errors via Result rather than exceptions; ensure you reference the
operationTypeSuffix validation logic and preserve the original error message
text in the returned error.
In `@packages/zenko/zenko-config.schema.json`:
- Around line 60-64: operationTypeSuffix accepts arbitrary strings now, causing
IDE/schema validation to allow invalid values like "my-op" or "123Op"; add a
JSON Schema "pattern" constraint to the operationTypeSuffix schema entry
(operationTypeSuffix) to enforce a valid identifier format used at runtime—for
example add "pattern": "^[A-Za-z][A-Za-z0-9_]*$" (start with a letter, then
letters/digits/underscores) and update the description if needed to mention the
identifier requirement so IDE validation matches runtime checks.
RawToast
force-pushed
the
rename-operation-suffix
branch
from
75f136e to
22e59f5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 97.45% 97.90% +0.45%
==========================================
Files 16 16
Lines 2277 2295 +18
==========================================
+ Hits 2219 2247 +28
+ Misses 58 48 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
nuke.ts (2)
83-107:findGeneratedFilesbypasses the injectedfs, making it untestable.Unlike
scanDirectoryandfindAllTargets, this function importsreaddirdirectly. For consistency and testability, accept thefsparameter here too.♻️ Proposed fix
-export async function findGeneratedFiles(root: string): Promise<string[]> { +export async function findGeneratedFiles( + root: string, + fs: Pick<FsOperations, "readdir"> = defaultFs +): Promise<string[]> { const generatedFiles: string[] = [] const examplesDir = join(root, "packages", "examples") const walkDir = async (dir: string): Promise<void> => { - const entries = await readdir(dir, { withFileTypes: true }).catch( + const entries = await fs.readdir(dir, { withFileTypes: true }).catch( () => [] as Dirent[] )Also update the call in
main:const [dirTargets, fileTargets] = await Promise.all([ findAllTargets(".", fs), - findGeneratedFiles("."), + findGeneratedFiles(".", fs), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuke.ts` around lines 83 - 107, The function findGeneratedFiles currently imports and calls readdir directly, bypassing the injected fs and making it untestable; change its signature to accept an fs parameter (same shape used by scanDirectory/findAllTargets), replace direct readdir calls with fs.readdir (and handle fs.readdir returning Dirent[]), and propagate errors the same way as the other helpers; then update the caller in main to pass the injected fs instance into findGeneratedFiles. Ensure you reference the function name findGeneratedFiles and the main caller when making these edits.
122-133: Consider logging each target path before deletion for debuggability.When a deletion fails, the error message includes the path, but successful deletions are silent. A brief log per target (or at least the full list up front) would help users verify what was cleaned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuke.ts` around lines 122 - 133, Log each target path before attempting deletion to improve debuggability: when iterating over allTargets in the Promise.all block (the map over allTargets that calls fs.rm for each target), emit a short message (e.g., via console.log or process.stdout.write) showing the target path prior to calling fs.rm(target, { recursive: true, force: true }), so successful deletions are visible as well; you can also optionally print the full allTargets list up front using the existing console.log(`Deleting ${allTargets.length} items...`) for context.packages/zenko/src/__tests__/operation-suffix.test.ts (1)
118-157: Consider adding a test for a suffix containing Unicode identifier characters.The regex
^[a-zA-Z_$][a-zA-Z0-9_$]*$rejects valid TypeScript identifiers with Unicode letters (e.g.,"Opération"). This is likely fine as a pragmatic restriction, but a test documenting this behavior would clarify the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zenko/src/__tests__/operation-suffix.test.ts` around lines 118 - 157, Add a unit test in the existing "validation" describe block that documents the Unicode behavior: call generate(simpleSpec, { types: { operationTypeSuffix: "Opération" } }) and assert it throws "Invalid operationTypeSuffix" (so the behavior of the current regex is explicit); reference the generate function and the operationTypeSuffix option in the new test.packages/zenko/src/zenko.ts (1)
541-548: Throwing in library code for invalid config.Per coding guidelines, library code should prefer Result-style returns. That said,
generate()already returnsstring(not a Result), so thisthrowis consistent with the existing API contract. Worth noting for a future refactor if the API surface evolves toward Result types.Based on learnings: "Applies to packages/zenko/src/**/*.{ts,tsx}: Avoid throwing in library code when possible" and "Prefer Result-style returns for recoverable errors in library code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zenko/src/zenko.ts` around lines 541 - 548, The code currently throws an Error when operationTypeSuffix is invalid inside the generate() flow; replace this throw with a Result-style return: introduce/return a consistent { success: false, error: string } (or a shared Result<T,E> type) from the validation so callers can handle recoverable config errors instead of throwing; specifically change the validation block around operationTypeSuffix (the if (...) { throw new Error(...) } block) to return { success: false, error: `Invalid operationTypeSuffix "${operationTypeSuffix}": must be a valid TypeScript identifier suffix...` } and update generate()'s signature (and all call sites) to use the Result type so valid generation returns { success: true, value: string } while validation failures return the error shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/zenko/src/zenko.ts`:
- Around line 536-548: The JSDoc for normalizeTypesConfig is stale and mentions
HTTP methods; update the comment above the normalizeTypesConfig function to
accurately describe its purpose: normalizing a TypesConfig into a
NormalizedTypesConfig, defaulting operationTypeSuffix to "Operation", validating
operationTypeSuffix as a TypeScript identifier or allowing an empty string, and
throwing an Error when validation fails; reference the function name
normalizeTypesConfig and the types TypesConfig and NormalizedTypesConfig as you
revise the docstring.
---
Nitpick comments:
In `@nuke.ts`:
- Around line 83-107: The function findGeneratedFiles currently imports and
calls readdir directly, bypassing the injected fs and making it untestable;
change its signature to accept an fs parameter (same shape used by
scanDirectory/findAllTargets), replace direct readdir calls with fs.readdir (and
handle fs.readdir returning Dirent[]), and propagate errors the same way as the
other helpers; then update the caller in main to pass the injected fs instance
into findGeneratedFiles. Ensure you reference the function name
findGeneratedFiles and the main caller when making these edits.
- Around line 122-133: Log each target path before attempting deletion to
improve debuggability: when iterating over allTargets in the Promise.all block
(the map over allTargets that calls fs.rm for each target), emit a short message
(e.g., via console.log or process.stdout.write) showing the target path prior to
calling fs.rm(target, { recursive: true, force: true }), so successful deletions
are visible as well; you can also optionally print the full allTargets list up
front using the existing console.log(`Deleting ${allTargets.length} items...`)
for context.
In `@packages/zenko/src/__tests__/operation-suffix.test.ts`:
- Around line 118-157: Add a unit test in the existing "validation" describe
block that documents the Unicode behavior: call generate(simpleSpec, { types: {
operationTypeSuffix: "Opération" } }) and assert it throws "Invalid
operationTypeSuffix" (so the behavior of the current regex is explicit);
reference the generate function and the operationTypeSuffix option in the new
test.
In `@packages/zenko/src/zenko.ts`:
- Around line 541-548: The code currently throws an Error when
operationTypeSuffix is invalid inside the generate() flow; replace this throw
with a Result-style return: introduce/return a consistent { success: false,
error: string } (or a shared Result<T,E> type) from the validation so callers
can handle recoverable config errors instead of throwing; specifically change
the validation block around operationTypeSuffix (the if (...) { throw new
Error(...) } block) to return { success: false, error: `Invalid
operationTypeSuffix "${operationTypeSuffix}": must be a valid TypeScript
identifier suffix...` } and update generate()'s signature (and all call sites)
to use the Result type so valid generation returns { success: true, value:
string } while validation failures return the error shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores