fix: shell quoting, ACP cwd option, data URL images, and code block tests#27
Merged
fix: shell quoting, ACP cwd option, data URL images, and code block tests#27
Conversation
Add support for rendering markdown images with data URLs (base64-encoded). Changes: - Backend (markdown.go): Add custom bluemonday policy that allows data URLs for PNG, JPEG, GIF, and WebP images via AllowDataURIImages() - Frontend (lib.js): Update DOMPurify config to allow img tags with data: URLs - Tests: Add comprehensive test cases for data URL image handling Security: SVG data URLs are explicitly blocked to prevent XSS attacks, as SVG can contain embedded JavaScript. Only raster image formats are allowed.
Add Playwright test to verify that fenced code blocks with language identifiers render correctly: - Text before code block is in <p> tags (not monospace) - Text after code block is in <p> tags (not monospace) - No empty <pre> or <code> elements appear - Language identifier (e.g., 'python') is not visible as text This is a regression test for GitHub issue #22 which reported garbled output when AI responses contain fenced code blocks with language identifiers like ```python. The test verifies the fix works correctly in the web interface by checking the DOM structure of rendered messages. Refs: #22
Add support for specifying a working directory (cwd) for ACP server
processes, eliminating the need for shell tricks like
'sh -c "cd /path && command"'.
Configuration:
acp:
- my-agent:
command: my-agent --acp
cwd: /home/user/my-project
Changes:
- Add Cwd field to ACPServer, ACPServerSettings, WorkspaceSettings
- Update NewConnection() to accept and use cwd parameter
- Thread acpCwd through BackgroundSession and SessionManager
- Set cmd.Dir for direct execution (non-restricted runners)
- Log warning when cwd is used with restricted runners (not supported)
Documentation:
- Update docs/config/overview.md with cwd examples
- Update docs/config/web/acp.md with detailed cwd documentation
- Add cwd limitation note to docs/config/restricted.md
- Add cwd comments to config/config.default.yaml
Tests:
- Add TestParse_ACPServerCwd for YAML parsing
- Update TestConfigToSettings_RoundTrip to verify cwd preservation
- Update connection tests with new cwd parameter
Replace strings.Fields() with shlex.Split() from google/shlex library to properly handle shell-style quoting in ACP command configuration. This fixes cases where command arguments contain spaces in quoted strings, e.g., claude --profile "my profile". Fixes #16
Run make fmt fmt-docs to ensure consistent formatting: - Go code formatting (gofmt) - Markdown documentation formatting (prettier)
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple issues related to shell quoting, working directory configuration, data URL images, and code block rendering.
Changes:
- Fixed shell quoting in ACP command parsing by replacing
strings.Fields()withshlex.Split()to correctly handle quoted arguments - Added
cwd(working directory) configuration option for ACP servers to eliminate the need for shell command tricks - Enabled data URL images in markdown (PNG, JPEG, GIF, WebP only - SVG blocked for security)
- Added regression test for code block rendering with language identifiers
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/acp/connection.go | Switched to shlex.Split() for shell-aware command parsing and added cwd parameter support |
| internal/auxiliary/manager.go | Switched to shlex.Split() for shell-aware command parsing |
| internal/web/background_session.go | CRITICAL BUG: Still uses strings.Fields() instead of shlex.Split(); added cwd parameter propagation |
| internal/web/session_manager.go | Added acpCwd field propagation through session creation and resumption |
| internal/cmd/cli.go | Added cwd parameter to ACP connection creation |
| internal/config/config.go | Added Cwd field to ACPServer struct with YAML parsing |
| internal/config/settings.go | Added Cwd field to ACPServerSettings struct |
| internal/config/workspaces.go | Added ACPCwd field to WorkspaceSettings |
| internal/conversion/markdown.go | Added custom data URI sanitization policy to allow safe image types |
| web/static/lib.js | Updated DOMPurify configuration to allow data URL images |
| tests/ui/specs/markdown-chunks.spec.ts | Added regression test for code block rendering |
| tests/fixtures/responses/data-url-image.json | Added test fixture for data URL images |
| internal/conversion/markdown_test.go | Added comprehensive tests for code blocks and data URL images |
| internal/acp/connection_test.go | Added tests for shell quoting parsing |
| internal/config/config_test.go | Added test for ACP server cwd configuration |
| internal/config/settings_test.go | Updated test to verify cwd field in round-trip conversion |
| go.mod/go.sum | Added github.com/google/shlex dependency |
| docs/*.md | Updated documentation for cwd option and restricted runner limitations |
| config/config.default.yaml | Added comments explaining cwd option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sing Address review feedback from Copilot: BackgroundSession.doStartACPProcess() was still using strings.Fields() which breaks on shell quoting. This ensures all ACP command parsing uses shlex.Split() consistently: - internal/acp/connection.go - internal/auxiliary/manager.go - internal/web/background_session.go
…rsing Centralize the shell-aware command parsing logic into a single function: - Add acp.ParseCommand() that uses shlex.Split() with proper error handling - Update internal/acp/connection.go to use ParseCommand() - Update internal/auxiliary/manager.go to use ParseCommand() - Update internal/web/background_session.go to use ParseCommand() - Add comprehensive unit tests for ParseCommand() This ensures consistent behavior across all ACP command parsing locations and makes it easier to maintain or extend the parsing logic in the future.
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
This PR includes several fixes and improvements:
Bug Fixes
Shell quoting in ACP commands (fixes Configuration: ACP command parsing breaks on shell quoting #16): Replace
strings.Fields()withshlex.Split()to properly handle shell-style quoting in ACP command configuration. Commands likesh -c 'cd /dir && cmd'now parse correctly.Data URL images in markdown (fixes Frontend: Markdown images with data URLs don't render #20): Allow data URL images to be rendered in markdown output.
New Features
cwdoption (fixes Configuration: Addcwdoption to ACP server config #17): Addcwdconfiguration option for ACP servers to set the working directory for the process, eliminating the need for shell tricks.Tests
Maintenance
Testing
make ci)Pull Request opened by Augment Code with guidance from the PR author