Skip to content

feat: Feature parity for remote mode#253

Closed
kdy1 wants to merge 5 commits intomainfrom
claude/issue-252-20260208-0536
Closed

feat: Feature parity for remote mode#253
kdy1 wants to merge 5 commits intomainfrom
claude/issue-252-20260208-0536

Conversation

@kdy1
Copy link
Copy Markdown
Contributor

@kdy1 kdy1 commented Feb 8, 2026

Implement remote mode support for previously unimplemented Tauri commands.

Summary

  • Add CancelTask, GetCompositeTaskNodes, GetTaskLogs RPC types
  • Add 5 new server endpoints: cancel, dismiss-approval, create-pr, get-composite-nodes, get-logs
  • Add corresponding remote client methods
  • Update Tauri cancel_task to call remote API instead of returning error
  • Update Tauri get_composite_task_nodes to fetch from server
  • Update Tauri get_task_logs to stream logs from server
  • Update API documentation

Closes #252

Generated with Claude Code

…_composite_task_nodes

Implement feature parity for remote mode by adding missing server
endpoints and connecting Tauri commands to use the remote client.

Changes:
- Add CancelTask, GetCompositeTaskNodes, GetTaskLogs RPC types
- Add 5 new server endpoints: cancel, dismiss-approval, create-pr,
  get-composite-nodes, get-logs
- Add corresponding remote client methods
- Update Tauri cancel_task to call remote API instead of returning error
- Update Tauri get_composite_task_nodes to fetch from server
- Update Tauri get_task_logs to stream logs from server
- Update API documentation

Closes #252

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
@kdy1 kdy1 closed this Feb 8, 2026
@kdy1 kdy1 deleted the claude/issue-252-20260208-0536 branch February 8, 2026 05:49
@kdy1 kdy1 restored the claude/issue-252-20260208-0536 branch February 8, 2026 05:49
@kdy1 kdy1 reopened this Feb 8, 2026
@claude

This comment has been minimized.

@kdy1
Copy link
Copy Markdown
Contributor Author

kdy1 commented Feb 8, 2026

@claude Fix all critical issues in https://github.com/delinoio/delidev/pull/253#issuecomment-3866230758

@claude

This comment has been minimized.

- Fix critical issue: cancel_task now signals the worker via /cancel endpoint
  to actually stop execution (previously only updated DB status)
- Fix data duplication: server sends raw sessions only, client parses events
  from output_log (avoids sending both raw logs and parsed events)
- Fix event ID calculation: extract parse_output_log_events helper with
  simple line-index-based IDs, replacing complex arithmetic
- Fix silent failures: add debug logging for unparseable log lines
- Add find_worker_by_task_id to WorkerRegistry
- Add reqwest dependency to main-server for worker communication
- Update API and design documentation

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
@claude

This comment has been minimized.

@kdy1
Copy link
Copy Markdown
Contributor Author

kdy1 commented Feb 9, 2026

@claude fix all critical issues in https://github.com/delinoio/delidev/pull/253#issuecomment-3866358070

@claude

This comment has been minimized.

- Add shared HTTP client to AppState with 10s timeout, replacing
  per-request client creation in cancel_task (fixes resource leak
  and missing timeout)
- Prevent duplicate PR creation instructions in create_pr by
  checking for existing marker before appending
- Add input validation for after_event_id (must be non-negative)
  in get_task_logs endpoint
- Return explicit error for invalid dependency IDs instead of
  silently dropping them in get_composite_task_nodes

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

This comment has been minimized.

@kdy1
Copy link
Copy Markdown
Contributor Author

kdy1 commented Feb 9, 2026

@claude Fix critical issues in https://github.com/delinoio/delidev/pull/253#issuecomment-3869607422

@claude

This comment has been minimized.

- Fix worker cancellation race condition: signal worker BEFORE updating
  DB status to Cancelled, preventing a window where the DB shows
  Cancelled but the worker continues running unaware.
- Add upper bound validation (10M) for after_event_id in get_task_logs
  to prevent clients from sending unreasonably large values.

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

This comment has been minimized.

@kdy1
Copy link
Copy Markdown
Contributor Author

kdy1 commented Feb 9, 2026

@claude Fix all critical issues in https://github.com/delinoio/delidev/pull/253#issuecomment-3869660787

@claude

This comment has been minimized.

…feature

- SSRF prevention: Add URL validation for worker endpoint URLs during
  registration and before making outbound HTTP requests. Block private IPs,
  localhost, and non-HTTP schemes.
- HTTP client hardening: Add connection pool limits (max_idle_per_host=5,
  idle_timeout=30s) to prevent resource exhaustion from malicious workers.
- Log size limits: Truncate output_log to MAX_LOG_LINES_PER_SESSION (5000)
  lines and support incremental log retrieval via after_event_id to prevent
  memory exhaustion from large logs.
- Worker cancellation: Return error when worker actively rejects cancellation
  instead of silently marking task as cancelled while worker continues running.
- N+1 query fix: Include unit_tasks in GetCompositeTaskNodesResponse so the
  Tauri client doesn't need separate API calls per node.
- Tests: Add integration tests for cancel_task, dismiss_approval, create_pr,
  get_composite_task_nodes, get_task_logs, and SSRF URL validation.

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Feb 9, 2026

PR Review: Feature Parity for Remote Mode

I've reviewed PR #253 and overall this is a well-implemented feature that brings important remote mode capabilities. Here are my findings:

✅ Strengths

  1. Excellent Security Implementation

    • Strong SSRF protection in validate_worker_endpoint_url() (worker_registry.rs:18-67)
    • Proper validation at registration time AND before making requests (defense-in-depth)
    • Comprehensive test coverage for SSRF validation
    • Good error messages that don't leak internal details
  2. Well-Structured Code

    • Clear separation of concerns between server endpoints and client methods
    • Consistent error handling patterns
    • Good use of constants for limits (MAX_AFTER_EVENT_ID, MAX_LOG_LINES_PER_SESSION)
    • Extensive test coverage with 20+ new tests
  3. Documentation

    • API documentation updated in docs/api.md
    • Design document updated to reflect cancellation workflow
    • Clear inline comments explaining non-obvious logic

🔧 Issues to Address

1. CRITICAL: IPv6 Private Network Detection Incomplete (worker_registry.rs:80-84)

The IPv6 validation is missing critical ranges that could enable SSRF:

  • fc00::/7 - Unique Local Addresses (IPv6 equivalent of 10.0.0.0/8)
  • fe80::/10 - Link-local addresses
  • IPv4-mapped IPv6 addresses like ::ffff:192.168.0.1

2. Race Condition in Task Cancellation (api/task.rs:108-189)

The code checks task status before signaling the worker, but the task could transition between the check and the update. If the worker completes the task between the check and the update, you'll mark a completed task as cancelled.

Recommendation: Use a database transaction or compare-and-swap operation to ensure atomicity.

3. Potential Information Leak (api/task.rs:128-140)

Logging the full endpoint URL could expose internal infrastructure details in logs. Sanitize the URL in logs or log only the validation error.

4. Unbounded Resource Usage (api/task.rs:315-327)

N+1 query pattern in get_composite_task_nodes. For a composite task with 1000 nodes, this makes 1000 database queries. Consider adding a batch fetch method to TaskStore.

5. Client-Side Log Parsing Could Fail Silently (commands/task.rs:1024-1032)

Parse failures are only logged at debug level. Log at warn level at minimum or consider adding a metric.

⚠️ Concerns

  1. HTTP Client Pool Configuration - HTTP_POOL_MAX_IDLE_PER_HOST: 5 seems low for many workers
  2. No Retry Logic for Worker Communication - A single network glitch causes cancellation failure
  3. Test Coverage Gaps - No tests for concurrent cancellation or timeout behavior
  4. Error Message Quality - Using {:?} in user-facing errors exposes internal enum representation

🎯 Recommendation

Approve with changes required for the IPv6 SSRF vulnerability. The other issues are important but not blocking.

  1. MUST FIX before merge: IPv6 private network detection
  2. SHOULD FIX before merge: Race condition in cancellation, N+1 queries
  3. NICE TO HAVE: Other suggestions can be follow-up PRs

Great work overall! The security-first approach and comprehensive testing are particularly commendable.

@kdy1 kdy1 closed this Feb 13, 2026
@kdy1 kdy1 deleted the claude/issue-252-20260208-0536 branch February 13, 2026 05:02
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.

Feature parity for remote mode

1 participant