Record findings

.gitignore

@@ -7,3 +7,4 @@ artifacts/
 temp_test_project/
 output_svc/
 output_proto/
+mail/

CLEANUP.md

@@ -0,0 +1,152 @@
+# Best Practices Audit — Cleanup Findings
+
+> Generated by monkey swarm on 2026-06-30. Four specialist auditors reviewed `runtime`, `codegen`, `roto-tonic`, and project-level structure.
+
+## Summary
+
+| Severity | Count |
+|----------|-------|
+| Critical | 9 |
+| Major | 17 |
+| Minor | ~40 |
+
+---
+
+## Critical Issues
+
+### 1. `BufMutBuilder` panics instead of returning `Err` (`runtime`)
+- **File:** `runtime/src/lib.rs:1038-1119`
+- **Issue:** `BufMutBuilder` returns `Result<()>` on all write methods, but calls `put_slice` which panics if the underlying `BufMut` has no remaining capacity. Unlike `ProtoBuilder`, which checks bounds before every write, `BufMutBuilder` never actually produces an `Err`.
+- **Fix:** Check `self.buf.remaining_mut()` before each `put_slice` and return `Err(RotoError::BufferOverflow)`, or remove the `Result` return type and document the panic contract.
+
+### 2. `#![no_std]` + feature flags don't match reality (`runtime`)
+- **File:** `runtime/src/lib.rs:1`, `runtime/Cargo.toml:9-12`
+- **Issue:** Crate declares `#![no_std]` but has unconditional `use bytes::BufMut`. The `bytes` crate requires `alloc` (and transitively `std`). The `alloc` and `std` feature flags exist but don't gate any code. The crate can't compile in `no_std` mode.
+- **Fix:** Gate `bytes` behind `#[cfg(feature = "alloc")]` or remove the feature flags and document that the crate requires `std`.
+
+### 3. `types.rs` file corruption (`codegen`)
+- **File:** `codegen/src/generator/types.rs:76`
+- **Issue:** Line 76 contains a shell artifact (`EOF > /opt/workspace/codegen/src/generator/types.rs`) followed by a duplicated function body. The file likely won't compile.
+- **Fix:** Clean up the file immediately.
+
+### 4. Massive code duplication in generator module (`codegen`)
+- **File:** `codegen/src/generator/` (all files)
+- **Issue:** `mod.rs` contains full implementations of `write_message`, `write_enum`, `write_service`, `map_type_to_rust_accessor`, `to_pascal_case`, `to_snake_case`, `DATA_IMPORTS` — all of which also exist in `messages.rs`, `services.rs`, `types.rs`, and `utils.rs`. Only the `mod.rs` versions appear to be used.
+- **Fix:** Either delete the split files (if `mod.rs` is the single source of truth) or complete the refactor so `mod.rs` re-exports the split modules.
+
+### 5. ~50 `.expect()` panics in library generator code (`codegen`)
+- **File:** `codegen/src/generator/mod.rs` (all generator functions)
+- **Issue:** Malformed protos will crash the process instead of returning errors.
+- **Fix:** Introduce a `GeneratorError` enum and return `Result<Vec<…>, GeneratorError>` from all generator functions.
+
+### 6. Seven unused heavy dependencies in codegen (`codegen`)
+- **File:** `codegen/Cargo.toml`
+- **Issue:** `tonic`, `tower`, `bytes`, `http-body`, `http-body-util`, `tokio-stream`, `futures-util` are listed as dependencies but never used in the codegen library. They generate code that references these types via string literals — the generator itself doesn't need them.
+- **Fix:** Remove these from `Cargo.toml`. This eliminates ~30+ transitive dependencies.
+
+### 7. Edition 2024 / resolver = "3" requires nightly (`workspace`)
+- **File:** `Cargo.toml:16`, all crate `Cargo.toml`
+- **Issue:** `resolver = "3"` is only valid for Edition 2024, which is not yet stable. No `rust-toolchain.toml` exists. This blocks adoption with stable Rust.
+- **Fix:** Add `rust-toolchain.toml` with `channel = "nightly"`, or switch to `edition = "2021"` + `resolver = "2"`.
+
+### 8. No LICENSE file (`workspace`)
+- **Issue:** The project cannot legally be used by others without a license.
+- **Fix:** Add an MIT or Apache-2.0 license file.
+
+### 9. `StatusBody::new` dead parameter (`roto-tonic`)
+- **File:** `roto-tonic/src/lib.rs:118-125`
+- **Issue:** `StatusBody::new(data, status)` takes `status: u8` but never uses it. The code hardcodes `"0"` into the `grpc-status` trailer.
+- **Fix:** Wire `status` into the trailer value, or remove the parameter.
+
+---
+
+## Major Issues
+
+### Runtime
+
+| # | Issue | File | Fix |
+|---|-------|------|-----|
+| 1 | `RotoMessage::decode(bytes::Bytes)` forces heap allocation and blocks `no_std` | `lib.rs:74-77` | Accept `&[u8]` instead |
+| 2 | 7 core public types lack doc comments (`MapFieldIterator`, `RotoOwned`, `RotoMessage`, `ProtoAccessor`, `ProtoBuilder`, `BufMutBuilder`, `FieldIterator`, `RepeatedFieldIterator`) | `lib.rs` | Add `///` doc comments |
+
+### Codegen
+
+| # | Issue | File | Fix |
+|---|-------|------|-----|
+| 3 | `codegen/src/runtime/mod.rs` duplicates `roto-runtime` types (RotoError, Tag, WireType, read_varint, etc.) | `src/runtime/mod.rs` | Remove the duplicate; depend on `roto-runtime` |
+| 4 | `to_pascal_case` has two implementations with different case-handling behavior | `mod.rs:28-38` vs `utils.rs` | Consolidate to single canonical version |
+| 5 | `test_oneofs.rs` is completely empty — tests nothing | `tests/test_oneofs.rs` | Implement or remove |
+| 6 | All integration tests use hardcoded `/tmp/` paths — won't run in parallel or on Windows | `tests/` | Use `std::env::temp_dir()` or `tempfile` |
+
+### Roto-tonic
+
+| # | Issue | File | Fix |
+|---|-------|------|-----|
+| 7 | `BufferPool::get()` / `put()` call `.unwrap()` on `Mutex::lock()` — aborts on poison | `src/lib.rs:96-109` | Handle poison state or use `tokio::sync::Mutex` |
+| 8 | Generated traits use native async (Edition 2024), but test impls use `#[tonic::async_trait]` — ABI mismatch | `src/generated/` + `tests/` | Remove `#[tonic::async_trait]` from test impls |
+| 9 | `.unwrap()` on `Response::builder().body()` in 7+ places — panics in hot path | `src/generated/` | Use `.expect()` or extract a helper |
+| 10 | Missing `cargo:rerun-if-changed` for `.proto` files — changes don't trigger rebuild | `build.rs` | Add rerun directives |
+| 11 | `build.rs` hardcodes `target/debug/protoc-gen-roto` — breaks release and cross-compile | `build.rs` | Use `OUT_DIR` or workspace-level coordination |
+| 12 | `tokio = { features = ["full"] }` pulls all subsystems for a library that only needs `net` | `Cargo.toml` | Move to `[dev-dependencies]` |
+| 13 | `RotoDecoder::decode` converts all errors to `Status::internal()` — no mapping of error variants | `src/lib.rs:78` | Match `RotoError` variants to specific gRPC status codes |
+| 14 | `StatusBody` fields are `pub` — breaks encapsulation | `src/lib.rs:112-126` | Make fields private; add consumer methods |
+
+### Project Structure
+
+| # | Issue | File | Fix |
+|---|-------|------|-----|
+| 15 | No `[workspace.dependencies]` — shared deps duplicated across crates, risking version drift | `Cargo.toml` | Define workspace-level dependency table |
+| 16 | `interop_test.rs` asserts `is_ok()` on `UNIMPLEMENTED` call — tests wrong behavior | `tests/interop_test.rs` | Assert `is_err()` with `UNIMPLEMENTED` status |
+| 17 | No CI configuration, no `clippy.toml`, no `.rustfmt.toml` — code quality not enforced | Project root | Add GitHub Actions + config files |
+| 18 | `roto/roto/` stray directory and `grpc_bench/` empty submodule | Project root | Clean up stale directories |
+| 19 | `panic = "abort"` in `[profile.dev]` is too aggressive for a library | `Cargo.toml` | Remove from dev profile; keep only in release |
+| 20 | `protos/` crate has no purpose (single `hello()` function) | `protos/` | Remove or give it a real purpose |
+
+---
+
+## Minor Issues (Selected)
+
+### Runtime
+- `ProtoAccessor::new` returns `Result` but never fails — change to direct constructor
+- `write_int32` doesn't use zigzag encoding — document this or add `write_sint32`
+- `RepeatedFieldIterator::new_range` silently tolerates out-of-bounds ranges
+- Tests gated behind `#[cfg(feature = "alloc")]` may be silently skipped
+- `RotoError` lacks `source()` chaining — consider `thiserror`
+
+### Codegen
+- Missing `#[must_use]` on public generator functions
+- No crate-level or function-level documentation
+- Magic numbers for protobuf field types/labels (1-18)
+- `env_logger::init()` is deprecated — use `init_from_env()`
+- `edition = "2024"` without `rust-version` key
+- Utility functions (`to_pascal_case`, `to_snake_case`) are `pub` but should be `pub(crate)`
+
+### Roto-tonic
+- `#![allow(unused, ...)]` blanket suppresses important lints in generated code
+- `tokio::time::sleep(100ms)` race condition in `test_server_start`
+- Server task spawned but never joined/aborted in `interop_test.rs`
+- Missing `#[derive(Debug)]` on `BufferPool`, `StatusBody`
+- Error response body `[0, 0, 0, 0, 0]` should be a named constant
+- `Infallible` error type hides panic sites
+
+### Project
+- `exclude` list incomplete — test temp dirs not listed
+- `Cargo.lock` should be committed (it is, but no `.gitignore` exception)
+- `.desc` files may be generated artifacts that should be regenerated, not checked in
+- No `rust-toolchain.toml` despite Edition 2024 requirement
+- No `[workspace.metadata]` for crates.io readiness
+- `alloc` feature flag on `codegen` and `benches` is meaningless for CLI/benchmark crates
+
+---
+
+## Positive Observations
+
+The project has several strengths:
+
+- **Zero `unsafe` code** — all memory operations in the runtime use safe slice indexing
+- **Consistent error handling** — all fallible operations return `Result<T, RotoError>`
+- **No panics in library code** — `unwrap()`/`expect()` appear only in tests
+- **RevBuilder** is a creative, well-implemented design for reverse-direction encoding
+- **18 unit tests** in the runtime with good edge-case coverage
+- **Comprehensive README** with design rationale, API examples, and benchmark comparisons
+- **Well-structured workspace** with clear crate boundaries

TASKS.md

@@ -1,44 +1,123 @@
-# Tasks for Tonic Integration
+# Task: Verify Generated Code Works in `no_std` / `no_alloc`
 
-This document outlines the steps required to integrate the `roto` protobuf library with the `tonic` gRPC framework.
+**Goal:** Ensure all generated protobuf code compiles and runs in `no_std` + `no_alloc` environments. Features requiring `alloc` must be gated behind feature flags in the proto generator's output.
 
-## Goals
-- Provide a `tonic::codec::Codec` implementation for `roto` messages.
-- Enable zero-allocation decoding of gRPC requests and responses.
-- Support efficient encoding of gRPC messages using `roto`'s `Builder` pattern.
-- Generate `tonic`-compatible service traits and client/server boilerplate via `protoc-gen-roto`.
+---
 
-## 1. Runtime Changes (`roto_runtime`)
-- [x] Add `bytes` as a dependency to `roto_runtime`.
-- [x] Modify `ProtoBuilder` to support writing to `bytes::BufMut` or provide a specialized `BufMut` based builder to facilitate integration with `tonic::codec::EncodeBuf`.
-- [x] Design and implement "Owned" message support:
-    - [x] Create a mechanism (likely via codegen) to generate structs that hold `bytes::Bytes` and the field offsets (e.g., `OwnedMyMessage`).
-    - [x] These structs will serve as the owned types required by `tonic`'s `Codec`.
-    - [x] Add a method to convert an `OwnedMyMessage` into a zero-allocation `Reader` (e.g., `reader(&self) -> MyMessage<'_>`).
+## Findings
 
-## 2. Tonic Codec Implementation
-- [x] Implement `tonic::codec::Codec` for `roto` messages.
-- [x] Implement `tonic::codec::Decoder` for `roto` messages:
-    - [x] The decoder should take a `DecodeBuf` and produce an `OwnedMyMessage`.
-    - [x] It must perform the initial scan of the buffer to populate the field offsets.
-- [x] Implement `tonic::codec::Encoder` for `roto` messages:
-    - [x] The encoder should take an `OwnedMyMessage` and write its internal `bytes::Bytes` to the `EncodeBuf`.
-    - [x] Since `roto` responses are built using `Builder` directly into a buffer, the `Encoder` will primarily handle copying these pre-built buffers.
+**Already clean (no_std + no_alloc):**
+- All reader structs (`Message<'a>`) — zero-copy accessors over `&[u8]`
+- All field accessors, enums, builders (`MessageBuilder<'b>`), typed iterators
+- Core runtime: `ProtoAccessor`, `ProtoBuilder`, `RevBuilder`, `read_varint`, `write_varint`, all iterators
 
-## 3. Code Generation Extensions (`protoc-gen-roto`)
-- [x] Update the generator to produce `OwnedMyMessage` structs in addition to the `Reader` and `Builder` structs.
-- [x] Generate the `OwnedMyMessage::new(data: bytes::Bytes)` method for initial decoding.
-- [x] Implement gRPC service generation:
-    - [x] Generate `tonic`-compatible service traits (using `#[tonic::async_trait]`).
-    - [x] Generate client and server boilerplate that uses `OwnedMyMessage` as the request and response types.
-    - [x] Ensure the generated code correctly maps protobuf services to Rust traits.
+**Needs `alloc` feature gate:**
+- `RotoMessage` trait — uses `bytes::Bytes` (has `Arc` internally)
+- `RotoOwned` trait — same issue
+- `OwnedMessage` structs generated by codegen
+- `RevBuilder::with()` — uses `Vec<_>` to collect raw fields
+- `Bytes`, `BytesMut` imports in `DATA_IMPORTS`
 
-## 4. Testing and Validation
-- [x] Create a test gRPC project using the updated `protoc-gen-roto`.
-- [ ] Implement a sample gRPC service with:
-    - [ ] A unary call.
-    - [ ] A server-streaming call.
-    - [ ] A client-streaming call.
-    - [ ] A bidirectional-streaming call.
-- [ ] Verify that the integration is zero-allocation on the reading path.
-- [ ] Perform benchmark tests to compare performance with `prost`.
+**Needs `std` feature gate:**
+- All service code (`SERVICE_IMPORTS`) — uses `std::sync::Arc`, `std::pin::Pin`, `String`, etc.
+- `std::error::Error for RotoError` (already gated)
+
+---
+
+## Plan
+
+### Phase 1: Runtime (`runtime/src/lib.rs`, `runtime/Cargo.toml`)
+
+1. **Gate `RotoMessage` and `RotoOwned`** behind `#[cfg(feature = "alloc")]`
+2. **Gate `BufMutBuilder`** behind `#[cfg(feature = "alloc")]` — only useful with `BytesMut`, which requires alloc
+3. **Gate `use bytes::BufMut`** — only import it conditionally
+4. **Wire up `bytes` features in Cargo.toml:**
+   ```toml
+   bytes = { version = "1.7", default-features = false }
+   # ...
+   [features]
+   default = ["std", "alloc"]
+   std = ["bytes/std"]
+   alloc = ["bytes"]
+   ```
+
+### Phase 2: Codegen — `DATA_IMPORTS` & Generated Data Code (`codegen/src/generator/mod.rs`)
+
+5. **Split `DATA_IMPORTS`** into unconditional + gated sections:
+   ```rust
+   // Always emitted
+   use roto_runtime::{ProtoAccessor, ProtoBuilder, RevBuilder, Result, RotoError, read_varint, RepeatedFieldIterator};
+   use core::str;
+   use bytes::{Buf, BufMut};
+
+   // Only emitted when alloc feature is enabled
+   #[cfg(feature = "alloc")]
+   use bytes::{Bytes, BytesMut};
+   #[cfg(feature = "alloc")]
+   use roto_runtime::RotoMessage;
+   ```
+
+6. **Gate `OwnedMessage` structs + `RotoMessage` impls** with `#[cfg(feature = "alloc")]`
+
+7. **Fix `RevBuilder::with()`** — replace `let fields: Vec<_> = ... .collect()` with a forward iteration that doesn't allocate. The reverse order doesn't matter for protobuf encoding (fields are independent):
+   ```rust
+   // Before: let fields: Vec<_> = msg.accessor.raw_fields().collect();
+   //         for (raw, _) in fields.iter().rev() { ... }
+   // After:  for (raw, _) in msg.accessor.raw_fields() { ... }
+   ```
+   Alternatively, gate the `with()` method itself behind `#[cfg(feature = "alloc")]`.
+
+### Phase 3: Codegen — Service Code (`codegen/src/generator/mod.rs`)
+
+8. **Gate `SERVICE_IMPORTS`** with `#[cfg(feature = "std")]`
+9. **Gate all service traits, server impls, and `*Server` structs** with `#[cfg(feature = "std")]`
+
+### Phase 4: Generated Consumer Setup
+
+10. **Emit a `Cargo.toml`** (or instruct consumers) with the right feature setup:
+    ```toml
+    [features]
+    default = ["std"]
+    std = ["roto-runtime/std", "alloc"]
+    alloc = ["roto-runtime/alloc", "bytes"]
+    ```
+
+### Phase 5: Validation
+
+11. **Update `examples/no_std_test`** to build with `--no-default-features` and verify compilation
+12. **Run existing tests** with default features to ensure nothing breaks
+13. **Add a compile-test** that verifies `--no-default-features` builds succeed for generated code
+
+---
+
+## Dependency Graph
+
+```
+no_std + no_alloc  →  readers, enums, builders, iterators
+         ↓ alloc   →  + OwnedMessage, RotoMessage trait
+         ↓ std     →  + service code, std::error::Error
+```
+
+## Risk Assessment
+
+| Risk | Mitigation |
+|------|-----------|
+| Breaking existing consumers who use `OwnedMessage` | `OwnedMessage` was already behind a manual `#[cfg]` in the `no_std_test` example — consumers aren't relying on it unconditionally |
+| `BufMutBuilder` removal in no-alloc mode | It's only useful with `BytesMut` anyway, which requires alloc |
+| `RevBuilder::with()` change | Forward iteration is semantically equivalent for protobuf encoding |
+
+## Files to Modify
+
+| File | Changes |
+|------|---------|
+| `runtime/src/lib.rs` | Gate `RotoMessage`, `RotoOwned`, `BufMutBuilder`, `use bytes::BufMut` |
+| `runtime/Cargo.toml` | Wire `alloc` → `bytes` dependency, add `bytes/std` to `std` feature |
+| `codegen/src/generator/mod.rs` | Split `DATA_IMPORTS`, gate `OwnedMessage`, gate service code, fix `RevBuilder::with()` |
+| `examples/no_std_test/` | Update to use new feature structure |
+
+## Execution Order
+
+1. Phase 1 (runtime) — foundation
+2. Phase 2 (codegen data imports) — depends on runtime
+3. Phase 3 (codegen services) — independent
+4. Phase 4–5 (consumer setup + validation) — depends on all above