Development

docs/analysis/reviews/business-reviews/2026-04-21-A2-completion.md

@@ -0,0 +1,77 @@
+# Business review 2026-04-21 — A2 completion
+
+- **Trigger:** milestone-completion
+- **Scope:** Milestone A2 — Capability table foundation
+- **Period:** 2026-04-20 (roadmap-system establishment, commit `abe1b94`) → 2026-04-21 (T-001 landed on `main` via PR #1)
+- **Participants:** @cemililik (+ Claude Opus 4.7 agent as scribe)
+
+## What landed
+
+### Commits (reverse chronological, A2-relevant)
+
+| SHA | Date | Subject | Advances |
+|-----|------|---------|----------|
+| `75ca576` | 2026-04-21 | `docs(roadmap): T-001 → Done; A2 closed; advance current.md` | closes A2 |
+| `e937537` | 2026-04-21 | `docs(adr): propose ADR-0016 — Kernel object storage` | forward into A3 |
+| `a587761` | 2026-04-21 | `docs(roadmap): open T-002 — Kernel object storage foundation (Draft)` | forward into A3 |
+| `2e1d943` | 2026-04-21 | `docs: apply second-round PR review nits` | T-001 review round 2 |
+| `cd8511c` | 2026-04-21 | `fix(cap): apply PR review findings to the capability subsystem` | T-001 review round 1 (code) |
+| `d7ff460` | 2026-04-21 | `docs: apply PR review feedback, renumber reserved ADRs, add WOSR-derived patterns` | T-001 review round 1 (docs) + forward patterns |
+| `e58a235` | 2026-04-21 | `chore: gitignore local technical-analysis notes` | housekeeping |
+| `95db0f4` | 2026-04-20 | `docs(roadmap): AI integration stance (ADR-0015) + Phase J + supporting updates` | Plan C accepted |
+| `8fe59d0` | 2026-04-20 | `feat(kernel): implement capability table (T-001)` | T-001 main implementation |
+| `c574a12` | 2026-04-20 | `docs(adr): propose ADR-0014 — capability representation; T-001 → In Progress` | T-001 start |
+
+### ADRs
+
+- **ADR-0014 — Capability representation** (Accepted 2026-04-20). Index-based arena with generation-tagged handles, move-only `Capability`, narrowing-only rights.
+- **ADR-0015 — AI integration stance** (Accepted 2026-04-20). Kernel stays AI-neutral; AI features live opt-in in userspace per Plan C.
+- **ADR-0016 — Kernel object storage** (Proposed 2026-04-21 — snapshot; Accepted the same day). Per-type fixed-size-block arenas with typed handles, mirroring the capability-table shape.
+
+### Tasks reaching `Done`
+
+- **T-001 — Capability table foundation.** Shipped `cap::CapabilityTable`, `cap::CapHandle`, `cap::CapRights`, `cap::Capability`, `cap::CapError`. Zero `unsafe`, no heap, 29 host tests (14 rights, 15 table) green. `CapObject` placeholder encapsulated via `new`/`raw`.
+
+## What changed in the plan
+
+- **Roadmap + analysis system established** (commit `abe1b94`, predates this review period but relevant context): ten phases, per-phase task folders, four typed review folders each with a master plan. See [ADR-0013](../../../decisions/0013-roadmap-and-planning.md).
+- **Phase J added** alongside ADR-0015. The phase plan grew from nine phases to ten; [`phases/README.md`](../../../roadmap/phases/README.md) and [`roadmap/README.md`](../../../roadmap/README.md) were updated to match.
+- **ADR renumber cascade.** ADR-0015 was originally reserved for A3 "Kernel object storage" but was taken by the AI-integration decision that landed out of sequence. A3/A4/A5 reservations shifted +1 (A3 → 0016, A4 IPC → 0017, A4 Badge → 0018, A5 Scheduler → 0019, A5 Cpu v2 → 0020); Phase B–I reservations shifted +1 each. The contiguous reservation range is now 0012–0057.
+- **WOSR-derived pattern notes inserted** into three phases' sub-breakdowns:
+  - Phase A3 — fixed-size-block allocator per kernel-object kind (applied in ADR-0016).
+  - Phase B2 — typed `MapperFlush`-analog acknowledgement token on the `Mmu` trait.
+  - Phase C3 — closure-based `Cpu::without_interrupts` HAL primitive for IRQ-masked critical sections.
+- **T-002 opened** in `Draft` status for Milestone A3.
+
+## What we learned
+
+**Unplanned ADR insertions ripple through reserved numbers.** The ADR-0015 AI-integration decision landed between the original reservation pass and A3, forcing a +1 shift across Phase A–I. The fix was mechanical but touched 9 phase files. Future unplanned ADRs should either (a) take the *next* free number rather than displacing a reservation, or (b) accept the ripple up front. Phase-a.md already notes "Numbers may shift if unexpected decisions land in between" — that is the accurate mental model, and the ledger should be treated as intent, not a promise.
+
+**The WOSR analysis format earned its keep.** Reading Philipp Oppermann's *Writing an OS in Rust* produced three concrete patterns (typed flush tokens, `without_interrupts` primitive, fixed-size-block arenas) that were worth naming in advance of the code that will use them. ADR-0016 adopts one of those patterns (fixed-size-block arenas) directly. A similar study of seL4 or Hubris before Phase B is likely worth doing, again kept local via `.gitignore`.
+
+**Reviews caught semantic bugs that tests did not.** Two findings in T-001's review were not caught by the 29 kernel tests:
+- `cap_drop` of an interior node orphaned its children — a correctness bug the tests happened not to trigger because every `drop` test was against a leaf. Fixed with `CapError::HasChildren` plus a new test.
+- `CapRights::from_raw` accepted any `u32`, letting reserved bits smuggle themselves past subset checks. Fixed with a `KNOWN_BITS` mask.
+
+Both bugs were present under good test coverage of the *typed-error* paths. The lesson: typed-error coverage is not semantic-invariant coverage. Future tasks should enumerate *invariants to uphold* as explicit acceptance criteria, not just "operations return the documented errors".
+
+**Encapsulation-by-default saved us later.** The second review round flagged `CapObject(pub u64)` as too open even for a placeholder. Changing the field to private with `new`/`raw` accessors is trivial today and will be load-bearing when ADR-0016 replaces the placeholder with a typed enum — every construction site is already auditable. Same lesson applies to other placeholder types: keep fields private from day one.
+
+**Two review rounds despite "measured pace".** Round 1 addressed correctness; round 2 addressed cross-file consistency (Rust module-path style `::` hyphens vs. underscores, "Pi 4" vs. "Pi 5", "markdown" vs. "Markdown", MMU-capable board filtering for the RISC-V BSP, untagged code fence). A pre-commit self-review pass on the diff — specifically comparing wording across all changed files — would likely catch most of these. Worth adopting as part of the commit workflow.
+
+**Zero-`unsafe` target was achievable.** The capability table is 350 lines of safe Rust. The zero-`unsafe` goal from ADR-0014 held. This is a useful data point for ADR-0016: if the kernel-object arenas follow the same pattern, they should also be `unsafe`-free.
+
+## Adjustments
+
+- [ ] **Open a standards doc** — `docs/standards/kernel-api-conventions.md` — capturing the "encapsulation-by-default, typed non-exhaustive errors, `new`/`raw` accessor pattern" conventions the capability subsystem now embodies. Trigger: before T-002 implementation begins, so the same conventions are applied from the start. Execution: `propose-standard-change` skill.
+- [ ] **Tighten task acceptance criteria** to enumerate semantic invariants, not just error-return paths. Trigger: when T-002 moves from `Draft` to `Ready`, revisit its acceptance criteria against this pattern. Execution: inline edit during the T-002 transition.
+- [ ] **Repeat the WOSR-analysis exercise** for one of: seL4, Hubris, Theseus, NuttX. Trigger: before Phase B (userspace) starts — seL4 is the most relevant prior art for userspace / address-space / syscall design. Execution: local under `docs/analysis/technical-analysis/` (gitignored), with any extracted patterns surfaced into phase-b sub-breakdowns the same way WOSR was.
+- [ ] **Add a pre-commit diff-scan step** to the maintainer's workflow: before staging, scan every changed file for cross-file wording/style consistency. Does not need to block; a five-minute re-read. Execution: informal; may grow into a checklist in `CONTRIBUTING.md` if the value becomes clear.
+- [ ] **ADR-0016 → Accepted** before T-002 implementation code lands. Trigger: maintainer review of the Proposed ADR. Execution: status edit on [`ADR-0016`](../../../decisions/0016-kernel-object-storage.md) + a one-line commit.
+
+## Next
+
+- **Active phase:** A
+- **Active milestone:** A3 — Kernel objects
+- **Active task:** [T-002 — Kernel object storage foundation](../../tasks/phase-a/T-002-kernel-object-storage.md) (Draft → Ready after this review is committed, then → In Progress once ADR-0016 is Accepted)
+- **Next review trigger:** code + security review of the T-002 implementation when it reaches `In Review`; business review waits for A6 per [phase-a.md closure](../../../roadmap/phases/phase-a.md).

docs/analysis/reviews/business-reviews/README.md

@@ -22,8 +22,6 @@ A business review may point at outcomes from those other reviews as part of "wha
 
 ## Index
 
-_No reviews yet._ The first business review will accompany the completion of Milestone A2.
-
 | Date | Scope | File |
 |------|-------|------|
-| _pending_ | Milestone A2 | — |
+| 2026-04-21 | Milestone A2 — Capability table foundation | [2026-04-21-A2-completion.md](2026-04-21-A2-completion.md) |

docs/analysis/tasks/phase-a/README.md

@@ -6,6 +6,7 @@ Tasks belonging to [Phase A — Kernel core on QEMU `virt`](../../../roadmap/pha
 
 | ID | Title | Milestone | Status |
 |----|-------|-----------|--------|
-| [T-001](T-001-capability-table-foundation.md) | Capability table foundation | A2 | In Review |
+| [T-001](T-001-capability-table-foundation.md) | Capability table foundation | A2 | Done |
+| [T-002](T-002-kernel-object-storage.md) | Kernel object storage foundation | A3 | In Review |
 
 Tasks are added here as they become active. See [`../../../roadmap/phases/phase-a.md`](../../../roadmap/phases/phase-a.md) for the full phase plan.

docs/analysis/tasks/phase-a/T-001-capability-table-foundation.md

@@ -2,7 +2,7 @@
 
 - **Phase:** A
 - **Milestone:** A2 — Capability table foundation
-- **Status:** In Review
+- **Status:** Done
 - **Created:** 2026-04-20
 - **Author:** @cemililik
 - **Dependencies:** none (Milestone A1 — Bootable skeleton — is complete)
@@ -25,23 +25,23 @@ This task deliberately stops short of introducing the kernel objects capabilitie
 
 ## Acceptance criteria
 
-- [ ] **ADR-0014 Accepted.** Defines: in-kernel capability representation (struct layout, rights bits, object-reference encoding), handle type exposed to callers, derivation-tree storage (intrusive vs. index-based), per-task bound on table size, and the error type for operations.
-- [ ] **`CapabilityTable` type** in a new `umbrix_kernel::cap` module. Bounded capacity (compile-time or per-instance), no heap allocation.
-- [ ] **`Capability` type** (enum or struct with a kind field) covering the v1 placeholder variants. Concrete object references are placeholders until Milestone A3 replaces them — the point is the *table's* correctness, not the objects'.
-- [ ] **Rights** (`CapRights` or similar) represented as a bitfield with the operations exposed so far: duplicate, derive, revoke, transfer-on-IPC (placeholder — no IPC yet).
-- [ ] **Handle-based access.** Callers receive a `CapHandle` (opaque index); raw capability bits are never exposed.
-- [ ] **Four operations** implemented:
+- [x] **ADR-0014 Accepted.** Defines: in-kernel capability representation (struct layout, rights bits, object-reference encoding), handle type exposed to callers, derivation-tree storage (intrusive vs. index-based), per-task bound on table size, and the error type for operations.
+- [x] **`CapabilityTable` type** in a new `umbrix_kernel::cap` module. Bounded capacity (compile-time or per-instance), no heap allocation.
+- [x] **`Capability` type** (enum or struct with a kind field) covering the v1 placeholder variants. Concrete object references are placeholders until Milestone A3 replaces them — the point is the *table's* correctness, not the objects'.
+- [x] **Rights** (`CapRights` or similar) represented as a bitfield with the operations exposed so far: duplicate, derive, revoke, transfer-on-IPC (placeholder — no IPC yet).
+- [x] **Handle-based access.** Callers receive a `CapHandle` (opaque index); raw capability bits are never exposed.
+- [x] **Four operations** implemented:
   - `cap_copy(src, narrower_rights) -> Result<CapHandle, CapError>` — install a peer in the caller's table with the same or narrower rights.
   - `cap_derive(src, narrower_scope) -> Result<CapHandle, CapError>` — install a child capability whose scope is strictly narrower; record the parent-child relationship.
   - `cap_revoke(src) -> Result<(), CapError>` — invalidate the derivation subtree rooted at `src`.
   - `cap_drop(handle) -> Result<(), CapError>` — release a capability from the caller's table with no effect on others.
-- [ ] **Move-only discipline.** The `Capability` type must not be `Copy` or `Clone`. Duplication is strictly through `cap_copy` or an explicit in-kernel duplication operation; the Rust type system enforces this.
-- [ ] **Rights narrowing invariant.** `cap_copy` and `cap_derive` cannot broaden rights; a test demonstrates that attempting to widen returns an error.
-- [ ] **Revocation cascade.** A test constructs a derivation tree of depth ≥ 3 and verifies that revoking a parent invalidates all descendants atomically.
-- [ ] **Bounded state.** A test fills the capability table to capacity and confirms the next insert returns `CapError::CapsExhausted` rather than panicking or allocating. See [architectural-principles.md — bounded kernel state](../../../standards/architectural-principles.md) and [security-model.md — Bounded kernel resources](../../../architecture/security-model.md).
-- [ ] **Documentation:** new rustdoc on every public item; no `missing_docs` warnings.
-- [ ] **Tests:** unit tests in the kernel module (using `#[cfg(test)]`), plus any integration tests that need `test-hal` fakes.
-- [ ] **No new `unsafe`** if achievable; if any is required, audit-log entry per [`unsafe-policy.md`](../../../standards/unsafe-policy.md).
+- [x] **Move-only discipline.** The `Capability` type must not be `Copy` or `Clone`. Duplication is strictly through `cap_copy` or an explicit in-kernel duplication operation; the Rust type system enforces this.
+- [x] **Rights narrowing invariant.** `cap_copy` and `cap_derive` cannot broaden rights; a test demonstrates that attempting to widen returns an error.
+- [x] **Revocation cascade.** A test constructs a derivation tree of depth ≥ 3 and verifies that revoking a parent invalidates all descendants atomically.
+- [x] **Bounded state.** A test fills the capability table to capacity and confirms the next insert returns `CapError::CapsExhausted` rather than panicking or allocating. See [architectural-principles.md — bounded kernel state](../../../standards/architectural-principles.md) and [security-model.md — Bounded kernel resources](../../../architecture/security-model.md).
+- [x] **Documentation:** new rustdoc on every public item; no `missing_docs` warnings.
+- [x] **Tests:** unit tests in the kernel module (using `#[cfg(test)]`), plus any integration tests that need `test-hal` fakes.
+- [x] **No new `unsafe`** if achievable; if any is required, audit-log entry per [`unsafe-policy.md`](../../../standards/unsafe-policy.md).
 
 ## Out of scope
 
@@ -73,14 +73,14 @@ Every step keeps `cargo host-test` green.
 
 ## Definition of done
 
-- [ ] `cargo fmt --all -- --check` clean.
-- [ ] `cargo host-clippy -- -D warnings` clean.
-- [ ] `cargo kernel-clippy` clean (the kernel builds for aarch64 with the new code).
-- [ ] `cargo host-test` passes with the new tests; coverage-of-contract is readable from the test names.
-- [ ] Any new `unsafe` has an audit entry per [`unsafe-policy.md`](../../../standards/unsafe-policy.md). Ideally none.
-- [ ] Commit(s) follow [`commit-style.md`](../../../standards/commit-style.md). At minimum: ADR-0014 as one commit, implementation as one commit. Trailers `Refs: ADR-0014, ADR-0001`.
-- [ ] [`../../../roadmap/current.md`](../../../roadmap/current.md) updated on status transitions (to `In Progress`, then `In Review`, then `Done`).
-- [ ] Milestone A2 business review written after this task is Done, per [`conduct-review`](../../../../.claude/skills/conduct-review/SKILL.md).
+- [x] `cargo fmt --all -- --check` clean.
+- [x] `cargo host-clippy -- -D warnings` clean.
+- [x] `cargo kernel-clippy` clean (the kernel builds for aarch64 with the new code).
+- [x] `cargo host-test` passes with the new tests; coverage-of-contract is readable from the test names.
+- [x] Any new `unsafe` has an audit entry per [`unsafe-policy.md`](../../../standards/unsafe-policy.md). Ideally none.
+- [x] Commit(s) follow [`commit-style.md`](../../../standards/commit-style.md). At minimum: ADR-0014 as one commit, implementation as one commit. Trailers `Refs: ADR-0014, ADR-0001`.
+- [x] [`../../../roadmap/current.md`](../../../roadmap/current.md) updated on status transitions (to `In Progress`, then `In Review`, then `Done`).
+- [x] Milestone A2 business review written after this task is Done, per [`conduct-review`](../../../../.claude/skills/conduct-review/SKILL.md).
 
 ## Design notes
 
@@ -105,3 +105,5 @@ Every step keeps `cargo host-test` green.
 | 2026-04-20 | @cemililik | opened; status Ready |
 | 2026-04-20 | @cemililik | ADR-0014 Accepted; status → In Progress; work begins on `development` branch |
 | 2026-04-20 | @cemililik | implementation landed on `development`; status → In Review. 27 new host tests green on top of the existing 34 (61/61 total). |
+| 2026-04-21 | @cemililik | review-round code/doc fixes landed; CapRights masks reserved bits, `cap_drop` rejects interior nodes with `HasChildren`, `CapObject` encapsulated (`new`/`raw`), BFS uses `debug_assert` for invariants, `cap_derive` cleaned up; two new host tests added (29 kernel + 34 test-hal = 63/63 green). |
+| 2026-04-21 | @cemililik | PR #1 merged to `main`; status → Done. |