feat(opencode): declare-and-stat external artifact protocol in bash tool#561
Conversation
📝 WalkthroughWalkthroughThe bash tool now supports declaring expected output files and tracking their changes. The ChangesBash Tool Expected Output Tracking
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces an expected_outputs parameter to the bash tool, allowing the runtime to track and register file changes made by external CLI commands. It includes logic for hashing file states, detecting binary content, and recording these changes as artifacts in the session's turn history. A review comment identified significant code duplication between the new readTrackedState function and existing logic in turn-change.ts, recommending consolidation into a shared utility. Additionally, the reviewer noted inconsistent indentation in the newly added block that violates the project's 2-space rule.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/opencode/src/tool/bash.ts`:
- Line 4: Refactor readTrackedState to stop using Effect.promise with direct
nodefs calls and instead use Effect.gen to access the AppFileSystem service;
remove the import of node:fs/promises, call yield* AppFileSystem.Service inside
Effect.gen, replace nodefs.stat() with the service's stat() method and
nodefs.readFile() with the service.readBytes() (which returns a Buffer for
binary detection), and ensure the function yields/handles the Effect results
appropriately using Effect.gen and the AppFileSystem.Service API.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 09d52749-e454-4174-b178-b6baebc5c5ca
📒 Files selected for processing (3)
packages/opencode/src/tool/bash.tspackages/opencode/test/tool/bash.test.tsskills/document-processing/SKILL.md
|
Batching the internal review loop here in English for the public PR record. GLM first-pass reviewVerdict: P3 minor, non-blocking. Finding:
Everything else in the first-pass review matched the brief:
Opus second-pass reviewVerdict before fixup: conditional pass with one P2 + two P3. Main points:
Follow-up from this branchFixup
The smaller coverage additions for a normal text-file case and a relative-path case are tracked separately in follow-up issue #562 to keep this fixup tight. |
Astro-Han
left a comment
There was a problem hiding this comment.
Engineering final review from @GPT-X.
No new implementation blockers found in the current PR head (e467089d4). The fixup addresses the previously identified correctness hole: indeterminate file states are now non-comparable, so before=EACCES / after=ENOENT no longer creates a false-positive recordWrite(). The regression test covers that path. Directory artifact metadata now reports directory: true instead of binary: true, and the SKILL wording matches the implementation's absolute-or-workdir-relative path support.
I re-verified locally:
bun --cwd packages/opencode test test/tool/bash.test.ts --timeout 30000-> 32 passbun --cwd packages/opencode typecheck-> passgit diff --check dev...HEAD-> pass
Remaining merge gates are procedural, not new code findings:
unit-opencodeis still pending on GitHub Actions at the time of this review.- CodeRabbit's latest review is still pending.
- Existing bot review threads should be replied to and resolved before merge. I do not consider the requested AppFileSystem refactor or shared-helper consolidation necessary for this v1; the current implementation follows the already-used
turn-change.tsdirect filesystem pattern, and broader deduplication can stay out of this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/tool/bash.test.ts (1)
417-466: 💤 Low valueSpy mock with type bypassing is fragile but necessary.
Lines 432 and 438 use
as anyto bypass TypeScript's type checking when mockingnodefs.readFile. While this is a common pattern for mocking native modules in tests, it reduces type safety:
- The mock implementation relies on runtime type inspection (
typeof args[0] === "string") instead of compile-time checks- Changes to the real
readFilesignature won't be caught at compile timemockImplementationOnceassumes a specific call order (first read fails, subsequent reads succeed)The test correctly restores the spy in the finally block and validates the expected behavior. Consider documenting why only the first call is mocked (to simulate "before" state read failure while allowing "after" state checks to succeed).
🤖 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 `@packages/opencode/test/tool/bash.test.ts` around lines 417 - 466, The test uses a fragile type-bypassing spy on nodefs.readFile (see readSpy created with spyOn(nodefs as any, "readFile") and mockImplementationOnce) to simulate an EACCES on the initial read only; add an inline comment immediately above that spy explaining: why we cast to any, that mockImplementationOnce is intentionally used to fail only the "before" read while allowing subsequent reads to call the real originalReadFile, and that this preserves the post-action verification flow; mention that the spy is restored in the finally block (readSpy.mockRestore()) to avoid leaking the mock into other tests.
🤖 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/opencode/test/tool/bash.test.ts`:
- Around line 55-73: The object passed into SessionNs.updateMessage in
createTurn currently uses a double-cast "as unknown as MessageV2.Info"; replace
that cast with a proper typed value (e.g., declare const message:
MessageV2.Assistant = { ... } or build the literal and append "satisfies
MessageV2.Assistant") and then pass message to SessionNs.updateMessage; ensure
you remove the "as unknown as MessageV2.Info" and use the MessageV2.Assistant
type/satisfies check so the shape matches compile-time expectations in
createTurn and the updateMessage call.
---
Nitpick comments:
In `@packages/opencode/test/tool/bash.test.ts`:
- Around line 417-466: The test uses a fragile type-bypassing spy on
nodefs.readFile (see readSpy created with spyOn(nodefs as any, "readFile") and
mockImplementationOnce) to simulate an EACCES on the initial read only; add an
inline comment immediately above that spy explaining: why we cast to any, that
mockImplementationOnce is intentionally used to fail only the "before" read
while allowing subsequent reads to call the real originalReadFile, and that this
preserves the post-action verification flow; mention that the spy is restored in
the finally block (readSpy.mockRestore()) to avoid leaking the mock into other
tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0432a37-1836-4240-8da5-c1f7d34fe42a
📒 Files selected for processing (3)
packages/opencode/src/tool/bash.tspackages/opencode/test/tool/bash.test.tsskills/document-processing/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/document-processing/SKILL.md
- packages/opencode/src/tool/bash.ts
Summary
Add a declared external artifact protocol to the bash tool so assistant-declared output files are stat-verified after command execution and registered in turn-change.
Why
PawWork's "本轮生成文件" only showed files written through write/edit/apply_patch. Files created by third-party CLIs through bash, such as officecli-generated
.docxfiles, bypassedturnChange.recordWrite()and never appeared in the UI.Related Issue
No GitHub issue. This PR implements Slock task #33 from
#PawWork:2b44653e.Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
packages/opencode/src/tool/bash.ts: declaredexpected_outputsflow, pre/post state capture, andrecordWrite()only on real file changespackages/opencode/test/tool/bash.test.ts: failed-but-wrote behavior, unchanged-path suppression, and no-declaration compatibilityskills/document-processing/SKILL.md: the one-line guidance for third-party CLI output declarationRisk Notes
Low to medium. This changes bash tool schema and records declared file side effects, including binary files, into turn-change. Main things to verify are path normalization, external-directory permission behavior, and that undeclared bash commands remain unchanged.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI change was introduced directly; this is tool/runtime plumbing verified through turn-change tests.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Tests
Documentation