chore: promote staging to staging-promote/e2544ed4-24637363119 (2026-04-20 03:46 UTC) - #2705
Merged
Merged
Conversation
…ironclaw#2599 stages 4d + 5 (#2687) * refactor(gateway): extract extensions + jobs + settings + routines slices — ironclaw#2599 stages 4d + 5 Bundles the last feature-slice migrations in one PR. After this lands, server.rs has zero feature handlers — it's a pure backward-compat re-export shim that stage 6 will delete. New feature slices: - features/extensions/ (stage 4d) — nine routes: list / readiness / tools / install / activate / remove / registry / setup / setup-submit. Owns derive_activation_status, derive_onboarding, extension_phase_for_web, and apply_extension_readiness_to_response. Every handler that takes `{name}` from the URL validates via `ExtensionName::new` at the boundary (400 on path-traversal / invalid chars / oversized). The setup-submit path routes through `AuthManager::resolve_auth_flow_extension_name` (canonical resolver) and `platform::engine_dispatch`, preserving the identity invariants called out in CLAUDE.md. - features/jobs/ (stage 5) — nine routes covering sandbox-job lifecycle (list / summary / detail / cancel / restart / prompt / events / files-list / files-read). Straight file move from handlers/jobs.rs. - features/settings/ (stage 5) — eight routes (list / export / import / get / set / delete / tools-list / tools-set). `resolve_settings_store` promoted to `pub(crate)` for `handlers/tool_policy.rs`. Straight file move from handlers/settings.rs. - features/routines/ (stage 5) — seven routes merged from two sources: `handlers/routines.rs` (list / summary / detail / trigger / toggle / delete) plus the previously-canonical `routines_runs_handler` from `server.rs` (the `handlers/routines.rs` copy of the same function was marked `#[allow(dead_code)]` and kept in sync manually — that redundancy is gone now). Uses the cleaner `routine.is_owned_by(...)` ownership predicate throughout instead of the direct `user_id` match the `server.rs` copy used. server.rs changes: - Deleted 549 lines of extension handlers + 47 lines of routines_runs_handler + the now-redundant axum/Json/types/Uuid imports the moved handlers pulled in. - Reduced to 14 lines of `pub use` re-exports for `start_server` and `platform::state::*` so external callers (src/main.rs, src/app.rs, tests) keep resolving. Stage 6 follow-up deletes even those once the callers flip to `platform::*` directly. - The `#[cfg(test)] mod tests` block stays in place (it's the only module still using `test_gateway_state*` helpers that construct state for chat + extensions + oauth + pairing together). Test imports updated to pull handlers/helpers from their new feature-slice homes. Promoting `test_gateway_state*` to `test_helpers.rs` is the last blocker before stage 6 and is tracked in the ironclaw#2599 follow-ups comment. Cross-cutting updates: - platform/router.rs: consolidated feature-slice imports into one section with an updated docstring noting the new completion state. - handlers/mod.rs: dropped `pub mod extensions / jobs / routines / settings` declarations (the files physically moved via `git mv`). handlers/ now lists only the still-transitional modules. - handlers/tool_policy.rs: switched `use super::settings::resolve_settings_store` to `use crate::channels::web::features::settings::resolve_settings_store`. - src/extensions/manager.rs: two `handlers::extensions::derive_onboarding` call sites redirected to the new slice path. - src/channels/web/tests/multi_tenant.rs: jobs + routines handler imports updated. - features/{jobs,settings,routines,extensions}/mod.rs: all `crate::channels::web::server::{GatewayState, PerUserRateLimiter, RateLimiter, ActiveConfigSnapshot}` imports redirected to `platform::state::*` directly, so the new slices don't depend on the dying shim. Quality gate: - cargo fmt clean - cargo clippy --all --tests --examples --all-features clean - cargo test --lib channels::web — 424 passed - scripts/check_gateway_boundaries.py — clean, empty allowlist preserved - scripts/check_gateway_boundaries.py test — 16/16 Net shape: four slices added under features/, four handlers files deleted (via `git mv`), server.rs −651 lines (to 14), plus 467 lines added to features/extensions/ (the nine canonical extension handlers + helpers). Migration is functionally complete — stage 6 is the final cleanup. Explicit non-scope: - Stage 6 (server.rs shim deletion) is NOT in this PR. Needs the `test_gateway_state*` test-helper promotion to `test_helpers.rs` as the prerequisite, which is a coordinated cleanup touching every caller that still imports `crate::channels::web::server::*`. - `handlers/` still holds 12 transitional modules (auth, engine, frontend, llm, memory, secrets, skills, system_prompt, tokens, tool_policy, users, webhooks). Per ironclaw#2599 stage 7, those migrate only if churn justifies — most are low-churn and pattern- free. - No `Deps`-view narrowing in any slice; every handler still takes the full `GatewayState`. Separate hardening PR per the tracking issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(gateway): fix CLAUDE.md stale references — PR #2687 review Two Copilot doc fixes: - `platform/engine_dispatch.rs` description said "server.rs (chat + extensions_setup_submit)" but chat migrated in 4c (#2680) and extensions in 4d (this PR). Updated to list the three current feature slices that compose the dispatch helpers. - `features/chat/` description referenced `AuthManager::resolve_auth_flow_extension_name`, but the public `AuthManager` method is `resolve_extension_name_for_auth_flow` (src/bridge/auth_manager.rs:406). `resolve_auth_flow_extension_name` is the underlying `pub(crate)` free function the method delegates to. Corrected to the method name so the docs match the public API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): wire channel_relay kind_hint + refresh router docstring — PR #2687 review Two PR #2687 review fixes: - `extensions_install_handler` now maps `kind: "channel_relay"` to `ExtensionKind::ChannelRelay`. Frontend registry entries send `channel_relay` as the kind, and `ExtensionManager::install` uses `kind_hint` to disambiguate registry name collisions and URL-install inference. The dropped arm meant Slack-relay-style installs could land on the wrong disambiguation path. Pre-existing in the canonical `server.rs` parser; folding the fix here because two reviewers (Gemini + Copilot) flagged it and the fix is one line. - `platform/router.rs` top-of-file docstring rewritten to match the post-stage-4d reality: feature handlers live in `features/<slice>/` or the transitional `handlers/*.rs` flat folder — none live in `server.rs`, which is now a pure re-export shim awaiting stage 6 deletion. The old wording contradicted the updated inline comment at line 64. Regression coverage: single-arm addition to a match on a small closed enum; caller-level behavior is covered by existing `ExtensionManager::install` paths that consume `kind_hint`. No new test added — the change is a one-arm parity fix, and a test asserting the match arm would duplicate the compiler's exhaustiveness check. [skip-regression-check] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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>
…2e 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>
…s enum (#2678) * refactor(events): replace JobResult.status String with JobResultStatus enum Producers at 7 sites and consumers at 3 sites previously agreed by convention only. Promotes the status field to a typed enum with snake_case serde — wire format preserved. Maps to bug pattern from #2570, #2531, #2517 where status transitions drifted between producer and consumer. Tests cover snake_case serialization, wire-format round-trip, and the is_success() predicate used at consumer sites. * refactor(events): add Stuck variant, accept "error" alias, case-insensitive parse JobResultStatus now covers the full set of wire values producers emit: - New `Stuck` variant for worker/job.rs `mark_stuck` path (was coerced to Failed + warn log, losing the distinction that job monitor and recovery logic care about). - `FromStr` accepts `"error"` as a legacy alias for `Failed` so claude_bridge and acp_bridge wire payloads deserialize cleanly instead of hitting the default-on-unknown branch. - `FromStr` trims whitespace and uses `eq_ignore_ascii_case`, so `" COMPLETED "` and `"Failed"` parse rather than falling back. - Empty / whitespace-only input now returns `Err` (distinct from "unknown value") so callers can log it separately. Producer migration: worker/job.rs emits `JobResultStatus::Stuck` directly via `serde_json::json!` so the wire string stays pinned to `as_str()`. claude_bridge and acp_bridge keep emitting `"error"` on the wire; the FromStr alias covers them without churn on those call sites. Added unit tests for each variant, the `"error"` alias, case insensitivity, whitespace trimming, empty-string error, and preservation of the original input in `JobResultStatusParseError`. --------- Co-authored-by: Henry Park <henrypark133@gmail.com>
github-actions
Bot
added
size: XL
Code reviewFound 1 issue:
|
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-promotion from staging CI
Batch range:
7fb41555a9e55677d1aaea29ca567a5b369c2b05..e88236ab08c007343587efd3efd984ca0ee39ce8Promotion branch:
staging-promote/e88236ab-24647433078Base:
staging-promote/e2544ed4-24637363119Triggered by: Staging CI batch at 2026-04-20 03:46 UTC
Commits in this batch (25):
Current commits in this promotion (0)
Current base:
mainCurrent head:
staging-promote/e88236ab-24647433078Current range:
origin/main..origin/staging-promote/e88236ab-24647433078Auto-updated by staging promotion metadata workflow
Waiting for gates:
Auto-created by staging-ci workflow