Skip to content

chore: promote staging to staging-promote/3ac8e5f7-24538841293 (2026-04-17 04:57 UTC)#2565

Open
ironclaw-ci[bot] wants to merge 2 commits intostaging-promote/3ac8e5f7-24538841293from
staging-promote/fe5cdcee-24548466471
Open

chore: promote staging to staging-promote/3ac8e5f7-24538841293 (2026-04-17 04:57 UTC)#2565
ironclaw-ci[bot] wants to merge 2 commits intostaging-promote/3ac8e5f7-24538841293from
staging-promote/fe5cdcee-24548466471

Conversation

@ironclaw-ci
Copy link
Copy Markdown
Contributor

@ironclaw-ci ironclaw-ci bot commented Apr 17, 2026

Auto-promotion from staging CI

Batch range: a53eac5c2dec6b6cd5c08189086093fde64aa9cb..fe5cdceeb2f596532350ca228c2888b2cdc28589
Promotion branch: staging-promote/fe5cdcee-24548466471
Base: staging-promote/3ac8e5f7-24538841293
Triggered by: Staging CI batch at 2026-04-17 04:57 UTC

Commits in this batch (62):

Current commits in this promotion (2)

Current base: staging-promote/3ac8e5f7-24538841293
Current head: staging-promote/fe5cdcee-24548466471
Current range: origin/staging-promote/3ac8e5f7-24538841293..origin/staging-promote/fe5cdcee-24548466471

Auto-updated by staging promotion metadata workflow

Waiting for gates:

  • Tests: pending
  • E2E: pending
  • Claude Code review: pending (will post comments on this PR)

Auto-created by staging-ci workflow

henrypark133 and others added 2 commits April 17, 2026 07:08
…start (#2561)

* fix: owner_id was stored as string when recovered from settings on restart

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

* test: add regression test for owner_id type on restart

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

* chore: use existing function

---------

Co-authored-by: Guillermo Alejandro Gallardo Diez <gagdiez@iR2.local>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added scope: channel/web Web gateway channel scope: channel/wasm WASM channel runtime scope: config Configuration scope: extensions Extension management scope: setup Onboarding / setup scope: docs Documentation size: L 200-499 changed lines risk: high Safety, secrets, auth, or critical infrastructure contributor: core 20+ merged PRs labels Apr 17, 2026
@claude
Copy link
Copy Markdown

claude bot commented Apr 17, 2026

Code review

Found 8 issues:

  1. [HIGH:85] O(NM) collision check — registered_channel_names is passed as &[String] and checked with .iter().any() for each loaded channel. With many registered channels, this becomes O(NM). Convert to HashSet at the caller for O(1) lookups.

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/channels/wasm/setup.rs#L189-L198

        // Also reject any name that collides with an already-registered
        // channel to prevent a WASM module from shadowing a channel that
        // was registered earlier in the startup sequence.
        if registered_channel_names
            .iter()
            .any(|n| n.to_ascii_lowercase() == name_lower)
  1. [MEDIUM:85] Implicit owner_id type inference — build_runtime_config_updates() tries to parse owner_actor_id as i64, falling back to string if parsing fails. This means the owner_id in config can be either JSON number or string depending on parse success. If a caller expects a specific type (e.g., the new test expects a number), this could cause runtime errors for non-numeric owner IDs.

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/pairing/approval.rs#L127-L133

    if let Some(owner_actor_id) = owner_actor_id {
        let owner_id_value = owner_actor_id
            .parse::<i64>()
            .map(serde_json::Value::from)
            .unwrap_or_else(|_| serde_json::Value::String(owner_actor_id.to_string()));
  1. [MEDIUM:80] Fragmented activation state — Three sources of truth: config.channels.configured_wasm_channels (setup wizard), activated_channels (runtime DB), and ExtensionManager.active_channel_names (in-memory RwLock). This mirrors the legacy "cache + DB" pattern that CLAUDE.md discourages. A single canonical source at startup would improve coherence.

  2. [MEDIUM:78] Caller-level testing incomplete — New tests cover register_startup_channels() and load_startup_active_channels(), but the critical orchestration path in main.rs (lines 415–426) that builds startup_active_channel_names and passes it to setup_wasm_channels() is not tested end-to-end. See .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper").

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/main.rs#L415-L426

    let startup_active_channel_names = if let Some(ref ext_mgr) = components.extension_manager {
        ext_mgr
            .load_startup_active_channels(
                &config.owner_id,
                config.channels.configured_wasm_channels.clone(),
            )
            .await
    } else {
        normalize_extension_names(config.channels.configured_wasm_channels.clone())
    };
  1. [MEDIUM:75] String normalization deferred to call sites — normalize_extension_names() is called in two places (main.rs and ExtensionManager::load_startup_active_channels), but ChannelsConfig::resolve() doesn't normalize configured_wasm_channels at load time. Malformed names silently disappear only when startup code calls the function. Normalizing in the config layer would fail fast.

  2. [MEDIUM:75] Inefficient deduplication — normalize_extension_names() clones the name twice per insertion (once in seen.insert(), once in normalized.push()). Collect into HashSet first, then convert to Vec.

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/extensions/naming.rs#L50-L72

        if seen.insert(name.clone()) {
            normalized.push(name);
        }
  1. [LOW:70] Error messages lack specificity — normalize_extension_names() logs "Skipping invalid startup channel name" for all canonicalization failures, but doesn't distinguish uppercase vs. path traversal vs. empty string. Include the ExtensionError reason in the log.

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/extensions/naming.rs#L65-L71

            Err(error) => {
                tracing::warn!(
                    channel = %name,
                    error = %error,
                    "Skipping invalid startup channel name"
                );
            }
  1. [LOW:65] Stale comment — ExtensionManager.active_channel_names comment says "Names of WASM channels that were successfully loaded at startup," but after this refactor it holds "currently running" channels, not boot-discovery artifacts. Update the doc comment to reflect actual semantics.

https://github.com/anthropics/ironclaw/blob/78d59a2bbb12717580c44e24adc404d1308a5ba7/src/extensions/manager.rs#L395-L399

    /// Names of channels that are currently running in the process.
    ///
    /// For WASM channels this is no longer seeded from on-disk discovery at
    /// boot; startup only records channels that were actually restored.

No security vulnerabilities or logic bugs detected. The architecture properly separates fallback config from runtime state and handles the owner_id type conversion correctly for both numeric and string cases (though the implicit type inference could be clearer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: high Safety, secrets, auth, or critical infrastructure scope: channel/wasm WASM channel runtime scope: channel/web Web gateway channel scope: config Configuration scope: docs Documentation scope: extensions Extension management scope: setup Onboarding / setup size: L 200-499 changed lines staging-promotion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants