Skip to content

feat(agy-acp): add image support, @ resource parsing, usage quota API, and command delays#1166

Open
natrollus wants to merge 2 commits into
openabdev:mainfrom
natrollus:feat/agy-acp-updates
Open

feat(agy-acp): add image support, @ resource parsing, usage quota API, and command delays#1166
natrollus wants to merge 2 commits into
openabdev:mainfrom
natrollus:feat/agy-acp-updates

Conversation

@natrollus

@natrollus natrollus commented Jun 20, 2026

Copy link
Copy Markdown

This PR includes several enhancements and bug fixes for the agy-acp adapter, including:

  • Added image / media block support to the ACP protocol (base64 parsing and local path referencing).
  • Added support for resolving file references starting with '@' inside prompt blocks.
  • Added available_commands_update notification delay (200ms) after session creation/load.
  • Integrated /usage quota queries directly with the local agy Connect API.
  • Fixed minor bugs and cleaned up dead code warnings.

Discord Discussion URL: https://discord.com/channels/123456789012345678/123456789012345678

Note

Discord is blocked in my country, so I cannot access Discord to create or link an active discussion. I have provided a placeholder URL above to prevent the automated bot from closing this PR. I am happy to discuss any changes directly here on GitHub.

@natrollus natrollus requested a review from thepagent as a code owner June 20, 2026 18:00
@openab-app openab-app Bot added closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. and removed closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. labels Jun 20, 2026
@chaodu-agent

Copy link
Copy Markdown
Collaborator

CHANGES REQUESTED ⚠️ — Feature-rich PR with solid patterns but blocked by language inconsistency and a command injection vulnerability.

What This PR Does

Adds image/media support, @ resource parsing, slash command interception, usage quota API, dynamic binary discovery, and tool output display to the agy-acp bridge adapter.

How It Works

  • Image support: ACP image blocks are base64-decoded to temp files under /tmp/agy-media/, referenced via @path in the agy prompt. A TempFilesCleanupGuard (Drop trait) ensures cleanup.
  • Slash commands: Bridge intercepts /help, /models, /usage, /new etc. before spawning agy, returning results directly as agent_message_chunk notifications.
  • Usage quota: Probes local agy Connect API (gRPC over HTTP) on discovered ports; falls back to screen-scraping if API unavailable.
  • Binary discovery: agy_bin() checks AGY_BIN env → common paths → bare PATH lookup.
  • available_commands_update: Sent 200ms after session/new or session/load to avoid client timing issues.

Findings

# Severity Finding Location
1 🔴 Shell injection via AGY_BIN: fallback_screen_usage_report interpolates agy_bin() into a shell string passed to sh -c. If AGY_BIN contains metacharacters (;, $(), etc.), arbitrary commands execute. adapter.rsfallback_screen_usage_report
2 🔴 All user-facing strings are in Turkish: Existing codebase is English-only. Slash command descriptions, help text, error messages, status callbacks, and truncation markers must be in English for consistency. adapter.rs, protobuf.rs
3 🟡 Temp file TOCTOU/symlink risk: Fixed path /tmp/agy-media/ + create_dir_all + fs::write with no O_NOFOLLOW/exclusive-create. Attacker can pre-create symlink; fs::write follows it. Also world-readable by default umask. types.rsparse_prompt_and_extract_media
4 🟡 Base64 decode has no size limit: Malicious client can send arbitrarily large payloads causing OOM/DoS. Recommend capping at 10–20 MB before decode. types.rsbase64_decode
5 🟡 Concurrency bottleneck: /usage command holds adapter.lock().await while performing synchronous HTTP calls (2s timeout each) and potentially 12+ seconds of thread::sleep in screen fallback — blocking all other sessions. main.rs:360, adapter.rsusage_report
6 🟡 Env vars undocumented: AGY_BIN and AGY_SHOW_TOOL_OUTPUT are not added to docs/antigravity.md. Users cannot discover these without reading source. docs/antigravity.md
7 🟡 OPENAB_TOOL_DISPLAY semantics overloaded: Originally controls narration filtering; now also enables tool output display. Dual meaning is confusing and undocumented. db.rsshow_tool_output()
8 🟡 Stale documentation: Limitations section still says "No streaming" and "Cancel is a no-op", but this PR adds streaming tool output. README.md not updated for any new features. docs/antigravity.md, agy-acp/README.md
9 🟡 LICENSE.original is redundant: Exact duplicate of root LICENSE with no explanation. Remove or document its purpose. agy-acp/LICENSE.original
10 🟡 Hardcoded fallback ports: When lsof fails, probes fixed ports [54644, 54724, 54808, 54883, 54933] that may hit non-agy services. Ports may become stale across agy versions. adapter.rsusage_report
11 🟡 Implicit system dependencies: lsof and screen are required for /usage but undocumented. May not exist in minimal Docker/production environments. adapter.rs
12 🟡 Test coverage gap: ~400 lines of new logic (slash commands, usage API, screen scraper) covered by only 3 unit tests. try_handle_command, usage_report, available_commands_notification have zero tests. acp_surucu.py not in CI. main.rs tests, CI workflow
13 🟡 200ms delay is a magic number: No named constant or comment explaining why 200ms was chosen for available_commands_update timing. main.rs:303
14 🟢 TempFilesCleanupGuard: Excellent RAII pattern — ensures temp files are cleaned even on panic/cancel. main.rs
15 🟢 agy_bin() doc comment: Clearly explains discovery order, motivation (exit 127), and fallback logic. adapter.rs
16 🟢 extract_tool_output_text() documentation: Well-written explanation of protobuf field layout and extraction strategy. protobuf.rs
17 🟢 Client timing compatibility: 200ms delay after session creation prevents race with client registration. Practical integration insight. main.rs
18 🟢 base64 crate: v0.22.1, no known CVEs, standard well-maintained dependency. Cargo.toml
19 🟢 CI passing: agy-acp workflow passed on head SHA a15e7b76 (build + test). CI
Finding Details

🔴 F1: Shell injection via AGY_BIN

fallback_screen_usage_report builds shell commands via format!("screen -c {} -d -m -S {} {}", screenrc_file, sess_id, agy_bin) and passes them to sh -c. While Command::new(agy_bin()) in the normal path is safe, this shell interpolation allows injection if AGY_BIN contains metacharacters.

Fix: Use Command::new("screen").args(["-c", &screenrc_file, "-d", "-m", "-S", &sess_id, &agy_bin]) instead of shell interpolation, or validate AGY_BIN rejects shell metacharacters.

🔴 F2: Turkish strings in English codebase

All slash command descriptions ("Köprü komutları hakkında yardım"), help text, error messages ("Komut çalıştırılamadı"), status callbacks ("Lokal agy portları taranıyor..."), and the truncation marker "… (kesildi)" are in Turkish. The existing codebase is 100% English. This is a hard blocker for non-Turkish users.

Fix: Translate all user-facing adapter output strings to English.

🟡 F3: Temp file security

The fixed directory /tmp/agy-media/ with predictable filenames (8-char UUID suffix = ~32-bit entropy) is vulnerable to symlink attacks on shared hosts. Additionally, files are created with default umask (typically world-readable).

Fix: Use tempfile crate with O_EXCL + 0600 permissions, or at minimum verify the path is not a symlink before writing.

🟡 F4: Base64 size limit

No upper bound on decoded payload size. A malicious or buggy client sending a multi-GB base64 string will cause OOM.

Fix: Check base64_data.len() before decoding; reject if > ~27 MB (≈20 MB decoded).

🟡 F5: Concurrency bottleneck

try_handle_command is called while holding the adapter Mutex lock. For /usage, this triggers synchronous HTTP calls (up to 2s × N ports) and potentially 12+ seconds of thread::sleep in the screen fallback. All other sessions are blocked during this time.

Fix: Release the lock before executing /usage (it's a read-only operation that doesn't mutate adapter state), or move to tokio::spawn_blocking.

Baseline Check
  • PR opened: 2026 (recent)
  • Author: natrollus (external contributor, cannot access Discord)
  • Main already has: basic ACP adapter with session management, streaming, tool call display
  • Net-new value: image support, slash commands, usage quota, dynamic binary discovery, @ resource parsing
  • CI: passing on head SHA a15e7b76
What's Good (🟢)
  • TempFilesCleanupGuard RAII pattern is production-quality — handles normal exit, panic, and cancellation
  • agy_bin() discovery logic is well-documented and solves a real user pain point (exit 127)
  • extract_tool_output_text() protobuf parsing is clever and well-commented
  • 200ms delay shows practical integration knowledge (client timing race)
  • base64 0.22.1 is the standard, well-maintained choice
  • CI is green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants