relayburn-cli: make pending-stamp adapter compatible with phf registry (follow-up to #307)#311
Conversation
The Wave 2 plan in `harnesses/registry.rs` calls for codex/opencode to register through `pending_stamp::adapter`, but that factory returned a runtime `Box<dyn HarnessAdapter>` while the `ADAPTERS` map is a compile-time `phf::Map<&'static str, &'static dyn HarnessAdapter>`. A `Box` cannot satisfy the `&'static` value bound, so the codex / opencode adapters would not have compiled when their PRs landed. Add `pending_stamp::adapter_static` that `Box::leak`s the impl and returns `&'static dyn HarnessAdapter`. Wave 2 adapters wrap the call in a `LazyLock<&'static dyn HarnessAdapter>` next to their module declaration. The leak is bounded (one Box per harness, ~3 ever) and adapters live until process exit anyway, so it's the right trade-off versus converting the registry to a runtime `OnceLock<HashMap<...>>`. Add a regression test in `harnesses/registry.rs` that proves a `pending_stamp::adapter_static` value satisfies the same value-type bound `ADAPTERS` requires. Follow-up to #307. Addresses the CodeRabbit review comment at #306 (comment). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a static-lifetime factory ChangesStatic HarnessAdapter Factory & Registry Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d091cbb695
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let map: std::collections::HashMap<&'static str, &'static dyn HarnessAdapter> = | ||
| std::iter::once(("codex", leaked)).collect(); |
There was a problem hiding this comment.
adapter_static through phf_map!
This regression test is meant to prove compatibility with the production ADAPTERS table, but it inserts the leaked adapter into a runtime HashMap instead of a phf_map! static. That misses the key constraint of the real registry path (compile-time/static initialization), so the test can pass even if codex/opencode wiring still cannot be represented in ADAPTERS. Please assert compatibility via a phf-shaped static check rather than HashMap so this catches the original class of failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b0a7542 by going with the architectural option ("split eager vs runtime registries") rather than just rewriting the test in place.
Why a split: pending_stamp::adapter_static does Box::leak at runtime, so the resulting &'static dyn HarnessAdapter is not a const expression and phf_map! can't hold it directly. The previous registry doc-comments implied codex/opencode would land in ADAPTERS, but that was wrong — those slots could only ever hold const references like &CLAUDE_ADAPTER. The new layout reflects the real constraint:
EAGER_ADAPTERS: phf::Map<…>— claude (const&to a static unit struct).RUNTIME_ADAPTERS: LazyLock<HashMap<&'static str, &'static dyn HarnessAdapter>>— codex / opencode (adapter_static-built leaked references).
lookup probes phf first then falls back to the runtime tier, so common-case lookups still pay only a single perfect-hash probe.
Test now exercises the real production path. The regression test (pending_stamp_adapter_static_fits_runtime_registry) uses a module-scoped FAKE_RUNTIME_REGISTRY: LazyLock<HashMap<…>> with the exact value type production uses for RUNTIME_ADAPTERS. If adapter_static ever stopped returning &'static dyn HarnessAdapter, construction of that fixture would fail to compile.
Belt-and-braces: I also added a compile-time assertion that pins adapter_static's signature to the registry's value bound:
const _ASSERT_ADAPTER_STATIC_FITS_REGISTRY:
fn(PendingStampAdapter) -> &'static dyn HarnessAdapter =
pending_stamp::adapter_static;Verified by deliberate breakage: I temporarily made adapter_static return Box<dyn HarnessAdapter> and confirmed both the runtime fixture and the const assertion fail to compile (expected \&dyn HarnessAdapter`, found `Box`). Reverted and cargo test --workspace` is green.
The previous registry doc-comments claimed codex/opencode would land in the `phf::Map`, but pending-stamp adapters are constructed at runtime via `Box::leak` and cannot appear in `phf_map!` (which needs const expressions). Split the registry into two tiers that match the real constraint: * `EAGER_ADAPTERS: phf::Map<…>` — for stateless unit-struct adapters whose value is a const reference (claude lands here). * `RUNTIME_ADAPTERS: LazyLock<HashMap<…>>` — for adapters built via `pending_stamp::adapter_static` whose `&'static dyn HarnessAdapter` is produced at runtime (codex / opencode land here). `lookup` checks the phf tier first, then falls back to the runtime tier; common-case lookups still pay only a single perfect-hash probe. Rewrites the regression test to exercise the actual production wiring path: `FAKE_RUNTIME_REGISTRY` uses the same `LazyLock<HashMap< &'static str, &'static dyn HarnessAdapter>>` value type that production uses, so insertion of a leaked `pending_stamp::adapter_ static(...)` reference proves compatibility with the registry it will actually live in. Adds a compile-time assertion (`_ASSERT_ADAPTER_STATIC_FITS_REGISTRY`) that pins `adapter_static`'s signature to the registry's value bound — if the factory regresses to `Box<dyn HarnessAdapter>`, both the runtime test fixture and the const assertion fail to compile. Addresses Codex review comment on PR #311. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
🤖 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/relayburn-cli/src/harnesses/registry.rs`:
- Around line 115-118: list_harness_names currently dereferences
RUNTIME_ADAPTERS (a LazyLock) which forces its initialization and also relies on
HashMap key order, causing non-deterministic help output; instead, introduce a
separate static ordered list (e.g. RUNTIME_ADAPTER_NAMES: &[&'static str']) that
contains runtime adapter names in the desired deterministic order and change
list_harness_names to extend the eager names with that static slice without
touching RUNTIME_ADAPTERS; update references to RUNTIME_ADAPTERS only where
runtime initialization is actually required.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d14b0e7-8a92-43fb-bfbc-82c6e90bc0b5
📒 Files selected for processing (1)
crates/relayburn-cli/src/harnesses/registry.rs
…erministic ordering (review fix round 2) list_harness_names previously called RUNTIME_ADAPTERS.keys(), which forces LazyLock init via Deref and defeats the cold-start goal stated in the registry's own doc comment (burn --help shouldn't pay harness-table construction cost). It also leaked HashMap's HashDoS-randomized iteration order into user-facing help output, so the harness list could flicker between runs once Wave 2 adapters land. Fix: * Add a sibling RUNTIME_ADAPTER_NAMES: &[&str] static next to RUNTIME_ADAPTERS. Doc comment calls out the Wave 2 sync requirement: every codex (#248-e) / opencode (#248-f) PR must uncomment two rows in lockstep — its RUNTIME_ADAPTERS insert and its RUNTIME_ADAPTER_NAMES entry. * list_harness_names now extends with RUNTIME_ADAPTER_NAMES.iter(). RUNTIME_ADAPTERS is no longer touched on the help path. * Replace the empty-list test with list_harness_names_is_deterministic, which snapshots the expected ordering against an EXPECTED_HARNESS_NAMES constant and asserts two consecutive calls return the identical Vec. Doc comment leaves a cold-start proof: by inspection, list_harness_names doesn't touch the LazyLock, so the test passes without ever forcing init. * Add runtime_adapter_names_match_runtime_adapters as a sync-invariant test: every name in RUNTIME_ADAPTER_NAMES must resolve through lookup. Empty no-op today, pinned for Wave 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Follow-up to #307 (commit
fb47c86) addressing the CodeRabbit review comment at #306 (comment).The bug
crates/relayburn-cli/src/harnesses/registry.rsdeclares the harness registry aswhile
pending_stamp::adapterreturnedBox<dyn HarnessAdapter>. The Wave 2 plan documented in the same file (lines 28–31 of the original) said codex and opencode would be "constructed throughsuper::pending_stamp::adapter", but a runtimeBoxcannot satisfy a&'static dyn HarnessAdaptervalue bound. The codex / opencode adapter PRs (#248-e / #248-f) would not have compiled.The fix — Option A (
Box::leak)Add
pub fn adapter_static(config: PendingStampAdapter) -> &'static dyn HarnessAdapternext to the existingadapter. The new functionBox::leaks the impl so it can be stored as&'static. Wave 2 adapters register via:Why option A over option B (runtime
OnceLock<HashMap<...>>)phf::Mapperfect-hash lookup intact — the original PR called this out as a deliberate cold-start optimisation forburn --help/burn summary.Send + Syncbound spread, no first-lookup runtime cost, noArcwrapper to hand back a borrow.adapter(returningBox) is preserved for unit tests and bespoke drivers that want a droppable instance.The
Box::leakis documented onadapter_staticso callers know what they're opting into.Test added
harnesses/registry.rs::tests::pending_stamp_adapter_static_fits_phf_registryconstructs a fake pending-stamp config, runs it throughadapter_static, and proves the resulting&'static dyn HarnessAdaptersatisfies the same value-type bound thatADAPTERSrequires. This is the regression test that would have caught the original architectural mismatch.Test plan
cargo build -p relayburn-clicleancargo test --workspacegreen (605 sdk + 10 cli + 2 integration + 3 doc-tests, 0 failed)RUSTDOCFLAGS="-D warnings" cargo doc -p relayburn-cli --no-depsclean (also fixed three pre-existing rustdoc warnings around private-item links)pending_stamp_adapter_static_fits_phf_registryruns and passes