Master-review 2026-05-22 remediation: all verified findings (4 Blockers, 18 Majors, ~46 Minors, ~33 Nits)#32
Merged
Conversation
…ng [MR-001] Blocker MR-001: phase-c/phase-d reused ADR numbers already Accepted on main (0027-0029/0032/0035) and reserved by Phase B (0030/0031/0033/0034). Renumber the forward chain above the live ceiling, ascending by phase: C->0037-0041, D->0042-0046, and cascade E->0047-0052, F->0053-0057, G->0058-0062, H->0063-0065, I->0066-0068. Provenance recorded in each ledger Notes column; 0030/0031/0033/0034 left reserved for Phase B; 0036 left for the ADR-0036 supersession. MR-006: phase-c/phase-d GICv3->GICv2/GIC-400 (QEMU virt is GICv2). MR-021: add a light Phase-F5 secure-field-update milestone stub + Open-Questions (detailed design deferred to a future ADR). MR-004: .claude/skills -> .agents/skills and hal/src/mmu.rs -> hal/src/mmu/mod.rs in phase-b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on riders [MR-006/005/019/020] MR-006 (Blocker-strategic): new ADR-0036 'QEMU virt is GICv2 / no-IOMMU in v1' correcting the GICv3/SMMUv3 statements in ADR-0004/0006/0012; one-line append-only redirect riders added to the top of all three (bodies preserved verbatim per the append-only rule). MR-005: ADR-0020 Revision-notes rider (168-byte context struct, d8-d15 saved not deferred). MR-020: ADR-0008 Revision-notes rider (IrqGuard<C: Cpu> generic + vtable-aliasing rationale). MR-019: ADR-0019 rider pointing to ADR-0021/0026/0028 for the evolved scheduler shape. MR-004: .claude/skills + hal/src/mmu.rs link rot. D2a/D2b hygiene (template, README, 0014/0017/0027/0028/0029/0035); ADR index synced (32 files). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ain [MR-002/003/007/008/009] MR-002 (Blocker): the two 'stable' jobs ran the pinned nightly (rust-toolchain.toml shadows rustup default). Select the pin explicitly, rename the jobs, and add a real cargo +stable host-only subset job for a genuine stable signal. MR-003 (Blocker): reconcile documented vs enforced gates - cargo-audit/cargo-vet/QEMU-smoke marked planned-not-enforced; coverage header lie vs continue-on-error fixed; clippy gate command aligned. MR-007: drop the global RUSTFLAGS env that clobbered .cargo/config.toml per-target panic=abort/frame-pointers. MR-008: SHA-pin every third-party action + top-level permissions: contents: read. MR-009: keep miri non-continue-on-error + document the intended branch-protection gating. C9 nits: run-qemu.sh --help/unknown-flag/PID-suffixed int-log, perf-harness threshold prose + report-overwrite guard, stale counts, .gitignore comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ty & IRQ polarity [MR-005/011/017/018] MR-005: ContextSwitch # Safety contract now enumerates d8-d15 (SIMD/FP callee-saved, whenever CPACR_EL1.FPEN != 0) - the BSP code was already correct; the contract a 2nd BSP reads was not. MR-011: open UNSAFE-2026-0028 for QemuVirtAddressSpace::from_existing_root and narrow the call-site Audit attribution (was misattributed to 0010+0014). MR-017: FakeCpu uses DAIF polarity (IrqState(0)=enabled), matching the BSP; IrqState type doc now defines the canonical zero value (concrete contract). MR-018: new OutOfFramesMmu / BlockMappedMmu failure-injecting fakes + FakeContextSwitch (test-hal/src/context_switch.rs) so the kernel rollback contract is testable; # Safety added to the create_address_space impls; VecFrameProvider zero-fill gap documented. C6/C7/C8 nits (Mmu::map # Errors, descriptor truncation docs, FakeIrqController GIC_MAX_IRQ guard, MAIR/WFI/console comments, module-doc staleness). A Miri-incompatible fn-ptr-identity assertion in the new fake test is guarded under cfg(not(miri)). Behaviour of the shipped kernel/BSP is unchanged; this is contracts, docs, audit trail, and test fakes. Security-sensitive (unsafe/MMU/context-switch) -> warrants a second reviewer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ive polish [MR-010/018] MR-010: replace could_yield_pa_overlapping's O(range_frames x R) per-frame loop with interval arithmetic (O(R)), behaviour-preserving - proven by an equivalence test that keeps the old per-frame algorithm as a reference oracle across 6x13 cases. MR-018: cap_map intermediate-OutOfFrames and BlockMapped tests via the new test-hal fakes (closes the C2-006 path FakeMmu could not model). C1 nits: free_slot publishes free_head last + debug_assert; unlink_from_siblings debug_assert; peer-of-root revoke doc+test; link_child helper; MAX_DERIVATION_DEPTH and SlotEntry-size const asserts (ADR-0023's 32 bytes confirmed). C2 nits: PMM banner refresh; alloc_frame computes the fallible value before mutating; destroy_address_space/get_address_space_mut -> pub(crate); Pmm::new OutOfRange test; bitmap-helper debug_asserts. C4-003: lib.rs Subsystems rustdoc adds mm. table.rs intra-doc link disambiguated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…es, load_image failure tests [MR-022/017/018]
MR-022: factor a private Scheduler::enqueue_ready encapsulating the 'infallible by the no-double-enqueue invariant' panic, called from unblock_receiver_on + yield_now (was duplicated); add_task keeps its typed QueueFull path. Behaviour-preserving (same panic on the same impossible case); forward note added for the future preemption/multi-waiter ADR. MR-017: sched doc states the IrqState(0)=enabled convention; inline FakeCpu/FakeCtx replaced by the shared tyrne_test_hal::{FakeCpu, FakeContextSwitch} (closes X4c-002/005); IRQ-mask-across-switch is now asserted.
MR-018: load_image intermediate-OutOfFrames and BlockMapped rollback tests via the new fakes. C3 nits: RecvOutcome derives aligned; ipc_notify generation-bump test; &/&mut table-borrow asymmetry + CapError->IpcError mapping documented; destroy_endpoint release-leak hazard documented. C4 nits: WidenedRights doc+tests; intermediate_frame_count BSP-coupling note. C5 nits: four-&mut SAFETY strengthened; QueueFull-source doc; default-initialised wording.
Security-sensitive (sched/ipc raw-pointer bridge) -> warrants a second reviewer. Miri-clean, 0 UB.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mu framing & index [MR-012/013/014] MR-012: overview.md no longer claims both IPC flavours share EndpointCap - EndpointCap/Endpoint (sync) vs independent NotificationCap/Notification (async) named distinctly. MR-013: hal.md moves context-switch off Cpu into a ContextSwitch subsection, annotates non-existent Cpu methods (PSCI/core-count/enable_interrupts) as future, relabels the Iommu node 'planned'. MR-014: architecture README index marks memory-management.md Accepted (linked) and adds the task-loader.md row. MR-006 share: hal.md Iommu 'planned for a future SMMUv3 ADR' wording. MR-004 link rot. Nits: ipc.md line-count, scheduler.md class-diagram fields, boot.md Stage-4 entry point. Docs corrected to match the (correct) code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-b task index [MR-016/019] MR-016: flip T-019 to Done (date_done 2026-05-16, PR #31 @ 7f876af), working branch -> none/awaiting B4 closure trio, last-completed-milestone -> B4, resolve the self-contradiction with the dated banners, host-test count -> 260. MR-019: add T-018/T-019 rows to the phase-b task index. MR-004: link rot. The B4 closure trio itself is a separate follow-up (not run here). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…; sweep links [MR-015/004] MR-015: CLAUDE.md/CONTRIBUTING.md no longer say 'architecture phase / most code not yet written' (mid-Phase B; MMU/PMM/AS/task-loader done; syscall ABI next); CONTRIBUTING self-contradiction fixed; SECURITY.md status -> B0-B3 closed/B4 active; README volatile counts de-hardcoded to links (ADR index, unsafe-log). MR-004: .claude/skills sweep incl. the genuinely-wrong .agents/skills/README.md:76 path; review master-plan links. D5a-007: CLAUDE rule-2 audit-log pointer. Nits: docs/README layout, glossary Badge/TCB/Reply-capability, NOTICE slug, D5c review-index markers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…th reality [D3-005/006/007, X3-003] D3-006: code-style.md missing_docs aligned to the actual workspace 'warn'. D3-005: commit-style documents the audit/style types and that the format is convention/review-enforced. D3-007: security-review.md drops the 'once security-model.md exists' conditional (it exists). X3-003: unsafe-policy.md codifies the test-only unsafe exemption (// SAFETY required, no audit-log entry). D3-011: error-handling.md clarifies result_large_err/missing_errors_doc are active via clippy::pedantic=warn (not a missing config). Cargo.toml lint/overflow-checks comments; bsp-boot-checklist --int-log flag fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis PR updates CI/workflows for least-privilege and pinned toolchains; adds deterministic HAL test fakes and MMU failure injectors; tightens kernel internals (scheduler, IPC, cap table, PMM, address-space, task loader) with tests; and performs a broad documentation/ADR/roadmap/glossary synchronization and fixes skill-link paths. ChangesCI, toolchain & infrastructure
Test infrastructure (host fakes)
Kernel behavior, internals & tests
HAL, BSP and low-level docs
Documentation, ADRs, roadmap & skills
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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 significantly advances the Tyrne microkernel to mid-Phase B, introducing ADR-0036 to correct hardware assumptions and renumbering the roadmap. It features extensive refactoring of the capability, IPC, scheduler, and memory management subsystems, alongside documentation updates and enhancements to the test-HAL and tooling. Feedback highlights an opportunity to use the idiomatic div_ceil method in the physical memory manager for improved readability and consistency with the project's coding standards.
Comment on lines
+648
to
+652
| .saturating_add(PAGE_SIZE) | ||
| .saturating_sub(1) | ||
| .wrapping_div(PAGE_SIZE); | ||
| // Walk frame PAs; return true on first non-reserved frame. | ||
| for idx in start_idx..end_idx { | ||
| let frame_pa = extent_start.saturating_add(idx.saturating_mul(PAGE_SIZE)); | ||
| let frame_addr = PhysAddr(frame_pa); | ||
| let in_reserved = self | ||
| .reserved_ranges | ||
| .iter() | ||
| .flatten() | ||
| .any(|r| r.contains(frame_addr)); | ||
| if !in_reserved { | ||
| return true; | ||
|
|
||
| // Interval-coverage walk in frame-index space. `cursor` is the |
There was a problem hiding this comment.
The manual implementation of ceiling division can be replaced with the more idiomatic and readable div_ceil method. This simplifies the logic and matches the implementation used in the test reference at line 1311. Since clippy::arithmetic_side_effects is denied in the kernel, using this method is a preferred way to express the intent clearly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/roadmap/phases/phase-c.md (1)
62-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate ordered-list numbering in C3 sub-breakdown.
Line 62 repeats
6.; this should be the next ordinal item to keep milestone references unambiguous.Proposed fix
-6. **Tests** — two userspace tasks (from B6) time-slice; tick frequency observable; tasks that never yield still get preempted. +7. **Tests** — two userspace tasks (from B6) time-slice; tick frequency observable; tasks that never yield still get preempted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-c.md` at line 62, The ordered list item currently labeled "6. **Tests** — two userspace tasks (from B6) time-slice; tick frequency observable; tasks that never yield still get preempted." is a duplicate ordinal; change its leading "6." to the next correct ordinal (e.g., "7.") so the C3 sub-breakdown numbering advances correctly and milestone references remain unique, and scan the rest of the C3 block to ensure subsequent items use consistent, sequential numbering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 75: All actions/checkout steps currently leave credentials persisted;
update every actions/checkout invocation (the five occurrences of
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683) to include with:
persist-credentials: false so git credentials are not written to local git
config—i.e., for each actions/checkout step (the ones around the CI job checkout
points) add a with block and set persist-credentials: false.
In `@docs/decisions/0012-boot-flow-qemu-virt.md`:
- Line 160: The fenced code block opened with ``` at the noted location lacks a
language tag; update that opening fence (the ``` marker in the document, e.g.,
the code block starting on the shown line) to include an appropriate language
identifier (for example "mermaid" if converting diagrams or "bash"/"text" as
applicable) so markdown linting passes and the block is properly parsed.
- Around line 160-173: Replace the ASCII fenced block with a Mermaid flowchart
code block: use ```mermaid and a flowchart TD mapping the sequence starting at
"0x40080000 _start (.text.boot)" to nodes for ".text", ".rodata", ".data",
".bss (zeroed in _start)", a branched ".boot_pt" node that contains "4 × 4 KiB
(16 KiB)" and the identifiers "__boot_pt_start / __boot_pt_end" and "pre-zeroed
by the BSS-zero loop", an "other BSS" node, then "64 KiB initial stack region"
and finally "__stack_top"; keep the same textual content/phrasing for _start,
.bss, .boot_pt, __boot_pt_start, __boot_pt_end and __stack_top and preserve line
breaks within node labels where helpful.
In `@docs/decisions/0023-cross-table-capability-revocation-policy.md`:
- Line 38: Update the broken in-document link that uses the fragment
"`#decision-outcome-not-applicable-deferred`" so it matches the actual anchor for
the "Decision outcome" heading: either replace that fragment with the exact
generated slug for the "Decision outcome" heading in this document or add an
explicit HTML anchor (e.g., <a
id="decision-outcome-not-applicable-deferred"></a>) immediately before the
"Decision outcome" heading and point the link to that anchor; ensure the link
text "Decision outcome" continues to target the corrected fragment.
In `@docs/roadmap/phases/phase-h.md`:
- Around line 67-69: The three Phase H ledger rows for ADR-0063, ADR-0064, and
ADR-0065 currently claim “renumbered 2026-05-22” but also list the same prior
IDs (self-contradiction); edit those table rows (ADR-0063, ADR-0064, ADR-0065)
to either remove the “renumbered 2026-05-22” phrase or replace the parenthetical
“was ADR-0063/0064/0065 (cascade…)” with the actual pre-cascade IDs you
confirmed from history, ensuring the text consistently reflects either no
renumbering or the correct previous IDs.
In `@docs/roadmap/phases/phase-i.md`:
- Around line 68-70: The ledger notes for entries ADR-0066/0067/0068 are
self-contradictory and reference the wrong F5 ADR; update the three table rows
so the renumbering note is unambiguous (remove or rephrase the “renumbered … was
ADR-00XX” language so it does not claim both identities) and change the Phase F5
reference from ADR-0068 to ADR-0057 to match phase-f.md; ensure the date
(2026-05-22) remains correct and the three entries consistently state either
their original ID or their renumbered ID but not both.
In `@hal/src/cpu.rs`:
- Around line 24-31: Update the Cpu trait docs for restore_irq_state to remove
the contradiction by explicitly stating that while the general contract is
callers should pass a state previously returned from disable_irqs, there is a
canonical exception: IrqState(0) is permitted to be synthesized by callers and
must be treated as "enable interrupts" by implementations (i.e.,
restore_irq_state(IrqState(0)) enables IRQs). Reference the trait name Cpu, the
methods restore_irq_state and disable_irqs, and the special sentinel IrqState(0)
in the docstring so the rule is unambiguous to implementors and test doubles.
In `@kernel/src/mm/address_space.rs`:
- Around line 1308-1311: The existing SAFETY note for the unsafe call to
mmu.create_address_space(frame(0x4000_0000)) is too brief—replace it with a
3-part SAFETY comment that (a) states why unsafe is required
(create_address_space is an unsafe API because it may dereference a raw/physical
root pointer or perform privileged memory setup), (b) lists the invariants you
guarantee here (we pass a host-side mock PhysFrame aligned and non-null, FakeMmu
never dereferences the provided root, and the frame value is valid for the mock
so no UB occurs), and (c) explains why a safe alternative was not used (we must
call the unsafe factory API to exercise real address-space creation code paths
and wrapping it in a safe API would hide the required invariants). Reference
mmu.create_address_space, FakeMmu, and the frame(0x4000_0000) argument in the
comment so reviewers can verify the promised invariants.
In `@test-hal/src/mmu.rs`:
- Around line 365-383: The current map method consumes a frame via
frames.alloc_frame() before delegating to self.inner.map, so invalid-map errors
still deplete the provider; change the flow to call self.inner.map(as_, va, pa,
flags, frames) first and only attempt frames.alloc_frame() after inner.map
returns Ok; if the alloc_frame() then fails, rollback the inserted mapping (e.g.
call the appropriate undo on FakeAddressSpace or self.inner.unmap(as_, va) /
as_.remove_mapping(va)) and return Err(MmuError::OutOfFrames) so non-OutOfFrames
failures do not consume provider frames.
---
Outside diff comments:
In `@docs/roadmap/phases/phase-c.md`:
- Line 62: The ordered list item currently labeled "6. **Tests** — two userspace
tasks (from B6) time-slice; tick frequency observable; tasks that never yield
still get preempted." is a duplicate ordinal; change its leading "6." to the
next correct ordinal (e.g., "7.") so the C3 sub-breakdown numbering advances
correctly and milestone references remain unique, and scan the rest of the C3
block to ensure subsequent items use consistent, sequential numbering.
🪄 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: 1b69a3f8-2460-4cf1-9e6d-ac5d7eff660a
📒 Files selected for processing (94)
.agents/skills/README.md.agents/skills/add-bsp/SKILL.md.github/workflows/ci.yml.gitignoreCLAUDE.mdCONTRIBUTING.mdCargo.tomlNOTICEREADME.mdSECURITY.mdbsp-qemu-virt/Cargo.tomlbsp-qemu-virt/src/console.rsbsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/exceptions.rsbsp-qemu-virt/src/main.rsbsp-qemu-virt/src/mmu.rsdocs/README.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/business-reviews/master-plan.mddocs/analysis/reviews/security-reviews/master-plan.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-019-task-loader.mddocs/architecture/README.mddocs/architecture/boot.mddocs/architecture/hal.mddocs/architecture/ipc.mddocs/architecture/memory-management.mddocs/architecture/overview.mddocs/architecture/scheduler.mddocs/architecture/security-model.mddocs/audits/unsafe-log.mddocs/decisions/0004-target-platforms.mddocs/decisions/0006-workspace-layout.mddocs/decisions/0008-cpu-trait.mddocs/decisions/0012-boot-flow-qemu-virt.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0014-capability-representation.mddocs/decisions/0017-ipc-primitive-set.mddocs/decisions/0019-scheduler-shape.mddocs/decisions/0020-cpu-trait-v2-context-switch.mddocs/decisions/0023-cross-table-capability-revocation-policy.mddocs/decisions/0025-adr-governance-amendments.mddocs/decisions/0026-idle-dispatch-fallback.mddocs/decisions/0027-kernel-virtual-memory-layout.mddocs/decisions/0028-address-space-data-structure.mddocs/decisions/0029-initial-userspace-image-format.mddocs/decisions/0035-physical-memory-manager.mddocs/decisions/0036-qemu-virt-gicv2-no-iommu-v1.mddocs/decisions/README.mddocs/decisions/template.mddocs/glossary.mddocs/guides/ci.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-f.mddocs/roadmap/phases/phase-g.mddocs/roadmap/phases/phase-h.mddocs/roadmap/phases/phase-i.mddocs/standards/bsp-boot-checklist.mddocs/standards/code-style.mddocs/standards/commit-style.mddocs/standards/error-handling.mddocs/standards/infrastructure.mddocs/standards/release.mddocs/standards/security-review.mddocs/standards/testing.mddocs/standards/unsafe-policy.mdhal/src/context_switch.rshal/src/cpu.rshal/src/mmu/mod.rshal/src/mmu/vmsav8.rshal/src/timer.rskernel/src/cap/mod.rskernel/src/cap/rights.rskernel/src/cap/table.rskernel/src/ipc/mod.rskernel/src/lib.rskernel/src/mm/address_space.rskernel/src/mm/mod.rskernel/src/mm/pmm.rskernel/src/obj/endpoint.rskernel/src/obj/task_loader.rskernel/src/sched/mod.rstest-hal/src/context_switch.rstest-hal/src/cpu.rstest-hal/src/irq_controller.rstest-hal/src/lib.rstest-hal/src/mmu.rstools/perf-harness.shtools/run-qemu.sh
Verified each finding against current code; fixed the still-valid ones, kept changes minimal, re-ran all gates.
Real bugs: phase-h/phase-i ADR ledgers had self-referential provenance ('was ADR-0063' instead of 0052, etc.) — a replace_all ordering slip in the renumber commit; phase-i F5 reference corrected to ADR-0057. Hardening: actions/checkout x5 now set persist-credentials: false. Rule compliance: the new 0012 §Revision-notes memory-layout diagram converted from an ASCII tree to a Mermaid flowchart (CLAUDE rule 4). Docs: 0023 broken intra-doc anchor fixed (em-dash heading -> '--deferred' slug); hal Cpu::restore_irq_state doc now states the IrqState(0) canonical-synthesis exception (matches the IrqState type doc); phase-c duplicate '6.' ordinal -> '7.'. Code: kernel pmm could_yield_pa_overlapping ceiling now uses div_ceil (matches the test oracle; clippy-clean); test-hal OutOfFramesMmu::map validates via the inner FakeMmu BEFORE consuming a provider frame, so only the OutOfFrames path depletes the provider (more faithful to the real walker).
Skipped: the bootstrap_setup_generic test SAFETY comment (reviewer wanted full 3-part rigor) — the existing // SAFETY: already satisfies the test-only unsafe exemption codified in unsafe-policy.md; production 3-part rigor is not required for #[cfg(test)] doubles and applying it to one helper would be inconsistent with the rest of the suite.
Validation: fmt clean; host-clippy + kernel-clippy 0 warnings; host-test 283 + 3 doctests pass; kernel-build clean; miri 0 UB; QEMU smoke reaches 'tyrne: all tasks complete' with 0 faults (behaviour unchanged).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F1 (README over-claim): README said the kernel proper has 'a single unsafe (the task-loader byte-copy)' — false and self-contradicted by this PR's own PMM banner (UNSAFE-2026-0026). Replaced with a drift-proof, true statement: production unsafe in the kernel crate is the PMM frame-zeroing + task-loader byte-copy + the scheduler/IPC raw-pointer bridge, each audit-logged; no count asserted. F3 (CI forward-risk): host-stable-check ran 'cargo +stable host-clippy' (= -D warnings with clippy::pedantic=warn) on a ROLLING stable toolchain — a future stable pedantic lint would redden the gate with zero code change, the exact upstream-drift the rest of the PR removed. Narrowed the job to 'cargo +stable build' + 'cargo +stable host-test' (its real 'host crates compile/test on stable' purpose); lint/fmt enforcement stays on the pinned-nightly jobs. Updated the job header, infrastructure.md, and ci.md. N1 (branch-protection doc): infrastructure.md and ci.md listed required checks by job id, but GitHub matches the job display name. Both now give the exact name strings (with ids in parens). F2 (governance) — verified, no code change: the mechanical-edit (append-only-relaxing) exemption was ALREADY on main (this PR only translated a stray token in it), so it was not 'landed' here; the new test-only-unsafe exemption is additive (exempts test doubles from logging; weakens no production/append-only guarantee), codifies practice the master review independently confirmed (X3-003/D5b-006), and there is no existing ADR governing unsafe-policy to rider (ADR-0025 governs ADR amendments, not the unsafe-log). Formal ADR ratification is flagged as an optional maintainer follow-up (a standalone ADR would re-collide with the open Phase-C 0037 numbering). N2 skipped — reviewer judged the OutOfFramesMmu single-intermediate-frame fidelity adequate. No Rust code changed this round; ci.yml validates as YAML, host-stable-check = checkout + install stable + cache + build + host-test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review all |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
test-hal/src/irq_controller.rs (1)
102-110: 💤 Low valueConsider whether production panic is appropriate for test-fake bounds violation.
The assertion uses
assert!(always-on) rather thandebug_assert!. For a test-fake that runs only on the host, a debug-only assertion might suffice since violating the architectural bound is a kernel programming error that should be caught during testing. However, if the intention is to fail loudly in all scenarios (including release-mode tests), the current choice is correct.Alternative using debug_assert!
- assert!( + debug_assert!( irq.0 < FAKE_MAX_IRQ, "FakeIrqController::enable: irq.0 = {} exceeds architectural max {}", irq.0, FAKE_MAX_IRQ, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-hal/src/irq_controller.rs` around lines 102 - 110, The code uses a production (always-on) assert in FakeIrqController::enable to check irq.0 < FAKE_MAX_IRQ; change the assertion to a debug-only check by replacing assert!(irq.0 < FAKE_MAX_IRQ, ...) with debug_assert!(irq.0 < FAKE_MAX_IRQ, ...) so the bound is enforced during testing/debug builds but won’t panic in release builds, keeping the check for kernel-programming errors while avoiding production panics from the test-fake.docs/standards/release.md (1)
61-61: ⚡ Quick winConsider restructuring the process gates section for better scannability.
The first checkbox item is a ~400-character sentence that packs both "enforced in CI" gates and "manual check required" gates into one item. While accurate, this format is difficult to scan during a pre-release checklist walkthrough.
Consider splitting into:
- [ ] All enforced CI gates green: `cargo fmt --check`, clippy (`host-clippy` + `kernel-clippy`), host tests, `kernel-build`, host crates on stable (`host-stable-check`), and Miri. - [ ] Manual verification gates passed (not yet wired to CI per [infrastructure.md](infrastructure.md) §"Planned gates"): QEMU smoke, `cargo-audit`, `cargo-vet`.This preserves the same information but makes each gate category a separate checklist item.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/standards/release.md` at line 61, The long first checklist item under the "process gates" section in docs/standards/release.md is hard to scan; split it into two checklist items: one labeled "All enforced CI gates green" listing `cargo fmt --check`, clippy (`host-clippy` + `kernel-clippy`), host tests, `kernel-build`, host crates on stable (`host-stable-check`), and Miri, and a second labeled "Manual verification gates passed (not yet wired to CI per infrastructure.md §'Planned gates')" listing QEMU smoke, `cargo-audit`, and `cargo-vet`; keep the same wording reference to infrastructure.md so readers know why those are manual.docs/decisions/0020-cpu-trait-v2-context-switch.md (1)
318-319: ⚡ Quick winConsider breaking the revision note into multiple paragraphs for readability.
The 2026-05-22 revision note is a single ~1000-character sentence that is difficult to parse. While the content is accurate and important, breaking it into 3-4 shorter paragraphs would improve readability without changing the substance.
For example:
- Para 1: State the fact (168 bytes, not 104; d8-d15 saved)
- Para 2: Explain what this supersedes
- Para 3: Provide rationale (AAPCS64 requirements)
- Para 4: Note what remains unchanged (trait split)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/decisions/0020-cpu-trait-v2-context-switch.md` around lines 318 - 319, Split the long 2026-05-22 revision note into 3–4 short paragraphs to improve readability: Paragraph 1 state the corrected fact (Aarch64TaskContext is 168 bytes not 104; the struct includes pub d8_d15: [u64; 8] so d8–d15 are saved), Paragraph 2 say what this supersedes (remove/replace the previous §Neutral lines that claimed 104 bytes and deferred NEON/FP saves), Paragraph 3 give the rationale (AAPCS64 callee-saved v8–v15 lower 64 bits and CPACR_EL1.FPEN implication; mention context_switch_asm saves/restores d8–d15), and Paragraph 4 note what is unchanged (Decision-outcome ContextSwitch trait and Cpu v2 split; cross-reference hal/src/context_switch.rs correction). Use the identifiers Aarch64TaskContext, d8_d15, context_switch_asm, ContextSwitch, and Cpu v2 to locate and edit the text.kernel/src/ipc/mod.rs (1)
330-346: ⚡ Quick winConsider defensive error return instead of
unreachable!().The detailed comment (C3-009) correctly identifies this as a temporal invariant under v1's single-threaded cooperative flow. However, the comment also notes that preemption (B5+) could make this branch reachable, which would panic in release.
As the comment itself suggests, the defensive alternative would be to return
Err(IpcError::QueueFull)here. This would be behavior-preserving today (the pre-check ensures this path is unreachable) but would gracefully handle the race if preemption lands without requiring re-audit of this site.🛡️ Defensive alternative
EndpointState::SendPending { .. } | EndpointState::RecvComplete { .. } => { - // Excluded by the pre-check above; unreachable in correct code. - // - // Note (C3-009): this unreachability is a *temporal* invariant — - // the `peek_state` queue-full check above runs, then this commit - // match runs, with nothing mutating the state in between because - // v1 is single-threaded cooperative (no interleaving between peek - // and commit). It is NOT a structural invariant. If a future - // change splits peek and commit across a yield / await / preemption - // point (B5+), a second sender could land a SendPending/RecvComplete - // in this window and make this branch reachable, panicking in - // release. Re-audit when preemption lands (cross-ref ADR-0032 - // §Context's preemption note); the defensive alternative is to - // return `Err(IpcError::QueueFull)` here. - unreachable!() + // Note (C3-009): this is a temporal invariant under v1's + // single-threaded cooperative flow — the `peek_state` queue-full + // check above ensures no SendPending/RecvComplete exists before + // we reach this match. With preemption (B5+), a concurrent sender + // could land SendPending/RecvComplete between peek and commit, + // making this branch reachable. Return QueueFull defensively + // rather than panicking. + Err(IpcError::QueueFull) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/src/ipc/mod.rs` around lines 330 - 346, The match arm handling EndpointState::SendPending and EndpointState::RecvComplete currently calls unreachable!(); replace that panic with a defensive error return (return Err(IpcError::QueueFull)) so the function returns a graceful queue-full error if this temporal-invariant is violated; update the surrounding function (the commit/peek_state path that returns Result<..., IpcError>) to propagate this Err if necessary and keep the existing explanatory comment about temporal unreachability (C3-009).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bsp-qemu-virt/src/exceptions.rs`:
- Around line 69-78: The TrapFrame struct currently derives Debug while
containing the deliberately-uninitialised field _reserved ([u64; 2]), creating a
kernel-stack information-leak risk; remove the auto-derive(Debug) from the
TrapFrame definition and add a custom impl Debug for TrapFrame that formats all
fields but redacts or omits _reserved (so printing never exposes those 16
bytes), and as an optional complementary hardening update, zero that slot in the
vectors.s trampoline where sp is adjusted to eliminate the uninitialised
contents at runtime.
In `@docs/roadmap/phases/phase-f.md`:
- Line 114: The open-question bullet about "Field update / OTA (F5)" references
a stale ADR id ADR-0045; update that token to the current Pi 4 memory-layout ADR
identifier/link (one of the renumbered ADRs in the Phase F range, e.g.
ADR-0053–ADR-0057) so the bullet points to the correct memory-layout ADR and
avoid cross-doc drift.
In `@docs/standards/code-style.md`:
- Line 97: The sentence in the docs ("Heap allocation is **not** available...
allocator ADR, to be written when the allocator is introduced — no ADR in the
current 0001–0035 range covers it") contains a stale ADR range reference; update
the line by removing or generalizing the explicit range (e.g., replace "no ADR
in the current 0001–0035 range covers it" with a non-dated phrase like "no
existing ADR covers it" or "no ADR at the time of writing covers it") and keep
the rest of the sentence and the "allocator ADR" mention intact so the text
remains accurate without future-range drift.
In `@hal/src/mmu/mod.rs`:
- Around line 410-417: The documentation for Mmu::map incorrectly says a
large-block collision returns MmuError::AlreadyMapped; update the doc so it
states that when the requested VA falls inside an existing large block the
function returns MmuError::BlockMapped (keeping AlreadyMapped for exact-slot
collisions), matching the MmuError enum and the cap_map BlockMapped propagation
tests; reference Mmu::map, MmuError::BlockMapped, and MmuError::AlreadyMapped
when making the doc change so callers and implementors branch on the correct
error.
In `@kernel/src/obj/endpoint.rs`:
- Around line 80-108: The current destroy_endpoint implementation is cap-blind
and can silently drop a parked Capability; change destroy_endpoint to accept
&mut IpcQueues (or otherwise obtain mutable access to the queue for the endpoint
slot), have it call IpcQueues::reset_if_stale_generation (or a new helper) to
detect cap-bearing states and return a typed Err(ObjError::HasPendingTransfer)
instead of freeing the slot when a SendPending/RecvComplete with cap: Some(_) is
present, and remove/rework the debug_assert in
IpcQueues::reset_if_stale_generation to rely on this runtime check; update
callers to handle the new Result return and add a brief inline justification
where immediate changes aren’t possible.
In `@test-hal/src/context_switch.rs`:
- Around line 62-70: The unsafe blocks around calls to cs.init_context(&mut a,
never_returns, top) and cs.context_switch(&mut a, &b) need full repository
safety-comment format: replace the short notes with a three-part comment that
(1) explains why unsafe is required (e.g., calling into low-level context APIs:
cs.init_context and cs.context_switch), (2) lists the invariants the call relies
on (e.g., `top` is one-past a 512-byte live stack region, `a`/`b` lifetimes and
mutability guarantees, `never_returns` diverges, the fake context does not
dereference pointers), and (3) states why a safe alternative wasn’t used (e.g.,
these are inherently low-level context operations or test fakes that must bypass
Rust’s borrow rules). Apply the same expanded comment pattern to the other
unsafe sites noted (lines around 176–178, 201–202, 207–208) referencing the same
symbols (FakeTaskContext, never_returns, cs.init_context, cs.context_switch) so
reviewers can verify the invariants.
---
Nitpick comments:
In `@docs/decisions/0020-cpu-trait-v2-context-switch.md`:
- Around line 318-319: Split the long 2026-05-22 revision note into 3–4 short
paragraphs to improve readability: Paragraph 1 state the corrected fact
(Aarch64TaskContext is 168 bytes not 104; the struct includes pub d8_d15: [u64;
8] so d8–d15 are saved), Paragraph 2 say what this supersedes (remove/replace
the previous §Neutral lines that claimed 104 bytes and deferred NEON/FP saves),
Paragraph 3 give the rationale (AAPCS64 callee-saved v8–v15 lower 64 bits and
CPACR_EL1.FPEN implication; mention context_switch_asm saves/restores d8–d15),
and Paragraph 4 note what is unchanged (Decision-outcome ContextSwitch trait and
Cpu v2 split; cross-reference hal/src/context_switch.rs correction). Use the
identifiers Aarch64TaskContext, d8_d15, context_switch_asm, ContextSwitch, and
Cpu v2 to locate and edit the text.
In `@docs/standards/release.md`:
- Line 61: The long first checklist item under the "process gates" section in
docs/standards/release.md is hard to scan; split it into two checklist items:
one labeled "All enforced CI gates green" listing `cargo fmt --check`, clippy
(`host-clippy` + `kernel-clippy`), host tests, `kernel-build`, host crates on
stable (`host-stable-check`), and Miri, and a second labeled "Manual
verification gates passed (not yet wired to CI per infrastructure.md §'Planned
gates')" listing QEMU smoke, `cargo-audit`, and `cargo-vet`; keep the same
wording reference to infrastructure.md so readers know why those are manual.
In `@kernel/src/ipc/mod.rs`:
- Around line 330-346: The match arm handling EndpointState::SendPending and
EndpointState::RecvComplete currently calls unreachable!(); replace that panic
with a defensive error return (return Err(IpcError::QueueFull)) so the function
returns a graceful queue-full error if this temporal-invariant is violated;
update the surrounding function (the commit/peek_state path that returns
Result<..., IpcError>) to propagate this Err if necessary and keep the existing
explanatory comment about temporal unreachability (C3-009).
In `@test-hal/src/irq_controller.rs`:
- Around line 102-110: The code uses a production (always-on) assert in
FakeIrqController::enable to check irq.0 < FAKE_MAX_IRQ; change the assertion to
a debug-only check by replacing assert!(irq.0 < FAKE_MAX_IRQ, ...) with
debug_assert!(irq.0 < FAKE_MAX_IRQ, ...) so the bound is enforced during
testing/debug builds but won’t panic in release builds, keeping the check for
kernel-programming errors while avoiding production panics from the test-fake.
🪄 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: 06b887a5-30b1-4fd0-929e-a50c25d548c1
📒 Files selected for processing (94)
.agents/skills/README.md.agents/skills/add-bsp/SKILL.md.github/workflows/ci.yml.gitignoreCLAUDE.mdCONTRIBUTING.mdCargo.tomlNOTICEREADME.mdSECURITY.mdbsp-qemu-virt/Cargo.tomlbsp-qemu-virt/src/console.rsbsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/exceptions.rsbsp-qemu-virt/src/main.rsbsp-qemu-virt/src/mmu.rsdocs/README.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/business-reviews/master-plan.mddocs/analysis/reviews/security-reviews/master-plan.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-019-task-loader.mddocs/architecture/README.mddocs/architecture/boot.mddocs/architecture/hal.mddocs/architecture/ipc.mddocs/architecture/memory-management.mddocs/architecture/overview.mddocs/architecture/scheduler.mddocs/architecture/security-model.mddocs/audits/unsafe-log.mddocs/decisions/0004-target-platforms.mddocs/decisions/0006-workspace-layout.mddocs/decisions/0008-cpu-trait.mddocs/decisions/0012-boot-flow-qemu-virt.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0014-capability-representation.mddocs/decisions/0017-ipc-primitive-set.mddocs/decisions/0019-scheduler-shape.mddocs/decisions/0020-cpu-trait-v2-context-switch.mddocs/decisions/0023-cross-table-capability-revocation-policy.mddocs/decisions/0025-adr-governance-amendments.mddocs/decisions/0026-idle-dispatch-fallback.mddocs/decisions/0027-kernel-virtual-memory-layout.mddocs/decisions/0028-address-space-data-structure.mddocs/decisions/0029-initial-userspace-image-format.mddocs/decisions/0035-physical-memory-manager.mddocs/decisions/0036-qemu-virt-gicv2-no-iommu-v1.mddocs/decisions/README.mddocs/decisions/template.mddocs/glossary.mddocs/guides/ci.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-f.mddocs/roadmap/phases/phase-g.mddocs/roadmap/phases/phase-h.mddocs/roadmap/phases/phase-i.mddocs/standards/bsp-boot-checklist.mddocs/standards/code-style.mddocs/standards/commit-style.mddocs/standards/error-handling.mddocs/standards/infrastructure.mddocs/standards/release.mddocs/standards/security-review.mddocs/standards/testing.mddocs/standards/unsafe-policy.mdhal/src/context_switch.rshal/src/cpu.rshal/src/mmu/mod.rshal/src/mmu/vmsav8.rshal/src/timer.rskernel/src/cap/mod.rskernel/src/cap/rights.rskernel/src/cap/table.rskernel/src/ipc/mod.rskernel/src/lib.rskernel/src/mm/address_space.rskernel/src/mm/mod.rskernel/src/mm/pmm.rskernel/src/obj/endpoint.rskernel/src/obj/task_loader.rskernel/src/sched/mod.rstest-hal/src/context_switch.rstest-hal/src/cpu.rstest-hal/src/irq_controller.rstest-hal/src/lib.rstest-hal/src/mmu.rstools/perf-harness.shtools/run-qemu.sh
| - **Wi-Fi on Pi 4.** The Broadcom Wi-Fi chip requires proprietary firmware; Tyrne's policy rejects blobs. Options: use Ethernet instead on Pi 4 (simplest), use USB Wi-Fi dongles with open-source firmware, or accept a documented exception for firmware that lives outside the kernel (in-scope for an ADR). | ||
| - **Battery operation.** Power-management is substantial; may belong in Phase I alongside mobile. | ||
| - **Encryption at rest** on device storage — crosses into Phase G. | ||
| - **Field update / OTA (F5).** How much of the verification stack (signatures, measured boot) must land in F5 versus being pulled forward from Phase G (G1 / G2). Whether the A/B dual-bank layout is decided here or in the Pi 4 memory-layout ADR (ADR-0045). What the trust root for the update-signing key is, and where it lives on-device. Whether the update path reuses the E6 network service or the E4 storage service for transport. |
There was a problem hiding this comment.
Update the stale ADR identifier in this open-question bullet.
ADR-0045 appears inconsistent with the Phase F renumbering in this same file (ADR-0053–ADR-0057) and is likely a stale reference. Please update it to the current Pi 4 memory-layout ADR id/link to avoid cross-doc drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/roadmap/phases/phase-f.md` at line 114, The open-question bullet about
"Field update / OTA (F5)" references a stale ADR id ADR-0045; update that token
to the current Pi 4 memory-layout ADR identifier/link (one of the renumbered
ADRs in the Phase F range, e.g. ADR-0053–ADR-0057) so the bullet points to the
correct memory-layout ADR and avoid cross-doc drift.
Comment on lines
+410
to
417
| /// - [`MmuError::AlreadyMapped`] if `va` already has a mapping. Note | ||
| /// that a `va` falling inside an existing large block (e.g. a 2 MiB | ||
| /// block at L1/L2 from the bootstrap mapping) also returns | ||
| /// `AlreadyMapped` on `map` — **not** [`MmuError::BlockMapped`]: | ||
| /// the requested 4 KiB slot is structurally occupied, and | ||
| /// block-split is deferred to B3+. (`unmap` *does* distinguish the | ||
| /// block case as `BlockMapped`; the asymmetry is deliberate.) | ||
| /// - [`MmuError::MisalignedAddress`] if `va` is not |
There was a problem hiding this comment.
Fix the Mmu::map large-block error contract mismatch.
At Line 410, the docs now state that a large-block collision on map should return AlreadyMapped, but this conflicts with the dedicated MmuError::BlockMapped semantics in this module and with the cap_map BlockMapped propagation tests in kernel/src/mm/address_space.rs (MR-018 additions). This can mislead implementors and callers about which failure mode to branch on.
Suggested doc correction
- /// - [`MmuError::AlreadyMapped`] if `va` already has a mapping. Note
- /// that a `va` falling inside an existing large block (e.g. a 2 MiB
- /// block at L1/L2 from the bootstrap mapping) also returns
- /// `AlreadyMapped` on `map` — **not** [`MmuError::BlockMapped`]:
- /// the requested 4 KiB slot is structurally occupied, and
- /// block-split is deferred to B3+. (`unmap` *does* distinguish the
- /// block case as `BlockMapped`; the asymmetry is deliberate.)
+ /// - [`MmuError::AlreadyMapped`] if `va` already has a page mapping.
+ /// - [`MmuError::BlockMapped`] if `va` falls inside a large-block
+ /// descriptor and page-granularity split is deferred.📝 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
| /// - [`MmuError::AlreadyMapped`] if `va` already has a mapping. Note | |
| /// that a `va` falling inside an existing large block (e.g. a 2 MiB | |
| /// block at L1/L2 from the bootstrap mapping) also returns | |
| /// `AlreadyMapped` on `map` — **not** [`MmuError::BlockMapped`]: | |
| /// the requested 4 KiB slot is structurally occupied, and | |
| /// block-split is deferred to B3+. (`unmap` *does* distinguish the | |
| /// block case as `BlockMapped`; the asymmetry is deliberate.) | |
| /// - [`MmuError::MisalignedAddress`] if `va` is not | |
| /// - [`MmuError::AlreadyMapped`] if `va` already has a page mapping. | |
| /// - [`MmuError::BlockMapped`] if `va` falls inside a large-block | |
| /// descriptor and page-granularity split is deferred. | |
| /// - [`MmuError::MisalignedAddress`] if `va` is not |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hal/src/mmu/mod.rs` around lines 410 - 417, The documentation for Mmu::map
incorrectly says a large-block collision returns MmuError::AlreadyMapped; update
the doc so it states that when the requested VA falls inside an existing large
block the function returns MmuError::BlockMapped (keeping AlreadyMapped for
exact-slot collisions), matching the MmuError enum and the cap_map BlockMapped
propagation tests; reference Mmu::map, MmuError::BlockMapped, and
MmuError::AlreadyMapped when making the doc change so callers and implementors
branch on the correct error.
Comment on lines
+80
to
+108
| /// # In-flight capability hazard (C3-001 — deliberate v1 deferral) | ||
| /// | ||
| /// `destroy_endpoint` is **cap-blind**: it frees the arena slot and bumps | ||
| /// the generation but does **not** consult [`IpcQueues`][crate::ipc::IpcQueues]. | ||
| /// If an endpoint is destroyed while its queue slot holds a | ||
| /// `SendPending { cap: Some(_) }` or `RecvComplete { cap: Some(_) }`, the | ||
| /// parked move-only [`Capability`][crate::cap::Capability] is owned solely by | ||
| /// that state. On the next IPC op against a *new* endpoint allocated in the | ||
| /// same slot, `IpcQueues::reset_if_stale_generation` overwrites the state with | ||
| /// `Idle` and the parked cap is dropped on the floor — a silently leaked | ||
| /// authority. In **debug** builds the `debug_assert!` in | ||
| /// `reset_if_stale_generation` fires on exactly this case; in **release** it | ||
| /// is compiled out, so the leak is silent. | ||
| /// | ||
| /// This is *currently benign and intentional*, not unhandled: no production | ||
| /// code calls `destroy_endpoint` on a cap-bearing pending state, and the | ||
| /// destroy-drain primitive (which must *return* the parked cap to its origin | ||
| /// or destroy it) is deferred to the Phase B2+ endpoint-destroy ADR per | ||
| /// [ADR-0032] §Consequences. The conservative future improvement is to have | ||
| /// this (or a thin IPC-layer wrapper) take `&mut IpcQueues` and return a typed | ||
| /// `ObjError::HasPendingTransfer` when the slot is cap-bearing — converting the | ||
| /// debug-only assert into a release-safe refusal. Until that ADR lands, a | ||
| /// caller that frees a cap-bearing endpoint is the one path to the leak. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// [`ObjError::InvalidHandle`] when `handle` is stale or already freed. | ||
| /// | ||
| /// [ADR-0032]: https://github.com/cemililik/Tyrne/blob/main/docs/decisions/0032-endpoint-rollback-and-cancel-recv.md |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Excellent hazard documentation; consider accelerating the mitigation.
The C3-001 documentation is thorough and clearly explains the in-flight capability leak scenario. However, the current mitigation relies on (a) no production caller triggering the hazard and (b) debug-only assertions—both fragile in the face of future refactoring.
Since the conservative fix is already outlined ("take &mut IpcQueues and return ObjError::HasPendingTransfer when the slot is cap-bearing"), consider implementing it now rather than deferring to ADR-0032/Phase B2+. The change would:
- Convert a silent release-build leak into a loud typed refusal,
- Remove the need for the debug_assert dance in
reset_if_stale_generation, - Align with the "security-first" discipline (never silently leak authority).
If the deferral stands, ensure all current call sites of destroy_endpoint include an inline comment justifying why they cannot trigger the cap-bearing case.
Based on learnings: "Adopt a security-first mindset: never weaken a capability check, never introduce ambient authority, and never suppress a failing security test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@kernel/src/obj/endpoint.rs` around lines 80 - 108, The current
destroy_endpoint implementation is cap-blind and can silently drop a parked
Capability; change destroy_endpoint to accept &mut IpcQueues (or otherwise
obtain mutable access to the queue for the endpoint slot), have it call
IpcQueues::reset_if_stale_generation (or a new helper) to detect cap-bearing
states and return a typed Err(ObjError::HasPendingTransfer) instead of freeing
the slot when a SendPending/RecvComplete with cap: Some(_) is present, and
remove/rework the debug_assert in IpcQueues::reset_if_stale_generation to rely
on this runtime check; update callers to handle the new Result return and add a
brief inline justification where immediate changes aren’t possible.
Re-verified all 12 PR #32 review comments against live code. Applied: - exceptions.rs: hand-written TrapFrame Debug redacts the uninitialised _reserved slot, closing the C7-010 stack info-leak via the Debug path. - address_space.rs / context_switch.rs: expand test-only unsafe SAFETY notes to the full (a)/(b)/(c) triad (unsafe-policy.md exempts test unsafe only from the audit log, not from a conforming SAFETY comment). - code-style.md: de-hardcode the stale "0001-0035 range" allocator-ADR phrase. - release.md: split the CI-gates checklist item into enforced vs manual gates. - ADR-0020: split the 2026-05-22 d8-d15 revision note into paragraphs (no wording change; append-only preserved). Skipped (with reason): - phase-f.md ADR-0045: false positive; 0045 IS the Pi 4 memory-layout ADR (phase-d.md D5). - mmu/mod.rs map contract: doc matches shipped BSP (map -> AlreadyMapped; only unmap -> BlockMapped). - endpoint.rs destroy_endpoint: deliberate v1 deferral tied to ADR-0032. - ipc/mod.rs unreachable!(): fail-stop is the conservative choice; the C3-009 re-audit trigger (B5+/preemption) is already documented. - irq_controller.rs assert!: the fake deliberately mirrors the production GIC assert!. - pmm.rs div_ceil: already applied. Validation: fmt, host-clippy, host-test (incl. doctests), kernel-clippy, kernel-build (aarch64) all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…/Tyrne) The repository is moving to the HodeTech organisation. Mechanical URL sweep plus an ownership update. Changed: - Rewrote all 238 live `https://github.com/cemililik/Tyrne` URLs to `.../HodeTech/Tyrne` across source doc-comments, ADRs, architecture / standards / roadmap / guides / task docs, skills, SECURITY.md advisory URL, NOTICE, and Cargo.toml `repository`. Per maintainer decision, the URLs inside the dated review records under docs/analysis/reviews/** were updated too. - NOTICE copyright -> "Copyright 2026 HodeTech and Tyrne contributors". Cargo.toml `authors = ["Cemil Ilik"]` kept unchanged (org owner + individual author). Deliberately preserved (not URLs, or would falsify recorded history): - `cemililik/TyrneOS` (18) and `cemililik/Umbrix*` (2): old project names cited in dated review snapshots; the sweep used a negative lookahead so the 4 historical TyrneOS URLs stay intact. - bare `cemililik/Tyrne` prose tokens in the TyrneOS->Tyrne rename narrative and the unsafe-policy mechanical-edit example. - `@cemililik` maintainer handle (ADR deciders, review participants). Note: the local `git remote` still points at cemililik/Tyrne; after the GitHub transfer, run `git remote set-url origin https://github.com/HodeTech/Tyrne.git`. Validation: cargo fmt --all --check, host-test (283 pass + doctests), kernel-build (aarch64) all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hal/src/mmu/mod.rs (1)
410-416:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
BlockMappedas the documented large-block collision error.Lines 410-416 still tell implementors and callers to collapse a large-block hit into
AlreadyMapped, but this module already exposesMmuError::BlockMappedas the distinct failure mode for that condition. That leaves the public trait contract internally inconsistent and encourages the wrong branch behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hal/src/mmu/mod.rs` around lines 410 - 416, The documentation for the map/unmap behavior is inconsistent: update the doc comment near the Mmu trait/map documentation to state that a large-block collision returns MmuError::BlockMapped (not MmuError::AlreadyMapped) when the requested small slot falls inside an existing large block, while unmap continues to distinguish the block case as BlockMapped; specifically change the wording that currently collapses large-block hits into AlreadyMapped to explicitly reference MmuError::BlockMapped and keep the asymmetry note about unmap unchanged so callers and implementors use MmuError::BlockMapped for large-block collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@hal/src/mmu/mod.rs`:
- Around line 410-416: The documentation for the map/unmap behavior is
inconsistent: update the doc comment near the Mmu trait/map documentation to
state that a large-block collision returns MmuError::BlockMapped (not
MmuError::AlreadyMapped) when the requested small slot falls inside an existing
large block, while unmap continues to distinguish the block case as BlockMapped;
specifically change the wording that currently collapses large-block hits into
AlreadyMapped to explicitly reference MmuError::BlockMapped and keep the
asymmetry note about unmap unchanged so callers and implementors use
MmuError::BlockMapped for large-block collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49d8d018-6c8a-49b0-8d2b-e2954ab0a543
📒 Files selected for processing (60)
Cargo.tomlNOTICESECURITY.mdbsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/exceptions.rsbsp-qemu-virt/src/gic.rsbsp-qemu-virt/src/main.rsbsp-qemu-virt/src/mmu.rsbsp-qemu-virt/src/mmu_bootstrap.rsdocs/analysis/reviews/business-reviews/2026-04-28-B1-closure.mddocs/analysis/reviews/business-reviews/2026-05-07-B1-closure.mddocs/analysis/reviews/business-reviews/2026-05-14-B3-closure.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-a-kernel.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-f-tests.mddocs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review.mddocs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-1-pr-19-mechanical.mddocs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-2-pr-20-design.mddocs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-3-pr-20-governance.mddocs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-4-pr-21-perf-harness.mddocs/analysis/reviews/master-review/2026-05-22-152729/tracks/D5a-meta-core.mddocs/analysis/reviews/performance-optimization-reviews/2026-05-07-B1-closure.mddocs/analysis/reviews/performance-optimization-reviews/2026-05-14-B3-closure.mddocs/analysis/reviews/security-reviews/2026-04-28-B1-closure.mddocs/analysis/reviews/security-reviews/2026-05-07-B1-closure.mddocs/analysis/reviews/security-reviews/2026-05-14-B3-closure.mddocs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.mddocs/architecture/ipc.mddocs/decisions/0020-cpu-trait-v2-context-switch.mddocs/guides/run-under-qemu.mddocs/roadmap/current.mddocs/standards/code-style.mddocs/standards/release.mdhal/src/console.rshal/src/context_switch.rshal/src/cpu.rshal/src/irq_controller.rshal/src/lib.rshal/src/mmu/mod.rshal/src/mmu/vmsav8.rshal/src/timer.rskernel/src/cap/mod.rskernel/src/cap/rights.rskernel/src/cap/table.rskernel/src/ipc/mod.rskernel/src/lib.rskernel/src/mm/address_space.rskernel/src/mm/mod.rskernel/src/mm/pmm.rskernel/src/obj/arena.rskernel/src/obj/endpoint.rskernel/src/obj/mod.rskernel/src/obj/notification.rskernel/src/obj/task.rskernel/src/obj/task_loader.rskernel/src/sched/mod.rstest-hal/src/context_switch.rstest-hal/src/lib.rstest-hal/src/mmu.rs
✅ Files skipped from review due to trivial changes (45)
- docs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-2-pr-20-design.md
- docs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-4-pr-21-perf-harness.md
- kernel/src/obj/arena.rs
- kernel/src/obj/notification.rs
- docs/guides/run-under-qemu.md
- docs/analysis/reviews/business-reviews/2026-05-07-B1-closure.md
- docs/analysis/reviews/performance-optimization-reviews/2026-05-14-B3-closure.md
- hal/src/lib.rs
- bsp-qemu-virt/src/mmu_bootstrap.rs
- kernel/src/obj/task.rs
- hal/src/console.rs
- docs/analysis/reviews/security-reviews/2026-05-14-B3-closure.md
- docs/analysis/reviews/performance-optimization-reviews/2026-05-07-B1-closure.md
- bsp-qemu-virt/src/gic.rs
- kernel/src/obj/mod.rs
- docs/analysis/reviews/business-reviews/2026-04-28-B1-closure.md
- docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-a-kernel.md
- docs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-3-pr-20-governance.md
- docs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.md
- docs/architecture/ipc.md
- docs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review/track-1-pr-19-mechanical.md
- docs/analysis/reviews/security-reviews/2026-04-28-B1-closure.md
- Cargo.toml
- docs/analysis/reviews/security-reviews/2026-05-07-B1-closure.md
- bsp-qemu-virt/src/mmu.rs
- docs/analysis/reviews/business-reviews/2026-05-14-B3-closure.md
- NOTICE
- docs/standards/release.md
- kernel/src/obj/endpoint.rs
- docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review.md
- hal/src/timer.rs
- docs/standards/code-style.md
- docs/analysis/reviews/master-review/2026-05-22-152729/tracks/D5a-meta-core.md
- hal/src/irq_controller.rs
- kernel/src/cap/mod.rs
- docs/analysis/reviews/code-reviews/2026-05-08-pr-19-20-21-multi-axis-review.md
- kernel/src/mm/mod.rs
- docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md
- bsp-qemu-virt/src/cpu.rs
- docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-f-tests.md
- kernel/src/lib.rs
- bsp-qemu-virt/src/main.rs
- hal/src/cpu.rs
- docs/roadmap/current.md
- hal/src/mmu/vmsav8.rs
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.
Master-review 2026-05-22 remediation
Addresses every verified finding from the master review at
docs/analysis/reviews/master-review/2026-05-22-152729/consolidated.md(anchor
288ddb2): 4 Blockers, 18 Majors, ~46 Minors, ~33 Nits.Each finding was independently re-verified against live code before action. The defining
result — confirmed by verification — is that for almost every contradiction the shipped
kernel code is correct and the docs / trait-contracts / ADRs were wrong, so the fixes
correct the docs/contracts to match the code. Only MR-010 (PMM interval arithmetic),
MR-011 (audit-log entry), MR-018 (test-hal fakes + tests), and MR-022 (sched
helper) involve real code edits — all behaviour-preserving (QEMU smoke trace unchanged).
Work was done across 11 conflict-free workstreams (disjoint file sets) and is presented as
grouped, critical→cosmetic commits.
Blockers (4)
main. Renumbered the forward chain off the live ceiling (C→0037–0041, D→0042–0046; cascaded E/F/G/H/I→0047–0068, ascending by phase, provenance in each ledger).0030/0031/0033/0034left reserved for Phase B.cargo +stablehost subset added; the audit/vet/QEMU-smoke gates marked planned-not-enforced; coverage header/continue-on-errorcontradiction fixed..claude/skills/+hal/src/mmu.rscross-references in live docs swept (historical dated snapshots correctly preserved).Iommustub. New ADR-0036 ("QEMU virt is GICv2 / no-IOMMU in v1") + append-only redirect riders (bodies preserved).Majors (18)
MR-005 (d8–d15
ContextSwitchcontract), MR-007 (RUSTFLAGS clobber), MR-008 (SHA-pin actions +permissions:), MR-009 (miri gate intent), MR-010 (O(R) PMM helper), MR-011 (UNSAFE-2026-0028forfrom_existing_root), MR-012 (overview.mdIPC objects), MR-013 (hal.mdCpu/ContextSwitch/Iommu), MR-014 (architecture index), MR-015 (front-door status + de-hardcoded counts), MR-016 (current.mdpost-merge truth), MR-017 (IrqState(0)polarity made concrete + fake consolidation), MR-018 (FakeMmu fidelity decorators + tests), MR-019 (ADR-0019 + task index), MR-020 (ADR-0008 rider), MR-021 (Phase-F5 OTA stub), MR-022 (enqueue_readyhelper), plus the standards-vs-reality set (D3-005/006/007).Minors & Nits
All addressed in their owning commits (defensive
debug_assert!s, doc/banner refresh, link rot, contract-text accuracy, test-coverage edges, cosmetic consistency). A few review sub-claims were found stale/false on verification and are documented rather than blindly applied — notably D3-011 (result_large_err/missing_errors_docare already active viaclippy::pedantic = "warn").Verification — all gates green
cargo fmt --all --checkcargo host-testcargo kernel-build(aarch64-none)cargo +nightly miri test(workspace −BSP)cargo llvm-covtyrne: all tasks complete, 0 faults, behaviour unchangedOne Miri failure surfaced and was fixed: a new
FakeContextSwitchtest assertedfunction-pointer-to-integer identity, which Miri assigns non-stable synthetic addresses for
(passes on real hardware) — now guarded under
cfg(not(miri)).Security-sensitive
Commits touching capabilities / IPC / memory /
unsafe/ MMU / context-switch(
hal,bsp,test-hal …,kernel(cap,mm)…,kernel(sched,ipc,obj)…,docs(adr)…) warrant asecond reviewer per CLAUDE.md rules 1/2/4. ADR changes are append-only (rule 5).
Deferred follow-ups (not in this PR, by design)
conduct-review×3), now correctly recorded as pending incurrent.md.unsafeexemption ADR — the policy text is codified inunsafe-policy.md; a formal ADR is deferred to avoid re-colliding with the freshly-assigned Phase-C ADR-0037 number.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Testing
Infrastructure
Chores