Skip to content

Improve workspace experience in OpenClaw#180

Open
andyrewlee wants to merge 34 commits intomainfrom
projects-nested
Open

Improve workspace experience in OpenClaw#180
andyrewlee wants to merge 34 commits intomainfrom
projects-nested

Conversation

@andyrewlee
Copy link
Owner

@andyrewlee andyrewlee commented Feb 22, 2026

Summary

Describe the change and intended behavior.

Quality Checklist

  • Ran make devcheck locally.
  • Ran make lint-strict-new locally for changed code.
  • If UI/rendering changed, ran make harness-presets.
  • If tmux/e2e changed, ran go test ./internal/tmux ./internal/e2e.

Open with Devin

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@andyrewlee andyrewlee force-pushed the projects-nested branch 3 times, most recently from 2159d87 to 95e3452 Compare February 24, 2026 20:43
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 19 additional findings in Devin Review.

Open in Devin Review

local lower collapsed
lower="$(printf '%s' "$line" | tr '[:upper:]' '[:lower:]')"
collapsed="$(printf '%s' "$lower" | tr -s '[:space:]' ' ')"
case "$line" in

Choose a reason for hiding this comment

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

🟡 Bash is_chrome_line overly aggressive "> "* pattern strips all quoted lines from output

The is_chrome_line function in assistant-step.sh was changed to include a "> "* pattern in its case statement, which drops all lines starting with "> " as TUI chrome. This is inconsistent with the Go counterpart shouldDropAgentChromeLine at internal/cli/cmd_agent_output_signals.go:126-183, which uses the more nuanced isInlinePromptChromeLine function that only drops "> " lines matching a specific "reply exactly ... in one line." pattern.

Root Cause and Impact

The old bash is_chrome_line had "› "* and "❯ "* (fancy Unicode prompt markers) but did NOT have "> "*. The new code adds "> "* which matches any line starting with the Markdown blockquote / shell echo prefix "> ".

The Go code correctly distinguishes these via isInlinePromptChromeLine (cmd_agent_output_signals.go:189-199) which only drops lines matching the specific droid prompt pattern (reply exactly READY in one line.). The Go test at cmd_agent_output_signals_test.go explicitly verifies that quoted lines like "> The user asked to fix the login bug" are preserved (TestCompactAgentOutput_PreservesQuotedLines).

Because is_chrome_line is used by compact_agent_text and other summary-building functions in the step and turn scripts, any agent output containing Markdown blockquotes, git diff context lines, shell echo output, or quoted questions will have those lines silently stripped from the compacted summary shown to users.

Impact: Degraded summary quality in chat notifications when agent output contains "> " prefixed content (common in code review, diff output, and quoted conversation context).

Prompt for agents
In skills/amux/scripts/assistant-step.sh, the is_chrome_line() function at approximately line 244 has a case pattern "> "* that matches ALL lines starting with "> ". This should be changed to only match the specific droid inline prompt chrome pattern (lines matching "> Reply exactly ... in one line."), consistent with the Go implementation in internal/cli/cmd_agent_output_signals.go isInlinePromptChromeLine() (lines 189-199). Remove "> "* from the main case statement and add a separate check after the esac block that tests whether the line starts with "> " AND matches the specific "reply exactly" pattern. Generic "> " lines (Markdown blockquotes, shell output, diff context) must be preserved.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +408 to +413
if info, err := os.Stat(path); err == nil {
if ttl <= 0 || time.Since(info.ModTime()) < ttl {
return nil, false, nil
}
_ = os.Remove(path)
}

Choose a reason for hiding this comment

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

🟡 TOCTOU race in acquireTaskStartLock allows two concurrent task starts to both acquire the lock

The acquireTaskStartLock function has a time-of-check-time-of-use race between checking/removing an expired lock and creating a new one with O_EXCL. Two concurrent processes can both bypass the lock.

Root Cause: Stat-Remove-Create sequence is not atomic across processes

The lock acquisition does os.Statos.Removeos.OpenFile(O_EXCL). When two processes both see an expired lock simultaneously, the following interleaving can occur:

  1. Process A: Stat(path) → sees expired lock (modtime > TTL)
  2. Process B: Stat(path) → sees same expired lock
  3. Process A: Remove(path) → removes expired lock
  4. Process A: OpenFile(O_EXCL) → creates fresh lock → acquired
  5. Process B: Remove(path)deletes A's freshly created lock! (B still holds the stale Stat result from step 2)
  6. Process B: OpenFile(O_EXCL) → creates new lock → acquired

Both processes now believe they hold the lock. Both proceed to call taskRunAgent, resulting in duplicate agent tabs for the same workspace/assistant — which is the exact scenario the lock is designed to prevent.

Impact: Under concurrent task start commands with an expired lock file, two agent tabs can be created simultaneously for the same workspace, confusing orchestrators and wasting resources.

Prompt for agents
In internal/cli/cmd_task_helpers.go, the acquireTaskStartLock function at lines 403-424 has a TOCTOU race between os.Stat (checking TTL), os.Remove (cleaning expired lock), and os.OpenFile with O_EXCL (creating new lock). Two concurrent processes that both see the expired lock can interleave such that process B deletes process A's freshly created lock and creates its own, resulting in both holding the lock.

To fix this, restructure the function to attempt the O_EXCL create first (optimistic path), and only check TTL when creation fails with EEXIST. If the existing lock is expired, use os.Rename to atomically replace it rather than Remove+Create. Alternatively, use flock/fcntl advisory locking if available on the target platforms (darwin/linux).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant