fix(cascade-tools): harden multiline text and large diff I/O for shell agents#1381
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for two agent-facing contract issues: the new diff flag is documented with an invalid spelling in one prompt-visible description, and the CLI-only outputFile example still leaks into the SDK gadget examples.
Code Issues
Should Fix
- src/gadgets/github/definitions.ts:307 — The GetPRDiff description tells agents to use
--output-file, but the generated oclif flag is--outputFile. I verified the parser accepts--outputFileand rejects--output-fileas a nonexistent flag, so this prompt/help text can send shell agents down the failure path for the exact escape hatch this PR adds. - src/gadgets/github/definitions.ts:364 — The new
outputFileexample is attached to the shared ToolDefinition, socreateGadgetClass()copies it into the in-processGetPRDiffgadget examples even thoughbuildZodSchema()stripscliOnlyparams. That means SDK gadget users can be shown an example withoutputFile, but the schema removes it and the gadget returns the raw diff string instead of writing a file summary.
🕵️ codex · gpt-5.5 · run details
| name: 'GetPRDiff', | ||
| description: | ||
| 'Get the unified diff of all file changes in a GitHub pull request. Shows each file with additions, deletions, and the patch content.', | ||
| 'Get the unified diff of all file changes in a GitHub pull request. Shows each file with additions, deletions, and the patch content. For large diffs or one-line JSON patches that would truncate stdout, use --output-file <path> to write the full diff to disk and return a compact summary.', |
There was a problem hiding this comment.
This description is prompt/help-visible, but --output-file is not the flag this definition actually generates. The parameter key is outputFile, and the current parser accepts --outputFile while rejecting --output-file as a nonexistent flag. Either document --outputFile here or add output-file as an alias; otherwise agents following this guidance will hit the error path for the new large-diff escape hatch.
There was a problem hiding this comment.
Fixed in ddbf4d0. The prompt/help description now points at the canonical --outputFile spelling, and --output-file is accepted as an alias so either spelling works. I also added CLI coverage for the kebab-case alias.
| repo: 'myapp', | ||
| prNumber: 42, | ||
| path: 'src/big-generated.json', | ||
| outputFile: '/tmp/pr-42-diff.md', |
There was a problem hiding this comment.
Because this example lives on the shared ToolDefinition, it is also copied into the SDK Gadget examples by createGadgetClass(), but outputFile is marked cliOnly and is stripped from the gadget schema. A non-native gadget agent can therefore see and follow an outputFile example even though that parameter is removed before GetPRDiff.execute() runs, so it will get the raw diff string instead of the advertised file-output summary. The CLI-only example needs to be filtered out of gadget examples or represented in CLI-only metadata.
There was a problem hiding this comment.
Fixed in ddbf4d0. createGadgetClass() now filters examples that reference cliOnly params out of SDK gadget examples, so the outputFile example stays available to CLI/native-tool surfaces without leaking into the in-process gadget surface.
|
@aaight address code review concerns |
|
Addressed the code review concerns in ddbf4d0:
Verification: focused regression tests, 🕵️ codex · gpt-5.5 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for one prompt-visible recovery path: the new skipped-file command suggests an output file path that fails for typical changed files under directories.
Code Issues
Should Fix
- src/agents/shared/prFormatting.ts:262 — The
/tmp/diff-<path>.mdtemplate turns a skipped path likesrc/big.jsoninto/tmp/diff-src/big.json.md.getPRDiff()writes withwriteFileSync(outputFile, ...)and does not create parent directories, so an agent following this guidance gets anENOENTruntime envelope instead of the full diff. Use a slash-safe filename template everywhere this guidance appears, or create parent directories before writing.
Focused tests passed locally for the new stdin guard, prompt rendering, manifest threading, and get-pr-diff output mode.
🕵️ codex · gpt-5.5 · run details
| // the agent about the file-output escape hatch so it does not lose | ||
| // content to terminal truncation. | ||
| prNumber !== undefined | ||
| ? ` • \`cascade-tools scm get-pr-diff --prNumber ${prNumber} --path <path> --outputFile /tmp/diff-<path>.md\` for large or one-line JSON diffs, then \`Read /tmp/diff-<path>.md\`` |
There was a problem hiding this comment.
This output path template breaks for the common case where the skipped file path contains a slash. Following it for src/big.json produces /tmp/diff-src/big.json.md; getPRDiff writes with writeFileSync and does not create /tmp/diff-src, so the recovery command fails with ENOENT instead of producing the diff. Please use a slash-safe template such as /tmp/pr-diff.md, or create parent directories before writing.
There was a problem hiding this comment.
Fixed in 185ecb0. I replaced the slash-derived /tmp/diff-<path>.md guidance with the slash-safe /tmp/pr-diff.md path in the skipped-files injection and review prompt/docs, and added unit coverage to keep that template from regressing.
|
@aaight address code review concerns |
|
Addressed the latest code review concern in 185ecb0:
Verification: focused unit tests, 🕵️ codex · gpt-5.5 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - I reviewed the updated MNG-1059 changes and the follow-up fixes. The stdin guard runs before file reads, the diff output escape hatch is kept to the CLI surface with SDK examples filtered, the skipped-file guidance now uses a stable slash-safe temp path, and live CI is green.
🕵️ codex · gpt-5.5 · run details
Summary
Closes MNG-1059.
Hardens
cascade-toolsagainst three friction clusters that wasted agent budget:--body '...markdown...'and lost backticks /$(...)to shell expansion.--body-file - --comments-file -silently truncated one payload becausereadFileSync(0, ...)can only drain stdin once.Changes
1. Shared multiple-stdin guard (
src/gadgets/shared/cli/params.ts)rejectMultipleStdinConsumers()helper scansfileInputAlternativesfor-values and emits a structuredflag-parseerror envelope (error.flag: "body-file,comments-file", hint pointing at temp files) before anyreadFileSync(0, ...)call.cliCommandFactory.tsafter oclif parsing but beforeresolveDirectParams().--body-file - --comments-file /tmp/c.jsonand--body-file /tmp/b.md --comments-file -both work.2. Shell-safe multiline guidance in native tool prompts (
src/backends/shared/nativeToolPrompts.ts)buildSystemPrompt()— documents the one-stdin-consumer invariant and shows safe heredoc / temp-file patterns for one and two payloads.fileInputAlternative(on direct text params) andfileInputFor(on--*-filesynth flags) so the prompt renderer can semantically link the pair.isShellSensitiveExample()detects examples with backticks, code fences,$(...), or newlines. When a file-input companion exists, the renderer suppresses the inline--body '...'example and emits--body-file <path> # write the markdown/multiline content to a temp file (shell-sensitive: ...)instead.3. Refreshed file-input descriptions (
src/gadgets/{github,pm}/definitions.ts)--body-file,--text-file,--description-file,--details-file,--comments-filedescriptions now explicitly call out "markdown / multiline content with backticks, code fences, $(...) or newlines."--comments-filedescription also documents the single-stdin rule.4.
get-pr-diff --outputFileescape hatch (src/gadgets/github/core/getPRDiff.ts)cliOnly: trueflag support onParameterDefinition— symmetric withgadgetOnly. Included in CLI + manifest (so prompts show it) but EXCLUDED from the SDK Gadget Zod schema. Gadget factory skipscliOnlyparams inbuildZodSchema.outputFileparameter ongetPRDiffDef(cliOnly). When set,getPRDiff()writes the full multiline Markdown payload to disk and returns a compact{outputFile, fileCount, bytes, pathFilter}summary instead of the raw text. Default behavior preserved.Promise<string>withoutoutputFile,Promise<PRDiffFileOutputSummary>with it.filterByPath,formatPRDiffFile,formatPRDiffPayload— testable in isolation.5. Review-agent context hints + docs
src/agents/shared/prFormatting.ts:formatSkippedFilesInjection— skipped-files guidance now suggests--outputFile /tmp/diff-<path>.mdfor large or one-line JSON diffs.src/agents/prompts/templates/review.eta— review prompt mentions--outputFile, the one-stdin-consumer rule, and the safe--body-file <path>pattern for markdown bodies with backticks / code fences.src/gadgets/README.md— documents the one-stdin-consumer rule,cliOnlyparameter flag, and theget-pr-diff --outputFilereference.docs/architecture/07-gadgets.md— new "Shell-safety contract (MNG-1059)" subsection covers all three changes.CLAUDE.md/AGENTS.md— references the new contract from the review-context section.CHANGELOG.md— Unreleased > Fixed entries for both clusters.6. Regression coverage
tests/unit/gadgets/shared/cli/params.test.tsandtests/unit/cli/file-input-flags.test.ts(realCreatePRReview) assert--body-file - --comments-file -emits the envelope and does NOT call the core function.--body-file - --comments-file /tmp/c.jsonand--body-file /tmp/b.md --comments-file -round-trip through the factory.$(...)/ multi-line markdown reaches the core function byte-for-byte (CreatePR --body-file).tests/unit/backends/shared-nativeToolPrompts.test.tscovers the new shell-safety rules section and the suppression of unsafe inline examples when a file companion exists.tests/unit/gadgets/github/core/getPRDiff.test.tsexercises a synthesized ~100KB one-line JSON diff and asserts stdout summary stays <500 bytes while the file holds the full payload.tests/unit/gadgets/shared/factories.test.tspinsfileInputFor/fileInputAlternativeflow through the manifest generator.tests/unit/cli/scm/scm-commands.test.tscovers--outputFilevia realGetPRDiffcommand.All 256 focused tests pass. Full
npm testpasses (one unrelated flaky test inasync-resolver.test.ts— passes on retry).npm run typecheckandnpm run lintare clean.Test plan
npm run typechecknpm run lintnpm test(full unit suite)🕵️ claude-code · claude-opus-4-7 · run details