Skip to content

Review follow-ups from the remote-agent stack (#201–#218) #220

Closed

Description

@joelmoss

Follow-ups from the bot reviews (CodeRabbit, Greptile) on the remote-agent stack #201–#218, which merged into master as ce284372…827033d2 before these were addressed. Part of #154.

Each item was triaged against the merged code by an independent reviewer. Resolved threads (fixed, false positive, or already-documented limits) are not listed. Ranked most severe first; one PR should cover each group.

High

  • Submodule diff.external still runs during agent git reads. --no-ext-diff is not passed on to the child git diff inside a submodule. Reproduced by the triage reviewer; fix is --submodule=short in read_diff, plus a tests/reads.rs regression. vcs/crates/wr-vcs-git/src/diff.rs:339 (#218 coderabbitai)
  • Intent-to-add rollback runs after a commit whose outcome is unknown or that was killed during a hook. If the commit landed, the files it added are staged as deletions, and the user's next commit removes them. macapp/WorkroomApp/Core/VCSWriting.swift:1905 (#210 coderabbitai)

Both fixed in #221 (9edf62d3).

Medium

  • A lost connection during git add --intent-to-add shows "this commit may have been written" and disables Commit, although no commit was sent. macapp/WorkroomApp/Core/VCSWriting.swift:1898 (#210 greptile-apps)
  • A full request pool, or agent LockContention before spawn, is reported as launchFailed ("This workroom's folder is no longer there") with no Retry. macapp/WorkroomApp/Core/AgentCommandRunner.swift:97 (#210 coderabbitai)
  • Files-panel git ls-files runs without -c core.fsmonitor=, both through the agent (file.rs) and natively (FileTree.swift:166). vcs/crates/wr-agent/src/file.rs:287 (#212 coderabbitai)
  • Agent LockContention or Busy/Io during a Files-panel reload blanks the tree instead of keeping it. macapp/WorkroomApp/Core/FileTreeModel.swift:279 (#212 greptile-apps, #212 coderabbitai)
  • The wakefulness badge's .task sits on a Group. One failed poll removes its only child and cancels polling until the next reconnect. macapp/WorkroomApp/Views/ChangesPanel.swift:444 (#216 coderabbitai)

Fixed in #222 (bc85d1c3, 09cba81b). The wakefulness badge item was measured not to be a bug: a .task on a childless Group does run. A test now pins that.

Low

Fixed in #223 (e47e4f84, e85c4385, 3240b482). Its review round also fixed a test-host crash: tests that changed the process environment after ghostty_init corrupted libghostty's copy of environ.

  • kill_recorded sends SIGKILL to recorded PIDs about 2s later without a start-time check (process::Descendant::is_running already exists). vcs/crates/wr-agent/src/vcs.rs:435 (#210 greptile-apps)
  • A request on an already-closed connection throws .connectionLost, which reads as an unknown outcome although nothing was sent. macapp/WorkroomApp/Core/AgentVCSConnection.swift:248 (#210 greptile-apps)
  • Request deadlines report .connectionLost, not .requestTimedOut, so a hung agent takes the respawn path. macapp/WorkroomApp/Core/AgentVCSConnection.swift:136 (#210 coderabbitai)
  • The outcome-unknown commit notice is titled "Commit failed". macapp/WorkroomApp/Views/CommitSheet.swift:549 (#210 coderabbitai)
  • TODOS.md #205 status and two vcs.rs comments are stale. TODOS.md:1147 (#210 coderabbitai)
  • The deadline retires the connection even when its own request already completed. Add a pending[stream] != nil check. macapp/WorkroomApp/Core/AgentVCSConnection.swift:540 (#218 coderabbitai)
  • Forward drain deadline, and sendStarted, use the wall clock. Use ContinuousClock/DispatchTime. macapp/WorkroomApp/Core/AgentPortForwarding.swift:623 (#218 greptile-apps)
  • add() reconciles against the cached latest snapshot, so a reconnect during add() can drop a valid forward. Nothing enforces the ordering it relies on. macapp/WorkroomApp/Core/AgentPortForwarding.swift:720 (#218 greptile-apps)
  • The 64-connection cap is counted per listener, but the agent enforces it per multiplex connection. macapp/WorkroomApp/Core/AgentPortForwarding.swift:139 (#218 greptile-apps)
  • attach_survives_detach.rs discards four wait_for results (lines 316/483/547/600). vcs/crates/wr-agent/tests/attach_survives_detach.rs:600 (#218 coderabbitai)
  • std::thread::spawn panics when the OS is out of threads, leaking the connection count and stream. There are 18 sites crate-wide. vcs/crates/wr-agent/src/forward.rs:481 (#217 coderabbitai)
  • The test doesn't check MIN_* versions against the shipped agent binary (wr-agent protocol prints only PROTOCOL_VERSION). macapp/WorkroomApp/Core/Session/AgentControlClient.swift:53 (#216 coderabbitai)
  • Test race in AgentWakefulnessTests: script.reply changes before the watch's follow-up request. macapp/WorkroomAppTests/AgentWakefulnessTests.swift:842 (#216 coderabbitai)
  • A session program named cron/wr-wakeshim is excluded from the wakefulness verdict by name alone. vcs/crates/wr-agent/src/wakefulness.rs:270 (#215 greptile-apps)
  • IPv4 default-route parsers accept unreachable/blackhole defaults (Rust sample.rs and Python procfs.py). vcs/crates/wr-agent/src/wakefulness/sample.rs:176 (#215 coderabbitai)
  • The golden-fixture test asserts >= 8, but the contract is exactly 10. vcs/crates/wr-agent/src/wakefulness/tests.rs:150 (#215 coderabbitai)
  • socket.with_extension("wake") gives agent.wake, while the design doc says <socket>.wake. Settle this before the Phase 3 shim. vcs/crates/wr-agent/src/wakefulness.rs:1010 (#215 coderabbitai)
  • The design doc says an unanswered ceiling prompt hibernates the box. In fact the agent stops asserting BUSY and the provider's idle timer decides. docs/designs/remote-workrooms.md:1845 (#215 coderabbitai)

OQ19 measurement harness (vcs/scripts/oq19/)

Fixed in #225 (6d466d30, 495c397e). The recorded boxd confirmation was re-scored under the stricter gate and is still PASS.
All low. None changes a recorded result: manifests, boxd.json, holdout.json and frozen.json were checked.

  • analyze.py's boxd gate passes on missing scenarios and uses any() for sleep-after-IDLE. vcs/scripts/oq19/analyze.py:996 (#214 greptile-apps, #214 coderabbitai)
  • record.py retries and then discards driver failures as infra_fail. vcs/scripts/oq19/record.py:234 (#214 coderabbitai)
  • wakeshim.sh has no trap to release provider timers on SIGTERM. vcs/scripts/oq19/scenarios/tools/wakeshim.sh:35 (#214 coderabbitai)
  • | in config keys breaks markdown table cells (results/tuning.md). vcs/scripts/oq19/analyze.py:586 (#214 coderabbitai)
  • golden/README.md candidate-set description predates amendment 2. vcs/scripts/oq19/golden/README.md:31 (#214 coderabbitai)
  • golden/README.md and the wakefulness.rs:533 comment overstate that the fixtures are "what the shim asserted". vcs/scripts/oq19/analyze.py:364 (#214 greptile-apps)

Process note

Fix all of these on one branch from master, run the full local suite, and push once. Each restack of the stack re-ran CI and both bots on every PR above the change.

🤖 Filed by Claude Code.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions