Skip to content

Conversation

@KJ7LNW
Copy link
Contributor

@KJ7LNW KJ7LNW commented Apr 15, 2025

Context

There was a race condition in terminal command handling where fast-executing commands would incorrectly signal to the AI that they were running in background mode. This occurred because:

  1. The command would execute and complete before event handlers were registered
  2. Due to the timing, the system would mistakenly interpret this as the user choosing to run the command in background mode
  3. This caused the AI to wait for background output that would never come, since the command had already completed

The issue specifically manifests when the terminal delay is set to zero, typically appearing after 3-5 command iterations.

Implementation

  • Added a CommandCallbacks interface to define terminal event handlers
  • Moved handler registration to happen before process start to ensure accurate command state detection
  • Each callback receives the TerminalProcess instance for direct control
  • Added debug logging to help diagnose command completion vs background mode confusion

Before

image

After

image

How to Test

  1. Set terminal delay to zero and give the AI this exact instruction:

    run ls repeatedly until it says that it is running in the background and then stop and ask for instructions. this is a test so it is very important that you use this exact command and do not deviate from my instructions

  2. Verify that:

    • Command completion is correctly detected (even after 3-5 iterations)
    • No false background mode is reported to the AI
    • All output is captured

Get in Touch

Discord: KJ7LNW


Important

Fixes race condition in terminal command handling by registering event handlers before process start and introducing CommandCallbacks for accurate command state detection.

  • Behavior:
    • Fixes race condition in terminal command handling by registering event handlers before process start in Cline.ts and Terminal.ts.
    • Introduces CommandCallbacks interface in Terminal.ts for handling terminal events like onLine, onCompleted, etc.
    • Updates runCommand method in Terminal.ts to accept CommandCallbacks for accurate command state detection.
  • Logging:
    • Adds debug logging in Cline.ts to diagnose command completion vs background mode confusion.

This description was created by Ellipsis for 688da6d. It will automatically update as commits are pushed.

Previously, handlers for terminal output were registered after starting the process,
which could cause output to be missed for fast-executing commands that complete
before handlers are set up.

- Adds CommandCallbacks interface to register handlers upfront
- Moves handler registration before process start to ensure no output is missed
- Provides process instance in callbacks for direct control
- Adds debug logging to help diagnose timing issues

Signed-off-by: Eric Wheeler <[email protected]>
@changeset-bot
Copy link

changeset-bot bot commented Apr 15, 2025

⚠️ No Changeset found

Latest commit: 688da6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Apr 15, 2025

// keep in case we need it to troubleshoot user issues, but this should be removed in the future
// if everything looks good:
console.debug(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug logging is quite detailed (using JSON.stringify with indentation) and logs several internal state fields. Consider gating this under a debug flag or proper logging level check to avoid performance overhead or inadvertent leakage of sensitive/internal information in production.

@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Apr 15, 2025

@mrubens @hannesrudolph

please test this but I think it is worth expediting.

Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 15, 2025
@mrubens mrubens merged commit 00609aa into RooCodeInc:main Apr 15, 2025
21 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 15, 2025
weiz3630 pushed a commit to weiz3630/zgsm that referenced this pull request May 9, 2025
Previously, handlers for terminal output were registered after starting the process,
which could cause output to be missed for fast-executing commands that complete
before handlers are set up.

- Adds CommandCallbacks interface to register handlers upfront
- Moves handler registration before process start to ensure no output is missed
- Provides process instance in callbacks for direct control
- Adds debug logging to help diagnose timing issues

Signed-off-by: Eric Wheeler <[email protected]>
Co-authored-by: Eric Wheeler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants