lywsvip/ironclaw
1
0
Fork 0
mirror of https://github.com/nearai/ironclaw.git synced 2026-09-03 08:06:01 +08:00
Code Issues Packages Projects Releases Wiki Activity
1,244 Commits 1,246 Branches 71 Tags
77e746f683fd3263facd6c81b78fbe021925b07f
Commit Graph

247 Commits

Author SHA1 Message Date
Illia Polosukhin
77e746f683 feat(portfolio): complete tool, tests, widget, and share-gains flow (#2368) 
* feat(portfolio): complete tool, tests, widget, and share-gains flow

Portfolio WASM tool with full pipeline:
- Indexer (fixture, dune, dune-replay backends)
- Analyzer (6 protocol classifiers, health extraction, stablecoin detection)
- Strategy filter (yield-floor, health-guard, LP impermanent-loss-watch)
- Intent builder (fixture + solver backends, bounded checks, leg bundling)
- Format (suggestion markdown, progress metric, widget state)

172 unit tests covering all modules including edge cases:
- filter.rs: 33 tests (yield floor, health guard, LP watch, helpers)
- bounded.rs: 16 tests (slippage, cost, chain allowlist, multi-leg)
- parser.rs: 18 tests (delimiters, YAML, kind inference, real strategies)
- fixture.rs: 14 tests (slippage calc, ID formats, payload structure)
- analyzer: 18 tests (stablecoin detection, health extraction, debt/yield)
- format.rs: 16 tests (totals, empty states, progress windowing)
- widget.rs: 10 tests (rendering, intents, non-ready filtering)
- types: 16 tests (parse_decimal, ChainSelector serde)
- 14 YAML replay scenarios + 4 live Dune API tests (ignored by default)

Share-gains feature:
- Gateway-level IronClaw.api.share() modal with X, LinkedIn, Facebook,
  copy-to-clipboard, and download buttons
- Portfolio widget generates SVG card showing gains (APY, annual savings,
  moves found) — no addresses or balances exposed
- "Share gains" button appears only when portfolio has positive delta

E2E Playwright tests (11 scenarios):
- Skill discovery via API and settings UI
- Chat integration (keyword + wallet address triggering)
- Widget rendering with pre-seeded state (positions, totals, suggestions)
- Share button visibility (present with gains, absent without)
- Share modal lifecycle (opens with card image, social buttons, closes)

Supporting changes:
- E2E conftest: SKILLS_DIR points to workspace skills/
- Mock LLM: canned responses for portfolio/defi and wallet address patterns
- Skill YAML, registry entry, capabilities JSON, 3 strategy docs, 4 scripts

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(portfolio): address PR review — XSS, OnceLock, bounded checks, docs

Addresses review comments from #2368:

- XSS: widget renders all interpolated fields through escapeHtml();
  share modal creates <img> via DOM API with data:image/ prefix check
- OnceLock: protocol registry parsed once via std::sync::OnceLock
- to_ascii_lowercase() for wallet address lookups (fixture + dune_replay)
- bounded.rs: reject empty value_usd in single-leg slippage check
- fixture.rs: compute min_out amount and value_usd separately
- fixture.rs: clarify expires_at=0 comment (fixture = no expiry)
- schema.json: add "dune-replay" to source enum
- parser.rs: fix doc comment re kind inference (defaults, not inferred)
- live_tests.rs: fix log placeholder (raw_count vs classified.len())
- intent.rs: expand kind comment to match SCHEMA.md

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(portfolio): escape remaining innerHTML fields, add tests, WASM build

- Escape delta_vs_last_run_usd and next_mission_run in widget innerHTML
- Add fixture test with amount != value_usd (stETH: 3.5 tokens / $12250)
  to verify the review fix separating amount from value_usd
- Add empty-legs test for bundling.rs order_legs
- Add comment explaining multi-leg empty value_usd tolerance in bounded.rs
- WASM component builds successfully (754K release binary)
  via: cargo component build --release --target wasm32-wasip2

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(portfolio): address second-round PR review comments

- Tighten share image validation to data:image/png only (was data:image/*)
- Add ClipboardItem existence check to prevent runtime errors in some browsers
- Fix SCHEMA.md to correctly attribute invariant enforcement (bounded.rs vs bundling.rs)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(portfolio): NEAR support end-to-end with engine v2 quality fixes

Add full NEAR Protocol support to the portfolio tool: scan via FastNEAR +
Intear, classify positions through new protocols (Linear, Meta Pool, Rhea
lending, Rhea LP), match against new NEAR-specific yield strategies, and
build intent bundles. Plus assorted infrastructure fixes uncovered while
exercising the v2 / CodeAct path.

Indexer
- New `near` source: FastNEAR `/v1/account/{id}/full` + Intear
  `/list-token-price` (235 KB, vs `/tokens` at 3.2 MB which exceeded fuel).
- New `near-replay` source for offline fixture replay.
- `auto` source dispatches per address: `0x...` → Dune, `*.near`/`*.tg` →
  NEAR backend. Mixed lists are split and merged.
- `classify_near_token()` tags known NEAR DeFi contracts (Linear, Meta
  Pool, Rhea/Burrow, Rhea/Ref) with proper `protocol_id`. Default for
  unknown FT contracts is `wallet`.
- Dust filter raised from \$0.01 → \$1 to keep wallets like `root.near`
  from passing 100+ micro-cap positions through the analyzer.
- Dune `value_usd` now accepts both string and number (Dune started
  returning floats).

Analyzer
- New protocols: `wallet`, `near-staking`, `linear`, `meta-pool`,
  `rhea-lending`, `rhea-lp`. Wallet positions are no longer silently
  dropped (the prior bug that made root.near show "meteor-private" only).

Strategies
- New `near-staking-yield`, `near-lending-yield`, `near-lp-yield` —
  match wallet/staking/LP positions on `chain == "near"`.
- `StrategyAppliesTo` gains `chains` and `tokens` filters.

Tool API
- `propose.strategies` is now optional → falls back to bundled defaults
  (3 EVM + 3 NEAR strategies).
- `propose.config` is now optional → falls back to `ProjectConfig::default()`.
- `build_intent.config` optional with default.
- `propose` recovers from stringified positions (common LLM mistake of
  calling `json.dumps()` first) and returns a clearer error message.
- Capability `dune_api_key` marked `optional: true` — NEAR-only and
  fixture flows no longer block on a missing Dune key.
- Default source is now `auto`.

WASM runtime
- Default fuel limit raised 10M → 500M across config, settings, channel
  runtime, and ResourceLimits. Production was using 10M (config path)
  while tests used `ResourceLimits::DEFAULT_FUEL_LIMIT` (was 100M) — the
  divergence masked the real fuel exhaustion. The 235 KB Intear parse
  uses ~27M fuel, so 500M provides ample headroom.
- Wrapper now logs fuel consumption at debug level for diagnostics.

Engine v2 / CodeAct UX
- Preamble: 3 new rules
  - Never reconstruct tool results manually — reference variables.
  - Never paste Python code outside `\`\`\`repl` or `FINAL(answer)`.
  - Chain tool calls in a single block.
  - Pass native Python objects to tools, never `json.dumps()` first.
- Postamble: explicit good/bad chaining example + `FINAL()` answer
  quality guidance (no terse counts).
- Orchestrator: when an action result exceeds 500 chars, the truncated
  preview now tells the LLM the full result is in `state['<tool>']`
  to discourage manual reconstruction.

Skill (`skills/portfolio/SKILL.md`)
- Step 4 (Propose): explicit anti-patterns for fabricated positions,
  strategy-name-only strings, and `floor_apy` percentage integers.
- Step 5 (Rank): allows informational LLM-only suggestions when
  `propose` returns no `ready` proposals.
- Step 6 (Build intents): explicit skip when no `ready` proposals;
  documents required `plan` shape (`legs`, `expected_out`,
  `expected_cost_usd`, `proposal_id`).
- Step 8 (Summarize): require detailed Markdown output, not counts.

Tests
- `tests/e2e_wasm_portfolio.rs` (5 tests): scan, propose, full pipeline
  via `TestRigBuilder` with canned HTTP — exercises real wasmtime sandbox
  with fuel metering.
- `tests/e2e_live_portfolio.rs` (2 tests, live-only via `IRONCLAW_LIVE_TEST=1`):
  end-to-end via `LiveTestHarness` against real LLM + real FastNEAR/Intear,
  with `engine_v2(true)`. Requires `--test-threads=1` due to a v2
  thread-registry race.
- Portfolio unit tests: 183 pass (added NEAR indexer parsers, dispatch
  auto-detection, new strategy filter cases).
- Live portfolio tests: 10 pass against real APIs.
- Updated `hostile/fake-token-dust` scenario for the new "wallet"
  protocol behaviour.

Bug fixes uncovered along the way
- `intents/bounded.rs`: epsilon raised to 0.005 to tolerate the 2-decimal
  truncation in `intents/fixture.rs` (intent bundles previously failed
  the slippage check on synthetic targets).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(portfolio): address review findings from #2368

Correctness:
- bounded.rs: multi-leg slippage now checks the terminal leg (matching
  plan.expected_out.chain), not just single-leg bundles. Regression
  tests added for the bypass and for a multi-leg bundle with min_out=0
  on the terminal leg.
- bounded.rs: reject zero/negative/NaN/infinite expected_out (would
  make min_required = 0 and every leg pass vacuously).
- indexer/mod.rs: is_near_address now validates NEAR account rules
  (2..64 chars, lowercase, separators). Previously any non-0x string
  (empty, whitespace, emoji, SQL injection) passed.
- indexer/mod.rs: scan_auto rejects addresses that are neither valid
  EVM nor valid NEAR, instead of silently routing them to Dune.

Code quality:
- bundling.rs: replace .expect("indegree") and .expect("leg by id")
  with explicit error returns.
- fixture.rs: replace .unwrap() on plan.legs.last() with an Err path.
- types/mod.rs: pub use → pub(crate) use (crate-internal only).
- dune.rs / near.rs: warn (via host::log at Warn level) when a
  non-zero amount has a missing/zero value_usd, so silent undercounts
  surface in diagnostics rather than being invisible.

Security:
- gateway config.js: hoist the data:image/png prefix check to the top
  of IronClaw.api.share() so both img.src and a.href are gated.
- gateway config.js: add noopener,noreferrer to window.open features
  on share popups to close reverse-tabnabbing surface.
- widget/index.js: extend escapeXml to also escape apostrophes.

Infrastructure:
- limits.rs: TODO comment noting that 500M fuel default is driven by
  one tool (portfolio/near) and follow-up should add a per-tool
  override so the global default can stay tighter.
- test_portfolio.py: silent-return on missing widget tab converted to
  pytest.skip via shared _open_portfolio_tab_or_skip helper, so a
  regression that removes widget registration fails loudly instead of
  passing silently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(portfolio): address follow-up review comments

- lib.rs: BuildIntent.solver now defaults to "fixture" (a valid
  value), not "auto" (unrecognized by intents::build — was shipping
  the default straight into an "Unknown intent solver: 'auto'" error
  whenever the caller omitted the field).
- capabilities.json: update discovery_summary to reflect that
  strategies/config on propose and config/solver on build_intent are
  optional. Stale text had propose requiring both positions and
  strategies.
- limits.rs + config/wasm.rs: fix the fuel-limit doc comments. The
  prior value in limits.rs was 100M (not 10M — that was the config
  path). Clarify both paths converged at 500M in #2368.
- config.js (share modal): add aria-label, aria-modal, role=dialog,
  aria-labelledby for the modal and explicit aria-label on every
  icon-only share button. Mark decorative SVGs aria-hidden. Toast
  becomes role=status with aria-live=polite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-20 14:47:51 +09:00
Illia Polosukhin
3c7925c100 refactor(gateway): delete server.rs shim + relocate tests to slices — ironclaw#2599 stage 6 (#2706) 
Finishes the feature-slice migration started in stage 4a. After this:

- `src/channels/web/server.rs` no longer exists.
- Every caller of `crate::channels::web::server::*` now points at
  `platform::router::start_server` or `platform::state::*` directly.
- All ~60 caller-level tests that used to live in `server.rs::tests`
  now live inside the feature slice they actually exercise, next to
  the handler they test.

## What moved where

Classification driven by the handler each test drives:

| Slice | Tests |
|---|---|
| `features/chat/mod.rs::tests` | 3 × history, 4 × auth-token/cancel + gate-resolve, 1 × approval, 3 × pending-gate-extension-name, 1 × test_auth_manager helper |
| `features/pairing/mod.rs::tests` | 1 × list, 5 × approve (claim / no-followup / with-thread / external-callback / blank-code), `make_pairing_test_state` helper |
| `features/extensions/mod.rs::tests` | 2 × activation classifier, 2 × path-traversal guards, 1 × setup-submit-not-activated, 2 × list-inactive-wasm-channel, 1 × phase-precedence, 1 × readiness handler, 2 × apply_extension_readiness |
| `features/oauth/mod.rs::tests` | 13 × oauth callback (missing params / unknown state / expired × 2 / no-ext-mgr / strip-prefix / versioned × 2 / happy × 3 / exchange-fail), 5 × relay oauth callback, + `TestOauthProxy`, `EnvVarGuard`, `set_env_var`, `fresh_pending_oauth_flow`, `expired_flow_created_at`, `test_oauth_router`, `test_relay_oauth_router` helpers |
| `platform/static_files.rs::tests` | 3 × CSP header / base / nonce, 2 × css etag, 1 × css handler, 2 × css multi-tenant, 4 × stamp nonce + build frontend HTML, 1 × test_build_frontend_html_returns_none_in_multi_tenant_mode |
| `platform/state.rs::tests` | 1 × workspace_pool_resolve_seeds_new_user_workspace |
| `handlers/llm.rs::tests` | 3 × llm admin-role guards |
| `handlers/users.rs::tests` | 1 × delete_user_evicts_auth_and_pairing_caches |

## Cross-slice test fixtures

Four helpers that multiple slices share (`insert_test_user`,
`test_secrets_store`, `test_ext_mgr`, `test_ext_mgr_with_db`) moved
into `src/channels/web/test_helpers.rs` as `#[cfg(test)] pub(crate)`
free functions, following the pattern from stage 6a (#2704) for
`test_gateway_state*`. All four keep the exact signatures they had in
`server.rs::tests`, so the move was mechanical. Rust expect suppressions
on the five `.expect(...)` lines inside these fixtures carry
`// safety: cfg(test) fixture` comments — the pre-commit safety check
is diff-line based and doesn't look up whether the containing function
is already `cfg(test)`-gated.

## Mechanical renames (25 files)

`channels::web::server::<item>` call sites now import from:
- `platform::router::start_server`
- `platform::state::{GatewayState, RateLimiter, PerUserRateLimiter,
  WorkspacePool, FrontendCacheKey, FrontendHtmlCache,
  ActiveConfigSnapshot, PromptQueue, RoutineEngineSlot,
  rate_limit_key_from_headers}`

Covers `src/main.rs`, `src/app.rs`, `src/tools/builtin/{job,memory}.rs`,
all 13 handlers in `handlers/*.rs`, the four integration tests
(`ws_gateway_integration`, `openai_compat_integration`,
`multi_tenant_integration`, `oauth_greeting_integration`), plus
`tests/support/gateway_workflow_harness.rs` and
`src/channels/web/tests/multi_tenant.rs`. No behavior change.

## Boundary checker retained

`scripts/check_gateway_boundaries.py` still rejects any
`crate::channels::web::server::` path as a defense-in-depth guard
against accidental re-introduction (literal new `server.rs`, stray
imports, etc.). The explanatory comment and the regression test's
docstring now reflect "shim is gone; this guard prevents re-creation"
instead of "shim exists; don't route through it."

## Documentation updates

- `src/channels/web/CLAUDE.md`: deleted the `server.rs` File Map row,
  updated the `test_helpers.rs` row to list all seven `pub(crate)`
  fixtures (stages 6a + 6 together), fixed all prose references that
  pointed at `server.rs`, and updated the "Adding a New API Endpoint"
  recipe to point at `features/<slice>/` and `platform/router.rs`.
- `src/channels/web/platform/state.rs`: module docstring now says
  "shim was removed" instead of "shim exists pending migration."
- `src/bridge/CLAUDE.md`: `pending_gate_extension_name` reference now
  points at `features/chat/mod.rs`.

## Quality gate

- [x] `cargo fmt --all`
- [x] `cargo clippy --all --benches --tests --examples --all-features` — zero warnings
- [x] `cargo check -p ironclaw --no-default-features --features libsql --tests` — clean
- [x] `cargo test -p ironclaw --lib channels::web` — 434 passed (up from 431 — three tests that were incorrectly filtered under `channels::web::server::tests` now surface under their proper slice's module path)
- [x] `cargo test -p ironclaw --test multi_tenant_integration` — 40 passed
- [x] `cargo test -p ironclaw --test openai_compat_integration` — 16 passed
- [x] `cargo test -p ironclaw --test ws_gateway_integration` — 11 passed
- [x] `python3 scripts/check_gateway_boundaries.py` — clean
- [x] `python3 scripts/check_gateway_boundaries.py test` — 16/16
- [x] `bash scripts/pre-commit-safety.sh` — clean

## Regression coverage

Pure relocation + mechanical rename; no behavior change. The existing
~60 tests from `server.rs::tests` continue to pass unmodified, which is
the regression evidence. A "test that would have caught this" would
necessarily duplicate the existing tests — no new test adds coverage.
[skip-regression-check]

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-20 14:20:40 +09:00
jinxin
d30c76de69 feat: add debug inspector panel for web gateway (#1873) 
* feat(web): add debug inspector panel for web gateway chat UI (#1493)

Add a debug inspector sidebar with three tabs (Prompt, Activity, Stats)
activated via ?debug=true URL parameter. Consolidate theme-init.js into
a new init.js for early initialization. The panel shows real-time SSE
event timeline, system prompt component breakdown with token estimates,
and session-wide statistics including per-model usage.

- New files: init.js, debug-panel.js, debug-panel.css
- New endpoint: /api/debug/debug/prompt for system prompt inspection
- i18n support (en + zh-CN) for all debug panel strings
- Responsive layout: sidebar on desktop, overlay on tablet, hidden on mobile

* chore: minor

* feat(web): add debug inspection endpoints and verbose SSE mode (#1492)

Add per-subscriber verbose filtering to SSE, new AppEvent variants
(ToolResultFull, TurnMetrics), and enhanced debug prompt endpoint.
Debug subscribers (?debug=true) receive full tool output, per-LLM-call
metrics with model/duration/cache tokens, and tool parameters on
success. Non-debug subscribers see no change (backward compatible).

- New AppEvent variants: tool_result_full, turn_metrics with is_verbose_only()
- SseManager.subscribe()/subscribe_raw() accept verbose flag
- Emit TurnMetrics from dispatcher after each LLM call
- Emit ToolResultFull with 50KB cap after tool execution
- tool_completed() always includes redacted parameters
- /api/debug/prompt returns system_prompt, model, context_limit
- Frontend: turn-based activity tracking, turn navigation, message click
- Frontend: prompt tab with model name, progress bar, full prompt view
- Unit test for verbose SSE filtering

* fix(debug-panel): start turn counter at 0 so first message shows turn 1

* chore: minor

* chore: minor

* chore: fix lint

* fix(i18n): add Korean debug panel translations and fix hardcoded string

* fix(i18n): add Korean debug panel translations and fix hardcoded string

* fix: fix lint

* fix(web): propagate call_id to SSE events, gate debug mode on admin role, and skip verbose broadcasts without subscribers

- Add call_id field to AppEvent::ToolStarted/ToolCompleted/ToolResult and
  propagate from StatusUpdate conversion instead of silently dropping it,
  fixing mismatched tool start/complete pairs during concurrent same-name
  tool calls in the debug panel
- Update debug-panel.js to key pending tools by call_id (flat map) instead
  of FIFO name-based queues
- Skip ToolResultFull/TurnMetrics allocation and broadcast when no
  SSE/WebSocket subscribers are connected (SseManager::has_receivers)
- Require admin role for verbose/debug SSE and WebSocket event streams,
  matching the existing AdminUser gate on /api/debug/prompt
- Add audit log (tracing::debug) on debug prompt endpoint access

* fix(gateway): add admin gate to WS debug mode, add call_id to ToolResultFull, fix debug panel i18n

- Require admin role for WebSocket debug mode (server.rs), matching the
  existing SSE handler check — prevents non-admin users from receiving
  verbose tool output via ?debug=true
- Add call_id: Option<String> to StatusUpdate::ToolResultFull and
  AppEvent::ToolResultFull for correct concurrent same-name tool
  matching; update dispatcher, web gateway conversion, and debug-panel.js
- Remove dead chat_ws_handler from handlers/chat.rs (superseded by
  server.rs local version)
- Fix debug panel overlay blocking page on viewport resize by switching
  from inline style to CSS class toggle with transparent background
- Internationalize hardcoded English strings in debug panel (In/Out/
  Cost/Model/Cache labels) with en/ko/zh-CN translations
- Fix activity entries not updating on language switch: store labelKey,
  resolve during render without mutating entry, rebuild activity DOM
  in refreshDynamicI18n
- Fix pre-existing subscribe_raw() test compilation errors (missing
  verbose parameter)

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-20 13:02:37 +09:00
firat.sertgoz
141435eb0b feat(gateway): expose engine v2 threads in chat history and sidebar (#2532) 
* feat(gateway): expose engine v2 threads in chat history and sidebar

Engine v2 threads weren't appearing in the gateway sidebar and
deep-linking to one by id (`#/chat/<engine-thread-id>`) returned an
empty history because the v1 `assistant` flow dual-writes into the
single assistant conversation id, not the engine thread id.

Three coordinated fixes:

- `chat_history_handler`: extend the ownership check with an engine v2
  lookup so an engine thread id is recognized, then fall back to
  loading messages via `bridge::get_engine_thread` when the v1
  conversation table has nothing.
- `chat_threads_handler`: merge engine threads from
  `bridge::list_engine_threads` into the sidebar, label them with
  their goal, and re-sort by `updated_at`. Bump the v1 conversation
  cap from 50 to 500 so older threads stop silently aging off the
  sidebar.
- Gateway frontend (`app.js`): when restoring from `#/chat/<id>` on
  load, switch even if the id is not in the loaded sidebar list — the
  history endpoint resolves it via the DB. Log a warning instead of
  silently dropping the URL.

Cherry-picked from 8df22ab2 (feat/skills-engine-fixes), adapted to
staging where chat_history_handler and chat_threads_handler still live
in both server.rs and handlers/chat.rs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gateway): address v2 history review comments

* test(gateway): caller-level coverage for v2 thread ownership + history

Adds tests exercising chat_history_handler through the three ownership
branches the PR introduces, and two HTTP-driven e2e scenarios against
an ENGINE_V2=true server. Fills the caller-level coverage gap flagged
in review.

- Rust caller-level tests on chat_history_handler:
  - v2-owned: engine thread owned by user returns synthesized history
  - cross-user: alice can't read bob's engine thread (404)
  - session-only: in-memory session-owned thread returns 200 without DB
- Playwright e2e under ENGINE_V2=true:
  - engine-only thread appears in sidebar with channel=engine
  - deep-link by engine thread id returns synthesized turns

Exposes a minimal bridge::test_support module (ThreadTestStore,
install_engine_state_with_threads, clear_engine_state, shared test
lock) so cross-module tests can seed ENGINE_STATE without the weight
of the bridge's own full-featured TestStore.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ci): align with staging EngineState fields and clippy rules

- `EngineState` on staging now has `extension_manager` and `project_root`
  fields (added in #2549 and the attachments flow). Update the test-only
  `install_engine_state_with_threads` helper to populate them.
- Rewrite the `let Some(...) else { return None }` in
  `engine_history_entry_to_message` as a `?` — clippy::question_mark is
  denied on staging's all-features CI.
- Add missing `AuthenticatedUser` + `Query` imports to the server.rs
  test module for the `history_request` helper.
- `cargo fmt` on the 500-error test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-20 12:57:27 +09:00
Illia Polosukhin
fdaba100a6 feat(gateway): add attachment flows, v2 skill install coverage, and e2e stabilization (#2385) 
* feat(gateway): add attachment flows and slash-skill coverage

* feat(v2): persist project attachments across channels

* feat(skills): install GitHub skill bundles

* feat(v2): cover live skill install and setup flow

* test(e2e): stabilize gateway and auth coverage

* test(e2e): stabilize post-merge warnings and browser flows

* fix(review): address follow-up PR feedback

* fix(review): address remaining attachment and skill install comments

* Address remaining attachment review comments

* fix(ci): allowlist ws.rs → server::inline_attachments_to_incoming

ws.rs was already allowlisted for the attachment shim symbols
(`images_to_attachments`, the rate limiter types, etc.) so the new
unified entrypoint added by this branch (combining images and
generic attachments before validation) follows the same pattern.
The entry will be removed together with the rest of the ws.rs
server:: block once the attachment helpers migrate into platform/.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): attachment persistence path and Slack activate signature

Two e2e-surfacing regressions after merging staging:

1. `persist_project_attachments` was writing to
   `<base_dir>/projects/.ironclaw/attachments/...` because PR #2385's
   reviewer-requested switch from `std::env::current_dir()` to an
   explicit `project_root` kept the `.ironclaw/` prefix baked into
   `PROJECT_ATTACHMENT_DIR` while rooting at `ironclaw_base_dir()/projects`.
   Point `resolve_project_root()` at the parent of the base dir so
   `<parent>/.ironclaw/attachments/<owner>/<project>/...` matches the
   prompt's `project_path` and the user's expectation when base dir is
   `~/.ironclaw`. Updates the corresponding assertion in
   test_v2_engine_auth_flow.py to resolve paths against the fixture's
   home tempdir instead of the repo root.

2. `activate_slack()` grew a required `http_url` arg during the
   skill-install branch work but the `active_slack` fixture in
   test_slack_e2e.py still passed the old three-arg shape. That tripped
   every Slack scenario at setup (TypeError). Thread `http_url` from
   `slack_e2e_server` through the fixture.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(engine-v2): auth-prompt surfacing, bundle_path injection, attachment-only inputs

- Orchestrator formatter now writes `Installed bundle path on disk:` into
  each skill block so the skill body sees the bundle location it needs to
  reference (e.g. running `pip install -r <bundle>/requirements.txt`).
  Previously the bundle_path metadata field was populated but never
  surfaced into the prompt, so skills that rely on filesystem paths
  silently no-op'd.
- The router no longer rejects messages whose text body is empty when
  the payload carries attachments. Safety validation's empty-input
  guard is a v1 input-sanity check; a pure-attachment follow-up (image
  upload with no caption) is a legitimate submission in the v2 gateway
  contract and previously tripped "Input cannot be empty".
- The engine auth-flow e2e tests now detect gate-paused state via
  `HistoryResponse.pending_gate` (and `resume_kind.Authentication`)
  rather than scanning the turn response text for "paste your token".
  Auth instructions live in the `onboarding_state` SSE event, not in
  the chat response (see `test_auth_no_duplicate_response.py`); the old
  string-matching assertion was checking the wrong surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): switch approval/auth-prompt probes to pending_gate

Approval and auth prompts are surfaced through HistoryResponse.pending_gate
and the onboarding_state/gate_required SSE events, not as text in
turns[-1].response — the duplicate-response regression guard in
test_auth_no_duplicate_response.py explicitly forbids them from appearing
in the chat transcript.

Update the helpers in test_v2_engine_approval_flow.py,
test_v2_engine_auth_cancel.py, and test_v2_kernel_auth_preflight.py to
poll pending_gate instead of scanning turn text for "requires approval"
or "paste your token". Unblocks 5 approval, 1 auth-cancel, and 3
preflight tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): google-oauth _wait_for_auth_prompt / _wait_for_response use pending_gate

Bring the Google Drive / skill-OAuth regression file in line with the rest
of the v2 e2e helpers: poll `HistoryResponse.pending_gate` for auth/approval
prompts, and accept a pending_gate as a valid terminal state for
`_wait_for_response` (an auth-retry chain that hits another gate is still
progress, not a hang).

Unblocks the oauth-cancel, invalid-token-paste, and api-key-then-api-call
scenarios; the lingering token-refresh scenario still exposes a real v2
auto-refresh regression (the engine prompts the user instead of issuing a
refresh against the stored refresh_token).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): relax a few stale v2-surface assertions

- `test_skill_oauth_flow::test_auth_required_sse_event` was pinned to the
  old `onboarding_state/auth_required` SSE payload. The v2 gate pipeline
  delivers credential gates as `gate_required` (resume_kind
  `Authentication`) or, when preflight falls through to approval first,
  `approval_needed`. Accept any of those three, and treat a `thinking`
  "Running <tool>" status as evidence the tool call fired when no
  standalone `tool_started` event is emitted.
- `test_message_persistence` helpers asserted HTTP 200 on `/api/chat/send`,
  but the gateway now returns 202 ACCEPTED (fire-and-forget). Accept both.
- `test_project_detail` flipped the wrong global (`engineV2`) instead of
  `engineV2Enabled`, leaving the `data-v2-only` Projects tab hidden so the
  click timed out. Set the real flag.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(review): address attachment index note correctness

Two Copilot review findings on the attachment persistence path:

- `attachment_index_note` in `src/bridge/router.rs` used the raw
  user-supplied filename in the markdown `# Uploaded attachment:` header
  and in the memory-doc `title` field. A filename with newlines /
  backticks / control characters would corrupt the agent-visible
  transcript and break searchable titles. Route the filename through a
  new `sanitize_filename_for_display` that strips control chars,
  collapses newlines/tabs to spaces, swaps backticks for apostrophes,
  truncates at 256 chars, and falls back to `"attachment"` when the
  sanitized result is empty.
- `persist_project_attachments` cleared `attachment.data` before
  calling `attachment_index_note`, so the `size_bytes.unwrap_or(
  data.len() as u64)` fallback reported `0` bytes whenever the channel
  hadn't pre-populated `size_bytes`. Swap the order — build the index
  note while the buffer is still populated, then drop the bytes.

Also adjust `src/agent/attachments.rs::format_attachment` for the
Image arm: when `data` has been cleared but `local_path` is set (the
engine-v2 persist-then-clear flow), the "visual content not available
in this conversation" message is misleading — the image is available,
just on disk. Surface a dedicated prompt that tells the agent to
reference the project file path instead of trying to load bytes from
memory.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): cancel_during_auth asserts pending_gate clears, not chat text

The test polled \`turns[-1].response\` for "cancel" but the cancel flow
never writes an assistant row to the chat-history DB: resolve_gate
returns \`BridgeOutcome::Respond("Cancelled.")\` which broadcasts via
SSE and calls \`stop_thread\` on the engine thread, neither of which
goes through the DB persistence path that populates turn responses.

Switch the test to verify the user-visible signal the gateway actually
emits — \`history.pending_gate\` disappears after "cancel" resolves the
gate. Matches the approach used in \`test_v2_engine_approval_flow.py\`'s
deny-flow tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): pairing approve test tolerates ExtensionName boundary reject

Staging's new `features/pairing/` slice (ironclaw#2599 stage 4b) validates
the `{channel}` URL segment through `ExtensionName::new` at the handler
boundary: a path-traversal / control-character / whitespace-containing
segment (like `evil.Ignore all`) now returns 400 instead of silently
routing to a pairing-store miss.

The regression test used to assert the older 200+JSON shape. Relax it
to accept either 200 (generic `Invalid or expired pairing code.`) or 400
(boundary validation); the real invariant the test exists to protect —
the raw injection-shaped channel string must not echo back into the
response — is still asserted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(review): preserve image bytes through LLM call + document drive mock pin

Two review findings:

- `src/bridge/router.rs::persist_project_attachments` was clearing
  `attachment.data` after writing the file to disk. The very next step
  in `handle_with_engine_inner` is `augment_with_attachments`, which
  only emits a multimodal `image_parts` entry when `att.data` is
  non-empty — so every engine-v2 image upload was silently dropped
  from the LLM request even though the file landed on disk. The
  `persisted_attachments` Vec is local to the dispatch and is dropped
  as soon as the engine call returns, so the "storage hygiene" comment
  the clear used to justify was a no-op. Stop clearing; let RAII free
  the bytes. Updates `src/agent/attachments.rs`'s Image-arm prompt to
  reflect the refined invariant (`data.is_empty()` now implies a
  downstream caller or channel stripped the buffer, not the normal
  persist path).

- `tests/e2e/scenarios/test_v2_engine_oauth_google.py::_pin_mock_drive_api_url`
  posts to `/__mock/set_github_api_url`. The wire name is historical
  — the Drive suite reused the knob — but the fixture name made the
  intent hard to follow. Adds a docstring that calls out the shared
  `_github_api_url` in `mock_llm.py` and explains why the endpoint
  rename would cascade into every other test that uses it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(review): address remaining Copilot feedback on PR 2385

- audio attachments: include `mime` (and size) attribute in `<attachment>`
  XML for parity with image/document so the frontend can render MIME and
  size in attachment cards
- /api/skills list/search: parallelize per-skill filesystem I/O
  (`read_install_metadata`, `try_exists`, `metadata`) via
  `futures::future::join_all` instead of awaiting serially — keeps the
  handler O(n) in wall time for large skill sets
- history parseUserMessageContent: only strip the trailing
  `<attachments>…</attachments>` block when at least one `<attachment>`
  tag is parsed from inside it, otherwise leave the raw text intact so
  user messages that legitimately end with that markup are preserved
- sync_v1_skill_to_store: look up existing shared skill doc via
  `list_skills_global()` instead of `list_shared_memory_docs(project_id)`
  so shared skills installed under one project are updated in place when
  re-synced from another project (prevents duplicate shared docs across
  per-user projects) and preserve the original `project_id` on in-place
  update

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-20 12:38:30 +09:00
Illia Polosukhin
fb4fc829e1 refactor(ownership): collapse OwnerId+Identity into UserId with role variants (#2677) 
* refactor(ownership): collapse OwnerId+Identity into UserId with role variants

- Expand UserRole to {Owner, Admin, Regular}
- UserId carries role; methods is_owner()/is_admin()/is_regular()
- Remove From<String>/From<&str> impls (enforces types.md rule)
- Validated construction via new(); from_trusted() for DB-sourced values

Addresses bug pattern from #2561, #2620, #2349 where owner_id silently
round-tripped as String.

* refactor(ownership): address review feedback — id-only equality, persist owner role, doc fixes

- UserId PartialEq/Eq/Hash now compare only `id`, not `role`. Role is
  metadata that travels with the identity; two UserIds with the same id
  but different roles must be interchangeable as HashMap/HashSet keys
  and cache lookup targets. Added a regression test that builds a
  HashSet keyed on UserId and asserts cross-role `.contains()`
  membership, plus a hash-equality check.
- CLI pairing path now persists the "owner" role string (via
  UserRole::Owner.as_db_role()) instead of the hardcoded "admin", so
  a reload through UserRole::from_db_role stays Owner rather than
  being silently downgraded to Admin.
- Update the feature/pairing approve handler to mirror the refactor:
  build UserId via from_trusted + UserRole::from_db_role(&user.role)
  instead of the removed OwnerId::from.
- AdminScope doc comment now reflects that Owner also passes
  is_admin().
- AdminUser extractor error message now reads "Admin privileges
  required (admin or owner)" so the forbidden response matches the
  actual gate.

---------

Co-authored-by: Henry Park <henrypark133@gmail.com>
 2026-04-20 12:31:37 +09:00
Henry Park
64193474dd Preserve paused leases across engine auth resume (#2631) 
* Preserve paused leases across engine auth resume

* fix(review): validate paused_lease snapshot at gate resume

Addresses PR #2631 review comments from Copilot:

1. **Snapshot used without validation** (src/bridge/router.rs:684 orig):
   `pending.paused_lease.clone()` was used directly to resume a gated
   action. A gate can sit in the pending-gate store for hours or across
   process restarts; during that window the original lease may have
   been revoked, expired, or the pending record could have drifted off
   its original thread.

   Extract `snapshot_lease_still_valid` + `resume_lease_for_pending_gate`
   helpers. The snapshot must pass four checks before use:
     - `lease.thread_id == pending.thread_id`
     - `granted_actions.covers(&pending.action_name)`
     - `!revoked`
     - `expires_at` is unset or in the future

   If any check fails, fall through to `LeaseManager::find_lease_for_action`
   (the normal path). Matches the reviewer's suggestion to avoid silently
   resuming a stale snapshot; still prefers the snapshot when valid so
   the original bug (no active lease at resume after restart) stays
   fixed.

2. **No router-level regression test** for the snapshot-vs-fallback
   decision. Six new libsql-free tests in `bridge::router::tests`:
     - `resume_lease_prefers_snapshot_even_when_lease_manager_empty` —
       reproduces the original bug; snapshot must carry the resume.
     - `resume_lease_rejects_revoked_snapshot_and_falls_back`
     - `resume_lease_rejects_expired_snapshot_and_falls_back`
     - `resume_lease_rejects_snapshot_with_wrong_thread_id`
     - `resume_lease_rejects_snapshot_missing_action_coverage`
     - `resume_lease_returns_none_when_no_snapshot_and_no_active_lease`

Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples
--all-features` (0 warnings), `cargo test -p ironclaw_engine` (435
passed), `cargo test -p ironclaw --lib` (5182 passed, +6 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style: collapse assert!(matches!()) per rustc 1.95 rustfmt

CI rustfmt (nightly/stable 1.95.0) wants the `assert!(matches!())` in
`orchestrator.rs::parse_outcome_gate_paused` collapsed to fewer lines.
Local rustfmt 1.94 was happy with the expanded form; matching CI to
unblock the fmt check.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-20 01:45:49 +09:00
Illia Polosukhin
cad5e50f10 feat(llm): hot-reload provider chain from settings (supersedes #2059) (#2673) 
* feat(llm): hot-reload provider chain from settings (#1350)

Adds SwappableLlmProvider and LlmReloadHandle so changes to the active
LLM backend/model via the settings API take effect without restarting
the daemon. The settings handlers trigger a chain rebuild from the
latest Config::from_db_with_toml whenever an LLM-relevant key is
written, and atomically swap the inner provider under the running
wrappers.

Addresses review feedback on the original PR #2059 (superseded):
- single RwLock<ProviderSnapshot> for atomic metadata updates (no
  torn reads across model_name / cost / cache multipliers)
- interned &'static str for model_name() to cap Box::leak at the set
  of distinct names a process ever sees, not one leak per swap
- single critical section around swap+snapshot refresh to kill the
  race between concurrent reloads
- tokio::sync::Mutex on LlmReloadHandle to serialize reloads and
  avoid overlapping OAuth refreshes / HTTP probes
- warn!, not silent Ok, when reload wiring is missing from the
  gateway state
- integration coverage per .claude/rules/testing.md: a test that
  drives settings_set_handler end-to-end and asserts the same
  Arc<dyn LlmProvider> reports the new active_model_name after swap

Co-authored-by: Nigel Coleman <coleman.nige@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(llm): gate hot-reload on scope + admin; re-hydrate secrets

Addresses review findings on #2673:

- Scope gate: reload only fires when the written scope actually feeds
  the global provider chain (admin scope or gateway owner scope). A
  member writing their own `selected_model` lands in their user row
  but no longer triggers a chain rebuild that would read back from a
  different scope — fixing both the "write ignored by reload" bug and
  the DoS vector where any authed user could force expensive rebuilds.

- Admin-only provider selection: `llm_backend` and `bedrock_{region,
  cross_region, profile}` join the existing admin-only LLM key list,
  matching the product directive "admins choose the provider, members
  pick the model within it". `selected_model` stays non-admin so every
  user can change their own model.

- Secret re-hydration on reload: `reload_llm_after_settings_change`
  now calls `re_resolve_llm_with_secrets` after the bare `from_db_with_toml`
  read, so a new OPENAI_API_KEY / NEARAI_SESSION_TOKEN added alongside
  a backend switch is visible to the rebuilt chain.

- Style cleanup: drop dead `llm_model` allowlist entry; drop unused
  `Clone` on `ProviderSnapshot`; document `reload_lock`'s purpose;
  explicit comment that `active_config.enabled_channels` is not
  refreshed (channel enablement is orthogonal to LLM config).

New regression tests (5149 → 5154 passing):

- `llm_reload_handle_preserves_old_chain_on_build_failure` — a failed
  reload leaves the primary wrapper pointing at the old chain.
- `settings_set_handler_rejects_member_writing_llm_backend` — member
  writing `llm_backend` gets 403 (admin-only).
- `settings_set_handler_member_selected_model_skips_reload` — member
  can set their own model, and it does NOT trigger a global reload.
- `settings_set_handler_owner_scope_triggers_reload` — owner writing
  their own scope (no `scope=admin`) still reloads the chain.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(llm): decouple reload from HTTP status; atomic set_model; cap interner

Addresses PR #2673 review comments from Copilot and gemini-code-assist:

- **Reload failure no longer 500s the setting write** (Copilot):
  `reload_llm_after_settings_change` is now infallible — it logs at
  `error!` when the chain rebuild fails but the handler still returns
  204. Returning 500 after a successful `set_setting` misrepresented
  the outcome (DB committed, chain stale) and drove client retries
  that re-ran the same failing reload.

- **set_model race with swap closed** (gemini): the write lock is now
  held across the inner `set_model` call and the snapshot refresh, so a
  concurrent `swap()` can't clobber the just-updated inner with a
  snapshot of the older one.

- **Interner leak capped** (gemini): `intern_model_name` now refuses
  names longer than 256 bytes and caps distinct entries at 1024, past
  either limit returning a static `<model-name-overflow>` sentinel and
  logging at `warn!`. Protects against adversarial `set_model` input.

New regression tests (5154 passing):

- `settings_set_handler_returns_success_when_reload_fails` — admin
  switches backend to a value with no credentials; handler returns
  204, DB has the new value, old chain still serving.
- `set_model_and_swap_are_mutually_atomic` — concurrent set_model +
  swap stress; final wrapper is readable and consistent.
- `intern_into_rejects_oversized_input` — oversized name never leaks,
  returns sentinel without touching the map.
- `intern_into_caps_distinct_entries` — past the cap, sentinel;
  already-interned names still resolve.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(llm): load reload config from owner scope; rollback on build failure

Addresses PR #2673 review comments from @serrrfirat and Copilot.

- **Reload scope fix** (serrrfirat, Copilot): `reload_llm_after_settings_change`
  now reads with `state.owner_id` instead of the just-written `effective_user_id`.
  `Config::from_db_with_toml` skips the admin-merge step when `user_id == __admin__`,
  so reloading at admin scope was dropping owner-scope overlays that startup
  normally applies. Using `state.owner_id` matches `AppBuilder::init_config`
  and keeps the layering consistent.

- **Rollback on reload failure** (serrrfirat): handlers now snapshot the
  affected keys before the DB write and restore them if the chain rebuild
  returns `ConfigLoadFailed` or `BuildFailed`. The handler then returns
  422 with the rolled-back state. This closes the split-brain window where
  a bad `llm_backend=openai` write could leave the DB saying "openai"
  while the runtime kept serving "nearai". `set_setting`, `delete_setting`,
  and `set_all_settings` all participate.

- **`ReloadOutcome` enum** replaces the previous infallible return, so
  callers can distinguish transient "nothing wired" (skip) from actual
  "chain rebuild failed" (roll back) outcomes.

Regression tests (5184 passing):

- `reload_rebuilds_from_owner_scope_not_effective_scope` — pre-seeds an
  owner-scope `selected_model` overlay and has admin write a benign
  key under `scope=admin`. Assertion: after reload, the wrapper reports
  the owner's overlay, not admin's default. This fails pre-fix because
  reading at `__admin__` scope silently skipped the admin merge.
- `settings_set_handler_rolls_back_on_reload_failure` — pokes a poisoned
  `bedrock_cross_region` sibling into admin scope, triggers a handler
  write, asserts 422 and that the DB is back to its pre-request state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(llm): fail-loud on snapshot read errors; surface 422 reason in body

Addresses Copilot review comments on PR #2673.

- **Snapshot read errors** (c5, c6): `settings_{set,delete,import}_handler`
  used to `.unwrap_or(None)` when reading the previous value for rollback,
  which would silently treat a DB read failure as "no prior value" and
  turn a later rollback into `delete_setting` on a key whose prior value
  we couldn't actually read — a silent data-loss path. The handlers now
  map snapshot read errors to 500 and abort before persisting. The import
  handler does the same inside its per-key snapshot loop.

- **422 body carries the reason** (c7): the `ReloadOutcome::BuildFailed`
  and `ConfigLoadFailed` reason strings are now included in the 422
  response body. Handler error type changed from `Result<StatusCode,
  StatusCode>` to `Result<StatusCode, (StatusCode, String)>` (axum's
  `IntoResponse` for tuples). The web UI's `apiFetch` can surface the
  reason to the operator instead of a bare "Unprocessable Entity".
  Auth/validation paths keep empty-body semantics via the `no_body`
  helper.

Test updates:
- Existing handler tests: `.0` on the error tuple where they previously
  compared bare `StatusCode`.
- Extended `settings_set_handler_rolls_back_on_reload_failure` to assert
  the 422 body includes the failure reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Nigel Coleman <coleman.nige@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-20 00:18:38 +09:00
firat.sertgoz
e8ae9487bb fix(telegram): unblock e2e activation flow (#2652) 
* fix(telegram): unblock e2e activation flow

* fix: address review findings (iteration 1)

* test: isolate telegram e2e activation state
 2026-04-20 00:03:24 +09:00
standardtoaster
81aec813e1 fix(gateway): v2 engine tool_calls persistence + e2e test coverage (#2452) 
* test(e2e): add v2 engine tool execution lifecycle tests

The v2 engine had zero e2e coverage for the tool call -> result ->
response path. This gap was flagged in the #2193 audit and is the
same code path that breaks in QA bug #2402 (infinite loop after
tool operations).

New test file: test_v2_engine_tool_lifecycle.py
- Single tool call (echo, time) completes through v2
- Text-only message completes through v2
- Parallel tool calls (2 tools in one response)
- Multi-step chain (echo -> result -> time -> result -> completion)
- Multi-turn tool usage across conversation turns

Mock LLM additions:
- "parallel echo and time" trigger for multi-call responses
- "multi step echo then time" trigger for sequential chains

Also documents that v2 engine does not populate the tool_calls
array in chat history (tool names show as "unknown"). This is a
separate gap from execution correctness.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gateway): persist v2 engine tool_calls to chat history

The v2 engine executed tools correctly but never wrote a
`role="tool_calls"` message to the v1 conversation DB. This
meant the chat history API returned `tool_calls: []` for all
v2 threads, breaking the web UI's tool call display.

Fix: after thread completion, extract ActionExecuted/ActionFailed
events from the v2 event log and write them as a tool_calls DB
row before the assistant response. The v1 history API now shows
tool names, results, and errors for v2 engine threads.

Steps are evicted from the in-memory store after join_thread,
so this reads from the append-only event log instead.

E2E test updated to assert tool_calls are populated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: use thread internal_messages for tool_calls persistence

The events approach used params_summary (input parameters) where
result_preview (output) was expected. Thread internal_messages
carry the actual tool output in ActionResult messages.

Also fixes stale test file docstring that said tool_calls were
not populated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: log conversation ID resolution failures instead of swallowing

The v1 write_v1_response silently drops errors via .ok(). Don't
replicate that -- log a warning so failed tool_calls persistence
is diagnosable.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review feedback on v2 tool_calls persistence

- Drop redundant .chain(thread.messages.iter()) — ActionResult messages
  only exist in internal_messages
- Change tracing::warn! to debug! for fire-and-forget persistence
  failures (warn corrupts TUI per CLAUDE.md)
- Add tool_calls assertions to parallel, multi-step, and multi-turn
  tests — all 6 tests now verify the core persistence feature
- Add result_preview content assertion to echo test for tighter coverage

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: cargo fmt + add V24 migration checksum

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: move persist_v2_tool_calls to Completed arm + add unit tests

Move persist_v2_tool_calls into the ThreadOutcome::Completed match arm
so it only fires for final outcomes. Previously it ran for all outcomes
including GatePaused, which caused duplicate/orphaned tool_calls rows
when a gate resumed. Also fixes the Completed { response: None } gap
where tool_calls were never persisted for threads that completed with
tool output but no final text.

Add two libsql-backed unit tests for persist_v2_tool_calls verifying
correct extraction from internal_messages and skip behavior for
text-only threads.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(review): address PR #2452 review follow-ups

Three polish items from the PR #2452 review
(https://github.com/nearai/ironclaw/pull/2452#pullrequestreview-4135957005),
flagged under the Engine v2 review-follow-up tracker issue #2669.

1. **Restore `warn!` for `persist_v2_tool_calls` failures** — commit
   `ff372e11` changed them to `debug!` citing CLAUDE.md's "background
   tasks must not use info/warn" rule. That rule is about REPL/TUI
   corruption; `router.rs` is an HTTP handler path, not a background
   task. Silent `debug!` hid a user-visible bug (chat history missing
   `tool_calls` array) unless someone set `RUST_LOG=debug`. All four
   failure sites (load thread, serialize, resolve conv id, DB write)
   now emit at `warn!` and include the `thread_id` field for
   correlation.

2. **Regression test: `persist_v2_tool_calls` must only be called from
   the `Completed` arm** — commit `652315e8` fixed the original bug
   where the call was shared across all `ThreadOutcome` variants,
   causing partial tool executions on `GatePaused` to orphan DB rows
   that duplicated on resume. The existing unit tests call the function
   directly, so they cover the write path but not the gating. A future
   refactor could silently move the call back out of the `Completed`
   arm and nothing would fail. The new
   `persist_v2_tool_calls_only_called_from_completed_arm` test parses
   the source of `router.rs`, asserts exactly one call site, and
   asserts that site sits between the `Completed` and `GatePaused`
   match arms.

3. **Multi-byte UTF-8 truncation test** — the 500-byte preview
   truncation uses `char_indices()` + `len_utf8()` to avoid slicing
   mid-char. Behavior was correct but unexercised. New test constructs
   an ActionResult with 400 × 3-byte CJK chars (1200 bytes) and pins
   (a) no panic, (b) valid UTF-8 (via JSON round-trip), (c) body
   length < 500+max_char_width, (d) body contains only complete
   3-byte chars.

Verified: `cargo fmt`, `cargo clippy --no-default-features --features
libsql --tests -- -D warnings` (0 warnings), `cargo test -p ironclaw
--lib --features libsql` (5125 passed, +3 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-19 22:45:59 +09:00
firat.sertgoz
08693aa3cc feat(skills): activation feedback pipeline + install idempotence (#2530) 
* feat(events): SkillActivated carries activation feedback notes

Add an optional `feedback: Vec<String>` field to the SkillActivated
event so the engine and selector can surface human-readable activation
notes (chain-load reasons, marker exclusions, scoring summaries) to the
UI. Wire the field through the StatusUpdate, the SSE bridge, and the
gateway's activity timeline; serialize-skip empty vectors so the wire
format stays backwards compatible.

* fix(skills): skill_install never prompts when skill is already loaded

When the LLM force-activates a persona via `/ceo-setup` it sometimes
follows up with a redundant `skill_install("ceo-setup")` call. The
`execute` path was already idempotent (returns `already_installed`
without touching the catalog), but `requires_approval` still gated
the call behind a confirmation prompt — pure friction on a guaranteed
no-op.

Mirror the idempotent shortcut in `requires_approval`: when a skill
with the requested name is already loaded (bundled, user, workspace,
or previously installed), return `ApprovalRequirement::Never`. The
shortcut wins even when `install_dependencies=true` because the
top-level execute is still a no-op (companions get reconciled by their
own activation paths). Regression test covers all three cases.

* fix(skills): preserve approval for dependency installs

* fix(events): include feedback in AppEvent::SkillActivated all-variants list

The variant-enumeration constructor in event.rs:501 was missed when
the new `feedback` field was added to AppEvent::SkillActivated, breaking
the build with E0063. All three Clippy CI jobs failed on this.

Regression: covered by `cargo build --all-features`, which fails to
compile if any variant in this list is constructed with missing fields.

* feat(skills): wire up v1 feedback producer for SkillActivated

The `SkillActivated` event carried an empty `feedback` field because
nothing populated it. This adds the producer end of the pipeline.

**Selector:**
- `prefilter_skills` now returns `SelectionOutcome { selected, notes }`.
- `try_select` returns a reason enum (`Selected`, `BudgetFull`,
  `CandidateLimit`, `MarkerSatisfied`, `AlreadySelected`) so callers
  can render distinct notes instead of opaque "skipped".
- Notes generated for:
  - `<companion>: chain-loaded from <parent>`
  - `<companion>: chain-load skipped (budget full)`
  - `<companion>: chain-load skipped (max active skills reached)`
  - `<companion>: chain-load skipped (setup already complete)`
  - `<skill>: skipped (skill context budget exhausted)` for parents
    that scored but didn't fit.

**Agent loop:**
- `select_active_skills` returns the notes alongside selected skills
  and prepends a `<skill>: force-activated via /mention` note for each
  explicit mention.

**Dispatcher:**
- Emits `StatusUpdate::SkillActivated { skill_names, feedback }` via
  `channels.send_status` whenever something activated or notes exist
  (so "nothing loaded because budget exhausted" surfaces too).
- Silent when nothing activated and no notes — no UI noise.

**Stale comment:**
- Router's v2-bridge comment no longer claims v1 callers populate
  feedback "directly on `StatusUpdate`"; the v1 dispatcher now emits
  its own event, and v2 remains empty until the Python orchestrator
  is updated.

Regression: existing selector test `test_chain_load_respects_budget`,
`test_chain_load_skips_companion_with_satisfied_marker`, and
`test_chain_load_is_non_transitive` now also assert that the
corresponding note is in `outcome.notes`. The 42 selector tests and
503 agent-module tests all pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-19 22:19:32 +09:00
Illia Polosukhin
0af0267125 feat(engine-v2): per-project sandbox (Phases 1–7) (#2211) 
* feat(engine-v2): mount-backend abstraction for per-project sandbox (Phase 1)

Adds the engine-side `MountBackend` trait + minimal `WorkspaceMounts` registry
and a host-side bridge interceptor that routes sandbox-eligible tool calls
(`file_read`, `file_write`, `list_dir`, `apply_patch`, `shell`) through a
backend when their path argument starts with `/project/`. Default behavior is
unchanged: until `EffectBridgeAdapter::set_workspace_mounts(Some(...))` is
called (Phase 6), the interception path is dormant.

This is the first phase of the per-project sandbox plan
(`docs/plans/2026-04-10-engine-v2-sandbox.md`) and a deliberately small subset
of the unified Workspace VFS proposed in nearai/ironclaw#1894 — just enough
abstraction so the sandbox can be a `MountBackend` rather than a special case
in the bridge. When #1894's full mount table lands, the sandbox backend slots
in unchanged.

Engine crate (`crates/ironclaw_engine/src/workspace/`):
- `mount.rs` — `MountBackend` trait, `MountError` (NotFound / InvalidPath /
  PermissionDenied / Io / Tool / Backend / Unsupported), `DirEntry`,
  `EntryKind`, `ShellOutput`
- `filesystem.rs` — `FilesystemBackend`: passthrough host-fs implementation
  with two-layer path validation (lexical reject of absolute / `..`, then
  symlink-escape canonicalization). `read`/`write`/`list` fully implemented;
  `patch`/`shell` return `Unsupported` so the bridge falls through to the
  host tool until Phase 5
- `registry.rs` — `WorkspaceMounts` per-project registry with lazy
  `ProjectMountFactory`, longest-prefix-match resolution, cached and
  invalidatable

Bridge (`src/bridge/sandbox/`):
- `intercept.rs` — `maybe_intercept` and `SANDBOX_TOOL_NAMES`. Returns
  `Handled(json)` on a successful backend dispatch, `FellThrough` for
  non-sandbox tools, host paths, missing path params, or `Unsupported`
  backend ops
- `effect_adapter.rs` — `workspace_mounts` field + `set_workspace_mounts`
  setter; interception block in `execute_action_internal` right before
  `execute_tool_with_safety`, gated on the optional mount table

Tests (31 new):
- 17 engine workspace unit tests covering trait error mapping, path safety
  (lexical + symlink), longest-prefix routing, and lazy factory caching
- 9 bridge sandbox unit tests including `intercept_actually_dispatches_into_backend`
  (counting backend) which proves the interceptor reaches the backend
- 5 integration tests in `tests/engine_v2_sandbox_integration.rs` driving
  `EffectBridgeAdapter::execute_action()` end-to-end per the
  "Test Through the Caller" rule (`.claude/rules/testing.md`), including
  a host-path-falls-through test that asserts the sandbox tempdir was
  not touched, and a `..`-escape test that verifies no `/etc/passwd`
  content leaks even after safety-layer redaction

Drive-by: feature-gate two pre-existing dead-code helpers in
`crates/ironclaw_skills/src/parser.rs` on `#[cfg(feature = "registry")]` to
match their only call site, fixing a pre-existing clippy warning that blocked
the workspace's `-D warnings` policy when `ironclaw_skills` is built with
`default-features = false` (as the engine crate does).

Verification:
- `cargo fmt --check` clean
- `cargo clippy --all --benches --tests --examples --all-features` zero warnings
- 31 / 31 new tests passing; no existing tests broken

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(engine-v2): per-project sandbox — Phases 2–7 + live Docker e2e test

Completes the per-project sandbox plan (docs/plans/2026-04-10-engine-v2-sandbox.md
Phases 2–7), building on Phase 1's mount-backend abstraction (#2211).

Phase 2 — Project workspace folder:
- `Project.workspace_path: Option<PathBuf>` field + `with_workspace_path()`
- Host-side `project_workspace_path()`, `ensure_project_workspace_dir()` (creates
  `~/.ironclaw/projects/<id>/` mode 0700, idempotent)
- `FilesystemMountFactory` taking a `ProjectPathResolver` closure (decoupled from
  `Store`); wired into `EffectBridgeAdapter` via `set_workspace_mounts()`

Phase 3 — Standalone daemon binary:
- `src/bin/sandbox_daemon.rs` — NDJSON over stdin/stdout, health/shutdown/execute_tool
- Constructs ReadFileTool/WriteFileTool/ListDirTool/ApplyPatchTool/ShellTool with
  `base_dir=/project` (override via `IRONCLAW_SANDBOX_BASE_DIR`)

Phase 4 — Dockerfile.sandbox:
- Multi-stage build: rust-slim builder (+ python3 for pyo3) compiles sandbox_daemon;
  debian-slim runtime with tini PID 1, common build tools, `/project` mount target

Phase 5 — ProjectSandboxManager + ContainerizedFilesystemBackend:
- protocol.rs: Request/Response/RpcError matching daemon wire format
- transport.rs: `SandboxTransport` trait (seam for testing without Docker)
- containerized_backend.rs: `ContainerizedFilesystemBackend` impls `MountBackend`,
  translates relative→`/project/<rel>`, maps tool-error→MountError
- docker_transport.rs: real bollard exec session, serialized Mutex, lazy reconnect
- lifecycle.rs: deterministic `ironclaw-sandbox-<pid>` naming, ensure_running/stop/remove
- manager.rs: `ProjectSandboxManager` per-project transport cache

Phase 6 — Router gating on ENGINE_V2_SANDBOX:
- `engine_v2_sandbox_enabled()` helper (truthy: 1/true/yes/on)
- Router selects `ContainerizedMountFactory` when enabled + Docker reachable;
  falls back to `FilesystemMountFactory` with warning otherwise

Live e2e bugs caught and fixed:
- Shell without explicit `workdir` defaulted to host (not sandbox); fixed by
  defaulting to `/project/` in `extract_path_param`
- `ContainerizedFilesystemBackend::shell` parsed `stdout`/`stderr` but host
  ShellTool returns merged `output` field; fixed with fallback key lookup
- SANDBOX_TOOL_NAMES only had v2 names (`file_read`/`file_write`) but host
  registry uses v1 names (`read_file`/`write_file`); added both aliases

Tests (62 sandbox-related, all green):
- 27 bridge sandbox unit tests (intercept, workspace_path, factory, protocol,
  lifecycle, containerized_backend with ScriptedTransport mock)
- 7 containerized-backend tests (including 2 regression tests for the shell bugs)
- 5 engine v2 sandbox integration tests (EffectBridgeAdapter end-to-end)
- 5 daemon binary smoke tests (real subprocess + NDJSON I/O)
- 17 engine workspace unit tests
- 1 live Docker e2e test: agent clones nearai/ironclaw into sandbox, renames
  to megaclaw via sed, verifies with grep — 70s, $0.09, recorded trace committed

Verification:
- `cargo fmt --check` clean
- `cargo clippy --all --benches --tests --examples --all-features` zero warnings
- All 62 sandbox tests passing; no existing tests broken

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: replace .expect() with Result in DockerTransport::ensure_session

CI's no-panics checker flagged the .expect("just inserted") in production
code. Replace with .ok_or_else() returning MountError::Backend.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: multi-tenant project paths + unify sandbox env var with v1

Two issues addressed:

1. Project workspace paths now namespace by user_id:
   `~/.ironclaw/projects/<user_id>/<project_id>/` instead of
   `~/.ironclaw/projects/<project_id>/`. Prevents filesystem collisions
   in multi-tenant deployments where two users could theoretically have
   the same project UUID.

2. Sandbox enablement now reads `SANDBOX_ENABLED` (same env var as v1
   sandbox) in addition to `ENGINE_V2_SANDBOX`. Either being truthy
   enables the per-project sandbox. This means a single flag governs
   sandbox behavior regardless of engine version, while the v2-specific
   override remains available for transitional setups.

Tests: 30 bridge sandbox unit tests passing (added multi-tenant path
tests + env var combination tests).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review — TOCTOU race, shell env passthrough, canonicalize guard

Three issues flagged by the code review bot on #2211:

1. TOCTOU race in WorkspaceMounts::resolve (HIGH): Added double-checked
   locking — re-check the cache after acquiring the write lock so two
   threads racing on the same project's first access don't both call
   factory.build(). The second thread finds the insert from the first.

2. Shell intercept ignores env parameter (MEDIUM): The shell arm in
   maybe_intercept was passing HashMap::new() instead of forwarding
   the tool call's env map. Fixed to parse parameters["env"] and pass
   it through to backend.shell().

3. Canonicalization fails when root doesn't exist (MEDIUM): When
   self.root hasn't been created yet (first write to a new project),
   canonicalize_under_root would walk up to a real ancestor and the
   starts_with check against the non-existent root would always fail.
   Now skips canonicalization entirely when root doesn't exist — lexical
   safety is already guaranteed by safe_join.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review round 2 — apply_patch schema, content validation, dir perms, docs

- Fix apply_patch schema mismatch: MountBackend::patch now takes
  (old_string, new_string, replace_all) matching ApplyPatchTool's
  actual contract. Previously sent {patch: diff} which would fail
  with invalid_params in the containerized daemon.
- Validate file_write content param: return error instead of silently
  writing empty string when content is missing.
- Log stderr frames from sandbox daemon at debug! instead of silently
  discarding them in docker_transport StreamReader.
- Tighten permissions on intermediate directories created by
  ensure_project_workspace_dir (projects/, <user_id>/) to 0o700,
  not just the leaf.
- Fix stale module doc in sandbox/mod.rs (referenced "Phase 5 will
  add" but all phases shipped).
- Fix doc path mismatch: workspace path is <user_id>/<project_id>/,
  not <project_id>/ (workspace_path.rs, CLAUDE.md, design plan).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review round 3 — symlink safety, visibility, debug logging

- Close TOCTOU window in canonicalize_under_root: re-canonicalize and
  verify containment when the reassembled path exists on disk
- Fix list_dir_recursive: use symlink_metadata (lstat) so symlinks are
  detected instead of followed; validate directories against root before
  recursive traversal
- Tighten is_mountable_path to /project/, /memory/, /home/ prefixes
  instead of any absolute path (defense-in-depth)
- Narrow sandbox module visibility to pub(crate) and remove unused
  pub use re-exports
- Remove concrete types (FilesystemBackend, DirEntry, EntryKind,
  ShellOutput) from engine crate top-level re-exports; access via
  ironclaw_engine::workspace:: module path
- Add debug! tracing to sandbox intercept routing decisions
- Add read_file/write_file v1 aliases to daemon SUPPORTED_TOOLS health
  response
- Remove developer-local path from sandbox mod.rs doc comment
- Merge staging to fix CI (user_timezone field on ThreadExecutionContext)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review round 4 — safety validation, network isolation, binary writes

- Add pre-intercept safety param validation so sandbox-dispatched calls
  go through the same checks as host-dispatched calls (#1)
- Set network_mode: "none" on sandbox containers to prevent outbound
  network access (#3)
- Reject binary content in containerized write instead of silently
  corrupting via from_utf8_lossy (#5)
- Cap list_dir depth to 10 to prevent unbounded traversal (#8)
- Change container creation log from info! to debug! to avoid breaking
  REPL/TUI output (#10)
- Make is_truthy case-insensitive so SANDBOX_ENABLED=True works (#11)
- Return error instead of unwrap_or_default for missing container ID (#12)
- Propagate set_permissions errors instead of silently ignoring (#13)
- Return error for missing daemon output key instead of defaulting to
  empty object (#14)
- Add env mutex guard in sandbox_live_e2e test (#15)
- Fix rustfmt formatting for let-chain in canonicalize_under_root

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review round 5 — path traversal, error types, tests

Security fixes:
- Sanitize user_id in workspace path to prevent directory traversal via
  malicious user IDs containing `..` or `/`
- Add Component::ParentDir check in ContainerizedFilesystemBackend::container_path
  matching the defense-in-depth approach of FilesystemBackend::safe_join

Correctness:
- Use MountError::Tool instead of MountError::InvalidPath for missing
  tool parameters (content, old_string, new_string) — fixes confusing
  LLM-visible error messages
- Fix clippy sort_by_key suggestion in registry.rs

Cleanup:
- Remove spurious Notify import and dead _notify_link function

New tests:
- ContainerizedFilesystemBackend path traversal rejection (read + write)
- container_path unit tests for safe and unsafe paths
- Adversarial user_id test in workspace_path
- Daemon-side path traversal test in sandbox_daemon_smoke

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review round 6 — param normalization, error types, edge cases

- Normalize sandbox params via prepare_tool_params() before validation,
  matching the host execution path (fixes inconsistent validation)
- Return ToolError::InvalidParameters instead of EngineError::Effect for
  sandbox param validation failures (consistent error surface)
- ensure_dir checks path.is_dir() not path.exists() (rejects files)
- Empty user_id returns "_anonymous" sentinel instead of empty hex string
  that would drop the tenant namespace via PathBuf::join("")
- Restore ENGINE_V2_SANDBOX env var after sandbox live E2E test
- Tighten is_mountable_path to /project/ only (no mounts for /memory/
  or /home/ yet)
- Add v1 tool name aliases (read_file, write_file) to SUPPORTED_TOOLS

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: unify sandbox env var — remove ENGINE_V2_SANDBOX, use SANDBOX_ENABLED only

Single env var controls sandboxing for both engine versions. The
transitional ENGINE_V2_SANDBOX override is removed from code, tests,
docs, and Dockerfile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: double-checked locking in transport_for, explicit stdin close in smoke test

- ProjectSandboxManager::transport_for no longer holds the mutex across
  the Docker ensure_running await. Uses double-checked locking so
  concurrent projects initialize in parallel.
- sandbox_daemon_smoke: explicitly take() stdin before wait_with_output
  so EOF is sent even without a shutdown request.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review — network mode, error types, race, protocol dedup

- Change sandbox container network_mode from "none" to default bridge
  so git clone / cargo build / pip install work inside the container
- Fix binary content rejection to use MountError::Tool instead of
  MountError::InvalidPath (semantic mismatch)
- Fix list depth: use actual depth value instead of depth.max(1)
- Fix orphan container race in transport_for by holding lock across
  container creation instead of double-checked locking
- Deduplicate protocol types: daemon now imports from shared
  bridge::sandbox::protocol instead of defining its own copies
- Make bridge::sandbox pub (narrow exposure: only protocol and
  workspace_path sub-modules are pub)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs: update plan doc — sandbox uses bridge networking, not network_mode=none

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-19 20:20:17 +09:00
Illia Polosukhin
77c3821f33 feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2) (#2617) 
* feat(common): add CredentialName and ExtensionName newtypes

Introduce typed identifiers for the backend-secret vs user-facing extension
identity split that the Extension/Auth Invariants section of CLAUDE.md
describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity-
confusion bugs with the same shape: a stringly-typed value passed through
multiple layers with each layer meaning a different thing. Newtypes make
each of those a compile error.

This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam
(ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth,
LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate
AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the
remaining extension_name: String fields.

Wire format is unchanged — both newtypes use #[serde(transparent)] so on-
wire and on-disk representations stay plain strings and legacy persisted
rows keep deserializing. Validation runs at explicit construction
(::new / ::try_from / ::from_str), not at deserialize time.

Also adds .claude/rules/types.md codifying the "no stringly-typed
internals" rule.

Regression coverage: 17 new unit tests in identity.rs; existing
auth_manager, router, and gate tests (130+ cases) all pass unchanged.

* fix(common): address PR #2611 review feedback

Four fixes from Copilot, Gemini, and Claude reviews:

- **identity.rs docs**: drop reference to a non-existent `validate()`
  re-validation API. Document that instances represent "passed
  validation at some point in history" rather than "guaranteed valid
  right now" — by design.

- **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token`
  gate path was using `CredentialName::from_trusted` to wrap a value
  read straight out of a tool's JSON output. Tool output is
  external/untrusted; use `CredentialName::new` (validating) with a
  cascade: external → tool name → `from_trusted(tool_name)` as final
  fallback. Closes a credential-name shape-injection vector.

- **canonicalize()**: reorder checks cheapest-first against the trimmed
  slice so invalid inputs reject without allocating a canonicalized
  `String`. `replace('-', "_")` is deferred until after the structural
  checks pass; since `-`/`_` are both one byte, the earlier length
  check stays valid.

- **Remove `Deref<Target = str>`** from identity newtypes, keep
  `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`,
  which is exactly the implicit-conversion pattern these newtypes
  exist to prevent. Callers that had a `&CredentialName` where `&str`
  was expected now write `.as_str()` explicitly. Added a regression
  test for the accessor contract and updated the rule template in
  `.claude/rules/types.md` to document the decision.

Declined one review item (Claude): the remaining `to_string()` calls
inside `IdentityError` variants are on the exception path; the common
invalid-input case no longer allocates twice after the canonicalize
reorder, and errors must carry owned strings so they can escape the
function.

Regression coverage: 5035 lib tests + 18 identity tests (one new —
`explicit_accessors_work`) pass. Zero clippy warnings.

* feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2)

Follow-up to #2611. Migrates the remaining stringly-typed extension_name
and credential_name fields to use the ExtensionName and CredentialName
newtypes introduced in ironclaw_common::identity.

Fields now typed:

- AppEvent::{OnboardingState, GateRequired, ExtensionStatus}.extension_name
  (serde transparent — wire format unchanged)
- StatusUpdate::{AuthRequired, AuthCompleted}.extension_name
- TuiEvent::{AuthRequired, AuthCompleted}.extension_name (adds
  ironclaw_common dep to ironclaw_tui)
- PendingOAuthLaunchParams.extension_name
- PendingOAuthFlow.extension_name
- PendingAuth.extension_name, PendingAuthPrompt.extension_name
- ParsedAuthData.extension_name, selected_auth_prompt tuple
- emit_auth_required_status() and Session::enter_auth_mode() parameters
- event_from_configure_result() parameter
- resolve_extension_for_action() and resolve_auth_gate_display_name()
  return types
- normalize_extension_name() return type

PendingAuthPrompt::new is now infallible (accepts ExtensionName directly)
since the identity validator carries the non-empty invariant the
constructor used to re-check. The "blank extension name" rejection test
moved out — that logic lives in ironclaw_common::identity tests.

Test updates use `ExtensionName::new("...").unwrap()` at construction
sites and `from_trusted(...)` where a trusted upstream string is being
adapted. Every site is a compile-time audit of where the type was
crossing a boundary untyped.

Regression coverage: existing 5034 lib tests + 26 engine_v2_gate
integration tests + 40 ironclaw_common tests all pass. Zero clippy
warnings across all features.

* fix(web): return ExtensionName from pending_gate_extension_name

Addresses Claude's review comment on #2611: the function was doing
`Some(credential_name.as_str().to_string())` in the fallback branch,
defeating the newtype's purpose by re-stringifying the identity.

Return `Option<ExtensionName>` instead. Plumbs through `PendingGateInfo.
extension_name` (wire format unchanged — `#[serde(transparent)]`).
The fallback path's cross-identity conversion (credential name →
extension name) is now an explicit `ExtensionName::from_trusted` call,
making the boundary crossing visible at the call site.

Also fixes the `Deref<Target = str>` removal fallout that followed the
rebase onto the updated PR 1: call sites that relied on auto-deref
(`ext.contains(...)`, `auth_manager.submit_auth_token(&cred_name, ...)`)
now explicitly call `.as_str()`.

* fix(router,web): address PR #2617 review feedback

Four Gemini review comments, all on the boundary between credential/
extension identifiers and user input.

1. [HIGH, security] extensions_setup_submit_handler was wrapping the
   URL path segment in ExtensionName::from_trusted, which skips the
   newtype's path-traversal / invalid-character validation. That path
   is user-controlled (`/api/extensions/{name}/setup`). Validate with
   ExtensionName::new at the handler entry and return 400 on failure;
   downstream uses switch to .as_str() or .clone() of the validated
   value, and the three in-handler from_trusted sites disappear.

2. Rename resolve_auth_gate_display_name ->
   resolve_auth_gate_extension_name. The function returns an
   identifier/slug, not a human-readable display name — the old name
   was a leftover from when the value was a String.

3. Return Option<ExtensionName> from the renamed function. Previously
   the non-Authentication gate branch fabricated an
   ExtensionName::from_trusted(pending.action_name), which was
   semantically wrong (an action name is not an extension identifier)
   and silently defeated the type's invariants. Now it returns None
   for Approval/External gates, and callers thread an Option through.
   send_pending_gate_status accepts Option<&ExtensionName> and only
   uses it on the Authentication arm, with a warn! log if upstream
   plumbing ever reaches the arm with None. The GateRequired SSE
   event's extension_name is now a clean .clone() of the Option.

4. Rename auth_display_name -> extension_name on
   send_pending_gate_status so the parameter name matches both its
   type and the StatusUpdate::AuthRequired.extension_name field it
   feeds.

Regression: new test_extensions_setup_submit_rejects_path_traversal_name
at the handler tier (per .claude/rules/testing.md "Test Through the
Caller, Not Just the Helper") drives the handler with malformed path
segments and asserts 400 before the value reaches extension lookup or
any from_trusted wrap. 5035 lib tests pass, zero clippy warnings.

* docs(identity): codify web-boundary rules + add static check

Three rule additions + one enforcement hook covering the identity
boundary that PR #2617 review uncovered:

- src/channels/web/CLAUDE.md — extend "Unified Extension Onboarding"
  with explicit rules:
  * Setup/configure/activate routes MUST validate `{name}` via
    `ExtensionName::new` at handler entry (return 400 on failure).
  * Web DTOs and handlers MUST NOT reference `CredentialName` —
    credential identity is backend-only; the dispatcher/auth_manager
    resolves it from the ExtensionName server-side.
  * Auth-flow extension resolution happens in *one* place
    (`AuthManager::resolve_extension_name_for_auth_flow`). Wrappers
    are thin and delegate; they must not duplicate the precedence
    logic or re-derive from credential prefixes. The four recent
    identity bugs (#2561, #2473, #2512, #2574) were duplicate-
    resolution drift.

- src/bridge/CLAUDE.md — new module spec documenting auth_manager.rs
  as the single authority for auth-flow extension resolution, with
  the resolver's four-step precedence order and the approved wrapper
  call sites.

- scripts/pre-commit-safety.sh — new check #8 (CREDNAME): flags
  `CredentialName` references in newly-added production lines under
  `src/channels/web/**`. Test-mod code is excluded via the existing
  `strip_test_mod_lines` filter. Suppression via
  `// web-identity-exempt: <reason>` for the rare legitimate case of
  reading an already-typed value off a backend struct. Smoke-tested:
  * baseline (current branch) — no warnings
  * injected violation — fires with CREDNAME warning
  * injected violation + `// web-identity-exempt:` — suppressed

The rules and the check live at the same level — humans read the
rule, CI enforces it.

* fix(auth): validate user-influenced names at the resolver boundary

Addresses four Copilot review comments on PR #2617 that all pointed at
the same seam: the canonical `AuthManager::resolve_extension_name_for_auth_flow`
returned a raw `String` whose first branch (the LLM-supplied `name`
parameter on `tool_install` / `tool_activate` / `tool_auth` actions)
passed through without `ExtensionName` validation. Both call sites
then wrapped the result in `ExtensionName::from_trusted`, promoting an
unvalidated user-influenced value to a typed identity.

- **Resolver now returns `ExtensionName`.** Branch 1 validates the
  user-controlled name via `ExtensionName::new` and falls through on
  failure; branches 2–4 use `from_trusted` because their sources
  (tool registry hint, canonicalizer, typed credential fallback) are
  already trusted upstream. This consolidates validation in the single
  "resolve once" site documented in `src/bridge/CLAUDE.md`.

- **router.rs and server.rs drop their wraps.** `resolve_extension_for_action`
  (router) and `pending_gate_extension_name` (server) return the
  resolver's typed output directly. The tool-registry fallback in
  router.rs (no-auth-manager path) keeps its `from_trusted` wrap
  since it operates on the same trusted sources as branch 2.

- **`restore_selected_auth_prompt` re-validates rehydrated prompts.**
  `PendingAuthPrompt` is `#[serde(transparent)]`, so deserialize does
  not re-check the inner `ExtensionName` string. A legacy-persisted
  invalid name would previously have been dropped by the old
  `PendingAuthPrompt::new(String, ...)` empty-string rejection; now
  `restore_selected_auth_prompt` re-runs `ExtensionName::new` and
  drops + warns on failure, upgrading the old non-empty-only check to
  the full identity invariant. New test
  `test_restore_selected_auth_prompt_rejects_invalid_legacy_row` forges
  three invalid rows (empty / uppercase / path-traversal) straight
  through serde and asserts each is dropped.

- **Docstring on `PendingAuthPrompt` refreshed.** The old comment
  claimed `::new` "trims and validates extension_name is non-empty",
  which is no longer true — `::new` is infallible and the invariant
  lives in `ExtensionName` itself. The new comment documents the
  split: validation runs at `ExtensionName::new` construction and at
  restore-from-persistence, not inside `PendingAuthPrompt`.

Regression: 5063 lib tests pass (+1 new). Clippy zero warnings.

* fix(ci): adapt post-merge-from-staging sites to ExtensionName

Staging shipped #2640 (repl unlock) and gateway refactor commits after
my last merge. The CI build picked them up via auto-merge and hit three
type mismatches my branch hadn't seen:

- src/channels/repl.rs:908 — new test constructs
  `StatusUpdate::AuthRequired { extension_name: "google_oauth_token"
  .to_string(), ... }`. Typed field; now `ExtensionName::new(...).unwrap()`.

- src/channels/web/server.rs:1405-1424 — staging added a no-auth-manager
  fallback chain to `pending_gate_extension_name` that returned raw
  `Some(String)` on three branches. Aligned with
  `AuthManager::resolve_extension_name_for_auth_flow`: branch 1
  (user-influenced `tool_install`/`tool_activate`/`tool_auth` `name`
  param) validates via `ExtensionName::new` and falls through on
  failure; branches 2-3 (provider-extension hint, credential-name
  fallback) use `from_trusted` because they're sourced from typed
  upstream state. Mirrors the fix applied to the canonical resolver
  in c813caa9.

- src/channels/web/server.rs:3831 — test used `.as_deref()` on the
  function's Option<ExtensionName> return; switched to
  `.as_ref().map(|n| n.as_str())` matching the pattern from the
  adjacent test.

No new logic — just adapting two staging landings to the typed surface
PR #2617 introduces. The validation behaviour for the fallback path is
already locked in by the identity-layer tests in
`ironclaw_common::identity` (rejects_path_traversal, rejects_uppercase,
etc.) and by the regression test added in c813caa9
(test_restore_selected_auth_prompt_rejects_invalid_legacy_row).

[skip-regression-check] — type adaptation to unblock CI, no behaviour
change needing its own regression test.

Clippy with `-D warnings` clean, 5074 lib tests pass.

* fix(auth): extract shared resolver; wrapper delegates instead of duplicating

Addresses two Copilot comments on PR #2617 that surfaced the same
architectural issue: the no-auth-manager fallback in
`pending_gate_extension_name` had grown a three-branch copy of the
resolver's precedence that quietly skipped branch 3 (canonicalize
action_name + check `ExtensionManager::extension_info`). Exactly the
duplicate-resolution drift the "one resolver" rule in
`src/bridge/CLAUDE.md` warns against — four prior identity bugs
(#2561, #2473, #2512, #2574) were the same pattern.

- Extracted `pub(crate) async fn resolve_auth_flow_extension_name` to
  `src/bridge/auth_manager.rs` as the single site of the four-branch
  precedence. Takes `Option<&ToolRegistry>` + `Option<&ExtensionManager>`
  so both the `AuthManager` method (which passes its own fields) and
  the web wrapper (which passes `state.tool_registry` /
  `state.extension_manager`) share identical logic.

- `AuthManager::resolve_extension_name_for_auth_flow` is now a 1-block
  delegator.

- `pending_gate_extension_name` in `web/server.rs` drops its inline
  fallback entirely and calls the shared free function from both
  branches. The bare-test-harness path now runs branch 3 (canonicalize
  + installed-extension check) that it previously missed.

- Updated `src/bridge/CLAUDE.md` to document the free function as the
  single authority, the three approved wrappers as thin delegators,
  and the return type as `ExtensionName` (was stale `String` from the
  pre-c813caa9 era).

Regression coverage: the existing
`resolve_extension_name_for_auth_flow_prefers_installed_channel_name`
test passes unchanged — it exercises branch 3 through the method, which
now reaches it via the extracted free function.

* Merge remote-tracking branch 'origin/staging' into feat/identity-newtypes-pr2

Picks up #2644 (platform/ extraction) and #2645 (features/oauth/ move).

Manual resolutions:
- src/channels/web/server.rs: staging removed 720 lines of OAuth
  callback code (moved to features/oauth/mod.rs in #2645). My PR 2
  ExtensionName changes to two of those functions (oauth_callback_handler,
  slack_relay_oauth_callback_handler) ported to the new location.
- src/bridge/auth_manager.rs: extended the shared resolver's
  branch-1 action pattern to include 'tool-activate' and 'tool-auth'
  variants, matching staging's new
  pending_gate_extension_name_uses_install_parameters_for_hyphenated_activate_tool
  test expectation. Underscore + hyphen variants for all three actions.

No new PR 2 logic — just aligning the type surface with two staging
refactors. 5074 lib tests pass (+1 vs previous — the new staging
hyphenated-tool test). Clippy -D warnings clean.

* fix(web): address PR #2617 round-3 review feedback

Two Copilot findings from the 2026-04-18 review:

1. `/api/extensions/{name}/{activate,remove,setup}` handlers accepted
   `Path<String>` and forwarded it to the extension manager without
   validating path-traversal, invalid characters, or case — only
   `extensions_setup_submit_handler` had the `ExtensionName::new` guard.
   Applied the same boundary validation to all three siblings.

2. `restore_pending_auth_mode` took `extension_name: &str` and
   re-wrapped it with `ExtensionName::from_trusted`, re-introducing an
   unvalidated string boundary even though every caller already held
   an `ExtensionName` (`pending_auth.extension_name`). Changed the
   helper to accept `&ExtensionName` so the identity stays typed
   end-to-end; `from_trusted` is no longer needed here.

Regression: added `test_extensions_sibling_handlers_reject_path_traversal_name`
covering activate / remove / setup-GET with the same malformed slugs
the setup-submit test already locks in (path traversal, slash in
segment, uppercase, space, trailing underscore). Drives the handlers
through axum routing so the boundary is exercised end-to-end.

* fix(ci): adapt replay_outcome to ExtensionName after staging merge

Staging #2621 added `tests/support/replay_outcome.rs`, which destructures
`StatusUpdate::{AuthRequired,AuthCompleted}.extension_name` into a
`String` field of `EventSummary`. This PR made those `StatusUpdate`
fields `ExtensionName`, so the post-merge build breaks in the replay
snapshot gate and all-features clippy jobs.

Convert to `String` at the destructure via `ExtensionName::into()` so
the `EventSummary` shape (and the persisted `.snap` files) stay
unchanged. The test-support / snapshot wire format is a legitimate
String boundary per `.claude/rules/types.md`.
 2026-04-19 19:58:43 +09:00
Illia Polosukhin
c4927ba6e1 fix(ci): unblock staging Docker Build and echo tool E2E test (#2661) 
Two independent staging CI regressions:

1. Docker Build was failing because `cargo install wasm-tools@1.246.1`
   re-resolved to the newest compatible `constant_time_eq@0.4.3`, which
   requires rustc >= 1.95, while the chef stage is pinned to rust:1.92.
   Add `--locked` so cargo uses the Cargo.lock shipped with each crate.

2. `test_builtin_echo_tool` started failing after PR #2555 intentionally
   aligned the in-memory history path with DB semantics: tool previews
   now surface in `result` with `result_preview` left empty. The test
   only inspected `result_preview`, so it timed out. Accept the preview
   from either field in `_wait_for_turn`.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-19 18:44:01 +09:00
Illia Polosukhin
ff119531d4 test(replay): promote engine traces to insta-backed snapshot gate (#2621) 
* test(replay): promote engine replay traces to insta-backed snapshot gate

Adds a ReplayOutcome snapshot type, a replay-gate CI workflow, and a
developer script wrapper for cargo-insta. Replaces unreviewable 3,000-line
JSON diffs on engine changes with a YAML snapshot of the observable run
shape (tool sequence, final state, retrospective analyzer issues).

Why: engine v2 live-fixture traces had grown past reviewability. A single
prompt-wording change could move the whole fixture, and reviewers had no
way to see which behaviour actually changed. Splitting the fixture into a
"replay driver" (JSON stays in tests/fixtures/) and a "regression
snapshot" (YAML in tests/snapshots/) gives reviewers a narrow, stable diff
to approve, while keeping the full recorded context for deterministic
replay.

Changes:
- `tests/support/replay_outcome.rs` — ReplayOutcome + assert_replay_snapshot!
  macro; snapshots include retrospective analyzer output (TraceIssue
  severity/category) via a new `ironclaw::bridge::engine_retrospectives_for_test()`
  helper that runs `build_trace()` over engine threads
- `tests/e2e_engine_v2.rs` — three POC snapshot tests
  (single_tool_echo, tool_error_recovery, zizmor_scan_v2)
- `tests/e2e_bug_bash_snapshots.rs` + `tests/fixtures/llm_traces/bug_bash/`
  — bug-regression fixture template, mapped to open issues in the README
- `.github/workflows/replay-gate.yml` — cargo insta test --check on
  engine/agent/LLM/tools/bridge path changes; rejects committed .snap.new
- `scripts/replay-snap.sh` — review/accept/test/record wrappers around
  cargo-insta and IRONCLAW_RECORD_TRACE
- `scripts/trace-coverage.sh` — reports EventKind variants with
  snapshot coverage; `--strict` mode for future CI promotion
- `tests/e2e_live.rs` — `#[ignore]` swapped for
  `cfg_attr(not(feature="replay"), ignore)` so the replay CI job can
  run the scenarios without `-- --ignored`
- `Cargo.toml` — new `replay = ["libsql"]` feature; insta gains
  the `yaml` feature
- `tests/fixtures/llm_traces/README.md` — documents the two-role
  driver/snapshot split

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(replay): address PR #2621 review + swap cargo-insta installer

Review fixes:

- Replay gate was missing the bug-bash snapshot suite. Adds
  `tests/e2e_bug_bash_snapshots.rs` to the workflow paths trigger and the
  `cargo insta test --check` invocation so bug-regression snapshots are
  actually gated. (copilot-pull-request-reviewer)

- `cargo install cargo-insta --locked` added ~40s of cold-cache compile
  to the gate. Swapped for `taiki-e/install-action@v2`, which downloads
  a precompiled binary in a few seconds. Also updated
  `scripts/replay-snap.sh` to *fail closed* when cargo-insta is missing
  instead of silently auto-installing it. (gemini-code-assist)

- `engine_retrospectives_for_test` was `pub` and re-exported under the
  default-enabled `libsql` feature, contradicting its "not part of any
  public API" doc. Split the re-export, kept `reset_engine_state` as a
  plain `pub use`, and hid `engine_retrospectives_for_test` behind
  `#[doc(hidden)]` — it still needs to cross the crate boundary for
  integration tests (which live in a separate crate, so `#[cfg(test)]`
  doesn't reach them), but no longer appears in published docs.
  (copilot-pull-request-reviewer)

- Added an explicit "caller must serialize" note on
  `engine_retrospectives_for_test` explaining the `ENGINE_STATE`
  singleton and pointing new callers at `engine_v2_test_lock()` /
  `reset_engine_state()`. Matches what the existing snapshot tests
  already do. (gemini-code-assist)

Doc corrections:

- `snapshot_zizmor_scan_v2` doc claimed the snapshot pinned
  `ApprovalNeeded` events and response wording — it doesn't. Rewrote to
  describe what the snapshot actually asserts (tool order, step count,
  retrospective issues, final state). (copilot-pull-request-reviewer)

- `llm_call_count` was documented as "bucketed" but passed through
  verbatim. Updated the field doc to reflect the raw value. Bucketing
  wasn't needed because fixtures are deterministic. (copilot-pull-request-reviewer)

- `src/bridge/router.rs` doc referenced a non-existent
  `ReplayOutcome.trace_issues` field — the struct uses `engine_threads`.
  Fixed the reference. (copilot-pull-request-reviewer)

- `scripts/trace-coverage.sh` header claimed CI runs it with `--strict`;
  the workflow runs it in advisory mode. Rewrote the header to match,
  with a pointer for when to promote to strict. (copilot-pull-request-reviewer)

No-change replies (rationale commented in the code):

- `event_kind_name` uses an exhaustive `match` on `EventKind` rather
  than `Debug` or a `strum` derive. The compile-time exhaustiveness
  check is the point — adding a new engine event should force a
  conscious decision about how the snapshot represents it, not a silent
  fallthrough. Added a comment making that intent explicit.

- `trace-coverage.sh` awk parser of `event.rs` is fragile — agreed, but
  the script is advisory and its failure mode is false negatives
  (uncovered variants simply aren't gated). Documented the tradeoff and
  the rewrite-in-Rust escape hatch in the script header.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(replay-gate): prime cache on staging, restrict PR runs to read-only

The second run on PR #2621 missed the cache ("No cache found" in the
rust-cache restore step) even though the workflow is wired correctly.
Root cause: the repo sits close to GitHub's 10 GB per-repo cache quota
(~59 entries, many >500 MB), and the LRU policy evicts PR-scoped caches
before they get reused.

Fix:
- Add `push: [staging, main]` so the gate runs (and saves a ~1.2 GB
  cache under the `replay-gate` key) on every merge to the branches
  PRs actually target. Subsequent PRs restore from that base-branch
  cache — GitHub Actions permits cross-ref restore when the restoring
  ref's base matches the saved ref.
- Set `save-if: ${{ github.event_name == 'push' }}` so PR runs only
  *read* the cache. Without this gate, each PR push would save its
  own copy and crowd out the primed base-branch cache, putting us
  right back in the eviction loop.

Expected effect: cold-cache 9m → warm ~2-3m once staging has a run with
the new workflow. Base-branch prime run still pays 9m (no regression).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(replay): drop bug-bash fixture scaffolding

Replay fixtures can't reproduce the Phase 3 target bugs because the
fixture *is* the LLM's output — handwriting a trace where the LLM
emits a tool call doesn't test whether the real LLM would have emitted
that call, only that the harness dispatches a scripted one. What
`summarization_uses_tools.json` actually pinned was the happy path,
not the #2541 bug.

Of the 7 open bug-bash issues, only #2544 ("plans and delegates but
never executes") is catchable by replay, and only via a live-recorded
fixture. The other six are LLM-behavior or infra-timing bugs outside
replay's reach. Rather than ship regression theater, tear out the
scaffolding.

Removed:
- tests/e2e_bug_bash_snapshots.rs
- tests/fixtures/llm_traces/bug_bash/
- tests/snapshots/replay__bug_bash_summarization_uses_tools.snap

Unwired:
- Replay-gate workflow paths + test list no longer mention bug_bash
- scripts/replay-snap.sh test command drops the extra --test flag

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: switch to cargo-nextest with per-test timeouts

Nextest runs each integration test in its own process and runs test
binaries in parallel, which is a big unlock for this repo:

- Engine v2 tests share a process-global `ENGINE_STATE` singleton
  (OnceLock), which the current test lock serialises inside a single
  test binary. Nextest's process-per-test model gives each test a
  clean state automatically, so the 16 engine_v2 tests stop running
  one-by-one.

- Cross-binary parallelism: `cargo test --test A --test B` runs
  binaries in sequence; nextest runs them concurrently.

Measured locally: the replay-gate test set (3 binaries, 21 tests)
went from ~30s sequential to **2.7s parallel**.

Adds `.config/nextest.toml` with:
- `slow-timeout = 60s / terminate-after 3` in the default profile so
  a hung test fails fast instead of blocking the workflow-level 25-
  minute cap.
- A `ci` profile with `fail-fast = false` (one flake shouldn't mask
  other failures), `failure-output = immediate-final`,
  `success-output = never` for readable Actions logs.
- Per-test 300s override for the handful of genuinely slow scenarios
  (zizmor scan, e2e_thread_scheduling).

Workflows updated:
- `replay-gate.yml`: installs cargo-nextest via taiki-e/install-action
  alongside cargo-insta (one step), runs `cargo insta test
  --test-runner nextest` with `NEXTEST_PROFILE=ci`.
- `test.yml`: all five `cargo test` invocations swapped for
  `cargo nextest run --profile ci`. Nextest doesn't execute doctests,
  so every nextest step is paired with a `cargo test --doc` follow-up
  to preserve coverage.

Local dev is unchanged — `cargo test` still works; nextest is only
required in CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci: re-trigger replay-gate workflow after nextest migration

Previous push only modified workflow files and `.config/nextest.toml`;
GitHub skipped the `pull_request` workflow events for that sync, so
the nextest migration didn't actually get exercised in CI. Empty
commit forces re-evaluation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(replay): note nextest wiring in the fixtures README

Also forces a CI re-run: the previous empty commit had no matching
paths, so the `pull_request.paths` filters skipped every workflow
including replay-gate. Touching a file under
`tests/fixtures/llm_traces/**` re-matches the filter and runs the
nextest-based gate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(test): defer test.yml nextest migration

Staging restructured test.yml significantly while this PR was open
(matrix-config dynamic matrix, `changes` code-detection job,
composite install-cargo-component action, save-if restricted to
base-branch pushes). The merge into staging had heavy conflicts for
every nextest-swap hunk.

Rather than force a re-layering of the new staging structure on top
of the nextest migration in this PR, revert test.yml to staging's
current version. This PR now scopes the nextest change to just the
replay-gate workflow (where it cleanly demonstrates the value) plus
the shared `.config/nextest.toml` profile. Migrating the rest of
test.yml to nextest is a follow-up that can rebase on the new
structure without the heavy conflict surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Henry Park <henrypark133@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-19 17:34:01 +09:00
firat.sertgoz
ce88b6eac7 test(e2e): harden tab_button selector against strict-mode duplicates (#2656) 
Closes #2626.

`tests/e2e/helpers.py` used `.tab-bar button[data-tab="{tab}"]` to locate
every main-nav tab button. Commit 5058a1cf removed the duplicate
right-side `status-logs-btn` Jobs button that was resolving that selector
to two elements on staging, so `test_connection.py` passes again. The
underlying selector is still fragile: any future auxiliary button or
`_addWidgetTab`-injected tab that reuses a built-in `data-tab` id (even
accidentally, as #2353 did) will make the selector resolve to multiple
elements and trip Playwright strict mode the next CI run.

Harden the selector in place — the issue's suggested direction of
scoping to a more specific parent region — so the regression can't
repeat without a test-side opt-in:

- `.tab-bar > button`: direct child, skipping any hypothetical nested
  buttons (e.g. menu popovers).
- `:not(.status-logs-btn)`: excludes the right-side logs/docs cluster
  that uses the same `data-tab` hook for click routing.
- `:not(.tab-btn)`: excludes widget-injected tabs (see `_addWidgetTab`
  in `crates/ironclaw_gateway/static/app.js`), which always carry the
  `tab-btn` class and could in principle collide with a built-in id.

Verified against a synthetic DOM with three colliding `data-tab="jobs"`
buttons (original, status-logs-btn duplicate, widget-injected): the old
selector matches 3 and trips strict mode on `.click()`; the new
selector matches 1 and clicks cleanly. No production HTML change is
required — the acceptance criterion explicitly forbids one.

`pytest tests/e2e/scenarios/test_connection.py -v` → 3 passed.
 2026-04-18 23:01:34 +03:00
firat.sertgoz
1b99d0c325 test(e2e): fix Slack fixture boot path (#2638) 
* test(e2e): fix Slack fixture boot path

Fixes #2623

* test(e2e): tighten slack fixture teardown

- Wrap tmpdirs and process lifecycle in an outer try/finally so reserved
  sockets always close, including when TemporaryDirectory construction
  fails before yield.
- Drop redundant `reset_fake_slack` calls at the start of tests now that
  the `active_slack` fixture already resets between tests. Keeps the
  intentional mid-test reset in the malformed-payload resilience case.

Review follow-ups on #2638. No behavior change for passing tests.

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-18 18:39:09 +09:00
Illia Polosukhin
5058a1cf0c fix(ci): three staging regressions — skill chain-load, duplicate Jobs tab, onboarding E2E (#2637) 
Scheduled batched CI on staging was red across three unrelated paths.
All three are fixed in-place; the existing tests become the regression
coverage.

1. `tests/support/test_rig.rs`: rebuild the skill registry against the
   test's `with_skills_dir()` tempdir and actually run `discover_all()`.
   `AppBuilder::init_database()` reloads `config` from DB/TOML/env at the
   top of `build_all()`, which clobbered `config.skills.local_dir` back
   to the default (`~/.ironclaw/skills/`). Any registry `build_all()`
   constructed therefore pointed at the user's real skills dir, not the
   tempdir the test had laid down — so `loaded_skill_names()` came back
   empty and the v1 chain-load assertion panicked. Write the tempdir
   paths back onto `components.config.skills.*` so `AgentDeps::skills_config`
   agrees with the registry. `skill_chain_load_lifecycle::v1_chain_load_pulls_in_required_companions`
   now passes.

2. `crates/ironclaw_gateway/static/index.html`: drop the duplicate
   right-side `status-logs-btn` Jobs button added in #2353. The main
   tab-bar already has `<button data-tab="jobs">Jobs</button>`, and the
   duplicate had no `data-v1-only`/`data-v2-only` marker, so both
   rendered simultaneously. That broke `test_connection.py` (Playwright
   strict-mode rejected `.tab-bar button[data-tab="jobs"]` resolving to
   two elements) and also left both buttons visually `active` when the
   Jobs tab was open.

3. `tests/e2e/scenarios/test_extensions.py`: align
   `test_onboarding_failed_sse_shows_error_toast_and_reloads_extensions`
   with every other auth-card test in the file — resolve the real
   thread id via `_active_thread_id(page)` before calling
   `_show_auth_card`. `showAuthCard` short-circuits on
   `isCurrentThread(data.thread_id)`, and the synthetic `"thread-fail"`
   id fails that check once `currentThreadId` is populated after
   `go_to_extensions(page)`. The auth card was never rendered, so the
   follow-up `wait_for` for `.auth-card` hit its 5s timeout.

Verified: `cargo test --features libsql --test skill_chain_load_lifecycle`
and `--test skill_setup_marker_lifecycle` pass; `cargo clippy --tests
--features libsql` is clean.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-18 18:37:58 +09:00
Henry Park
82a0b7598a Fix gateway tool output visibility and timing (#2555) 
* Fix gateway tool output visibility

* Address PR review follow-ups

* fix(web): truncate live tool activity previews

* fix(engine): preserve failed tool durations in v2 gateway events

* fix(engine): default missing ActionFailed durations

* style: format scripting executor

* fix(web): keep history tool results aligned with preview

* fix(web): restore persisted tool result parsing

* fix(web): align in-memory turn result/preview with DB path

Live in-memory turns have only the full tool result, not a separately
persisted short preview. Populate `ToolCallInfo.result` from the live
value and leave `result_preview` empty so both paths surface the same
field semantics to the UI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: re-trigger CI

GitHub Actions dropped the Code Style workflow on the prior push.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tests): collapse nested match arm in live_harness

Rust 1.95's stricter clippy::collapsible_match warning trips on the
inner `if` inside the ToolResult arm. Fold the preview check into the
arm's guard to match the same predicate-in-guard style as the arm
above. Fixes the Clippy (all-features) CI failure inherited from
staging.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-18 18:33:35 +09:00
Illia Polosukhin
9fee70906e feat(common): CredentialName + ExtensionName newtypes (PR 1/2) (#2611) 
* feat(common): add CredentialName and ExtensionName newtypes

Introduce typed identifiers for the backend-secret vs user-facing extension
identity split that the Extension/Auth Invariants section of CLAUDE.md
describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity-
confusion bugs with the same shape: a stringly-typed value passed through
multiple layers with each layer meaning a different thing. Newtypes make
each of those a compile error.

This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam
(ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth,
LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate
AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the
remaining extension_name: String fields.

Wire format is unchanged — both newtypes use #[serde(transparent)] so on-
wire and on-disk representations stay plain strings and legacy persisted
rows keep deserializing. Validation runs at explicit construction
(::new / ::try_from / ::from_str), not at deserialize time.

Also adds .claude/rules/types.md codifying the "no stringly-typed
internals" rule.

Regression coverage: 17 new unit tests in identity.rs; existing
auth_manager, router, and gate tests (130+ cases) all pass unchanged.

* fix(common): address PR #2611 review feedback

Four fixes from Copilot, Gemini, and Claude reviews:

- **identity.rs docs**: drop reference to a non-existent `validate()`
  re-validation API. Document that instances represent "passed
  validation at some point in history" rather than "guaranteed valid
  right now" — by design.

- **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token`
  gate path was using `CredentialName::from_trusted` to wrap a value
  read straight out of a tool's JSON output. Tool output is
  external/untrusted; use `CredentialName::new` (validating) with a
  cascade: external → tool name → `from_trusted(tool_name)` as final
  fallback. Closes a credential-name shape-injection vector.

- **canonicalize()**: reorder checks cheapest-first against the trimmed
  slice so invalid inputs reject without allocating a canonicalized
  `String`. `replace('-', "_")` is deferred until after the structural
  checks pass; since `-`/`_` are both one byte, the earlier length
  check stays valid.

- **Remove `Deref<Target = str>`** from identity newtypes, keep
  `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`,
  which is exactly the implicit-conversion pattern these newtypes
  exist to prevent. Callers that had a `&CredentialName` where `&str`
  was expected now write `.as_str()` explicitly. Added a regression
  test for the accessor contract and updated the rule template in
  `.claude/rules/types.md` to document the decision.

Declined one review item (Claude): the remaining `to_string()` calls
inside `IdentityError` variants are on the exception path; the common
invalid-input case no longer allocates twice after the canonicalize
reorder, and errors must carry owned strings so they can escape the
function.

Regression coverage: 5035 lib tests + 18 identity tests (one new —
`explicit_accessors_work`) pass. Zero clippy warnings.
 2026-04-18 18:14:30 +09:00
firat.sertgoz
c74f9555da ci: speed up CI feedback loop (#2566) 
* ci: speed up feedback loop — concurrency, dynamic matrix, path skip, faster staging

- Add cancel-in-progress concurrency groups to 6 workflows (test, code_style,
  e2e, regression-test-check, pr-label-classify, pr-label-scope) so pushes
  to the same branch cancel stale CI runs instead of queuing behind them.

- Collapse test/clippy matrix on PRs from 3 configs to 1 (all-features).
  Full 3-config matrix still runs on staging promotion and push-to-main.
  Cuts PR compilation from ~3x to ~1x.

- Reduce staging-ci poll interval from 60 minutes to 10 minutes, cutting
  worst-case promotion latency by 6x.

- Add path-based skip to test.yml and code_style.yml: a lightweight
  changes-detection job checks if any code files changed (src/, crates/,
  Cargo.*, etc.). Docs-only PRs skip all Rust compilation while the
  rollup job still passes for branch protection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: collapse nested ifs in trace_contains_tool_call match arms

Clippy 1.95 added/tightened `clippy::collapsible_match`. The two
nested `if`s in this helper are equivalent to additional match-arm
guards, which is what the lint suggests. No behavior change.

Inherited from #2268's merge into staging; would have failed
`Clippy (all-features)` on every PR until fixed.

* test: rustfmt struct destructure in collapsed match arm

* ci: drop --benches from clippy invocations

`--benches` pulls in `criterion` (heavy dep) but only covers 2 bench
files in `crates/ironclaw_safety/`. Lints rarely differ in bench code,
and `bench-compile` in test.yml already provides the type-check signal.

Cold-cache impact: ~30s+ saved per Linux/Windows leg (criterion +
plotters + ciborium chain). Warm-cache: marginal but non-zero.

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: ilblackdragon@gmail.com <ilblackdragon@gmail.com>
 2026-04-18 16:46:03 +09:00
Henry Park
12fb3b1437 Fix gateway thread retention and stale in-progress state (#2517) 
* fix(gateway): persist in-progress chat state

* Fix gateway thread retention and stale in-progress state

* Use stable message IDs for gateway in-progress state

* Fix gateway live state review follow-ups

* Fix follow-up PR review comments

* Fix clippy warning in skills catalog

* Fix in-progress review follow-ups

* Fix all-features clippy in TUI renderer

* Fix legacy in-progress reconciliation

* Fix remaining clippy warnings

* Fix gateway review follow-ups

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
 2026-04-18 15:49:24 +09:00
firat.sertgoz
ab276eb94d fix(gateway): time-gate SSE reconnect history reload (#2404) (#2415) 
* fix(gateway): time-gate SSE reconnect history reload to prevent tab-switch flicker (#2404)

Every SSE reconnection unconditionally called loadHistory(), which clears
the entire chat DOM and re-renders all messages — losing scroll position
and causing visible flicker on every browser tab switch. Now tracks when
the SSE connection was lost and only reloads history if disconnected for
more than 10 seconds. Brief reconnects (tab visibility change, transient
network blip) preserve the existing DOM and rely on the "Done without
response" safety net for missed events.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review findings (iteration 1)

Set _sseDisconnectedAt before server restart in E2E test to prevent
flaky timeout when the restart completes in <10s.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-18 15:45:44 +09:00
Illia Polosukhin
e3df3ec4ae feat(skills): setup-marker lifecycle, chain-loading, and live GitHub workflow test (#2268) 
* chore: gitignore live test fixture containing recorded credentials

The github_dev_workflow live test records HTTP exchanges including
the github_token Bearer header. GitHub push protection correctly
blocks this. The fixture is only useful locally for replay; the
test skips gracefully without it.

* test: add live test for github developer workflow

Adds tests/e2e_github_dev_workflow.rs — a multi-turn live/replay test that
drives the developer-assistant + github-workflow skills end-to-end against
a synthetic nearai/ironclaw repository:

  1. Setup           — installs the wf-* mission set (excluding
                       wf-staging-review per the implement-but-don't-
                       auto-merge autonomy contract)
  2. Issue opened    — synthetic github.issue.opened webhook payload
  3. Maintainer LGTM — pr.comment.created from a maintainer
  4. PR review       — non-maintainer review comment
  5. CI failure      — failing check_run
  6. Approval        — maintainer approval; asserts NO merge call ever
                       fires across the whole session
  7. Digest          — status report referencing the issue/PR

Webhook payloads are injected via TestRig::send_message with a
[GITHUB WEBHOOK] frame that matches what a real webhook→channel
adapter would emit. The mission OnSystemEvent firing path is covered
separately by mission.rs unit tests; this test exercises skill
behavior given the right inputs.

Adds two helpers to tests/support/live_harness.rs:
  - trace_contains_tool_call(name, needle)
  - assert_trace_contains_tool_call(name, needle, ctx)

Both scan ToolStarted.detail and ToolResult.preview for case-insensitive
substring matches, so behavior tests can assert *what the agent
actually called* without scraping the recorded trace JSON.

Drive-by cleanups from the extension-lifecycle merge:
  - thread_ops.rs: drop orphaned RecordingStatusChannel + helper that
    came from a dropped extension-lifecycle test variant
  - bridge/router.rs: clippy needless_borrow on PendingGate args
  - skills/mod.rs: SkillManifest no longer has metadata field; add
    requires: GatingRequirements::default() to test fixture
  - cargo fmt fallout in recording.rs / live_mission.rs / trace_llm.rs

The test is #[ignore]-tagged (live tier) and skips gracefully in replay
mode until tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json
is recorded with IRONCLAW_LIVE_TEST=1. Compile coverage is automatic
via the existing test matrix; live execution follows the same pattern
as e2e_live_personas.rs (manual recording + commit fixture).

cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
cargo test --features libsql --test e2e_github_dev_workflow -- --ignored: passes (skips, fixture missing)

* test(harness): add pre-seed secrets + diagnostic activity dump

Three additions to make the github_dev_workflow live test runnable:

1. **TestRigBuilder::with_secret(name, value)** — pre-seed credentials
   in the SecretsStore before the agent starts. The kernel pre-flight
   auth gate fires when a skill with a credential spec activates (e.g.
   the github skill needs github_token); without a stored credential
   the agent gets stuck in 'Authentication required' mode and can't
   make progress. Tests inject a fake/dummy value so the gate is
   satisfied — the test isn't actually hitting the credentialed API.

   Implementation: AppComponents.secrets_store is captured during
   build_all() and any pre-seeded (name, value) pairs are written via
   secrets_store.create() with user_id = config.owner_id. Already-exists
   errors are silenced so the helper is idempotent on seeded DBs.

2. **LiveTestHarnessBuilder::with_secret** — forwards to
   TestRigBuilder::with_secret. Plumbed through both build_live and
   build_replay so the same fixture works in both modes.

3. **dump_activity helper in e2e_github_dev_workflow.rs** — formats
   captured StatusUpdate stream (skill activations + every tool
   started/completed/result) to stderr. Used as a pre-assertion
   diagnostic so failing live runs surface the agent's actual tool
   sequence instead of an opaque panic on a workspace check.

Test relaxations from running this against the real LLM:
- verify_setup_landed accepts either developer-assistant OR
  github-workflow as the active skill (the deterministic selector
  picks based on keyword scoring + token budget; both routes are
  valid since github-workflow owns the mission templates)
- final required-skills check drops developer-assistant in favor of
  github-workflow + github (the orchestrator persona is optional)
- setup turn now pre-seeds github_token via with_secret

cargo check --features libsql --tests: clean

* test: rewrite github_dev_workflow as fully real live integration

Pivots the test from synthetic webhook simulation to a real end-to-end
integration test against the real nearai/ironclaw repo. Per project
owner: 'fully real live tests doing useful work on github repo... test
everything like it's live while recording all interactions to debug
what doesn't work and improve that'.

## Why the rewrite

The previous synthetic-event version injected fake GitHub payloads as
channel messages. With a real github_token in scope, the agent
attempted to fetch the fake issue 99001, got a 404, and helpfully
created 3 real issues + 3 real comments on nearai/ironclaw to
"reconcile" the discrepancy. The synthetic approach didn't surface
realistic failure modes anyway (auth gates, payload format mismatches,
rate limits), so we go all-in on real artifacts.

## New flow (2 turns + real artifact lifecycle)

1. Setup turn — agent installs the wf-* mission set for nearai/ironclaw
2. Test (NOT the agent) creates a real issue via direct REST API with
   the title "[live-test {timestamp}] Add /metrics Prometheus endpoint"
   and a real feature-request body.
3. Triage turn — test asks agent to triage issue #N. Agent reads via
   github skill, generates a plan, posts a real comment back.
4. Verification — test polls api.github.com/issues/N/comments and
   asserts at least one new comment exists since baseline. Comment
   bodies are logged to stderr for human review (the most useful
   debug output for iterating on skill quality).
5. Cleanup — std::panic::catch_unwind wraps the body so cleanup runs
   regardless of pass/fail. Closes the issue with a final "live test
   complete" comment. If cleanup itself fails, the issue URL is
   printed for manual recovery.

## Test infrastructure additions

- TestRig.get_secret(name) — read decrypted secrets back from the
  rig's SecretsStore. Required so the test can read the github_token
  the harness pre-seeded via with_secrets(["github_token"]).
- TestRig captures secrets_store + owner_id from AppComponents during
  build (needed for get_secret).
- github_api submodule inside the test file — direct REST helpers for
  create_issue, list_issue_comments, post_issue_comment, close_issue.
  Uses reqwest directly so the test has guaranteed GitHub access
  regardless of skill selection / tool gating.

## Recording

- LLM trace fixture: tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json (65K)
- Session log: github_dev_workflow_full_loop.log (5.9K)
- Both committed so future runs can replay deterministically without
  hitting real GitHub.

## What's NOT covered yet

Dropped from the previous version (can be added back as follow-ups):
- PR creation flow (agent opens a real PR with a real branch + real
  code change)
- CI failure simulation (would need a real failing CI run)
- Mission OnSystemEvent firing via real webhooks (needs an HTTP
  server registered as a GitHub webhook)
- Maintainer approval flow

This first version validates the most valuable slice: setup → react
to real issue → produce real comment → cleanup. If the agent's
comment quality is good, we expand from here.

cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
Live recording: passed in 85.9s
  - Created issue #2185
  - Agent posted 2 comments (full plan + follow-up)
  - Closed issue #2185

* feat(skills): one-time setup-marker exclusion + rename persona skills to *-setup

The persona orchestrator skills (developer-assistant, ceo-assistant,
trader-assistant, content-creator-assistant) are pure first-time
onboarding flows — their entire body is Steps 1-N of workspace setup,
mission registration, and calibration memory writes. After those steps
run successfully, there is nothing left for the skill to do, but the
deterministic selector kept evaluating them on every conversation
turn, burning ~3000 tokens of activation budget for work already
completed and risking partial re-runs of setup steps.

This commit makes setup skills opt-in to one-time activation:

## Mechanism: setup_marker exclusion

New optional field on ActivationCriteria:

  activation:
    setup_marker: commitments/.developer-setup-complete

Before scoring, the selector caller (Agent::select_active_skills)
collects every distinct setup_marker referenced by loaded skills,
checks the workspace for each via Workspace::exists(), and passes
the set of satisfied markers into prefilter_skills. Any skill whose
marker is in the satisfied set is excluded from scoring entirely
(returns None from the filter map, skipping the score_skill call).

The selector check is opt-in: skills without a setup_marker are
unaffected. Reactive operational skills (commitment-triage,
decision-capture, github, github-workflow, etc.) keep activating
on every matching message as before.

Tests:
- 4 unit tests in crates/ironclaw_skills/src/selector.rs covering
  marker present/absent, marker mismatch, and skill-without-marker
  unaffected paths
- All 152 ironclaw_skills tests pass
- Live e2e_github_dev_workflow run on real nearai/ironclaw passes
  (issue #2186 created, comment posted, closed) in 88s

## Rename: *-assistant → *-setup

Per project owner: 'rename persona skills to -setup skills to make
it explicit they are called once'. The -assistant suffix obscured
the lifecycle — these are not always-on assistants, they are
one-time onboarding wizards.

Renamed directories (via git mv) and updated SKILL.md `name:`
fields:
- skills/ceo-assistant            → skills/ceo-setup
- skills/content-creator-assistant → skills/content-creator-setup
- skills/developer-assistant       → skills/developer-setup
- skills/trader-assistant          → skills/trader-setup

All four now declare `setup_marker: commitments/.<name>-setup-complete`
and have a new final 'Step N: Mark setup complete' instructing the
agent to write the marker via memory_write after confirming setup
with the user. Different personas have different markers so they
remain independently triggerable in separate workspaces.

Cross-references updated:
- tests/e2e_live_personas.rs (4 persona test invocations)
- tests/e2e_github_dev_workflow.rs (doc comments)
- tests/e2e/LIVE_TOOL_FAILURES.md (1 reference)
- crates/ironclaw_skills/src/types.rs (doc comment example)

## Bump: SKILLS_MAX_CONTEXT_TOKENS default 4000 → 6000

The previous default was so tight that a setup skill (3000 tokens)
plus its companion github-workflow (2000) plus github (2000) would
overflow at 7000. Reactive operational skills like
commitment-triage, decision-capture, tech-debt-tracker often got
budget-evicted. With setup skills now excluded after onboarding,
the freed budget plus the bump to 6000 lets the most useful
combinations fit comfortably (e.g. github-workflow + github +
product-prioritization is now active in the live recording, where
previously product-prioritization would have been evicted).

## Plumbing changes

- ActivationCriteria gains pub setup_marker: Option<String>
  (#[serde(default)], so existing skills are unaffected)
- prefilter_skills signature gains
  &satisfied_setup_markers: &HashSet<String> (caller passes empty
  set to disable filtering — used by all existing tests via the
  prefilter_no_markers wrapper)
- Agent::select_active_skills is now async — it needs to
  Workspace::exists() each marker. dispatcher.rs caller updated
  to .await. Snapshots the skill list under the read lock then
  drops the guard before any await to avoid holding a poisonable
  RwLock across an await point.

cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
cargo test -p ironclaw_skills: 152 passed
Live e2e_github_dev_workflow run: passes (88s)

* feat(skills): chain-load companions + v2 marker exclusion + commitment-setup marker

Three orthogonal follow-ups to the skill lifecycle work.

## 1. Chain-loading via requires.skills (v1 Rust + v2 Python)

When a parent skill is selected by the scorer, its requires.skills
companions are now automatically loaded, bypassing the score filter.
Persona/bundle skills like developer-setup can finally work as
designed: the orchestrator declares which operational skills it
delegates to, and selecting the orchestrator pulls them all in.

- **v1 Rust** (crates/ironclaw_skills/src/selector.rs): extracted
  skill_token_cost() and try_select() helpers used by both the
  scored-selection loop and the new chain-loading pass. Companions
  consume the same budget and respect max_candidates. Non-transitive
  (depth 1 only) to keep behavior predictable.
- **v2 Python** (crates/ironclaw_engine/orchestrator/default.py):
  select_skills() gains an inline chain-loading pass that mirrors
  the Rust logic. Uses a name-indexed lookup built from the skill
  list passed in by handle_list_skills. No closure-over-outer-var
  tricks that Monty would reject — the inner try-add is inlined.

7 chain-load unit tests in selector.rs covering: pulls in
companions, skipped when parent not selected, respects budget,
skips companion with satisfied marker, non-transitive (depth 2
not pulled), missing companion silent, dedup across parents.

## 2. v2 setup_marker exclusion

The v2 engine's Python orchestrator handles skill selection via
handle_list_skills (Rust) -> select_skills (Python). Since
handle_list_skills already has the full project doc list in scope,
we filter there: any skill whose metadata.activation.setup_marker
is in the set of existing doc titles gets excluded before the
Python orchestrator ever sees it. Zero extra store calls — we
reuse the existing list_memory_docs_with_shared result to build
an O(1) title set.

This is the v2 parity of the v1 satisfied_setup_markers parameter
threaded through prefilter_skills. Both paths now implement the
same rule: a one-time setup skill whose marker file has been
written has finished its job and should not keep burning
activation budget.

## 3. commitment-setup gets a setup_marker

commitment-setup writes commitments/README.md as its first step,
so the marker is automatically set after a successful first run.
Added:
  activation:
    setup_marker: commitments/README.md

To re-trigger (e.g. migrate to a new schema), delete README.md
first. project-setup was NOT given a marker — it's per-repo,
invoked repeatedly, not a singleton (each call creates a new
projects/<owner>-<repo>/project.md).

## 4. Lifecycle integration test

tests/skill_setup_marker_lifecycle.rs drives a real agent turn
through the v1 selector pipeline (Agent::select_active_skills ->
Workspace::exists -> prefilter_skills) to verify that a setup
skill:
  Phase 1: activates on the first matching message (marker absent)
  Phase 2: marker file is written via workspace.write()
  Phase 3: is excluded on the second matching message

The test asserts on the captured LLM system prompt content (via
rig.captured_llm_requests) rather than on StatusUpdate events so
it's agnostic to v1/v2 path differences in how skill activations
are announced. The skill's body contains a distinctive marker
string (LIFECYCLE-TEST-SKILL-BODY-MARKER-Z7Q) — if the skill was
selected, that string appears in the system prompt; if excluded,
it doesn't.

Cover matrix after this commit:
- v1 selector: 35 unit tests + 4 setup-marker tests + 7 chain-load tests
- v2 handle_list_skills marker exclusion: 1 integration test (lifecycle)
  plus structural verification via cargo check (the filter uses the
  existing list_memory_docs API, no new store calls to test)
- v2 Python select_skills chain-load: covered by the v1 unit tests
  through shared semantic contract (both paths mirror the same
  algorithm); a direct Python-level test would require spinning up
  the Monty interpreter which is out of scope for this session.

Verification:
  cargo test -p ironclaw_skills --lib:   159 passed
  cargo test -p ironclaw_engine:         304 passed
  cargo test --features libsql --test skill_setup_marker_lifecycle: 1 passed
  cargo clippy --features libsql --tests --all-targets -- -D warnings: clean

* feat(skills): carry requires through v1→v2 migration + chain-load test

V2SkillMetadata was missing the `requires` field entirely, so the
v1→v2 skill migration silently dropped `requires.skills` and the
chain-loading code I added to the v2 Python orchestrator in the
previous commit was effectively dead code — it always read an empty
companion list.

This was caught while writing an end-to-end chain-load test: the v1
test (through the Rust selector) passes, the v2 test (through the
Python orchestrator) was failing in a way that only made sense if
the companion metadata never reached Python. Inspection confirmed
`V2SkillMetadata` had no `requires` field, only `activation`.

## Fix

1. `V2SkillMetadata` gains `pub requires: GatingRequirements` with
   `#[serde(default)]` for backwards compatibility (legacy
   MemoryDocs in existing databases deserialize with an empty
   `requires`).
2. `src/bridge/skill_migration.rs::v1_skill_to_memory_doc` now
   copies `skill.manifest.requires.clone()` into the new field.
3. Four other explicit `V2SkillMetadata { ... }` literal
   constructions updated with `requires: Default::default()`:
   - `crates/ironclaw_engine/src/memory/skill_tracker.rs` (test helper)
   - `crates/ironclaw_engine/src/runtime/mission.rs` (test helper)
   - `crates/ironclaw_skills/src/v2.rs` (serde roundtrip test)
   - `tests/engine_v2_skill_codeact.rs` (test fixture)

## New test: tests/skill_chain_load_lifecycle.rs

End-to-end lifecycle test for chain-loading. Writes three skills to
a tempdir:
- `parent-setup-test` — scored by a distinctive keyword, declares
  two companions via `requires.skills`
- `companion-one-test` / `companion-two-test` — zero-scoring on
  their own (keywords deliberately don't match)

Each skill body carries a distinctive marker string
(`CHAIN-LOAD-PARENT-BODY-J4V`, `CHAIN-LOAD-COMPANION-ONE-K5W`,
`CHAIN-LOAD-COMPANION-TWO-L6X`) that the test greps for in the
captured LLM system prompt via `rig.captured_llm_requests()`. If a
marker is present, the skill was injected into the prompt; if
absent, it wasn't.

Two test variants:
- **v1** (default rig, Rust selector path): **PASSES**. Proves the
  chain-loading pass in `prefilter_skills` correctly pulls in both
  companions despite their zero individual scores.
- **v2** (with_engine_v2, Python orchestrator path):
  **`#[ignore]`d** with a detailed explanation. The v2 engine runs
  a Python orchestrator that makes multiple LLM calls per user
  message, but the default TestRig uses a single-turn TraceLlm that
  exhausts after the first call — observing skill injection through
  the v2 path needs a multi-turn TraceLlm harness or a dedicated v2
  skill test rig. The structural wiring for v2 chain-loading
  (V2SkillMetadata.requires + skill_migration copy + Python
  select_skills chain-load pass) compiles and passes the 304-test
  engine suite, so this is a test-harness gap, not a code gap.

When the multi-turn harness exists, flipping `#[ignore]` on the v2
test will exercise the full path.

Verification:
  cargo test -p ironclaw_skills --lib:                159 passed
  cargo test -p ironclaw_engine --lib:                304 passed
  cargo test --features libsql --test skill_chain_load_lifecycle
    -- --test-threads=1:                              1 passed, 1 ignored
  cargo test --features libsql --test skill_setup_marker_lifecycle
    -- --test-threads=1:                              1 passed
  cargo clippy --features libsql --tests --all-targets -- -D warnings: clean

Also includes an updated fixture recording from the last live
`e2e_github_dev_workflow` run (issue #2204, agent posted 2 comments,
cleanup closed it). No functional difference; committed for
completeness since the fixture was modified on disk by the live run
and the test is hermetic in replay mode.

* fix: adapt thread_ops test to staging's test helper API

Use make_test_agent_with_status_channel instead of removed
make_thread_ops_test_agent, StdMutex instead of TokioMutex,
and fix String comparison direction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: cargo fmt

* fix: remove dead try_add function and stale comments in Python orchestrator

Addresses PR #2268 review feedback: the try_add closure was defined but
never called since the logic was inlined for Monty compatibility.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: reconcile test harness after staging merge

Restore our branch's test helpers (SessionTurn, finish_turns_strict,
with_skills_dir, loaded_skill_names, active_skill_names, etc.) that
staging removed, while incorporating staging's new features
(record_trace, with_no_trace_recording, secrets_store/owner_id
accessors). Bridge the API gap with finish_turns_simple for tests
using staging's (String, Vec<String>) tuple convention.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR #2268 review feedback

- live_harness: replace panic with graceful TestMode::Skipped when
  record_trace=false in replay mode; update e2e_live callers to check
  mode() != Live instead of == Replay
- test_rig: match SecretError::NotFound explicitly in get_secret(),
  return None silently instead of logging expected misses
- test_rig: replace brittle "already exists" string matching in
  pre-seed loop with get_decrypted existence check before create
- default.py: align max_context_tokens fallback from 1000 to 2000
  to match Rust ActivationCriteria default (both parent and companion)
- e2e_builtin_tool_coverage: fix routine_create_list using hardcoded
  "test-user" instead of rig.owner_id() (broke when .with_skills()
  changed channel user to config owner_id)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR #2268 review feedback (round 2)

1. Fix memory_write `path:` → `target:` in all 4 setup skill completion
   markers (developer, ceo, content-creator, trader). The `memory_write`
   tool reads `target`, not `path`, so markers were never written to the
   correct location.

2. Add setup_marker validation in enforce_limits(): max 256 chars, reject
   `..` path traversal. Prevents untrusted skills from abusing markers.

3. Fix v2 Python skill budget: default 4000 → 6000 to match v1 Rust
   config. Also port the approx_tokens > declared * 2 sanity check from
   Rust to prevent budget bypass via low max_context_tokens declarations.

4. Reorder developer-setup companion skills to put github/github-workflow
   first (critical for setup) and fix misleading budget comment in config.

5. Move AssertUnwindSafe cleanup guard in e2e GitHub test to wrap
   everything after create_issue, preventing orphaned issues on panic.

6. Scope workspace in select_active_skills to the requesting user_id so
   multi-user channels check the correct user's setup marker state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove duplicate skills_dir field from LiveTestHarnessBuilder

Both sides of the merge added the same field, resulting in a duplicate
declaration that failed compilation in test targets.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address CI failures and Copilot review feedback

1. Fix formatting (cargo fmt).

2. Filter existing_titles to non-Skill docs in v2 orchestrator so setup
   markers don't collide with skill doc titles of the same name.

3. Fix stale doc comment in types.rs (commitments/README.md →
   commitments/.developer-setup-complete).

4. Fix misleading comment on v2 requires field — the full
   GatingRequirements struct is preserved, not just the companion list.

5. Match SecretError::NotFound explicitly in test_rig pre-seed loop
   instead of catching all errors — other errors (DB, crypto) now
   surface instead of triggering a blind create.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-18 14:32:52 +09:00
Illia Polosukhin
6a28a4c861 fix(test): case-insensitive tool_search description assertion (#2608) 
* fix(test): case-insensitive assertion in tool_search description

The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for
a lowercase "use the `message` tool ..." substring, but #2515 capitalized
the first word in src/tools/builtin/extension_tools.rs:110. The local
unit test in that file was updated; this e2e test was missed, breaking
the Run Tests job on main and blocking release-plz PR #2606.

Normalize to lowercase before substring match so a future copy-edit
doesn't silently break CI again.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Update tests/e2e_builtin_tool_coverage.rs

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
 2026-04-18 13:29:38 +09:00
Alex Rozgo
dcca2187fa fix(telegram): emit chat_type metadata for group-safe prompt behavior (#2513) 2026-04-18 13:07:21 +09:00
firat.sertgoz
275c3c2198 fix(e2e): resolve 12 E2E test failures across routines and features groups (#2503) 
* fix(e2e): resolve 12 E2E test failures across routines and features groups

Three root causes fixed:

1. Read-only thread regression (9 routines failures): ed2d6dc3 changed
   loadThreads() to follow the server's active_thread on page load. When
   a prior test creates an HTTP-channel thread, new browser pages auto-switch
   to it — disabling the chat input. Fix: skip read-only channel threads
   when auto-switching; also restore placeholder text in enableChatInput().

2. Tool approval state contamination (2 features failures):
   test_chat_reply_always permanently auto-approves the http tool for the
   session. Tests running after it that need the approval gate to fire
   (test_text_approval_resolves_real_tool_call,
   test_slash_approve_is_thread_scoped_api) find it pre-approved. Fix:
   reorder so real-approval-gate tests run before the "always" test.

3. Silent /approve on idle thread (1 features failure):
   /approve is always routed as an approval command. process_approval()
   returns an empty message when no approval is pending, producing
   HandleOutcome::NoResponse. send_chat_and_wait_for_terminal_message
   times out waiting for a visible response. Fix: return "No pending
   approval for this thread." so the response is visible.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gateway): address henrypark133 review — readonly thread regression test (#2503)

- Remove redundant `!activeThread` guard in loadThreads() (threads.some()
  already guarantees existence), use `const` instead of `var`
- Add E2E browser regression test: sets an external-channel (HTTP) thread
  as the server active_thread, reloads without a hash, and asserts the UI
  falls back to the assistant thread with chat input enabled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-18 12:52:28 +09:00
Nige
ce98cf2cd2 fix(status): report active WASM channels accurately (#2420) 
* fix(cli): report active wasm channels in status

* Update src/cli/status.rs

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
 2026-04-18 12:37:58 +09:00
Henry Park
0dc0fb495d test: run MCP lifecycle trace in gateway mode (#2595) 
* test: run MCP lifecycle trace in gateway mode

* test: isolate MCP lifecycle oauth env
 2026-04-17 15:20:02 -07:00
Henry Park
2536835436 Fix gateway auth/pairing flow handling (#2594) 2026-04-17 15:11:56 -07:00
Illia Polosukhin
ab8d64cbfc feat: new-project skill and template ref resolution for parallel tool calls (#2353) 
* feat(gateway): project metrics dashboard, mission scheduling UI, and new-project skill

Adds project metrics types, mission cadence scheduling via gateway,
and a /new-project skill for creating autonomous projects with goals,
metrics, and missions. Includes gateway frontend enhancements for
project views with metrics and goal tracking.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(engine): resolve template refs in parallel tool calls and rewrite new-project skill

Two fixes from trace analysis (trace_20260411T133641.json):

1. Skill rewrite: new-project skill now instructs the model to use
   memory_write + mission_create directly instead of referencing
   nonexistent project_create/project_update tools. Includes goals
   and metrics when appropriate. Instructs sequential execution.

2. Template ref resolution: some OpenAI-format models (e.g. Qwen)
   emit {{call_id.field}} references in parallel tool call arguments.
   Added resolution pass in LlmBridgeAdapter that scans ActionCall
   parameters for these patterns and resolves them from prior tool
   results in the conversation history.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add project detail page screenshot test

Playwright test that seeds mock project data via page.route() API
interception, navigates to the Projects tab, drills into a project,
and captures a screenshot showing goals, missions, and activity.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* docs: add project detail screenshot for PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review — remove project tools, fix IDOR, scope widgets, add tests

- Remove project_create/project_update/project_list tools and capability
  registration (skill uses memory_write + mission_create only)
- Add ownership check on mission_create project_id override to prevent IDOR
- Reject non-UUID project_id values explicitly instead of silent fallback
- Add goals field to ProjectOverviewEntry so frontend drill-in renders them
- Propagate store errors in overview instead of unwrap_or_default masking failures
- Scope project widget CSS server-side via scope_css (prevents style leakage)
- Fix template ref doc comment to match partial resolution semantics
- Fix E2E mock widget response shape (bare array, not wrapped object)
- Call crBackToOverview() on tab switch to tear down project widgets
- Add caller-level test for template ref resolution through LlmBridgeAdapter
- Clean up stale cargo-deny advisory ignores, add RUSTSEC-2026-0097 (rand)
- Run cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: resolve project slugs in mission_create, fix widget CSS comments

- mission_create now accepts project name/slug (not just UUID) by matching
  against the user's projects — fixes the skill's slug-based project_id
- Fix misleading CSS comment in app.js (CSS is scoped server-side)
- Fix style variable hoisting issue in widget mounting
- Log workspace.list() errors instead of silently swallowing them

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review round 3 — slug matching, template injection, N+1 queries

- Remove over-broad `starts_with` slug prefix matching in mission_create
  project_id resolution — require exact name/slug match only (serrrfirat)
- Fix slug generation inconsistency: frontend.rs now uses
  is_ascii_alphanumeric() matching effect_adapter.rs (serrrfirat)
- Prevent second-order template injection: resolve_template_refs now
  advances past resolved content instead of re-scanning from position 0,
  and skips unresolvable refs instead of breaking (serrrfirat)
- Parallelize N+1 overview queries: per-project thread/mission fetches
  now use tokio::try_join! + futures::try_join_all (serrrfirat, Copilot)
- Add two new security tests for template ref resolution

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-18 01:37:42 +09:00
firat.sertgoz
fbb904116c fix(web): prevent user messages from vanishing on thread switch (#2409) (#2498) 
* fix(web): prevent user messages from vanishing during safety-pipeline window (#2409)

When loadHistory() re-renders the chat (thread switch, SSE reconnect,
page reload), user messages that haven't been persisted yet disappear
because the agent loop persists them after safety checks (100ms-1s
delay). This fix tracks pending messages client-side and re-injects
them into the DOM when loadHistory() doesn't find them in the DB yet.

- Add _pendingUserMessages Map with 60s TTL
- Record pending messages in sendMessage() before the fetch call
- Clear pending entries when SSE events confirm agent processing
- Re-inject non-persisted pending messages in loadHistory() fresh path
- Suppress welcome card when pending messages exist

Purely frontend fix — no backend changes, no safety pipeline bypass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add Playwright tests for pending message persistence (#2409)

Six scenarios covering the frontend fix for disappearing user messages:
- User message visible immediately after send (optimistic display)
- Pending message survives SSE reconnect (re-injected by loadHistory)
- Pending messages cleared after agent response (no stale entries)
- No duplicates when DB already has the message
- Welcome card suppressed when pending messages exist
- Full round-trip message survives page reload (DB persistence)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(e2e): use domcontentloaded for reload test — SSE blocks networkidle

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address review — remove SSE early-clear race, use frequency map for pending dedup (#2498)

Remove _pendingUserMessages.delete() from response/tool_started/stream_chunk
SSE handlers to prevent race condition when user sends multiple messages
in quick succession. Replace Set-based dedup in loadHistory with a
frequency map so duplicate-content messages ("ok", "ok") are tracked
correctly. Simplify welcome-card guard using hoisted freshPending.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): clear pending entry on turn completion — address henrypark133 review (#2498)

* fix(web): address review — remove pending on send fail, Map for dedup, improve reconnect test (#2498)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove unused imports in pending message test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* ci: retrigger checks against updated staging base

* fix(web): preserve images in pending messages, harden tests (#2498)

Address remaining review feedback:
- Capture attached image data URLs in optimistic display and in the
  _pendingUserMessages entry so a thread switch / SSE reconnect re-injects
  thumbnails alongside the text instead of just an "(images attached)"
  placeholder.
- Rewrite the SSE-reconnect test to drive the real production path: stub
  apiFetch so /api/chat/send hangs, send via the real UI, force a
  reconnect, and assert the message survives — instead of manually
  pre-populating the pending map.
- Add coverage for the .catch() cleanup branch in sendMessage so a
  rejected /api/chat/send leaves _pendingUserMessages clean.
- Add a FIFO-assumption comment on the response-handler shift() and
  drop the leading underscore on the function-local `pending` (the
  underscore convention in this file is for module-level state).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-18 01:26:35 +09:00
firat.sertgoz
22cd378461 fix(safety): add inbound secret scanning to engine v2 path (#2494) 
* fix(safety): add inbound secret scanning to engine v2 path (#2491)

The v2 engine path (`handle_with_engine_inner` in `bridge/router.rs`)
forwarded user messages directly to the conversation manager without
any safety checks. This allowed secrets (API keys, Slack tokens, AWS
credentials, etc.) pasted in chat to reach the LLM and be permanently
stored in conversation history.

Add the same three safety checks that the v1 path (`thread_ops.rs`)
already enforces: `validate_input`, `check_policy`, and
`scan_inbound_for_secrets`. Messages containing detected secrets are
now rejected with a user-facing warning before reaching the engine.

Includes a regression test exercising Slack bot tokens and OpenAI keys
through the v2 code path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style(safety): fix rustfmt formatting in secret scan test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(safety): fix OpenAI key test — payload too short for regex (#2494)

The mock OpenAI key `sk-abc123def456ghi789` had only 19 chars after
the prefix, but the leak detector regex requires 20+. Extended the
key and added a specific assertion matching the Slack token check.

Addresses gemini-code-assist review feedback.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore(deps): ignore RUSTSEC-2026-0099 webpki advisory

Wildcard name constraint bypass in rustls-webpki 0.102.8, pinned by
the libsql transitive dependency chain. Same root cause as the
already-ignored RUSTSEC-2026-0049.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: minor comment tweak to retrigger CI

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(ci): resolve clippy and fmt errors

Remove useless .into_iter() in catalog.rs and fix rustfmt style in e2e_attachments.rs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(bridge): use BridgeOutcome instead of Option<String> in safety checks

The inbound safety scanning code was written against the old
Option<String> return type, but handle_with_engine_inner now returns
BridgeOutcome. Replace Ok(Some(...)) with Ok(BridgeOutcome::Respond(...))
and update tests to match on the enum variants.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
 2026-04-18 01:25:26 +09:00
Coffee
79ad2e38ae fix(gateway): align historical/live tool call cards and preserve tool call correlation (#2182) 
* Align history tool cards with live activity cards

* Make tool cards keyboard accessible

* Preserve tool call IDs in web event handling

* fix: chevron icon size

* Guard response call IDs against unknown tool outputs

---------

Co-authored-by: italic-jinxin <106428113+italic-jinxin@users.noreply.github.com>
 2026-04-17 18:02:09 +03:00
firat.sertgoz
27d53f5153 docs(skills): code-review v2 + GitHub endpoint fixes + minor text updates (#2528) 
* feat(skills): paranoid-architect code-review skill v2

Rewrite the code-review skill from a 6-bullet checklist into a
paranoid-architect workflow that handles both local diffs and GitHub
PRs end-to-end:

- Two input shapes: local `git diff` or `owner/repo N` /
  `github.com/.../pull/N` URLs.
- Step 1 wraps GitHub fetches in `async def` + `FINAL(await ...)` to
  avoid the closure-capture quirk that kept tripping LLMs (see the
  paired codeact preamble update); reads metadata, diff, and files
  via three sequential awaits instead of `asyncio.gather`.
- Step 2 reads each changed file in full (raw media type, no base64
  module needed) so reviews account for surrounding context.
- Step 3 runs the change through six lenses: correctness, edge cases,
  security (with a real adversarial checklist), test coverage, docs,
  architecture.
- Step 4 renders findings as a severity table and asks which to post.
- Step 5 posts line-level comments via the PR comments endpoint with
  the captured head SHA, falling back to issue comments for
  multi-file findings.

Bumps `requires.skills` to include `github` so the activation pulls
in the GitHub API recipes via the chain-loader.

Adds a live e2e test (`e2e_live_code_review.rs`) plus a recorded
trace fixture (PR #2483) so the workflow is replayable without
hitting GitHub.

* docs(github): clarify search endpoints, response envelope, @me queries

LLMs kept inventing a `search_issues` action and looping over
`/repos/{owner}/{repo}/pulls` for "my PRs" queries. Clarify the
GitHub tool surface in three places:

- `tools-src/github/src/lib.rs` and `registry/tools/github.json`:
  enumerate the three real search actions and call out that
  `search_issues_pull_requests` covers both. Add the canonical
  `is:pr author:@me sort:updated-desc` recipe for cross-repo "my PRs".

- `skills/github/SKILL.md`: add an "Authenticated User & Cross-Repo
  Queries" section with copy-paste recipes for `@me`, the search
  endpoints with proper URL encoding, and the response-envelope
  contract (`body` is parsed JSON for application/json, raw `str` for
  diff endpoints — never call `json.loads()` on it, never write
  `.get("body", body)` as a fallback).

* fix: resolve CI failures — clippy useless_conversion + missing test harness methods

- Remove `.into_iter()` on `details` in catalog.rs (clippy::useless_conversion)
- Add `with_skills_dir` to `LiveTestHarnessBuilder` for e2e_live_code_review test
- Add `active_skill_names` to `TestRig` extracting from SkillActivated status events

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(skills): address zmanian + gemini review — URL encoding, multi-line comments, description trimming (#2528)

- URL-encode file paths in GitHub API content URLs
- Add start_line/start_side to multi-line comment example
- Add 'locally' keyword override for mode detection
- Trim overly long schema descriptions
- Remove duplicated /search/issues note from Common Mistakes
- Fetch PR title from trace fixture instead of hard-coding

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(test): propagate skills_dir into TestRig config (#2528)

LiveTestHarnessBuilder::with_skills_dir() stored a PathBuf but only
used it as an is_some() flag — the actual SkillRegistry always pointed
at an empty temp directory. Now the stored path flows through
TestRigBuilder::with_skills_dir() into config.skills.local_dir and
the SkillRegistry constructor.

Also generalizes the hardcoded nearai/ironclaw repo name in the
github skill's response-handling example to {owner}/{repo}.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-17 23:11:59 +09:00
firat.sertgoz
a6443dd450 fix: resolve staging CI test failures blocking promotion (#2574) 
* fix: resolve 3 categories of staging CI test failures

1. pending_gate_extension_name now extracts extension name from
   tool_install/tool_activate/tool_auth parameters even when
   auth_manager is unavailable, matching the AuthManager logic and
   returning "telegram" instead of "telegram_bot_token".

2. Updated CLI help snapshots to match new onboard/config/doctor/login
   descriptions and the addition of the profile subcommand.

3. Relaxed E2E pairing approve assertions to check only the code field,
   accommodating the new optional thread_id the frontend now sends.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address ilblackdragon review — ensure auth_manager always available, remove fallback duplication (#2574)

- Remove inline fallback that duplicated AuthManager::resolve_extension_name_for_auth_flow() logic
  in pending_gate_extension_name(); auth_manager is now always wired in tests via a minimal
  InMemorySecretsStore-backed AuthManager
- Fix trim inconsistency in AuthManager::resolve_extension_name_for_auth_flow() where the
  predicate trimmed whitespace but the return value did not

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: allow clippy::too_many_arguments on register_startup_channels

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-17 22:50:59 +09:00
standardtoaster
89b350ec56 feat(engine): execution obligation -- require tool attempt on explicit user commands (#2539) 
* feat(engine): execution obligation for v2 — require tool attempt on explicit user commands

When a user explicitly asks the engine to execute something ("run the
tests", "fetch the data", "please check the logs"), the v2 engine now
requires the model to attempt at least one tool/action call before
accepting a plain-text response.

Adds `user_signals_execution_intent()` heuristic in reasoning.rs that
detects imperative execution phrases. The router sets
`require_action_attempt = true` on ThreadConfig when detected. The
Python orchestrator enforces this by nudging the model if it responds
with text-only without attempting any action.

The obligation resolves when the model enters a code/action path
(before execution), preventing retry loops on approval gates. The
obligation nudge and tool-intent nudge are mutually exclusive to
avoid double-nudging.

Closes #2447

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review feedback on execution obligation

- Fix nudge interaction bug: move obligation check before
  consecutive_nudges reset so tool-intent nudge exhaustion
  can't trick the mutual exclusion guard
- Add available-actions guard: obligation only fires when
  __get_actions__() returns tools, preventing useless nudges
  when no tools are loaded
- Remove "check the " from heuristic: too broad for personal
  assistant context ("check the calendar" is a query, not
  an execution command)
- Add exhaustion e2e test: model refuses 3 times, hits
  max_action_requirement_nudges, text accepted as final
  (proves the feature terminates)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: remove useless .into_iter() to satisfy clippy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(engine): enforce execution obligation on follow-up messages

The obligation nudge only fired when spawning a new thread (where
ThreadConfig.require_action_attempt was set). Follow-up messages
injected into a running thread or resuming a suspended thread used
the original thread config, so "run the tests" in turn 2+ was
silently ignored.

Fix: detect execution intent per-message in the Python orchestrator
rather than only from thread config. Two paths covered:

- inject (running thread): check injected message text for intent
  keywords, enable obligation and reset state if detected
- resume (suspended thread): check the last user message in the
  initial context on run_loop startup

Adds signals_execution_intent() to default.py (ported from Rust
user_signals_execution_intent), plus a multi-turn e2e test that
verifies the inject path: turn 1 is conversational (no obligation),
turn 2 says "run the echo tool" and the nudge fires.

Closes review feedback from henrypark133 on #2539.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: fix doc comment placement on strip_code_blocks

The doc comment for strip_code_blocks was incorrectly placed above
user_signals_execution_intent. Moved it to its own function and
cleaned up the user_signals_execution_intent doc.

Addresses gemini review feedback on #2539.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(engine): reset obligation state on resume + add gate resume test

The context-based obligation check at run_loop startup did not reset
_obligation_resolved and _obligation_nudge_count from persisted state.
On resume, a stale "resolved" flag from a prior run would silently
suppress the new obligation. Fixed by resetting both state flags when
execution intent is detected from context.

Also: the multi-turn e2e test (followup_inject) was mislabeled -- the
test rig processes messages sequentially so turn 2 always spawns a new
thread (the already-working spawn path). Renamed to reflect what it
actually tests.

Added a proper gate-based resume test in engine_v2_gate_integration:
1. Thread spawns with no execution intent in goal
2. Tool call hits a gate, thread enters Waiting
3. Resume with "run the echo tool" (execution intent)
4. Obligation nudge fires, echo tool called
This tests the real resume path through ThreadManager.resume_thread
where ThreadConfig.require_action_attempt was never set.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-17 18:31:36 +09:00
Henry Park
4308628301 Refine v2 web activity shell (#2560) 
* Refine v2 web activity shell

* Polish v2 web activity shell
 2026-04-17 08:34:05 +03:00
Henry Park
3ac8e5f7e8 Unify gateway onboarding, auth gates, and pairing flows (#2515) 
* fix(channels): wire up pairing approval, polling restart, and onboarding state

The Telegram channel setup flow via the gateway was broken end-to-end.
Four interconnected bugs prevented pairing/ownership from completing:

1. pairing_approve_handler only wrote to channel_identities DB — the
   running WasmChannel's owner_actor_id was never updated, so the
   owner was never recognized and broadcast metadata was never stored.

2. refresh_active_channel() re-ran on_start() but never called
   ensure_polling(), leaving polling in a stale state on repeated
   tool_activate calls and causing Telegram 409 conflicts.

3. activate_wasm_channel() had a TOCTOU race on active_channel_names
   that allowed duplicate polling loops, and hot_add() didn't await
   old polling task termination.

4. onboarding_state was always None in extension API responses and
   PairingRequired SSE was never emitted, so the frontend could
   never render the pairing card.

Changes:
- approve_pairing (DB trait + both backends) now returns external_id
- WasmChannel.owner_actor_id wrapped in RwLock with set_owner_actor_id()
- ExtensionManager.complete_pairing_approval() orchestrates: persist
  owner_id → update running channel → restart polling
- pairing_approve_handler calls complete_pairing_approval and emits
  PairingCompleted SSE (scoped to approving user)
- refresh_active_channel() calls ensure_polling() and syncs owner
- Per-channel activation mutex prevents TOCTOU race
- hot_add() drops write lock before awaiting shutdown
- Extension list handlers populate onboarding_state when Pairing
- derive_onboarding() helper in handlers/extensions.rs
- Regression tests for derive_onboarding and resolve_message_scope

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(bridge): eliminate dual card + text emission for gate-paused flows

When the v2 engine hits a gate-paused state (approval needed, auth
required), the web gateway was sending BOTH an interactive card (via
send_status → SSE) AND a redundant text message (via AppEvent::Response).
Users saw a duplicate prompt.

Root cause: v2 bridge functions returned Ok(Some(text)) for gate-paused
outcomes, which mapped via from_legacy to HandleOutcome::Respond — sending
both the card and the text. The v1 path correctly used HandleOutcome::Pending.

Fix:
- Gate-paused paths in router.rs now return Ok(None) instead of text
- New bridge_to_outcome() checks has_any_pending_gate() after each v2
  bridge call — if a gate exists, returns Pending (suppresses text + Done)
- New from_bridge() maps None → NoResponse (not Shutdown) for v2 paths
- Removed pending_gate_prompt_message() — the function that generated
  the duplicate text
- notify_pending_gate() no longer emits GateRequired SSE directly
  (redundant with send_pending_gate_status per-channel routing)
- Updated 3 tests to assert None return + StatusUpdate delivery

Each channel renders the approval/auth card natively via send_status:
web → SSE card, TUI → widget, relay → buttons.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review comments

- bridge_to_outcome: only return Pending when handler returned None
  (preserves legitimate text responses for ambiguous gate messages)
- process_emitted_messages: clone owner_actor_id out of read lock
  before awaiting resolve_message_scope_with_pairing
- Normalize channel_name to lowercase in complete_pairing_approval
  and pairing_approve_handler for consistent webhook/store lookups
- cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address self-review — BridgeOutcome enum, ExternalId newtype, pairing extraction

- Replace Option<String> bridge handler returns with typed BridgeOutcome
  enum (Respond/NoResponse/Pending), eliminating post-hoc has_any_pending_gate
  query and the None→NoResponse mapping that swallowed v2 shutdown signals
- Add ExternalId newtype for approve_pairing return (was bare String)
- Fix noop PairingStore::approve to return NotFound instead of Ok("")
- Extract pairing approval orchestration to src/pairing/approval.rs
- Clone RwLock<owner_actor_id> before awaiting in respond()
- Downgrade warn! to debug! in pairing handlers (TUI logging rule)
- Gate TELEGRAM_TEST_API_BASE_ENV const behind cfg(test/debug_assertions)
- Remove hardcoded Telegram auth instructions; use capabilities prompt
- Fix unused mut receiver in test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(channels): remove dead Telegram verification flow, consolidate to generic pairing

The Telegram-specific verification challenge (/start CODE deep link flow)
blocked the generic pairing flow from ever running — configure() returned
early with activated:false when the challenge was pending, so the channel
never started polling and users couldn't generate pairing codes.

Removed ~1200 lines:
- TelegramBindingResult, TelegramBindingData, TelegramOwnerBindingState,
  TelegramVerificationMeta, PendingTelegramVerificationChallenge types
- configure_telegram_binding, resolve_telegram_binding,
  issue_telegram_verification_challenge, notify_telegram_owner_verified
  and all Telegram API response types (getUpdates polling loop, etc.)
- ConfigureResult.verification field + VerificationChallenge re-export
- All verification-related test fixtures and 6 test functions
- Dead RecordingChannel test helper, unused set_channel_owner_id method
- Gated send_telegram_text_message + helpers behind cfg(test)

Replaced with:
- validate_telegram_token() — lightweight getMe call for token validation
  + bot_username extraction (persisted for mention detection)
- All channels now follow: credentials → validate → activate → pairing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(channels): broadcast PairingRequired SSE after activation in pairing mode

After a channel activates with no owner binding, broadcast a per-user
PairingRequired SSE event so the web UI shows the pairing card without
requiring a manual refresh. Also populate pairing_required, onboarding_state,
and onboarding fields on ConfigureResult so callers know the channel
needs pairing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(agent): don't persist auth instructions as turn response

When a tool triggers an auth gate (awaiting_token), the dispatcher
already sends an AuthRequired card and puts the thread in auth mode.
The thread_ops handler was then calling complete_turn(&instructions)
which overwrote auth mode back to Idle AND persisted the auth prompt
("Enter your Telegram Bot API token...") as the turn response — rendering
a redundant text bubble alongside the auth card.

Fix: skip complete_turn and persist_assistant_response for AuthPending.
The turn is paused (not complete), and the auth card is the only
user-facing signal. Tool calls are still persisted for history.

Also removes the now-unused `instructions` field from
AgenticLoopResult::AuthPending — the instructions were already sent
via the AuthRequired status event before AuthPending is returned.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(channels): resume agent turn after auth + pairing completion

After the web UI submits a token via /api/chat/auth-token or approves
pairing via /api/pairing/{channel}/approve, the agent's turn was stuck
at Pending forever — these HTTP handlers configured the extension
directly but never signaled the agent loop to resume.

Fix: inject a follow-up message through msg_tx (the agent's message
channel) after successful auth/pairing. This uses the same pattern as
the OAuth callback handler — the LLM picks up the injected message,
sees the activation/pairing result, and produces a natural response.
The response goes through the full agent pipeline (hooks, safety,
history persistence, Done event).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(channels): also resume agent turn on auth cancel

When the user dismisses the auth card, the frontend calls
/api/chat/auth-cancel which clears auth mode. But the original agent
turn was still paused at Pending with no Done event. The UI stayed
stuck at "Processing..." forever.

Fix: inject a cancellation message through msg_tx so the LLM can
acknowledge the cancellation and the turn completes naturally.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(channels): pass thread_id in pairing approve for proper routing

The injected follow-up message after pairing approval had no thread_id,
causing the gateway to fail with "missing a routing target." The
response from the LLM was produced but couldn't be delivered.

Fix: add optional thread_id to PairingApproveRequest. The frontend
passes currentThreadId so the agent responds in the same conversation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add Playwright tests for channel pairing flow

Covers:
- Auth-token/cancel handlers don't 500
- Pairing approve accepts optional thread_id field
- Backward compatibility: approve without thread_id works
- PairingRequired SSE shows pairing card
- PairingCompleted SSE dismisses pairing card
- Frontend sends currentThreadId in pairing approve request body

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(agent): transition thread to Idle on AuthPending

The AuthPending handler was not calling complete_turn() (to avoid
persisting redundant auth instructions as the response), but this
also skipped the ThreadState::Processing → Idle transition. The
thread stayed stuck in Processing forever, so the follow-up message
injected through msg_tx after auth/pairing was silently rejected.

Fix: explicitly set thread.state = Idle in both AuthPending arms
without calling complete_turn().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): remove dead verification challenge branch from telegram e2e

The Telegram verification challenge flow was removed — channels now
go straight to activation and use the generic pairing flow. The
conditional verification retry in setup_telegram() was dead code.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review comments

- Gate TELEGRAM_TEST_API_BASE_ENV and telegram_api_base_url() behind
  cfg(any(test, debug_assertions)) to prevent production env var override
  (serrrfirat HIGH — ship blocker)
- Sanitize validate_telegram_token() error messages to avoid leaking bot
  tokens via reqwest Display (Copilot)
- Log failed msg_tx sends instead of silently dropping (ilblackdragon)
- Forward thread_id in PairingCompleted SSE event (Copilot)
- Fix stale doc comment on persist_numeric_owner_id (Copilot)
- Hoist duplicate parse::<i64>() in propagate_approval (ilblackdragon)
- Delete dead _removed_telegram_verification_test (ilblackdragon)
- Fix always-passing E2E thread_id assertion (Copilot)
- Add V24 migration checksum to checksums.lock

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: update PairingStore::approve doc for noop mode

The doc said "silently succeeds" but the implementation returns
NotFound when no database is configured.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: sanitize extension names in agent prompts + live owner_actor_id in spawned tasks

Two hardening fixes from PR review deferrals:

1. Extension names from HTTP request bodies were interpolated directly into
   format strings that become IncomingMessage content fed to the agent loop.
   Add sanitize_extension_name() that strips non-alphanumeric chars and apply
   it at the two prompt injection points in chat_auth_token_handler and
   chat_auth_cancel_handler.

2. start_polling() and start_websocket_runtime() captured owner_actor_id as
   an owned Option<String> at spawn time. After pairing approval, WebSocket
   channels kept using the stale pre-approval value. Change to pass
   Arc<RwLock<Option<String>>> so spawned tasks read the current owner on
   each tick/event.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review findings — TurnOutcome refactor, security hardening, WS parity

Structural changes:
- Replace Thread::complete_turn/fail_turn/interrupt with single
  conclude_turn(TurnOutcome) that makes it impossible to forget the
  turn state. Fixes AuthPending arms leaving Turn stuck at Processing.
- Add TurnOutcome::CompletedSilently for auth-card-only turns.

Security:
- Sanitize channel name in pairing_approve_handler (missed injection site)
- Fix bot token leak in validate_telegram_token — log safe fields
  (is_timeout, is_connect, status) instead of reqwest error display
  which includes the URL containing the token
- Consume stale fallback auth gate before replaying message to prevent
  duplicate agentic runs on repeated OAuth callbacks
- Sanitize channel_name in derive_onboarding user-visible strings
- Add #[must_use] to BridgeOutcome enum

WS/REST parity:
- Add thread_id to WsClientMessage::AuthToken and AuthCancel
- WS AuthToken handler now injects follow-up message via msg_tx
  (matching REST chat_auth_token_handler behavior)
- WS AuthCancel handler now clears engine pending auth and injects
  cancellation message (matching REST chat_auth_cancel_handler)

Cleanup:
- Deduplicate build_runtime_config_updates (manager.rs imports from
  approval.rs instead of maintaining its own copy)
- Downgrade info! to debug! for auto-generated secret log
- Downgrade warn! to debug! for OAuth fallback diagnostic
- Upgrade debug! to warn! for on_start failure in propagate_approval
- Rename misleading e2e test to match what it actually tests
- Add mixed-character truncation test for sanitize_extension_name

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add critical coverage for auth flow security and msg_tx injection

New e2e tests:
- test_auth_cancel_injects_follow_up_message_via_sse: verifies the msg_tx
  injection path actually delivers messages end-to-end (SSE response event
  appears after auth-cancel)
- test_sanitize_extension_name_in_auth_cancel: verifies injection characters
  in extension_name are stripped before reaching the agent loop
- test_pairing_approve_sanitizes_channel_name: verifies channel path param
  is sanitized in pairing approve handler
- test_ws_auth_token_accepts_thread_id: verifies WS auth_token messages
  accept the new thread_id field
- test_ws_auth_cancel_accepts_thread_id: verifies WS auth_cancel messages
  accept thread_id and connection stays alive

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: always inject follow-up message after auth token submission

When result.activated was false, the chat_auth_token_handler skipped
the msg_tx injection. This left the paused turn (Pending with Done
suppressed) permanently stuck — the UI showed "Running tool_install..."
forever.

Now both REST and WS handlers always:
1. Clear auth mode
2. Broadcast AuthCompleted (with success=true/false)
3. Inject a follow-up message via msg_tx

The message content varies based on activation status so the LLM
can respond appropriately.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: revert hot_add to clone-then-shutdown to preserve message_tx receiver

The previous fix (drop write lock before shutdown) removed the channel
from the map before calling shutdown(). This dropped the last strong
Arc reference in the channel manager, killing the forwarding task's
receiver. The router holds its own Arc to the inner WasmChannel, so
propagate_approval's ensure_polling() could still send via message_tx
— but the receiver was dead, causing "channel closed" errors.

Revert to the staging pattern: read-lock to clone the Arc, drop the
lock, shutdown the clone, then write-lock to insert the replacement.
The old entry stays in the map (keeping the forwarding task alive)
until the insert atomically replaces it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: log bot_username set_setting failure instead of silently dropping

Copilot review: the set_setting result for bot_username was silently
dropped with `let _ =`. Now logs at debug level if the DB write fails,
giving visibility into mention detection degradation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: repair message_tx when Channel::start() fails at boot

When a WASM channel is loaded at boot without credentials (fresh DB),
on_start fails (e.g., Telegram deleteWebhook returns 404 with unresolved
{TELEGRAM_BOT_TOKEN}). Previously, message_tx was set BEFORE on_start,
so the sender survived but the receiver (rx) was dropped on error return.
Later, refresh_active_channel restarted polling which cloned the orphaned
sender — every send failed with "channel closed".

Fixes:
- Move message_tx creation AFTER on_start succeeds in Channel::start()
- Add WasmChannel::ensure_message_channel() that creates (tx, rx) if
  message_tx is None or closed, returning the stream for forwarding
- refresh_active_channel calls ensure_message_channel() after on_start
  succeeds and wires up a forwarding task if needed

Also:
- Revert hot_add to match staging exactly (no behavior change needed)
- Remove temporary debug logging (message_tx state before dispatch)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address remaining review comments — stale doc, websockets import

- Update AuthPending doc to reflect TurnOutcome::CompletedSilently
  (was "turn NOT completed", now accurately describes conclude_turn)
- Move `import websockets` inside try block so ImportError is caught
  by the except handler when the package isn't installed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address review comments — propagate on_start error, dedupe helpers, tighten tests

- propagate_approval: propagate on_start() error as ActivationFailed
  instead of swallowing it (zmanian review #1)
- router.rs: move test-only HashMap import into mod tests (zmanian #2)
- chat.rs: remove duplicate clear_auth_mode (Copilot review #1)
- e2e: strengthen auth-token assertion to check status 200 + success
  field, remove overlapping test_auth_cancel_returns_success (Copilot #2/#3)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address serrrfirat review — TOCTOU race, missing v2 auth clear, warn log

- ensure_message_channel: single write lock for atomic check-and-create
  (fixes TOCTOU race where concurrent callers could orphan a forwarding task)
- chat_auth_token_handler: add missing clear_engine_pending_auth() call
  (REST/WS parity — WS and REST cancel already had it, REST token did not)
- pairing_approve_handler: debug! → warn! for complete_pairing_approval
  failure (operationally significant — channel won't route until restart)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web,extensions): address review — sanitize agent messages, fix approve propagation, skip double Telegram getMe (#2432)

- Sanitize result.message before interpolation into synthetic agent input
  to prevent prompt injection via crafted validation errors (server.rs + ws.rs)
- Surface complete_pairing_approval() failure to frontend with success=false
  SSE event and ActionResponse::fail instead of silently succeeding
- Return ActionResponse::ok when auth_url is present even if activated=false
  so OAuth flows can progress through the frontend popup
- Skip generic validation_endpoint check for Telegram (validate_telegram_token
  already calls getMe and extracts bot_username — avoids double API round-trip)
- Sanitize generic validation_endpoint error messages to avoid leaking
  sensitive URL paths (e.g. bot tokens) via reqwest::Error Display

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Unify gateway onboarding and pairing flows

* Fix gateway message metadata scoping

* Clean up web gateway warnings

* Fix auth and onboarding regression fallout

* Fix gate resolution and pairing rollback trust boundaries

* Guard legacy agent loop from v2 submissions

* Fix PR review follow-ups for onboarding flow

* Fix CI clippy failure in pairing tests

* Fix onboarding review follow-ups

* Fix clippy warning in skills catalog

* Tighten pairing flow e2e assertions

* Fix onboarding auth review follow-ups

* Fix auth routing and tui clippy lint

* Fix pairing gate handoff in onboarding flow

* Fix clippy guard in mission event scan

* Fix merged clippy regressions

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
 2026-04-16 15:58:44 -07:00
Emil Bogomolov
a34bba249e resolve an issue with shared skills (#2086) 
* resolve an issue with shared skills

* fix(engine): non-breaking default for list_memory_docs_by_owner:

* address comments

* fix given github copilot comments

* fix project_id and user_id are not stored in the frontmatter

* fix(skills): address review feedback on shared-skill visibility

- Add caller-level regression test driving handle_list_skills end-to-end
  so a future revert to project-scoped listing fails at the call site, not
  just the helper (per .claude/rules/testing.md).
- Drop the redundant sort in list_skills_global — callers sort/dedupe the
  merged result anyway; keep only dedup-by-DocId within the shared set.
- Document the N+1 caveat on the default list_memory_docs_by_owner impl
  so production Store impls know they must override with a flat query.
- Log a debug! when deserialize_knowledge_doc falls back to nil project_id
  or "legacy" user_id, so stale on-disk frontmatter is traceable instead
  of silently invisible to scoped queries.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(skills): address remaining review comments

- Escape user_id, title, and tag strings before embedding in YAML
  frontmatter. A user_id containing a quote, backslash, or newline
  (e.g., an OIDC sub with unexpected characters) would otherwise
  produce unparsable YAML and make the doc unloadable.
- Clarify the comment in handle_list_skills: the initial call loads
  all doc types for the user, not just skills — skill filtering
  happens later in the filter pass.
- Add list_memory_docs_by_owner stubs to the three non-overriding
  TestStore impls (tests/engine_v2_gate_integration.rs,
  tests/engine_v2_skill_codeact.rs, src/bridge/router.rs) so the
  default-impl fallback into list_all_projects (which errors) can't
  silently swallow shared-skill visibility in integration tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix migrate_legacy_user_ids to preserve __shared__ ownership for Skill docs instead of stamping them with owner_id

* add nil-project pass to migrate_legacy_user_ids, and tests

---------

Co-authored-by: Emil Bogomolov <emil.bogomolov@near.ai>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-16 15:36:42 +03:00
firat.sertgoz
853b6a531d fix: restore issue-2402 v2 gate resume and action alias consistency (#2458) 
* fix(engine): normalize granted action aliases across lease checks

Keep lease preflight, policy, and consumption consistent for hyphen/underscore action aliases so installed tools do not fail mid-turn after being allowed.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(web): preserve pending gate call ids on auth resume

Resolve or synthesize the original action call id when resuming auth or external callback gates so resumed ActionResult messages remain correctly paired with the waiting assistant call.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* test(engine): cover install resume followed by aliased tool use

Add a higher-fidelity v2 gate integration regression that proves an install auth resume can flow directly into an aliased follow-up tool call and still complete the thread instead of stalling.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* fix(engine): apply policy checks to aliased action names

Resolve structured preflight action definitions with the same hyphen/underscore alias semantics as lease matching so aliased calls cannot bypass approval or deny policies.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(bridge): scan internal_messages in legacy call_id fallback

The `resolved_call_id_for_pending_action` legacy fallback scanned only
`thread.messages`, but in production the orchestrator writes ActionResult
and assistant messages to `thread.internal_messages` via
`sync_runtime_state`. This meant the `resolved_ids` set was always empty
and the fallback never found a match, silently falling through to a
synthetic id.

Scan both `messages` and `internal_messages` so the legacy path works
correctly for orchestrator-driven threads.

Addresses review feedback from @standardtoaster on #2458.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: format bridge router

---------

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: Zaki <zaki@iqlusion.io>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-16 15:11:12 +03:00
jinxin
5b2bd1563a fix(gateway): extend settings search to card, tool, and user sections (#2518) 
* fix(gateway): extend settings search to card, tool, and user sections

Settings search only filtered .settings-row elements, leaving Channels,
Extensions, MCP, Skills, Tools, and User Management sections unsearchable.
Add filtering for .ext-card, .tool-permission-row, and #users-tbody tr
elements, and update CSS to hide matched elements.

* fix(gateway): extend settings search to card, tool, and user sections

Settings search only filtered .settings-row elements, leaving Channels,
Extensions, MCP, Skills, Tools, and User Management sections unsearchable.
Add filtering for .ext-card, .tool-permission-row, and #users-tbody tr
elements, update CSS to hide matched elements, and reorder logic so
container visibility checks run after all items are filtered.

Add E2E tests covering search across tool rows and extension cards.

* chore: minor

---------

Co-authored-by: Robert Yan <46699230+think-in-universe@users.noreply.github.com>
 2026-04-16 15:07:56 +03:00
Henry Park
7008e9a881 feat(gate): persist "always approve" decisions to DB in v2 engine path (#2428) 
* feat(db): add per-user CachedSettingsStore decorator

SettingsStore methods hit the database on every call. The v2 engine
path (effect_adapter) and the dispatcher's per-turn tool permission
loading both called get_all_settings() without caching, adding
unnecessary DB round-trips on every agentic loop iteration.

Add a write-through CachedSettingsStore decorator that caches
get_all_settings() results per user_id. Write operations (set_setting,
delete_setting, set_all_settings) delegate to the inner store then
invalidate that user's cache entry. The write lock is held across DB
loads to prevent stale-data races from concurrent invalidations.

Wire the cache into TenantScope via a new settings_store field on
AgentDeps, so all settings reads in the agent loop go through the
cache. Remove the per-turn cached_tool_permissions Mutex hack from
ChatDelegate that was working around the missing cache layer.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review feedback

- Store Arc<HashMap> in cache instead of bare HashMap to avoid cloning
  the full settings map on every cache hit. get_setting/has_settings now
  only clone the single requested value or check emptiness through the Arc.
- Route get_setting_with_admin_fallback() through self.settings() instead
  of self.inner so both the per-user and admin lookups go through the cache.
- Update settings section comment to accurately describe which methods
  delegate through settings().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(gate): persist "always approve" decisions to DB in v2 engine path

The v2 engine's resolve_gate() only stored "always approve" decisions
in-memory via EffectBridgeAdapter::auto_approve_tool(), losing them on
process restart. The v1 path (thread_ops.rs) already persisted to DB.

Add persist_always_allow() and revert_always_allow() helpers that write
tool_permissions.{name} = AlwaysAllow to the SettingsStore, preferring
the CachedSettingsStore for write-through cache invalidation. Includes
defense-in-depth: tools declaring ApprovalRequirement::Always are never
persisted regardless of what the client sends. Reverts the DB write if
the resumed tool execution fails.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove no-op test flagged in PR review

Remove test_single_approval_does_not_persist — it only asserted an
empty store was empty without exercising any production code.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gate): address PR review — security and correctness fixes

- Use pending.parameters (not empty json) for defense-in-depth check so
  param-dependent tools like shell correctly detect Always requirement
- Validate action_name with is_valid_admin_tool_name() before persisting
  to prevent settings key injection via dots or special characters
- Save pre-existing permission value before overwriting; restore it on
  revert instead of blindly deleting (preserves long-standing prefs)
- Replace serde_json::to_value().unwrap_or() with json!("always_allow")
- Add tests: prior-value restoration, invalid tool name rejection,
  settings_store=None fallback to state.db

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gate): downgrade warn! to debug! in persist/revert paths

Internal diagnostics in persist_always_allow and revert_always_allow
used tracing::warn!, which corrupts the REPL/TUI per CLAUDE.md logging
rules. Downgraded all 5 call sites to debug!.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gate): address ilblackdragon + serrrfirat review feedback

- Upgrade persist/revert failure logs from debug! to warn! — DB
  persistence failures are security-relevant (user believes preference
  is permanent but it silently vanishes on restart). Matches v1 pattern
  at thread_ops.rs:1256. Safe in v2 (web gateway, not TUI).
- Fix serialization drift: use serde_json::to_value(PermissionState::
  AlwaysAllow) instead of hardcoded json!("always_allow"), coupling to
  the enum's serde rename attribute.
- Add dispatch-exempt comments on direct set_setting/delete_setting
  calls per .claude/rules/tools.md.
- Add #[cfg(feature = "libsql")] gate to test_persist_falls_back_to_
  state_db — test_db() requires the libsql feature.
- Update ApprovalGate docstring: v2 persistence is now wired, not
  aspirational.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add Playwright persistence happy-path test (#2485)

Add 3 e2e tests for always-approve persistence:
- test_always_approve_persists_to_db: verifies DB row after "always"
- test_revoke_always_approve_updates_db: verifies PUT revocation
- test_always_approve_survives_restart: restartable server, verifies
  auto-approve persists across process restart

Fix: remove raw Database fallback from persist/revert_always_allow.
The state.db fallback bypassed CachedSettingsStore cache invalidation,
causing GET /api/settings/tools to serve stale data until the 5-min
TTL expired. In production agent.deps.settings_store is always
available when the DB is; the fallback was dead code that broke cache
coherence.

Also: unit test for Settings::from_db_map tool_permissions parsing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gate): downgrade always when allow_always is false

A crafted client could send always:true on a gate where the pending
ResumeKind had allow_always:false (e.g. ApprovalRequirement::Always
tools). The in-memory auto_approve_tool would be set, silently
bypassing future approval prompts. persist_always_allow already
guarded against this via the ApprovalRequirement::Always check, but
the in-memory path did not.

Now resolve_gate downgrades always to false when the pending gate's
resume_kind doesn't permit it, before touching either the in-memory
set or the DB.

Also: fix ApprovalGate docstring to distinguish persistence from
hydration per Copilot review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-15 08:21:28 -07:00
Henry Park
7206bf0694 feat(gateway): rich tool cards in history + thread processing indicator (#2477) 
* feat(gateway): rich tool cards in history + thread processing indicator

History rendering:
- Add createActivityGroupFromHistory() to render the most recent turn's
  tool calls as the same .activity-tool-card DOM structure used during
  live SSE (expandable cards with icons, output preview, error details).
  Older turns keep the compact "N tools used" summary to limit DOM size.

Thread processing indicator:
- Track background threads with active agent work via processingThreads
  Set (fed by thinking, tool_started, stream_chunk SSE events for
  non-current threads; cleared on status "Done" and SSE reconnect).
- Render a .thread-processing spinner in the sidebar for threads that
  are actively processing.

E2E tests:
- test_message_persists_across_page_reload: message + response survive
  full page reload
- test_tool_calls_rendered_as_activity_cards_after_reload: echo tool
  renders as rich .activity-tool-card with data-status="success"
- test_tool_calls_expandable_after_reload: summary click expands cards
  container, card header click expands body
- test_background_thread_shows_processing_indicator: background thread
  gets unread badge after completion

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add processing indicator tests + review fixes

- test_processing_indicator_shows_on_thread_switch: verify completed
  turns show no stale "Processing..." when switching back
- test_processing_indicator_shows_for_incomplete_turn: verify the
  thinking indicator appears when switching to a mid-turn thread
  (gracefully skips if agent completes too fast to catch)
- Add activity_thinking/activity_thinking_text selectors to helpers.py

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gateway): address PR #2477 review comments

- Clear processingThreads + refresh sidebar on SSE reconnect so stale
  spinners are removed immediately
- Clear processingThreads on "Awaiting approval" status (terminal state
  where agent is blocked on user input, not actively processing)
- Map tool call status from has_result/has_error: running (neither),
  success (has_result), fail (has_error) — shows spinner for in-progress
  tools in history instead of misleading checkmark
- Auto-expand activity group when any tool call has an error
- Add data-thread-id attribute to .thread-item for testability
- Scope processing spinner and unread badge assertions to specific
  thread ID in E2E tests
- Add explicit spinner visibility/removal assertions to background
  thread processing indicator test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(gateway): address second round of PR #2477 review comments

- Use activity-icon-success/activity-icon-fail CSS classes for history
  tool card icons (matches live card styling with colored ✓/✗)
- Fix _wait_for_completed_turn to check turns[-1] instead of any() to
  avoid early return when earlier turns are already completed
- Rename test_processing_indicator_shows_on_thread_switch to
  test_no_stale_processing_indicator_for_completed_thread to match
  what it actually verifies (no stale indicator, not indicator presence)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-15 08:11:52 -07:00
Henry Park
16a07316d0 test(e2e): add Playwright persistence happy-path test (#2475) 
* test(e2e): add Playwright persistence happy-path test

Add `test_message_persists_across_page_reload` which validates the full
persistence round-trip: send a message via the chat UI, reload the page
(clearing all client-side state), switch back to the thread, and verify
both user message and assistant response are restored from the database.

Cross-checks via the history API that exactly one user turn exists with
a completed response.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(e2e): address PR review comments

- Replace fixed `wait_for_timeout(2000)` with polling via history API
  until the turn reaches `Completed` state (avoids CI flakiness)
- Use `SEL["auth_screen"]` instead of hardcoded `"#auth-screen"`
  selector (follows project convention from helpers.py)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-14 16:20:50 -07:00
Henry Park
2dc78b2d94 feat(db): add per-user CachedSettingsStore decorator (#2425) 
* feat(db): add per-user CachedSettingsStore decorator

SettingsStore methods hit the database on every call. The v2 engine
path (effect_adapter) and the dispatcher's per-turn tool permission
loading both called get_all_settings() without caching, adding
unnecessary DB round-trips on every agentic loop iteration.

Add a write-through CachedSettingsStore decorator that caches
get_all_settings() results per user_id. Write operations (set_setting,
delete_setting, set_all_settings) delegate to the inner store then
invalidate that user's cache entry. The write lock is held across DB
loads to prevent stale-data races from concurrent invalidations.

Wire the cache into TenantScope via a new settings_store field on
AgentDeps, so all settings reads in the agent loop go through the
cache. Remove the per-turn cached_tool_permissions Mutex hack from
ChatDelegate that was working around the missing cache layer.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address PR review feedback

- Store Arc<HashMap> in cache instead of bare HashMap to avoid cloning
  the full settings map on every cache hit. get_setting/has_settings now
  only clone the single requested value or check emptiness through the Arc.
- Route get_setting_with_admin_fallback() through self.settings() instead
  of self.inner so both the per-user and admin lookups go through the cache.
- Update settings section comment to accurately describe which methods
  delegate through settings().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address all PR review feedback

- Use `crate::db::` imports instead of `super::` (convention fix)
- Add `wrap()` factory fn to CachedSettingsStore, simplify app.rs construction
- Store `Arc<HashMap>` in cache to avoid full map clones on hits
- Expose `invalidate_user()` and `flush()` public methods
- Wire `flush()` into SIGHUP handler via concrete `settings_cache` on AppComponents
- Wire `settings_store` into GatewayState and route all settings handlers
  through it so web UI writes invalidate the cache (critical fix)
- Route `get_setting_with_admin_fallback()` through `self.settings()`
- Add error-path test (FailingStore mock, cache not poisoned on error)
- Add concurrent-access test (8 concurrent readers, inner store hit once)
- Add TenantScope caller-level test (read/write through cache wiring)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: reuse resolve_settings_store() in settings_tools_set_handler

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: address all review feedback on CachedSettingsStore

- Add TTL (300s) and max-entries cap (1000) to bound cache staleness
  and memory growth. Entries expire after TTL; cache clears when cap
  exceeded.
- Route admin tool_policy GET/PUT through resolve_settings_store() so
  writes invalidate the __admin__ cache entry.
- Route settings_export_handler and settings_tools_list_handler through
  resolve_settings_store() (were bypassing cache on reads).
- Wire invalidate_user() into users_delete_handler and
  users_suspend_handler so deleted/suspended users' settings are evicted.
- Replace GatewayState.settings_store (trait object) with
  settings_cache (concrete CachedSettingsStore) — single field for both
  trait dispatch and cache management, no desync risk.
- Add settings_override to ExtensionManager with with_settings_store()
  builder. All settings reads/writes in ExtensionManager now route
  through the cached store when available.
- Make ExtensionManager::settings_store() pub(crate); update
  AuthManager to call it instead of database(), closing the auth
  descriptor cache bypass.
- Remove unused wrap() method; merge redundant invalidate/invalidate_user.
- Add tracing::debug on SIGHUP cache flush.
- Expand module docs with design assumptions, known bypass paths, TTL
  and eviction semantics.
- Add tests: expired_entry_triggers_reload, fresh_entry_does_not_reload,
  max_entries_cap_triggers_eviction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: collapse nested if into filter to satisfy clippy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-14 16:10:10 -07:00
Henry Park
37669e6ec8 fix(web): prevent browser crash from timer leaks, DOM growth, SSE buffer (#2406) (#2441) 
* fix(web): prevent browser crash from timer leaks, DOM growth, SSE buffer (#2406)

Extended sessions with heavy bot interactions caused Chrome's "Pages
Unresponsive" dialog due to accumulated browser resources that were
never cleaned up.

Fixes:
- Add cleanupConnectionState() to clear leaked setInterval/setTimeout
  timers on SSE reconnect, tab visibility change, and page unload
- Cap DOM at 200 message nodes via pruneOldMessages() with streaming-
  aware pruning (skips data-streaming elements, called at turn
  boundaries and after loadHistory)
- Cap jobEvents Map at 50 entries with LRU eviction (excludes current
  job from eviction scan)
- Increase SSE broadcast buffer from 256 to 1024 (configurable via
  SSE_BROADCAST_BUFFER env var, with zero-guard to prevent panic)

Closes #2406

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR #2433 review — move SSE buffer to GatewayConfig, fix E2E timer test

Move SSE_BROADCAST_BUFFER env var from direct std::env::var() in sse.rs
to GatewayConfig in config/channels.rs, following the convention that all
gateway env vars flow through structured config. Add MAX_BROADCAST_BUFFER
(65,536) clamp to prevent OOM from misconfiguration.

Fix E2E timer leak test to install setInterval monkey-patch via
page.add_init_script() before navigation so initialization timers are
tracked. Add test_dom_resource_limits.py to E2E CLAUDE.md scenario table.

Add unit test for buffer config parsing, zero-rejection, and clamp.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address review — clean gatewayStatusInterval, prune user msgs, use constants

- Add gatewayStatusInterval to cleanupConnectionState() so it is cleared
  on reconnect/tab-hide/unload; add guard in startGatewayStatusPolling()
  to prevent double-start; restart polling on tab visibility restore
- Call pruneOldMessages() after addMessage('user', ...) in sendMessage()
  so DOM stays bounded even during rapid user input
- Replace hardcoded broadcast_buffer: 1024 with DEFAULT_BROADCAST_BUFFER
  in all test construction sites (5 occurrences across 4 files)
- Document in from_sender() doc comment why broadcast_buffer is absent
- Tighten E2E timer leak assertion from baseline+1 to baseline

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address PR #2441 review — prune/timer/assert/doc fixes

- Remove pruneOldMessages() from loadHistory() pagination path to avoid
  immediately evicting just-prepended older messages
- Move MAX_DOM_MESSAGES constant to top-level constants block
- Add _loadThreadsTimer to cleanupConnectionState() for consistency
- Add assert!(broadcast_buffer > 0) to SseManager constructor with
  panic doc (tokio broadcast channel requires capacity > 0)
- Use Set-based interval tracking in E2E test to prevent counter
  underflow from double-clear
- Update CLAUDE.md broadcast buffer docs (256 → 1024, SSE_BROADCAST_BUFFER)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): remove assert! from SseManager to pass no-panics CI check

Replace assert!(broadcast_buffer > 0) with a doc comment noting the
precondition. GatewayConfig already rejects 0 at the config layer.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): address ilblackdragon review — correctness, e2e tests, docs (#2406)

Correctness:
- pruneOldMessages: clean up orphaned leading time-separators after pruning
- jobEvents LRU: replace O(n) scan with O(1) Map insertion-order eviction
- Document degenerate all-streaming under-prune case

Playwright e2e tests:
- Tab hide/restore: no duplicate gateway status polling intervals
- DOM cap + streaming: 260 elements prune to ≤200, streaming preserved, no orphan separators
- jobEvents bounded: 60 jobs stay capped at ≤50 via LRU eviction
- Fix assertion selector to match pruneOldMessages superset, tighten lower bound

Rust:
- Unit test: SseManager buffer size parameter actually controls lag behavior
- Document MAX_BROADCAST_BUFFER memory impact (65K×100×200B ≈ 1.3 GB)
- Move "capacity baked into tx" comment from from_sender to rebuild_state

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(web): protect currentJobId from LRU eviction in jobEvents map (#2441)

The O(1) LRU eviction skips the job that just received an event (moved
to end via delete+set), but did not protect the job the user is actively
viewing in the detail panel (currentJobId). If the user views a quiet
job while 50+ other jobs fire events, the viewed job's events would be
evicted and the activity tab would appear empty.

Add a currentJobId guard to the eviction loop and a Playwright e2e test
that verifies the actively-viewed job survives LRU pressure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(e2e): add real-flow Playwright tests for DOM resource limits (#2406)

Add 4 E2E tests that exercise pruning and timer cleanup through actual
UI interactions (mock LLM round-trips, real SSE reconnects) instead of
page.evaluate() injection. Also fix the existing timer leak test which
failed due to execution context destruction from add_init_script.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
 2026-04-14 10:25:42 -07:00
Zaki Manian
a9cea6c21d fix(ci): skip NearAI URL DNS validation for non-NearAI backends (#2080) 
* fix(ci): skip NearAI URL DNS validation when NearAI is not the active backend

LlmConfig::resolve() unconditionally called validate_base_url() on
default NearAI URLs (private.near.ai), which performs synchronous DNS
resolution. In environments without external DNS access (CI runners,
containers), this blocks startup then fails — breaking all E2E tests
when a different LLM backend is configured.

Conditionally skip validation when NearAI is not the active backend
and the user hasn't explicitly set the URL. Also removes redundant
@pytest.mark.asyncio decorators from test_webhook.py (asyncio_mode =
"auto" handles this automatically per project convention).

https://claude.ai/code/session_01FybyQXiX2HDhaGizxr2PFC

* fix(ci): also validate NearAI URLs when DB override or NearAI embeddings are active

The validation gate for NEARAI_BASE_URL and NEARAI_AUTH_URL previously
only checked whether NearAI was the primary chat backend or the URL was
explicitly set via env var. This allowed a base_url supplied through
settings.llm_builtin_overrides (DB override) or used by NearAI
embeddings (embeddings.provider=nearai) to bypass the SSRF guard.

Widen both validation gates to also fire when:
- nearai_override provides a base_url (DB builtin override)
- NearAI embeddings are enabled (embeddings enabled + provider=nearai)

https://claude.ai/code/session_01YSmxv6gT4d9kJu5vsjxpCz

* ci: retrigger CI checks

https://claude.ai/code/session_01AQ4iNcEfFeniBA1iMvFTuN

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: ilblackdragon@gmail.com <ilblackdragon@gmail.com>
 2026-04-13 18:07:43 +09:00
firat.sertgoz
ed2d6dc3b1 fix web chat refresh active thread (#2330) 2026-04-12 22:44:01 +09:00
firat.sertgoz
4032f6de23 Fix paired Telegram owner scope routine visibility (#2258) 
* Fix paired Telegram owner scope routing

* fix: address review findings (iteration 1)

* fix: address telegram owner routing feedback

* test: isolate telegram routines e2e fixture

---------

Co-authored-by: Guille <gagdiez.c@gmail.com>
 2026-04-11 08:36:35 +03:00