fix: heartbeat completion shows LLM result instead of garbled streaming fragment#970
fix: heartbeat completion shows LLM result instead of garbled streaming fragment#970dj-oyu wants to merge 307 commits intosipeed:mainfrom
Conversation
… calls The previous tail-only trimming didn't catch mid-history corruption from session collisions (e.g. a user message interleaved between an assistant tool call and its result). APIs like MiniMax reject this with "tool call result does not follow tool call". Rewrote SanitizeHistory to walk the full history, keeping only well-formed groups where tool results immediately follow their assistant message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Past entries: always exactly 1 line, truncated at 36 runes with result marker (✓/✗) always visible at the end regardless of truncation - Latest entry: reserved space below (blank line or code-fenced error detail), so the bubble height doesn't jump when errors appear - Error detail on last entry uses ``` code fence for proper decoration - formatCompactEntry() ensures no line exceeds maxEntryLineWidth (36) to prevent wrapping on Telegram mobile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Past entries stay compact (36 chars). The latest/active entry shows up to 70 chars (~2 Telegram lines) of the command with result on a separate indented line, giving enough context without excessive height. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iority Layout is now constant height regardless of entry count or error state: - 3 compact past entries (padded if fewer) - Latest entry: up to ~70 chars command + result on next line + 2 reserved - Error section: always present as code block (sticky — persists until a newer error replaces it) - File tool paths now prioritize filename: "dir…/backend.py" instead of "projects/terra-py-fo..." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
File tool entries (edit_file, read_file, etc.) in compact mode now: - Omit duration (always near-instant, wastes space) - Truncate path from the start, keeping filename visible e.g. "…/backend.py ✓" instead of "projects/te… ✓ 0.0s" Exec entries keep full duration display and truncate from the end. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Heartbeat/cron running with NoHistory loses context between invocations. Add two mechanisms to ensure plan progress is recorded in MEMORY.md: 1. System prompt preamble: appends "Background Execution" section to system prompt (high attention position) reminding LLM to mark [x] 2. Tool→text transition nudge: at the boundary where LLM switches from tool calls to text response, check if unchecked step count decreased. If not, inject one continuation message that serves as both a marking reminder and a work continuation trigger. Also fix string concatenation inefficiencies: - buildRichStatus: use strings.Builder for prefix assembly - error header: sequential WriteString instead of + concat - displayAvailableSkills: fmt.Fprintf instead of WriteString(Sprintf) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… placeholders The Target Format in GetInterviewContext() had Go-specific examples (go build, go test, golangci-lint) which biased the AI to use them even for non-Go projects (e.g. Python with uv run). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ions - Add statusToHTML() to convert backtick fences to <pre> tags with HTML escaping - Apply ParseMode=HTML to EditStatus/EditTaskStatus so code blocks render properly - Replace escapeHTML's chained ReplaceAll with single-pass strings.Replacer - Extract statusSeparator const to deduplicate separator literals - Replace fmt.Fprintf with strconv.Itoa+WriteString to avoid reflection - Remove intermediate strings.Builder for latest entry prefix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…xec flags - Increase displayPastEntries 3→4, maxEntryLineWidth 36→42 - Latest entry: command on own line, result below (no inline result) - Remove 2nd separator before error section, increase error lines 3→5 - Show full workspace path (📁) instead of just project name - Strip exec option flags (--opt, -O) in buildArgsSnippet via regex - Add compressRepeats: runs of 3+ identical symbols → 2 (e.g. ====→==) - Truncate error detail lines to maxEntryLineWidth - Reduce reserved lines from 2 to 1 - Increase ErrDetail truncation limit 120/200→300 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Full path wastes horizontal space on Telegram mobile bubbles. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sanitizeHistoryForProvider only allowed tool messages when the preceding message was an assistant with tool_calls. For assistant turns with 2+ tool calls, the 2nd tool result's predecessor was the 1st tool result (role "tool"), causing it to be dropped. This made the API see an assistant with N tool_calls but <N results, triggering "tool call result does not follow tool call". Fix: also allow tool messages after sibling tool messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Paths like `/home/user/project/` caused empty project name because LastIndex found the trailing slash. TrimRight ensures correct extraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Single-file HTML Mini App with 4 tabs: Plan, Skills, Session, Config - Plan tab shows phase/step list parsed from MEMORY.md with tap-to-complete - Skills tab lists available skills with send bar for command construction - Tailscale auto-detection for HTTPS hosting with TLS cert provisioning - HMAC-SHA256 validation of Telegram initData for API authentication - WebAppData handler in Telegram channel for command relay - Menu Button registration for "Dashboard" in Telegram - Health server extended with Mux() accessor and StartTLS support - GetPlanPhases() as single source of truth for plan parsing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Parse --stats flag in gatewayCmd and forward to NewAgentLoop - Add input field + Start button when plan is empty in Mini App Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sendData() only works for Mini Apps launched via Keyboard buttons, not Menu Buttons. Replace with POST /miniapp/api/command endpoint that injects commands into the message bus via CommandSender interface. - Add CommandSender interface and POST /miniapp/api/command endpoint - Extract user ID from initData for sender identification - Replace all tg.sendData() calls with fetch POST in JS - Remove deleted /todo from Quick Commands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Document the Dashboard Mini App with setup instructions for both Tailscale auto-detection and custom URL configurations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Chip buttons flash theme color briefly on success - Send buttons flash green on success - Remove showAlert for all commands except errors and plan start - Add scale-down on press for tactile feel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
/plan status falls through to the default case in handlePlanCommand, which treats "status" as a task description and starts a new plan. The correct command to show plan progress is just /plan (no args). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Quick Commands: small chips → large 2-column tile grid - Skills: add right arrow indicator and tinted background on selection - Plan steps: undone steps get card background, done steps are flat - Refresh button: full-width block button - All tappable elements: scale-down on press for tactile feedback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tiles and skill items had secondary-bg background on secondary-bg parent, making them invisible as buttons. Switch to bg background with hint-color borders so they stand out as distinct tappable areas. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The workspace path (e.g. ~/.picoclaw/workspace) showed "workspace" as the directory name. Now extracts the actual project directory from the exec cd prefix (e.g. cd workspace/projects/my-app && ...) and displays "my-app" instead. Falls back to workspace basename if no cd prefix seen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Refactored extractProjectDir to accept any tool call. For read_file, edit_file etc., the file path is used to detect the project directory. Extracted shared logic into projectDirFromPath. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add @media (prefers-color-scheme: dark) with dark fallback values for when Telegram theme variables are not available (testing/preview) - Remove color-mix() for skill selection highlight (not supported in all WebView versions) - All runtime colors use Telegram theme variables which automatically adapt to light/dark mode Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaced assumption-based logic (skipping "projects", "repos", "src") with dynamic extraction: exec uses basename of cd target, file tools use the first path component after workspace. No conventions assumed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For file tools (read_file, edit_file, etc.), track the longest common directory prefix across all accessed file paths. As more files are touched, the LCP converges to the actual project root. Exec cd target remains authoritative; file LCP is used as fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Gradient background (bg → secondary-bg) gives depth - box-shadow lifts elements off the surface - Press: shadow disappears + translateY(1px) for physical push feel - Applied to: command tiles, skill cards, refresh button Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The plan nudge was background-only and only fired when no progress was made. Now it fires for all plan executions (foreground included) and whenever unchecked steps remain, with a context-aware message depending on whether progress was recorded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Track tool call count, per-tool breakdown, and duration in ToolLoopResult, propagate via bus Metadata, and format as "📋 scout-1 completed (3.2s, 5 tool calls)." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix 9 lint issues (predeclared min shadow, golines, gofmt, gci) and 2 test failures (spawn error message assertions). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- spawn.go:97: break long fmt.Sprintf into multi-line
- loop.go:1179: break long if-condition into multi-line
- loop.go:2742: map[string]interface{} -> map[string]any (gofmt rewrite)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(orch): orchestration spawn adoption and sandbox improvements
The --stats flag was not carried over when the CLI was migrated from manual os.Args parsing to Cobra commands. Add it back to the gateway command and forward enableStats to NewAgentLoop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(cli): restore --stats flag lost in Cobra migration
channelManager and healthServer both created independent http.Server instances on the same address, causing "address already in use" on startup. Consolidate to a single listener owned by healthServer — channelManager now registers webhook/health handlers directly onto the healthServer mux instead of creating its own server. Also raise healthServer timeouts from 5s to 30s to accommodate webhook request handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix: eliminate dual HTTP server bind on gateway port
…ider - Strip <think> blocks from force response path (max iterations) in runLLMIteration, which was missing StripThinkBlocks - Add StripThinkBlocks safety net in heartbeat sendResponse to prevent LLM reasoning artifacts from leaking to users - Add ProviderName() to WebSearchTool and log selected search provider (brave/perplexity/tavily/duckduckgo) at startup for diagnostics - Show web search provider in gateway startup console output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix: strip think tags from heartbeat and log web search provider
Two issues addressed: 1. Per-provider rate limiting via RPM config field. When multiple subagents share the same provider instance, concurrent API calls can trigger rate limit errors. WithMinInterval() enforces a minimum wait between requests, derived from the existing model_list rpm setting. 2. Spawned subagents now outlive their parent session. Previously, heartbeat runAgentLoop would cancel its child context on exit, killing any spawned subagents mid-flight and cleaning up worktrees they were still using. Fix: spawn goroutines use context.Background() and the heartbeat cleanup defer waits for SubagentManager.WaitAll() before deactivating worktrees. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Spawn now uses a detached context. Add CancelTask() method with per-task cancel func so subagents can still be explicitly stopped. Update TestSubagentManager_Spawn_CancelledDuringExecution to use CancelTask() instead of parent context cancellation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Spawn goroutines now have a 30-minute hard timeout (spawnTimeout) - WaitAll accepts a timeout parameter so heartbeat cleanup doesn't block forever if a subagent gets stuck - CancelTask stores per-task cancel func for explicit cancellation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix: add LLM rate limiting and fix heartbeat subagent lifecycle
…new bubbles IsStatus and IsTaskStatus flags on OutboundMessage were set by the agent loop but completely ignored by the channel Manager. Every status update created a new message bubble, flooding chats with redundant messages. Add MessageSenderWithID interface and implement it for Telegram, Discord, and Pico channels. Manager now routes IsStatus/IsTaskStatus messages to dedicated handlers that track message IDs and edit in-place. Final responses also reuse the tracked status message via preSend. TTL janitor extended to clean up stale tracking entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix: deduplicate status/task messages by editing in-place
Heartbeat worktrees were auto-committed but never merged back, so their changes (e.g. project scaffolding) were effectively lost. Now the heartbeat defer attempts a fast-forward merge into the base branch after auto-commit. On conflict, the merge is aborted and the user is notified to merge manually. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename err variables to avoid shadowing the outer CreateWorktree error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat: auto-merge heartbeat worktree branch on completion
Adds two dedicated tools that bypass the shell exec deny patterns while maintaining strict safety invariants: - git_push: pushes the current worktree branch to origin. Requires active worktree context, blocks protected branches (main/master/develop/release/*), forbids force push, auto-commits uncommitted changes before pushing. - create_pr: creates a GitHub PR via `gh` CLI. Auto-detects base branch from worktree info, verifies branch is pushed, checks for merge conflicts with base via `git merge-tree --write-tree` before creating. Sandbox integration: - coder+ presets: git_push allowed - worker+ presets: create_pr also allowed - WorktreeInfo injected into tool context alongside workspace override https://claude.ai/code/session_01WWttNE5xShanYD6PhMzgKz
create_pr now implements AsyncTool. After successfully creating a PR, it spawns a background goroutine that polls `gh pr checks` every 30s (up to 15 min) and reports CI pass/fail via AsyncCallback. Flow: 1. PR creation returns immediately with AsyncResult (PR URL) 2. Background goroutine waits 10s for CI to register, then polls 3. On pass/fail/no-checks/timeout, calls callback with result 4. Agent receives notification and can act (e.g., gh run view for logs) https://claude.ai/code/session_01WWttNE5xShanYD6PhMzgKz
The asyncCallback in runLLMIteration was a no-op that only logged. For spawn this didn't matter (SubagentManager.runTask does its own PublishInbound), but create_pr's CI goroutine had no way to deliver results back to the conductor. Now asyncCallback publishes a system inbound message with senderID="async:<tool>" so processSystemMessage injects it into the conductor's session history. The conductor sees the CI result on its next turn. https://claude.ai/code/session_01WWttNE5xShanYD6PhMzgKz
…Tool Both tools get their working directory from worktree context, making the workspace field redundant. Also replace custom contains helper with strings.Contains in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat: add git_push and create_pr tools with async CI polling
…rnizations Resolve 65 file conflicts merging upstream/main into local branch. Key decisions: - Accept upstream's channel interface refactoring (clean Channel interface) - Accept upstream's Cobra CLI restructuring, Launcher TUI, armv6 support - Accept upstream's HTTP client caching, resp.Body leak fix, regex precompile - Accept upstream's WeChat/WeCom resource leak fixes and context handling - Accept upstream's migrate API restructure (NewMigrateInstance pattern) - Accept upstream's Go 1.22+ syntax modernizations (for range, max/min) - Preserve local orchestration, plan mode, heartbeat, sandbox, spawn system - Preserve local SendWithID/MessageSenderWithID for status message dedup - Preserve local Stream/RPM/functional options in providers - Rename AGENT.md → AGENTS.md to match upstream template convention Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ng fragment Two issues with heartbeat (SendResponse: false) tasks: 1. The last streaming status bubble persists as a garbled fragment because no final non-status message triggers preSend cleanup. 2. The "Task completed" notification shows the heartbeat prompt instead of the LLM response. Add Result/streamedChunks fields to activeTask, store the LLM response summary after the loop, publish a clean status replacement for background tasks that streamed, and use task.Result in the completion notification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes heartbeat completion UX by ensuring background/heartbeat tasks publish a clean final message (instead of leaving a garbled streaming fragment) and by using the actual LLM result in “Task completed” notifications. The PR also introduces broader infrastructure around streaming/status messages, worktree-based git tools, orchestration UI support, and observability utilities.
Changes:
- Track and replace streaming/status messages so heartbeat/background tasks clean up correctly and completion notifications show the final result.
- Add new tools (git push / create PR / dev preview / bg monitor) and supporting git worktree helpers.
- Add Mini App WebSocket endpoints (logs + orchestration), log ring-buffer + subscription, and token-usage stats tracking.
Reviewed changes
Copilot reviewed 86 out of 110 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tools/gitpush.go | Adds a safe worktree-restricted git_push tool. |
| pkg/tools/filesystem_test.go | Updates filesystem tests and adds validatePath error-content coverage. |
| pkg/tools/filesystem.go | Improves validatePath error, resolves FS per-path, and wraps ReadDir errors. |
| pkg/tools/edit_test.go | Updates test comments to match new FS abstractions. |
| pkg/tools/edit.go | Routes edit/append through resolveFS for correct sandbox/host dispatch. |
| pkg/tools/dev_preview.go | Adds tool to control Mini App dev reverse proxy targets. |
| pkg/tools/cron.go | Minor builder refactor for job listing output. |
| pkg/tools/createpr_test.go | Adds unit tests for create_pr tool behavior and preset allowlists. |
| pkg/tools/createpr.go | Adds create_pr async tool with CI polling via gh pr checks. |
| pkg/tools/bg_monitor_test.go | Adds tests for background process monitoring tool behavior. |
| pkg/tools/bg_monitor.go | Adds bg_monitor tool for listing/watching/tailing bg exec output. |
| pkg/tools/base.go | Introduces optional StatusProvider interface for runtime status injection. |
| pkg/tailscale/detect_test.go | Adds tests for parsing Tailscale hostname. |
| pkg/tailscale/detect.go | Adds Tailscale hostname detection + cert fetching helpers. |
| pkg/stats/tracker_test.go | Adds tests for stats persistence, rolling, reset, and formatting. |
| pkg/stats/tracker.go | Adds persistent token-usage tracker with periodic flush. |
| pkg/skills/search_cache.go | Preallocates cache maps/slices for less allocation churn. |
| pkg/skills/registry.go | Preallocates merged results for SearchAll to reduce allocations. |
| pkg/skills/loader.go | Switches to strings.Builder for skill context/summary assembly. |
| pkg/skills/clawhub_registry.go | Uses strconv.Itoa for limit param formatting. |
| pkg/session/manager_test.go | Adds tests for history sanitization around tool-call ordering. |
| pkg/session/manager.go | Adds history sanitization + deferred write-behind persistence hooks. |
| pkg/providers/types.go | Adds streaming types and StreamingProvider interface. |
| pkg/providers/tool_call_extract.go | Adds fuzzy XML tool-call extraction + stripping helpers. |
| pkg/providers/protocoltypes/types.go | Adds StreamEvent + StreamToolCallDelta protocol types. |
| pkg/providers/http_provider.go | Adds options-based HTTP provider ctor; XML tool-call fallback; streaming passthrough. |
| pkg/providers/factory_test.go | Adds tests for CreateProviderByName behavior. |
| pkg/providers/factory_provider.go | Adds provider options (streaming, timeout, RPM throttle) and MiniMax protocol support. |
| pkg/providers/factory.go | Refactors selection resolver and adds CreateProviderByName. |
| pkg/providers/codex_cli_provider.go | Avoids extra string conversion when embedding JSON parameters. |
| pkg/providers/claude_cli_provider_test.go | Adds extensive tests for XML tool-call extract/strip behavior. |
| pkg/providers/claude_cli_provider.go | Avoids extra string conversion when embedding JSON parameters. |
| pkg/orch/state.go | Defines typed agent lifecycle states for orchestration UI. |
| pkg/orch/reporter_test.go | Adds tests verifying reporter mappings and noop safety. |
| pkg/orch/reporter.go | Adds AgentReporter interface + noop implementation. |
| pkg/orch/broadcaster_test.go | Adds tests for event delivery, snapshot behavior, and non-blocking publish. |
| pkg/orch/broadcaster.go | Adds orchestration event broadcaster + live agent snapshot. |
| pkg/miniapp/ws.go | Adds WebSocket log streaming and orchestration event streaming. |
| pkg/miniapp/types.go | Adds Mini App API types + state notifier for SSE. |
| pkg/miniapp/static/map.js | Adds “orchestration room” canvas map rendering logic. |
| pkg/miniapp/miniapp.go | Adds Mini App handler with routes and static embedding. |
| pkg/miniapp/logs.go | Adds log snapshot tar.gz creation + download + cleanup. |
| pkg/miniapp/auth.go | Adds Telegram WebApp initData validation + allowlist enforcement. |
| pkg/miniapp/api.go | Adds Mini App JSON APIs + SSE state stream. |
| pkg/logger/logger_test.go | Adds tests for ring buffer, RecentLogs, subscriptions, and sanitization. |
| pkg/logger/logger.go | Adds ring buffer, subscriptions, field sanitization, and RecentLogs/ParseLevel. |
| pkg/heartbeat/service.go | Adds heartbeat notification suppression + think-block stripping safety net. |
| pkg/health/server.go | Exposes mux + adds TLS start helper. |
| pkg/git/worktree_test.go | Adds tests for worktree helpers and disposal/merge/prune behavior. |
| pkg/git/worktree.go | Adds worktree creation/disposal/merge/prune + branch name sanitization. |
| pkg/config/migration.go | Preallocates provider→model_list conversion slice. |
| pkg/config/defaults.go | Adds default TaskReminderInterval. |
| pkg/config/config_test.go | Adds tests for plan model parsing and override behavior. |
| pkg/config/config.go | Adds plan-model config fields, telegram webapp URL, stream flag, and model ref lookup. |
| pkg/channels/telegram/telegram_commands.go | Updates help text for new Telegram commands. |
| pkg/channels/telegram/telegram.go | Adds SendWithID for message tracking/editing. |
| pkg/channels/slack/slack.go | Refactors content building to reduce string churn. |
| pkg/channels/pico/pico.go | Adds SendWithID with generated message IDs. |
| pkg/channels/manager.go | Adds status/task message tracking + editing flows; moves webhooks onto health mux. |
| pkg/channels/interfaces.go | Adds MessageSenderWithID interface. |
| pkg/channels/discord/discord.go | Adds SendWithID for Discord. |
| pkg/bus/types.go | Adds status/task flags and IDs to outbound messages. |
| pkg/agent/session_tracker_test.go | Adds tests for session activity tracking APIs. |
| pkg/agent/session_tracker.go | Adds active-session tracker for coordination/observability. |
| pkg/agent/loop_reporter_test.go | Adds tests for orchestration event emission by agent loop. |
| pkg/agent/instance.go | Registers new tools, adds orchestration defaults, plan-model selection, and worktree lifecycle. |
| docs/plan-interview-improvements.md | Adds design notes for plan interview improvements. |
| cmd/picoclaw/internal/gateway/command.go | Adds CLI flags for orchestration and stats. |
| cmd/picoclaw/internal/agent/helpers.go | Adds CLI orchestration enablement wiring. |
| cmd/picoclaw/internal/agent/command.go | Adds --orchestration flag to agent command. |
| Makefile | Adds build/install hints and additional build targets. |
| .claude/settings.local.json | Adds Claude local settings/permissions file. |
Comments suppressed due to low confidence (3)
pkg/tools/filesystem.go:39
- The workspace path is interpolated without quoting, which can be hard to read (especially with spaces) and ambiguous. Consider using
%q(or a clearer... outside workspace: <path>format) so callers can reliably see the exact boundary value.
return "", fmt.Errorf("access denied: path outside workspace %s", absWorkspace)
pkg/miniapp/logs.go:88
- There are duplicated comment lines for
apiLogsSnapshotDownloadandcleanOldSnapshots. Please remove the duplicates to keep the file’s documentation clean and avoid confusion during future edits.
// apiLogsSnapshotDownload serves a snapshot tar.gz file.
// apiLogsSnapshotDownload serves a snapshot tar.gz file.
pkg/miniapp/logs.go:118
- There are duplicated comment lines for
apiLogsSnapshotDownloadandcleanOldSnapshots. Please remove the duplicates to keep the file’s documentation clean and avoid confusion during future edits.
// cleanOldSnapshots removes snapshot files older than maxAge.
// cleanOldSnapshots removes snapshot files older than maxAge.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## build-whatsapp-native: Build with WhatsApp native (whatsmeow) support; larger binary | ||
| build-whatsapp-native: generate | ||
| ## @echo "Building $(BINARY_NAME) with WhatsApp native for $(PLATFORM)/$(ARCH)..." | ||
| @echo "Building for multiple platforms..." | ||
| @mkdir -p $(BUILD_DIR) | ||
| GOOS=linux GOARCH=amd64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-linux-amd64 ./$(CMD_DIR) | ||
| GOOS=linux GOARCH=arm GOARM=7 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-linux-arm ./$(CMD_DIR) | ||
| GOOS=linux GOARCH=arm64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-linux-arm64 ./$(CMD_DIR) | ||
| GOOS=linux GOARCH=loong64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-linux-loong64 ./$(CMD_DIR) | ||
| GOOS=linux GOARCH=riscv64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-linux-riscv64 ./$(CMD_DIR) | ||
| GOOS=darwin GOARCH=arm64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-darwin-arm64 ./$(CMD_DIR) | ||
| GOOS=windows GOARCH=amd64 $(GO) build -tags whatsapp_native $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME)-windows-amd64.exe ./$(CMD_DIR) | ||
| ## @$(GO) build $(GOFLAGS) -tags whatsapp_native $(LDFLAGS) -o $(BINARY_PATH) ./$(CMD_DIR) | ||
| @echo "Build complete" | ||
| ## @ln -sf $(BINARY_NAME)-$(PLATFORM)-$(ARCH) $(BUILD_DIR)/$(BINARY_NAME) |
There was a problem hiding this comment.
The build-whatsapp-native target is defined twice in the Makefile. GNU Make will treat this as an override (and may warn), and only the latter recipe will apply, which is likely unintended. Remove the duplicate target (or merge the recipes) so there is exactly one build-whatsapp-native definition.
| ## build-whatsapp-native: Build with WhatsApp native (whatsmeow) support; larger binary | ||
| build-whatsapp-native: generate |
There was a problem hiding this comment.
The build-whatsapp-native target is defined twice in the Makefile. GNU Make will treat this as an override (and may warn), and only the latter recipe will apply, which is likely unintended. Remove the duplicate target (or merge the recipes) so there is exactly one build-whatsapp-native definition.
| ## build-whatsapp-native: Build with WhatsApp native (whatsmeow) support; larger binary | |
| build-whatsapp-native: generate | |
| ## build-whatsapp-native-all: Build with WhatsApp native (whatsmeow) support for multiple platforms; larger binaries | |
| build-whatsapp-native-all: generate |
| entry := LogEntry{ | ||
| Level: logLevelNames[level], | ||
| Timestamp: time.Now().UTC().Format(time.RFC3339), | ||
| Timestamp: time.Now().Format(time.RFC3339), |
There was a problem hiding this comment.
Timestamp formatting changed from UTC to local time (time.Now() vs time.Now().UTC()). This can break ordering/correlation across machines and make logs ambiguous without an offset. Prefer time.Now().UTC().Format(time.RFC3339) (or RFC3339Nano) to keep timestamps consistent.
| Timestamp: time.Now().Format(time.RFC3339), | |
| Timestamp: time.Now().UTC().Format(time.RFC3339), |
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs | ||
| groupOK := true | ||
| if i+needed >= len(history) { | ||
| groupOK = false | ||
| } else { | ||
| for j := 0; j < needed; j++ { | ||
| next := history[i+1+j] | ||
| if next.Role != "tool" || !expectedIDs[next.ToolCallID] { | ||
| groupOK = false | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
SanitizeHistory validates tool-result groups using only set membership (expectedIDs[next.ToolCallID]) but does not ensure each expected ToolCallID appears exactly once. If the next messages duplicate one tool ID (e.g., tool=a, tool=a) the group will be treated as valid even though another expected tool result is missing. Fix by tracking seen IDs (or deleting from expectedIDs as they’re matched) and requiring that all expected IDs are matched exactly once.
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs | |
| groupOK := true | |
| if i+needed >= len(history) { | |
| groupOK = false | |
| } else { | |
| for j := 0; j < needed; j++ { | |
| next := history[i+1+j] | |
| if next.Role != "tool" || !expectedIDs[next.ToolCallID] { | |
| groupOK = false | |
| break | |
| } | |
| } | |
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs, | |
| // and each expected ID must appear exactly once. | |
| groupOK := true | |
| if i+needed >= len(history) { | |
| groupOK = false | |
| } else { | |
| for j := 0; j < needed; j++ { | |
| next := history[i+1+j] | |
| if next.Role != "tool" { | |
| groupOK = false | |
| break | |
| } | |
| if !expectedIDs[next.ToolCallID] { | |
| // Either this ID was never expected, or it has already been matched. | |
| groupOK = false | |
| break | |
| } | |
| // Mark this ID as consumed so it cannot be matched again. | |
| delete(expectedIDs, next.ToolCallID) | |
| } | |
| // After consuming `needed` tool messages, all expected IDs must have been matched exactly once. | |
| if groupOK && len(expectedIDs) != 0 { | |
| groupOK = false | |
| } |
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs | ||
| groupOK := true | ||
| if i+needed >= len(history) { | ||
| groupOK = false | ||
| } else { | ||
| for j := 0; j < needed; j++ { | ||
| next := history[i+1+j] | ||
| if next.Role != "tool" || !expectedIDs[next.ToolCallID] { | ||
| groupOK = false | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
SanitizeHistory validates tool-result groups using only set membership (expectedIDs[next.ToolCallID]) but does not ensure each expected ToolCallID appears exactly once. If the next messages duplicate one tool ID (e.g., tool=a, tool=a) the group will be treated as valid even though another expected tool result is missing. Fix by tracking seen IDs (or deleting from expectedIDs as they’re matched) and requiring that all expected IDs are matched exactly once.
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs | |
| groupOK := true | |
| if i+needed >= len(history) { | |
| groupOK = false | |
| } else { | |
| for j := 0; j < needed; j++ { | |
| next := history[i+1+j] | |
| if next.Role != "tool" || !expectedIDs[next.ToolCallID] { | |
| groupOK = false | |
| break | |
| } | |
| } | |
| // Peek ahead: the next `needed` messages must all be tool results with matching IDs, | |
| // and each expected ID must appear exactly once. | |
| groupOK := true | |
| if i+needed >= len(history) { | |
| groupOK = false | |
| } else { | |
| seen := make(map[string]bool, needed) | |
| for j := 0; j < needed; j++ { | |
| next := history[i+1+j] | |
| id := next.ToolCallID | |
| if next.Role != "tool" || !expectedIDs[id] || seen[id] { | |
| groupOK = false | |
| break | |
| } | |
| seen[id] = true | |
| } | |
| if groupOK && len(seen) != needed { | |
| groupOK = false | |
| } |
| // Verify the branch has been pushed by checking if the remote ref exists | ||
| checkCtx, checkCancel := context.WithTimeout(ctx, 15*time.Second) | ||
| defer checkCancel() | ||
| checkCmd := exec.CommandContext(checkCtx, "git", "ls-remote", "--exit-code", "origin", branch) |
There was a problem hiding this comment.
git ls-remote origin <pattern> matches by pattern and can yield false positives (e.g., tags or similarly-prefixed refs). If the intent is to verify the remote branch exists, prefer querying the fully-qualified ref (e.g., refs/heads/<branch>). That makes the push-precondition check accurate.
| checkCmd := exec.CommandContext(checkCtx, "git", "ls-remote", "--exit-code", "origin", branch) | |
| remoteRef := "refs/heads/" + branch | |
| checkCmd := exec.CommandContext(checkCtx, "git", "ls-remote", "--exit-code", "origin", remoteRef) |
| switch action { | ||
| case "start": | ||
| target, _ := args["target"].(string) | ||
| if target == "" { | ||
| return ErrorResult("target is required for start action") | ||
| } | ||
| name, _ := args["name"].(string) | ||
| if name == "" { | ||
| name = inferName(target) | ||
| } | ||
| id, err := t.manager.RegisterDevTarget(name, target) |
There was a problem hiding this comment.
The tool description/parameters state that target 'Must be a localhost URL', but Execute() does not enforce that. If DevTargetManager doesn’t strictly validate, this could open SSRF-style proxying to arbitrary hosts. Validate target in the tool (parse URL, require loopback/localhost hostname, reject non-http(s), and disallow credentials) before calling RegisterDevTarget.
| cid, err := parseChatID(chatID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid chat ID %s: %w", chatID, channels.ErrSendFailed) | ||
| } |
There was a problem hiding this comment.
Both error paths lose important context: the parse error isn’t included, and the underlying Telegram API error is discarded on send failure. Include the original err in the wrapped error (while still wrapping the sentinel error) so logs/debugging can pinpoint the root cause.
| sent, err := c.bot.SendMessage(ctx, tgMsg) | ||
| if err != nil { | ||
| // Fallback to plain text | ||
| tgMsg.ParseMode = "" | ||
| sent, err = c.bot.SendMessage(ctx, tgMsg) | ||
| if err != nil { | ||
| return "", fmt.Errorf("telegram send: %w", channels.ErrTemporary) | ||
| } | ||
| } |
There was a problem hiding this comment.
Both error paths lose important context: the parse error isn’t included, and the underlying Telegram API error is discarded on send failure. Include the original err in the wrapped error (while still wrapping the sentinel error) so logs/debugging can pinpoint the root cause.
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(grep:*)", | ||
| "Bash(find:*)", | ||
| "Bash(go build:*)", | ||
| "Bash(go test:*)", | ||
| "Bash(git add:*)", | ||
| "Bash(git commit:*)", | ||
| "Bash(git push:*)", | ||
| "Bash(gh issue view:*)", |
There was a problem hiding this comment.
A settings.local.json file typically contains machine/user-local permissions and environment identifiers and is usually not intended to be committed. This can inadvertently leak local environment details and encourages relying on developer-specific config. Consider removing it from the repo and adding it to .gitignore, and (if needed) commit a sanitized template file instead.
Summary
Result/streamedChunksfields toactiveTask, store the LLM response summary after the loop, publish a clean status replacement for background tasks that streamed, and usetask.Resultin the completion notificationTest plan
go build ./.../go test ./.../go vet ./...— all pass🤖 Generated with Claude Code