42 amend calls in api and mcp and cli#77
Merged
Conversation
|
👀 QA.tech will run a exploratory tests to review this PR as soon as a deployment is available for this PR. Alternatively, you can comment @qa.tech to manually trigger a review. Learn more about configuring preview deployments. What happens next
🤖 AI end-to-end testing powered by |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an "amend" capability: new core export amendFromTextSpecAndData, POST /v1/generate/amend endpoint and OpenAPI, CLI ChangesAmend Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cf0fcb10c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mcp/src/index.js (1)
557-565:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the failure message tool-specific.
When
amend_data_from_specfails, clients still get"Failed to generate data from specification.", which is misleading for the new tool and makes troubleshooting harder.Suggested tweak
if (!result.ok) { + const failureMessage = + name === 'amend_data_from_spec' + ? 'Failed to amend data from specification.' + : 'Failed to generate data from specification.'; return writeToolResult( id, - createToolError('generation_failed', 'Failed to generate data from specification.', { + createToolError('generation_failed', failureMessage, { errors: result.errors, diagnostics: result.diagnostics, }), true );🤖 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 `@apps/mcp/src/index.js` around lines 557 - 565, When handling a failed generation result, make the error message tool-specific: inspect the incoming name (used to choose amendFromTextSpecAndData vs generateFromTextSpec) and pass a different human-readable message into createToolError before calling writeToolResult; for example, if name === 'amend_data_from_spec' use a message like "Failed to amend data from specification." otherwise keep "Failed to generate data from specification.", and continue to include result.errors and result.diagnostics when calling writeToolResult with id and normalizedArgs.
🧹 Nitpick comments (1)
apps/cli/src/run-cli.js (1)
61-73: ⚡ Quick winPass
stream: falseexplicitly in amend mode.The CLI warns that streaming is ignored but still forwards
options.shouldStreamtoamendFromTextSpecAndData(). The core function ignores this parameter and emits a diagnostic warning. Hardcodingstream: falsemakes the intent explicit—we don't want streaming in amend mode—and prevents any future confusion about whether the parameter is actually used.Proposed change
const result = amendFromTextSpecAndData({ textSpec, inputData, inputFormat: options.inputFormat, rowCount: options.rowCount, outputFormat: options.format, unsafeFakerExpressions: options.unsafeFakerExpressions, - stream: options.shouldStream, + stream: false, });🤖 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 `@apps/cli/src/run-cli.js` around lines 61 - 73, The code warns streaming is ignored in amend mode but still passes options.shouldStream into amendFromTextSpecAndData; change the call to explicitly set stream: false so amendFromTextSpecAndData receives a clear non-streaming intent. Locate the call to amendFromTextSpecAndData and replace the stream argument (currently stream: options.shouldStream) with stream: false, keeping all other parameters the same and retaining the existing progress warning about ignored flags.
🤖 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 `@apps/api/src/index.js`:
- Around line 553-563: The catch block is leaking internal stack traces to
clients by returning error?.stack in the diagnostics field; update the handler
around the return that calls toErrorResponse so it does not include
diagnostics.stack (remove or replace with a generic diagnostics value), and
instead log the full error/server stack locally using the existing server logger
or console.error (referencing the same error variable), keeping the public
response from toErrorResponse generic (e.g., only include errors:
[error?.message || 'Unhandled amend error'] and no stack) while preserving the
500 status path.
In `@apps/api/src/openapi.js`:
- Around line 428-429: The OpenAPI response schema currently restricts the amend
response "rows" cells to strings (rows: items: items: { type: 'string' }), which
is too narrow; update the schema for the "rows" cell items in the amend response
to allow any JSON value (e.g., replace the inner items type with a free-form
schema such as an empty schema {} or a union/anyOf of string, number, boolean,
object, null) so clients/validators accept non-string cell values while leaving
"rendered" as string.
In `@packages/core/src/index.js`:
- Around line 167-175: normaliseAmendCount currently uses Number.parseInt which
silently accepts "1.5" and "2abc"; change the validation to explicitly reject
non-integer or junk-suffixed strings by validating the raw input string (e.g.,
use a regex like /^[+-]?\d+$/ on String(value).trim() or parse with Number and
verify Number.isInteger and that String(Number(value)) equals the trimmed input)
before converting to an integer; keep the existing behavior of returning
fallbackCount when value is undefined/null/'' and returning undefined for
invalid inputs, and update the implementation in the normaliseAmendCount
function.
---
Outside diff comments:
In `@apps/mcp/src/index.js`:
- Around line 557-565: When handling a failed generation result, make the error
message tool-specific: inspect the incoming name (used to choose
amendFromTextSpecAndData vs generateFromTextSpec) and pass a different
human-readable message into createToolError before calling writeToolResult; for
example, if name === 'amend_data_from_spec' use a message like "Failed to amend
data from specification." otherwise keep "Failed to generate data from
specification.", and continue to include result.errors and result.diagnostics
when calling writeToolResult with id and normalizedArgs.
---
Nitpick comments:
In `@apps/cli/src/run-cli.js`:
- Around line 61-73: The code warns streaming is ignored in amend mode but still
passes options.shouldStream into amendFromTextSpecAndData; change the call to
explicitly set stream: false so amendFromTextSpecAndData receives a clear
non-streaming intent. Locate the call to amendFromTextSpecAndData and replace
the stream argument (currently stream: options.shouldStream) with stream: false,
keeping all other parameters the same and retaining the existing progress
warning about ignored flags.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67c53d6a-0e6e-4ef5-9966-72433d13d093
⛔ Files ignored due to path filters (2)
test-fixtures/amend-cross-format/expected-output.csvis excluded by!**/*.csvtest-fixtures/amend-cross-format/input.csvis excluded by!**/*.csv
📒 Files selected for processing (24)
README.mdapps/api/src/index.jsapps/api/src/openapi.jsapps/api/src/tests/generate/v1-generate-amend.spec.jsapps/cli/README.mdapps/cli/src/cli-options.jsapps/cli/src/run-cli.jsapps/cli/src/tests/cli-options.test.jsapps/cli/src/tests/integration.cli-params.test.jsapps/cli/src/tests/run-cli.test.jsapps/mcp/src/index.jsapps/mcp/src/mcp.test.jsapps/web/src/tests/browser/abstraction-smoke-tests/grid-header-controls.spec.jsapps/web/src/tests/browser/app-coverage/filtering-sorting/filtering-sorting.spec.jsapps/web/src/tests/browser/planned-functional/filtering-sorting/column-sorting.spec.jsapps/web/src/tests/browser/planned-functional/grid-operations/clear-sort-before-add-rows-below.spec.jsdocs-src/docs/070-interfaces-and-deployment/030-rest-api.mddocs-src/docs/070-interfaces-and-deployment/040-mcp.mddocs-src/docs/070-interfaces-and-deployment/050-cli-node-and-bun.mdpackages/core/src/index.jspackages/core/src/tests/core-api/amendFromTextSpecAndData.test.jstest-fixtures/amend-cross-format/expected-output.dsvtest-fixtures/amend-cross-format/input.dsvtest-fixtures/amend-cross-format/schema.txt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/mcp/src/index.js (1)
408-411: 🏗️ Heavy liftDerive amend
inputFormatvalues from core instead of hardcoding them here.This enum is now a second source of truth alongside the importer-backed validation in core. If importer support changes, MCP discovery can advertise the wrong contract even though runtime behavior is correct.
🤖 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 `@apps/mcp/src/index.js` around lines 408 - 411, Replace the hardcoded enum in the inputFormat schema with the canonical list exported by core: import or call the core module that exposes the supported importer formats and assign that array to inputFormat.enum (i.e., remove ['csv','dsv',...] and use core's supported formats). Ensure you reference the core export that represents importer-backed validation (the single source of truth for supported formats) and use it where inputFormat.enum is defined so MCP discovery always reflects core's importer support.
🤖 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 `@packages/core/js/data_formats/delimiter-options.js`:
- Around line 10-12: The comment and behavior for skipEmptyLines in
delimiter-options.js are wrong: using skipEmptyLines: 'greedy' will drop rows
like ",," and ""; change the parser option to skipEmptyLines: true (in the same
config object where skipEmptyLines is currently set) so only trailing blank
lines are skipped and explicit empty records are preserved, and update the
inline comment to reflect the correct behavior; ensure this change occurs in the
configuration that defines skipEmptyLines in
packages/core/js/data_formats/delimiter-options.js.
In `@packages/core/src/index.js`:
- Around line 349-361: The code currently trims inputFormat only when validating
but then lowercases it without trimming before asking the importer, causing
values like " csv " to be rejected; update the normalization step to trim and
lowercase (e.g., set normalisedInputFormat =
String(inputFormat).trim().toLowerCase()) before calling createImporter() and
importer.canImport(normalisedInputFormat) so the importer lookup uses the
trimmed value; ensure references to inputFormat, normalisedInputFormat,
createImporter(), and importer.canImport() are updated accordingly.
---
Nitpick comments:
In `@apps/mcp/src/index.js`:
- Around line 408-411: Replace the hardcoded enum in the inputFormat schema with
the canonical list exported by core: import or call the core module that exposes
the supported importer formats and assign that array to inputFormat.enum (i.e.,
remove ['csv','dsv',...] and use core's supported formats). Ensure you reference
the core export that represents importer-backed validation (the single source of
truth for supported formats) and use it where inputFormat.enum is defined so MCP
discovery always reflects core's importer support.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c12bf2e5-60e7-499c-b92e-83038322e80a
📒 Files selected for processing (8)
apps/api/src/index.jsapps/api/src/openapi.jsapps/api/src/tests/generate/v1-generate-amend.spec.jsapps/mcp/src/index.jsapps/mcp/src/mcp.test.jspackages/core/js/data_formats/delimiter-options.jspackages/core/src/index.jspackages/core/src/tests/core-api/amendFromTextSpecAndData.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/openapi.js
- apps/mcp/src/mcp.test.js
- apps/api/src/index.js
This was referenced May 9, 2026
Merged
This was referenced Jun 10, 2026
Merged
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
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.
closes #42
Summary by CodeRabbit
New Features
Documentation
Tests
Behavior