Skip to content

Code-side polish — close kernel/HAL/BSP non-blockers from comprehensive review (Track A/B/F/G/I)#15

Merged
cemililik merged 3 commits into
mainfrom
code-polish-sweep
May 7, 2026
Merged

Code-side polish — close kernel/HAL/BSP non-blockers from comprehensive review (Track A/B/F/G/I)#15
cemililik merged 3 commits into
mainfrom
code-polish-sweep

Conversation

@cemililik
Copy link
Copy Markdown
Collaborator

@cemililik cemililik commented May 7, 2026

Summary

This is the γ PR in the post-B1 fix-arc plan (α = PR #13 merged; β = PR #14 merged; γ = code-side small polish; then B1 closure trio; then B2 prep + δ).

Closes the localised tightening items from the 2026-05-06 comprehensive code review across Tracks A (Kernel), B (HAL), F (Tests), G (BSP), and I (Cross-track integration). All Phase A non-blockers that "still apply" + the new B0/B1-era non-blockers — kept as small surface, no architectural changes.

Commits

  1. d86746a chore(kernel,hal) — kernel + HAL polish (7 files, +40/-4):

    • cap/table.rs CAP_TABLE_CAPACITY <= Index::MAX migrated to inline const { assert!(...) } form (matches Arena::new's style); cap_revoke's descendants-buffer size proof made explicit as a multi-line comment above the BFS scratch allocation.
    • sched/mod.rs SchedQueue::new gains const { assert!(N > 0, "SchedQueue requires N > 0") } so a SchedQueue<0> instantiation fails build instead of behaving as a zero-capacity queue at runtime; # Panics (compile-time) doc-comment paragraph added.
    • hal/src/context_switch.rs ContextSwitch trait gains : Send + Sync super-trait bound (alignment with the other four HAL traits — closes the seam Track B flagged as "currently masked because all consumers also bound C: Cpu").
    • kernel/src/lib.rs denylist comment now explicitly says "these layer onto [workspace.lints]; do NOT re-state workspace denies here" — discipline-locking comment.
    • kernel/src/obj/{task,endpoint,notification}.rs test-module #[allow(...)] blocks aligned to the trio unwrap_used + expect_used + panic (matches the other seven test modules in the kernel; forward-protection for the next test author).
  2. 03606a6 chore(bsp) — BSP polish (4 files, +40/-1):

    • bsp-qemu-virt/src/cpu.rs Aarch64TaskContext gains a compile-time size guard const _: () = assert!(size_of::<Aarch64TaskContext>() == 168) mirroring TrapFrame's PR Development #10 — T-012 (B1 IRQ infrastructure) + B0 closure retros #10 R2 guard.
    • bsp-qemu-virt/src/exceptions.rs irq_entry's _frame parameter gains a one-paragraph justification comment (future arcs will read it; underscore quiets the lint until then); the spurious-branch compiler_fence gains a defence-in-depth comment.
    • bsp-qemu-virt/src/vectors.s IRQ trampoline gains a comment block pointing at exceptions.rs:77's TrapFrame size assertion (asm/Rust frame-layout contract paired in the maintainer's mental model).
    • bsp-qemu-virt/src/main.rs adds a 14-line comment block at kernel_entry's tail recording why Track G's "defensive parking loop" suggestion is declined for this site: start is -> ! so the type system already proves no fall-through; adding a loop fails clippy::unreachable_code + clippy::too_many_lines; the suggested guard's only protection (against start dropping -> !) is itself a structural refactor that becomes a build error in callers.

Verification

Check Result
cargo fmt --all -- --check
cargo host-clippy ✓ (-D warnings)
cargo kernel-clippy ✓ (-D warnings)
cargo host-test 152/152 (unchanged from B1 closure baseline; no test logic changed)
cargo +nightly miri test 152/152 clean
cargo kernel-build
QEMU smoke full demo trace + tyrne: all tasks complete; boot-to-end ~5.8 ms; -d int,unimp,guest_errors empty

Out of scope (deferred, with reasons)

The comprehensive review's verdict §"Non-blocking follow-ups" listed several items that need ADRs or are larger than the PR γ scope:

  • IpcError::WrongObjectKind / MissingRight split — needs ADR; per phase-b.md ADR-0030 (B5 syscall-ABI design venue) is the natural home.
  • ipc_recv_and_yield Deadlock endpoint rollback / ipc_cancel_recv — needs ADR; queued for δ (post-B1-closure, pre-B2).
  • IrqQuard/IrqState(pub usize) BSP-only contract narrowing — needs Track B follow-up; can be a small ADR-0008 amendment.
  • FakeCpu + FakeTaskContext lift to tyrne-test-hal — needs an ADR-0020 follow-up + arena refactor; medium-size.
  • 11 perf proposals (P1–P11) — each gets its own performance-review cycle; P3 already complete (see comprehensive review §Verdict).
  • QEMU smoke as CI regression gate — needs a CI job; B2-or-later roadmap task.
  • ObjError::StillReachable no-producer decision (Track F §F-2) — needs testing.md extension or variant deletion; small but needs maintainer call.
  • proptest/bolero setup for CapabilityTable derivation-tree invariants — B2-or-later.
  • 2024 edition + unsafe extern "C" block consistency — workspace toolchain bump; separate PR.
  • #![deny(clippy::todo)] migration to [workspace.lints.clippy] (Track J §J-OBS1) — minor lint hygiene.
  • docs/decisions/0023-...md placeholder file (Deferred) — δ work.

Test plan

  • Reviewer pulls the branch and runs cargo fmt --check, cargo host-clippy, cargo kernel-clippy, cargo host-test — all expected clean (152/152 host tests).
  • Reviewer runs cargo +nightly miri test — expected 152/152 clean.
  • Reviewer runs ./tools/run-qemu.sh — expected full demo trace through tyrne: all tasks complete.
  • Reviewer reads each commit's body for the non-applied "Track G defensive loop" rejection rationale and confirms the type-system-vs-belt-and-braces argument.
  • Reviewer confirms each const { assert!(...) } migration / addition fails build (with a clear message) under contrived violations (e.g. set CAP_TABLE_CAPACITY = u32::MAX as usize + 1 or instantiate SchedQueue::<0>::new() in a test). (Recommended but not required; the assertions are mechanically obvious from the diff.)

🤖 Generated with Claude Code

Summary by Sourcery

Tighten kernel, HAL, and BSP safety and invariants based on the 2026-05-06 comprehensive review non-blockers, without changing architecture or behavior.

Enhancements:

  • Enforce compile-time capacity and layout invariants for scheduler queues, capability tables, and AArch64 task context to fail misconfigurations at build time.
  • Align HAL context-switch trait with Send/Sync requirements and clarify kernel lint layering and test-only lint exceptions.
  • Document IRQ entry, spurious interrupt handling fences, TrapFrame/TrapContext layout coupling, and the rationale for omitting a defensive loop in BSP kernel entry.

Summary by CodeRabbit

  • Refactor

    • Removed a redundant post-start defensive loop and clarified unreachable control flow.
  • Tests

    • Relaxed test-module lint allowances to permit additional diagnostic patterns.
  • Chores

    • Added compile-time assertions enforcing exact sizes/capacities and improved inline documentation describing layout and ABI contracts.

cemililik and others added 2 commits May 7, 2026 09:09
…A/B/F)

Closes the kernel/HAL non-blocker items from the 2026-05-06
comprehensive code review. Each is a localised tightening that the
reviewers identified as "still applies, low-priority" — none changes
runtime behaviour; build / test / miri all remain green.

## kernel/src/cap/table.rs (Track A non-blocker + obs)

- `CAP_TABLE_CAPACITY <= Index::MAX` migrated from `const _: () =
  assert!(...)` to inline `const { assert!(...) }` form. Matches the
  shape used in `Arena::new` ([obj/arena.rs:90](kernel/src/obj/arena.rs))
  and applied during PR #10 R2 to other call sites; the const-block
  form is the modern preferred shape per Phase A code review §Style.
- `cap_revoke`'s descendants-buffer size proof made explicit. The
  invariant ("each live capability slot has exactly one parent and
  appears in at most one parent's `first_child`/`next_sibling`
  chain, so the BFS visits at most `CAP_TABLE_CAPACITY` distinct
  indices") was implicit in the `cap_copy` / `cap_derive` /
  `free_slot` invariants; Phase A code review asked for a one-liner
  inside the function body. Comment added immediately above the
  `descendants` allocation so a reader does not need to chase the
  three sibling functions to reconstruct the bound.

## kernel/src/sched/mod.rs (Track A non-blocker)

- `SchedQueue::new` gains `const { assert!(N > 0, "SchedQueue
  requires N > 0") }`. The wrap arithmetic in `enqueue` / `dequeue`
  was already correct under N>0; Phase A code review noted N=0 was
  *behaviourally* a zero-capacity queue but the type-level invariant
  was unstated. Now a build-time hard error if any caller tries
  `SchedQueue<0>`. `# Panics (compile-time)` doc-comment paragraph
  added.

## hal/src/context_switch.rs (Track B non-blocker)

- `ContextSwitch` trait gains `: Send + Sync` super-trait bound.
  Every other HAL trait (`Cpu`, `Console`, `Timer`, `IrqController`)
  carries the bound; this was the only outlier. Comprehensive
  review flagged it as "currently masked because all consumers also
  bound `C: Cpu`, but a future `<C: ContextSwitch>`-only callsite
  would silently lose multi-core safety". Aligning the bound now
  closes the seam without observable behaviour change at HEAD.

## kernel/src/lib.rs (Track A observation)

- The kernel-stricter denylist's leading comment now explicitly says
  "these layer onto `[workspace.lints]`; do NOT re-state workspace
  denies here". Tracks the discipline Track A flagged as
  "harmless-but-double-stated" in Phase A; the intersection at HEAD
  is already correct (kernel adds `clippy::panic` / `unwrap_used` /
  `expect_used` / `todo` / `arithmetic_side_effects` /
  `float_arithmetic` over the workspace's `pedantic` + safety-block
  set, no overlap), and the comment locks the discipline so a future
  PR does not silently re-introduce duplication.

## kernel/src/obj/{task,endpoint,notification}.rs (Track F §F-3)

Three `#[cfg(test)] mod tests` blocks each had an `#[allow(...)]`
listing only `clippy::unwrap_used`. The other seven kernel-test
modules carry the trio `unwrap_used + expect_used + panic`. Aligned
all three to the trio. No new test logic; the existing tests in
these three modules don't currently use `expect` or `panic`, so the
addition is forward-protection (next test author can use the trio
without round-tripping a separate fix).

## Verification

`cargo fmt --check` clean; `cargo host-clippy` clean; `cargo
kernel-clippy` clean; `cargo host-test` 25 + 93 + 34 = **152/152**
(unchanged from B1 closure baseline); `cargo +nightly miri test`
**152/152** clean; `cargo kernel-build` clean.

Refs: comprehensive-review-2026-05-06, Track A, Track B, Track F

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the BSP-side non-blocker items from the 2026-05-06
comprehensive code review. Each is a localised hardening that
matches the discipline already applied to sibling sites.

## bsp-qemu-virt/src/cpu.rs (Track I §Non-blocking #3)

- `Aarch64TaskContext` gains a compile-time size guard:
  `const _: () = assert!(core::mem::size_of::<Aarch64TaskContext>()
  == 168);`. PR #10 R2 added the same guard to `TrapFrame` (192
  bytes) but did not back-fill the older `Aarch64TaskContext`
  site. The asm `naked_asm!` body in `context_switch_asm` reads
  byte offsets `0`/`80`/`88`/`96`/`104`; a Rust-side `repr(C)`
  drift would corrupt every cooperative switch. The guard turns
  that drift into a build-time hard error. Comment block above the
  assertion mirrors `TrapFrame`'s discipline.

## bsp-qemu-virt/src/exceptions.rs (Track G non-blockers)

- `irq_entry`'s unused `_frame` parameter gains a one-paragraph
  doc-comment justifying its presence: future arcs (preemption,
  ESR/ELR-aware diagnostics, the scheduler-wake hook UNSAFE-2026-0014's
  Amendment named) will read or modify the frame. The leading
  underscore quiets the unused-parameter lint while the parameter
  remains live in the signature for the trampoline's calling-
  convention contract.
- `irq_entry`'s spurious-IRQ branch's `compiler_fence(Ordering::SeqCst)`
  gains a defence-in-depth comment. The fence is structurally
  redundant on the early-return path (no following memory access
  reorders ahead of the return); kept as a guard against a future
  refactor that adds shared-state writes between spurious-detect
  and return. Documenting it as defence-in-depth makes the intent
  clear and pre-empts a future "drop the dead fence" PR that would
  remove the safeguard.

## bsp-qemu-virt/src/vectors.s (Track G non-blocker)

- The IRQ trampoline gains a comment block pointing at the
  `TrapFrame` size assertion in `exceptions.rs:77`. The 192-byte
  stack carve-out and `stp` byte offsets are tightly coupled to
  the Rust-side `#[repr(C)]` struct definition; pairing the asm
  side with an explicit cross-reference to the Rust-side compile-
  time guard keeps both halves of the contract in the maintainer's
  mental model.

## bsp-qemu-virt/src/main.rs (Track G non-blocker — declined)

- Track G's "append a defensive `loop { spin_loop }` after `start()`"
  recommendation is **declined** for `kernel_entry` specifically:
  `start` is `-> !`, so the type system already proves any code after
  the call is unreachable. Adding a parking loop fails build under
  `clippy::unreachable_code` and `clippy::too_many_lines`; the only
  "refactor regression" the loop would protect against is
  `start` losing its `-> !` qualifier — which itself becomes a
  compile error in every caller's return-type analysis. A 14-line
  comment block above the `start(...)` call records this rationale
  so a future reader does not re-litigate the suggestion.

## Verification

`cargo fmt --check`, `cargo host-clippy`, `cargo kernel-clippy`,
`cargo host-test` (152/152), `cargo +nightly miri test` (152/152),
`cargo kernel-build` all clean. **QEMU smoke** reproduces the full
demo trace through `tyrne: all tasks complete` (boot-to-end ~5.8 ms;
`-d int,unimp,guest_errors` empty for the entire run):

```text
tyrne: hello from kernel_main
tyrne: timer ready (62500000 Hz, resolution 16 ns)
tyrne: starting cooperative scheduler
tyrne: task B — waiting for IPC
tyrne: task A -- sending IPC
tyrne: task B — received IPC (label=0xaaaa); replying
tyrne: task A — received reply (label=0xbbbb); done
tyrne: all tasks complete
tyrne: boot-to-end elapsed = 5768000 ns
```

Refs: comprehensive-review-2026-05-06, Track G, Track I

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented May 7, 2026

Reviewer's Guide

Polishes kernel, HAL, and BSP code by tightening compile-time guarantees, documenting invariants and review decisions, and aligning lint/test policy without changing runtime behaviour or architecture.

Class diagram for updated scheduling and context switch structures

classDiagram

class ContextSwitch {
  <<trait>>
  +TaskContext
  +unsafe fn context_switch(current: *mut TaskContext, next: *const TaskContext)
}

class SchedQueue_N_ {
  <<generic N>>
  -OptionTask buf[N]
  -usize head
  -usize tail
  -usize len
  +const fn new() SchedQueue_N_
  +fn enqueue(task: TaskHandle) bool
  +fn dequeue() OptionTask
}

class Aarch64TaskContext {
  <<reprC>>
  +u64 x19_x28[10]
  +u64 fp
  +u64 lr
  +u64 sp
  +u64 pc
  +u64 pstate
  +u64 d8_d15[8]
}

class TrapFrame {
  <<reprC>>
  +u64 gpr[31]
  +u64 sp
  +u64 elr
  +u64 spsr
}

class KernelEntry {
  +extern fn kernel_entry() !
}

class QemuVirtContextSwitch {
  <<impl ContextSwitch>>
}

ContextSwitch <|.. QemuVirtContextSwitch
QemuVirtContextSwitch ..> Aarch64TaskContext : TaskContext

KernelEntry ..> SchedQueue_N_ : uses
KernelEntry ..> ContextSwitch : uses

TrapFrame ..> KernelEntry : irq_entry frame
TrapFrame ..> ContextSwitch : interrupt state
Loading

File-Level Changes

Change Details Files
Strengthen kernel compile-time invariants and scheduler queue safety while documenting capability-table correctness assumptions.
  • Replaced the capability-table capacity guard with an inline const { assert!(...) } and documented the BFS descendants buffer size proof in cap_revoke.
  • Added a compile-time const { assert!(N > 0, ...) } guard to SchedQueue::new plus a doc comment section explaining the compile-time failure semantics.
  • Clarified kernel lint layering on workspace lints and tightened guidance for adding kernel-only denies in kernel/src/lib.rs.
kernel/src/cap/table.rs
kernel/src/sched/mod.rs
kernel/src/lib.rs
Align HAL context-switch trait bounds and kernel test lint allowances with the rest of the codebase.
  • Required ContextSwitch implementations to be Send + Sync to match other HAL traits and close a previously masked bound seam.
  • Expanded test-module #[allow(...)] lists in task/endpoint/notification objects to include expect_used and panic, matching other kernel test modules.
hal/src/context_switch.rs
kernel/src/obj/task.rs
kernel/src/obj/endpoint.rs
kernel/src/obj/notification.rs
Harden BSP AArch64 context-switching and IRQ handling contracts with compile-time guards and explanatory comments.
  • Added a compile-time size assertion for Aarch64TaskContext to ensure consistency with naked_asm! stack layout.
  • Documented why irq_entry keeps an unused _frame parameter and explained the defensive compiler_fence in the spurious IRQ path.
  • Linked the IRQ trampoline’s stack layout to the Rust TrapFrame with comments referencing the existing compile-time size assertion.
  • Documented why kernel_entry intentionally omits a defensive parking loop, tying this to the -> ! signature and lint behaviour.
bsp-qemu-virt/src/cpu.rs
bsp-qemu-virt/src/exceptions.rs
bsp-qemu-virt/src/vectors.s
bsp-qemu-virt/src/main.rs

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a6680529-4e5f-4799-8137-2af3b5b12ec7

📥 Commits

Reviewing files that changed from the base of the PR and between 03606a6 and 54b3c78.

📒 Files selected for processing (6)
  • bsp-qemu-virt/src/cpu.rs
  • bsp-qemu-virt/src/exceptions.rs
  • bsp-qemu-virt/src/main.rs
  • bsp-qemu-virt/src/vectors.s
  • kernel/src/cap/table.rs
  • kernel/src/sched/mod.rs
✅ Files skipped from review due to trivial changes (2)
  • bsp-qemu-virt/src/vectors.s
  • kernel/src/cap/table.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • bsp-qemu-virt/src/cpu.rs
  • bsp-qemu-virt/src/main.rs
  • bsp-qemu-virt/src/exceptions.rs
  • kernel/src/sched/mod.rs

📝 Walkthrough

Walkthrough

This PR strengthens compile-time safety and documentation clarity across the kernel. It adds build-time assertions linking Rust struct layouts to assembly hard-coded offsets, clarifies ABI contracts between Rust trampolines and assembly, adds Send + Sync bounds to the ContextSwitch trait, and updates test lint allowances while expanding kernel-level lint documentation. No runtime behavior changes.

Changes

Build-time Safety and Documentation

Layer / File(s) Summary
Trait Bounds for Concurrency
hal/src/context_switch.rs
ContextSwitch trait now explicitly requires Send + Sync implementers.
Task Context Layout Safety
bsp-qemu-virt/src/cpu.rs
Aarch64TaskContext is constrained to exactly 168 bytes via build-time assertion, ensuring Rust struct layout matches hard-coded assembly offsets.
Trap Frame and IRQ Entry Safety
bsp-qemu-virt/src/vectors.s, bsp-qemu-virt/src/exceptions.rs
Assembly TrapFrame documented as requiring exactly 192 bytes with per-stp offsets matching Rust #[repr(C)] layout; irq_entry ABI clarified that unused _frame parameter is retained for uniformity.
Kernel Entry Point Safety
bsp-qemu-virt/src/main.rs
Clarification that start returns -> ! so code following the call is unreachable; no defensive infinite loop intentionally added.
Compile-time Invariants
kernel/src/cap/table.rs, kernel/src/sched/mod.rs
Inline const assertions enforce zero-capacity queue prevention and capability table capacity bounds; documentation links invariants to safe arithmetic and array sizing.
Lint Configuration and Documentation
kernel/src/lib.rs, kernel/src/obj/endpoint.rs, kernel/src/obj/notification.rs, kernel/src/obj/task.rs
Kernel crate lint policy documentation expanded; test module Clippy allowances adjusted to include expect_used and panic suppressions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • cemililik/Tyrne#6: Modifies the same BSP startup call site in bsp-qemu-virt/src/main.rs to use the raw-pointer start(SCHED.as_mut_ptr(), cpu) bridge; related to the kernel-entry changes.

Poem

🐰 A rabbit's ode to safety
Stack frames must align, byte-for-byte,
Assertions catch the drifts at compile's light,
Traits now march with Sync and Send,
Assembly and Rust, together they blend.
Build-time checks guard the kernel's soul.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly matches the PR's scope: it describes code polish addressing multiple non-blockers from a comprehensive review across kernel/HAL/BSP tracks, which aligns with the actual changes (compile-time assertions, trait bounds, lint clarifications, comments).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch code-polish-sweep

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • Several of the new comments embed very specific process metadata (dates, track names, review section numbers, relative paths, etc.); consider trimming these to the minimal technical rationale so the comments age well and don’t require updates when docs move or review structures change.
  • Where comments refer to concrete locations (e.g. exceptions.rs:77, ../obj/arena.rs), it may be more robust to describe the relationship conceptually rather than via brittle file paths/line references that can easily drift with future refactors.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Several of the new comments embed very specific process metadata (dates, track names, review section numbers, relative paths, etc.); consider trimming these to the minimal technical rationale so the comments age well and don’t require updates when docs move or review structures change.
- Where comments refer to concrete locations (e.g. `exceptions.rs:77`, `../obj/arena.rs`), it may be more robust to describe the relationship conceptually rather than via brittle file paths/line references that can easily drift with future refactors.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several documentation and safety improvements across the kernel and BSP. Key changes include adding compile-time size assertions for task contexts and trap frames to prevent layout drift, expanding lint rules for tests, and adding detailed architectural explanations and 'size proofs' in comments. Additionally, the ContextSwitch trait now requires Send + Sync and SchedQueue includes a compile-time check for non-zero capacity. Feedback focuses on improving the maintainability of comments by avoiding hardcoded line numbers and using robust rustdoc paths instead of relative file links for better documentation generation.

Comment thread kernel/src/cap/table.rs Outdated
// `Index::MAX`, so every `index.wrapping_add(1)` fits in `Index`.
const _: () = assert!(CAP_TABLE_CAPACITY <= Index::MAX as usize);
// Inline `const { assert!(...) }` matches the form used in
// `Arena::new` ([obj/arena.rs:90](../obj/arena.rs)); per Phase A
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Referencing specific line numbers in comments (e.g., :90) is fragile as they will likely become stale as the referenced file evolves. It is better to refer to the item name or the file generally without a hardcoded line number to improve maintainability.

        // Arena::new (see obj/arena.rs); per Phase A

Comment thread bsp-qemu-virt/src/vectors.s Outdated
// the Rust-side `TrapFrame` `#[repr(C)]` definition; a size or layout drift
// would corrupt every saved register on every IRQ. The compile-time guard
// `const _: () = assert!(core::mem::size_of::<TrapFrame>() == 192)` lives
// at `bsp-qemu-virt/src/exceptions.rs:77` and fails the build before the
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Referencing a specific line number (:77) in exceptions.rs is prone to becoming outdated. Consider referring to the TrapFrame size assertion by name or context instead to ensure the comment remains accurate as the code changes.

// in bsp-qemu-virt/src/exceptions.rs and fails the build before the

Comment thread kernel/src/sched/mod.rs Outdated
/// is allowed because the inline `const { assert!(N > 0, ...) }` here
/// turns the violation into a hard build error rather than runtime
/// short-circuit. Mirrors the `N == 0` discipline applied in
/// [`Arena::new`](../obj/arena.rs); per Phase A code review + comprehensive
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The relative file link ../obj/arena.rs in a doc comment may not resolve correctly in generated documentation (e.g., via cargo doc). Using a rustdoc path like [crate::obj::arena::Arena::new] is more robust and provides better navigation in the generated HTML.

Suggested change
/// [`Arena::new`](../obj/arena.rs); per Phase A code review + comprehensive
/// [crate::obj::arena::Arena::new]; per Phase A code review + comprehensive

…trim)

Address inline + overall comments on PR #15. All four findings
applied; comments now describe relationships conceptually rather
than via brittle file-path / line-number references.

## Applied

1. **`kernel/src/cap/table.rs`** — `Arena::new`'s line ref `:90`
   dropped from the `const { assert!(...) }` migration comment.
   Reference now says "matches the form used in `Arena::new`"
   without a hardcoded line number. (gemini-code-assist-bot.)

2. **`bsp-qemu-virt/src/vectors.s`** — `exceptions.rs:77` line ref
   dropped from the cross-reference comment block. Comment now says
   the size-assertion "lives next to the `TrapFrame` definition in
   `bsp-qemu-virt/src/exceptions.rs`" — the symbolic anchor is
   stable as the file evolves. (gemini-code-assist-bot.)

3. **`kernel/src/sched/mod.rs`** — the `# Panics (compile-time)` doc
   on `SchedQueue::new` switched the `Arena::new` reference from a
   relative file link `[Arena::new](../obj/arena.rs)` to the rustdoc
   intra-doc link `[crate::obj::arena::Arena::new]`. The new form
   resolves both in `cargo doc` output and in IDE / GitHub-rendered
   markdown without depending on the relative-path hop.
   (gemini-code-assist-bot.)

4. **Process-metadata trim across the polish commits** — sourcery's
   overall feedback flagged the "per Phase A code review §Style +
   comprehensive review 2026-05-06 Track A non-blocker" tails
   embedded in several added comments. These are accurately tracked
   by the original commits' messages (`d86746a`, `03606a6`) and by
   git blame; the inline tails added review-process metadata that
   ages poorly when track names / review structures change. Tails
   trimmed from comments in:
   - `kernel/src/cap/table.rs` (×2 — both the const-block migration
     and the cap_revoke size-proof comment).
   - `kernel/src/sched/mod.rs` (the SchedQueue<0> doc-comment).
   - `bsp-qemu-virt/src/cpu.rs` (the Aarch64TaskContext size-guard
     comment).
   - `bsp-qemu-virt/src/exceptions.rs` (the spurious-IRQ defence-in-
     depth comment — kept the technical rationale, dropped the
     `Per comprehensive review ...` tail).
   - `bsp-qemu-virt/src/vectors.s` (the trampoline cross-reference
     block).
   - `bsp-qemu-virt/src/main.rs` (the kernel_entry "no defensive
     loop" rationale, simplified).

   The technical rationale (size proofs, defence-in-depth justifications,
   `-> !` type-level argument, `N == 0` discipline source) is fully
   preserved.

## Verification

`cargo fmt --check` clean; `cargo host-clippy` clean; `cargo
kernel-clippy` clean; `cargo host-test` 152/152; `cargo kernel-build`
clean. No code logic changed; comments only.

Refs: PR #15, comprehensive-review-2026-05-06

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cemililik cemililik merged commit e9fa019 into main May 7, 2026
6 checks passed
cemililik added a commit that referenced this pull request May 7, 2026
…2 prep activated

Closes B1 — Drop to EL1 + exception infrastructure. The fresh
closure trio replaces the 2026-04-28 trio's load-bearing role:
that trio approved B1 implementation-complete based on host-test
+ miri + paper-review evidence; the maintainer-side QEMU smoke
(still `Pending QEMU smoke verification` on UNSAFE-2026-0019/0020/0021
at the time) had not run. When the maintainer ran it on 2026-05-06,
the smoke surfaced an idle-dispatch regression. T-014 fixed the
regression; the comprehensive multi-agent code review (also
2026-05-06) generated α/β/γ doc-polish PRs; today's trio records
what B1 actually is once smoke-verified end-to-end.

## Three new review artefacts

- docs/analysis/reviews/business-reviews/2026-05-07-B1-closure.md
  — Period 2026-04-28 → 2026-05-07. What landed (T-014 + ADR-0026 +
  PRs #12 / #13 / #14 / #15); what changed in the plan (B1
  reopen → T-014 fix → fresh closure; B2 prep reactivated; ADR-0026
  repurposed); what we learned (smoke is the project's only end-to-
  end liveness oracle; ADR analysis must simulate, not just argue;
  comprehensive review's blind spot was "did you actually run the
  program?"; bot-driven review-rounds are productive when findings
  are factual-mechanical, less so when stylistic). Adjustments
  include "no closure-trio without recorded smoke", write-adr
  skill simulation-table check, comprehensive-review Track K — Live
  execution.

- docs/analysis/reviews/security-reviews/2026-05-07-B1-closure.md
  — Eight axes, all OK. ADR-0026 / T-014 introduce no new attack
  surface, capability widening, memory-safety hazard, or threat-
  model shift. UNSAFE-2026-0014 third Amendment for register_idle;
  UNSAFE-2026-0019/0020 partial-verification + post-T-014 smoke
  Amendments; UNSAFE-2026-0021 no-verification Amendment. Eight
  inherited forward-flagged items unchanged at original severity.
  Verdict: Approve.

- docs/analysis/reviews/performance-optimization-reviews/2026-05-07-B1-closure.md
  — Re-baseline. Net footprint-neutral vs 2026-04-28: .text 21,792
  bytes (-116), .rodata 2,928 (+144), .bss 22,256 (+8). The +144
  .rodata is panic-message clarity strings; the +8 .bss is
  idle: Option<TaskHandle>. Smoke 5.5–6.5 ms boot-to-end, zero
  events. 11 P-numbered proposals from Track D remain queued (P3
  partially landed by γ; P1 / P10 / P4 highest-ROI near-term). No
  proposals to merge this cycle. Verdict: Merge.

## Status flips + index updates

- T-014 In Review → Done. T-014 user-story file's review-history
  gains row 4 recording the maintainer's independent verification
  and the closure trio's landing.
- docs/analysis/tasks/phase-b/README.md — T-014 row to Done.
- docs/roadmap/phases/phase-b.md — sub-breakdown item 7 (T-014)
  flipped to Done; B1 status block rewritten ("B1 closed 2026-05-07")
  with citations to the three new review artefacts.
- docs/roadmap/current.md — top callout rewritten to record B1
  truly closed (2026-05-07); active phase remains B; active
  milestone advances to B2 (MMU activation); active task
  cleared (B2 prep / ADR-0027 drafting opens next per ADR-0025
  §Rule 1); audit status footnote gains the 2026-05-07 update.
- The three review-folder README index tables (business / security /
  performance) gain 2026-05-07-B1-closure rows.

## Verification recap

- cargo fmt --check, cargo host-clippy -D warnings, cargo
  kernel-clippy -D warnings, cargo kernel-build — all clean.
- cargo host-test 25 + 93 + 34 = 152/152.
- cargo +nightly miri test 152/152 clean.
- QEMU smoke at HEAD e9fa019 reproduces the full demo trace + the
  boot-to-end elapsed = ... line; -d int,unimp,guest_errors
  empty for the entire ~5.8 ms run.

## What stays open for δ + B2 prep

- δ — write ADR-0023 placeholder file with Status: Deferred body
  (the README index gained the row in α; the file body is δ's job).
- δ — endpoint rollback / ipc_cancel_recv ADR before B2 lands the
  first userspace destroy path (Track A non-blocker; SchedError::Deadlock
  rollback leaves endpoint in RecvWaiting).
- B2 prep — ADR-0027 (kernel virtual memory layout) drafting +
  docs/architecture/memory-management.md design-first. The ADR's
  Dependency chain opens T-015 in the same commit per ADR-0025
  §Rule 1.

Refs: ADR-0026, ADR-0022, ADR-0025, T-014, B1 closure trio
Audit: UNSAFE-2026-0014, UNSAFE-2026-0019, UNSAFE-2026-0020, UNSAFE-2026-0021

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cemililik cemililik deleted the code-polish-sweep branch May 25, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant