Development #12 — B1 smoke regression: ADR-0026 supersedes ADR-0022 Option A; T-014 fixes idle dispatch - #12
Merged
Merged
Conversation
Holistic full-tree code review at HEAD 214052d, run via the plan in 2026-05-06-full-tree-comprehensive-review-plan.md: pre-flight verification (cargo fmt/clippy/test/miri all green) plus ten parallel agent tracks (kernel, HAL, security, performance, docs, tests, BSP, infra, integration, hygiene), merged into one consolidated artifact. Verdict: Request changes — Track E (docs) returned 7 blocker-class doc-drift items; the other 9 tracks returned Approve/Comment/Iterate with no code or security blockers. The 7 doc-drift items are tracked as a separate post-B1-closure follow-up sweep; this commit lands the review artefacts only and does not act on the findings. The artefacts are pure information — no source or doc state outside the review folder is mutated by this commit. Cross-references from the regression-arc commits that follow this one rely on these artefacts existing, so they ship together in the same PR. Refs: comprehensive-review-2026-05-06 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
macOS ships /usr/bin/bash 3.2 by default. Under `set -u`, expanding an
empty array via `"${arr[@]}"` errors with "INT_LOG_FLAGS[@]: unbound
variable" because bash 3.2 treats unset-and-empty-array differently
from later versions.
Replace `"${INT_LOG_FLAGS[@]}"` with the standard idiom
`${INT_LOG_FLAGS[@]+"${INT_LOG_FLAGS[@]}"}`, which expands to nothing
if the array is unset/empty, or to the quoted array elements if set.
Works on bash 3.2+ and bash 4+.
Surfaced when running tools/run-qemu.sh on macOS during the 2026-05-06
B1 smoke verification.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…supersedes ADR-0022 The first end-to-end QEMU smoke at HEAD 214052d (2026-05-06) surfaced a kernel-side regression: after task A's ipc_send_and_yield delivers to task B and yields, the dispatcher selects idle (sitting in the FIFO from kernel_entry's add_task ordering) ahead of the just- unblocked B; idle issues WFI; v1's demo never arms a deadline; the kernel hangs. Root cause is structural in ADR-0022 §Decision outcome's chosen Option A (idle as a regular ready-queue resident). The "yield_now's only-one-ready fast path collapses solo-idle" claim the ADR cited was true only when idle is the sole Ready task; the demo's three-task moment (B unblocked, A yielding, idle in FIFO) falls outside that case. The defect entered the tree at T-007 (B0) and was inherited unmodified through B1's closure. This commit records the regression and proposes the structural fix: - docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md — mini-retro: trace, root cause, simulation gap, learnings, adjustments. - docs/audits/unsafe-log.md — 2026-05-06 partial-verification Amendments on UNSAFE-2026-0019 / 0020 (init MMIO + VBAR install reached) and no-verification Amendment on 0021 (timer-write site unreachable in v1 demo). All three retain their `Pending QEMU smoke verification` status; full clearance is gated on a future arm_deadline caller. - docs/decisions/0026-idle-dispatch-fallback.md — Accepted today. Supersedes ADR-0022's *idle-task-location* axis only (Option A → Option B: dedicated `Scheduler::idle: Option<TaskHandle>` slot, dispatched as fallback). ADR-0022's *typed-error* axis (Option G: SchedError::Deadlock + IpcError::PendingAfterResume + start's panic) stands. Includes a queue-state simulation table that ADR-0022 lacked — the discipline this regression motivated. - docs/decisions/0022-...md — Status flipped to `Superseded by 0026 (idle-task-location axis only; typed-error axis stands)`; body preserved unmodified per ADR-0025 §Rule 2 (append-only) with a callout at the top pointing forward. - docs/decisions/README.md — index reflects ADR-0022 + ADR-0026 status. - docs/roadmap/phases/phase-b.md — B1 status update (reopened); ADR-0026 row repurposed (originally reserved for T-012 vector- table design which T-012 absorbed without writing); sub-breakdown items 6 (ADR-0026 ✅ Accepted) + 7 (T-014 Draft) added. - docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md — user-story Draft. Will refactor the scheduler per ADR-0026's Decision outcome §Dependency chain. - docs/roadmap/current.md — banner + Active task pointer + Last reviews + audit-status update reflect the B1 reopen. Phase 1 + Phase 2 of the fix arc. Phase 3 (kernel refactor + smoke re-run + closure docs) follows in the next two commits. Refs: ADR-0026, ADR-0022, ADR-0025, T-014 Audit: UNSAFE-2026-0019, UNSAFE-2026-0020, UNSAFE-2026-0021 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…DR-0026) Implements ADR-0026's Option B for idle-task dispatch — supersedes ADR-0022's Option A on the *idle-task-location* axis only. The idle task moves out of the FIFO ready queue into a dedicated `Scheduler::idle: Option<TaskHandle>` slot consulted only when `s.ready.dequeue()` returns `None`. Idle never displaces a real Ready task; the structural property closes the 2026-05-06 smoke regression. ## kernel/src/sched/mod.rs - `Scheduler<C>` gains `idle: Option<TaskHandle>` field; `new()` initialises to `None`. - New `pub unsafe fn register_idle<C>(sched: *mut Scheduler<C>, cpu: &C, handle, entry, stack_top)` — raw-pointer free function in the ADR-0021 shape, mirroring `add_task`'s `init_context` call and the momentary-`&mut` discipline. Idle is recorded in `s.idle`; never enqueued in `s.ready`. - `start_prelude` dispatch chain becomes `s.ready.dequeue().or(s.idle)`; the panic-on-empty message tightens to "empty ready queue and no idle task". The `start_prelude_panics_on_empty_ready_queue` test still asserts the substring "empty ready queue" so its `#[should_panic]` matcher continues to bind. - `yield_now`: re-enqueues `current` only when `current != s.idle` (idle is never in the FIFO). Dequeue chain falls back to `s.idle` when the queue is empty; fast-path early-return when the resolved next handle equals `current_handle`. - `ipc_recv_and_yield` Phase 2: dispatch chain becomes `s.ready.dequeue().or(s.idle)`. `SchedError::Deadlock` fires only when both halves are `None` — the variant's defensive- return semantics tighten and remain testable via the existing T-007 test (which constructs a scheduler without registering idle). - Module-level docstring §"Idle task" rewritten for the supersession; the previous "idle as ready-queue resident" paragraph now points at ADR-0026 and explains why the demo's three-task moment broke ADR-0022's solo-idle assumption. - `SchedError::Deadlock` doc-comment updated; cross-link to ADR-0026 added. ## kernel/src/sched/mod.rs — three new host tests - `register_idle_stores_handle_in_idle_slot_and_not_in_ready_queue` asserts the structural invariant: handle goes to `Scheduler::idle`, not the FIFO. - `dispatcher_picks_idle_only_when_ready_queue_empty` asserts fallback semantics: regular task A is selected first; idle is picked only after A blocks. - `unblock_after_yield_dispatches_unblocked_receiver_not_idle` is the regression guard. Reproduces the demo's failing flow in a host-testable form: with B Blocked-on-ep, A current, queue empty, idle registered, A's `ipc_send_and_yield` delivers → unblock enqueues B → `yield_now` switches to B (NOT idle). Under ADR-0022 Option A this test would have failed at `assert_eq!(sched.current, Some(h_b))`. ## bsp-qemu-virt/src/main.rs - `kernel_entry` switches the third registration call from `sched.add_task(... idle_entry ...)` to `register_idle(core::ptr::from_mut(&mut sched), ... idle_entry, ...)`. Tasks B and A continue to register via `add_task`. The order (B before A before idle) is preserved for narrative continuity but is no longer load-bearing — idle is in its own slot, so its order in the BSP setup sequence does not affect dispatch. - `idle_entry` doc-comment + the inline `wait_for_interrupt` comment rewritten to describe ADR-0026's fallback model (replacing ADR-0022's "FIFO resident" prose). - New import line picks up `register_idle` from `tyrne_kernel::sched`. ## Verification `cargo fmt --check` clean; `cargo host-clippy` clean; `cargo kernel-clippy` clean; `cargo host-test` 25 + 93 + 34 = **152 / 152** (was 149; +3 new tests); `cargo +nightly miri test` **152 / 152** clean; `cargo kernel-build` clean. QEMU smoke verification + audit- log post-T-014 Amendments + closure docs follow in the next commit. Phase 3 of the B1-smoke-regression fix arc. Refs: ADR-0026, ADR-0022, ADR-0021, T-014 Audit: UNSAFE-2026-0014 (existing entry covers `register_idle`'s momentary-`&mut` pattern; explicit naming follows in a future review-fix commit if asked) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-fix Amendments Closes Phase 3 of the B1-smoke-regression fix arc. T-014's implementation landed in the previous commit; this commit records the verification artefacts and propagates the post-fix state across the doc tree. ## docs/audits/unsafe-log.md UNSAFE-2026-0019 + 0020 each gain a 2026-05-06 *post-T-014 smoke* Amendment reaffirming the partial-verification claim under sustained execution: with idle dispatch fixed, the smoke now produces the full demo trace (~6.3 ms boot-to-end) and `-d int,unimp,guest_errors` stays empty for the entire window. The setup sites (GIC init + VBAR install + DAIF unmask) are confirmed; the IRQ-take + dispatch path itself remains unexercised because v1's demo has no `arm_deadline` caller. UNSAFE-2026-0021 is unchanged — the no-verification Amendment from the morning's smoke still applies (no deadline arm either way). All three retain `Pending QEMU smoke verification` status; full clearance is gated on a future B-phase task that arms a real deadline. ## docs/architecture/scheduler.md New §Revision notes section explains the ADR-0026 supersession without rewriting the body. The §"Idle task and structural non- emptiness" prose above continues to describe the *behaviour* idle provides (FIFO never empty when current is set, Deadlock as defensive return); ADR-0026 changes the *mechanism* (idle in a fallback slot, not a FIFO resident). Both ADR-0022 (typed-error axis) and ADR-0026 (location axis) are now load-bearing. ## docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md Status: `In Progress → In Review`. Review-history row 3 records verification (152/152 host tests + 152/152 miri + clippy + smoke) with the literal QEMU serial trace pasted verbatim. Maintainer flips to `Done` after independent verification. ## docs/analysis/tasks/phase-b/README.md ## docs/roadmap/phases/phase-b.md T-014 promoted in the phase-b task index (Draft → In Review). B1 sub-breakdown item 7 flipped from Draft to ✅ verification snapshot. ## docs/roadmap/current.md Active task points at T-014 In Review; In review row populated. Next event after maintainer Done flip: fresh B1 closure trio (business + consolidated security + performance baseline). B2 prep (ADR-0027) stays paused until then. ## docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md New "Follow-up note (Phase 3 closure)" section appended (append- only convention — body above is the state at smoke-discovery time; this section is the state at fix-landed time). Includes: - Verification snapshot (test counts + miri + smoke trace pointer). - "What the three new tests actually catch" — which one would have surfaced the bug pre-merge if it had existed in the test suite. - Lessons revisited — the "smoke is the only liveness oracle" point is empirically reaffirmed (152 host + 152 miri tests passed silently; one 6-second smoke surfaced the bug instantly). The "ADR analysis must simulate" point gets a concrete proposed pattern: pair an ADR's simulation table with a host test that mechanically replays it. Both stay deferred to the post-fix B1 closure retro for codification — sample size 1. Refs: ADR-0026, ADR-0022, T-014 Audit: UNSAFE-2026-0019, UNSAFE-2026-0020 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ 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[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
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements idle task dispatch via a separate fallback slot (ADR-0026) to resolve B1 smoke regressions. The kernel scheduler gains an ChangesIdle Dispatch via Fallback Slot
Full-Tree Code Review Documentation
Tool Script Fix
Sequence DiagramsequenceDiagram
participant Entry as Kernel Entry (BSP)
participant Sched as Scheduler
participant Ready as Ready Queue
participant Idle as Idle Slot
participant Task as Task
Entry->>Sched: start_prelude()
activate Sched
Sched->>Ready: dequeue()
activate Ready
Ready-->>Sched: None (empty)
deactivate Ready
alt Ready queue empty
Sched->>Idle: check idle present?
activate Idle
Idle-->>Sched: Some(idle_handle)
deactivate Idle
Sched->>Task: dispatch(idle_handle)
else Ready queue has task
Sched->>Task: dispatch(ready_task)
end
deactivate Sched
activate Task
Task->>Task: execute
Task->>Sched: yield_now()
deactivate Task
activate Sched
Sched->>Ready: dequeue()
activate Ready
Ready-->>Sched: next_task or None
deactivate Ready
alt Ready task available
Sched->>Task: redispatch(next_ready)
else Only idle
Sched->>Task: idle_loop_continues
end
deactivate Sched
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes The kernel scheduler logic is non-trivial, introducing idle as a fallback across multiple dispatch paths ( Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 implements a structural fix for an idle-dispatch regression surfaced by QEMU smoke testing. The fix introduces a dedicated fallback slot for the idle task in the scheduler, ensuring it is only dispatched when the ready queue is empty, thus preventing it from displacing real tasks. The implementation includes a new register_idle function, updates to the dispatcher's fallback logic, and new host tests to guard against the regression. I have reviewed the code and suggested simplifications to the dispatch logic in start_prelude and yield_now to improve readability and maintainability.
Comment on lines
+545
to
551
| let next_handle = match s.ready.dequeue() { | ||
| Some(h) => h, | ||
| None => match s.idle { | ||
| Some(idle_h) => idle_h, | ||
| None => panic!("scheduler start called with empty ready queue and no idle task"), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The dispatch logic can be simplified by using Option::or and Option::expect to handle the fallback to the idle task, which improves readability while maintaining the same panic semantics.
#[allow(
clippy::panic,
reason = "empty ready queue and no idle task is a kernel programming error \
(BSP failed to register any task at boot)"
)]
let next_handle = s.ready.dequeue().or(s.idle).expect("scheduler start called with empty ready queue and no idle task");
Comment on lines
691
to
709
| let next_handle = match s.ready.dequeue() { | ||
| Some(h) if h != current_handle => h, | ||
| _ => { | ||
| // Only one ready task exists. The queue is transiently empty | ||
| // and `s.current` is unchanged. The next yield will re-enqueue | ||
| // the current task; no switch is performed here. | ||
| Some(_) => { | ||
| // Dequeue returned current itself (case a). No other ready | ||
| // task. Stay on current — nothing to switch to. Idle is | ||
| // not a useful fallback here because there is no event | ||
| // pending that idle's WFI could service. | ||
| return Ok(()); | ||
| } | ||
| None => match s.idle { | ||
| Some(idle_h) if idle_h != current_handle => idle_h, | ||
| _ => { | ||
| // Either: queue empty and idle is None (no fallback | ||
| // exists; nothing to switch to); or idle is current | ||
| // (case b). Either way, no switch. | ||
| return Ok(()); | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The dispatch logic in yield_now can be simplified by chaining dequeue and or to resolve the next task, reducing the nesting level and improving readability.
let next_handle = s.ready.dequeue().or(s.idle);
let next_handle = match next_handle {
Some(h) if h != current_handle => h,
_ => {
// Dequeue returned current itself (case a) or fallback resolved
// to current (case b), or no fallback exists.
// Stay on current — nothing to switch to.
return Ok(());
}
};![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md (1)
283-283: 💤 Low valueStale claim — pre-dates CI discovery; acknowledged in the companion artifact.
The
*CI absence* … **This is the largest documented gap in the project**sentence was accurate when the plan was written but CI existed at.github/workflows/ci.ymlsince 2026-04-23. The comprehensive review's §Plan-level amendments already records this discrepancy. No action required here, but consider appending a one-line amendment note at the bottom of §12 so future readers find the correction in the plan itself rather than needing to cross-reference the comprehensive artifact.🤖 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/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md` at line 283, The sentence "*CI absence:* no GitHub Actions / circle / etc. workflow at HEAD. **This is the largest documented gap in the project.**" in §12 is now stale; append a one-line amendment at the bottom of §12 noting that CI was added at .github/workflows/ci.yml on 2026-04-23 and reference the comprehensive review's §Plan-level amendments which records this correction so future readers can find the update without cross-referencing other artifacts.
🤖 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 `@docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md`:
- Around line 97-98: Adjust the contradictory status by making the
UNSAFE-2026-0021 status consistent with the other items: update the mention in
the Phase 3/adjustment list (specifically where "adjustment item 6" is marked ✅)
and the later paragraph that currently says 0021 remains open so both places
report UNSAFE-2026-0019/0020 and UNSAFE-2026-0021 with the same final state;
ensure the text around "Phase 3" and the paragraph referencing UNSAFE-2026-0021
use identical wording (either mark 0021 as done like 0019/0020 or explicitly
mark all three as still open) to resolve the inconsistency.
In `@docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md`:
- Around line 11-16: The markdown fenced code blocks are missing language tags;
update each block by adding appropriate languages to fix MD040: tag the shell
command blocks (the ones starting with "git grep -i 'umbrix'" and the one
starting with "grep -rn -E '// (TODO|FIXME|HACK|XXX)'" / "grep -rn -i -E
'(TODO|FIXME|HACK|XXX)'") with ```sh and tag the TOML snippet (the block
containing lines like "unreachable_pub = \"warn\"", "unused_must_use =
\"deny\"", "missing_docs = \"warn\"") with ```toml so the linter stops flagging
MD040 and syntax highlighting is preserved.
In `@docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md`:
- Around line 111-117: The review-history table has an unclosed row (the entry
starting "2026-05-06 | `@cemililik` (+ Claude Opus 4.7 agent) | Promoted `Draft →
In Progress`." ) which breaks Markdown rendering before the fenced code block;
fix it by closing that row properly (ensure the trailing pipe '|' is present and
the row has the same number of '|' separators as the header) and add a blank
line between the table and the following fenced code block so the block renders
correctly.
In `@docs/audits/unsafe-log.md`:
- Around line 358-359: Add an append-only amendment under the UNSAFE-2026-0014
entry stating (with the post-T-014 date) that the audit now includes the new
raw-pointer site register_idle: note that post-T-014 smoke confirms init MMIO
and IRQ-unmask behavior but that full verification of
acknowledge/end_of_interrupt remains pending because the v1 demo never arms an
arm_deadline; mark coverage as partial verification for register_idle and retain
"Pending QEMU smoke verification" until a preemption-using task exercises the
IRQ path.
In `@kernel/src/sched/mod.rs`:
- Around line 535-549: Update the rustdoc for the start-path (start_prelude /
start) to reflect ADR-0026: explain that when s.ready.dequeue() returns None the
scheduler will fall back to s.idle and only panic if both the ready queue is
empty and s.idle is None; replace any wording that claims start/start_prelude
always panics on an empty ready queue with this new contract and reference the
dispatch decision using s.ready.dequeue() and s.idle in the doc text.
- Around line 41-43: Replace the non-canonical repo slug in the rustdoc links so
they point to the project's canonical GitHub repository instead of
"cemililik/TyrneOS": update the three module-level doc links referencing
ADR-0022, ADR-0026 and the docs/analysis report (the strings containing
"github.com/cemililik/TyrneOS/...") to use the canonical slug used by this
repository; apply the same substitution for the matching additions in
bsp-qemu-virt/src/main.rs and any other occurrences (search for "ADR-0022",
"ADR-0026" and "docs/analysis/reviews/...2026-05-06-B1-smoke-regression.md" to
locate each instance).
- Around line 449-452: The release build currently allows a second call to
Scheduler::register_idle to silently overwrite s.idle because the debug_assert
vanishes; change register_idle (and the similar block around the second
occurrence) to return a Result or explicitly check and panic/return an Err on
duplicate registration even in release builds by replacing debug_assert! with an
unconditional check that either returns an error or aborts (e.g., using panic!
or returning Err) when s.idle is already Some, and ensure callers of
register_idle handle the Result or that the function's contract documents the
fatal behavior.
---
Nitpick comments:
In
`@docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md`:
- Line 283: The sentence "*CI absence:* no GitHub Actions / circle / etc.
workflow at HEAD. **This is the largest documented gap in the project.**" in §12
is now stale; append a one-line amendment at the bottom of §12 noting that CI
was added at .github/workflows/ci.yml on 2026-04-23 and reference the
comprehensive review's §Plan-level amendments which records this correction so
future readers can find the update without cross-referencing other artifacts.
🪄 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: d8ad59f0-6846-4434-9e24-abc0e07a0344
📒 Files selected for processing (28)
bsp-qemu-virt/src/main.rsdocs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/00-preflight.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-a-kernel.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-b-hal.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-c-security.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-d-performance.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-e-docs.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-f-tests.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-g-bsp.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-h-infra.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-i-integration.mddocs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.mddocs/analysis/reviews/code-reviews/README.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.mddocs/architecture/scheduler.mddocs/audits/unsafe-log.mddocs/decisions/0022-idle-task-and-typed-scheduler-deadlock.mddocs/decisions/0026-idle-dispatch-fallback.mddocs/decisions/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mdkernel/src/sched/mod.rstools/run-qemu.sh
Comment on lines
+41
to
+43
| //! [ADR-0022]: https://github.com/cemililik/TyrneOS/blob/main/docs/decisions/0022-idle-task-and-typed-scheduler-deadlock.md | ||
| //! [ADR-0026]: https://github.com/cemililik/TyrneOS/blob/main/docs/decisions/0026-idle-dispatch-fallback.md | ||
| //! [`docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md`]: https://github.com/cemililik/TyrneOS/blob/main/docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md |
There was a problem hiding this comment.
Switch the new rustdoc links to the canonical repo slug.
These ADR/report links still point at cemililik/TyrneOS, which the hygiene sweep in this PR already calls out as non-resolving. Leaving the new ADR-0026 references in that form makes the generated rustdoc links broken immediately. Please sweep the matching additions in bsp-qemu-virt/src/main.rs as well.
Also applies to: 196-196, 231-231, 465-465
🤖 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/sched/mod.rs` around lines 41 - 43, Replace the non-canonical repo
slug in the rustdoc links so they point to the project's canonical GitHub
repository instead of "cemililik/TyrneOS": update the three module-level doc
links referencing ADR-0022, ADR-0026 and the docs/analysis report (the strings
containing "github.com/cemililik/TyrneOS/...") to use the canonical slug used by
this repository; apply the same substitution for the matching additions in
bsp-qemu-virt/src/main.rs and any other occurrences (search for "ADR-0022",
"ADR-0026" and "docs/analysis/reviews/...2026-05-06-B1-smoke-regression.md" to
locate each instance).
Address inline + nitpick comments on PR #12 from coderabbitai and gemini-code-assist. Each finding verified against current code; 8 applied, 2 skipped with reason. ## Applied (8) 1. **B1 mini-retro UNSAFE-2026-0021 status alignment** ([docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md](docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md)) — clarify that adjustment item 6 is *partially complete*: 0019/0020 received post-T-014 Amendments while 0021 stayed in its no-verification state; remove the inconsistency between line 97 and lines 124-126. 2. **track-j-hygiene fenced code blocks gain language tags** ([docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md](docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md)) — three blocks now tagged `sh` / `toml` / `sh` (lines 11-16, 44-48, 58-63). Closes markdownlint MD040. 3. **T-014 review-history broken table row + code block fix** ([docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md](docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md)) — the row at line 115 had a fenced code block embedded inside a table cell, breaking markdown rendering. Compressed the row to a one-liner with closing pipe; moved the smoke trace + audit-log Amendments paragraph out of the table into a new `## Verification artefacts` section directly below. Closes markdownlint MD055. 4. **UNSAFE-2026-0014 register_idle Amendment** ([docs/audits/unsafe-log.md](docs/audits/unsafe-log.md)) — append-only Amendment names `register_idle` as the fourth sanctioned site of the momentary-`&mut Scheduler<C>` pattern (alongside `add_task`, `start_prelude`, and the IPC bridge). Records the post-T-014 smoke verification status and the unconditional `assert!` discipline (see #6 below). Mirrors the 2026-04-27 `start_prelude` Amendment and the 2026-04-28 `irq_entry` Amendment shape. 5. **URL slug fix on the 8 lines added in this PR** (kernel/src/sched/mod.rs lines 42, 43, 196, 231, 465, 1479; bsp- qemu-virt/src/main.rs lines 260, 705) — `cemililik/TyrneOS` → `cemililik/Tyrne`. Pre-existing `TyrneOS` URLs in unmodified parts of these files (and the other 60+ across 27 files) stay untouched — they are scoped to a separate post-B1-closure doc-fix sweep per Track J's J-NB1 finding. 6. **register_idle hardened: `debug_assert!` → unconditional `assert!`** ([kernel/src/sched/mod.rs](kernel/src/sched/mod.rs) — `register_idle` body) — the single-idle invariant is load-bearing for ADR-0026's dispatch chain; a release-build silent overwrite would let the previously-registered idle task re-enter normal scheduling on its next `yield_now`. Replace `debug_assert!` with an unconditional `assert!` wrapped in `#[allow(clippy::panic, reason = ...)]`. The function's `# Panics` doc-comment updated to describe the unconditional behaviour (was "panics in debug builds; release silently overwrites" — now "panics unconditionally in both debug and release"). 7. **start_prelude / start `# Panics` rustdoc updated for ADR-0026** ([kernel/src/sched/mod.rs](kernel/src/sched/mod.rs)) — the docs previously said "panics if the ready queue is empty"; the new contract is "panics if ready queue empty AND `s.idle` is None". `start_prelude` and `start` doc-comments now reference both halves of the dispatch chain explicitly. Notes that registering only an idle task (via `register_idle`, no `add_task`) is sufficient to avoid the panic — the post-T-014 "minimum viable boot" shape. 8. **Comprehensive review plan §12 amendment for stale CI claim** ([docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md](docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md)) — the plan was written assuming "no GitHub Actions workflow at HEAD"; Track H's actual run discovered `.github/workflows/ci.yml` was added 2026-04-23. Append a one-line plan-level amendment recording the correction so future readers find it without cross-referencing the merged artefact (which carries the same correction in its verdict). ## Skipped (2 style nits) - **gemini-code-assist on `start_prelude` line 551** — proposed `s.ready.dequeue().or(s.idle).expect("…")` simplification. Skipped: current `match`-arm shape with explicit `Some(h)` / `Some(idle_h)` / `None` branches is more readable in context (mirrors ADR-0026 §Decision outcome's prose explicitly enumerating the dispatch fallback chain), and the panic message is identical between the two forms. - **gemini-code-assist on `yield_now` line 709** — proposed coalescing the two-level `match` into a single `match next_handle`. Skipped: current arms carry case-comments distinguishing "dequeue returned current itself (case a)" from "fallback resolved to current (case b)", which the simplified form would lose. The branch-by-branch comments are documentation that ADR-0026's queue-state simulation table relies on; coalescing them is a regression on readability. ## Verification `cargo fmt --check` clean; `cargo host-clippy` clean (`-D warnings`); `cargo kernel-clippy` clean; `cargo host-test` 152/152; `cargo +nightly miri test` 152/152 clean; `cargo kernel-build` clean. QEMU smoke reproduces the full demo trace through `tyrne: all tasks complete` (boot-to-end ~7.9 ms; `-d int,unimp,guest_errors` empty). Refs: ADR-0026, T-014, PR #12 Audit: UNSAFE-2026-0014 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
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>
8 tasks
cemililik
added a commit
that referenced
this pull request
May 8, 2026
…losure status Re-verified all 13 §Follow-up backlog items from the 2026-05-07 PR #12-#17 multi-axis review against the current integration-branch state. All 9 hygiene items + the 1 forward-flagged P10 harness item are now closed; 3 remain forward-flagged on appropriate downstream venues. Two updates to the consolidated review file: 1. **Top-of-file closure-status banner** — readers see the per-item disposition at-a-glance without scrolling to the bottom backlog. 2. **§Follow-up backlog per-item closure annotations** — items 1-9 gain ✅ + closing-PR + closing-commit references; item 11 (P10 harness) gains ✅ + integration-PR reference + measured-baseline numbers; items 10/12/13 keep their forward-flagged status with "status unchanged 2026-05-08" markers. Re-verification at integration-branch HEAD (per item): | # | Item | Closing PR / commit | Verification | |---|---|---|---| | 1 | current.md + perf re-baseline `.text 22,020` | PR #18 / `94a6c0f` | grep "22,020 bytes" current.md → 1 hit | | 2 | cancel_recv_on_recv_complete test | PR #18 / `25854a1` | grep test name in ipc/mod.rs → 1 hit; host-test 159/159 | | 3 | ipc_cancel_recv doc-rider on cap-bearing state | PR #18 / `25854a1` | grep "destroy-drain callers (Phase B2+)" → 1 hit | | 4 | cancel-block SAFETY wording | PR #18 / `25854a1` | grep "caller_table.*shared.*reborrow" → 1 hit | | 5 | UNSAFE-2026-0014 SHA back-fill (c30f4ee, 7a402cb) | PR #18 / `94a6c0f` | grep both SHAs in unsafe-log.md → 2 hits each | | 6 | unsafe-policy.md §3 mechanical-edit exemption | PR #18 / `94a6c0f` | grep "Mechanical-edit exemption" → 1 hit | | 7 | ADR-0026 §Simulation chronology rider | PR #18 / `94a6c0f` | grep "§Simulation rule was retro-extracted" → 1 hit | | 8 | master-plan AC cross-reference | PR #18 / `94a6c0f` | grep "Closure-trio coordination cross-reference" in security + perf master-plans → 1 hit each | | 9 | ADR-0026 §skill-clause reconciliation rider | PR #18 / `94a6c0f` | grep "single-commit Propose+Accept landing reconciliation" → 1 hit | | 11 | P10 wall-clock harness | this integration PR (replaces #19/#20/#21) | tools/perf-harness.sh exists; baseline report exists; band p10=3.884/p50=4.642/p90=5.584 ms | Forward-flagged (status unchanged): - Item 10: RecvWaiting waiter-identity gap — ADR-0030 / ADR-0019 venue - Item 12: cancel-on-cap-bearing-state destroy-drain ADR — first userspace-destroy task venue - Item 13: B5+ preemption-rollback re-validation of ADR-0032 — B5+ preemption ADR venue This commit only touches the consolidated review's annotation; track files preserved as historical artefacts (their per-track verdicts are the snapshot at the moment of the review, not subject to back-edits). The review's per-item findings (Track-A NIT-2 SchedQueue::new doc rename; Track-G MIN-G1/G2/G3; Track-H MIN-1/MIN-2; Track-A MIN-2 ipc_cancel_recv doc-rider; Track-D D1; Track-F §F-1) are all closed in PR #18 + this integration PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
214052d(last commit before this branch) surfaced a kernel-side regression: round-robin FIFO dispatched idle ahead of a just-unblocked receiver and the kernel hung inWFI. Root cause is structural in ADR-0022 §Decision outcome (Option A — idle as a regular ready-queue resident); the defect entered the tree at T-007 (B0) and was inherited unmodified through B1's "implementation complete" claim of 2026-04-28.Scheduler::idle: Option<TaskHandle>slot, dispatched only as a fallback whens.ready.dequeue()returnsNone). ADR-0022's typed-error axis (Option G —SchedError::Deadlockdefensive return +IpcError::PendingAfterResume+start's panic) is preserved unmodified.kernel_entryline + 3 new host tests). StatusIn Reviewawaiting maintainerDoneflip.cargo fmt/host-clippy/kernel-clippy/kernel-buildclean;cargo host-test152/152 (was 149; +3 new tests);cargo +nightly miri test152/152 clean; QEMU smoke produces the full demo trace throughtyrne: all tasks complete(boot-to-end ~6.3 ms;-d int,unimp,guest_errorsempty).Commits
86a14a3docs(refs)— comprehensive multi-agent code review at HEAD214052d(10 parallel tracks + merge). VerdictRequest changeson doc-drift only; informational artifact, findings tracked separately for a post-B1-closure sweep. Lands together because the regression-arc commits cross-reference its artefacts.0f0c97cchore(tools)—tools/run-qemu.shworks under bash 3.2set -u+ empty array (macOS default shell quirk surfaced during the smoke run).10dea48docs(roadmap,audits,analysis)— Phase 1 + Phase 2 of the fix arc: B1 smoke-regression mini-retro, audit-log Amendments on UNSAFE-2026-0019/0020/0021, ADR-0026 Accepted, ADR-0022 superseded callout, T-014 opened as Draft.c30f4eefeat(kernel,bsp)— T-014 implementation:Scheduler::idlefield +register_idleraw-pointer free fn + dispatch-chain updates instart_prelude/yield_now/ipc_recv_and_yield;kernel_entryswitched toregister_idle; three new host tests.a29c8a3docs(architecture,roadmap,audits,analysis)— Phase 3 closure: post-T-014 Amendments on UNSAFE-2026-0019/0020, scheduler.md §Revision notes, T-014 review-history smoke trace, current.md + phase-b.md status flips, mini-retro follow-up tail.QEMU smoke trace (post-fix)
Test plan
cargo host-test— expect 152/152 green.cargo +nightly miri test— expect 152/152 clean.cargo fmt --check,cargo host-clippy,cargo kernel-clippy,cargo kernel-build— all expected clean../tools/run-qemu.shand confirms the trace above (boot-to-end timing may vary a few ms; structure is what matters). Exit withCtrl-Athenx.unblock_after_yield_dispatches_unblocked_receiver_not_idlemechanically replays the same flow.register_idlefollows the ADR-0021 raw-pointer discipline (momentary&mut Scheduler<C>inside an inner block; UNSAFE-2026-0014 audit citation in the// SAFETY:comment).Donein a follow-up commit ondevelopmentand opens the post-fix B1 closure trio (business + consolidated security + performance baseline).Out of scope
docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive.md.arm_deadline(the v1 cooperative IPC demo never does, so the timer-IRQ path remains unexercised regardless of this fix).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
register_idleAPI for BSP idle task registration.Bug Fixes
Documentation