Repository navigation
fix(filesystem): pool libSQL connections to stop concurrent-CAS SQLITE_MISUSE (#5466) - #5751
Merged
Merged
Conversation
…E_MISUSE (#5466) LibSqlRootFilesystem opened a fresh connection (sqlite3_open_v2 + PRAGMA batch) for every RootFilesystem operation. Under genuinely parallel CAS storms against one WAL database that unbounded open/PRAGMA/close churn intermittently fails inside the C library with SQLITE_MISUSE ("bad parameter or other API misuse") or spurious disk I/O errors — the ~10% failure / SIGABRT reported in #5466. A single shared connection is also wrong: the CAS rows-affected readback is per-connection state, and two tasks interleaving statements on one connection corrupt compare-and-swap into silent lost updates (reproduced during diagnosis). Fix: a bounded deadpool-managed pool (same pooling core the Postgres backend already uses) in the new libsql_pool module — each operation checks out one PRAGMA-initialized connection for exclusive use and returns it on drop; recycle() rejects connections left mid-transaction. put()'s three CasExpectation arms now drop their checkout before the nested current_version readback, upholding the documented one-checkout-per-call-stack invariant. Regression test: tests/concurrent_cas_storm.rs drives 16 spawned writers x 100 cas_update increments on a multi-thread runtime against in-memory, libSQL, and (env-gated) Postgres backends, asserting zero backend errors and an exact final count. Mutation-verified: re-injecting per-op churn (recycle always discarding) goes RED with the exact SQLITE_MISUSE signature; the shared-connection probe goes RED on the lost-update assertion. Closes #5466 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
⏳ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5751
July 7, 2026 01:58
Destroyed
github-actions
Bot
added
scope: dependencies
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds deadpool-backed libSQL pooling, rewires filesystem checkout to use pooled connections, and expands regression coverage for concurrent CAS writes and missing-path version mismatches. ChangeslibSQL pooling and filesystem wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 58bf13cbf303a992dd9af007728e8319b796937d
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking issues found in the libSQL connection-pool change. The PR keeps the change scoped to ironclaw_filesystem, preserves per-operation connection exclusivity, and adds a multi-threaded CAS storm regression test covering libSQL, Postgres, and in-memory backends.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_filesystem/src/libsql.rs (1)
2037-2061: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry helper is tested directly, not through the real pool call site.
connect_retries_transient_open_failures_before_succeedingcallsconnect_with_retrywith a synthetic closure, bypassingLibSqlConnectionManager::create()(the actual production call site invoked bypool.get()). This is a pre-existing test relocated by the import change, but now that the retry logic backs adeadpoolManager::create, there's no test asserting the pool itself surfaces/retries transient opens correctly end-to-end (only the free function is exercised).Consider driving this through
LibSqlConnectionManager::create()(or the pool checkout) directly so the test covers the real call site rather than only the helper.As per path instructions: "Test through the caller: when a helper gates a side effect, require a test driving the real call site (handler/factory/manager), not only the helper."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_filesystem/src/libsql.rs` around lines 2037 - 2061, The retry test only exercises connect_with_retry directly with a synthetic closure, so it does not cover the real production path. Update connect_retries_transient_open_failures_before_succeeding to drive the retry logic through LibSqlConnectionManager::create() or an actual pool checkout via pool.get(), so the test validates transient open failures at the deadpool Manager::create call site instead of only the helper.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_filesystem/tests/concurrent_cas_storm.rs`:
- Around line 129-161: The silent skip paths in
postgres_concurrent_cas_storm_has_no_errors_or_lost_updates need inline
justification at each fallback, not just the top-level note. Add a `//
silent-ok: ...` comment beside each `let Ok(...) = ... else { return; }` and the
`root.run_migrations().await.is_err()` return, naming the specific operation
being skipped (env var read, config parse, pool build, and migrations) so the
intent is clear where the `IRONCLAW_FILESYSTEM_POSTGRES_URL`,
`tokio_postgres::Config`, `deadpool_postgres::Pool::builder`, and
`PostgresRootFilesystem::run_migrations` checks occur.
---
Outside diff comments:
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 2037-2061: The retry test only exercises connect_with_retry
directly with a synthetic closure, so it does not cover the real production
path. Update connect_retries_transient_open_failures_before_succeeding to drive
the retry logic through LibSqlConnectionManager::create() or an actual pool
checkout via pool.get(), so the test validates transient open failures at the
deadpool Manager::create call site instead of only the helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26f4de5b-b509-462d-93ff-4585e207d21c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (5)
crates/ironclaw_filesystem/Cargo.tomlcrates/ironclaw_filesystem/src/lib.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/libsql_pool.rscrates/ironclaw_filesystem/tests/concurrent_cas_storm.rs
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.32% — 279168 / 327190 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-5751 environment in ironclaw-ci-preview
|
henrypark133
commented
Jul 7, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Pool libSQL filesystem connections to prevent concurrent CAS SQLITE_MISUSE failures without changing filesystem traits or callers.
Stats: 4 findings (from 5 raw, 4 after dedup/filter) across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Tests
- Medium Open-transaction recycle rejection is untested (
crates/ironclaw_filesystem/src/libsql_pool.rs:124-129, confidence 100) — anchor:crates/ironclaw_filesystem/src/libsql_pool.rs:124
The pool manager now rejects a connection returned while not in autocommit, but no test exercises that branch. This is part of the PR's stated safety contract for preventing a failed rollback from leaking an open transaction to the next caller. - Medium Exhausted connect retry path is untested (
crates/ironclaw_filesystem/src/libsql_pool.rs:183-196, confidence 100) — anchor:crates/ironclaw_filesystem/src/libsql_pool.rs:183
The existing test covers transient open failures that eventually succeed, but not the new failure path after all retry attempts are exhausted and surfaced as aConnectinfrastructure error.
Local Patterns
- Low Log libSQL pool checkout failures like Postgres (
crates/ironclaw_filesystem/src/libsql.rs:99-104, confidence 75) — anchor:crates/ironclaw_filesystem/src/postgres.rs:81
The new libSQL pool checkout path returns checkout failures without the debug log used by the sibling Postgres backend for the same failure shape, making checkout timeouts harder to correlate in traces.
Maintainability
- Medium Pool safety is encoded as a manual drop invariant (
crates/ironclaw_filesystem/src/libsql.rs:173-175, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql_pool.rs:22
The new pool relies on ordinary filesystem methods remembering to drop a checkout before calling any helper that may check out again. That rule is enforced by comments and scattered explicitdrop(conn)calls input, so future helper calls require manual audit for self-exhaustion.
Note: I did not duplicate the existing CodeRabbit thread about // silent-ok: comments in the Postgres storm test.
| // A connection returned mid-transaction (e.g. a failed ROLLBACK in | ||
| // `run_migrations`) must not be handed to an unrelated caller — | ||
| // reject it here so the pool discards it and opens a fresh one. | ||
| if connection.is_autocommit() { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Open-transaction recycle rejection is untested.
The pool manager now rejects a connection returned while not in autocommit, but no test exercises that branch. This is part of the PR's stated safety contract for preventing a failed rollback from leaking an open transaction to the next caller.
Fix: Add a crate test such as tests::libsql_pool::recycle_rejects_connection_returned_inside_transaction that checks out a libSQL connection, starts BEGIN, returns it, and asserts the pool discards/replaces it instead of handing the in-transaction connection back out.
| } | ||
| } | ||
|
|
||
| let reason = match last_error { |
Collaborator
Author
There was a problem hiding this comment.
Medium — Exhausted connect retry path is untested.
The existing test covers transient open failures that eventually succeed, but not the new failure path after all retry attempts are exhausted and surfaced as a Connect infrastructure error.
Fix: Add tests::libsql_pool::connect_with_retry_returns_connect_error_after_exhausting_open_failures with an opener that fails for all LIBSQL_CONNECT_ATTEMPTS, then assert the returned error uses FilesystemOperation::Connect and includes the final cause.
| /// `self` method that also checks out — see the invariant note in | ||
| /// [`crate::libsql_pool`]. | ||
| async fn connect(&self) -> Result<PooledLibSqlConnection, FilesystemError> { | ||
| self.pool.get().await.map_err(|error| match error { |
Collaborator
Author
There was a problem hiding this comment.
Low — Log libSQL pool checkout failures like Postgres.
The new libSQL pool checkout path returns checkout failures without the debug log used by the sibling Postgres backend for the same failure shape, making checkout timeouts harder to correlate in traces.
Fix: Build the checkout failure reason first, emit tracing::debug!(%reason, "libSQL root filesystem pool checkout failed"), then return the existing FilesystemOperation::Connect infrastructure error.
| .map_err(|error| { | ||
| libsql_db_error(path.clone(), FilesystemOperation::WriteFile, error) | ||
| })?; | ||
| // Release the checkout before `current_version` claims its |
Collaborator
Author
There was a problem hiding this comment.
Medium — Pool safety is encoded as a manual drop invariant.
The new pool relies on ordinary filesystem methods remembering to drop a checkout before calling any helper that may check out again. That rule is enforced by comments and scattered explicit drop(conn) calls in put, so future helper calls require manual audit for self-exhaustion.
Fix: Move the invariant into connection-taking query helpers where practical, starting with a current_version_with_conn(&libsql::Connection, ...) helper so put() can reuse its checkout for version readbacks instead of dropping and re-checking out.
henrypark133
added a commit
that referenced
this pull request
Jul 7, 2026
HsmBackend advertised Delete+Cas but fell through to Unsupported at runtime; now delegates to its inner backend. Adds scoped-mount permission/routing tests, drops the libsql delete's connection before the zero-rows current_version lookup (pool-exhaustion guard ahead of #5751), and adds Any-branch contract tests for libsql and postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- put()'s CAS-mismatch/success version readbacks reuse the already checked-out connection via a new current_version_with_conn helper instead of drop-then-recheckout, making the one-checkout-per-call- stack invariant structural for that call site. - Add pool-internal tests: recycle rejects a connection returned mid-transaction, and connect_with_retry surfaces a Connect error with the final cause after exhausting its retry budget. - Log libSQL pool checkout failures at debug level, mirroring the Postgres backend's shape, for trace correlation on checkout timeouts. - Annotate the four silent-skip fallbacks in the Postgres CAS-storm test with per-site silent-ok rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5751
July 7, 2026 04:14
Destroyed
Collaborator
Author
|
Addressed all review findings in 73a6775.
Verification: |
github-actions
Bot
added
size: XL
henrypark133
commented
Jul 7, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Pool libSQL filesystem connections to eliminate concurrent CAS SQLITE_MISUSE failures without changing RootFilesystem callers or CAS semantics.
Stats: 3 findings (from 6 raw, 3 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Conventions
- Medium Keep current_version out of the public API (
crates/ironclaw_filesystem/src/libsql.rs:1522-1522, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:1522
The diff promotescurrent_versionfrom private helper to public inherent method on the exportedLibSqlRootFilesystem, but the PR states this is internal-only and all in-repo call sites usecurrent_version_with_conn. That creates a libSQL-only API outside the RootFilesystem trait without a caller or rationale.
Tests
- Medium libSQL missing-path CAS version branch is untested (
crates/ironclaw_filesystem/src/libsql.rs:219-228, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:223
CasExpectation::Versionnow usescurrent_version_with_connon the pooled connection before returningVersionMismatch. Existing libSQL CAS tests cover stale existing paths wherefoundisSome, but not the missing-path error case wherefoundmust beNone; only the Postgres suite has that regression test. - Low Pool checkout timeout mapping has no test (
crates/ironclaw_filesystem/src/libsql.rs:99-108, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql.rs:99
The newconnectpath maps non-backend pool checkout failures, such as an exhausted pool wait timeout, intoFilesystemOperation::Connect. The tests cover PRAGMA setup and connect retry failures, but no adjacent or crate integration test exhausts the libSQL pool and asserts the checkout-timeout error shape.
Notes: the previous review's open-transaction recycle and exhausted connect-retry test findings appear addressed on this head; this forced pass did not carry those stale findings forward.
| /// checked-out connection (e.g. `put()`'s version-mismatch and | ||
| /// success-path readbacks) call `current_version_with_conn` directly | ||
| /// instead, to keep at most one pooled connection per call stack. | ||
| pub async fn current_version( |
Collaborator
Author
There was a problem hiding this comment.
Medium — Keep current_version out of the public API.
The diff promotes current_version from private helper to public inherent method on the exported LibSqlRootFilesystem, but the PR states this is internal-only and all in-repo call sites use current_version_with_conn. That creates a libSQL-only API outside the RootFilesystem trait without a caller or rationale.
Fix: Change pub async fn current_version back to private, delete the wrapper if it has no caller, or use pub(crate) only if a crate-local test seam actually needs it.
Also flagged by: tests/Low, local-patterns/Low, maintainability/Low
Collaborator
Author
There was a problem hiding this comment.
Fixed. Deleted the pub current_version method entirely rather than making it private/pub(crate) — it had zero callers even inside the crate (all in-crate use went through current_version_with_conn), and main's #5724 (Optimize RootFilesystem latency substrate, merged concurrently) independently deleted the same method in favor of a free function (current_version_libsql) called from the new transactional put_libsql_inner. After adopting that transactional structure during the merge, this branch's own current_version_with_conn became a dead duplicate of current_version_libsql, so it's deleted too. Net: no wrapper, no dead code, one canonical version-lookup helper. See f6a9764.
| // Reuse the connection already checked out above rather | ||
| // than dropping and re-checking-out — one pooled | ||
| // connection for the whole call stack, structurally. | ||
| let found = current_version_with_conn(&conn, path).await?; |
Collaborator
Author
There was a problem hiding this comment.
Medium — libSQL missing-path CAS version branch is untested.
CasExpectation::Version now uses current_version_with_conn on the pooled connection before returning VersionMismatch. Existing libSQL CAS tests cover stale existing paths where found is Some, but not the missing-path error case where found must be None; only the Postgres suite has that regression test.
Fix: Add a libSQL contract test mirroring postgres_put_cas_version_on_missing_path_reports_no_found_version, asserting VersionMismatch { found: None, .. } for a missing path.
Collaborator
Author
There was a problem hiding this comment.
Fixed. Added libsql_native_put_cas_version_on_missing_path_reports_no_found_version in tests/db_root_filesystem_contract.rs, mirroring the Postgres test exactly: put with CasExpectation::Version on a path with no row asserts VersionMismatch { found: None, .. }. Mutation-verified — forcing found = Some(expected) on that branch turns the test red with a clear assertion failure; reverted after confirming. See f6a9764.
| /// drops. Callers must drop the guard before `.await`-ing any other | ||
| /// `self` method that also checks out — see the invariant note in | ||
| /// [`crate::libsql_pool`]. | ||
| async fn connect(&self) -> Result<PooledLibSqlConnection, FilesystemError> { |
Collaborator
Author
There was a problem hiding this comment.
Low — Pool checkout timeout mapping has no test.
The new connect path maps non-backend pool checkout failures, such as an exhausted pool wait timeout, into FilesystemOperation::Connect. The tests cover PRAGMA setup and connect retry failures, but no adjacent or crate integration test exhausts the libSQL pool and asserts the checkout-timeout error shape.
Fix: Add a focused libSQL test that exhausts the pool or uses a small test pool and asserts checkout timeout maps to FilesystemOperation::Connect.
Collaborator
Author
There was a problem hiding this comment.
Fixed. Added a build_libsql_pool_with_config test/config seam (parameterizes max_size/wait_timeout behind the existing build_libsql_pool) and a new test, connect_maps_pool_checkout_timeout_to_connect_infrastructure_error, that builds a size-1/50ms-timeout pool, holds the only connection, and asserts the second checkout fails as FilesystemError::BackendInfrastructure { operation: FilesystemOperation::Connect, .. } — i.e. hits the other debug!-logged arm in connect(). Mutation-verified — changing that arm's operation to FilesystemOperation::Stat turns the test red; reverted after confirming. See f6a9764.
…-cas-5466 # Conflicts: # crates/ironclaw_filesystem/src/libsql.rs
- Delete the pub current_version wrapper: main's #5724 independently removed it in favor of the free-function current_version_libsql used inside the transactional put_libsql_inner path, confirming it had no external caller. The pool-typed current_version_with_conn duplicate is dead after adopting that transactional structure; deleted too. - Add a libsql contract test mirroring postgres_put_cas_version_on_missing_path_reports_no_found_version: CasExpectation::Version against a missing path must report VersionMismatch { found: None }. - Add a pool checkout-timeout test via a new build_libsql_pool_with_config seam (tiny size-1/short-timeout pool) asserting the timeout maps to a FilesystemOperation::Connect infrastructure error through connect()'s debug!-logged fallback arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5751
July 7, 2026 17:21
Destroyed
Collaborator
Author
|
Merged origin/main (a8053cc) into this branch and resolved the one real conflict. Confirming scope for reviewers: Merge conflict: only Verification post-merge: Replied per-finding on the three review threads (current_version deletion, missing-path CAS-version test, pool-checkout-timeout test) — each mutation-verified (RED on the reintroduced bug, reverted). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 145-172: The transaction flow in `put_libsql`/`put_libsql_inner`
now holds a pooled connection while waiting on `BEGIN IMMEDIATE`, so sustained
write bursts can exhaust the 8-connection pool before the checkout timeout. Add
or expose pool-wait and SQLite busy-timeout telemetry around the
`connect`/`BEGIN IMMEDIATE` path so production write storms can be monitored,
and ensure the metrics distinguish time spent waiting for a pooled slot versus
time blocked on `BEGIN IMMEDIATE`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93e02839-5782-470c-80e3-e36d85a45f8b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (3)
crates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/libsql_pool.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 145-172: The transaction flow in `put_libsql`/`put_libsql_inner`
now holds a pooled connection while waiting on `BEGIN IMMEDIATE`, so sustained
write bursts can exhaust the 8-connection pool before the checkout timeout. Add
or expose pool-wait and SQLite busy-timeout telemetry around the
`connect`/`BEGIN IMMEDIATE` path so production write storms can be monitored,
and ensure the metrics distinguish time spent waiting for a pooled slot versus
time blocked on `BEGIN IMMEDIATE`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93e02839-5782-470c-80e3-e36d85a45f8b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (3)
crates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/libsql_pool.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rs
🛑 Comments failed to post (1)
crates/ironclaw_filesystem/src/libsql.rs (1)
145-172: 🚀 Performance & Scalability | 🔵 Trivial
One checkout for the whole transaction, dir/child checks moved inside
BEGIN IMMEDIATE, and the operation error propagates whileROLLBACK/COMMIT-fail leaves a mid-tx connection forrecycleto discard. Correct.Operational note: under sustained write bursts past the 8-connection ceiling, writers blocked on
BEGIN IMMEDIATE(up tobusy_timeout5s) hold their pooled slot, so effective checkout throughput degrades before the 10s wait timeout trips. Worth watching pool-wait/busymetrics under production write storms, not just the regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_filesystem/src/libsql.rs` around lines 145 - 172, The transaction flow in `put_libsql`/`put_libsql_inner` now holds a pooled connection while waiting on `BEGIN IMMEDIATE`, so sustained write bursts can exhaust the 8-connection pool before the checkout timeout. Add or expose pool-wait and SQLite busy-timeout telemetry around the `connect`/`BEGIN IMMEDIATE` path so production write storms can be monitored, and ensure the metrics distinguish time spent waiting for a pooled slot versus time blocked on `BEGIN IMMEDIATE`.
henrypark133
commented
Jul 7, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Pool libSQL filesystem connections to prevent concurrent CAS SQLITE_MISUSE failures without changing filesystem traits or CAS callers.
Stats: 1 finding (from 1 raw, 1 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
tests
- Low Permanent PRAGMA setup failures lack a regression test (
crates/ironclaw_filesystem/src/libsql_pool.rs:201-235, confidence 75) — anchor:crates/ironclaw_filesystem/src/libsql_pool.rs:201
The retry helper now handles PRAGMA setup failures after a connection opens, and tests cover a transient PRAGMA failure followed by success plus permanent open failures. They do not cover the branch where every open succeeds but every PRAGMA batch fails, so the final Connect infrastructure error path for post-open initialization remains unexercised.
| // `execute_batch` runs each statement and discards the rows | ||
| // PRAGMAs like `busy_timeout` return, which is exactly what | ||
| // we want — we only care about the side effect. | ||
| match conn.execute_batch(pragmas_for_attempt(attempt)).await { |
Collaborator
Author
There was a problem hiding this comment.
Low — Permanent PRAGMA setup failures lack a regression test.
The retry helper now handles PRAGMA setup failures after a connection opens, and tests cover a transient PRAGMA failure followed by success plus permanent open failures. They do not cover the branch where every open succeeds but every PRAGMA batch fails, so the final Connect infrastructure error path for post-open initialization remains unexercised.
Fix: Add a libsql_pool test such as connect_with_retry_returns_connect_error_after_exhausting_pragma_failures that uses connect_with_retry_and_pragmas with a real opener and an always-invalid PRAGMA batch, then asserts LIBSQL_CONNECT_ATTEMPTS attempts and FilesystemOperation::Connect with the final setup error in the reason.
Collaborator
Author
There was a problem hiding this comment.
Fixed in 0876cb5: added connect_with_retry_returns_connect_error_after_exhausting_pragma_failures — real opener succeeds every attempt, PRAGMA batch is always invalid SQL; asserts opens == initializers == LIBSQL_CONNECT_ATTEMPTS and the final FilesystemError::BackendInfrastructure { operation: Connect, .. } reason carries the PRAGMA error cause.
Mutation-checked: made the retry helper treat a PRAGMA failure as success (Err(_) => return Ok(conn)) — new test failed RED for the right reason (opens stuck at 1 instead of retrying to 3), then reverted.
Verified: cargo fmt clean, cargo clippy --all --benches --tests --examples --all-features zero warnings, cargo test -p ironclaw_filesystem --all-features green (108+7+3+72+24 passed), concurrent CAS storm test green across 3 consecutive runs.
…_retry Every open succeeds but every PRAGMA batch fails across all retry attempts; final error must be FilesystemOperation::Connect carrying the PRAGMA cause. Addresses PR #5751 round-3 review finding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5751
July 7, 2026 19:26
Destroyed
henrypark133
commented
Jul 7, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Fix libSQL filesystem concurrent CAS failures by pooling bounded connections and adding storm regression coverage.
Stats: 0 new findings (from 2 raw reviewer candidates; 0 after validation, source checks, and live duplicate suppression) across 0 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
No new non-duplicate findings survived this forced pass. The two raw candidates were suppressed because the caller-level pool error-path coverage concern is covered by the current retry/recycle/checkout-timeout test work and live review discussion, and the writer-lock/pool-headroom concern is already disclosed in existing PR review/comment text.
henrypark133
added a commit
that referenced
this pull request
Jul 8, 2026
) * feat(filesystem): add CAS-guarded delete_if_version to RootFilesystem put has a CAS precondition (CasExpectation) but delete is blind, so no caller can remove a record only-if-unchanged. Add an additive trait method delete_if_version(path, expected) with a default Unsupported impl (put's pattern) — ~20 blind-delete call sites are untouched. Semantics: single-key only (no subtree/event-log/sequence sweep, unlike blind delete); absent row → NotFound (already gone, benign) vs row at another version → VersionMismatch{expected, found} (gone stale) — a new two-branch diagnosis, since put's CAS paths collapse absent into VersionMismatch{found: None}; CasExpectation::Absent is meaningless for a delete and fails closed with Unsupported (decoded once in CasExpectation::required_delete_version, shared by all backends). Implemented for in-memory, libSQL, and Postgres (named SQL consts + single-round-trip/single-key pin test, mirroring the PUT_*_SQL idiom), with ScopedFilesystem and CompositeRootFilesystem passthroughs. Tests: dual-backend contract coverage (correct-version delete, stale version loses with entry surviving at the bumped version, missing path, single-key event-log survival) in db_root_filesystem_contract.rs plus in-memory crate tests and composite routing in catalog_contract.rs; Postgres tests skip without a reachable DB per suite convention. Mutation-checked: collapsing absent into VersionMismatch and deleting despite a mismatch each turn the pinned assertions red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filesystem): HSM delete_if_version delegation + CAS-delete coverage HsmBackend advertised Delete+Cas but fell through to Unsupported at runtime; now delegates to its inner backend. Adds scoped-mount permission/routing tests, drops the libsql delete's connection before the zero-rows current_version lookup (pool-exhaustion guard ahead of #5751), and adds Any-branch contract tests for libsql and postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(filesystem): CAS-delete Absent-rejection + scoped stale-version coverage Round-2 review fixes for #5749: - postgres/libsql: Absent rejects with Unsupported, entry preserved - scoped: stale Version through delete_if_version surfaces VersionMismatch and forwards `expected` rather than swallowing it - CasExpectation rustdoc now covers delete_if_version semantics Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(reborn): delete_if_version wrapper delegation + ABA doc + Version-only API Round 3 review fixes for PR #5749: - MountScopedRootFilesystem (ironclaw_host_runtime) and SkillManagementRootFilesystem (ironclaw_skills) both forward capabilities() to their inner backend but omitted delete_if_version, so a CAS-capable mount silently fell through to the trait default Unsupported. Both now resolve/delegate delete_if_version like their existing delete override. Workspace-wide audit of every RootFilesystem-wrapping decorator found only these two plus the already-fixed HsmBackend and the already-correct CompositeRootFilesystem — delegation tests added for both new fixes and mutation-checked (MountScoped) by reverting the override. - Maintainer verdict on the ABA finding: narrow delete_if_version's signature from CasExpectation to a plain RecordVersion (Any/Absent are unrepresentable for a delete) and document the ABA hazard as a caller invariant on the trait method's rustdoc instead of adding generation-stable tokens. Threaded the signature change through in_memory/libsql/postgres/scoped/catalog/hsm and every call site; along the way this also fixed a pre-existing libsql.rs build break (delete_if_version referenced a nonexistent self.current_version). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filesystem): atomic CAS-delete classification + overflow coverage Review round 4 (PR #5749): the zero-rows NotFound/VersionMismatch diagnosis in delete_if_version ran as a second, separate statement on both libSQL and Postgres, leaving a window for a concurrent delete+recreate to commit in between and misclassify the outcome. libSQL now wraps the conditional DELETE and the diagnosis SELECT in one BEGIN IMMEDIATE/COMMIT transaction on a single connection (same idiom as put). Postgres folds both into one WITH statement: a `locked` CTE takes SELECT ... FOR UPDATE, and the `deleted` CTE depends on it so the lock is held before the conditional delete runs; the SQL pin test now asserts FOR UPDATE and the CTE dependency. Also adds the missing out-of-range expected_version coverage on both backends (RecordVersion::from_backend(u64::MAX) -> CorruptRecordVersion, entry preserved), and syncs the PR body's stale HsmBackend-out-of-scope claim with the actual diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filesystem): postgres delete_if_version CTE path projection + default-Unsupported test Round-5 review findings on PR #5749: 1. `DELETE_IF_VERSION_ATOMIC_SQL`'s `locked` CTE only projected `version`, while the `deleted` CTE's predicate referenced `path IN (SELECT path FROM locked)`. Verified live against Postgres 16 (EXPLAIN VERBOSE): Postgres does not error on the missing projection — it silently resolves the unqualified `path` as a correlated reference back to the outer DELETE's own row, making the join trivially self-referential rather than a real semi-join against `locked`. That happened to still return correct results here because the DELETE's other predicates already pin the exact row, but it's a latent correctness footgun tied to a design intent (force lock-then-delete sequencing via a genuine CTE dependency) that the implicit correlation doesn't actually provide. Fix: project `path` explicitly in `locked`'s SELECT list, so the semi-join is real. Confirmed identical behavior before/after via direct psql repro (delete-success, version-mismatch, path-absent) and the full live postgres_tests suite (76/76). 2. Added `delete_if_version_default_returns_unsupported` in root.rs, reusing the existing `DefaultBoundedBackend` test fixture, to pin the previously-untested trait-default `Unsupported` arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(filesystem): Round-A review fixes — delete_if_version concurrency + ABA pin Round-A self-review (3 sonnet subagents: bugs/correctness, concurrency, tests) over the full PR diff. Bugs/correctness reviewer found nothing beyond the already-fixed CTE bug. Fixes from the other two lenses: Concurrency: - libsql delete_if_version now validates expected_version (overflow guard) before self.connect()/BEGIN IMMEDIATE, so an out-of-range version fails closed before taking a pool checkout and the SQLite write lock — avoids needless contention under CAS storms. - Reworded three test comments (in_memory.rs, db_root_filesystem_contract.rs x2) that overclaimed "concurrent-writer interleaving" for what are actually single-task sequential scripts; they now point at concurrent_cas_storm.rs for genuine parallel coverage. Tests: - Added a delete_if_version concurrency storm (all 3 backends): WRITERS-way contention racing to delete_if_version the same path at a known version across DELETE_STORM_ROUNDS recreate/delete cycles, asserting exactly one winner per round and no backend/infrastructure errors. The prior storm test only ever exercised put/cas_update concurrency — delete_if_version (this PR's new op, and the target of the BEGIN IMMEDIATE/FOR UPDATE atomicity fix) had none. - Added delete_if_version_is_vulnerable_to_aba_across_delete_recreate_cycles in in_memory.rs, pinning the ABA hazard the trait doc comment already documents (version restarts at 1 after a full delete, so a stale token can match a later incarnation) — previously asserted only in prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(filesystem): Round-B review fixes — deterministic atomicity pin + ABA parity + postgres ordering Round-B self-review (3 fresh sonnet subagents, same 3 lenses, over the Round-A-updated diff, explicitly checking Round A for regressions). Bugs/correctness and concurrency reviewers found no regressions from Round A. Fixes from the tests lens plus one concurrency polish item: - Round A's new delete_if_version concurrency storm (concurrent_cas_storm.rs) is real coverage for N-way contention/pool-exhaustion, but a sharp tests-lens finding showed it cannot discriminate a regression to the pre-1792aebb2 two-statement delete+diagnose race (every racer shares one pre-fetched version; nothing recreates the path mid-round, so ordinary single-row locking passes the test with or without that fix). Added a deterministic, non-flaky pin instead: libsql.rs::delete_if_version_diagnosis_reuses_the_delete_connection_under_a_size_one_pool builds a size-1 pool and drives the stale-version (0-rows) branch — if the diagnosis ever checks out a second connection, it deadlocks against itself and times out. Verified this discriminates: temporarily reintroduced a duplicate self.connect() and confirmed the test fails with a pool-checkout-timeout error, then reverted. - Reworded run_delete_storm's doc comment to stop overclaiming it guards the atomicity fix; it now correctly describes what it proves (contention/pool-exhaustion correctness) and points at the new deterministic test and postgres's existing SQL-shape pin test for the atomicity regression itself. - Tightened the storm's win assertion to a per-round check (was summed across all DELETE_STORM_ROUNDS, so a 0-win round and a 2-win round elsewhere could have canceled out) — Round-B concurrency finding. - postgres delete_if_version now validates expected_version before self.client() (was after), matching libsql's Round-A fix for the same reason — consistency finding from the tests lens. - Added libsql/postgres ABA-hazard pin tests mirroring in_memory.rs's Round-A test, since both DB backends share the same "version resets to 1 after full delete" precondition and had no equivalent pin. - Added composite_delete_if_version_returns_mount_not_found, mirroring the existing append_batch mount-not-found test (LOW finding, cheap gap-fill). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(filesystem): Round-C review — live-race atomicity proof + gap closes - Add a live two-connection Postgres integration test that drives the actual delete-then-recreate race delete_if_version's atomicity fix targets, asserting NotFound (not a misclassified VersionMismatch). Manually verified it fails against a reverted non-atomic implementation, confirming it's non-vacuous. - Harden the SQL string-inspection test against two concrete mutants that kept its substring checks green while breaking atomicity (FOR UPDATE SKIP LOCKED, misplaced version guard). - Add explicit-directory-row exclusion tests for delete_if_version on both DB backends (mirrors existing put-side coverage). - Prove the libsql size-1-pool connection returns to a clean, reusable state after a VersionMismatch, not just that it didn't deadlock. - Add delete=false-but-other-grants-true denial coverage at the MountScopedRootFilesystem layer (mirrors the ScopedFilesystem-layer test). - Document why delete_if_version takes a bare RecordVersion instead of CasExpectation; fix an unverifiable PR-number comment reference. Found by a fresh multi-lens review cycle (correctness/SQL-tracing, concurrency/pool, test-adequacy, API-contract, security, maintainability) run after 4 external + 2 internal review rounds; only test-adequacy gaps surfaced, no correctness/concurrency/security majors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
henrypark133
added a commit
that referenced
this pull request
Jul 8, 2026
… coverage, #5466 probe Cycle-3 fix lane for PR #5819, addressing verified review findings: - store.rs open(): add the §4.0 round-5 unconditional post-edge roster touch, closing the boot-recovery window where a stale-version delete could hide a scope from recovery forever. - store.rs prune_roster_if_parent_empty: add crate-tier coverage for the close-path's opportunistic roster prune (last edge closed -> pruned; sibling edge open -> marker survives). - ironclaw_turns: add prune_released_child coverage at the turn_coordinator_contract tier, pinned via the idempotency-key-reuse behavior the prune enables. - tests/integration/subagent_await_edge.rs: add the roster shard-walk enumeration test (>=3 scopes across distinct shards and every scope axis), per design doc line 179. - store.rs close_with_release: add a second crash-hook point (scenario (a), between the Released CAS and the prune) alongside the existing scenario (b) hook, with a matching recovery test. - resolver.rs: document the group-settle driver-election TOCTOU as accepted (idempotent downstream effects, bounded group size). - #5466 probe: #5751's deadpool fix holds -- 50 real LibSql runs + 40 InMemory runs of the parallel dual-gate scenario, 0 failures, 0 tolerated flakes. Removed the libsql exclusion and the now-unneeded retry-tolerance in scenario_concurrent_dual_gate_resume_parallel.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
henrypark133
added a commit
that referenced
this pull request
Jul 8, 2026
* feat(reborn): replace subagent gate store with CAS'd await-edge delivery (blocking mode)
Replaces the in-memory subagent gate/completion-observer mechanism with a
durable, CAS'd filesystem-backed "await edge" per (parent_run_id,
child_run_id), per docs/reborn/subagent-spawn/thread-harness-design.md
(§1-§8, §13 P1.x rows). Blocking mode only — background mode and the
edges CLI are PR2.
What moved:
- New crates/ironclaw_reborn/src/subagent/await_edge/{mod,store,roster,
resolver,boot_recovery}.rs: AwaitEdge CAS state machine (open -> settled
-> drained, open -> abandoned), scope roster (256-way blake3-shard,
percent-encoded __-joined filenames, write-marker-before-first-edge,
opportunistic + boot prune), settle/resume/close sequence, lazy
per-scope recovery.
- New AwaitEdgeWriter/AwaitEdgeSettler traits in ironclaw_loop_support
(DIP), concrete impls in ironclaw_reborn.
- Deleted gate_resolution.rs, tombstone_store.rs, completion_observer.rs
(~6.4k lines) and their production_readiness checks; resume/write-result
logic moved into the resolver; completion_observer's terminal-detection/
capacity-release duties stay, gate-store clusters removed.
- ironclaw_turns: reservation-release is now a durable tri-state
(Unclaimed -> Claimed -> Released) with an idempotency-keyed dedup
(release_tree_descendants(..., idempotency_key)) and a new
prune_released_child method to keep that dedup set bounded.
Spec deviations (all sound adaptations, not silent divergence):
- D3 batch-gate grouping (multiple children under one GateRef resuming
the parent exactly once) isn't addressed by the design doc — added an
additive `gate_ref` field on AwaitEdge + group-based settle logic.
- Added `parent_run_context: LoopRunContext` to AwaitEdge, captured once
at open/reconstruct time, to avoid a re-entrant turn-state-store call
from inside the child's own commit-triggered observer callback.
- Deferred-binding (OnceLock) for both `coordinator` and `result_writer`
on the resolver, mirroring the pre-existing bind_coordinator pattern,
since composition builds the result writer after the resolver.
- Non-libsql/non-postgres composition falls back to no-op
NonDurableAwaitEdgeSettler/NonDurableAwaitDependentRunEvidence, matching
the existing reduced-durability posture of that mode.
- Boot pass (run_boot_recovery) has no production call site in this PR
(background mode is PR2) and so has its own Semaphore, separate from
lazy recovery's — currently inert; needs a shared limiter when boot
pass is wired. Lazy recovery's fairness (bounded queue + per-tenant
cap) is not implemented, only a bare concurrency Semaphore.
- reconstruct_edge's parent-scope lookup is wrapped in a 10s timeout as
defense-in-depth against a possible re-entrant-lock class this PR
found and fixed once already (cached parent_run_context) but that this
recovery-only branch can't reuse (no edge was ever opened); the
timeout has no dedicated regression test proving/disproving the
concern under real production wiring.
Review cycles: two rounds of independent-agent review against the spec
found and fixed 6 real defects before merge candidacy, each mutation-
verified (defect reinjected, confirmed red, reverted, confirmed green):
resume_parent's benign-already-closed set was a wildcard match instead of
the spec's exact {Queued, Running, Completed}; released_children was
documented as pruned but never actually was (unbounded growth); a
concurrent mark_released version race surfaced as a hard close failure
instead of the spec's benign disposition; reconstruct_edge's rebuilt
parent scope defaulted thread_owner incorrectly (twice — first attempt
copied the child's own scope, which is itself always ActorFallback, not
the real recovered owner). A second review round did not get a fourth
independent test-adequacy/concurrency pass on the full diff (time-boxed);
flagging that gap rather than claiming a full 4-cycle clean bill.
Test evidence: cargo fmt clean, clippy zero warnings (--all --benches
--tests --examples --all-features), full workspace build, all touched
crate-tier suites green (ironclaw_turns, ironclaw_reborn, ironclaw_
loop_support, ironclaw_reborn_composition, ironclaw_product_workflow),
all 50 tests/integration targets green including the new
reborn_integration_subagent_await_edge (5 tests), and the previously
#[ignore]d blocking-mode e2e suite (5 tests, including the D3 batch-gate
parallel_blocking_spawn_resumes_once_after_last_child) passing again
against the new wiring.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(reborn): await-edge post-edge self-heal, roster/reservation prune coverage, #5466 probe
Cycle-3 fix lane for PR #5819, addressing verified review findings:
- store.rs open(): add the §4.0 round-5 unconditional post-edge roster
touch, closing the boot-recovery window where a stale-version delete
could hide a scope from recovery forever.
- store.rs prune_roster_if_parent_empty: add crate-tier coverage for the
close-path's opportunistic roster prune (last edge closed -> pruned;
sibling edge open -> marker survives).
- ironclaw_turns: add prune_released_child coverage at the
turn_coordinator_contract tier, pinned via the idempotency-key-reuse
behavior the prune enables.
- tests/integration/subagent_await_edge.rs: add the roster shard-walk
enumeration test (>=3 scopes across distinct shards and every scope
axis), per design doc line 179.
- store.rs close_with_release: add a second crash-hook point (scenario
(a), between the Released CAS and the prune) alongside the existing
scenario (b) hook, with a matching recovery test.
- resolver.rs: document the group-settle driver-election TOCTOU as
accepted (idempotent downstream effects, bounded group size).
- #5466 probe: #5751's deadpool fix holds -- 50 real LibSql runs + 40
InMemory runs of the parallel dual-gate scenario, 0 failures, 0
tolerated flakes. Removed the libsql exclusion and the now-unneeded
retry-tolerance in scenario_concurrent_dual_gate_resume_parallel.rs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(reborn): reconstruct_edge from cached metadata, shared boot/lazy semaphore
Cycle-4 fix lane for PR #5819, addressing arbitrated review findings:
- subagent_spawn_port.rs: SubagentThreadMetadata gains parent_run_context
+ gate_ref, populated in finish_spawn from values already in hand.
- resolver.rs: reconstruct_edge rebuilt as a pure data transformation off
the cached metadata -- zero turn_state_store calls for the parent, so
the re-entrant observer-callback deadlock no longer applies. Deletes
PARENT_RECORD_LOOKUP_TIMEOUT and its timeout wrapper. Anti-tamper
anchor preserved: scope/actor/thread_id/run_id come from the trusted
child record + recovered owner, never metadata wholesale.
recovered_gate_ref collapses to metadata.gate_ref for Blocking mode.
- boot_recovery.rs: run_boot_recovery takes the caller's Arc<Semaphore>
instead of constructing its own; ScopeRecoveryDriver::semaphore()
exposes the instance for future boot wiring to share.
- tests/integration/subagent_await_edge.rs: rollback-race backstop test,
substituting a direct store-level edge delete for the (unreachable
from this crate) capability-port rollback race, per the design's own
fallback.
- Design doc: corrects the lost-edge crash-window framing (rollback
race, not commit-vs-open), and splits the PR1/PR2 staging for the
bundled limiter fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(reborn): address external review on await-edge PR
Fixes multiple issues raised by codex, gemini, coderabbit, and
ironloopai on PR #5819 (subagent await-edge delivery):
- Multi-user parent scope mismatch (P1): child_turn_scope now mirrors
the parent's explicit thread owner instead of always defaulting to
ActorFallback, so the await-edge write and has_awaited_child_gate's
read land under the same resource scope.
- D3 batch-gate group drain misattributed the triggering/last-settling
sibling's status onto every member. AwaitEdge now carries a
terminal_reason field; drain_settled_group reads each member's own
terminal_kind/terminal_reason instead of a shared event.
- close_edge released spawn-tree capacity using parent_run_id instead
of the edge's own tree_root_run_id (latent while max_depth == 1, but
the close path is depth-agnostic).
- boot_recovery's Settled branch called close_edge directly, dropping
the parent's result write and resume on a crash-settled-but-undrained
edge; it now re-drives drain_settled_group.
- check_scope_recovered claimed the in_progress admission lock after
the async list call instead of before, letting concurrent first
touches redundantly list unclosed edges.
- roster.rs's agent/project segment encoding let Some("none") collide
with the missing-axis sentinel; segments are now tagged
agent-some-<v>/agent-none.
- Lazy-recovery admission reject now surfaces as a retryable
CapabilityOutcome::Failed(Transient) instead of Err(Unavailable),
which mapped to a terminal HostUnavailable.
- close_with_release consolidated a redundant get_edge read and its
two test-only crash hooks into one CloseCrashHooks struct, dropping
the too_many_arguments allow. resolver fields dropped a redundant
outer Arc around each OnceLock. Internal recovery diagnostics
downgraded from warn! to debug!.
Rejected: InMemoryAwaitEdgeWriter is not test-only (used as the
non-libsql/postgres production fallback in
ironclaw_reborn_composition::runtime). Boot sweep wiring is
out of scope for this PR by design (PR2).
Every fix ships with a test that is mutation-verified (bug reproduced
-> RED for the stated reason -> fix restored -> GREEN).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(reborn): cycle-4 review fixes
- roster_key_to_probe_scope now preserves RosterKey.user_id as the
probe scope's explicit owner (mirrors TurnScope::to_resource_scope's
forward mapping in reverse) — a bare TurnScope::new silently probed
the system mount, dropping recovery for every multi-user scope.
- design doc §4.5 updated to match roster.rs's shipped -some-/bare-none
tagged encoding (was still describing the collision-prone
agent-<id>/agent-none form); mod.rs's additive-field comment now
counts terminal_reason as the fourth deviation.
- check_scope_recovered's claim-before-list ordering now has a
discriminating concurrent regression test (gated+counting backend).
- resolver.rs's anti-tamper comments no longer claim thread_id is
anchored — it comes from metadata.parent_thread_id; safety relies on
downstream fail-closed lookups in update_parent_result_reference and
resume_turn.
- spawned lazy-recovery task now wraps its in_progress claim in an RAII
guard so a panic in recover_scope releases the claim instead of
wedging the scope shut until restart.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(reborn): note booted-set growth bound in boot recovery
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
henrypark133
added a commit
that referenced
this pull request
Jul 13, 2026
… fails (#5971) The cause was erased by map_err(|_| ...), making prod PersistenceFailed undiagnosable (seen 2026-07-10 incident, post-#5751 build). The reason is now logged at debug (enable with RUST_LOG=ironclaw_loop_support=debug); the user-facing safe summary is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
… latency, event fan-out) Adds a performance section grounded in the actual hotspots: consolidating stores onto RootFilesystem (§4.3) makes the backend latency profile the kernel's, and it can be remote (libSQL/Postgres). Ranked critical-path table (per-turn ~11-store fan-out; heartbeat vs store-lock with lease-TTL coupling; libSQL BEGIN IMMEDIATE single-writer + #5751/#6089 contention; authorize() per-tool-call reads; active- thread lock; event append/projection fan-out; recovery poll). Plus the no-lock-across-remote-I/O rule (turn_scheduler.rs:787/881), what the refactor helps vs risks (centralizing latency/writer contention onto one seam), and design guidance (batched per-transition write, isolated heartbeat, cached read-mostly authority, async coalesced events, writer sharding). Renumbered Open questions to §13. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Jul 17, 2026
…dyn, no local-specific structs (#6175) * docs(reborn): propose architecture simplification — fewer DTOs, less dyn, no local-specific structs Design note proposing a fundamental simplification of the Reborn host/runtime internals, grounded in a code audit and a cross-reference against the last ~30 days of PRs/issues. Thesis: DTO proliferation (~14 mirror structs per capability call), dyn proliferation (~6 hot-path trait objects, most single-impl), and local-specific store structs are three symptoms of one decision — treating every crate boundary as a trust boundary when Reborn has exactly one (loop <-> host). Proposes: one canonical payload type in ironclaw_host_api (Invocation/Authority/ Outcome), authority as a single fold, a closed RuntimeLane enum instead of dyn RuntimeAdapter, backend-generic stores (RowBackend) to delete the InMemory*/ Filesystem* tree, and DeploymentConfig-as-data instead of composition-mode struct families. Preserves all security invariants; incremental migration with a first-party-lane proof-of-concept slice. Complements #6168. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): show the field-level "why" behind the ~14 re-wraps Fold the mechanistic root cause into §1.1: a hop-by-hop field diff of the five request types, showing only three are genuinely distinct states and the other two are duplication forced by the crate DAG plus dead transitional fields (trust_decision is ignored by DefaultHostRuntime; idempotency_key is unimplemented). Names the four mechanisms and quantifies the ~40% that is pure duplication. Add §3.1 mapping the five request types onto the three real states (Invocation -> +Authority -> +resolved handles), showing how each mechanism is eliminated or made explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): show impl/store-level "why" for the dyn and stores sections §1.3: replace the flat dyn table with prod-vs-test-double counts and storage (Arc<dyn>), and name the three mechanisms — trait-as-test-seam (HostRuntime: 1 prod + 6 doubles; CapabilityDispatcher 1 + 2), speculative replaceability, and generic-and-dyn double indirection. Correct a mislabel: CapabilityHost is a concrete generic struct, not a trait; the dyn on that path is the dispatcher it holds. RuntimeAdapter = 5 impls (4 lanes + resolver), a closed set. §1.4: quantify the per-backend, per-domain store duplication with LOC (turns ~4,260 in-memory vs ~1,710 filesystem) and a domain table (turns/processes/ approvals/authorization/run_state), and name the two mechanisms — logic welded to storage, multiplied by the TurnRun/processes lifecycle split. §4.2: drop CapabilityHost from the "delete trait" list (already concrete) and route the test-seam need through generics/one boundary fake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): OS mechanism/policy framing — kill in-memory stores and Local* Fold in five directives: - §2.1: the operating-system lens — kernel = mechanism (small, frozen, feature- agnostic vocabulary + a few real seams); everything that varies by feature or deployment is policy resolved to data at the edge. Adding a feature must not change the kernel. - §4.3 (rewritten): the storage seam already exists — RootFilesystem, with a first-class InMemoryBackend. Delete every hand-written InMemory*Store; tests use FilesystemXStore<InMemoryBackend>. No RowBackend to invent. - §4.4 (new): local-dev is a policy config (a DeploymentConfig value), NOT an implementation. The ~66-identifier LocalDev* shadow runtime across 42 composition files collapses to one config literal selecting shared substrates. Rename the two genuine resource types (LocalFilesystem->DiskFilesystem, LocalHostProcessPort-> HostProcessPort). Enforce with a no-"Local*"-type-names boundary test. - §4.5 (new): enumerate and freeze the kernel boundary — host_api's ~124 types + the ~13 AgentLoopDriverHost ports + the mediators. Move runtime_policy (mode enums) out of the vocabulary; freeze the neutral authority survivors by test. - §5/§7/refs updated: before/after rows for in-memory stores and Local*; migration resequenced by risk (delete-in-memory and Local*->config are the low-risk first slices); new evidence pointers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): audit result — does DeploymentConfig express everything? Four-cluster audit of the ~40 LocalDev* types (policies, stores, capability wiring, trust/evidence) against "config not code". Adds §4.4.1 with the verified verdict: DeploymentConfig expresses every local-dev *selection*, but the LocalDev* family is three things, and zero-LocalDev is three moves not one — 1. Already config: the capability policy is literally a TOML file; stores are the same shared types prod uses (production_turn_state_store<F> called by both), backend-selected; LocalDevOverride trust seam is inert. 2. Mis-prefixed shared substrate (gate-evidence readers, lease-terms provider, auth read-model, capability IO): genuine code but not local — de-prefix and share, not configify. 3. Genuine local-only mechanism (capability-port decorator stack: synthetic tools, surface disclosure, mid-run refresh): behavior stays code, but config-GATED shared middleware, not a LocalDev* factory. Synthetic product-ops (project_create/skill_activate/result_read/outbound_delivery) should become first-party capabilities on the normal lane. Security note: no trust/approval bypass found — override inert, provider trust only user_trusted, gate-evidence readers fail closed. §8 Q3 answered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): add target-structure/interfaces section + shell-escape case study §5 (new) — Target structure: the minimal kernel and clean interfaces. Component table (kernel = authority/recovery; substrates = mechanism behind ports; loops and products = replaceable userland). Interface sketches: the one generic ProductSurface (open/submit_turn/events/reply/resolve_gate/cancel — feature- agnostic), the kernel authorize/dispatch, the AgentLoopHost trust membrane, the substrate ports (RootFilesystem, ProcessSandbox with scope-only SandboxMount, SecretBroker, NetworkPolicy), and DeploymentConfig-as-data. Structure diagram. §6 (new) — Case study: the shell cross-tenant escape (#6170). Verified root cause (shell is a real OS subprocess the virtual FS doesn't bound; unsafe host port is the default; HostedSingleTenant -> LocalSingleUser -> LocalHost) and how the §5 structure makes it structurally impossible (ProcessSandbox as the only path, unconstructible host port, mode-from-fact, fail-closed, two-user containment test). Renumbered subsequent sections (7 before/after, 8 invariants, 9 migration, 10 open questions); added #6170 to references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(rules): add process/shell tenant-isolation invariant; point architecture rule at the plan safety-and-sandbox.md: new "Process and shell execution: real OS isolation, per tenant" section — the standing invariant issue #6170 violated. Codifies that the virtual ScopedFilesystem does not contain a subprocess; multi-user/served deployments must route process spawns through TenantSandboxProcessPort with a scope-derived mount (never LocalHostProcessPort); deployment mode must reflect multi-user serving; fail closed (no sandbox => no shell, never host shell); and requires a two-user cross-tenant escape test for changes to process ports, planner backend rules, or the profile->mode mapping. architecture.md: add a "Direction" pointer to the simplification design doc as the owning plan for the DTO/dyn/InMemory*/LocalDev* debt the smells describe, and cross-reference type-placement.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): §5.8 — products are adapters over ProductSurface, collapse composition-split surfaces Every product owns its whole host side (protocol + transport + identity) as one adapter consuming the kernel ProductSurface; composition holds no product/transport code. Quantifies the split: WebUI across 4 places (webui_v2 + webui_ingress + static + composition/webui), and ~108K LOC of product code in the god-crate (slack 40.6K, product_auth 32.7K, runtime 14.5K, llm_admin 8.5K, automation 6.1K, webui 4.6K, outbound 1.8K). Telegram is the closest-to-clean reference shape. Invariant enforceable by an ironclaw_architecture test banning slack/webui/ telegram/openai/transport identifiers from composition. Ties to §4.4 and #6168. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): §10 — enforcement / anti-slippage ratchets pulling together all checks Consolidates the per-axis static checks into one table: process isolation (#6170 two-user escape test), mirror DTOs (check-type-duplicates.py), dyn mediators, InMemory*Store, Local* types, host_api freeze, and products-in-composition — each with its home, the addable-now ratchet (freeze current count/allowlist, fail on new), and the hard ban that is also its definition-of-done when the axis lands. Notes the guardrail self-test + two-hook-path requirement and that Local*/ InMemory*/composition bans must start as frozen allowlists (can't hard-ban today). Renumbered Open questions to §11. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): add §11 testing (state-machine invariants, idempotency from any state); distinguish crate seams from trust boundaries §11 Testing (new): specifies the behavioral suite — the state machines to pin (turn/run, capability invoke, lease, gate/resume), the invariants that must hold from ANY reachable state, the idempotency contract, and how to reach arbitrary states (model-based stateful property tests, exhaustive state×op enumeration, fault injection), cross-backend parity, fail-closed/adversarial, interface conformance harnesses, and integration-first tiering. Design only. Trust-boundary correction (per review): the doc overstated "exactly one trust boundary." Reborn has several — the untrusted loop, untrusted runtime-lane execution (WASM/script/MCP/containers/external services), and untrusted runtime-supplied data (egress/worker output) — all mediated and all preserved. What collapses is the trusted mediation-chain CRATE SEAMS, not a trust boundary. Fixed intro, §2 (retitled + enumerates the boundaries), §5.4, §8 invariants (added lane + data boundaries; fixed stale RowBackend/TurnStore refs to RootFilesystem), and §11.6 (lane/worker/egress adversarial). Renumbered Open questions to §12. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): fix last 'the one trust membrane' → the loop's (one of several) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): consistency pass + address PR review feedback End-to-end review fixes (also addressing gemini/coderabbit comments; note some reviewed a superseded head that still proposed RowBackend/TurnStore<B>): - authorize() no longer takes a separate `scope` that can diverge from Invocation.scope — derive from inv.scope (§3, §5.3). [coderabbit] - Clarify the result contract: Outcome carries success OR recoverable failure; the seam is Result<Outcome, Blocked>; no separate Err(terminate) (§3). [coderabbit] - Mark `Authority` SEALED — private fields, host-only construction via authorize() (§3, §5.3); resolve open-question 2 accordingly. [gemini + coderabbit] - Align §4.4 LOCAL_DEV process field with §5.6/§6: HostUnsandboxed(LocalOnly), gated by a local-only token a served boot can't mint. - Retire the RowBackend framing (superseded by RootFilesystem) and note deleting InMemory*Store has zero persistence-compat impact; durable backends untouched (§4.3). [gemini/coderabbit] - Add file:line evidence for the five request types + audit date/window (§1.1). [coderabbit] - Intro: "most with exactly one prod impl" -> "several" (only 2 of 5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): §12 — performance-critical paths (locking, remote-store latency, event fan-out) Adds a performance section grounded in the actual hotspots: consolidating stores onto RootFilesystem (§4.3) makes the backend latency profile the kernel's, and it can be remote (libSQL/Postgres). Ranked critical-path table (per-turn ~11-store fan-out; heartbeat vs store-lock with lease-TTL coupling; libSQL BEGIN IMMEDIATE single-writer + #5751/#6089 contention; authorize() per-tool-call reads; active- thread lock; event append/projection fan-out; recovery poll). Plus the no-lock-across-remote-I/O rule (turn_scheduler.rs:787/881), what the refactor helps vs risks (centralizing latency/writer contention onto one seam), and design guidance (batched per-transition write, isolated heartbeat, cached read-mostly authority, async coalesced events, writer sharding). Renumbered Open questions to §13. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): §13 — answer the open questions directly (now Decisions) 1. TurnRun vs ironclaw_processes: converge the mechanism (shared LeasedWorkUnit — §4.3 already collapses the store layer), keep the policy distinct (turn resume- from-checkpoint vs process terminal+re-spawn); bonus, the shared lease-recovery gives processes the reconciler they lack. 2. Authority: one sealed value (host-only construction via authorize()); narrowed read projection if an adapter needs a field. 3. DeploymentConfig: surface-disclosure derived from process:HostUnsandboxed; mid-run refresh gated by session:LongLived|PerRun; synthetic tools promoted to first-party capabilities (default hidden in hosted) — a security improvement (they gain authorize/scope-binding). Remaining items are tuning knobs, not architecture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): cross-reference the in-progress Unified Extension Runtime (BenKurrek gist) Adds §5.9 mapping this doc against the URT extension/adapter/auth design: strong, independent convergence (no-product-code-in-composition; config-not-code as recipe+engine auth; runtime-kind=closed lane set; built-ins on the identical pipeline; the trust boundaries; ProductSurface above the host pipelines). Complementary scope: URT is the deep extension/adapter/auth axis, this doc the broader kernel refactor; they compose (URT's dispatcher pipeline = authorize+ dispatch; adapter invoke/deliver = RuntimeLane execution). Adopts two URT refinements: (a) product_auth collapses to recipe data + one host AuthEngine, not per-adapter code (§5.8); (b) the Deletion/Addition/retired- taxonomy tests are the products-in-composition ratchet (§10). Clarifies WebUI consumes ProductSurface directly and is not a ChannelAdapter. Adds a reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): §5.9 — RuntimeLane reconciliation + ToolPorts↔Authority integration points Name the two seams where this doc and the Unified Extension Runtime must agree: 1. One closed execution enum RuntimeLane = {FirstParty|Wasm|Mcp|Process}; the URT's extension-declarable runtime *kinds* (first_party/wasm/mcp) are a strict subset. Process (OS-subprocess/script sandbox) is host-only — no manifest can select it; only host built-ins (shell/script) dispatch to it via ProcessSandbox. Load kind (URT) vs execution lane (this doc) are different axes; don't merge them. 2. ToolPorts is derived from Authority, never independent: dispatch() materializes egress (NetworkPolicy + host-side SecretBroker lease), state (ScopedFilesystem = Authority.mounts), logging from (&Invocation, &Authority, descriptor). ToolPorts can't be wider than Authority grants; the adapter never sees Authority itself. So URT's ToolAdapter::invoke(call, ports) IS the body of dispatch(inv, auth, lane). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): address CodeRabbit review findings on the latest head Nine substantive design-contract fixes: - §3 core model: LoopRequest (loop pre-trust, input-by-ref) resolved to Invocation at the membrane; Authorized = sealed AND invocation-bound (actor/scope/activity_id provenance) so dispatch can't be handed a mismatched (inv,auth); activity_id IS the invocation idempotency identity (idempotency_key unified with it, not deleted, satisfying §11.3); three distinct outcome channels Blocked | HostFailure | Outcome (no Ok(Failed)/Err ambiguity). Type count 3→4. §3.1/§5.3/§5.4/§11.1 aligned; Authority→Authorized throughout. - §11.2/§6: scope cross-tenant isolation to multi-user/served deployments (matrix test), not "any deployment state" (single-user local legitimately allows host proc). - §9/§10: quarantine the known-red two-user test (#[ignore]/expected-fail until the fix merges); ratchets freeze checked-in symbol allowlists (set membership), not aggregate counts (a swap evades a count). - §12: durable event append is atomic with the state transition (same tx/outbox); only subscriber fan-out is decoupled. - §5.8: adapters resolved via a product-neutral ExtensionId-keyed factory registry passed to composition as input; config lists ids, not types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
theredspoon
added a commit
to theredspoon/ironclaw
that referenced
this pull request
Jul 18, 2026
* chore(ci): dev metrics + composition mass ratchet gate (#6167)
* chore(ci): dev metrics + composition mass ratchet gate
Adds a three-tier development-metrics tool and a guardrail that stops the
ironclaw_reborn_composition crate from accreting more of the codebase.
scripts/dev_metrics.py — three tiers from git + GitHub + working tree:
- Tier 1 flow/speed: PR lead time, size distribution, merge cadence
- Tier 2 quality/stability: change-failure proxy, rework, test share
- Tier 3 codebase health: composition mass, v1 src burndown, file sprawl,
abstraction density, boundary-test coverage
Composition mass ratchet — the dependency-boundary tests police edges
*between* crates but are blind to mass piling up *inside* one crate.
ironclaw_reborn_composition is charter-bound to assembly-only wiring yet is
now ~26.7% of all production crate code. This gate is that missing guard:
- scripts/ci/composition-budget.toml — committed ceiling (enforce +
tolerance), modeled on the existing coverage-floor ratchet
- scripts/ci/check-composition-budget.sh — pure-bash gate; one-directional
(fails only on growth past the ceiling), emits a down-ratchet nudge as
carve-outs free up slack
- scripts/ci/test-check-composition-budget.sh — 22 assertions / 10 fixture
cases incl. a guard that the real tree passes the committed budget
Wiring:
- CI: new composition-budget job in code_style.yml (runs the gate + self-
tests it, registered in the aggregating code-style gate)
- Local: pre-commit-safety.sh runs the gate when composition or the gate
itself is staged; dev-setup.sh install message updated
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(ci): address review — production-only metric, script hardening, dev-metrics tests
Review feedback on #6167 (gemini, ironloopai, coderabbit):
Blocking — gate counted test-only code despite its documented "tests
excluded" contract. Exclude test-only FILES (tests.rs/test_*.rs/*_tests.rs
and /tests/ dirs) from both numerator and denominator; rebaseline the
ceiling 2670 -> 2398 bp (26.70% -> 23.98%). Inline #[cfg(test)] modules
remain a documented, symmetric residual (a line-counter can't parse them).
Added a regression case proving test files are excluded.
check-composition-budget.sh: toml_get no longer aborts under set -e +
pipefail when a key is missing (|| true) so schema validation is reached;
added a missing-key regression case.
test-check-composition-budget.sh: set -euo pipefail (repo invariant);
SIGPIPE-safe capture + fixture generation; pure-bash asserts (no pipes).
dev_metrics.py: bound `gh` with a 30s timeout and treat non-JSON output as
unavailable; fix the trait-impl density regex to count `impl<T> ... for`
generics; harden find/grep/wc probes with pipefail + rc checks (no more
false-zero metrics); UTF-8 file writes; surface the gate-aligned production
share as the ratchet metric and relabel the byte-based trend as a distinct,
coarser measurement; extract a pure classify_commit helper.
New scripts/test_dev_metrics.py — caller-level unit tests for
classification, percentiles, change-failure bucketing, rendering, and the
test-file/impl regexes; wired into the composition-budget CI job.
pre-commit hook: trigger on any staged crates/**.rs change (the metric is a
ratio) and document the working-tree/CI-authoritative limitation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(ci): harden PR classifier against transient GitHub API flakes
The classify job (#6167 CI) failed with `invalid character '<' looking
for beginning of value`: a transient API error returned an HTML page,
`gh --jq` aborted, and under `set -e` the whole labels-only job failed
and blocked the PR.
pr-labeler.sh now:
- routes every gh call through a `gh_retry` wrapper (retry + linear
backoff), and
- treats each classifier as best-effort — a step that still can't fetch
after retries only emits a `::warning::` and the script exits 0, so
labeling never gates a merge.
Two bash traps fixed along the way, both caught by the new test:
- a bare `if cmd; then …; fi` resets `$?` to 0 after `fi`, so gh_retry's
give-up looked like success — capture rc in the `else`;
- `set -e` is suppressed inside a function on the left of `||`, so the
classifiers check their own fetches explicitly instead of relying on
errexit.
Regression test: .github/scripts/test-pr-labeler.sh (retry/backoff,
give-up, and end-to-end non-fatal + happy-path via a fake `gh`), wired
into the code_style "Static-check self-tests" step and the has_code
path filter so it runs when the labeler or its test changes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(ci): add dispatch (Arc<dyn>) ratchet + dev-metrics dispatch signals
Companion to the mass ratchet for the "reduce traits & dispatch" goal
(#6168 / runtime-decomposition plan #4471).
check-composition-budget.sh now enforces TWO metrics: composition's share of
production crate code (existing) AND its Arc<dyn> dispatch count. The dispatch
count is scoped to composition production files EXCLUDING src/slack and
src/extension_host — those are owned by the separate channel/extension
refactor, so this gate must not govern or trip on their work. One-directional
like the mass ratchet: only trips on growth; nudges when slack accrues.
composition-budget.toml: arc_dyn_ceiling = 1093 (current governed count),
tolerance 15.
test harness: +6 dispatch cases (within / breach / dry-run / slack+extension
exclusion / missing-key schema error); budget() helper carries the dispatch
keys; count_arc_dyn tolerates no-match under set -e + pipefail. 36 cases pass.
dev_metrics.py: Tier-3 reports governed Arc<dyn> count and distinct dyn-trait
count (the dispatch-breadth trend), matching the ratchet scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(reborn-cli): background service install (launchd/systemd) + service restart (#6172)
* feat(reborn-cli): background service install (launchd/systemd) for ironclaw-reborn
Extracted from #6157 (service half only; TUI stays parked there). Adds
`service install/uninstall/start/stop/status`, the serve-invocation
plist/unit contract (IRONCLAW_REBORN_HOME only, no secrets), and the
`full` feature bundle with libsql as default storage.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(reborn-cli): add `service restart` verb
Composes the existing stop+start through the shared ServiceCommandRunner
dispatch. Stopped service starts cleanly; uninstalled service errors with
guidance; a failed start after a successful stop reports the service as
stopped rather than half-restarted.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(reborn-cli): pin service-PR surface invariants
Extend help_mentions_reborn_commands to assert `service` is listed
under webui-v2-beta and that no `tui` subcommand exists; add
service_help_lists_all_verbs pinning the six service verbs
(install/start/stop/status/restart/uninstall).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(reborn-cli): dedupe service restart, normalize status output
Extracts the shared restart decision tree into restart_generic (fn-pointer
seam; platforms keep only their own detection), normalizes `service status`
to one running/stopped/not-installed vocabulary on both platforms, hoists
write_atomic so launchd plist writes are crash-safe, and fixes a stale
verb-count doc.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): preserve raw systemd ActiveState as a status detail line
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): harden service module per PR review (PID parse, systemctl parsing, perms, reload, orphan status, rollback, preflight)
Addresses 9 verified findings from PR #6172 review:
1. launchd service_running misread `-` (loaded-but-stopped) PID as
running; mirrors operator_service_lifecycle's launchd_status_from_line
shape, split into service_running (has PID) vs service_loaded (any
status) since uninstall/install genuinely need the latter.
2. systemd query_unit_state now parses Key=Value lines (order-independent,
no --value) and errors on a missing required key instead of
unwrap_or_default(), which silently read as enabled=false.
3. write_atomic sets 0600 on unix before create_new, matching
operator_service_lifecycle's write_service_file.
4. launchd install now unloads/reloads a currently-loaded job after
rewriting the plist, so a reinstall actually picks up the new
ProgramArguments/EnvironmentVariables.
5. status now queries the manager unconditionally on both platforms so
an orphaned unit (file removed out-of-band, still loaded/enabled)
reports installed; two tests that pinned the old skip-when-absent
behavior were pinning the orphan-hiding bug and are updated/renamed.
6. systemd uninstall's remove_file failure now routes through the same
rollback path (restore file + reload + re-enable) as a daemon-reload
failure, via an extracted rollback_uninstall helper.
7. preflight_warnings gained a webui_token_file_is_valid check; doc
comment corrected to describe what's actually checked.
8. Documented (not built) that launchd's StandardOutPath/StandardErrorPath
logs are unrotated, in both a code comment and `service install --help`.
9. Cargo.toml `full` feature now includes root-llm-provider so
`--no-default-features --features full` stays self-contained.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(reborn-cli): adopt the canonical service identity (com.ironclaw.reborn)
The CLI service surface and the WebUI operator facade
(RebornLocalServiceLifecycle) now deliberately share one unit name and
launchd label. CLI installs atomically replace a facade-installed unit —
a security improvement, since the facade bakes the WebUI token into the
unit file while the CLI unit is secret-free. Consolidating the two
implementations is a documented follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): CI mock fidelity, token-file hygiene, review fixes
1. mod.rs: add the "systemctl show unit state" arm to the shared
SuccessfulServiceCommandRunner mock (install_with_runner now queries
unit state pre-write). Production code and the strict parser are
correct; only the mock modeled reality incompletely.
2. webui_token.rs: propagate real I/O errors instead of treating them
as "absent" (was silently overwriting unreadable tokens); reject
symlinked/oversized token files; repair (not reject) a wrongly
permissioned but valid token on accept; serve.rs no longer collapses
VarError::NotUnicode into "unset".
3. launchd.rs/mod.rs: suppress the "keeps the OLD definition" advisory
when install already reloaded a loaded job in place (the definition
is live immediately in that case); systemd's advisory is unaffected.
4. mod.rs: preflight-warning coverage now drives service install
(runner-injectable, warnings returned) instead of only unit-testing
the preflight_warnings helper directly.
5. systemd.rs: uninstall's remove_file step is now injectable so its
rollback test forces a deterministic failure, replacing the
chmod-0o555 approach that silently no-ops under a root test runner.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): preserve error source chain in service rollback failures
combined_failure flattened the primary error and rollback outcomes into
one anyhow!() string, losing the source chain. Use .context() so the
primary stays inspectable via source()/{:#} beneath the rollback text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): single-handle token read closes TOCTOU window
read_token_file_checked previously stat'd then read the token file as
two separate syscalls, letting a symlink/FIFO/oversized file be swapped
in between them; a FIFO also passed the length check and could block
serve startup indefinitely. Now opens once with O_NOFOLLOW|O_NONBLOCK,
checks type/size via fstat on that handle, and bounds the read to
MAX_BYTES+1 from the same fd. Non-unix keeps the prior best-effort path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): uninstall disable rollback, hermetic verb dispatch, dry-run coverage, docs
Four verified findings from PR #6172 review round: (1) systemd uninstall's
disable failure now rolls back like every sibling failure branch instead of
propagating with a bare `?`; (2) a smoke test pins that a directory at the
webui-token path fails `onboard --dry-run` non-zero without mutating home;
(3) start/stop/restart/status/uninstall get the same runner-injectable split
`install` already had (`ServicePlatform::*_with_runner`), with one
consolidated clap-dispatch test instead of duplicating per-verb coverage;
(4) FEATURE_PARITY.md and CHANGELOG.md reflect the shipped service-install
feature.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): single status line per service restart
restart_generic's stop/start fn pointers called the loud
start_with_runner/stop_with_runner, which each print their own
"Service started"/"Service stopped" line in addition to
restart_generic's own summary line, so `service restart` printed two
lines. Add quiet variants (start_with_runner_quiet/
stop_with_runner_quiet) that skip the println, used only by
restart_with_runner; the public start/stop commands keep printing as
before.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): stop/restart honor manager-loaded state like status/uninstall
launchd `stop` gated on service_running alone, leaving a loaded-but-not-
running KeepAlive job (`-` PID) registered for respawn; `restart` derived
`was_running` the same way, so it bare-loaded an already-loaded label
(which launchd errors on) instead of reloading. systemd `stop` gated only
on unit-file existence, silently no-opping on a unit removed out-of-band
while still loaded/enabled. All three now check manager state the way
status/uninstall already do.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(reborn-cli): honor XDG_CONFIG_HOME for systemd units, guard launchd start on loaded labels
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(reborn-cli): make service tests hermetic over XDG_CONFIG_HOME
commit 5b3f39eb9 made config_home() honor $XDG_CONFIG_HOME, which
unit_path() now reads. Service tests that fake $HOME into a tempdir
never cleared XDG_CONFIG_HOME, so on hosts where it's set (CI runners
observed setting it to $HOME/.config), unit_path() resolved to the
real path instead of the tempdir — causing
systemd::tests::restart_not_installed_errors_with_install_guidance
and
tests::install_then_uninstall_linux_writes_and_removes_unit_file to
fail. Production config_home() behavior is unchanged and correct.
Extends the TempHomeGuard helpers in mod.rs and systemd.rs (new,
mirroring mod.rs's) to also clear/restore XDG_CONFIG_HOME, and
switches all HOME-faking tests in systemd.rs onto the guard instead of
manual set/restore blocks.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ci): stop safety ReDoS timing guards flaking under coverage [skip-regression-check] (#6181)
The `*_100kb_near_miss` adversarial tests in ironclaw_safety are ReDoS
guards: they feed a ~100 KB near-miss payload to a regex scan and assert
it finishes fast enough to rule out catastrophic backtracking (which
would take seconds or hang). They used a hard 100 ms bound.
Under `cargo llvm-cov` instrumentation on shared CI runners the linear
scan is ~100x slower, so the Coverage (default) job flaked with
"anthropic_api_key pattern took 101ms on 100KB near-miss" — 1 ms over
the threshold. Only the instrumented coverage job is affected.
Replace the per-test hard thresholds (100 ms in leak_detector/validator/
sanitizer, 500 ms already in policy) with one documented shared constant
REDOS_SCAN_BUDGET_MS = 2000 in the crate root. 2 s keeps a large margin
below any real ReDoS while tolerating instrumentation overhead, and the
guards still fail loudly on genuine catastrophic backtracking.
Test-only change; no production behavior touched — hence the
regression-check skip.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): black-box smoke for ironclaw-reborn serve — restart + kill-9 durability (#5523)
The in-process Reborn integration harness cannot prove real process
startup, real HTTP end-to-end, or process-death durability — its
new_at_path() reopen approximates a restart but never actually kills a
process. Add a thin, permanent black-box smoke suite that boots the
real ironclaw-reborn binary and drives it purely over HTTP:
- boot -> /api/health -> scripted chat round-trip
- tool-call turn executes and finalizes a reply
- graceful restart (SIGINT) preserves thread history
- kill -9 durability: on-disk libsql state survives an unclean death,
server comes back healthy, no leaked child processes
- bearer-auth boundary (401 without token, 200 with)
Fixture design: reuses the existing reborn_v2_restartable_server
fixture (already restart-capable against a persistent home dir) rather
than porting the legacy ManagedIronclawServer class. Extends its
stop() closure with a `hard: bool = False` flag for SIGKILL, so the
fixture's tuple shape and existing consumer are untouched. Promotes
the capability-preview polling helpers out of
test_reborn_webui_v2_legacy_tool_execution.py into the shared harness
(now used by both files) instead of adding a second copy for the new
suite.
Mutation-verified the durability scenario: temporarily pointed
restart() at a fresh home dir per call, confirmed the kill-9 test goes
red for the right reason (persisted thread missing after "restart"),
then reverted to a clean diff.
Wires a `blackbox-smoke` CI job into the existing reborn-e2e.yml
job-per-file pattern (mirrors webui-v2-smoke's build step, no
Playwright/Node needed since this suite is HTTP-only).
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix(webui-v2): improve toast lifecycle and accessibility (#6151)
* fix(webui): improve toast lifecycle and accessibility
* test(e2e): cover toast lifecycle and stacking
* fix(webui): type toast presentation mappings
* test(e2e): fast-forward toast hover timing
* test(e2e): align toast clock requirements
* fix(webui-v2): add theme selection controls to Appearance settings (#6148)
* fix(webui): add theme controls to appearance settings
* test(e2e): cover appearance theme persistence
* fix(webui): address appearance accessibility review
* fix(webui): type appearance theme controls
* fix(webui): use native theme radios
* feat(reborn): serve webui at root path instead of `v2` (#6152)
* feat(reborn): serve the WebUI from root paths
* test(e2e): cover root-mounted Reborn WebUI
* fix(webui): reject noncanonical SPA paths
* fix(webui): address root-mount review feedback
* refactor(webui): derive static router config errors
* fix(webui-v2): surface workspace download failures (#6150)
* fix(webui-v2): surface workspace download failures
* test(e2e): cover workspace download failure feedback
* test(e2e): centralize workspace download selectors
* test(e2e): navigate workspace downloads through UI
* docs(reborn): propose architecture simplification — fewer DTOs, less dyn, no local-specific structs (#6175)
* docs(reborn): propose architecture simplification — fewer DTOs, less dyn, no local-specific structs
Design note proposing a fundamental simplification of the Reborn host/runtime
internals, grounded in a code audit and a cross-reference against the last ~30
days of PRs/issues.
Thesis: DTO proliferation (~14 mirror structs per capability call), dyn
proliferation (~6 hot-path trait objects, most single-impl), and local-specific
store structs are three symptoms of one decision — treating every crate boundary
as a trust boundary when Reborn has exactly one (loop <-> host).
Proposes: one canonical payload type in ironclaw_host_api (Invocation/Authority/
Outcome), authority as a single fold, a closed RuntimeLane enum instead of dyn
RuntimeAdapter, backend-generic stores (RowBackend) to delete the InMemory*/
Filesystem* tree, and DeploymentConfig-as-data instead of composition-mode
struct families. Preserves all security invariants; incremental migration with
a first-party-lane proof-of-concept slice. Complements #6168.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): show the field-level "why" behind the ~14 re-wraps
Fold the mechanistic root cause into §1.1: a hop-by-hop field diff of the five
request types, showing only three are genuinely distinct states and the other
two are duplication forced by the crate DAG plus dead transitional fields
(trust_decision is ignored by DefaultHostRuntime; idempotency_key is
unimplemented). Names the four mechanisms and quantifies the ~40% that is pure
duplication.
Add §3.1 mapping the five request types onto the three real states
(Invocation -> +Authority -> +resolved handles), showing how each mechanism is
eliminated or made explicit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): show impl/store-level "why" for the dyn and stores sections
§1.3: replace the flat dyn table with prod-vs-test-double counts and storage
(Arc<dyn>), and name the three mechanisms — trait-as-test-seam (HostRuntime: 1
prod + 6 doubles; CapabilityDispatcher 1 + 2), speculative replaceability, and
generic-and-dyn double indirection. Correct a mislabel: CapabilityHost is a
concrete generic struct, not a trait; the dyn on that path is the dispatcher it
holds. RuntimeAdapter = 5 impls (4 lanes + resolver), a closed set.
§1.4: quantify the per-backend, per-domain store duplication with LOC (turns
~4,260 in-memory vs ~1,710 filesystem) and a domain table (turns/processes/
approvals/authorization/run_state), and name the two mechanisms — logic welded
to storage, multiplied by the TurnRun/processes lifecycle split.
§4.2: drop CapabilityHost from the "delete trait" list (already concrete) and
route the test-seam need through generics/one boundary fake.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): OS mechanism/policy framing — kill in-memory stores and Local*
Fold in five directives:
- §2.1: the operating-system lens — kernel = mechanism (small, frozen, feature-
agnostic vocabulary + a few real seams); everything that varies by feature or
deployment is policy resolved to data at the edge. Adding a feature must not
change the kernel.
- §4.3 (rewritten): the storage seam already exists — RootFilesystem, with a
first-class InMemoryBackend. Delete every hand-written InMemory*Store; tests use
FilesystemXStore<InMemoryBackend>. No RowBackend to invent.
- §4.4 (new): local-dev is a policy config (a DeploymentConfig value), NOT an
implementation. The ~66-identifier LocalDev* shadow runtime across 42 composition
files collapses to one config literal selecting shared substrates. Rename the two
genuine resource types (LocalFilesystem->DiskFilesystem, LocalHostProcessPort->
HostProcessPort). Enforce with a no-"Local*"-type-names boundary test.
- §4.5 (new): enumerate and freeze the kernel boundary — host_api's ~124 types +
the ~13 AgentLoopDriverHost ports + the mediators. Move runtime_policy (mode
enums) out of the vocabulary; freeze the neutral authority survivors by test.
- §5/§7/refs updated: before/after rows for in-memory stores and Local*; migration
resequenced by risk (delete-in-memory and Local*->config are the low-risk first
slices); new evidence pointers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): audit result — does DeploymentConfig express everything?
Four-cluster audit of the ~40 LocalDev* types (policies, stores, capability
wiring, trust/evidence) against "config not code". Adds §4.4.1 with the verified
verdict: DeploymentConfig expresses every local-dev *selection*, but the LocalDev*
family is three things, and zero-LocalDev is three moves not one —
1. Already config: the capability policy is literally a TOML file; stores are the
same shared types prod uses (production_turn_state_store<F> called by both),
backend-selected; LocalDevOverride trust seam is inert.
2. Mis-prefixed shared substrate (gate-evidence readers, lease-terms provider,
auth read-model, capability IO): genuine code but not local — de-prefix and
share, not configify.
3. Genuine local-only mechanism (capability-port decorator stack: synthetic tools,
surface disclosure, mid-run refresh): behavior stays code, but config-GATED
shared middleware, not a LocalDev* factory. Synthetic product-ops
(project_create/skill_activate/result_read/outbound_delivery) should become
first-party capabilities on the normal lane.
Security note: no trust/approval bypass found — override inert, provider trust
only user_trusted, gate-evidence readers fail closed. §8 Q3 answered.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): add target-structure/interfaces section + shell-escape case study
§5 (new) — Target structure: the minimal kernel and clean interfaces. Component
table (kernel = authority/recovery; substrates = mechanism behind ports; loops
and products = replaceable userland). Interface sketches: the one generic
ProductSurface (open/submit_turn/events/reply/resolve_gate/cancel — feature-
agnostic), the kernel authorize/dispatch, the AgentLoopHost trust membrane, the
substrate ports (RootFilesystem, ProcessSandbox with scope-only SandboxMount,
SecretBroker, NetworkPolicy), and DeploymentConfig-as-data. Structure diagram.
§6 (new) — Case study: the shell cross-tenant escape (#6170). Verified root cause
(shell is a real OS subprocess the virtual FS doesn't bound; unsafe host port is
the default; HostedSingleTenant -> LocalSingleUser -> LocalHost) and how the §5
structure makes it structurally impossible (ProcessSandbox as the only path,
unconstructible host port, mode-from-fact, fail-closed, two-user containment test).
Renumbered subsequent sections (7 before/after, 8 invariants, 9 migration,
10 open questions); added #6170 to references.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(rules): add process/shell tenant-isolation invariant; point architecture rule at the plan
safety-and-sandbox.md: new "Process and shell execution: real OS isolation, per
tenant" section — the standing invariant issue #6170 violated. Codifies that the
virtual ScopedFilesystem does not contain a subprocess; multi-user/served
deployments must route process spawns through TenantSandboxProcessPort with a
scope-derived mount (never LocalHostProcessPort); deployment mode must reflect
multi-user serving; fail closed (no sandbox => no shell, never host shell); and
requires a two-user cross-tenant escape test for changes to process ports,
planner backend rules, or the profile->mode mapping.
architecture.md: add a "Direction" pointer to the simplification design doc as
the owning plan for the DTO/dyn/InMemory*/LocalDev* debt the smells describe, and
cross-reference type-placement.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): §5.8 — products are adapters over ProductSurface, collapse composition-split surfaces
Every product owns its whole host side (protocol + transport + identity) as one
adapter consuming the kernel ProductSurface; composition holds no product/transport
code. Quantifies the split: WebUI across 4 places (webui_v2 + webui_ingress +
static + composition/webui), and ~108K LOC of product code in the god-crate
(slack 40.6K, product_auth 32.7K, runtime 14.5K, llm_admin 8.5K, automation 6.1K,
webui 4.6K, outbound 1.8K). Telegram is the closest-to-clean reference shape.
Invariant enforceable by an ironclaw_architecture test banning slack/webui/
telegram/openai/transport identifiers from composition. Ties to §4.4 and #6168.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): §10 — enforcement / anti-slippage ratchets pulling together all checks
Consolidates the per-axis static checks into one table: process isolation (#6170
two-user escape test), mirror DTOs (check-type-duplicates.py), dyn mediators,
InMemory*Store, Local* types, host_api freeze, and products-in-composition — each
with its home, the addable-now ratchet (freeze current count/allowlist, fail on
new), and the hard ban that is also its definition-of-done when the axis lands.
Notes the guardrail self-test + two-hook-path requirement and that Local*/
InMemory*/composition bans must start as frozen allowlists (can't hard-ban today).
Renumbered Open questions to §11.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): add §11 testing (state-machine invariants, idempotency from any state); distinguish crate seams from trust boundaries
§11 Testing (new): specifies the behavioral suite — the state machines to pin
(turn/run, capability invoke, lease, gate/resume), the invariants that must hold
from ANY reachable state, the idempotency contract, and how to reach arbitrary
states (model-based stateful property tests, exhaustive state×op enumeration,
fault injection), cross-backend parity, fail-closed/adversarial, interface
conformance harnesses, and integration-first tiering. Design only.
Trust-boundary correction (per review): the doc overstated "exactly one trust
boundary." Reborn has several — the untrusted loop, untrusted runtime-lane
execution (WASM/script/MCP/containers/external services), and untrusted
runtime-supplied data (egress/worker output) — all mediated and all preserved.
What collapses is the trusted mediation-chain CRATE SEAMS, not a trust boundary.
Fixed intro, §2 (retitled + enumerates the boundaries), §5.4, §8 invariants
(added lane + data boundaries; fixed stale RowBackend/TurnStore refs to
RootFilesystem), and §11.6 (lane/worker/egress adversarial). Renumbered Open
questions to §12.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): fix last 'the one trust membrane' → the loop's (one of several)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): consistency pass + address PR review feedback
End-to-end review fixes (also addressing gemini/coderabbit comments; note some
reviewed a superseded head that still proposed RowBackend/TurnStore<B>):
- authorize() no longer takes a separate `scope` that can diverge from
Invocation.scope — derive from inv.scope (§3, §5.3). [coderabbit]
- Clarify the result contract: Outcome carries success OR recoverable failure;
the seam is Result<Outcome, Blocked>; no separate Err(terminate) (§3). [coderabbit]
- Mark `Authority` SEALED — private fields, host-only construction via authorize()
(§3, §5.3); resolve open-question 2 accordingly. [gemini + coderabbit]
- Align §4.4 LOCAL_DEV process field with §5.6/§6: HostUnsandboxed(LocalOnly),
gated by a local-only token a served boot can't mint.
- Retire the RowBackend framing (superseded by RootFilesystem) and note deleting
InMemory*Store has zero persistence-compat impact; durable backends untouched
(§4.3). [gemini/coderabbit]
- Add file:line evidence for the five request types + audit date/window (§1.1).
[coderabbit]
- Intro: "most with exactly one prod impl" -> "several" (only 2 of 5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): §12 — performance-critical paths (locking, remote-store latency, event fan-out)
Adds a performance section grounded in the actual hotspots: consolidating stores
onto RootFilesystem (§4.3) makes the backend latency profile the kernel's, and it
can be remote (libSQL/Postgres). Ranked critical-path table (per-turn ~11-store
fan-out; heartbeat vs store-lock with lease-TTL coupling; libSQL BEGIN IMMEDIATE
single-writer + #5751/#6089 contention; authorize() per-tool-call reads; active-
thread lock; event append/projection fan-out; recovery poll). Plus the
no-lock-across-remote-I/O rule (turn_scheduler.rs:787/881), what the refactor
helps vs risks (centralizing latency/writer contention onto one seam), and design
guidance (batched per-transition write, isolated heartbeat, cached read-mostly
authority, async coalesced events, writer sharding). Renumbered Open questions
to §13.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): §13 — answer the open questions directly (now Decisions)
1. TurnRun vs ironclaw_processes: converge the mechanism (shared LeasedWorkUnit —
§4.3 already collapses the store layer), keep the policy distinct (turn resume-
from-checkpoint vs process terminal+re-spawn); bonus, the shared lease-recovery
gives processes the reconciler they lack.
2. Authority: one sealed value (host-only construction via authorize()); narrowed
read projection if an adapter needs a field.
3. DeploymentConfig: surface-disclosure derived from process:HostUnsandboxed;
mid-run refresh gated by session:LongLived|PerRun; synthetic tools promoted to
first-party capabilities (default hidden in hosted) — a security improvement
(they gain authorize/scope-binding). Remaining items are tuning knobs, not
architecture.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): cross-reference the in-progress Unified Extension Runtime (BenKurrek gist)
Adds §5.9 mapping this doc against the URT extension/adapter/auth design: strong,
independent convergence (no-product-code-in-composition; config-not-code as
recipe+engine auth; runtime-kind=closed lane set; built-ins on the identical
pipeline; the trust boundaries; ProductSurface above the host pipelines).
Complementary scope: URT is the deep extension/adapter/auth axis, this doc the
broader kernel refactor; they compose (URT's dispatcher pipeline = authorize+
dispatch; adapter invoke/deliver = RuntimeLane execution).
Adopts two URT refinements: (a) product_auth collapses to recipe data + one host
AuthEngine, not per-adapter code (§5.8); (b) the Deletion/Addition/retired-
taxonomy tests are the products-in-composition ratchet (§10). Clarifies WebUI
consumes ProductSurface directly and is not a ChannelAdapter. Adds a reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): §5.9 — RuntimeLane reconciliation + ToolPorts↔Authority integration points
Name the two seams where this doc and the Unified Extension Runtime must agree:
1. One closed execution enum RuntimeLane = {FirstParty|Wasm|Mcp|Process}; the URT's
extension-declarable runtime *kinds* (first_party/wasm/mcp) are a strict subset.
Process (OS-subprocess/script sandbox) is host-only — no manifest can select it;
only host built-ins (shell/script) dispatch to it via ProcessSandbox. Load kind
(URT) vs execution lane (this doc) are different axes; don't merge them.
2. ToolPorts is derived from Authority, never independent: dispatch() materializes
egress (NetworkPolicy + host-side SecretBroker lease), state (ScopedFilesystem =
Authority.mounts), logging from (&Invocation, &Authority, descriptor). ToolPorts
can't be wider than Authority grants; the adapter never sees Authority itself. So
URT's ToolAdapter::invoke(call, ports) IS the body of dispatch(inv, auth, lane).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(reborn): address CodeRabbit review findings on the latest head
Nine substantive design-contract fixes:
- §3 core model: LoopRequest (loop pre-trust, input-by-ref) resolved to Invocation
at the membrane; Authorized = sealed AND invocation-bound (actor/scope/activity_id
provenance) so dispatch can't be handed a mismatched (inv,auth); activity_id IS
the invocation idempotency identity (idempotency_key unified with it, not deleted,
satisfying §11.3); three distinct outcome channels Blocked | HostFailure | Outcome
(no Ok(Failed)/Err ambiguity). Type count 3→4. §3.1/§5.3/§5.4/§11.1 aligned;
Authority→Authorized throughout.
- §11.2/§6: scope cross-tenant isolation to multi-user/served deployments (matrix
test), not "any deployment state" (single-user local legitimately allows host proc).
- §9/§10: quarantine the known-red two-user test (#[ignore]/expected-fail until the
fix merges); ratchets freeze checked-in symbol allowlists (set membership), not
aggregate counts (a swap evades a count).
- §12: durable event append is atomic with the state transition (same tx/outbox);
only subscriber fan-out is decoupled.
- §5.8: adapters resolved via a product-neutral ExtensionId-keyed factory registry
passed to composition as input; config lists ids, not types.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): approval stores over RootFilesystem, delete InMemory*Store (§4.3) (#6195)
* refactor(reborn): approval stores over RootFilesystem, delete InMemory*Store (§4.3)
First slice of the architecture-simplification note
(docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md §4.3):
"in-memory" stops being a bespoke store and becomes a filesystem backend, so
each approval domain has one production Filesystem*Store<F> exercised over the
in-memory backend in tests and libSQL/Postgres in production — no parallel
hand-written implementation to keep in lock-step.
Deletes the three hand-written approval stores in ironclaw_approvals
(InMemoryAutoApproveSettingStore, InMemoryPersistentApprovalPolicyStore,
InMemoryCapabilityPermissionOverrideStore, plus the InMemoryToolPermissionOverrideStore
alias). Everything now runs the existing Filesystem*Store<F>:
- ironclaw_approvals: adds a `test-support`-gated helper module with
in_memory_backed_* constructors (the production store over a fresh
InMemoryBackend mounted at /approvals). The stores' own unit tests move onto
Filesystem*Store<InMemoryBackend>, proving it covers the deleted stores' cases.
- composition factory.rs: the LocalDev* approval-store aliases collapse to one
unconditional Filesystem*Store<LocalDevRootFilesystem>; the no-durable-features
local-dev builder wires them over the composite root filesystem (in-memory
backed) via the existing scoped-filesystem path instead of the deleted
InMemory* stores. wrap_scoped / invocation_mount_view and the /approvals mount
machinery are un-gated so both builders share one path.
- host_runtime production-wiring guard: the fail-closed LocalOnly classification
now keys on FilesystemPersistentApprovalPolicyStore<InMemoryBackend> instead of
the deleted InMemory type. Production (<LibSql>/<Postgres>) and durable-local-dev
(<Composite>) classifications are unchanged; the guard contract test is
repointed and still asserts LocalOnly.
- downstream test suites (host_runtime, composition, product_workflow) repoint to
the test-support helpers; the affected crates enable ironclaw_approvals/test-support
in [dev-dependencies].
Net subtractive (−136 LOC). No trust boundary or persistence-compatibility change:
the in-memory approval stores were volatile/local; the durable libSQL/Postgres
backends are untouched. Boundary tests (ironclaw_architecture) stay green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): address review — migrate root harness, honest volatile approval-store type
Two review findings on the approvals-store consolidation:
1. Root integration harness left uncompilable. `tests/integration/support/
harness/mod.rs` still constructed the deleted `InMemory*Store`s (as
`Arc<dyn …>` defaults). Repoint to the `in_memory_backed_*` helpers and enable
`ironclaw_approvals/test-support` in the root `[dev-dependencies]`.
2. Guard weakened for the no-durable composition. The composite-unified alias made
the no-durable-features build wire
`FilesystemPersistentApprovalPolicyStore<CompositeRootFilesystem>`, whose
TypeId misses the guard's `<InMemoryBackend>` branch, so the volatile store was
classified `ProductionCandidate`. Fix by making the store type honestly reflect
its volatility: the no-durable build now backs the three approval stores with a
dedicated `InMemoryBackend` directly (via `wrap_scoped`), so the concrete type
is `Filesystem*Store<InMemoryBackend>` — which the production-wiring guard
classifies `LocalOnly`, exactly as the sibling `InMemoryRunStateStore` /
`InMemoryCapabilityLeaseStore` are. Durable builds keep the composite-backed
type (distinct, correctly a production candidate). The `LocalDev*` approval
aliases go back to cfg-split (InMemoryBackend vs composite); the guard contract
test now documents that it exercises the exact type the no-durable composition
wires. `local_dev_scoped_filesystem` is re-gated to durable-only (the no-durable
builder no longer uses it); `wrap_scoped`/`invocation_mount_view` stay ungated
since the no-durable builder now calls `wrap_scoped` directly.
Verified: composition compiles + clippy clean on default (no-durable) and libSQL;
guard contract test green; local_dev_authorization tests green; root
reborn_integration_* targets compile.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): consolidate WebUI host stack into a single ironclaw_webui crate (+ Slack/OpenAI-compat wiring) (#6194)
* refactor(reborn): merge WebUI host stack into ironclaw_reborn_webui_ingress
Fold `ironclaw_webui_v2` (route surface + SPA bundle) and composition's WebUI
middleware/assembly into `ironclaw_reborn_webui_ingress` so the whole WebUI host
stack is one crate above composition, and composition shrinks.
Move-only for behavior; the composed `webui_v2_app` Router, middleware order,
descriptors, and security invariants are unchanged (locked by the moved contract
tests + the composition/ingress router tests, all green under default features).
Structure:
- `ironclaw_webui_v2/src/*` -> ingress `src/webui_v2/` (public module,
unconditional); `build.rs` + `frontend/` moved to ingress; crate deleted and
removed from workspace members (68 -> 67).
- Composition WebUI middleware (`webui_body_limit`, `webui_operator_auth`,
`webui_rate_limit`, `webui_route_match`, `webui_ws_origin`) + `webui_serve.rs`
-> ingress `src/`.
- `webui_serve.rs` split: `WebuiServeConfig`/`webui_v2_app`/`WebuiV2App`/
`Webui{Serve,Config}Error`/`WebuiAuthenticator`/`WebuiAuthentication` move to
ingress; the mount vocabulary (`PublicRouteMount`/`ProtectedRouteMount`/
`PublicRouteDrain(s)`) stays in composition (`webui/route_mounts.rs`) because
nearai/openai/runtime construct it — moving it up would cycle.
- Product-auth decoupled: `ProductAuthRouteState`, `product_auth_route_mount`,
`ProductAuthRouteMount` exposed `pub` + re-exported from composition root;
ingress imports them (+ `RebornWebuiBundle`, `GoogleOAuthRouteConfig`) via the
composition facade. Composition no longer depends on `ironclaw_webui_v2`.
- Callers repointed: composition tests, ingress tests, reborn_cli
(serve/webui_auth), root v1 int-tier tests + dev-dep, Dockerfile.reborn +
smoke test frontend path, and the ironclaw_architecture boundary spec.
Deferred (out of scope, feature-gated off by default): the
`slack-v2-host-beta` / `openai-compat-beta` blocks in `webui_serve.rs` still
reference composition-internal surfaces and compile out under default features
(declared as known cfgs). Wiring the Slack/OpenAI-compat host surface through
ingress is a follow-up; composition's slack feature will not build until then.
[skip-regression-check] move-only refactor; behavior covered by relocated
contract tests and existing composition/ingress router suites.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): make Slack + OpenAI-compat host-beta build and wire after WebUI merge
The WebUI host-stack merge (parent commit) hoisted `webui_v2_app` + its config,
authenticator, and middleware surface from `ironclaw_reborn_composition` up into
`ironclaw_reborn_webui_ingress`, but left the `slack-v2-host-beta` /
`openai-compat-beta` blocks in the moved `webui_serve.rs` pointing at
`crate::slack::*` / composition internals that don't exist in ingress. Those
features were declared only as known-cfgs and compiled out, so:
- composition failed to build under `slack-v2-host-beta` (two mount-vocabulary
imports still on the old `webui::webui_serve` path);
- the ingress serve blocks were permanently dead, so the CLI's slack/openai
features forwarded to composition but never mounted the Slack routes —
a functional parity break, not just a compile break;
- composition's slack-gated tests still imported the moved `webui_v2_app`.
Wiring (behavior-preserving; restores pre-merge parity):
- ingress now defines real `slack-v2-host-beta` / `openai-compat-beta` features
that forward to composition (+ optional `ironclaw_reborn_openai_compat`); the
moved `webui_serve.rs` reaches Slack setup/route types and the
OpenAI-compat bearer-evidence helper through composition's public facade
(`ironclaw_reborn_composition::{SlackPersonalSetupServiceSlot,
SlackChannelRouteAdminRouteConfig, slack_channel_route_admin_route_mount,
SlackPersonalOAuthBindingConfig, mark_bearer_token_verified_for_tenant}`).
Ingress does NOT depend on `ironclaw_product_adapters` directly — the
architecture boundary (`reborn_dependency_boundaries.rs`) forbids it, so the
evidence helper is re-exported from composition instead.
- composition promotes `slack_channel_route_admin_route_mount` + its
`SlackChannelRouteAdminRouteMount` return type to `pub` (its sole caller,
`webui_v2_app`, moved up), mirroring the already-public `ProtectedRouteMount`.
- CLI forwards `slack-v2-host-beta` / `openai-compat-beta` to the ingress crate
as well as composition, so the serve blocks compile in and the routes mount.
Tests:
- The 7 composition slack unit tests that drove the now-relocated `webui_v2_app`
move to `ironclaw_reborn_webui_ingress/tests/slack_host_beta_webui_v2.rs`.
They use only composition's public builders, so ingress (which normal-deps
composition — single crate copy, no dev-dep cycle) is their correct home; a
composition lib-test cannot call the ingress `webui_v2_app` without cargo
building two incompatible copies of composition. composition's ingress
dev-dep gains `slack-v2-host-beta` so its own `webui_v2_product_auth*` tests'
`with_slack_*` blocks compile.
Verified (clean env): composition/ingress/cli build + clippy `-D warnings` under
both beta features; ingress `--all-features` suite green incl. the 7 relocated
tests; composition lib (1589) + router (webui_v2_serve 44 / product_auth 51) +
cli (440 incl. Dockerfile smoke) green; `ironclaw_architecture` boundaries hold.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(reborn): rename crate ironclaw_reborn_webui_ingress -> ironclaw_webui + doc pass
Now that the crate owns the whole WebUI host stack (route surface + SPA +
gateway assembly/middleware + serve loop + host auth), "reborn_webui_ingress"
undersells it. Rename the crate to `ironclaw_webui` and refresh its docs to
describe the composed subsystems.
Rename (pure identifier swap, no behavior change):
- `git mv crates/ironclaw_reborn_webui_ingress crates/ironclaw_webui`; package
`name` + workspace members + root dep alias updated.
- Every `ironclaw_reborn_webui_ingress` reference repointed across Rust, Cargo
manifests, Cargo.lock, Dockerfile.reborn, CI scripts (.sh/.py), the
`ironclaw_architecture` boundary spec (crate_name / forbidden lists / layer
exception / source-path prefixes), root + crate CLAUDE/AGENTS docs, .claude
rules & skills, and the security-parity docs. `openwiki/` (auto-generated) and
`docs/plans/` (historical) intentionally left for their own regen/record.
Docs (README.md new; AGENTS.md + CLAUDE.md restructured):
- README.md: human-facing overview with the three-piece fold-in map
(route surface + SPA from the former `ironclaw_webui_v2`; gateway assembly +
middleware from `ironclaw_reborn_composition::webui`; serve loop + host auth
from this crate's original scope), layering/boundaries, feature flags, build/test.
- AGENTS.md: replaced the stale "deliberately small" framing with an accurate
agent map — composed subsystems, do-not-move-in, allowed deps, how to add a
route / authenticator / OAuth provider.
- CLAUDE.md: reframed opening (it no longer is a "counterpart to webui_v2_app" —
that fn lives here now); Surface table gains the route/gateway symbols
(`webui_v2_router`, `webui_v2_routes`, `WebUiV2State`, `WebUiV2HttpError`,
`webui_v2_app`, `WebuiServeConfig`); folded in the WebChat v2 route table +
streaming/SSE model + SPA build detail; test layout now lists the
route-surface/gateway suites. OAuth login security contract retained verbatim.
Verified: `cargo metadata` resolves; `cargo build -p ironclaw_webui` and
`-p ironclaw_reborn_cli --features slack-v2-host-beta,openai-compat-beta` green;
`cargo test -p ironclaw_architecture reborn` (boundaries, new name) green;
`cargo test -p ironclaw_webui --features slack-v2-host-beta --test
slack_host_beta_webui_v2` green; 0 stale `ironclaw_reborn_webui_ingress` refs
outside openwiki/docs-plans.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(reborn): address PR review findings + stale ironclaw_webui_v2 refs post-merge
Review findings on PR #6194 (gemini-code-assist + ironloopai) and the leftover
references the `ironclaw_webui_v2` → `ironclaw_webui` fold-in left behind.
CI / build path migration (ironloopai "path migration incomplete"):
- Repointed the deleted `crates/ironclaw_webui_v2/frontend` build path to
`crates/ironclaw_webui/frontend` across all workflows (code_style, coverage,
ironclaw-stress, platform-and-compat, reborn-e2e, reborn-playwright),
`.dockerignore`, `scripts/run-reborn-webui.sh`, `scripts/ci/quality_gate_strict.sh`,
and the `regression-test-check.yml` frontend-test detector.
- Test bucketing: dropped the dead `ironclaw_webui_v2` entries from
`reborn-crate-test-buckets.sh` + `package-feature-flags.sh` (the renamed
`ironclaw_webui` entries already exist), repointed `classify-test-scope.sh`,
and widened the `reborn-tests.yml` jq filter to `startswith("ironclaw_webui")`
so the folded crate's tests still land in the webui bucket.
- QA inventory (`scripts/reborn_qa_matrix/audit_surface_inventory.py`) now reads
`crates/ironclaw_webui/src/webui_v2/descriptors.rs`.
- Regenerated `harness/latency/runner/Cargo.lock` (transitively referenced the
deleted crate via composition's `webui-v2-beta`).
Broken doc links / stale comments (gemini):
- `nearai_login_serve.rs` + `runtime.rs`: the broken intra-doc link
`crate::webui::route_mounts::WebuiServeConfig` (type moved out of composition)
is now a plain code span `ironclaw_webui::WebuiServeConfig::with_public_route_mount`
— composition cannot link into `ironclaw_webui` (not a dependency).
- `webui/facade.rs`: comment now says routing/auth/static/SSE live in
`ironclaw_webui`; only the route-mount vocabulary stays in `route_mounts`.
build.rs frontend opt-out (gemini):
- `SKIP_FRONTEND_BUILD=1` skips the Node/pnpm frontend build for backend-only
dev / docs.rs / minimal CI images (`webui_enabled = env::var_os(...).is_none()`).
Guidance docs (ironloopai "update AGENTS/CLAUDE + crates/AGENTS.md"):
- `crates/AGENTS.md`: rewrote the `ironclaw_webui` row to the whole WebUI host
stack, removed the deleted `ironclaw_webui_v2` row, repointed cross-refs.
- Refreshed `ironclaw_webui_v2` → `ironclaw_webui` across living guidance
(`.claude/` rules/skills/commands, `crates/README.md`, `crates/Architecture.md`,
`crates/ironclaw_projects/CLAUDE.md`, `ironclaw_reborn_composition/CLAUDE.md`,
product_workflow comments, security-parity docs) and the `-p ironclaw_webui_v2
--features webui-v2-beta` commands. `ironclaw_webui_v2_static` (a distinct,
still-live v1 crate) left untouched.
Already addressed earlier in this PR, confirmed still green post-merge:
- Slack / OpenAI-compat host-beta compile + wiring (the `slack-v2-host-beta` /
`openai-compat-beta` findings) — commit `a2ed602`.
- The obsolete `/v2` SPA mount — replaced by main's root-serving
`static_router_with_config` in the merge (`c000a16`).
Verified: clippy `-D warnings` on composition (`webui-v2-beta`) and
`ironclaw_webui` (`--all-features`); root int-tier webui tests compile;
QA-inventory path resolves; harness lock clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(ci): rustfmt import ordering after crate rename + refresh composition pub-use snapshot
Two CI failures on PR #6194:
- **Formatting / Code Style (fmt+clippy)**: the `ironclaw_reborn_webui_ingress`
→ `ironclaw_webui` rename shifted where the crate sorts in `use` blocks, so
rustfmt wanted to reorder imports across ~26 files. I had wrongly reverted
those fmt-only files during the rename commit (assuming rustfmt-version
drift); the reordering is deterministic and CI's gate caught it. Ran
`cargo fmt --all`.
- **Test Reborn crate bucket (adapters-misc)** →
`composition_public_pub_use_surface_matches_snapshot`: this PR intentionally
changed composition's public facade — `webui_serve`/`Webui*`/`webui_v2_app`
moved out to `ironclaw_webui` (so composition's `webui` re-export is now just
`route_mounts::*`), the product-auth mount builders were exposed, and the
Slack channel-route mount + `mark_bearer_token_verified_for_tenant` were
promoted. Regenerated `docs/plans/composition-pubuse.snapshot` by replaying
the test's own `extract_pub_use_surface` extraction; the diff is exactly those
intended facade changes.
Verified: `cargo fmt --all -- --check` clean; `cargo test -p
ironclaw_architecture --test reborn_composition_boundaries` green (8 passed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(composition): extract runtime.rs inline test module (Phase 0) (#6173)
* refactor(composition): extract runtime.rs inline test module to sibling file
Moves runtime.rs's trailing `#[cfg(test)] mod tests { … }` (~6.9k lines) into
`runtime/tests/core.rs` via the crate's existing `#[path = "runtime/tests/…"]`
convention. Pure move — the module keeps its identity (`crate::runtime::tests`),
so all `super::`/`crate::` refs resolve unchanged; cargo fmt de-indented the
relocated items. runtime.rs: 11,673 -> 4,709 lines.
Phase 0 of the composition decomposition (parent #4471, plan #6168): single-
crate, zero cross-crate coupling, does not touch slack/ or extension_host/.
Fixes the crate's worst file-size violation and — because the inline test block
no longer counts as production LOC (it's now a test-only file, excluded) —
ticks the composition mass ratchet down (~23.98% -> ~23.2%).
Verified: `cargo test -p ironclaw_reborn_composition --all-features --no-run`
compiles all relocated tests unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(composition): rustfmt runtime test module + merge main
Remove stray leading blank line in runtime/tests/core.rs flagged by the
Formatting CI check, and merge origin/main to bring the branch current.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Require read-before-edit and reject stale edits in reborn coding tools (#5978)
* Ride out provider outages and drop the 32-call turn cap in the reborn loop
Two failure modes discovered via claw-swe-bench-lite run 9ca133e5 (30% vs
hermes 65% on the same model) discarded hours of agent work:
- A transient provider 5xx storm aborted the whole run after 2 quick
retries (max_attempts_per_class=2, backoff capped at 5s). Availability-
class model errors (transient/unavailable/internal) now retry on their
own deeper budget: max_model_availability_attempts=12 with a 1s..60s
exponential backoff, riding out ~7 minutes of sustained provider
failure. MAX_MODEL_RETRIES raised 8 -> 16 to let the strategy govern.
- DefaultBudgetStrategy's iteration_limit=32 failed closed mid-task with
no synthesis (llm_calls in failed bench tasks clustered at exactly
63/64/127/128). The default is now DEFAULT_ITERATION_BACKSTOP=1024
(subagent 16 -> 256), documented as a runaway backstop: operational
bounds are the resource budget system and stop-condition strategy.
New seam mirroring IRONCLAW_REBORN_PLANNED_DEFAULT_ITERATION_LIMIT:
IRONCLAW_REBORN_MODEL_AVAILABILITY_RETRY_ATTEMPTS ->
DefaultPlannedRuntimeConfig.planned_model_availability_retry_attempts ->
families::default_with_overrides. The integration group harness pins
attempts=1 so scenarios that deliberately script provider failures
(failure_category_demasked) reach Failed in seconds, not minutes; the
availability-retry tests run under paused tokio time.
Family fingerprint digests regenerated for the new strategy parameters.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Surface tool-failure reasons to the model for shell and coding tools
Benchmark traces showed the model retrying identical failing calls blind:
builtin.shell parameter errors and coding-tool path rejections reached it
as a bare category ("the tool input could not be encoded") because the
handlers built FirstPartyCapabilityError/CodingCapabilityError with no
safe_summary — the model-visible Diagnostic detail channel downstream was
already wired but starved (one agent burned 13 apply_patch calls against
an out-of-scope /testbed path with empty errors).
- shell.rs: shell_error/process_error now carry the concrete reason
("missing 'command' parameter", timeout duration, spawn failure),
bounded to 512 chars. The strict safe-summary validator still falls
back to the fixed category string; the reason always survives on the
secret-scrubbed diagnostic channel.
- coding/paths.rs: scoped-path rejections name the offending path and
the available scoped roots; permission rejections say the operation is
not permitted on that mount.
Covered at the dispatch tier (coding state dispatch, host-runtime
invoke_capability) per test-through-the-caller.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Run agent_loop scenario test targets under paused tokio time
The deep availability-retry backoff added for provider-outage ride-out
made outage-scripting scenario tests sleep for real: safety_nets alone
took ~423s (the exact cumulative backoff schedule) because scripted or
script-exhausted model errors now retry for minutes. Pause the clock on
all executor scenario targets — they drive the in-process mock host
exclusively, so timers auto-advance and the suites return to seconds.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Fail fast when no LLM provider is configured instead of riding availability retries
The placeholder unconfigured provider's RequestFailed was mapped through the
catch-all Unavailable arm, so the new deep availability retry budget rode a
permanent configuration fault through ~7 minutes of exponential backoff.
Users with no LLM configured waited minutes for an error that retrying can
never fix, and the Reborn CLI smoke tests that pin fast nonzero exits timed
out (the 4 failures on CI run 29136954176).
Map errors carrying the shared UNCONFIGURED_PROVIDER_ID to
CredentialUnavailable, which is unclassified in loop recovery and therefore
terminal on first sight; the Settings → Inference hint travels on the
scrubbed detail channel. The provider id moves to a shared constant in
ironclaw_llm so the composition placeholder and the runner mapping cannot
drift.
Regression tests: unconfigured_provider_error_maps_to_credential_unavailable_
not_availability and unconfigured_provider_detection_requires_the_placeholder_
provider_id in model_gateway.rs; the existing smoke tests
(*_exits_nonzero_when_runtime_does_not_produce_reply) pin the fast-fail at
the caller tier.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Derive override-built default-family replay identity from resolved config
families::default_with_overrides swapped budget/recovery strategies but
reused the planner's static version digest, so an overridden composition
carried the pure-default replay identity — violating the component-identity
contract (family.rs: the digest identifies replay-relevant configuration).
- Turn the cfg(test) fingerprint const into a runtime
default_family_fingerprint(iteration_limit, model_availability_attempts)
builder; override-built families hash it with their resolved values at
composition time (BLAKE3, same path as the pinned const). The pure-default
composition keeps the static DEFAULT_FAMILY_DIGEST, and overrides spelling
out the production defaults hash to that same digest.
- Collapse the two Option args into a FamilyOverrides struct and drop the
now-dead (None, None) branch in the runner's registry factory.
- Tests: digest differs per override knob and is deterministic; explicit
production defaults reproduce the static digest; an attempts=1 override
reaches the composed recovery strategy (one retry then abort).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Let the recovery strategy own the model retry guard; wake backoff on cancel
Two model-stage fixes from the PR 5959 review:
MAX_MODEL_RETRIES=16 silently capped any configured availability budget of
16+: the retry loop fell through to a generic ModelError exit with
FailedExitDetails::default() — no failure category, no diagnostic ref —
before the strategy could reach its own Abort. The executor now derives the
loop bound from the composed strategy via
RecoveryStrategy::max_total_model_attempts() (DefaultRecoveryStrategy
computes it from its per-class + availability budgets with margin), so
every accepted override reaches the strategy's abort boundary. The
contract-bug fall-through now carries the last observed model error's
category and diagnostic ref instead of empty details.
The availability backoff sleep (up to 60s per attempt) was not
cancellation-aware: a cancel request could wait out the full delay. The
sleep now selects over the host's cancellation_requested() future (same
pattern as the prompt-compaction and failure-explanation waits), and a
boundary cancel check right after the alteration turns the wake into a
checkpointed Cancelled exit without issuing another model call.
Tests (paused tokio time): an availability budget of 20 — past the old
executor cap — fails with the strategy's model_unavailable category and
diagnostic ref after exactly 21 model calls; cancellation requested during
the first 1s backoff exits Cancelled without riding out the sleep.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Clarify DEFAULT_ITERATION_BACKSTOP doc: resource budgets are not yet enforced
The doc claimed operational bounds come from the resource budget system,
but ResourceBudgetPolicy.max_model_calls and the wall-clock cap are defined
and not applied; until they are, this backstop and the stop-condition
strategy are the only live ceilings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Carry tool-failure reasons to the model past the strict summary validator
PR 5959's headline feature (model-visible tool-failure reasons) never
reached the model for path-bearing reasons: LoopSafeSummary rejects
path/payload delimiters and newlines, and dispatch_failure_message
silently degraded every such reason to the generic category sentence
before it could reach the diagnostic channel.
- production.rs (failure_from): a host-authored safe_summary that fails
LoopSafeSummary validation is preserved as the new
DispatchFailureDetail::Diagnostic instead of being dropped; the
message keeps the fixed category sentence (host-authored, Invariant 2).
- capability_port.rs: maps the Diagnostic detail into the model-visible
CapabilityFailureDetail::Diagnostic, scrubbing secret values and
normalizing control characters the observation validator rejects (so
one stray escape byte cannot drop the whole observation); newlines
are preserved. The RetrySameCall arm now forwards structured detail
too.
- coding/paths.rs: scoped-path rejection summaries render the path and
available roots delimiter-free ("path testbed replacer.go",
"available roots: workspace") so they pass the strict validator —
FilesystemDenied surfaces as a Denied loop outcome whose only
model-visible channel is the summary itself.
- shell.rs: bounded_failure_reason documents the (now real) diagnostic
flow; truncation remains char-based (no byte-boundary panics).
Regre…
This was referenced Aug 21, 2026
This branch was successfully 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
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.
Root cause
LibSqlRootFilesystem::connect()opened a brand-new libSQL connection (sqlite3_open_v2+ multi-statement PRAGMA batch) for everyRootFilesystemoperation, by design ("every operation gets its own connection"). Under genuinely parallel CAS storms — multi-threaded tokio runtime, 16 concurrentcas_updatewriters against one shared WAL database — that unbounded open/PRAGMA/close churn intermittently fails inside the C library:SQLITE_MISUSE("bad parameter or other API misuse"), spuriousdisk I/O error, and occasionally a schema-visibility race (no such table), always surfacing from the post-open PRAGMAexecute_batch. This is #5466's ~10% failure / SIGABRT. No prior test could catch it: the crate's tests all ran on tokio's single-threaded flavor, which cannot produce cross-OS-thread simultaneity.During diagnosis the opposite extreme was also disproven: caching one shared connection for all callers eliminates the churn failures but corrupts CAS itself — the rows-affected readback after
UPDATE ... WHERE version = ?is per-connection state, so two tasks interleaving statements on one connection observe each other's counts, producing silent lost updates (reproduced: final counter below expected).Fix
A small bounded connection pool (
deadpool::managed, the same pooling corePostgresRootFilesystemalready uses viadeadpool-postgres; only the already-resolved core crate is added, gated behind thelibsqlfeature) in the newcrates/ironclaw_filesystem/src/libsql_pool.rs:create()= the existingconnect_with_retry(moved into the pool module),recycle()rejects connections returned mid-transaction so a failedROLLBACKcan never leak an open transaction to an unrelated caller.put()'s threeCasExpectationarms nowdrop(conn)before their nestedcurrent_versionreadback (mirroringvector_nearest_query's existing pattern), upholding the documented one-checkout-per-call-stack invariant.FilesystemOperation::Connect, matching the Postgres backend's pool-checkout error shape.Purely internal to
LibSqlRootFilesystem— no trait, caller, orcas_updatechanges.Proof
tests/concurrent_cas_storm.rs(16 spawned writers x 100cas_updateincrements, 8-worker runtime) failed ~90%+ of runs against the old code with the exact Parallel same-tenant turn-runs vs FilesystemTurnStateStore CAS / libsql backend (~10% failure) #5466 signature.ironclaw_filesystemandironclaw_turnssuites (--all-features).recycle()discard every returned connection (restoring per-op churn through the shipped pool machinery) → RED on first run withSQLITE_MISUSE. Reverted.no lost updatesfinal-count assertion (run 3 of a 12-run loop). Reverted.db_root_filesystem_contract.rs) backends; each backend variant lives in its own[[test]]bin file so a regression abort cannot take down unrelated suites.cargo fmt,cargo clippy --all --benches --tests --examples --all-features(zero warnings),cargo build --tests --all-featuresall clean.Closes #5466
🤖 Generated with Claude Code