docs(adr): separate binaries by default, opt-in unified build#1116
Merged
Conversation
Add ADR for restructuring OpenAB as a Cargo workspace that ships all platform adapters in a single binary, activated at runtime via config. This eliminates the two-process (core + gateway sidecar) deployment model for most users while keeping the standalone gateway available for advanced use cases.
This comment has been minimized.
This comment has been minimized.
- Default: two separate binaries (core + gateway), unchanged from today - Opt-in: --features unified compiles all adapters into single binary - Zero code changes needed — just a build flag or image swap - Published: openab:latest (core), openab:unified (all-in-one)
This comment has been minimized.
This comment has been minimized.
Contributor
|
From Fellowship of the Code ## 🧙 Gandalf — Aggregated Review Synthesis for PR #1116
### Verdict: **Request Changes** (4 request-changes, 2 approves, 1 request-changes — 7/7 reviewers reported)
---
### 🔴 Critical / Must-Fix
| # | Reviewer | Finding |
|---|----------|---------|
| F1 | Aragorn | **Silent feature-default regression** — `secrets-aws`/`agentcore` defaults dropped. Plain `cargo build` would lose AWS Secrets Manager support — a deployment regression hiding inside a docs-only PR. **Top-priority fix.** |
| F1 | Merry | **Config contract contradiction** — "Default behavior unchanged" (L213) conflicts with `[telegram]`/`[line]` sections working in both modes (L215). Today's two-binary path uses `[gateway]` config + env/Helm on the gateway side. Split the config contract: `[gateway]` stays as two-binary core contract; `[telegram]`/`[line]` are unified-mode only unless a compatibility/translation layer is specified. |
| F2 | Sam | **"Supersedes" overstates the relationship** — The custom-gateway ADR's outbound-only contract still holds (gateway is default). Use "Related" / "Amends" instead. Suggested reword (Frodo): `- **Amends:** Deployment model from [ADR: Custom Gateway](./custom-gateway.md) — outbound-only + gateway sidecar remains the default and is only waived under the --features unified (or BUILD_MODE=unified) opt-in path.` |
| F4 | Aragorn | **Superseding a Proposed ADR** — Cannot supersede a document still in Proposed status. Fix the lifecycle reference. |
---
### 🟡 Suggestions / Nits
| # | Reviewer | Finding |
|---|----------|---------|
| N1 | Boromir | Dockerfile `BUILD_MODE` RUN block is fragile for `FEATURES` quoting/multi-arg. Consider a dedicated target or `.cargo/config.toml` note. (Frodo notes this belongs in the implementation PR, not the ADR.) |
| N1 | Sam | `src/telegram.rs` example (L145) doesn't match workspace layout (`crates/openab-gateway/`). Use crate-level path or mark as illustrative. |
| N5 | Frodo | Workspace layout shows both `gateway/` and `crates/openab-gateway/` during transition — duplication risk and intended relationship (thin binary re-exporting the library crate, not a second copy) should be called out in the ADR body. |
---
### 🟢 Positive Observations
- **Correct conservative direction**: Flipping to separate-by-default with unified as opt-in respects "zero disruption" and existing users (Boromir, Sam, Frodo, Merry, Aragorn).
- **Clean runtime activation model**: compiled-in but unconfigured adapters have zero overhead; start only if config section is present (Frodo). Worth a "Design Principle" callout.
- **ADR authorship quality**: Scope correction between commits is model process (Boromir).
- **Opt-in via feature flag** is well-calibrated for an active fleet — existing two-binary deployments need no migration (Aragorn).
---
### 🔧 Recommended Actions (priority order)
1. **[Blocking]** Restore `secrets-aws`/`agentcore` as default features so plain `cargo build` doesn't regress.
2. **[Blocking]** Fix ADR lifecycle: cannot supersede a Proposed ADR — correct the status/relationship.
3. Fix config contract: clearly separate `[gateway]` (two-binary) from `[telegram]`/`[line]` (unified-only).
4. Change "Supersedes" → "Amends" with explicit outbound-only default statement.
5. Address workspace layout duplication narrative in the ADR body.
6. Minor: fix example path to match crate structure.
---
*7/7 reviewers reported. 4 request-changes, 2 approves.* |
This comment has been minimized.
This comment has been minimized.
- Status: Proposed → Accepted - Workspace layout: core stays in root crate, openab-core deferred to Phase 2 - Feature flags: discord = ["dep:serenity"] (not openab-core/discord) - Dockerfile: document root (--no-default-features) vs agent (additive) semantics - Add cross-reference to docs/image-tags.md for tagging convention - Add implementation link to PR #1146 Consensus: team unanimously chose option 2 (structural alignment only, operational details stay in dedicated docs).
Collaborator
Author
ADR Updated — Aligned with Implementation (#1146) ✅Changes (2 commits:
|
| # | Section | Update |
|---|---|---|
| 1 | Status | Proposed → Accepted + implementation link to #1146 |
| 2 | Workspace Layout | Corrected: core stays in root crate, openab-core extraction deferred to Phase 2 |
| 3 | Feature Flags | Updated to match implementation: discord = ["dep:serenity"], added secrets-aws/agentcore in defaults, added optional = true dependency example |
| 4 | Dockerfile | Added build semantics table: root (--no-default-features) vs agent (additive --features) |
| 5 | Image Tagging | Not added to ADR — cross-reference link to docs/image-tags.md instead |
| 6 | Supersedes → Amends | Custom-gateway ADR is still Proposed — cannot be superseded. Changed to "Amends" with scoped language preserving outbound-only default |
| 7 | Config contract | Clarified: [telegram]/[line] are unified-mode additions; [gateway] remains the two-binary contract |
External Review Findings Addressed
Responding to @canyugs Fellowship of the Code review:
| Finding | Severity | Status |
|---|---|---|
F1: secrets-aws/agentcore dropped from defaults |
🔴 | ✅ Fixed — restored in proposed default features |
| F2: "Supersedes" invalid lifecycle | 🔴 | ✅ Fixed — changed to "Amends" with scoped language |
| F3: Config contract contradiction | 🟡 | ✅ Fixed — clarified [telegram]/[line] are unified-mode only |
| F4: Cannot supersede Proposed ADR | 🔴 | ✅ Fixed — merged with F2 |
F5: Missing optional = true declaration |
🟡 | ✅ Fixed — added dependency example |
F6: gateway/ vs crates/openab-gateway/ unclear |
🟡 | ✅ Fixed — new layout makes relationship clear |
F7: src/telegram.rs path contradicts layout |
🟡 | ℹ️ Acceptable — §4 is a conceptual flow diagram, not a file path reference |
| N1: Dockerfile quoting fragility | 🟡 | ℹ️ Implementation concern — addressed in #1146, not ADR scope |
Team Consensus
All 5 reviewers chose Option 2 — update structural decisions only, keep operational details in dedicated docs.
Principle Applied
ADR records decisions and rationale. Implementation details live in code and dedicated docs. The two stay aligned on structural boundaries but don't duplicate each other.
This PR is now ready for merge — all critical and important findings from both internal team and external reviewers have been addressed.
- F2: Supersedes → Amends (custom-gateway ADR is still Proposed) - F3: Clarify config contract — [telegram]/[line] are unified-mode additions, [gateway] remains the two-binary contract - F5: Add optional dependency declaration example for dep:openab-gateway
thepagent
approved these changes
Jun 19, 2026
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.
Summary
Proposes restructuring OpenAB as a Cargo workspace that keeps two separate binaries by default (core + gateway), while enabling a single unified binary via
--features unifiedor a DockerfileBUILD_MODEarg — zero code changes required.Key Design
cargo build→ core binary (Discord + Slack). Gateway ships separately.cargo build --features unified→ all adapters in one binary.docker build --build-arg BUILD_MODE=unifiedflips to single binary.telegram,lineif you don't need all adapters.Published Artifacts
openab:latestopenab-gateway:latestopenab:unifiedChanges
docs/adr/unified-binary.md— new ADR document