Development#2
Merged
Merged
Conversation
Opens the second kernel task. Per phase-a.md, Milestone A3 cannot start until A2 is Done, so T-002 is recorded as Draft: the task file, the phase task index, and the phase-a.md sub-task line are all in place so that the moment the T-001 PR merges and T-001 moves to Done, T-002 is ready to advance to Ready → In Progress without a planning round. Scope: replace the opaque CapObject(u64) placeholder from ADR-0014 with typed references (TaskHandle / EndpointHandle / NotificationHandle) and introduce per-type fixed-size-block arenas for Task, Endpoint, and Notification — same shape as the existing CapabilityTable. MemoryRegion is left to Phase B. Refs: ADR-0013, ADR-0014, ADR-0016 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the design for the first three kernel-object kinds Phase A needs: Task, Endpoint, Notification. Picks per-type fixed-size-block arenas with generation-tagged typed handles — the same shape as the already-audited CapabilityTable — over four weighed alternatives (shared enum arena, intrusive linked list, slab-with-retyping, shared arena with external type tag). Lifecycle is explicit destruction with an O(n) reachability check; reference counting is deferred to a later ADR if a real use-case earns it. CapObject transitions from opaque u64 to a typed enum paralleling CapKind; ADR-0014's outer API is unchanged, delivering on the migration reservation it made. Refs: ADR-0014, ADR-0001 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #1 merged T-001 (capability table foundation) into main. Transition the bookkeeping: * T-001 status: In Review → Done; review history records the merge. * Milestone A2 marked done (2026-04-21). * Phase-a task index reflects Done status. * current.md repoints: last-completed milestone is A2, last-completed task is T-001, active milestone is A3, active task is the T-002 draft. The first business review (A2 completion) is the next trigger. Refs: ADR-0013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First milestone retrospective. Five sections per the business-review master plan. What landed — A2 shipped the capability subsystem (cap::CapabilityTable, cap::CapHandle, cap::CapRights, cap::Capability, cap::CapError) in T-001; ADR-0014 and ADR-0015 were Accepted; ADR-0016 is Proposed for A3. What changed — the roadmap itself grew during A2 (ten phases, not nine, after Plan C landed; ADR reservations 0016+ shifted +1 after the AI-integration decision took 0015; WOSR-derived pattern notes inserted at A3/B2/C3). What we learned — unplanned ADR insertions ripple; reviews caught semantic bugs (cap_drop orphaning, CapRights reserved-bit smuggling) that the 29 kernel tests did not; encapsulation-by-default on placeholder types paid off; a two-round review cycle suggests a pre-commit cross-file consistency pass is worth adopting; zero-`unsafe` is achievable for pattern-based kernel subsystems. Adjustments — open a kernel-api-conventions standards doc; tighten task acceptance criteria to name semantic invariants; plan a seL4 or Hubris technical-analysis before Phase B; add a pre-commit diff-scan step; move ADR-0016 to Accepted before T-002 code lands. Next — A3 / T-002, first code+security review on T-002's implementation. Refs: ADR-0013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A2 closure + the A2 business review clear T-002's dependency. Promote:
* T-002: Draft → Ready; first acceptance criterion (ADR-0016
Accepted) checked off; review history amended.
* ADR-0016: Proposed → Accepted. The per-type fixed-size-block
arena design with generation-tagged typed handles is now the
contract T-002 implements against.
* current.md: active task Ready, both ADR-0014 and ADR-0016 listed
as Accepted, T-002 implementation may begin.
* phase-a.md + tasks/phase-a/README.md: T-002 status Ready.
The next commit opens the implementation: module skeleton under
`umbrix_kernel::obj` and the `CapObject` enum rewiring.
Refs: ADR-0013, ADR-0016
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the kernel-object subsystem per ADR-0016. `umbrix_kernel::obj` holds a generic bounded `Arena<T, N>` with generation-tagged `SlotId`s, plus three concrete kinds — `Task`, `Endpoint`, `Notification` — each with its own typed handle (`TaskHandle` / `EndpointHandle` / `NotificationHandle`), typed arena alias, and `create_<kind>` / `destroy_<kind>` / `get_<kind>` functions. The arena pattern is the same shape as `cap::CapabilityTable` from T-001, generalised so the audit cost of "does this storage keep its invariants?" is paid once. `CapObject` transitions from an opaque `u64` placeholder to a typed enum (`CapObject::Task(TaskHandle)` / `::Endpoint(_)` / `::Notification(_)`) paralleling `CapKind`. `Capability` loses its separate `kind` field — kind is now derived from the object's variant, so kind-and-object can never disagree by construction. `Capability::new` is now `(rights, object)` — callers that passed a redundant `CapKind` update accordingly. `CapabilityTable::references_object` implements the ADR-0016 reachability query: callers that hold references to live tables pass them when destroying a kernel object and reject destruction if any watcher reports a live reference. The obj-side destroy functions document that callers wire the check in at their call site; a future ADR will bundle it when the kernel owns a registry of tables. Lifecycle in v1 is explicit: destroy_<kind>(handle) frees the slot and bumps the generation; stale handles fail lookup. No reference counting; no heap; zero `unsafe`. Per-type capacities are 16 each (conservatively small; revisit when a real deployment asks). Host tests: 14 new (7 arena + 3 task + 1 endpoint + 2 notification + 1 reachability). Total 77 host tests (43 kernel + 34 test-hal), all green. `cargo host-clippy`, `cargo kernel-clippy`, `cargo fmt --all --check` clean. T-002 status → In Review; phase-a.md / current.md updated. Refs: ADR-0016, ADR-0014 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements the Phase A3 kernel-object subsystem and rewires capabilities to point to typed kernel objects instead of an opaque u64, using a shared generic arena pattern; also adds a reachability helper for destroy paths and advances roadmap, tasks, and ADR documentation for milestones A2 and A3. Sequence diagram for destroy_task with capability reachability checksequenceDiagram
actor Caller
participant TaskArena as TaskArena
participant CapTable as CapabilityTable
participant Obj as ObjModule
Caller->>CapTable: references_object(target: CapObject::Task(handle))
CapTable-->>Caller: bool has_ref
alt has_ref == true
Caller-->>Caller: abort destroy (ObjError::StillReachable)
else has_ref == false
Caller->>Obj: destroy_task(arena: TaskArena, handle: TaskHandle)
Obj->>TaskArena: free(handle.slot())
TaskArena-->>Obj: Task or None
Obj-->>Caller: Result(Task, ObjError::InvalidHandle)
end
Class diagram for updated capability and CapObject modelclassDiagram
class CapRights {
}
class CapKind {
<<enum>>
Task
Endpoint
Notification
MemoryRegion
}
class TaskHandle {
+index: u16
+generation: u32
}
class EndpointHandle {
+index: u16
+generation: u32
}
class NotificationHandle {
+index: u16
+generation: u32
}
class CapObject {
<<enum>>
+Task(handle: TaskHandle)
+Endpoint(handle: EndpointHandle)
+Notification(handle: NotificationHandle)
+kind() CapKind
}
class Capability {
-rights: CapRights
-object: CapObject
+new(rights: CapRights, object: CapObject) Capability
+kind() CapKind
+rights() CapRights
+object() CapObject
}
class CapHandle {
+index: u16
+generation: u32
}
class CapabilityTable {
+cap_copy(src: CapHandle, new_rights: CapRights) CapHandle~Result~
+cap_derive(src: CapHandle, new_rights: CapRights, new_object: CapObject) CapHandle~Result~
+cap_revoke(src: CapHandle) CapError~Result~
+cap_drop(src: CapHandle) CapError~Result~
+lookup(handle: CapHandle) Capability~Result~
+references_object(target: CapObject) bool
}
class CapError {
<<enum>>
WidenedRights
InsufficientRights
DerivationTooDeep
HasChildren
InvalidHandle
}
Capability o-- CapObject : object
Capability --> CapRights : rights
CapObject --> CapKind : derives
CapObject --> TaskHandle
CapObject --> EndpointHandle
CapObject --> NotificationHandle
CapabilityTable "*" o-- Capability : slots
CapabilityTable --> CapHandle
CapabilityTable --> CapError
CapabilityTable --> CapRights
Class diagram for kernel object subsystem and generic arenaclassDiagram
class SlotId {
-index: u16
-generation: u32
+index() u16
+generation() u32
+from_parts(index: u16, generation: u32) SlotId
}
class Slot_T_ {
-entry: Option~T~
-generation: u32
-next_free: Option~u16~
}
class Arena_T_N_ {
-slots: Slot_T_[N]
-free_head: Option~u16~
+new() Arena_T_N_
+allocate(value: T) Option~SlotId~
+free(id: SlotId) Option~T~
+get(id: SlotId) Option~T&~
+get_mut(id: SlotId) Option~T&~
+contains(id: SlotId) bool
}
class ObjError {
<<non_exhaustive enum>>
ArenaFull
InvalidHandle
StillReachable
}
class Task {
-id: u32
+new(id: u32) Task
+id() u32
}
class TaskHandle {
-slot: SlotId
+from_slot(slot: SlotId) TaskHandle
+slot() SlotId
+test_handle(index: u16, generation: u32) TaskHandle
}
class Endpoint {
-id: u32
+new(id: u32) Endpoint
+id() u32
}
class EndpointHandle {
-slot: SlotId
+from_slot(slot: SlotId) EndpointHandle
+slot() SlotId
+test_handle(index: u16, generation: u32) EndpointHandle
}
class Notification {
-word: u64
+new(word: u64) Notification
+word() u64
+set(bits: u64) void
+consume() u64
}
class NotificationHandle {
-slot: SlotId
+from_slot(slot: SlotId) NotificationHandle
+slot() SlotId
+test_handle(index: u16, generation: u32) NotificationHandle
}
class TaskArena {
<<type alias>>
= Arena_T_N_~Task, TASK_ARENA_CAPACITY~
}
class EndpointArena {
<<type alias>>
= Arena_T_N_~Endpoint, ENDPOINT_ARENA_CAPACITY~
}
class NotificationArena {
<<type alias>>
= Arena_T_N_~Notification, NOTIFICATION_ARENA_CAPACITY~
}
class ObjModule {
<<module>>
+TASK_ARENA_CAPACITY: usize
+ENDPOINT_ARENA_CAPACITY: usize
+NOTIFICATION_ARENA_CAPACITY: usize
+create_task(arena: TaskArena, task: Task) TaskHandle~Result~
+destroy_task(arena: TaskArena, handle: TaskHandle) Task~Result~
+get_task(arena: TaskArena, handle: TaskHandle) Task&~Option~
+create_endpoint(arena: EndpointArena, endpoint: Endpoint) EndpointHandle~Result~
+destroy_endpoint(arena: EndpointArena, handle: EndpointHandle) Endpoint~Result~
+get_endpoint(arena: EndpointArena, handle: EndpointHandle) Endpoint&~Option~
+create_notification(arena: NotificationArena, notification: Notification) NotificationHandle~Result~
+destroy_notification(arena: NotificationArena, handle: NotificationHandle) Notification~Result~
+get_notification(arena: NotificationArena, handle: NotificationHandle) Notification&~Option~
}
Arena_T_N_ o-- Slot_T_ : slots
Slot_T_ --> SlotId : generation uses
TaskHandle --> SlotId
EndpointHandle --> SlotId
NotificationHandle --> SlotId
TaskArena --> Arena_T_N_
EndpointArena --> Arena_T_N_
NotificationArena --> Arena_T_N_
ObjModule --> TaskArena
ObjModule --> EndpointArena
ObjModule --> NotificationArena
ObjModule --> TaskHandle
ObjModule --> EndpointHandle
ObjModule --> NotificationHandle
ObjModule --> ObjError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR marks milestone A2 (capability table foundation) as completed via business review documentation and advances to A3 (kernel object storage). It refactors Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- ADR-0016 describes
destroy_*operations performing the reachability check and returningObjError::StillReachable, but the current implementations leave reachability entirely to callers; either wireCapabilityTable::references_objectinto the destroy paths or update the ADR/docs to match the implemented semantics. - The
objmodule publicly re-exportsArenaandSlotId, which lets external code bypass the typed*Handlewrappers; if you want to enforce the handle abstraction, consider keepingArena/SlotIdprivate toobjand only exposing the typed create/get/destroy APIs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ADR-0016 describes `destroy_*` operations performing the reachability check and returning `ObjError::StillReachable`, but the current implementations leave reachability entirely to callers; either wire `CapabilityTable::references_object` into the destroy paths or update the ADR/docs to match the implemented semantics.
- The `obj` module publicly re-exports `Arena` and `SlotId`, which lets external code bypass the typed `*Handle` wrappers; if you want to enforce the handle abstraction, consider keeping `Arena`/`SlotId` private to `obj` and only exposing the typed create/get/destroy APIs.
## Individual Comments
### Comment 1
<location path="kernel/src/obj/arena.rs" line_range="123-138" />
<code_context>
+ /// that refers to the allocation until it is freed.
+ ///
+ /// Returns `None` when every slot is in use.
+ pub fn allocate(&mut self, value: T) -> Option<SlotId> {
+ let head = self.free_head?;
+ let slot = self.slots.get_mut(head as usize)?;
+ let next_free = slot.next_free;
+ slot.entry = Some(value);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Treat `free_head` pointing out of bounds as an internal invariant violation rather than a silent `None`.
`free_head` is maintained internally and `new()` guarantees any `Some(i)` satisfies `i < N`. If that invariant is violated, `get_mut(..)?` will just return `None`, making the arena look full instead of exposing the bug.
For kernel-internal code, consider asserting the invariant in debug builds:
```rust
let head = self.free_head?;
debug_assert!((head as usize) < N);
let slot = &mut self.slots[head as usize];
```
This preserves release behavior when the invariant holds, while surfacing logic errors during development.
```suggestion
/// Allocate a new slot, storing `value` in it. Returns a [`SlotId`]
/// that refers to the allocation until it is freed.
///
/// Returns `None` when every slot is in use.
pub fn allocate(&mut self, value: T) -> Option<SlotId> {
let head = self.free_head?;
debug_assert!((head as usize) < N);
let slot = &mut self.slots[head as usize];
let next_free = slot.next_free;
slot.entry = Some(value);
slot.next_free = None;
self.free_head = next_free;
Some(SlotId {
index: head,
generation: slot.generation,
})
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes Milestone A2 and initiates Milestone A3 by introducing the kernel-object subsystem. Key changes include the implementation of a generic, generation-tagged Arena for object storage and the definition of Task, Endpoint, and Notification kernel objects. The CapObject type has been transitioned from an opaque placeholder to a typed enum, and the capability system now supports reachability checks to prevent the destruction of objects still referenced by capabilities. Feedback focuses on potential efficiency improvements for object destruction, the manual nature of reachability checks in the current implementation, and the scalability of linear scans in the capability table.
Comment on lines
+140
to
+154
| /// Free the slot named by `id`, returning the stored value. | ||
| /// | ||
| /// Returns `None` if the id is stale (generation mismatch) or | ||
| /// points at an already-free slot. | ||
| pub fn free(&mut self, id: SlotId) -> Option<T> { | ||
| let slot = self.slots.get_mut(id.index as usize)?; | ||
| if slot.generation != id.generation { | ||
| return None; | ||
| } | ||
| let value = slot.entry.take()?; | ||
| slot.generation = slot.generation.wrapping_add(1); | ||
| slot.next_free = self.free_head; | ||
| self.free_head = Some(id.index); | ||
| Some(value) | ||
| } |
There was a problem hiding this comment.
The free method implementation is robust against double-free and stale handles. However, it returns Option<T> by value. While modern Rust compilers are efficient with moves, for larger kernel objects this might involve unnecessary copying. Consider if returning a reference or using a closure for cleanup would be more efficient as the kernel objects grow in complexity.
Comment on lines
+93
to
+95
| pub fn destroy_task(arena: &mut TaskArena, handle: TaskHandle) -> Result<Task, ObjError> { | ||
| arena.free(handle.slot()).ok_or(ObjError::InvalidHandle) | ||
| } |
There was a problem hiding this comment.
The destroy_task implementation currently lacks the reachability check described in ADR-0016. While the documentation acknowledges this as a v1 limitation, the ObjError::StillReachable variant is defined but unused here. To maintain consistency with the architectural decision, the caller must manually perform the check using CapabilityTable::references_object before calling this function, which is a potential source of bugs if forgotten.
Comment on lines
+453
to
+458
| pub fn references_object(&self, target: CapObject) -> bool { | ||
| self.slots | ||
| .iter() | ||
| .filter_map(|s| s.entry.as_ref()) | ||
| .any(|entry| entry.capability.object() == target) | ||
| } |
There was a problem hiding this comment.
The references_object method performs a linear scan of the capability table. While acceptable for the current CAP_TABLE_CAPACITY of 64, this will become a performance bottleneck if the table size increases significantly in future phases. Consider adding a per-object reference count or a more efficient lookup mechanism when scaling beyond Phase A.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
kernel/src/cap/table.rs (2)
750-778: Nice coverage of the empty/populated/post-drop states.
references_object_sees_live_caps_onlyexercises the three meaningful states (empty table, matching live cap, post-cap_drop). One small gap worth considering as a follow-up: a test thatcap_revokeon a parent also clearsreferences_object(child_target). Not required, but it would tiereferences_objectto the revocation path that actually drives the ADR-0016 destroy check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/cap/table.rs` around lines 750 - 778, Add a test to ensure revoking a parent capability clears references to its child target: in the same test module (near references_object_sees_live_caps_only) create a parent capability that names a child target (e.g., insert_root(Capability::new(..., parent_target)) where that parent implies or points to child_target), call t.cap_revoke(parent_handle).unwrap(), and then assert that t.references_object(child_target) is false; this ties references_object to the revocation path exercised by cap_revoke and verifies ADR-0016 behavior for parent->child revocation.
227-247: Docstring oncap_deriveis stale after the ADR-0016 rewire.The comment still says "v1 passes scope narrowing through the opaque
new_objectsince kernel objects do not yet carry typed scope." That’s no longer true —new_objectis now the typedCapObjectenum carryingTaskHandle/EndpointHandle/NotificationHandle. Worth rewording so future readers don’t chase a mismatch between docs and the actual type.📝 Suggested wording
- /// Install a child capability — one whose parent is `src`. Typically - /// used when narrowing the capability's *scope*; v1 passes scope - /// narrowing through the opaque `new_object` since kernel objects do - /// not yet carry typed scope. + /// Install a child capability — one whose parent is `src`. Typically + /// used when narrowing the capability's *scope*; `new_object` is a + /// typed [`CapObject`] naming the (possibly new) kernel object the + /// derived capability points at.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/cap/table.rs` around lines 227 - 247, Update the cap_derive docstring to remove the outdated “v1 passes scope narrowing through the opaque `new_object`” wording and instead state that `new_object` is the typed `CapObject` enum carrying specific handles (e.g. `TaskHandle`, `EndpointHandle`, `NotificationHandle`), and adjust the description to reflect that scope/typed information is now encoded in `CapObject`; edit the comment above the `pub fn cap_derive(&mut self, src: CapHandle, new_rights: CapRights, new_object: super::CapObject, ...)` signature accordingly so it no longer references the legacy v1 behavior.kernel/src/obj/mod.rs (1)
56-68:ObjError::StillReachablehas no in-crate producer.Worth calling out in the variant doc: unlike
ArenaFull/InvalidHandle,StillReachableis never returned by any function inkernel::obj; it is produced by callers after consultingCapabilityTable::references_object. The current wording ("Destruction was refused …") reads as if the destroy functions emit it, which isn’t the case today. A one-liner ("returned by callers that layer a reachability check on top ofdestroy_*") would prevent confusion while the kernel still lacks a table registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/obj/mod.rs` around lines 56 - 68, Update the doc for the ObjError::StillReachable variant to clarify that it is not produced by functions inside kernel::obj but is returned by callers that perform a reachability check on top of the destroy_* functions; mention CapabilityTable::references_object as the mechanism callers consult and use concise wording such as "returned by callers that layer a reachability check (via CapabilityTable::references_object) on top of destroy_*" so readers won't assume destroy_* emits this variant.kernel/src/obj/notification.rs (1)
140-146: Add a stale-handle-after-reallocation assertion.This test proves the freed handle is invalid immediately after destroy, but generation-tagged storage should also keep that stale handle invalid after another notification is allocated.
Test coverage improvement
fn destroy_invalidates_handle() { let mut arena = NotificationArena::default(); let handle = create_notification(&mut arena, Notification::new(0)).unwrap(); destroy_notification(&mut arena, handle).unwrap(); assert!(get_notification(&arena, handle).is_none()); + + let new_handle = create_notification(&mut arena, Notification::new(0b10)).unwrap(); + assert!(get_notification(&arena, handle).is_none()); + assert_eq!( + get_notification(&arena, new_handle).map(Notification::word), + Some(0b10) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/obj/notification.rs` around lines 140 - 146, Update the destroy_invalidates_handle test to also verify a freed handle remains invalid after a reallocation: after calling destroy_notification(&mut arena, handle), allocate a new notification via create_notification(&mut arena, Notification::new(...)) and assert the new handle is not equal to the old handle (or at least allocation occurred) and that get_notification(&arena, handle) still returns None for the stale handle; reference NotificationArena, create_notification, destroy_notification, get_notification and ensure the additional allocation happens before the final stale-handle assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/reviews/business-reviews/2026-04-21-A2-completion.md`:
- Line 29: Clarify whether ADR-0016 and T-002 statuses in the A2 snapshot are
point-in-time by either (a) adding a qualifier like "Snapshot as of 2026-04-21"
or "Status at time of review" near the ADR-0016/T-002 mentions, or (b) update
the ADR-0016 and T-002 status labels to match the rest of the PR; specifically
adjust the lines referencing "ADR-0016 — Kernel object storage (Proposed
2026-04-21)" and the T-002 status entries (and the same wording in the block
covering lines 70-77) so the document unambiguously indicates whether these
states are historical or current.
In `@docs/analysis/tasks/phase-a/T-001-capability-table-foundation.md`:
- Line 5: The task header T-001 currently shows a **Status: Done** but its
acceptance criteria and Definition of Done checkboxes remain unchecked; update
the document T-001-capability-table-foundation (task ID T-001) so the checklist
state matches the status by marking completed acceptance criteria/DoD checkboxes
as checked, or if those checkboxes are intentionally preserved as the original
baseline, add a short clarifying note below the status explaining that the
checklist is intentionally left unchecked for baseline/history purposes.
In `@docs/decisions/0016-kernel-object-storage.md`:
- Around line 139-143: The ADR's destroy_* sketch must be brought into alignment
with the implemented APIs: update the ADR entry for destroy_task (and
destroy_endpoint/destroy_notification if present) to show the actual signature
used in code—destroy_task(arena, handle) -> Result<Task, ObjError> (i.e., return
the freed value) and briefly note why the returned value is useful (e.g., allows
teardown/destructors for stored fields); alternatively, if you prefer the ADR's
original shape, state that a follow-up ADR will change the implementations and
leave a TODO reference—ensure the ADR references the concrete function names
destroy_task/destroy_endpoint/destroy_notification so readers can find the
implementations.
In `@docs/roadmap/current.md`:
- Around line 8-20: The doc incorrectly calls T-002 “shipped” while also saying
it is In Review; update the wording so T-002 is consistently described as
"implementation landed on development and awaiting maintainer PR review" (or
similar) instead of "shipped", change the sentence that currently reads "T-002
introduced obj::{Arena, Task, Endpoint, Notification}..." to indicate these
symbols were implemented on the development branch (not shipped), and adjust the
"Next review trigger" wording to reflect that the code+security review is the
current pending action for T-002 while business review still waits for A6;
ensure references to CapObject, Capability::new, and CapKind remain factual but
not described as shipped.
In `@kernel/src/lib.rs`:
- Around line 21-22: Update the rustdoc for the `obj` subsystem to fix the
awkward phrase "entities capabilities name" — change it to a clear relative
phrase such as "entities that capabilities name" or, better, "entities the
capabilities name" (e.g., replace the line mentioning `obj` — kernel-object
subsystem with wording like: "kernel-object subsystem (Phase A3 / [T-002]):
per-type arenas holding the concrete entities the capabilities name."). Ensure
the change targets the crate-level doc comment mentioning `obj` so the meaning
is unambiguous.
---
Nitpick comments:
In `@kernel/src/cap/table.rs`:
- Around line 750-778: Add a test to ensure revoking a parent capability clears
references to its child target: in the same test module (near
references_object_sees_live_caps_only) create a parent capability that names a
child target (e.g., insert_root(Capability::new(..., parent_target)) where that
parent implies or points to child_target), call
t.cap_revoke(parent_handle).unwrap(), and then assert that
t.references_object(child_target) is false; this ties references_object to the
revocation path exercised by cap_revoke and verifies ADR-0016 behavior for
parent->child revocation.
- Around line 227-247: Update the cap_derive docstring to remove the outdated
“v1 passes scope narrowing through the opaque `new_object`” wording and instead
state that `new_object` is the typed `CapObject` enum carrying specific handles
(e.g. `TaskHandle`, `EndpointHandle`, `NotificationHandle`), and adjust the
description to reflect that scope/typed information is now encoded in
`CapObject`; edit the comment above the `pub fn cap_derive(&mut self, src:
CapHandle, new_rights: CapRights, new_object: super::CapObject, ...)` signature
accordingly so it no longer references the legacy v1 behavior.
In `@kernel/src/obj/mod.rs`:
- Around line 56-68: Update the doc for the ObjError::StillReachable variant to
clarify that it is not produced by functions inside kernel::obj but is returned
by callers that perform a reachability check on top of the destroy_* functions;
mention CapabilityTable::references_object as the mechanism callers consult and
use concise wording such as "returned by callers that layer a reachability check
(via CapabilityTable::references_object) on top of destroy_*" so readers won't
assume destroy_* emits this variant.
In `@kernel/src/obj/notification.rs`:
- Around line 140-146: Update the destroy_invalidates_handle test to also verify
a freed handle remains invalid after a reallocation: after calling
destroy_notification(&mut arena, handle), allocate a new notification via
create_notification(&mut arena, Notification::new(...)) and assert the new
handle is not equal to the old handle (or at least allocation occurred) and that
get_notification(&arena, handle) still returns None for the stale handle;
reference NotificationArena, create_notification, destroy_notification,
get_notification and ensure the additional allocation happens before the final
stale-handle assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dedc8391-6859-4b7f-8d2e-9997c54b81f9
📒 Files selected for processing (17)
docs/analysis/reviews/business-reviews/2026-04-21-A2-completion.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/tasks/phase-a/README.mddocs/analysis/tasks/phase-a/T-001-capability-table-foundation.mddocs/analysis/tasks/phase-a/T-002-kernel-object-storage.mddocs/decisions/0016-kernel-object-storage.mddocs/decisions/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-a.mdkernel/src/cap/mod.rskernel/src/cap/table.rskernel/src/lib.rskernel/src/obj/arena.rskernel/src/obj/endpoint.rskernel/src/obj/mod.rskernel/src/obj/notification.rskernel/src/obj/task.rs
Comment on lines
+8
to
+20
| - **Active milestone:** A3 — Kernel objects. | ||
| - **Active task:** [T-002 — Kernel object storage foundation](../analysis/tasks/phase-a/T-002-kernel-object-storage.md) (status: **In Review** — implementation landed on `development`, awaiting maintainer PR review). | ||
| - **Working branch:** `development`. | ||
| - **Last completed milestone:** A1 — Bootable skeleton, on 2026-04-20 (commit `2944e7d`). | ||
| - **Last review:** none yet; the first business review will accompany A2 completion. | ||
| - **Next review trigger:** maintainer PR review → if merged, T-001 moves to `Done` and A2 milestone review follows. | ||
| - **Last completed milestone:** A2 — Capability table foundation, on 2026-04-21 (PR #1 merged to `main`). | ||
| - **Last completed task:** [T-001 — Capability table foundation](../analysis/tasks/phase-a/T-001-capability-table-foundation.md) — `Done` 2026-04-21. | ||
| - **Last review:** [A2 completion business review](../analysis/reviews/business-reviews/2026-04-21-A2-completion.md) — 2026-04-21. | ||
| - **Next review trigger:** code + security review of T-002's implementation when it reaches `In Review`; business review waits for A6 per [phase-a.md closure](phases/phase-a.md). | ||
|
|
||
| ## Notes | ||
|
|
||
| - T-001 implementation landed on `development`; see the task file for evidence and test coverage: [T-001](../analysis/tasks/phase-a/T-001-capability-table-foundation.md). | ||
| - [ADR-0014](../decisions/0014-capability-representation.md) Accepted; the kernel now exposes `cap::CapabilityTable`, `cap::CapHandle`, `cap::CapRights`, `cap::Capability`, and `cap::CapError`. | ||
| - The new module is not yet wired into `run`. | ||
| - The capability subsystem (T-001) and kernel-object subsystem (T-002) both shipped with zero `unsafe` and no heap. Neither is wired into `run` yet; that is Phase-A later-milestone work. | ||
| - [ADR-0014](../decisions/0014-capability-representation.md) and [ADR-0016](../decisions/0016-kernel-object-storage.md) both Accepted. | ||
| - T-002 introduced `obj::{Arena, Task, Endpoint, Notification}` with typed handles and rewired `CapObject` to a typed enum paralleling `CapKind`. `Capability::new` lost its redundant `kind` parameter (kind is now derived from the object's variant). All 77 host tests green. |
There was a problem hiding this comment.
Avoid calling T-002 “shipped” while it is still In Review.
Line 9 says T-002 is awaiting maintainer PR review, but Line 18 says the subsystem “shipped,” and Line 14 describes the review trigger as a future event even though the task has already reached In Review.
Proposed status wording cleanup
-- **Next review trigger:** code + security review of T-002's implementation when it reaches `In Review`; business review waits for A6 per [phase-a.md closure](phases/phase-a.md).
+- **Next review trigger:** code + security review of T-002's implementation now that it is `In Review`; business review waits for A6 per [phase-a.md closure](phases/phase-a.md).
-- The capability subsystem (T-001) and kernel-object subsystem (T-002) both shipped with zero `unsafe` and no heap. Neither is wired into `run` yet; that is Phase-A later-milestone work.
+- The capability subsystem (T-001) shipped with zero `unsafe` and no heap; the kernel-object subsystem (T-002) currently does the same in review. Neither is wired into `run` yet; that is Phase-A later-milestone work.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Active milestone:** A3 — Kernel objects. | |
| - **Active task:** [T-002 — Kernel object storage foundation](../analysis/tasks/phase-a/T-002-kernel-object-storage.md) (status: **In Review** — implementation landed on `development`, awaiting maintainer PR review). | |
| - **Working branch:** `development`. | |
| - **Last completed milestone:** A1 — Bootable skeleton, on 2026-04-20 (commit `2944e7d`). | |
| - **Last review:** none yet; the first business review will accompany A2 completion. | |
| - **Next review trigger:** maintainer PR review → if merged, T-001 moves to `Done` and A2 milestone review follows. | |
| - **Last completed milestone:** A2 — Capability table foundation, on 2026-04-21 (PR #1 merged to `main`). | |
| - **Last completed task:** [T-001 — Capability table foundation](../analysis/tasks/phase-a/T-001-capability-table-foundation.md) — `Done` 2026-04-21. | |
| - **Last review:** [A2 completion business review](../analysis/reviews/business-reviews/2026-04-21-A2-completion.md) — 2026-04-21. | |
| - **Next review trigger:** code + security review of T-002's implementation when it reaches `In Review`; business review waits for A6 per [phase-a.md closure](phases/phase-a.md). | |
| ## Notes | |
| - T-001 implementation landed on `development`; see the task file for evidence and test coverage: [T-001](../analysis/tasks/phase-a/T-001-capability-table-foundation.md). | |
| - [ADR-0014](../decisions/0014-capability-representation.md) Accepted; the kernel now exposes `cap::CapabilityTable`, `cap::CapHandle`, `cap::CapRights`, `cap::Capability`, and `cap::CapError`. | |
| - The new module is not yet wired into `run`. | |
| - The capability subsystem (T-001) and kernel-object subsystem (T-002) both shipped with zero `unsafe` and no heap. Neither is wired into `run` yet; that is Phase-A later-milestone work. | |
| - [ADR-0014](../decisions/0014-capability-representation.md) and [ADR-0016](../decisions/0016-kernel-object-storage.md) both Accepted. | |
| - T-002 introduced `obj::{Arena, Task, Endpoint, Notification}` with typed handles and rewired `CapObject` to a typed enum paralleling `CapKind`. `Capability::new` lost its redundant `kind` parameter (kind is now derived from the object's variant). All 77 host tests green. | |
| - **Active milestone:** A3 — Kernel objects. | |
| - **Active task:** [T-002 — Kernel object storage foundation](../analysis/tasks/phase-a/T-002-kernel-object-storage.md) (status: **In Review** — implementation landed on `development`, awaiting maintainer PR review). | |
| - **Working branch:** `development`. | |
| - **Last completed milestone:** A2 — Capability table foundation, on 2026-04-21 (PR `#1` merged to `main`). | |
| - **Last completed task:** [T-001 — Capability table foundation](../analysis/tasks/phase-a/T-001-capability-table-foundation.md) — `Done` 2026-04-21. | |
| - **Last review:** [A2 completion business review](../analysis/reviews/business-reviews/2026-04-21-A2-completion.md) — 2026-04-21. | |
| - **Next review trigger:** code + security review of T-002's implementation now that it is `In Review`; business review waits for A6 per [phase-a.md closure](phases/phase-a.md). | |
| ## Notes | |
| - The capability subsystem (T-001) shipped with zero `unsafe` and no heap; the kernel-object subsystem (T-002) currently does the same in review. Neither is wired into `run` yet; that is Phase-A later-milestone work. | |
| - [ADR-0014](../decisions/0014-capability-representation.md) and [ADR-0016](../decisions/0016-kernel-object-storage.md) both Accepted. | |
| - T-002 introduced `obj::{Arena, Task, Endpoint, Notification}` with typed handles and rewired `CapObject` to a typed enum paralleling `CapKind`. `Capability::new` lost its redundant `kind` parameter (kind is now derived from the object's variant). All 77 host tests green. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/roadmap/current.md` around lines 8 - 20, The doc incorrectly calls T-002
“shipped” while also saying it is In Review; update the wording so T-002 is
consistently described as "implementation landed on development and awaiting
maintainer PR review" (or similar) instead of "shipped", change the sentence
that currently reads "T-002 introduced obj::{Arena, Task, Endpoint,
Notification}..." to indicate these symbols were implemented on the development
branch (not shipped), and adjust the "Next review trigger" wording to reflect
that the code+security review is the current pending action for T-002 while
business review still waits for A6; ensure references to CapObject,
Capability::new, and CapKind remain factual but not described as shipped.
- T-001: mark all acceptance-criteria and DoD checkboxes Done. - A2 business review: add snapshot qualifier to ADR-0016 status line. - ADR-0016: correct destroy_* return type (Result<T, ObjError>, not Result<(), ObjError>) and restate reachability as caller-managed. - lib.rs: fix "entities capabilities name" → "entities that capabilities name" in the obj subsystem doc. - cap_derive docstring: remove stale "opaque new_object" wording; CapObject is now a typed enum, not an opaque placeholder. - ObjError::StillReachable: clarify it is returned by callers, not by the destroy_* functions themselves. - obj/mod.rs: remove unused Arena/SlotId re-export (sub-modules import the arena module directly; the re-export served no purpose). - arena.rs: add debug_assert on free_head bounds in allocate. - notification.rs: extend destroy_invalidates_handle to verify a stale handle stays invalid after slot reuse. - cap/table.rs: add cap_revoke_clears_references_object test, confirming that revocation removes the object reference visible to references_object. 44/44 host tests green; clippy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Merged
cemililik
added a commit
that referenced
this pull request
Apr 21, 2026
Integrate every follow-up from the 2026-04-21 Phase-A code and security reviews into phase-b.md. No review item remains in an ad-hoc ledger; each is mapped to a specific B0..B6 milestone, and every decision that must close during Phase B is 🚩-flagged in its milestone and collected under the plan's "Open questions" section. New milestone **B0 — Phase A exit hygiene** prepends the original EL-drop pipeline and absorbs: - Security-review blockers #1 / #2 / #3 — UNSAFE-2026-0012 raw-pointer refactor, cross-table revocation policy, scheduler deadlock panic → idle task + typed error — as ADR-0021 / ADR-0022 / ADR-0023. - Code-review follow-ups: architecture docs for kernel-objects / IPC / scheduler; missing tests (ReceiverTableFull, slot-reuse with pending transfer cap, returns-Err replacements for should-panic tests); const {assert!(N>0)} type-level invariants on SchedQueue and CapabilityTable; debug_assert → typed-error hardening. - Non-blocking tracking items, placed in their natural milestone: generation wrap (B2 flag), fault containment scope (B5 flag → Phase E for full supervisor), Capability::Debug redaction (B5 before console_write syscall), write_bytes TX timeout (B6 flag), QEMU-smoke CI gate (B6 flag), cargo-vet init (B6 flag), TaskArena local → global StaticCell migration (bundled with T-006), boot.s early DAIFSet (B1 BSP checklist), TPIDR_EL0 save-set watch (TLS trigger, no decision yet). ADR numbers shifted: the pre-review 0021..0026 (EL-drop, MMU, AS, loader, syscall ABI, initial syscalls) become 0024..0029 so the B0 blockers can claim 0021..0023. Final numbers are assigned when the ADR is actually written per ADR-0013. current.md updated: active phase B, active milestone B0, first task to open is T-006 (raw-pointer scheduler API refactor). Also: docs/analysis/temp/ added to .gitignore as a local-only transient workbook directory (follow-up ledgers, drafts, scratch notes that do not belong in git history). Refs: ADR-0013 Code-Review: docs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.md Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 22, 2026
Second-read of the six-commit 2026-04-23 retrospective arc (c25f8c8…5ba0d44) surfaced one real policy violation and several worth-fixing-now clarifications. Two of the reviewer's "Yüksek" items were false alarms on verification and needed no change. Accepted findings: 1. unsafe-policy §3 append-only violation. R1's commit c25f8c8 edited UNSAFE-2026-0011's original body in place to extend it to TaskStack::top, which the log's own "Entries are append-only" rule forbids. Fix: - Reverted UNSAFE-2026-0011's original body to its pre-R1 shape. - Appended a dated "Amendment (2026-04-23, commit TBD):" block with the top() location / operation / invariants / rejected alternatives — the same content R1 moved, now clearly tagged as a post-hoc extension rather than a rewrite. - Formalised the pattern in unsafe-log.md's header + unsafe- policy.md §3: status-change and dated Amendment blocks are the two permitted post-hoc update forms; in-place body edits remain a policy violation reviewers must reject. - Condensed the in-code SAFETY comment on TaskStack::top to reference the Amendment rather than duplicating its rationale, closing a drift surface the verbose form would have opened. 2. CI: cargo-llvm-cov reinstall every run. The coverage job did not cache ~/.cargo/bin, so `cargo install --locked cargo-llvm-cov` rebuilt the binary on every push (~1–2 min). Added ~/.cargo/bin to every job's cache paths and made the install step idempotent-by-comment. 3. CI: rolling nightly flake risk. Miri and coverage jobs pulled floating `nightly`, so a public-channel regression would break master with no diff to blame. Introduced NIGHTLY_PIN env var (`nightly-2026-01-15`) at the workflow top; both jobs use it. Cache keys include the pin so a bump invalidates the cache cleanly. docs/guides/ci.md "Nightly pinning" section rewritten with a concrete bump procedure. 4. docs/guides/ci.md: `continue-on-error: true` interacts surprisingly with branch-protection (GitHub renders neutral as non-passing, blocking merges when the job is required). Added a "Branch protection and continue-on-error" section spelling out which jobs to add to required-checks today (lint-and- host-test, kernel-build, miri) and which to leave out (coverage, until it flips to enforcing post-T-011). 5. T-011 AC #2 debug_assert mechanic. The original wording ("confirms the assertion's dev-mode behaviour") was underspecified; a test that triggers a debug_assert! panics, which needs explicit `#[should_panic]` + `#[cfg(debug_assertions)]` scaffolding. Replaced AC #2 with a paired-test spec: one should-panic test for the cap-drop case, one must-not-panic test for the no-cap case, both exercising `reset_if_stale_generation` by name. Protects the assert from rotting into either a blanket panic or a no-op. 6. T-011 references to T-009 softened. T-009 has a reserved slot in phase-b.md but no task file; references to it in T-011's Out-of-scope section now link to phase-b.md and note the "not yet opened" state explicitly. Rejected findings (verified, no change needed): - "`reset_if_stale_generation` doesn't exist, it's still `sync_generation`." It exists at ipc/mod.rs:211; the rename landed in 7eaa10a. - "`debug_assert!` wasn't added by 7eaa10a." It was; it's at ipc/mod.rs:215 (`git show 7eaa10a -- kernel/src/ipc/mod.rs` confirms). - "BSP source still has 'umbrix:' strings." `git ls-files | xargs grep -ni umbrix` returns empty; the rename in 5d7cc87 was complete. `cargo pkgid -p tyrne-bsp-qemu-virt` succeeds, so the CI `--exclude` argument is correct. Verification: 77 kernel + 34 test-hal = 111 host tests green; cargo fmt / host-clippy / kernel-clippy / kernel-build clean; YAML workflow parses cleanly. Refs: unsafe-policy §3, ADR-0021, R6 CI pipeline Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 27, 2026
…le SAFETY) Verified each finding; all five applied as suggested. Inline #1 — boot.md halt-loop syntax in Mermaid diagram + body text - Line 41 (Mermaid sequence diagram): `halt: wfe; b .-` → `halt_unsupported_el: wfe ; b halt_unsupported_el`. The named-label form matches the actual asm in `bsp-qemu-virt/src/boot.s` and removes the malformed `.-` token. - Line 16 (text description): `wfe; b .-` halt loop wording corrected in two places — the EL3 path now reads "named-label `wfe`-loop (`halt_unsupported_el: wfe ; b halt_unsupported_el`)" and the kernel_entry-return defensive halt now reads `wfe ; b 2b` (matching the local-label loop at the bottom of `_start`). Inline #2 — UNSAFE-2026-0017 GAS halt-loop syntax (Amendment) - §Operation said "halt via `wfe; b -1b`". `-1b` is not valid GAS syntax — `1b` is the back-reference to local label `1:`, but a leading `-` is meaningless there. Real asm uses `b halt_unsupported_el`. - §Rejected alternatives → "Halt on EL3 with a panic frame" said "`wfe; b .-` is the visible silence". `b .-` is a similar malformed token (`.` is the current address; `b .-` with no offset is not a valid branch target). - Both occurrences corrected via an Amendment block per unsafe-policy.md §3 (the entry was committed in f289d4d / T-013; in-place edits to a committed entry's body are forbidden). The behaviour the audit describes is unchanged; only the prose's asm rendering was wrong. Outside-diff #4 — bsp-qemu-virt/src/cpu.rs SAFETY blocks for CNTFRQ_EL0 (`new()`) + CNTVCT_EL0 (`now_ns()`) - Both blocks claimed "boot.s performs no EL transition" — true before T-013, stale after. Updated to reflect ADR-0024: `boot.s` drives an EL2 → EL1 transition when the firmware/emulator delivers at EL2, falls through at EL1, halts at EL3. The runtime EL-check via `tyrne_hal::cpu::current_el()` (audited under UNSAFE-2026-0018) is the checked invariant pinning `CurrentEL == 1` at this point. Both SAFETY paragraphs now name UNSAFE-2026-0017 (the boot.s sequence) explicitly, and document (a) why-unsafe-is- required, (b) invariants, (c) rejected alternatives per CLAUDE.md rule 2. Nitpick — UNSAFE-2026-0018 cfg-gating wording (Amendment) - §Invariants relied on → "Cfg-gating" said "user code reading `CurrentEL` would trap or yield `EL0` with no useful information". The "or yield `EL0`" alternative is wrong: per ARM ARM §D11.2 / §C5.2, `MRS x, CurrentEL` at EL0 is undefined — the system register is not accessible at EL0 and the read raises an Undefined Instruction exception (which becomes `SIGILL` on hosted Unix-like targets such as `aarch64-apple-darwin`). There is no fallback EL0 read. Corrected via Amendment block (same §3 reasoning as UNSAFE-2026-0017 above). Nitpick — T-008 task file:120 review-history release-note structure - Single dense paragraph split into structured bullets using `<br/>•` separators inside the table cell: New docs / Updated docs / Scope discipline / Verification / State transition. Same factual content, scannable. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel- build clean; host-test 143 / 143 green. Refs: ADR-0024 Audit: UNSAFE-2026-0015, UNSAFE-2026-0017, UNSAFE-2026-0018 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 27, 2026
Verified each open item from the second review pass. All applied; the deferred Q1 settled with a strict-§3 discipline call. Q1 (HCR_EL2 Amendment) — strict discipline picked over carve-out The first review-round (commit 9a8e312) added the HCR_EL2 literal- write rationale to UNSAFE-2026-0017's §Invariants relied on as an in-place body edit. The second review-round flagged this as asymmetric against V13/V14 (which went via Amendment) and offered two paths: (1) codify a same-PR-mutable carve-out in unsafe-policy.md §3, or (2) convert V7 to an Amendment. Picked Path 2 (strict). Reasoning, in detail: - The asymmetry within the PR is the actual problem; Path 1 does not fix it because V13/V14 went via Amendment despite being same- PR. The carve-out would need a sub-rule "additions in-place, corrections via Amendment" — a gray-zone rule that just opens more gray zones. - Precedent: UNSAFE-2026-0006's 2026-04-23 Amendment for the post- T-009 QemuVirtCpu struct shape is exactly this pattern — adding to an existing entry's content via Amendment. Strict §3 reading matches the precedent. - The §3 text "must not be rewritten once committed" is unambiguous when read literally. The strict reading is what the rule says. - "Merge boundary" as the locking event introduces a second ambiguity (PR merge ≠ commit, and not observable from inside the audit log). Strict introducing-commit boundary is auditable. Mechanics: removed the HCR_EL2 paragraph from §Invariants relied on (reverting to f289d4d's introducing-commit shape); added a new Amendment block at the end of UNSAFE-2026-0017 with the same content + a "Discipline note for future readers" paragraph documenting the introducing-commit boundary so the next PR does not stumble into the same trap. The existing 2026-04-27-second-review-round Amendment (GAS halt-loop syntax correction) stays as-is, now placed after the HCR_EL2 Amendment in chronological order (first review-round commit 9a8e312 → HCR_EL2; second review-round commit 39dd978 → GAS-syntax). Orta #1 — T-008 + T-013 task-file checkbox flips Same discipline T-011 received in 9a8e312, applied to T-008 and T-013. Every delivered AC + DoD item flipped to [x] with parenthetical delivery notes. T-013's two QEMU smoke items (AC "Tests — QEMU smoke at default config" / "QEMU smoke at -machine virtualization=on" + DoD's QEMU smoke line) are *intentionally* left at [ ] — settled item 8 from the second review pass: those gates are deferred to the maintainer / CI runner and the PR test plan checklists them explicitly. T-008's "Cross-references go both ways" DoD item is now [x] thanks to Düşük #3 below. Düşük #1 — boot.s:21 leading-comment typo Same wfe; b .- malformed token that V13 corrected via Amendment in UNSAFE-2026-0017 also appeared in bsp-qemu-virt/src/boot.s line 21 (the leading comment block, not the actual asm — the asm uses the named-label form correctly). In-source comment, not audit-log body; in-place fine. Replaced with the named-label loop description so the comment matches the asm. Düşük #2 — coverage rerun report follow-up note The 2026-04-27 coverage report's tables describe T-011's 761af95-tip state. The second review-round's drop_first_child_… test fix (in 9a8e312) bumped cap/table.rs regions from 97.46 % → 97.60 % (+0.14 pp), with a corresponding +0.04 pp on the workspace total (96.33 % → 96.37 %). The original tables are intentionally not rewritten — the report describes T-011's tip; the post-fix delta lives in a follow-up note appended to the report. Both AC gates (sched ≥ 90 %, workspace ≥ 96 %) remain comfortably met. Düşük #3 — bidirectional ADR ↔ architecture-doc cross-references T-008's DoD line 88 ("Cross-references go both ways") was deferred ("ADRs cited from architecture docs are the same ADRs whose §References sections cite the new architecture docs — or will cite, in their next rider"). Closed in this commit with five short §Revision notes riders pointing back at the architecture docs: - ADR-0010 (Timer trait) → architecture/hal.md Timer subsection. - ADR-0017 (IPC primitive set) → architecture/ipc.md. - ADR-0019 (Scheduler shape) → architecture/scheduler.md. - ADR-0020 (ContextSwitch + Cpu v2) → architecture/scheduler.md. - ADR-0022 (Idle task + typed Deadlock) → architecture/scheduler.md. ADR-0021 already had a direct architecture/scheduler.md citation (observed by the second review pass), so it does not need a rider. Each rider is one short paragraph dated 2026-04-27, framed "pointer to architecture doc" — same shape as ADR-0013's 2026-04-27 pointer-to-ADR-0025 rider. ADR-0019's rider also notes that its previously-open "Idle task" question was settled by ADR-0022. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel-build clean; host-test 143 / 143 green. Refs: ADR-0010, ADR-0017, ADR-0019, ADR-0020, ADR-0022, ADR-0024 Audit: UNSAFE-2026-0017 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
Two findings from the parallel approval review of f9c145c. One Orta (must-fix this round) + one Düşük (cheap to bundle). Orta #1 — B0 security review §8 + Verdict structural alignment with master plan The 8-axis structure followed master-plan §1..§7 names but §8 was named "Sign-off" rather than "Threat-model impact" (the master plan's defined axis 8). The verdict was folded into §8 instead of sitting in a separate ## Verdict section using the master plan's Approve / Changes requested / Escalate vocabulary. A reviewer walking the master plan template-by-template would not find their checklist. Substantive fix: - Rename ## 8. Sign-off → ## 8. Threat-model impact. - Answer the master plan's threat-model question explicitly. B0's arc does have two real threat-model shifts that the prior shape buried: • ADR-0024 promotes EL1-only execution from defense-in-depth to a structural property. Pre-T-013, the kernel relied on firmware/emulator delivering at EL1 with UNSAFE-2026-0016 as a last-line guard. Post-T-013, boot.s actively drives EL1 regardless of entry EL; UNSAFE-2026-0016 is the post-condition, not the only defence. Threat-model implication: the "we run at EL1" assumption is now an enforced invariant, not a hoped- for one. • ADR-0021 reshapes the aliasing model the kernel proves correctness against. Pre-T-006, &mut Scheduler<C> aliasing across context switches was latent UB (UNSAFE-2026-0012) relying on the optimiser. Post-T-006, the raw-pointer bridge plus momentary-&mut discipline is Stacked-Borrows-checked by Miri on every test run. Threat-model implication: memory- safety story shifted from "hope the compiler doesn't exploit our UB" to "checked invariant the test gate enforces." - Move two pre-existing items (cross-table revocation gap, generation overflow) under a "Threat-model items inherited from Phase A (unchanged severity)" sub-bullet of §8 — they are genuinely threat-model items, not sign-off bullets, so this is the right home for them post-rename. - Keep the two positive observations (Audit-log Amendment discipline; arch-docs as security multiplier) under §8 as "Positive observations about the review surface itself" — the review-surface framing makes them sub-bullets of §8 honestly, not orphaned sign-off prose. - Add separate ## Verdict section ending with **Approve.** keyword per master-plan vocabulary. Verdict prose preserved from the original §8 closing paragraph. Citation preservation: T-012:44 cites "B0 closure consolidated security review §8 recommendation ('Architecture docs are a security multiplier')". The §8 recommendation sub-bullet is preserved verbatim under the new heading; the citation continues to resolve. No T-012 edit needed. Düşük #1 — B0 retro Appendix sourcing line tightened The Appendix introduction read "The T-009 mini-retro's second- follow-up note pre-loaded six agenda items for this retro." More accurate: items 1, 2, 4, 5 originate in the second follow-up's substantive paragraphs; item 3 (ADR-0024 first-week rider rate) originates in the first follow-up; item 6 (stale-link CI check) was a routing decision in the second follow-up's Adjustments- status snapshot rather than a fresh preloading. Rephrased the introduction to acknowledge "two follow-up notes plus the original Adjustments section" with a per-item provenance parenthetical. The six rows themselves are unchanged; net delta ("six items in → four closed + two routed forward") still holds. Out of scope for this fix-round (per the approval review): - Düşük #2 (UNSAFE-2026-0006 §1/§8 placement) — reviewer noted this is a prompt-expectation mismatch, not a delivery gap; "note logged for completeness" was the verdict. Skipped. - Open questions — none from this approval review. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel-build / host-test (143 / 143) all green; doc-only changes confirmed via git diff --stat (29 lines net). Refs: ADR-0021, ADR-0024 Approval-Review: independent agent, 2026-04-27, f9c145c tip Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
cemililik
added a commit
that referenced
this pull request
Apr 29, 2026
…sure trio + exceptions.md idle prose drift Two parallel inputs land here as one cohesive doc-only commit: (A) Independent approval-review on commit 9678d6c (the B1 closure trio) returned Approve verdict with 1 Medium + 7 Low + 2 Nit findings. The Approve verdict means the originating commit's substantive conclusions stand; the findings are prose / numerical accuracy / framing improvements. (B) A separate inline-comment finding flagged that `docs/architecture/exceptions.md` §"Idle's wfi activation" still contained the pre-T-012 framing ("Until T-012's full set lands, idle stays in the spin_loop form") even though `idle_entry`'s body uses `cpu.wait_for_interrupt()` since commit `b4ed68c`. The fix here aligns the prose with the shipped code. (A) — review-trio fixes: `docs/analysis/reviews/business-reviews/2026-04-28-B1-closure.md`: - (M1) "Codify verification-pending status notes" Adjustments item reconciled with Appendix item #2 — the canonisation criterion is now consistently "the next IRQ-related arc reproduces the pattern", not "three same-arc entries". UNSAFE-2026-0019 / 0020 / 0021 share one arc and one shape; they are *one* cross-arc data point. The §"Audit-log Pending QEMU smoke verification status notes" Learning section, the Adjustments item, and the Appendix item now all carry the same threshold language. - (L6) Cost-of-arc commit-count breakdown corrected from "3 implementation + 1 design-first + 1 doc-sweep + 2 review-fix + 1 Done = 8 (claimed 9)" to a 9-commit categorization that actually sums correctly: 2 implementation (a043079, b4ed68c) + 1 design- first doc (c366200) + 1 doc-sweep (28c5ce9 — T-012 step 6+7) + 1 SHA-backfill (6d2f725) + 1 review-fix-prep (67ff867) + 2 PR- review-round (d820a88, 5c9cf06) + 1 Done-sweep (ffefb76) = 9. Commit `28c5ce9` is now classified as the doc-sweep role rather than double-counted as both step 6+7 and the documentation-sweep commit. - (N1) "Result: zero 'doc and code disagree' findings" softened to "only two diagram/prose mismatches caught — no structural 'doc disagrees with code' findings"; the original phrasing self- contradicted the immediately-following "Two diagram/prose corrections" paragraph. - (N2) Stale-link drift section qualified: "stale-link drift suppressed" → "stale-link drift suppressed for new T-012 changes", with explicit acknowledgement that pre-existing repo drift (umbrix → tyrne rename leftovers) is a separate concern outside PR #10's scope. New Adjustments item ("Repo-wide umbrix-to-phase-a.md → tyrne-to-phase-a.md link sweep") opens the separate `docs(refs):` commit that handles the pre-existing drift. `docs/analysis/reviews/security-reviews/2026-04-28-B1-closure.md`: - (L1) New §3 forward-flagged item: cancel_deadline ordering race window between Step 1 (MSR CNTV_CTL_EL0=0b10 source-mask) and Step 2 (gic.disable(TIMER_IRQ)). Symmetric to the arm_deadline CVAL+CTL race that the original review flagged; the original review missed this symmetric case. v1 still benign (no caller). - (L4) "Kernel crate is still zero-unsafe" claim corrected. The `grep -r "unsafe " kernel/src/` description was misleading — the raw-pointer scheduler bridge `unsafe fn` bodies contain 15 inner `unsafe { ... }` blocks (mandatory under `#![deny(unsafe_op_in_unsafe_fn)]`). The substantive sentence ("T-012 added zero kernel-crate unsafe blocks; the new surface is BSP-internal") is preserved; only the misleading claim about the absence of `unsafe { ... }` blocks is rewritten. - (L8) GIC ITARGETSR loop-bound math precise now: last byte_idx is 1020 (loop condition `byte_idx < irq_count` exits at 1024); max offset = 0x800 + 0x3FC = 0xBFC. The original "0xC00" was the one-past-end byte. Conclusion ("well within the 4 KiB window") unchanged. - Verdict block: "One forward-flagged item" → "Two forward-flagged items" reflecting (L1)'s new entry; the cross-references in the README index and the business retro's Adjustments are kept consistent. `docs/analysis/reviews/performance-optimization-reviews/` `2026-04-28-B1-closure.md`: - (L7) §Metric 1 decomposition table: `exceptions.rs::irq_entry + panic_entry ~400 bytes` → `~244 bytes (irq_entry 116 B + panic_entry 128 B per rust-objdump)`. Verified by disassembling the release ELF. - (L5) "Roughly two-thirds of the A6→B1 .text delta is T-012 alone" → "Roughly 58 % of the A6→B1 .text delta is T-013 + T-012 specifically" with explicit T-013 / T-012 / B0 split. The arithmetic now uses the corrected 244-byte exceptions.rs figure; total estimate sums to ~7,810 bytes (~2 % residual to LLVM optimization shifts; matches empirical +7,968 within rounding). - (L2) §Metric 6 IRQ delivery cost trampoline line-items corrected. The `mov sp, x0` typo (which would have corrupted SP) → `mov x0, sp` (correct: writes the frame pointer to x0 for `bl irq_entry`). Save phase enumeration now verified line-by-line against vectors.s:115-130 (15 instructions: sub sp + 10 stp + mrs elr + mrs spsr + 1 stp + mov x0,sp). Restore phase corrected from "16" to 15 (the original "11 ldp" double-counted the elr/spsr ldp pair). New approximate dispatch cost: 51 instructions + ~25 cycles of microcode (down from the original 52). The §Hotspot observation (~60 % trampoline) becomes ~59 % (30/51). `docs/analysis/reviews/security-reviews/README.md`: - Index row's verdict description updated to match the artifact's two forward-flagged items. Substantive verdicts unchanged: security Approve, performance Baseline-recorded, business B2-prep pivot all stand. The findings are consistency / accuracy / framing improvements, not corrections to the underlying conclusions. (B) — exceptions.md idle prose drift: `docs/architecture/exceptions.md` §"Idle's wfi activation" restructured. The original "from spin_loop → to WFI" historical comparison framed the WFI form as a future change ("Until T-012's full set lands, idle stays in the spin_loop form"), which was accurate when the doc was drafted design-first in commit `c366200` but is now stale — `b4ed68c` flipped idle_entry to WFI. The new prose: - Leads with "Idle's body uses WFI as the default wake strategy — the form ADR-0022's Decision outcome originally specified, now shipped" + the WFI-form code block. - Records the historical flip as past tense: "T-012's commit `b4ed68c` flipped `idle_entry`'s body from the interim core::hint::spin_loop() shape ... to the wait_for_interrupt() shape above. This closes ADR-0022 first rider's Sub-rider gate." - Names the wake source explicitly (timer IRQ; arm_deadline callers from B5+ time_sleep_until or B-late preemption tick). - Notes that v1's IPC demo path makes WFI structurally unreachable (both application tasks tail-`spin_loop`-yield permanently); the shipped change is observable as a code-shape change, not a behavioural change in the v1 trace. - Flags remaining T-012 items (maintainer-side QEMU smoke + Miri) as separate from this section's scope, with cross-link to the B1 closure retro's Adjustments and to the audit-log Pending-QEMU-smoke status notes. The spin_loop code block is removed because preserving an obsolete pre-shipping form serves no current reader; the historical transition is captured in the prose plus the cross-reference to commit `b4ed68c`. Audit-log entries (UNSAFE-2026-0019 / 0020 / 0021) and ADR-0021 §Revision notes Amendment intentionally not edited — the findings here are prose/numerical fixes in review and architecture artifacts, not in append-only audit or ADR bodies. Introducing- commit-boundary discipline preserved. Pre-existing N2 (`umbrix-to-phase-a.md` → `tyrne-to-phase-a.md` repo-wide rename) deliberately out of scope for this commit; will land in a separate `docs(refs):` commit so the scope is clear. Gates (no code changed in this commit; documentation only): cargo fmt / kernel-build / host-clippy / kernel-clippy clean; host-test 149 / 149 (25 hal + 90 kernel + 34 test-hal) — same as `9678d6c` post-trio. Refs: T-012 (Done 2026-04-28; commit b4ed68c flipped idle to WFI), ADR-0021 (2026-04-28 Amendment unchanged), ADR-0022 (first rider Sub-rider closure paragraph unchanged), ADR-0025 (§Rule 2 planned-vs-unplanned-Amendment distinction observation unchanged), UNSAFE-2026-0019 / 0020 / 0021 (Pending QEMU smoke verification status notes maintained, untouched). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 29, 2026
Approval review on PR #11 returned a punch-list: 5 inline comments + 1 nitpick + 3 PR-level comments + 2 overall comments. This commit applies 9 of the 10 actionable findings; 1 finding (security review §4 irq_entry "loop" claim) was verified false against the source and is rejected with explanation below. The 2 overall comments (pattern-deduplication, narrative-tightening) are noted as known follow-up concerns but are out-of-scope for this commit. Findings applied: (F1) `docs/analysis/reviews/performance-optimization-reviews/` `2026-04-28-B1-closure.md` — security cross-reference line: "one forward-flagged non-blocking item" → "two forward-flagged non-blocking items" with both names (`arm_deadline` CVAL+CTL race + `cancel_deadline` CTL+GIC race). Synchronises with the security review (which gained the second item in commit `a5d4f9b`). (F3) `docs/analysis/reviews/security-reviews/2026-04-28-B1-closure.md` §Verdict — "no Yüksek findings" → "no high-severity findings". Brings the B1 trio's verdict prose under CLAUDE.md §3 (English in repository) without disrupting the established conduct-approval-review SKILL template's Turkish severity-label vocabulary (which the user explicitly approved earlier as "established pattern"). Scope kept narrow: this commit translates only the B1 trio's instances; the B0 closure security review's index entry retains "Yüksek" as a pre-existing committed artifact (unsafe-policy.md §3 Amendment discipline applies to audit-log entries, not review artifacts, but a broader sweep is a separate scope question and not trigger-driven by this review round). (F4) `docs/analysis/reviews/security-reviews/README.md` line 36 — same Yüksek → high-severity translation in the index entry's Verdict cell. (F5) `docs/analysis/tasks/phase-b/T-012-exception-and-irq-` `infrastructure.md` Definition-of-done checklist line 82 — "Task status updated to `In Review`" → "Task status updated to `Done`". The Status field at line 5 was already `Done` from ffefb76; the DoD checklist line was a stale residual from the `In Review` promotion. Now consistent. (F6) `docs/roadmap/phases/phase-b.md` B1 milestone status block — "B1 closure review trio is pending" → "B1 closure review trio landed 2026-04-28 via PR #11" with deep-links to the three artefacts. Only QEMU smoke + Miri remain pending. The B1 milestone-level `Done` flip happens after the maintainer-side verification. (N1) `README.md` Status: line — split from a dense single-paragraph summary into a four-bullet status list (Phase A / Phase B B0 / Phase B B1 / Path forward) plus a final pointer to `docs/roadmap/current.md`. Each bullet is a concise summary; dates retained; "Status:" label retained. (P1) `docs/analysis/reviews/business-reviews/2026-04-28-B1-closure.md` §"Design-first arc" — math `19 + 2 + 2 = 23 total findings` → `21 total review items (19 substantive findings — 11 round-1 + 8 round-2 — plus 2 round-2 nitpicks; the round-1 nitpick disposition is folded into the 11 applied)`. The `19+2+2` arithmetic was wrong (sums to 23, but the breakdown didn't match either round's commit-message numbers). The new text gives a clear decomposition matching `d820a88` ("apply 11 of 12 findings") and `5c9cf06` ("8 findings + 2 nitpicks") commit messages. (P2) `docs/analysis/reviews/business-reviews/2026-04-28-B1-closure.md` §"File renames and stale-link drift" — paragraph that said the `umbrix → tyrne` rename would land in "a follow-up `docs(refs):` commit (the immediate next commit after this retro lands)" was stale: the rename DID land in commit `10e3351`, which is one of PR #11's four commits — the same PR that lands this retro. Updated to "the `docs(refs):` commit `10e3351` (one of the four commits in PR #11, the same PR that lands this retro)" with deep-link. The Adjustments item already correctly says ✅ Done; this paragraph now matches. (P3) Merge-commit-SHA back-fill across 3 documents. PR #10's merge into `main` is commit `7b42bbe` (verified via `git log --oneline 9a66e8b..origin/main`); the documents that said "merge commit SHA pending back-fill in a follow-up `docs(audits)`-style commit" are now updated to reference `7b42bbe` directly with a clickable PR #10 link. Affected files: - `docs/analysis/tasks/phase-b/T-012-exception-and-irq-` `infrastructure.md` (review-history Done promotion row). - `docs/analysis/reviews/security-reviews/` `2026-04-28-B1-closure.md` (Change frontmatter). - `docs/roadmap/current.md` (Last-completed-tasks line; also added merge-commit `9a66e8b` cross-reference for T-013/PR #9 to match PR #10's back-fill shape). The earlier framing was process-detail noise (`docs(audits)-` style follow-up commit) that made each document feel dated immediately after merge; the new framing names the merge commit directly and is evergreen. Finding rejected: (F2) Security review §4 "Kernel-mode discipline" — claim that `irq_entry` is "straight-line / loop-free / constant-time". The reviewer asserted this is incorrect because "irq_entry uses a loop to acknowledge pending IRQs via repeated GICC_IAR reads". Verified against the source ([`bsp-qemu-virt/src/exceptions.rs` `irq_entry`](docs/architecture/exceptions.md) lines ~145-193): `irq_entry`'s body is straight-line. There is no `loop`, no `while`, no `for`. The dispatch shape is: 1. `let gic = ...` (single static deref) 2. `let Some(irq) = gic.acknowledge() else { return };` (single GICC_IAR read; spurious → early return) 3. `if irq.0 == TIMER_IRQ_ID { ... return; }` (single dispatch arm; mask + EOI + return) 4. `panic!("unhandled IRQ")` (structurally unreachable in v1) `gic.acknowledge()` returns `Option<IrqNumber>` — a single IRQ ID, not an iterator. The function does NOT drain pending IRQs in a loop; the GICv2 architecture actually delivers one IRQ per acknowledge cycle. To process additional pending IRQs, the CPU re-takes the exception after `eret` (if more are pending and DAIF.I is unmasked), which goes through the trampoline + `irq_entry` again — but each `irq_entry` invocation handles exactly one IRQ. The "constant-time / straight-line" claim is therefore correct and remains in the security review unchanged. The reviewer's mental model may have come from a typical edge-triggered GIC handler that DOES loop on IAR until spurious — but Tyrne's choice (per [docs/architecture/ exceptions.md](docs/architecture/exceptions.md) §"Dispatch flow") is one-IRQ-per-entry; the documentation accurately reflects the implementation. Overall comments not addressed (noted as known concerns): - **`Pending QEMU smoke verification` pattern repetition.** The pattern appears in `current.md`, `phase-b.md`, the three review artefacts, and the T-012 task file. Factoring into a single canonical explanation (in `unsafe-policy.md` or a short standard) is the right shape; per the [B1 closure retro Adjustments item + Appendix #2](docs/analysis/reviews/business-reviews/2026-04-28-B1-closure.md), the canonisation trigger is "the next IRQ-related arc reproduces the pattern" — cross-arc reproducibility threshold. Not actionable in this commit. - **B1 narrative repetition across files.** Real concern but addressing it via cross-linking would substantially restructure multiple committed review artefacts (which are append-only-style per the project convention). Better handled at the next milestone retro's plan-diff section, when the retro can decide whether to pivot to a cross-link-heavy style going forward. Recorded here for visibility. Gates (no code changed; documentation only): cargo fmt / kernel-build / kernel-clippy / host-clippy clean; host-test 149 / 149 unchanged. Refs: PR #11 approval review (the conversation-driven punch-list); T-012 (Done 2026-04-28; merge commit `7b42bbe`); ADR-0021 (2026-04-28 Amendment unchanged); ADR-0022 (first rider Sub-rider closed, unchanged); CLAUDE.md §3 (English-in-repository rule applied to verdict prose). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 8, 2026
… 2026-05-08 + 2026-05-07 follow-ups Closes the 12 remaining items flagged by the 2026-05-08 multi-axis review's §Follow-up backlog (3 Track-2 Majors, 1 Track-3 Major fix already closed in `59c08e9`, 6 Track-3 / Track-2 Minors, 2 Track-4 Minors, 2 Track-2 Nits) plus the carry-forward 2026-05-07 Track-H NIT-1. **Track 2 Majors (forward-flagged → ADR-0027 / phase-b ledger riders):** - M1 (escape-hatch doc): ADR-0027 §Decision outcome (c) gains a bullet documenting `mem::forget` / `ManuallyDrop` / `let _ = ...` as deliberate-but-rare escape hatches, mirroring the `x86_64::structures::paging::MapperFlush` precedent. - M2 (MMU-instance binding): ADR-0027 §Decision outcome (c) gains a bullet noting `MapperFlush::flush(self, mmu: &impl Mmu)` accepts any `Mmu`; multi-`Mmu` deployments (B3+ per-task `AddressSpace`, Phase C multi-CPU) will need a stronger token type. Out of scope for v1. - M3 (ADR-0034 placeholder): ADR-0027 §Decision outcome adds an "ADR-0034 (kernel-image section permissions) placeholder" block alongside ADR-0033, and `phase-b.md` ADR ledger gains rows for both ADR-0033 and ADR-0034 with named-but-unallocated discipline. **Track 3 Minors (governance / wording polish):** - m1 + m2 (current.md L52): drop "Phase-2" prefix on "§Simulation table" (the table walks Steps 0–4, not a "Phase 2"); "Accept will be" → "Accept landed as" + actual commit SHA `bb0a6ba`. - m3 (commit-style.md PR-numbering rider): new §"PR-number references in committed artefacts" subsection naming the recurrence (PR #18 + PR #20 each had a one-commit PR-number fix-up) and codifying three acceptable disciplines (defer banner authoring; reference branch slug; or use commit SHA). - n1 (framing alignment): "first to apply Simulation forward" wording in current.md (banner + Active decisions row) and phase-b.md §B2 status block aligned to the precise "first non-recovery-primitive state-machine ADR drafted under §Simulation" phrasing — ADR-0032's Propose did land with a table; the prior framing was technically defensible only under a narrow reading of "retro-extracted". **Track 2 Nits (substance riders in ADR-0027):** - #4 (DSB ISH vs DSB NSH rationale): §Simulation gains a rationale paragraph after the table — `ISH` is forward-compatible with the eventual SMP boot, sub-microsecond cost on single-core, matches Linux aarch64 `arch/arm64/mm/proc.S` for the same reason. - #5 (TCR_EL1.AS wording tighten): line 59 reworded — `AS = 0` selects 8-bit ASID *size*, not "the ASID value is 0" (the value is `TTBR0_EL1.ASID = 0` and is what's "globally used in v1"). - #7 (line 17 §-citation precision) + Track-3 n1: "first non-recovery-primitive state-machine" framing now precise. - #8 (memory-management.md L88 page-table descriptor cosmetic): ASCII bit-field diagram redrawn to match L2 block-descriptor reality (OutputAddress[47:21], not [47:12]); explanatory note added for L1-block / L3-page variants. **Track 4 Minors (perf-harness.sh):** - #1 (Ctrl-C cleanup trap): new `cleanup_in_flight` shell function + `trap '...' EXIT INT TERM` that kills any in-flight QEMU + watchdog PIDs tracked in `CURRENT_CMD_PID` / `CURRENT_WATCHDOG_PID` shell globals. `run_with_timeout` updates the globals at every call so the trap addresses whichever pair is currently in flight; clears them at every clean exit so the trap is a no-op outside iterations. - #2 (p99 small-N reporting hygiene): generated baseline report Methodology section gains a "**Note on p99 at small `n`**" paragraph explaining that under nearest-rank `p99 == max` for `n < 100` and callers should not over-read it as a tail-latency signal until `n >= 100`. **Track 4 Nit #3 (read_stats refactor):** **Already closed by PR #21 review-round commit `ef30b5c`** — the 8 `echo | awk` parses became a single `while read` loop. Verified at HEAD (`grep -c "while read -r key val" tools/perf-harness.sh` → 1 hit). **2026-05-07 Track-H NIT-1 (Pending Amendment closure-path indexing):** UNSAFE-2026-0019 / 0020 / 0021 each gain a 2026-05-08 "closure-path indexed" Amendment naming the canonical clearance trigger (B5 Milestone, ADR-0030 entry-point, deadline-arming syscall) explicitly, so a future reader of `unsafe-log.md` alone has the full picture without leaving the file. No semantic change; co-locates information that was previously distributed across `phase-b.md` cross-references. Verification gates re-run on the integration branch: - `cargo fmt --all -- --check` clean - `cargo host-test` 159/159 (25 + 100 + 34) - `cargo host-clippy` clean (-D warnings) - `cargo kernel-clippy` clean - `cargo kernel-build` clean - `tools/perf-harness.sh --iterations=3` runs end-to-end with the new trap + Methodology note + while-read parsing intact This commit + the prior 2026-05-08 review's recommendations close all follow-ups identified by both 2026-05-07 and 2026-05-08 multi-axis reviews. Forward-flagged items (10/12/13 from 2026-05-07) and any review-round bot input on this integration branch remain the only open items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 14, 2026
Bot review-round on PR #29 (CodeRabbit + gemini-code-assist; sourcery rate-limited). All six findings are valid drift in the closure-trio docs + `current.md` and were verified against empirical per-module test counts via `cargo test -p tyrne-kernel --lib -- <module>:: --list` at HEAD `3ec94b0` and at B2 closure `b0035ce`. **Empirical truth — per-module test deltas (B2 closure → HEAD):** | Module | B2 | HEAD | Δ | |---|---|---|---| | `mm::pmm::tests` | 0 | 15 | +15 | | `mm::address_space::tests` | 0 | 20 | +20 | | `sched::tests` | 22 | 27 | +5 | | `obj::task::tests` | 3 | 4 | +1 | | **Total kernel-host** | **25** | **66** | **+41** | Categorised by origin: 15 PMM (T-017) + 22 T-018-commits-1-3 (18 AS + 3 sched + 1 task) + 4 review-round (2 AS + 2 sched) = 41. **Six findings applied:** 1. **CodeRabbit @ current.md:7 — "three review rounds" → "five".** The lead phrase contradicted the enumeration of round-1..round-3 + round-4-follow-on + round-5-follow-on in the same sentence. 2. **CodeRabbit @ current.md:55 + gemini @ current.md:55 — bootstrap AS root `0x40092000` → `0x4008d000`.** Cross-doc drift: the closure-trio artefacts all cite `0x4008d000` (live smoke trace at HEAD `6334881`); current.md L50 + L55 had the older value from commit `daee78f` (pre-PR-#28-merge; the address shifted because review-round commits altered `.text`/`.bss` layout, moving `__boot_pt_l0`). Both L50 (Active task bullet, daee78f-inherited) and L55 (Last completed tasks bullet, my closure-trio commit) now match the live value. 3. **gemini @ business-reviews/2026-05-14-B3-closure.md:65 — `sched::tests` (5 new) text vs 6-item list mismatch.** The list incorrectly attributed the `Task::address_space_handle_round_trips` test to `sched::tests`; it actually lives in `obj::task::tests`. Rewrote the per-test breakdown paragraph to list each module empirically (PMM +15, AS +20, sched +5, task +1) with sum = 41 and origin-category cross-check; added the missing `obj::task::tests (+1, 3 → 4)` bullet. 4. **gemini @ perf-reviews/2026-05-14-B3-closure.md:73 — per-crate categorisation inconsistent with business retro.** Perf used "+14 PMM + 22 AS + 5 review-round = 41"; business retro used different sub-totals; both differed from empirical reality (+15 PMM, +20 AS-module, +4 review-round). Rewrote perf's Test count section to match the business retro's per-module empirical layout + added an origin-category sum + cross-link to the business retro per-test breakdown. 5. **gemini @ current.md:7 — "5 from the review-round fix-ups" inconsistent with other reports.** The "5th" entry (cited as "the existing test-discriminant cleanup") was a *phantom*: the only existing-test modification in the review-round arc was the T-007 Deadlock test gaining an endpoint-state assertion (a *modified* test, not a *new* one). Rewrote L7's test breakdown as "18 AS + 3 sched + 1 task + 4 review-round = 26", which matches the PR #27 → PR #28 delta exactly. 6. **gemini @ current.md:55 — same bootstrap AS root drift as CodeRabbit #2 (consolidated above).** **Business retro audit-note paragraph updated** to record the correction trail: the earlier draft mis-counted PMM as +14 (actual +15) and the review-round arc as +5 or +6 (actual +4); the closure-trio re-read pass on 2026-05-14 corrected both. No drift between the headline (41) and the breakdown after the correction. Security review unchanged — it doesn't cite per-module test counts, only the workspace-wide 226 total which is correct. No code changes; docs-only sweep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 14, 2026
cemililik
added a commit
that referenced
this pull request
May 15, 2026
Address 4 findings from PR #31 review-round 1 (2 reviewer passes; R1: P2 + P3 + P2; R2: P1 + P2 + P3 + P3; overlap on stale dead-code allow + missing test names). **P1 (R2 F1) — `load_image` non-overlap precondition runtime-enforced.** The safe `load_image` API's `copy_nonoverlapping` non-overlap invariant was previously upheld at the BSP-wiring layer (`.rodata` ⊆ PMM-reserved range per ADR-0027 + ADR-0035); the type system did not enforce it. Fix: new `pub fn Pmm::could_yield_pa_overlapping(pa_range)` query + new §Simulation row 4 preflight in `load_image` (`image.as_ptr() as usize` → PA range under v1 identity-mapped kernel AS per ADR-0027) + new `LoadError::ImageOverlapsAllocatableMemory` variant. Per-call invariant now runtime-enforced; BSP-layout discipline remains the production reality but is no longer the load-bearing soundness argument. UNSAFE-2026-0027 gained a 2026-05-15 Amendment recording the change. **P2 (R1 F3 / R2 F2) — missing OutOfFrames rollback test.** Added `task_loader::tests::rolls_back_on_pmm_exhausted_mid_image_loop` driven by a new `#[cfg(test)] pub(crate) fn Pmm::force_alloc_failure_after` injection hook (cleaner than a Pmm decorator since `Pmm` is a concrete generic, not a trait). Pins the defensive `LoadError::OutOfFrames` rollback path that is structurally unreachable in v1 post-FrameBudget preflight but lives in the code for forward-concurrency scenarios. §Simulation table refreshed to actual test names: row 2 names the split `rejects_invalid_parent_cap_lookup` + `_wrong_kind`; row 5 names `returns_loaded_image_with_correct_metadata` (no `creates_fresh_as_via_cap_create_address_space` ever existed). **P3 (R1 F1 / R2 F3) — stale `#[allow(dead_code)]` on BOOTSTRAP_AS_CAP.** The cap is now read by T-019's BSP wiring as the loader's `parent_as_cap` (`main.rs:988`); attribute + comment removed and replaced with a "Live as of T-019" doc comment. **P3 (R1 F2) — placeholder byte sequence decoded wrong.** `[0x40, 0x00, 0x80, 0xd2, ...]` LE word 0 = `0xd2800040` = `MOVZ x0, #2` (sf=1 → 64-bit MOVZ), not the documented `mov w0, #42`. Corrected to `[0x40, 0x05, 0x80, 0x52, ...]` (LE word 0 = `0x52800540` = correct `MOVZ w0, #42`) in all 4 sites (BSP, ADR-0029 ×2, T-019 user-story ×2, architecture chapter). Placeholder is not executed in B4 so the prior encoding was non-functional, but documented intent now matches the bytes. **P3 (R2 F4) — `current.md` stale "Next task to open: T-019".** T-019 is already In Review per the same file's banner; replaced with B4-closure + B5 syscall-ABI ADR pair (ADR-0030 + ADR-0031) wording. Pipeline narrative reflows from 7 to 8 steps (new row 4 preflight); `LoadError` variant count from 7 to 8. `docs/architecture/task-loader.md` Mermaid diagram + pipeline text + test surface list updated; T-019 user-story §Simulation table + §Rollback contract + §Acceptance criteria LoadError list + Review history all updated. Tests at HEAD: **250/250** host-test count (+3 from 247: 2 overlap- preflight tests + 1 OutOfFrames rollback test). All gates clean: `cargo fmt --check`, `cargo host-test`, `cargo host-clippy -D warnings`, `cargo kernel-clippy -D warnings`, `cargo kernel-build`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
cemililik
added a commit
that referenced
this pull request
May 15, 2026
Six valid review nits surfaced after commit 95efd62; all addressed in this single follow-up commit. No production-code behaviour change — every fix is documentation, SAFETY-comment hygiene, or test-listing accuracy. **(1) bsp-qemu-virt/src/main.rs SAFETY-comment expansion.** The `load_image` invocation's `unsafe { ... }` block had a terse SAFETY comment that partially satisfied the CLAUDE.md non-negotiable #2 (a)/(b)/(c) discipline. Expanded to explicitly state (a) why unsafe is required (materialising `&mut`/`&` to write-once `StaticCell` payloads via `assume_init_*`), (b) the exact invariants — initialisation order across the five named statics + single-core cooperative + scope-limited momentary `&mut` not crossing the cooperative switch + ADR-0021's discipline + audit IDs UNSAFE-2026-0010 + UNSAFE-2026-0014, and (c) why safer alternatives were rejected (`Box<Mutex<T>>` requires a heap allocator v1 doesn't have; `OnceCell`/`LazyCell` cannot express the boot-flow ordering constraints; the `StaticCell` write-once pattern is the minimal `Sync` shape that matches the actual boot semantics). **(2) docs/analysis/tasks/phase-b/T-019-task-loader.md §Simulation table rows 6+7.** Pipes inside `\`USER|EXECUTE\`` and `USER | WRITE` were breaking the GFM table-column parser. Escaped both as `USER\|EXECUTE` and `USER \| WRITE`. **(3) docs/architecture/task-loader.md §Rollback contract test list.** Stale test name `rolls_back_on_misaligned_image_base_va` replaced with the renamed identifier `rejects_misaligned_image_base_va_with_pmm_byte_stable`; added a parenthetical clarifying that this is the "no-rollback-needed" leg of the contract (the row-1 alignment preflight catches the misalignment before any state change). **(4) docs/roadmap/current.md banner.** Resolved the `HEAD <TBD>` placeholder to `95efd62` (the round-4 follow-up commit). The 7-commit list now names every commit by SHA; subsequent on-branch commits (this polish + any future) are framed as "post-review doc/style polish" rather than as part of the substantive arc. **(5) kernel/src/obj/mod.rs module rustdoc.** Refreshed the stale module-level documentation: - Removed the "all v1 kernel-object code is safe Rust" claim (no longer true: `task_loader` introduces one audited `unsafe` block at the `copy_nonoverlapping` byte-copy site, covered by UNSAFE-2026-0027). - Added a §"Public surface (v1)" section listing the four exported families: Endpoint, Notification, Task, and the Task-loader API (`load_image` + `LoadedImage` + `LoadError`). - Pointed at ADR-0029 + phase-b §B4 §Revision-notes for the loader's scope boundary (LoadedImage descriptor, not a runnable task cap — runnability gates on B5/B6). **(6) kernel/src/mm/mod.rs `phys_frame_kernel_ptr` forward-compat example.** The doc-comment showed a future-ADR-0033 migration snippet using `.expect(...)`, which would lint-fail under the kernel crate's `#![deny(clippy::expect_used)]` discipline if a maintainer copy-pasted it. Rewrote the example with two lint-clean patterns: - Pattern A: `checked_add(...).unwrap_or_else(|| { debug_assert!(...); fallback })` — branches on overflow, no panic in release. - Pattern B: `saturating_add(...)` — matches the rest of the kernel's `clippy::arithmetic_side_effects` discipline. Both reference `KERNEL_PHYS_BASE` and ADR-0033 explicitly so the example is concrete. Tests at HEAD: **259/259** (no test changes — pure doc/style). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 15, 2026
Verified all 5 review findings against current code state; all valid. Minimal-change fixes: 4 doc-only, 1 visibility narrowing. **(1) T-019 user-story §Review-history line 134 — unresolved `<TBD>` placeholders.** The 2026-05-14 implementation-arc row carried two literal `<TBD>` tokens (the BSP-wiring commit hash and the smoke- trace HEAD reference) that subsequent review-round commits never back-filled. Resolved both to `ae31bc8` (the round-3 BSP-wiring commit). The accompanying narrative (test count, gate names, smoke line content) was the state at commit `ae31bc8` and stays as the historical record per the user-story file's append-only convention. **(2) ADR-0029 §"Build pipeline (B4 / T-019)" + §"Host-side loader tests" — in-place edit of an Accepted ADR violated append-only policy.** Commit `196d3fb` (T-019 commit 4) silently rewrote the placeholder-byte literal from `[0x40, 0x00, 0x80, 0xd2, ...]` (the Accept-state form) to `[0x40, 0x05, 0x80, 0x52, ...]` (the documented-intent `mov w0, #42; ret` form). Per CLAUDE.md non-neg #5 + the ADR README §Format, ADRs are append-only — corrections land in §Revision notes, not via body rewrites. Fix: - Reverted both byte literals (lines 44 + 78) to their Accept- state form. - Added a new §Revision notes section before §References with a 2026-05-16 entry recording the byte-encoding correction, decoding both literals explicitly (Accept-state bytes → `MOVZ x0, #2; RET`; corrected bytes → `MOVZ w0, #42; RET`), citing the canonical kernel-source bytes in `bsp-qemu-virt/src/main.rs::USERSPACE_IMAGE`, and noting the discrepancy was non-load-bearing because the placeholder is not executed in B4. The kernel-source bytes are unchanged (BSP `USERSPACE_IMAGE` is the production reality and stays at the corrected form). **(3) `kernel/src/obj/task_loader.rs` test-only `unsafe` `FakeMmu::create_address_space` call sites — brief SAFETY comments missing (a)/(b)/(c) discipline.** Four sites use the pattern: - `fixture` helper (the canonical one) at line 908 - `missing_derive_surfaces_via_address_space_creation_failed` test body - `FailingMapMmu::create_address_space` `unsafe fn` body - `fixture_with_failing_mmu` helper Fix: expanded the `fixture` helper's SAFETY comment with the full (a) why-unsafe-required / (b) invariants-upheld / (c) why-safer-alternative-rejected discipline. Cross-referenced the three peer sites by name with brief "same argument as fixture" notes, including the `unsafe fn` body which separately documents the `unsafe_op_in_unsafe_fn` lint requirement. **(4) `kernel/src/mm/pmm.rs` test `unsafe` blocks for `core::ptr::write_bytes` and `*returned_ptr.add(off)` — brief SAFETY comments missing (a)/(b)/(c).** Both blocks (the pre-poison fill + the per-byte zero-fill assertion) were one-liners covering only (b). Expanded both to the full (a)/(b)/(c) discipline, naming `aligned_backing`'s host-allocation contract and the per-byte loop's in-bounds reasoning. **(5) `kernel/src/mm/mod.rs::phys_frame_kernel_ptr` — visibility widened public API unnecessarily.** Helper is only called from inside the kernel crate (`task_loader::load_image`'s byte-copy site; verified via `grep -rn phys_frame_kernel_ptr`). Narrowed `pub` → `pub(crate)` to keep the kernel-internal shim out of the crate's public API surface; no external (bsp / hal / test-hal) caller exists. Tests at HEAD: **260/260** (no test changes; all fixes are doc/visibility). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable — full demo through `tyrne: all tasks complete`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 31, 2026
…xt) (#37) * docs(security): T-022 high-half migration security review — Approve Standalone, time-separated (2026-05-31) security pass over the T-022 / ADR-0033 high-half kernel migration, discharging the "awaiting explicit security review" flag current.md carried for UNSAFE-2026-0031. Verdict: Approve — eight axes pass (cryptography N/A), no live finding, no fix required. The migration is a structural kernel/user isolation *strengthening*: the kernel becomes absent from the TTBR0_EL1 regime an EL0 task runs on (removing the Meltdown substrate, replacing a standing per-descriptor AP invariant with structural absence). UNSAFE-2026-0031 + the 0022/0023/0024 T-022 Amendments are policy-conformant and second-reviewer-signed; the migration is smoke-verified fault-clean. The review-round-3 hardenings (fail-fast assert! range guards + regime-correct panic-handler UART alias) are verified sound and cannot misfire in v1 (incl. the no-double-panic property on the panic path). Forward-flagged (non-blocking, pre-B6): the three T-021 carry-forward gates (gate #1 — per-task console_write window + per-page user-VA->kernel-VA translation — is the single most important pre-B6 gate) and ADR-0034 (kernel-image section W^X; privileged-side gap, EL0-unreachable). Refs: T-022, ADR-0033 Security-Review: @cemililik (+ Claude Opus 4.8 (1M context) agent) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(adr): propose ADR-0037 — EL0 entry context Sibling ADR to ADR-0033 (which scoped itself to the migration and named the EL0 context a separate B6 task). Settles how an EL0 task's first entry is expressed and how it integrates with the cooperative scheduler. Decision (Option 1): reuse the existing Aarch64TaskContext + a one-shot enter_el0 ERET trampoline; carry (user_entry, user_sp) in the x19/x20 callee-saved slots the cooperative switch already restores; lr = enter_el0, sp = kernel stack. SP_EL1 (gate #2) is closed by construction — the task enters EL0 from its own kernel context, so a later +0x400 trap lands on that kernel stack. D2 settled: neither add EL0 fields to the kernel-object Task NOR a separate TaskContext struct — the 168-byte Aarch64TaskContext layout (and the security-critical context_switch_asm offsets) stay UNCHANGED, the exact drift the T-022 security review flagged. v1 simplification: SPSR_EL1 = 0x3C0 (EL0t, DAIF masked — cooperative, no preemption). Opens T-023 (Draft) in the same commit per ADR-0025 §Rule 1. Introduces UNSAFE-2026-0032 (the enter_el0 asm; security-sensitive → second-reviewer). Refs: ADR-0037, T-023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(adr): Accept ADR-0037 — EL0 entry context Careful re-read (write-adr §10) passed in a separate commit from Propose: forward-references grounded (T-023 exists; downstream consumers deliberately absent as consumers-not-dependencies, per ADR-0033's pattern), dependency chain complete, negative consequences carry real mitigations, the §Simulation table walks the EL0 first-entry + trap state machine. Refs: ADR-0037 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(task): T-023 — EL0 entry context (init_user_context + enter_el0 trampoline + per-task SP_EL1) Implements ADR-0037 (the B6 EL0 entry-context decision) — the dormant mechanism a userspace task needs to drop to EL0 on first dispatch. Closes T-021 carry-forward gate #2 (SP_EL1). No runnable EL0 task yet (that is the B6 wire-up, after gate #1 / gate #3); the mechanism is dormant and the QEMU trace is byte-stable. What lands: - HAL: `ContextSwitch::init_user_context(ctx, user_entry, user_sp, kernel_stack_top)` — additive sibling of `init_context`; its `# Safety` states the EL0-trap kernel-stack size contract (must hold the ~272-byte trap frame + handler call tree per +0x400, stronger than init_context's). - BSP: `QemuVirtCpu::init_user_context` seeds the *existing* Aarch64TaskContext (x19=user_entry, x20=user_sp, lr=enter_el0, sp=kernel_stack_top) — no struct/layout change, so context_switch_asm offsets are untouched (D2) — plus the `enter_el0` `#[unsafe(naked)]` ERET trampoline. - Scheduler: `Scheduler::add_user_task` (mirrors add_task, calls init_user_context; debug_asserts kernel_stack_top non-null + 16-aligned — gate #2 belt-and-braces). - test-hal: FakeContextSwitch::init_user_context + FakeTaskContext {is_user,user_sp}; host tests for the test-hal recorder and the scheduler route. All four ContextSwitch implementors updated (BSP + test-hal + scheduler's FakeCpu/ResetQueuesCpu). - Audit: new UNSAFE-2026-0032 (the enter_el0 ERET asm; second-reviewer-flagged). 2026-05-31 review-round (1 HIGH + 3 Medium + 4 nits) folded in BEFORE this commit: - HIGH — enter_el0 now SCRUBS the register file (x0–x30 + v0–v31) before ERET. The cooperative switch restores only callee-saved regs, so without the scrub the first EL0 instruction would observe kernel pointers/state in x0/x1/x8 (context ptrs / kernel stack), x30 (kernel code addr), and the caller-saved SIMD — a disclosure orthogonal to ADR-0033's AP/UXN memory isolation. Recorded as a load-bearing invariant (UNSAFE-2026-0032) and in ADR-0037 §Revision notes (CLAUDE.md #1). - SPSR_EL1 written via the register form (mov x8,#0x3c0; msr spsr_el1,x8) — the immediate MSR form is PSTATE-fields-only. - gate #2 reconciled (by-construction + the debug_assert); EL0-trap stack-size contract documented; test-scope claim narrowed (the host test pins the scheduler route via the fake — the real QemuVirtCpu slot write + the scrub are audit + kernel-build verified, the BSP being bare-metal). Docs synced: ADR-0037 §Revision notes (rider, append-only per ADR-0025 §Rule 2), T-023 (AC + review-history + row-to-verification mapping), hal.md + scheduler.md (ContextSwitch surface gains init_user_context / add_user_task), current.md (B6-opening banner + active-task/working-branch). Gates green: cargo fmt; host + kernel clippy -D warnings; 342 host tests (+2: test-hal init_user_context recorder, scheduler add_user_task route); kernel build (the 67-instruction scrub asm assembles); QEMU smoke byte-stable + fault-clean (mechanism dormant); cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt clean (0 UB). Security-relevant (the EL1→EL0 first-entry trust-boundary primitive); the ADR + task design review-round is folded in, the formal code security pass on UNSAFE-2026-0032 remains a DoD item before a real EL0 task is wired. Refs: T-023, ADR-0037, UNSAFE-2026-0032 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(task): T-023 review-round 2 — FPCR/FPSR/TPIDR scrub, offset_of! asserts, user_sp + AS-active guards Folds in the 2026-05-31 second review-round against the committed implementation (verdict: Approve-with-nits). 2 Low + 4 nits, all verified against the code and addressed. - Low-1 — enter_el0's scrub omitted the FP-control/status + EL0 thread-ID register classes. It zeroed x0–x30 + v0–v31 but NOT FPCR/FPSR/TPIDR_EL0/ TPIDRRO_EL0 — all EL0-readable, with architecturally-UNKNOWN reset values on real HW (Pi 4), so "clean, defined register file" was over-claimed off-QEMU (FPCR governs EL0 rounding/FZ/default-NaN; Linux zeroes FPCR/FPSR on exec). enter_el0 now also `msr {fpcr,fpsr,tpidr_el0,tpidrro_el0}, xzr`. No current secret leak (kernel does no FP arithmetic, never writes TPIDR), but the EL0 environment is now defined on real HW, not just QEMU's zeroed reset. - Low-2 — the implicit x19/x20/lr/sp hand-off was pinned only by size_of == 168; a layout-preserving field reorder (e.g. fp/lr swap) would silently corrupt both context_switch_asm and the enter_el0 hand-off. Added offset_of! asserts for all five offsets (0/80/88/96/104) — hardens the pre-existing context_switch_asm coupling too. - Nit — add_user_task now debug_asserts user_sp 16-byte alignment (it becomes SP_EL0; SCTLR_EL1.SA0 faults a misaligned EL0 stack), symmetric with the kernel-stack assert. - Nit — the AS-must-be-active obligation (TTBR0 installed + EPD0 cleared at first dispatch) is now stated on the caller-facing # Safety of both init_user_context and add_user_task (it previously lived only on enter_el0's doc, which the future task_create_from_image author would not read). - Nit (process honesty) — the review-round-1 rider shipped WITH bbc42e8, after ADR-0037's Accept (546febc): the careful-re-read missed the scrub HIGH; recorded in ADR-0037 §Revision notes per ADR-0025 §Rule 2. - Nit — "67-instruction scrub" wording dropped (it is 63 register-zeros + 4 special-register writes); docs reworded. Docs synced: UNSAFE-2026-0032 (scrub scope), ADR-0037 §Revision notes (second rider), T-023 (AC + review-history), hal.md, current.md. Gates green: cargo fmt; host + kernel clippy -D warnings; 342 host tests; kernel build (offset_of! asserts + the new MSRs assemble); QEMU smoke byte-stable + fault-clean (mechanism still dormant); cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt clean (0 UB). Refs: T-023, ADR-0037, UNSAFE-2026-0032 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(task): T-023 review-round 3 — fix non-existent SHA, init_count doc drift, T-023 status/ACs A multi-agent adversarial review of the whole T-023 arc (b6549d7..HEAD; 5 lenses × per-finding skeptic verification) found NO code/security/correctness/ asm defect — the register scrub (incl FPCR/FPSR/TPIDR), the offset_of! asserts, the SPSR register form, the dormancy + gate ordering, all four ContextSwitch implementors, and the host tests were verified correct. The only confirmed findings were three doc-only consistency defects, all fixed: - The T-022 security review (2026-05-31) cited ADR-0033's Accept as `db392c1`, which does not exist (transposed digit) — corrected to `db892c1`. - `FakeContextSwitch::init_count()`'s rustdoc said "init_context calls" but the shared counter is now also incremented by `init_user_context`; doc broadened to match (mirrors the already-updated `FakeTaskContext.initialized` field doc). - T-023 was still `Status: Draft` with all nine ACs unchecked despite being implemented + gates-green; moved to "In Review (… awaiting explicit security review …)" per the T-022 house convention and checked the nine satisfied ACs. Gates re-confirmed: cargo fmt, host clippy -D warnings, 342 host tests (kernel binary unchanged → the prior byte-stable QEMU smoke + Miri-0-UB still hold). Refs: T-023, ADR-0037 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(test-hal): T-023 review-round 4 — init_context clears prior user markers on slot reuse FakeContextSwitch::init_context (the kernel path) seeded initialized/ entry_addr/stack_top but left is_user / user_sp untouched, so a context slot re-seeded user-task -> kernel-task would report stale is_user=true / a stale user_sp. init_context now fully re-seeds a kernel context: clears is_user and user_sp. Adds a regression test (init_context_clears_prior_user_markers_on_reuse). Test-double-only correctness; no runtime/kernel effect. Skipped (with reason) from the same review: - "msr {fpcr,fpsr,tpidr_el0,tpidrro_el0}, xzr leaks SP" — INVALID. For MSR/MRS, Rt=31 is XZR (the X[]/ZR accessor), not SP (the 31=SP rule is for load/store base + SP-form arithmetic). Verified: kernel-build assembles these cleanly, and the shipped `msr ttbr0_el1, xzr` disassembles as `msr TTBR0_EL1, xzr` (d518201f) and is smoke-clean (TTBR0 -> 0, would fault if SP). No change. - "factor the enter_el0 scrub into a .irp/macro" — SKIPPED. The explicit per-register form is deliberately maximally auditable for a security-critical scrub (every scrubbed register visible); the register set is fixed; no defect. - "give init_user_context a default impl (unimplemented!/delegate)" — SKIPPED. A required trait method already fails loudest (compile error) if an implementor omits it; unimplemented! would defer that to a runtime panic (against the kernel's panic-free discipline + kernel-clippy denies panic), and delegating to init_context would silently produce a broken EL0 entry. Gates: cargo fmt; host clippy -D warnings; 343 host tests (+1); Miri 0 UB (kernel binary unchanged -> prior byte-stable QEMU smoke holds). Refs: T-023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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 by Sourcery
Introduce the kernel object subsystem with typed arenas and handles, and rewire capabilities to reference these concrete objects while updating roadmap and planning docs for milestones A2 and A3.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Refactor
Documentation