Commit Graph
3 Commits
Author SHA1 Message Date
41ed0a0f98 feat(agent): thread per-tool reasoning through provider, session, and all surfaces (#1513)
* feat(agent): thread per-tool reasoning from LLM through to REPL, HTTP, SSE, and DB

Add end-to-end agent reasoning summaries so users can see *why* the
agent chose specific tools, not just what it did.

- Add `reasoning: Option<String>` to `ToolCall` (all providers)
- Populate from LLM response content in `Reasoning::respond_with_tools`
  and `select_tools`, with per-tool override when providers supply it
- Extend `Turn` with `narrative` and `TurnToolCall` with `rationale` +
  `tool_call_id` for identity-based result matching
- Persist reasoning in DB via existing tool_calls JSON (no migration)
- Add `StatusUpdate::ReasoningUpdate` and `SseEvent::ReasoningUpdate` +
  `SseEvent::JobReasoning` for real-time streaming
- Emit reasoning events in both chat dispatcher and worker job path
- Add `/reasoning [N|all]` command for inspecting turn reasoning
- Surface `narrative` and `rationale` in HTTP `/api/chat/history`

Based on the design from #361 and #456, reconstructed cleanly with
Option<String> to minimize blast radius (vs mandatory String that broke
compilation in #456).

Closes #456

Co-Authored-By: panosAthDBX <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address PR review feedback from Gemini and Copilot

- Fix `_ => Ok(None)` in agent_loop.rs to avoid accidental shutdown
- Fix fallback in record_tool_result_for/record_tool_error_for to use
  first pending call instead of last_mut (parallel execution safety)
- Include per-tool decisions in WASM channel reasoning messages
- Apply truncate_at_tool_tags + clean_response to shared_reasoning in
  select_tools (parity with respond_with_tools)
- Persist turn-level narrative to DB in tool_calls JSON wrapper
- Parse both old (array) and new (object) tool_calls formats in
  build_turns_from_db_messages for backward compatibility
- Populate reasoning from action.reasoning in execute_plan ToolCalls

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address second round of review comments + merge fixes

- Add reasoning: None to new github_copilot.rs ToolCall sites (from staging merge)
- Run cargo fmt on 4 files with formatting diffs
- Truncate narrative to 1000 chars before DB persistence
- Clone turn data and drop session lock in /reasoning command
- Extract ToolDecisionDto::from_json_array shared helper (deduplicate
  worker/job.rs and orchestrator/api.rs)
- Add unit tests for wrapped tool_calls JSON format with narrative

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address third round of review comments (Copilot + serrrfirat)

- Reword ToolCall.reasoning docstring to reflect provider-supplied or
  fallback contract
- Sanitize narrative through SafetyLayer before storage/emission
- Clean per-tool reasoning via truncate_at_tool_tags + clean_response
  in select_tools (parity with shared reasoning)
- Convert 4 approval-path recording sites in thread_ops.rs to
  identity-based record_tool_result_for/record_tool_error_for
- Preserve tool_call_id and reasoning through restore_from_messages
- Fix has_result/has_error to reject JSON null values
- Truncate tool_call_id to 128 chars before DB persistence
- Add 4 unit tests for record_tool_result_for/error_for edge cases

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address zmanian review — sanitize JobDelegate reasoning + warn on dropped results

- Sanitize narrative and per-tool rationale through SafetyLayer in
  JobDelegate reasoning events (parity with ChatDelegate)
- Add tracing::warn when record_tool_result_for/error_for drops a
  result because no matching or pending tool call exists
- Add 3 unit tests for reasoning normalization (thinking tags,
  tool tags, empty-after-cleaning)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address 4 remaining unreplied review comments

- Clean per-tool reasoning in respond_with_tools via truncate_at_tool_tags
  + clean_response (parity with select_tools)
- Handle wrapped JSON format in rebuild_chat_messages_from_db so cold
  hydration works after persist_tool_calls format change
- Update persist_tool_calls doc comment to describe new JSON shape
- Sanitize per-tool rationale through SafetyLayer in ChatDelegate before
  emission and storage (parity with JobDelegate)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address zmanian review round 2

- Add tracing::debug on fallback-to-pending path in record_tool_result_for
  and record_tool_error_for (item 1)
- Add comment explaining why /reasoning is special-cased in agent_loop.rs
  (item 4)
- Items 2 (narrative persistence), 3 (rationale sanitization), and 5
  (catch-all fix) were already addressed in prior commits

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: panosAthDBX <[email protected]>
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-03-25 08:35:41 -07:00
fa51b9f52d fix: post-merge review sweep — 8 fixes across security, perf, and correctness (#1550)
* fix: post-merge review sweep — 8 fixes across security, perf, and correctness

1. Fix code fence detection in extract_suggestions() (issue #1180)
   - rfind("```") couldn't handle odd fence counts (unclosed blocks)
   - Now counts all fence positions and checks parity

2. Cache routine parameters_schema() with OnceLock (issue #1361)
   - routine_create_parameters_schema() and event_emit_parameters_schema()
     were regenerating JSON on every LLM call

3. Replace O(n) LRU eviction with lru crate (issue #1430)
   - Embedding cache now uses lru::LruCache for O(1) eviction
   - Removes manual HashMap + last_accessed tracking

4. Fix WASM router secret_validated semantics (issue #1281)
   - Now reflects whether any auth (secret/Ed25519/HMAC) was performed
   - Previously only checked if a secret was configured

5. Sanitize channel/user in routine prompt interpolation (issue #1364)
   - Defense-in-depth: strip newlines, replace backticks, truncate to 128
     chars before injecting into LLM prompt

6. Remove duplicate 401 retry in github_copilot.rs (PR #1512 review)
   - Internal retry conflicted with outer RetryProvider causing nested
     retries; now invalidates token and lets RetryProvider handle retry

7. Fix token error classification in github_copilot.rs (PR #1512 review)
   - AccessDenied/Expired errors now map to AuthFailed (non-retryable)
   - Transient errors remain RequestFailed (retryable)

8. Fix parse_extra_headers() hardcoded env var name (PR #1512 review)
   - Error messages now report the actual env var being parsed instead
     of always saying LLM_EXTRA_HEADERS

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address PR review comments and fix formatting

- sanitize_prompt_field: single-pass with map() instead of collect+replace
- embed(): re-check cache under lock before cloning (thundering herd)
- embed_batch(): limit caching to cache capacity, skip overflow entries
- router: thread did_authenticate bool instead of re-calling async methods
- github_copilot 401: use generic error message, avoid leaking response body
- cargo fmt: fix two formatting violations caught by CI

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* chore: trigger CI re-run with updated refs

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-03-23 14:50:15 -07:00
6232609080 feat(llm): add GitHub Copilot as LLM provider (#1512)
* Add github copilot as LLM provider.

* Fix Copilot in Openclaw

* security: harden Copilot OAuth token handling

C1: Use secrecy::SecretString for oauth_token and cached session token
    in CopilotTokenManager/CachedCopilotToken. Expose only at HTTP
    header injection point via .expose_secret().

C2: Document risks of hardcoded VS Code OAuth client ID and editor
    identity headers (ToS, rotation, staleness). Remove the unreliable
    paste-token setup path (setup_github_copilot_manual_token).

C3: Fix TOCTOU race in get_token() — re-check token validity after
    acquiring write lock so concurrent callers don't all perform
    redundant token exchanges.

I1: Remove dead empty else {} block in get_token().

I2: Map 401 responses to LlmError::AuthFailed instead of RequestFailed
    so retry/circuit-breaker logic handles auth failures correctly.

I3: Replace prepare_github_copilot_setup() with call to existing
    set_llm_backend_preserving_model() helper to avoid logic drift.

I4: Add unit tests for CopilotTokenManager (caching, invalidation,
    expiry/buffer behavior), poll response parsing (all OAuth device
    flow states), and DeviceCodeResponse/CopilotTokenResponse deserialization.

Co-authored-by: Copilot <[email protected]>

* fix: address review feedback and code improvements (takeover #1202)

- Fix ContentPart::Text being silently dropped in convert_messages
- Replace custom truncate_for_error with crate::util::floor_char_boundary
- Fix CLAUDE.md: accurately describe dedicated provider (not "OpenAI-compatible path")
- Fix "Github" -> "GitHub" capitalization in READMEs
- Add manual token paste option to setup wizard (not just device login)
- Fix missing extension_manager field in EngineContext (merge fixup)
- cargo fmt applied

Co-Authored-By: fallenwood <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address PR review feedback for GitHub Copilot provider

- Plumb request_timeout_secs into GithubCopilotProvider (was hardcoded 120s)
- Forward stop_sequences to Copilot API via OpenAI `stop` field
- Skip empty text part in multimodal message conversion
- Improve paste-token wizard hint with specific file path guidance

Co-Authored-By: fallenwood <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: 401 retry, retryable token exchange errors, shared retry-after parsing

- Retry once inline on 401 after token invalidation (was returning
  AuthFailed immediately, guaranteeing user-visible failure)
- Map token exchange failures to RequestFailed (retryable) instead of
  AuthFailed (non-retryable by RetryProvider)
- Use shared crate::llm::retry::parse_retry_after for HTTP-date support
  and safe 60s default
- Improve paste-token wizard hint: mention `gh auth token` as primary source

Co-Authored-By: fallenwood <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: 401 retry error mapping, retry status logging, token whitespace safety

- Map 401 retry get_token() failure to RequestFailed (retryable),
  consistent with initial token acquisition path
- Log retry response status before returning AuthFailed
- Trim oauth_token in exchange_copilot_token to prevent header panics
  from whitespace in env vars

Co-Authored-By: fallenwood <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Fallenwood <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: fallenwood <[email protected]>
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-03-21 00:02:00 -07:00