Skip to content

chore: promote staging to staging-promote/91c4c7ca-25018206332 (2026-04-27 22:14 UTC) - #3001

Merged
henrypark133 merged 1 commit into
mainfrom
staging-promote/f11a49be-25021901653
Apr 29, 2026
Merged

chore: promote staging to staging-promote/91c4c7ca-25018206332 (2026-04-27 22:14 UTC)#3001
henrypark133 merged 1 commit into
mainfrom
staging-promote/f11a49be-25021901653

Conversation

@ironclaw-ci

@ironclaw-ci ironclaw-ci Bot commented Apr 27, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Auto-promotion from staging CI

Batch range: 7fb41555a9e55677d1aaea29ca567a5b369c2b05..f11a49be0b8c2d471c0e8aa65836517417488d5a
Promotion branch: staging-promote/f11a49be-25021901653
Base: staging-promote/91c4c7ca-25018206332
Triggered by: Staging CI batch at 2026-04-27 22:14 UTC

Commits in this batch (98):

Current commits in this promotion (1)

Current base: staging-promote/91c4c7ca-25018206332
Current head: staging-promote/f11a49be-25021901653
Current range: origin/staging-promote/91c4c7ca-25018206332..origin/staging-promote/f11a49be-25021901653

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
[codex] Fix bridge restart approval floor (#2978)
f11a49b 
* fix bridge restart approval floor

* address bridge permission review cleanup
@github-actions github-actions Bot added size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Apr 27, 2026
@claude

claude Bot commented Apr 27, 2026

Copy link
Copy Markdown

Code review

Found 11 issues:

  1. [CRITICAL:95] Stringly-typed tool special-casing violates typed internals rule — enforce_static_ask_floor_for_base_never_tool() uses string comparison (canonical_tool_name(lookup_name) == "restart") to determine tool-specific behavior. Per .claude/rules/types.md, hardcoding tool-specific behavior by name in a centralized adapter is an anti-pattern. The restart tool should carry metadata (e.g., a trait flag) in its definition, not be singled out by string matching. This creates an unbounded special-case list as new tools need similar gates.

    ironclaw/src/bridge/effect_adapter.rs

    Lines 339 to 343 in f10404e

    fn enforce_static_ask_floor_for_base_never_tool(lookup_name: &str) -> bool {
    // Other base-Never bridge tools manage auth/install gates internally.
    // Restart's command-level confirmation does not cover direct v2 calls.
    canonical_tool_name(lookup_name) == "restart"
    }

  2. [CRITICAL:90] Permission override logic conflates explicit vs. configured, causing wrong behavior — Line 44 of tool_permissions.rs defines configured = explicit.or_else(|| TOOL_RISK_DEFAULTS.get(...)), which conflates "explicit user permission" with "tool default configuration". These are semantically different. When apply_user_permission_override() matches on (base_requirement, user_permission.configured) at line 330 of effect_adapter.rs, the "ask floor" gate never triggers if there's an explicit override, even when it should. The configured field should represent only the tool's default risk level from TOOL_RISK_DEFAULTS, not the explicit override.

    ironclaw/src/bridge/tool_permissions.rs

    Lines 40 to 51 in f10404e

    pub(crate) fn resolve_permission(&self, tool_name: &str) -> ToolPermissionResolution {
    let canonical = canonical_tool_name(tool_name);
    let hyphenated = canonical.replace('_', "-");
    let explicit = self.explicit_permission_with_names(tool_name, &canonical, &hyphenated);
    let configured = explicit.or_else(|| TOOL_RISK_DEFAULTS.get(canonical.as_str()).copied());
    let effective = configured.unwrap_or(PermissionState::AskEachTime);
    ToolPermissionResolution {
    effective,
    explicit,
    configured,
    }
    }

  3. [HIGH:85] String allocation in hot path — resolve_permission() allocates twice: canonical_tool_name() creates a String, then .replace('_', "-") allocates again on every tool permission resolution.

    ironclaw/src/bridge/tool_permissions.rs

    Lines 40 to 42 in f10404e

    pub(crate) fn resolve_permission(&self, tool_name: &str) -> ToolPermissionResolution {
    let canonical = canonical_tool_name(tool_name);
    let hyphenated = canonical.replace('_', "-");

  4. [HIGH:80] String allocation on approval gate — enforce_static_ask_floor_for_base_never_tool() allocates a String on every approval check, then compares to literal "restart".

    ironclaw/src/bridge/effect_adapter.rs

    Lines 339 to 343 in f10404e

    fn enforce_static_ask_floor_for_base_never_tool(lookup_name: &str) -> bool {
    // Other base-Never bridge tools manage auth/install gates internally.
    // Restart's command-level confirmation does not cover direct v2 calls.
    canonical_tool_name(lookup_name) == "restart"
    }

  5. [HIGH:85] Missing test coverage for safety invariant — No test verifies gate behavior when configured=Some(AskEachTime) is set in defaults while explicit=None.

  6. [HIGH:80] Circular dependency — effect_adapter.rs imports canonical_tool_name() specifically to special-case "restart", coupling modules.

    ironclaw/src/bridge/effect_adapter.rs

    Lines 33 to 35 in f10404e

    use crate::bridge::tool_permissions::{
    ToolPermissionResolution, ToolPermissionSnapshot, canonical_tool_name,
    };

  7. [MEDIUM:75] Inconsistent field naming — Field configured semantically means "set by configuration" but actually means "resolved from defaults when not explicit".

  8. [MEDIUM:70] Refactoring removes public method without explanation — explicit_permission() made private; add a comment explaining the split.

  9. [MEDIUM:65] Lack of separation of concerns — apply_user_permission_override() couples explicit override enforcement with tool-specific approval floor enforcement.

  10. [MEDIUM:75] Duplicated string allocation pattern — Allocates canonical and hyphenated forms on every resolution.

  11. [LOW:60] Silent failure pattern on database error — ToolPermissionSnapshot::load() returns Self::default() without // silent-ok: comment.

ironclaw/src/bridge/tool_permissions.rs

Lines 29 to 35 in f10404e

Err(error) => {
tracing::warn!(
user_id,
error = %error,
"Failed to load tool permissions for engine v2"
);
Self::default()

Base automatically changed from staging-promote/91c4c7ca-25018206332 to main April 29, 2026 04:09
@henrypark133
henrypark133 merged commit 983a95c into main Apr 29, 2026
130 of 151 checks passed
@henrypark133
henrypark133 deleted the staging-promote/f11a49be-25021901653 branch April 29, 2026 04:09

This branch had an error being deployed

1 failed and 5 inactive deployments
Ironclaw-QA / production — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
cosmose-ironclaw / production — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
humble-cat / staging-cameron — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
Near Foundation Ironclaw / production — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
ironclaw-nearai / production — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
venice-ironclaw / production — f11a49be Deployed Apr 27, 2026 by railway-app[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: M 50-199 changed lines staging-promotion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@henrypark133