chore: promote staging to main (2026-03-11 03:47 UTC) - #917
Conversation
* Add generic host-verified webhook ingress for tools * Stabilize trace E2E test rig and approval behavior * Fix webhook security issues from review feedback - Reject tools without webhook_capability() (was unauthenticated RCE) - Remove secret-in-query-string fallback (leak via logs/referrers) - Require approval for event_emit tool (escalation via routine triggers) - Simplify header_value() (HeaderMap already case-insensitive) - Redact internal errors from webhook HTTP responses - Remove unused hmac_timestamp_tolerance_secs field - Add regression test for tool without webhook capability [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Harden webhook ingress: require auth mechanism, body limit layer, health check - Reject webhook capabilities that declare no auth mechanism (empty WebhookCapability would previously allow unauthenticated access) - Add DefaultBodyLimit layer to reject oversized payloads before buffering - Health check (GET) now verifies tool has webhook_capability(), not just existence - Add regression tests for all three fixes [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix auto_approve_tools inconsistency between dispatcher and thread_ops dispatcher.rs skips all approval checks (including Always) when auto_approve_tools is true, but thread_ops.rs still required approval for Always tools. This caused deferred tool calls to unexpectedly halt in test rigs and auto-approve configurations. Match dispatcher behavior: short-circuit all approval when auto_approve_tools is enabled. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Code reviewFound 6 issues:
The
Webhook signature header names are
Schema allows all webhook fields to be
https://github.com/anthropics/ironclaw/blob/486e417/src/main.rs#L281-L284
If webhook routes are registered but secrets store wasn't injected, requests fail at runtime. Should be caught at server startup.
Tool webhook errors lack context about what the tool actually returned. Green lights: ✓ Constant-time signature verification ✓ No .unwrap() in production code ✓ Comprehensive test coverage ✓ Good separation of concerns ✓ Proper async/Arc usage |
|
Bug Scan Update: Found one issue worth investigating: [LOW:60] Test assertion weakened without explanation In
This should be clarified in the PR description or git history. |
|
Performance & Production Review Update: Found 2 MEDIUM severity performance/security issues: [MEDIUM:70] Unbounded query parameters allow potential DoS attacks Query parameters are collected into HashMap with no size limits. A malicious client could send thousands of query parameters to cause memory exhaustion. Recommend: Add query parameter count/size validation in webhook handlers. [MEDIUM:68] Inefficient header HashMap allocation in hot path On every webhook request, all HTTP headers are converted to HashMap by cloning both keys and values to Strings (2 allocations per header). For typical webhooks with 25-50 headers, this could be 50-100+ allocations per request. Recommend: Use references or BTreeMap, or defer HashMap creation until actually needed by the tool. Additional LOW-severity findings:
Overall: Good async/await patterns, proper timeouts and body limits. Production-ready with optimizations noted above. |
|
Security & Safety Review Update: Found 2 MEDIUM severity security issues that should be addressed before merge: [MEDIUM:85] Information disclosure via secret names in error messages Error messages reveal which secrets are configured (e.g., 'Missing webhook secret github_webhook_secret'). Attackers can learn the naming convention and integrated services. Recommend: Return generic 'Authentication failed' messages. Log secret names internally only. [MEDIUM:75] Potential timing side-channel in fallback header lookup Header fallback lookup has variable timing before constant-time comparison. Attackers could measure response times to determine which header names are configured. Recommend: Always attempt both header lookups with constant timing. Additional LOW-severity findings:
Positive: Good use of constant-time comparison (ct_eq) for secret validation, proper signature verification pattern. |
Address three deferred implementation items flagged during code review: 1. SIGHUP lock held across .await (#883): Split restart_with_addr into merged_router_clone() + install_listener() so the async TcpListener bind happens outside the mutex, eliminating lock contention risk. 2. Recursion depth limit for check_strings (#848): Cap JSON traversal at 32 levels to prevent stack overflow on pathological tool params. 3. Named error type for add_tokens (#788): Replace Result<(), String> with TokenBudgetExceeded { used, limit } for type-safe budget errors. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
… translations (#929) * feat(i18n): Add internationalization support with Chinese and English translations * fix(i18n): fix duplicate keys, broken placeholders, and dead overrides --------- Co-authored-by: zwb1982 <133180666+zwb1982@users.noreply.github.com>
… translations (#929) (#950) * feat(i18n): Add internationalization support with Chinese and English translations * fix(i18n): fix duplicate keys, broken placeholders, and dead overrides --------- Co-authored-by: jinxin <106428113+italic-jinxin@users.noreply.github.com> Co-authored-by: zwb1982 <133180666+zwb1982@users.noreply.github.com>
* fix(ci): use explicit features in WASM WIT compat test to avoid sqlite3 symbol conflicts The `import` feature (added in #903) brings in `rusqlite[bundled]` which conflicts with `libsql-ffi` — both bundle SQLite C code, causing duplicate symbol linker errors. Use explicit features matching the test matrix instead of `--all-features`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: replace rusqlite with libsql in import module to fix sqlite3 symbol conflict The `import` feature used `rusqlite[bundled]` which bundled its own SQLite C code, conflicting with `libsql-ffi` (also bundles SQLite). This caused duplicate `sqlite3_*` symbol linker errors when both features were enabled via `--all-features`. Replace `rusqlite` with `libsql` (already a dependency) in the import reader. The `import` feature now implies `libsql`. This eliminates the duplicate symbol conflict and allows `--all-features` to compile cleanly. Also restores `--all-features` in the WASM WIT compat CI test (now safe) and converts all import test helpers from rusqlite to libsql. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply cargo fmt formatting fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- Use fetch-depth: 0 in update-tag to ensure current_head SHA is available even when staging receives new commits during the CI run - Only merge promotion PRs targeting main; leave chained PRs open to prevent delete_branch_on_merge from auto-closing downstream PRs Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
The telegram-tests, windows-build, wasm-wit-compat, and docker-build jobs were skipped during staging CI because their `if` conditions only matched `push` and `pull_request` events. When staging-ci.yml calls test.yml via workflow_call, github.event_name is `schedule` (inherited from the caller), which matched neither condition. Invert the conditions to blocklist the one case we want to skip (PRs targeting staging) instead of allowlisting specific events. This handles schedule, workflow_dispatch, and any future trigger types. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
The Claude review step was failing ~40% of the time because: - --allowedTools didn't include Read, Glob, Grep, Agent, causing 8-9 permission denials per run and preventing Claude from reading files or spawning the subagents the prompt required - Step 4 spawned N additional scoring agents per issue found, exhausting the 50-turn budget before the PR comment could be posted - Subagents could independently post PR comments, causing fragmented output Fix: add missing tools to --allowedTools, merge per-issue scoring into the review agents themselves, and add guardrails ensuring exactly one consolidated comment is always posted. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
chore: promote staging to main (2026-03-11 21:09 UTC)
chore: promote staging to main (2026-03-11 19:17 UTC)
chore: promote staging to main (2026-03-11 07:18 UTC)
…935740447 chore: promote staging to main (2026-03-11 03:47 UTC)
…935740447 chore: promote staging to main (2026-03-11 03:47 UTC)
Auto-promotion from staging CI
Batch range:
55b5a462a2d2056cebc8cb4dec1680bff925e01a..369741fc60bf4ec1a28445c23d99db4a7f9c04c3Promotion branch:
staging-promote/369741fc-22935740447Base:
staging-promote/55b5a462-22934480277Triggered by: Staging CI batch at 2026-03-11 03:47 UTC
Waiting for gates:
Auto-created by staging-ci workflow