Hygiene PR — PR #12-#17 multi-axis review follow-ups (10 Minor + 2 Nit)#18
Merged
Merged
Conversation
…iew (8 tracks, fan-out) Read-only artefact commit recording the multi-agent review of the 14-day window 2026-04-23 → 2026-05-07 (PR #12 T-014 hotfix → PR #17 T-015). 8 parallel axis agents (kernel correctness, HAL/BSP, security & capability discipline, performance / footprint, documentation drift, test coverage & quality, process & governance, audit-log discipline) running across `298b5d2a..8dc433e`. Verdict: Approve, 7 Minor follow-ups for a single hygiene PR before ADR-0027 drafting; 1 Major forward-flagged for ADR-0030 / ADR-0019 (`RecvWaiting` unit-variant identity gap surfaces with multi-waiter or syscall-ABI exposure); zero Blockers. Headline counts: 0 Blocker, 1 Major (forward-flagged, not v1), 13 Minor, 10 Nit, 15+ Praise. The follow-up backlog lands in subsequent commits on this branch as the hygiene PR. Refs: 2026-05-06-full-tree-comprehensive (prior baseline) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview
Doc-comment + test additions; zero behavioural change. Closes the
kernel-side Minor / Nit findings from the post-merge multi-axis review.
- kernel/src/ipc/mod.rs: ipc_cancel_recv doc-comment gains a forward
rider clarifying that the SendPending / RecvComplete no-op is a
*behavioural* no-op for *recovery* (the v1 Deadlock-rollback shape) —
parked sender messages and in-flight capabilities must survive a
spurious cancel call. The future destroy-drain caller (B2+) needs
different semantics for cap-bearing branches; ADR-0032 §Consequences
§Positive's "Forward-cleanup primitive for endpoint destroy" wording
should not be misread as "destroy is done". (Track A §MIN-2 + Track C
cap-bearing-state consolidated.)
- kernel/src/ipc/mod.rs: new test cancel_recv_on_recv_complete_does_not_
drop_message_or_cap pins the no-op contract for the cap-bearing
RecvComplete branch — drives the endpoint to RecvComplete { msg, cap:
Some(_) } via send-after-recv, calls cancel, asserts the parked
message AND transferred capability both survive. Closes Track F §F-1.
Test count: 158 → 159 host + miri.
- kernel/src/sched/mod.rs: tighten the cancel-block // SAFETY: comment
to mirror UNSAFE-2026-0014's 4th Amendment word choice — `caller_table`
is `&CapabilityTable` (shared/immutable; recovery does not mutate
caller-table state), not `exclusive`. The body's borrow types are
unchanged; only the comment wording. (Track A §NIT-1.)
- kernel/src/sched/mod.rs: SchedQueue::new doc-comment renames
`# Panics (compile-time)` → `# Compile-time errors` for rustdoc-
convention consistency (`const { assert!(...) }` is a build-time
hard error, not a runtime panic). (Track A §NIT-2.)
Verification gates re-run on the hygiene branch: cargo fmt clean,
host-clippy clean (-D warnings), kernel-clippy clean, kernel-build
clean, host-test 159/159, miri 159/159 (3.33s + 13.98s + 4.36s).
Refs: 2026-05-07-pr-12-to-17-multi-axis-review (Track A §MIN-2/§NIT-1/§NIT-2, Track C cap-bearing-state, Track F §F-1)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight doc-side fixes consolidating the hygiene backlog from the multi- axis review. Zero behavioural change. - docs/roadmap/current.md: post-T-015 amendment to B1-closure headline records `.text 22,020 bytes (+228 vs PR-#16 baseline; +112 vs 2026-04-28)`; new 2026-05-07 unsafe audit-status paragraph names UNSAFE-2026-0014's 4th Amendment site (the Deadlock-branch momentary &mut block) and confirms ADR-0032's "zero new audit-log entry" promise held byte-for-byte. (Track D §D1 + Track H cross-track.) - docs/analysis/reviews/performance-optimization-reviews/2026-05-07-B1- closure.md: append-only "Post-T-015 amendment" section with the +228 byte per-symbol attribution table (12 + 216 = 228, byte-exact), monomorphization analysis (ipc_cancel_recv non-generic), error- surface analysis (IpcError unchanged), coverage trade rider, and smoke-variance band promotion of P10 from "queued" to "load-bearing before B2 ADR-0027". (Track D §D1.) - docs/audits/unsafe-log.md: back-fill introducing-commit SHAs in UNSAFE-2026-0014's 3rd Amendment (`c30f4ee` for T-014 / register_idle) and 4th Amendment (`7a402cb` for T-015 / Deadlock-branch). Pattern of record: commits 6d2f725 + 80e2be3. (Track H §MIN-2.) - docs/standards/unsafe-policy.md §3: append a "Mechanical-edit exemption" paragraph allowing repository-wide find-and-replace sweeps (URL renames, localisation, project-name updates) to touch entry bodies in-place provided the substitution is uniform and no semantic field changes. Codified after PR #14's Yüksek → High sweep edited UNSAFE-2026-0016's body in-place — the append-only invariant exists to preserve verification-state-over-time history, not to ossify drifting terminology. (Track H §MIN-1.) - docs/decisions/0026-idle-dispatch-fallback.md: new §Revision notes section with two riders. (i) §Simulation rule chronology: the rule codified in `write-adr` skill §5 commit 77a578a was retro-extracted from this ADR's table on 2026-05-07; ADR-0032 (commit 4aa4b24) is the first ADR drafted under the rule. (ii) Skill-clause reconciliation between write-adr §10 (separate Accept commit) and supersede-adr §7 (solo-phase combined commit): this ADR's combined-commit landing is permitted by §7's reorganisational property (no new state-machine surface; mechanically derivable from ADR-0022's "idle is in the queue" framing). Future *introductory* state-machine ADRs should follow §10's separate-commit form (ADR-0032 is the project's first execution). (Track G §MIN-G1 + §MIN-G2.) - docs/analysis/reviews/security-reviews/master-plan.md + performance-optimization-reviews/master-plan.md: cross-reference rider after §Acceptance criteria pointing at the business master plan's "no closure-trio without recorded smoke" rule. Closure-trio reviews inherit the smoke-trace gate; independent-trigger reviews (threat-model escalation, hypothesis-driven baseline refresh) do not claim closure semantics. (Track G §MIN-G3.) Verification: cargo fmt / host-clippy / kernel-clippy / host-test / miri / kernel-build all clean post-doc-edits. The kernel-side hygiene commit (25854a1) is independent of these doc-side edits; both are needed to close the multi-axis review's Minor backlog. Refs: 2026-05-07-pr-12-to-17-multi-axis-review (Track D §D1, Track G §MIN-G1/§MIN-G2/§MIN-G3, Track H §MIN-1/§MIN-2) 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
You’ve run out of usage credits. Purchase more 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 (8)
📝 WalkthroughWalkthroughThis PR adds a master multi-axis post-merge review and eight track-specific review documents for PRs ChangesPost-Merge Multi-Axis Review Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-d-perf.md`:
- Line 147: Update the markdown link so the target anchors the exact line range
to match the link text: change the URL for the link labeled
`bsp-qemu-virt/src/main.rs:733–745` to include the fragment `#L733-L745` (e.g.,
append `#L733-L745` to the existing `../../../../bsp-qemu-virt/src/main.rs` URL)
so the link navigates directly to lines 733–745.
In
`@docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-e-docs.md`:
- Around line 231-233: Remove the accidental export artifacts from the markdown
body: delete the absolute local path string and the "Co-Authored-By: ..."
trailer from track-e-docs.md (these belong in commit metadata, not the
document); ensure the file contains only intended review content and, if
provenance is needed, add a short relative/path-agnostic note or move co-author
information into the commit message instead.
In
`@docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-f-tests.md`:
- Around line 43-44: Update the report to remove the incorrect claim that
RecvComplete lacks direct coverage: note that the new test
cancel_recv_on_recv_complete_does_not_drop_message_or_cap (added in
kernel/src/ipc/mod.rs) directly exercises RecvComplete, so amend the matrix
entry and Finding §F-1 language at the places referencing no direct coverage
(around the current matrix rows and lines referenced as 43–44 and 134–135) to
state the gap is closed and reference the added test by name.
In
`@docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-g-process.md`:
- Around line 41-46: The fenced code block starting with triple backticks is
missing a language identifier which triggers MD040; update the block header (the
opening ``` line) to include a language label such as text (e.g., change ``` to
```text) so the snippet is explicitly typed and the markdown lint rule is
satisfied.
In `@docs/roadmap/current.md`:
- Line 17: The phrase "closure trio that lands with this PR" is ambiguous over
time; update the sentence in the B1 milestone paragraph to use a fixed PR
reference (replace "lands with this PR" / "this PR" with an absolute PR number,
e.g., "landed in PR `#16`" or the correct PR #) so the doc remains accurate when
later edits touch the file—edit the string containing "closure trio that lands
with this PR" to read "closure trio that landed in PR `#16`" (or the correct PR #)
and ensure surrounding tense/grammar remain consistent.
🪄 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: 17a69228-608b-4698-b5cf-dbe626105429
📒 Files selected for processing (19)
docs/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-b-hal-bsp.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-c-security.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-d-perf.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-e-docs.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-07-pr-12-to-17-multi-axis-review/track-g-process.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-h-audit.mddocs/analysis/reviews/code-reviews/README.mddocs/analysis/reviews/performance-optimization-reviews/2026-05-07-B1-closure.mddocs/analysis/reviews/performance-optimization-reviews/master-plan.mddocs/analysis/reviews/security-reviews/master-plan.mddocs/audits/unsafe-log.mddocs/decisions/0026-idle-dispatch-fallback.mddocs/roadmap/current.mddocs/standards/unsafe-policy.mdkernel/src/ipc/mod.rskernel/src/sched/mod.rs
![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 introduces a comprehensive multi-axis post-merge review for PRs #12 through #17, detailing findings across kernel correctness, security, performance, and documentation. Significant code changes include the implementation of the ipc_cancel_recv recovery primitive and the symmetric Deadlock rollback mechanism in the scheduler. The reviewer identified a widespread issue with incorrect relative paths in documentation links across the new review files and pointed out a redundant link definition in the IPC module.
| | **#14** [`1cd810d9`](https://github.com/cemililik/Tyrne/pull/14) | Repo-wide `TyrneOS → Tyrne` URL rename (64 URLs) + `Yüksek → High` localization sweep (5 docs); review-round caught `cd TyrneOS` orphan + `TyrneOS repository` typo in SECURITY.md (both fixed in PR). | **Minor (H):** UNSAFE-2026-0016's body was edited in-place by the localization sweep, technically violating the introducing-commit-boundary discipline — fix is a small Amendment or a `unsafe-policy.md §3` exemption for mechanical localization. (Track B also notes the brief mis-attributed the `tools/run-qemu.sh` Bash 3.2 fix to PR #14 — actual introducing commit is `0f0c97c` on PR #12's branch.) | | ||
| | **#15** [`e9fa019a`](https://github.com/cemililik/Tyrne/pull/15) | γ code-side polish — Track A/B/F kernel + HAL + test, Track G/I BSP + integration; `register_idle` `debug_assert!` → `assert!`; γ.6 reverted "defensive loop after start()" (clippy::unreachable_code + too_many_lines); 4 line-ref drops + 1 metadata trim in review-round. | **Praise (B):** clean disposition of Track-G #4 by documented-rejection (`-> !` is the type-system belt-and-braces), plus `Aarch64TaskContext == 168` const-assert mirroring `TrapFrame == 192` extends the existing discipline. | | ||
| | **#16** [`95b15aa1`](https://github.com/cemililik/Tyrne/pull/16) | B1 closure trio (business + consolidated security + performance baseline 2026-05-07, replacing the 2026-04-28 trio's load-bearing role); δ items: ADR-0023 Deferred placeholder, ADR-0032 Propose, T-015 Draft, `write-adr` skill §Simulation codification, master-plan AC change "no closure-trio without recorded smoke." | **Minor (G):** the §Simulation rule was retro-extracted, not pre-existing — commit `77a578a` codifies it 84 seconds before commit `4aa4b24` proposes ADR-0032 with a Simulation table. Chronology is honest in commit bodies but artefacts read as if rule pre-existed; one-line rider in ADR-0026 §Revision notes naming the codifying commit closes it. Master-plan AC also landed only in `business-reviews/master-plan.md`; security and performance master-plans not cross-referenced. | | ||
| | **#17** [`8dc433ee`](https://github.com/cemililik/Tyrne/pull/17) | T-015 implementation — ADR-0032 Accept (separate commit per `write-adr` §10), new `ipc_cancel_recv` primitive, `ipc_recv_and_yield` Phase 2 Deadlock branch upgraded to symmetric scheduler + endpoint rollback; 6 new tests (5 IPC + 1 sched); UNSAFE-2026-0014 4th Amendment names the new Deadlock-branch momentary `&mut EndpointArena` + `&mut IpcQueues` site; ADR-0017 §Revision rider records additive recovery primitive (user-observable surface unchanged). | **Major forward-flag (C):** `RecvWaiting` unit-variant identity gap — kernel records no waiter identity; v1's depth-1 cooperative discipline makes it unobservable, but ADR-0030 (syscall ABI) and ADR-0019 (multi-waiter) must address it. ADR-0032 forecasts the `caller: TaskHandle` signature change correctly at [kernel/src/ipc/mod.rs:447-453](../../../kernel/src/ipc/mod.rs#L447). **Tracked, no v1 action.** | |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have two more ../ segments to correctly navigate to the repository root before accessing the kernel directory. Many other links in this document and related review files have similar issues with incorrect relative paths.
Suggested change
| | **#17** [`8dc433ee`](https://github.com/cemililik/Tyrne/pull/17) | T-015 implementation — ADR-0032 Accept (separate commit per `write-adr` §10), new `ipc_cancel_recv` primitive, `ipc_recv_and_yield` Phase 2 Deadlock branch upgraded to symmetric scheduler + endpoint rollback; 6 new tests (5 IPC + 1 sched); UNSAFE-2026-0014 4th Amendment names the new Deadlock-branch momentary `&mut EndpointArena` + `&mut IpcQueues` site; ADR-0017 §Revision rider records additive recovery primitive (user-observable surface unchanged). | **Major forward-flag (C):** `RecvWaiting` unit-variant identity gap — kernel records no waiter identity; v1's depth-1 cooperative discipline makes it unobservable, but ADR-0030 (syscall ABI) and ADR-0019 (multi-waiter) must address it. ADR-0032 forecasts the `caller: TaskHandle` signature change correctly at [kernel/src/ipc/mod.rs:447-453](../../../kernel/src/ipc/mod.rs#L447). **Tracked, no v1 action.** | | |
| | **#17** [`8dc433ee`](https://github.com/cemililik/Tyrne/pull/17) | T-015 implementation — ADR-0032 Accept (separate commit per `write-adr` §10), new `ipc_cancel_recv` primitive, `ipc_recv_and_yield` Phase 2 Deadlock branch upgraded to symmetric scheduler + endpoint rollback; 6 new tests (5 IPC + 1 sched); UNSAFE-2026-0014 4th Amendment names the new Deadlock-branch momentary `&mut EndpointArena` + `&mut IpcQueues` site; ADR-0017 §Revision rider records additive recovery primitive (user-observable surface unchanged). | **Major forward-flag (C):** `RecvWaiting` unit-variant identity gap — kernel records no waiter identity; v1's depth-1 cooperative discipline makes it unobservable, but ADR-0030 (syscall ABI) and ADR-0019 (multi-waiter) must address it. ADR-0032 forecasts the `caller: TaskHandle` signature change correctly at [kernel/src/ipc/mod.rs:447-453](../../../../../kernel/src/ipc/mod.rs#L447). **Tracked, no v1 action.** | |
|
|
||
| ### Minor | ||
|
|
||
| - **MIN-1 (PR #17).** [`kernel/src/ipc/mod.rs::ipc_cancel_recv`](../../../../kernel/src/ipc/mod.rs#L464) takes `ep_arena: &mut EndpointArena` but only calls the immutable `Arena::get(&self, id)` — the `&mut` is stricter than the function actually needs. Symmetric with `ipc_recv` / `ipc_send`'s shape (which also use `&mut EndpointArena` with only `.get(...)` for the live-handle check), so the over-strictness is consistent rather than novel; not a correctness issue. Suggested follow-up: when ADR-0030 (syscall ABI) lands and finalises the IPC entry-point signatures, sweep all four primitives' arena parameters to `&EndpointArena` if no current use mutates the arena. **Severity rationale: minor — symmetric with siblings; cosmetic; calling sites already pass `&mut`-borrowed arenas.** |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have two more ../ segments to correctly navigate to the repository root before accessing the kernel directory.
Suggested change
| - **MIN-1 (PR #17).** [`kernel/src/ipc/mod.rs::ipc_cancel_recv`](../../../../kernel/src/ipc/mod.rs#L464) takes `ep_arena: &mut EndpointArena` but only calls the immutable `Arena::get(&self, id)` — the `&mut` is stricter than the function actually needs. Symmetric with `ipc_recv` / `ipc_send`'s shape (which also use `&mut EndpointArena` with only `.get(...)` for the live-handle check), so the over-strictness is consistent rather than novel; not a correctness issue. Suggested follow-up: when ADR-0030 (syscall ABI) lands and finalises the IPC entry-point signatures, sweep all four primitives' arena parameters to `&EndpointArena` if no current use mutates the arena. **Severity rationale: minor — symmetric with siblings; cosmetic; calling sites already pass `&mut`-borrowed arenas.** | |
| - **MIN-1 (PR #17).** [`kernel/src/ipc/mod.rs::ipc_cancel_recv`](../../../../../../kernel/src/ipc/mod.rs#L464) takes `ep_arena: &mut EndpointArena` but only calls the immutable `Arena::get(&self, id)` — the `&mut` is stricter than the function actually needs. Symmetric with `ipc_recv` / `ipc_send`'s shape (which also use `&mut EndpointArena` with only `.get(...)` for the live-handle check), so the over-strictness is consistent rather than novel; not a correctness issue. Suggested follow-up: when ADR-0030 (syscall ABI) lands and finalises the IPC entry-point signatures, sweep all four primitives' arena parameters to `&EndpointArena` if no current use mutates the arena. **Severity rationale: minor — symmetric with siblings; cosmetic; calling sites already pass `&mut`-borrowed arenas.** |
|
|
||
| ### Minor | ||
|
|
||
| - **B-1 (Minor — doc-vs-code attribution drift in this review's brief, not in the tree):** The brief states the bash-3.2 `set -u` fix in [`tools/run-qemu.sh:67`](../../../../tools/run-qemu.sh#L67) landed in PR #14. The actual introducing commit is `0f0c97c` ("chore(tools): run-qemu.sh works under bash 3.2 set -u + empty array", 2026-05-06 23:18 +0300), which sits on the merge-history of PR #12 (`development` branch, `298b5d2a`) — *before* PR #13 / #14 were even open. PR #14's diff against main contains zero `tools/` changes (`gh pr diff 14 --name-only` returns no `tools/run-qemu.sh` row). PR #13's branch also inherits the commit, since `track-e-doc-fix-sweep` was branched from a tip that already included `0f0c97c`. The fix itself is correct (canonical `${INT_LOG_FLAGS[@]+"${INT_LOG_FLAGS[@]}"}` idiom — see [tools/run-qemu.sh:54-58 + 60-68](../../../../tools/run-qemu.sh#L54)) and the QEMU command-line bytes are unchanged when `INT_LOG_FLAGS` is empty (the conditional expansion produces *no tokens*, identical to the pre-fix `"${INT_LOG_FLAGS[@]}"` *under bash 4+* but no longer crashes under bash 3.2's `set -u`). The smoke-artefact-byte-for-byte-unchanged claim is verified — the only behavioural change is "macOS default bash no longer aborts the script before it gets to `qemu-system-aarch64`". **No tree-side action.** This is a `tracked, no action` annotation for the master review's audit log: the brief's PR attribution is one PR off; the fix itself is healthy. |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have two more ../ segments to correctly navigate to the repository root before accessing the tools directory.
Suggested change
| - **B-1 (Minor — doc-vs-code attribution drift in this review's brief, not in the tree):** The brief states the bash-3.2 `set -u` fix in [`tools/run-qemu.sh:67`](../../../../tools/run-qemu.sh#L67) landed in PR #14. The actual introducing commit is `0f0c97c` ("chore(tools): run-qemu.sh works under bash 3.2 set -u + empty array", 2026-05-06 23:18 +0300), which sits on the merge-history of PR #12 (`development` branch, `298b5d2a`) — *before* PR #13 / #14 were even open. PR #14's diff against main contains zero `tools/` changes (`gh pr diff 14 --name-only` returns no `tools/run-qemu.sh` row). PR #13's branch also inherits the commit, since `track-e-doc-fix-sweep` was branched from a tip that already included `0f0c97c`. The fix itself is correct (canonical `${INT_LOG_FLAGS[@]+"${INT_LOG_FLAGS[@]}"}` idiom — see [tools/run-qemu.sh:54-58 + 60-68](../../../../tools/run-qemu.sh#L54)) and the QEMU command-line bytes are unchanged when `INT_LOG_FLAGS` is empty (the conditional expansion produces *no tokens*, identical to the pre-fix `"${INT_LOG_FLAGS[@]}"` *under bash 4+* but no longer crashes under bash 3.2's `set -u`). The smoke-artefact-byte-for-byte-unchanged claim is verified — the only behavioural change is "macOS default bash no longer aborts the script before it gets to `qemu-system-aarch64`". **No tree-side action.** This is a `tracked, no action` annotation for the master review's audit log: the brief's PR attribution is one PR off; the fix itself is healthy. | |
| - **B-1 (Minor — doc-vs-code attribution drift in this review's brief, not in the tree):** The brief states the bash-3.2 `set -u` fix in [`tools/run-qemu.sh:67`](../../../../../../tools/run-qemu.sh#L67) landed in PR #14. The actual introducing commit is `0f0c97c` ("chore(tools): run-qemu.sh works under bash 3.2 set -u + empty array", 2026-05-06 23:18 +0300), which sits on the merge-history of PR #12 (`development` branch, `298b5d2a`) — *before* PR #13 / #14 were even open. PR #14's diff against main contains zero `tools/` changes (`gh pr diff 14 --name-only` returns no `tools/run-qemu.sh` row). PR #13's branch also inherits the commit, since `track-e-doc-fix-sweep` was branched from a tip that already included `0f0c97c`. The fix itself is correct (canonical `${INT_LOG_FLAGS[@]+${INT_LOG_FLAGS[@]}}` idiom — see [tools/run-qemu.sh:54-58 + 60-68](../../../../../../tools/run-qemu.sh#L54)) and the QEMU command-line bytes are unchanged when `INT_LOG_FLAGS` is empty (the conditional expansion produces *no tokens*, identical to the pre-fix `"${INT_LOG_FLAGS[@]}"` *under bash 4+* but no longer crashes under bash 3.2's `set -u`). The smoke-artefact-byte-for-byte-unchanged claim is verified — the only behavioural change is "macOS default bash no longer aborts the script before it gets to `qemu-system-aarch64`". **No tree-side action.** This is a `tracked, no action` annotation for the master review's audit log: the brief's PR attribution is one PR off; the fix itself is healthy. |
|
|
||
| Adversarial frame: *can a caller invoke `ipc_cancel_recv` without holding the right that authorised the original `ipc_recv` registration?* | ||
|
|
||
| - **Two-layer cap validation present.** **OK** — the first line of [`ipc_cancel_recv`](../../../../../kernel/src/ipc/mod.rs#L470) calls `validate_ep_cap(caller_table, ep_cap, CapRights::RECV)` ([`kernel/src/ipc/mod.rs:485`](../../../../../kernel/src/ipc/mod.rs#L485)). `validate_ep_cap` performs (a) `lookup` on the caller's table → `InvalidCapability` on stale handle, (b) `rights().contains(RECV)` → `InvalidCapability` on missing right, (c) object-kind discriminator match (`CapObject::Endpoint`) → `InvalidCapability` on a notification cap that happens to coexist at the same handle. The arena `get` re-check at [`:472-474`](../../../../../kernel/src/ipc/mod.rs#L472-L474) catches a generation mismatch *between* the lookup and the `state_of` call (TOCTOU window discussed in §5). Symmetric to `ipc_recv`'s validation — the same RECV right authorises the cancel as authorised the original registration. The doc-comment at [`:455-459`](../../../../../kernel/src/ipc/mod.rs#L455-L459) makes the contract explicit. |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have one more ../ segment to correctly navigate to the repository root before accessing the kernel directory.
Suggested change
| - **Two-layer cap validation present.** **OK** — the first line of [`ipc_cancel_recv`](../../../../../kernel/src/ipc/mod.rs#L470) calls `validate_ep_cap(caller_table, ep_cap, CapRights::RECV)` ([`kernel/src/ipc/mod.rs:485`](../../../../../kernel/src/ipc/mod.rs#L485)). `validate_ep_cap` performs (a) `lookup` on the caller's table → `InvalidCapability` on stale handle, (b) `rights().contains(RECV)` → `InvalidCapability` on missing right, (c) object-kind discriminator match (`CapObject::Endpoint`) → `InvalidCapability` on a notification cap that happens to coexist at the same handle. The arena `get` re-check at [`:472-474`](../../../../../kernel/src/ipc/mod.rs#L472-L474) catches a generation mismatch *between* the lookup and the `state_of` call (TOCTOU window discussed in §5). Symmetric to `ipc_recv`'s validation — the same RECV right authorises the cancel as authorised the original registration. The doc-comment at [`:455-459`](../../../../../kernel/src/ipc/mod.rs#L455-L459) makes the contract explicit. | |
| - **Two-layer cap validation present.** **OK** — the first line of [`ipc_cancel_recv`](../../../../../../kernel/src/ipc/mod.rs#L470) calls `validate_ep_cap(caller_table, ep_cap, CapRights::RECV)` ([`kernel/src/ipc/mod.rs:485`](../../../../../../kernel/src/ipc/mod.rs#L485)). `validate_ep_cap` performs (a) `lookup` on the caller's table → `InvalidCapability` on stale handle, (b) `rights().contains(RECV)` → `InvalidCapability` on missing right, (c) object-kind discriminator match (`CapObject::Endpoint`) → `InvalidCapability` on a notification cap that happens to coexist at the same handle. The arena `get` re-check at [`:472-474`](../../../../../../kernel/src/ipc/mod.rs#L472-L474) catches a generation mismatch *between* the lookup and the `state_of` call (TOCTOU window discussed in §5). Symmetric to `ipc_recv`'s validation — the same RECV right authorises the cancel as authorised the original registration. The doc-comment at [`:455-459`](../../../../../../kernel/src/ipc/mod.rs#L455-L459) makes the contract explicit. |
| - **Agent run by:** Claude Opus 4.7 (1M context), 2026-05-07. | ||
| - **Scope:** Performance / footprint axis-pass over the 6 PRs that landed since the 2026-04-28 B1 closure baseline — PR #12 (T-014 idle dispatch), PR #13 (Track-E doc-fix sweep), PR #14 (TyrneOS → Tyrne URL rename + Yüksek → High), PR #15 (kernel/HAL/BSP code polish, including the `register_idle` `debug_assert! → assert!` upgrade and the γ.6 "defensive loop after `start()`" revert), PR #16 (closure-trio docs + the 2026-05-07 perf re-baseline), PR #17 (T-015 / ADR-0032 — `ipc_cancel_recv` + symmetric Deadlock rollback). | ||
| - **HEAD reviewed:** `c258ee3` (T-015 doc-side commit, branch `t-015-endpoint-rollback-cancel-recv`; PR #17 merged at `8dc433e`). | ||
| - **Comparison anchor:** the [2026-05-07 B1-closure perf re-baseline](../../performance-optimization-reviews/2026-05-07-B1-closure.md) (`.text` 21,792 / `.rodata` 2,928 / `.bss` 22,256) — i.e. post-PR-#16 / pre-T-015 / commit `95b15aa`. PRs #12–#16 are absorbed into that baseline; the only delta this Track-D measures live is **PR #17 / T-015**, plus a verification pass over the inputs the re-baseline already integrates. |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have one more ../ segment to correctly navigate up to the reviews directory.
Suggested change
| - **Comparison anchor:** the [2026-05-07 B1-closure perf re-baseline](../../performance-optimization-reviews/2026-05-07-B1-closure.md) (`.text` 21,792 / `.rodata` 2,928 / `.bss` 22,256) — i.e. post-PR-#16 / pre-T-015 / commit `95b15aa`. PRs #12–#16 are absorbed into that baseline; the only delta this Track-D measures live is **PR #17 / T-015**, plus a verification pass over the inputs the re-baseline already integrates. | |
| - **Comparison anchor:** the [2026-05-07 B1-closure perf re-baseline](../../../performance-optimization-reviews/2026-05-07-B1-closure.md) (`.text` 21,792 / `.rodata` 2,928 / `.bss` 22,256) — i.e. post-PR-#16 / pre-T-015 / commit `95b15aa`. PRs #12–#16 are absorbed into that baseline; the only delta this Track-D measures live is **PR #17 / T-015**, plus a verification pass over the inputs the re-baseline already integrates. |
|
|
||
| | Blocker | Location | 2026-05-06 Status | PR #13 Action | Verified | | ||
| |---------|----------|------------------|---|---| | ||
| | GIC version (overview.md) | [`docs/architecture/overview.md:77`](../../../../architecture/overview.md#L77) | `GICv3` contradicts code | `GICv3 → GICv2` + clarifier added | ✓ | |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have one more ../ segment to correctly navigate to the docs directory before accessing architecture.
Suggested change
| | GIC version (overview.md) | [`docs/architecture/overview.md:77`](../../../../architecture/overview.md#L77) | `GICv3` contradicts code | `GICv3 → GICv2` + clarifier added | ✓ | | |
| | GIC version (overview.md) | [`docs/architecture/overview.md:77`](../../../../../architecture/overview.md#L77) | `GICv3` contradicts code | `GICv3 → GICv2` + clarifier added | ✓ | |
| ## References | ||
|
|
||
| - [ADR-0025 — ADR governance amendments: forward-reference contract, rider hygiene](../../../decisions/0025-adr-governance-amendments.md) — §Rule 1, §Rule 2, §Revision notes (cool-down withdrawal). | ||
| - [`.claude/skills/write-adr/SKILL.md`](../../../../.claude/skills/write-adr/SKILL.md) — full procedure, especially §10 separate-commit Accept and §Simulation (added in PR #16 commit `77a578a`). |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have two more ../ segments to correctly navigate to the repository root.
Suggested change
| - [`.claude/skills/write-adr/SKILL.md`](../../../../.claude/skills/write-adr/SKILL.md) — full procedure, especially §10 separate-commit Accept and §Simulation (added in PR #16 commit `77a578a`). | |
| - [`.claude/skills/write-adr/SKILL.md`](../../../../../../.claude/skills/write-adr/SKILL.md) — full procedure, especially §10 separate-commit Accept and §Simulation (added in PR #16 commit `77a578a`). |
| - **Agent:** Claude Opus 4.7 (1M context), 2026-05-07 | ||
| - **Scope:** Audit-log discipline across PR #12 / #13 / #14 / #15 / #16 / #17 merges. `unsafe`-log entries (existence, body completeness, Amendment cadence, introducing-commit-boundary discipline); cross-check that every new `unsafe` block in the six PRs has an audit entry or Amendment; verify every `Pending QEMU smoke verification` status note still reflects post-T-014 / post-T-015 reality. | ||
| - **Merge SHA range:** `298b5d2a..8dc433e` on `main`. | ||
| - **Key artefacts cross-checked:** [`docs/audits/unsafe-log.md`](../../../audits/unsafe-log.md), [`docs/standards/unsafe-policy.md`](../../../standards/unsafe-policy.md), [`docs/roadmap/current.md`](../../../roadmap/current.md), [`kernel/src/sched/mod.rs`](../../../../kernel/src/sched/mod.rs), [`kernel/src/ipc/mod.rs`](../../../../kernel/src/ipc/mod.rs), [`bsp-qemu-virt/src/{main.rs,exceptions.rs,gic.rs,cpu.rs,console.rs,boot.s,vectors.s}`](../../../../bsp-qemu-virt/src/), [`hal/src/{cpu.rs,context_switch.rs,mmu.rs}`](../../../../hal/src/), the 2026-05-06 [`track-c-security.md`](../../2026-05-06-full-tree/track-c-security.md) for the Pending-smoke history. |
There was a problem hiding this comment.
This relative link appears to be incorrect. Given the file's location, the path should likely have two more ../ segments to correctly navigate to the docs directory before accessing audits.
Suggested change
| - **Key artefacts cross-checked:** [`docs/audits/unsafe-log.md`](../../../audits/unsafe-log.md), [`docs/standards/unsafe-policy.md`](../../../standards/unsafe-policy.md), [`docs/roadmap/current.md`](../../../roadmap/current.md), [`kernel/src/sched/mod.rs`](../../../../kernel/src/sched/mod.rs), [`kernel/src/ipc/mod.rs`](../../../../kernel/src/ipc/mod.rs), [`bsp-qemu-virt/src/{main.rs,exceptions.rs,gic.rs,cpu.rs,console.rs,boot.s,vectors.s}`](../../../../bsp-qemu-virt/src/), [`hal/src/{cpu.rs,context_switch.rs,mmu.rs}`](../../../../hal/src/), the 2026-05-06 [`track-c-security.md`](../../2026-05-06-full-tree/track-c-security.md) for the Pending-smoke history. | |
| - **Key artefacts cross-checked:** [`docs/audits/unsafe-log.md`](../../../../../audits/unsafe-log.md), [`docs/standards/unsafe-policy.md`](../../../../../standards/unsafe-policy.md), [`docs/roadmap/current.md`](../../../../../roadmap/current.md), [`kernel/src/sched/mod.rs`](../../../../../../kernel/src/sched/mod.rs), [`kernel/src/ipc/mod.rs`](../../../../../../kernel/src/ipc/mod.rs), [`bsp-qemu-virt/src/{main.rs,exceptions.rs,gic.rs,cpu.rs,console.rs,boot.s,vectors.s}`](../../../../../../bsp-qemu-virt/src/), [`hal/src/{cpu.rs,context_switch.rs,mmu.rs}`](../../../../../../hal/src/), the 2026-05-06 [`track-c-security.md`](../../2026-05-06-full-tree/track-c-security.md) for the Pending-smoke history. |
| /// [adr-0032]: https://github.com/cemililik/Tyrne/blob/main/docs/decisions/0032-endpoint-rollback-and-cancel-recv.md | ||
| /// [ADR-0017]: https://github.com/cemililik/Tyrne/blob/main/docs/decisions/0017-ipc-primitive-set.md | ||
| /// [ADR-0019]: https://github.com/cemililik/Tyrne/blob/main/docs/decisions/0019-scheduler-shape.md | ||
| /// [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.
This link definition for [ADR-0032] is a duplicate of the one on line 479. To improve maintainability and reduce redundancy, you can remove this line.
`.claude/scheduled_tasks.lock` is Claude Code's per-session coordination file (sessionId / pid / timestamps) used by tools like ScheduleWakeup. Per-machine, per-session; no value being tracked. The `.claude/skills/` tree remains intentionally tracked because project skills are source of truth (Anthropic skill convention) — only runtime lock files are ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All five inline findings from coderabbitai's review of PR #18 verified against current state and applied. (qodo + sourcery rate-limit / cap prevented their input on this PR.) - track-d-perf.md L147: append `#L733-L745` to the bsp-qemu-virt main.rs link so the URL fragment matches the link text. - track-e-docs.md L229-233: remove accidental export artefacts (absolute local path + `Co-Authored-By:` trailer) — these belong in commit metadata, not the review body. - track-f-tests.md L43-44 + L134-138: §F-1 RecvComplete coverage gap is now closed by PR #18 commit `25854a1`'s new `cancel_recv_on_recv_complete_does_not_drop_message_or_cap` test; matrix entry updated and Finding §F-1 moved to "Minor (closed)" with a note that the new test is *stronger* than originally recommended (pins cap-bearing-state property as well). - track-g-process.md L41: fenced code block gains `text` language identifier to satisfy MD040. - current.md L17: "closure trio that lands with this PR" → "closure trio that landed in PR #16 (`95b15aa`)" — the ambiguous "this PR" reading was load-bearing only at the moment PR #16 was open; now permanent history requires a fixed reference. No code changes; no gate impact. fmt clean. Refs: PR #18 review-round (coderabbitai) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-f-tests.md (1)
80-80: 💤 Low valueOptional: Simplify nested code spans for cleaner markdown.
The comment reference contains nested backticks that trigger a markdown linting warning (MD038). Consider moving the code identifier outside the quoted comment text for cleaner formatting.
📝 Suggested markdown formatting
- - **First momentary `&mut`**: scoped block `{ let s = unsafe { &mut *sched }; ...; }` (lines [939-969](../../../../../kernel/src/sched/mod.rs#L939-L969)). Drops at end of block — explicit `// `s: &mut Scheduler<C>` drops here.` comment at [line 969](../../../../../kernel/src/sched/mod.rs#L969). + - **First momentary `&mut`**: scoped block `{ let s = unsafe { &mut *sched }; ...; }` (lines [939-969](../../../../../kernel/src/sched/mod.rs#L939-L969)). Drops at end of block — explicit comment `// s: &mut Scheduler<C> drops here.` at [line 969](../../../../../kernel/src/sched/mod.rs#L969).🤖 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-07-pr-12-to-17-multi-axis-review/track-f-tests.md` at line 80, The markdown line contains nested backticks around the snippet "`// `s: &mut Scheduler<C>` drops here.`" which triggers MD038; edit the text in docs/analysis/.../track-f-tests.md to remove the inner backticks and present the code identifier cleanly (e.g., place s: &mut Scheduler<C> as its own inline code span or outside the quoted comment), so the comment reads without nested backticks and still references the scoped block `{ let s = unsafe { &mut *sched }; ... }` and the drop comment clearly.
🤖 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.
Nitpick comments:
In
`@docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-f-tests.md`:
- Line 80: The markdown line contains nested backticks around the snippet "`//
`s: &mut Scheduler<C>` drops here.`" which triggers MD038; edit the text in
docs/analysis/.../track-f-tests.md to remove the inner backticks and present the
code identifier cleanly (e.g., place s: &mut Scheduler<C> as its own inline code
span or outside the quoted comment), so the comment reads without nested
backticks and still references the scoped block `{ let s = unsafe { &mut *sched
}; ... }` and the drop comment clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcde69fe-ac4f-40c8-8bba-3ceb93b85fb4
📒 Files selected for processing (6)
.gitignoredocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-d-perf.mddocs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-e-docs.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-07-pr-12-to-17-multi-axis-review/track-g-process.mddocs/roadmap/current.md
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review/track-e-docs.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/roadmap/current.md
…MD038) track-f-tests.md L80 had a nested-backtick code-span that triggered MD038 — the outer code-span's inner backticks around `s: &mut Scheduler<C>` opened a second span inside the first. Removed the inner backticks so the comment renders as a single inline code span: `// s: &mut Scheduler<C> drops here.` — content unchanged, only the backtick nesting was wrong. Refs: PR #18 review-round (coderabbitai nitpick) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…k def (8 of 10) Verified each gemini-code-assist finding against current state: **Applied (7 path corrections + 1 duplicate link def removal):** - Fixed off-by-one relative-path drift in PR #18's new review files — ~93 broken paths across the main review file + 5 track files (track-a, -b, -d, -g, -h). Repo-root targets (kernel/, tools/, bsp-qemu-virt/, hal/, .claude/) needed +1 `../`; docs-relative targets (audits/, standards/, decisions/, roadmap/, architecture/, glossary) also needed +1 `../` where they were drifted. Track files now resolve with 5 `../` to repo root + 4 `../` to docs/; main review file uses 4 `../` to repo root + 3 `../` to docs/. - kernel/src/ipc/mod.rs: removed duplicate `[ADR-0032]` link def at L482 (uppercase variant). The lowercase `[adr-0032]` def at L479 is the only one referenced by the body's `[ADR-0032][adr-0032]` and resolves case-insensitively per CommonMark. **Skipped (3 gemini findings — current state already correct):** - track-c-security.md:14 — track-c uses 5 `../` already (correct); gemini's "add 2 more `../`" would over-correct to 7 (above repo root). - track-d-perf.md:6 — `../../performance-optimization-reviews/...` is sibling-directory navigation (2 `../`); gemini's "add one more" over-corrects. - track-e-docs.md:28 — `../../../../architecture/overview.md` is 4 `../` resolving to docs/architecture/ (correct); gemini's "add one more" over-corrects above docs/. **Note on gemini's specific suggestions:** Each of the 7 applied findings flagged a real broken path, but gemini's suggested `../` count in each case over-corrected by exactly 1 (suggested 6 where 5 was right; suggested 5 where 4 was right). Applied the spirit of each finding with the verified-correct count via `realpath`-based path math. The same broken-path drift exists in the prior 2026-05-06 comprehensive review files (~120 broken paths there); fixing those is out of scope for this PR — separate doc-hygiene PR. Verification: cargo fmt / host-clippy / kernel-clippy / host-test (159) / kernel-build all clean post-sweep. Refs: PR #18 review-round (gemini-code-assist) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@gemini-code-assist Verified each path-related finding against current state via Applied (with verified-correct
Skipped (current state already correct; gemini's suggestion over-corrects):
Note on The same drift exists in the prior 2026-05-06 comprehensive review files (~120 broken paths there); that's out of scope for this PR and queued as a separate doc-hygiene cleanup. |
|
Thank you for the detailed verification and the summary of the applied hygiene follow-ups. Your diligence in cross-referencing the path-related findings with Regarding the findings that were skipped, your reasoning for the over-correction in the suggested I have noted your plan to address the remaining drift from the 2026-05-06 comprehensive review in a separate hygiene PR. This approach keeps the current pull request focused and manageable. Thank you for your continued commitment to maintaining the project's documentation and audit standards. |
3 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>
cemililik
added a commit
that referenced
this pull request
May 8, 2026
… 2026-05-08 + 2026-05-07 follow-ups Closes the 12 remaining items flagged by the 2026-05-08 multi-axis review's §Follow-up backlog (3 Track-2 Majors, 1 Track-3 Major fix already closed in `59c08e9`, 6 Track-3 / Track-2 Minors, 2 Track-4 Minors, 2 Track-2 Nits) plus the carry-forward 2026-05-07 Track-H NIT-1. **Track 2 Majors (forward-flagged → ADR-0027 / phase-b ledger riders):** - M1 (escape-hatch doc): ADR-0027 §Decision outcome (c) gains a bullet documenting `mem::forget` / `ManuallyDrop` / `let _ = ...` as deliberate-but-rare escape hatches, mirroring the `x86_64::structures::paging::MapperFlush` precedent. - M2 (MMU-instance binding): ADR-0027 §Decision outcome (c) gains a bullet noting `MapperFlush::flush(self, mmu: &impl Mmu)` accepts any `Mmu`; multi-`Mmu` deployments (B3+ per-task `AddressSpace`, Phase C multi-CPU) will need a stronger token type. Out of scope for v1. - M3 (ADR-0034 placeholder): ADR-0027 §Decision outcome adds an "ADR-0034 (kernel-image section permissions) placeholder" block alongside ADR-0033, and `phase-b.md` ADR ledger gains rows for both ADR-0033 and ADR-0034 with named-but-unallocated discipline. **Track 3 Minors (governance / wording polish):** - m1 + m2 (current.md L52): drop "Phase-2" prefix on "§Simulation table" (the table walks Steps 0–4, not a "Phase 2"); "Accept will be" → "Accept landed as" + actual commit SHA `bb0a6ba`. - m3 (commit-style.md PR-numbering rider): new §"PR-number references in committed artefacts" subsection naming the recurrence (PR #18 + PR #20 each had a one-commit PR-number fix-up) and codifying three acceptable disciplines (defer banner authoring; reference branch slug; or use commit SHA). - n1 (framing alignment): "first to apply Simulation forward" wording in current.md (banner + Active decisions row) and phase-b.md §B2 status block aligned to the precise "first non-recovery-primitive state-machine ADR drafted under §Simulation" phrasing — ADR-0032's Propose did land with a table; the prior framing was technically defensible only under a narrow reading of "retro-extracted". **Track 2 Nits (substance riders in ADR-0027):** - #4 (DSB ISH vs DSB NSH rationale): §Simulation gains a rationale paragraph after the table — `ISH` is forward-compatible with the eventual SMP boot, sub-microsecond cost on single-core, matches Linux aarch64 `arch/arm64/mm/proc.S` for the same reason. - #5 (TCR_EL1.AS wording tighten): line 59 reworded — `AS = 0` selects 8-bit ASID *size*, not "the ASID value is 0" (the value is `TTBR0_EL1.ASID = 0` and is what's "globally used in v1"). - #7 (line 17 §-citation precision) + Track-3 n1: "first non-recovery-primitive state-machine" framing now precise. - #8 (memory-management.md L88 page-table descriptor cosmetic): ASCII bit-field diagram redrawn to match L2 block-descriptor reality (OutputAddress[47:21], not [47:12]); explanatory note added for L1-block / L3-page variants. **Track 4 Minors (perf-harness.sh):** - #1 (Ctrl-C cleanup trap): new `cleanup_in_flight` shell function + `trap '...' EXIT INT TERM` that kills any in-flight QEMU + watchdog PIDs tracked in `CURRENT_CMD_PID` / `CURRENT_WATCHDOG_PID` shell globals. `run_with_timeout` updates the globals at every call so the trap addresses whichever pair is currently in flight; clears them at every clean exit so the trap is a no-op outside iterations. - #2 (p99 small-N reporting hygiene): generated baseline report Methodology section gains a "**Note on p99 at small `n`**" paragraph explaining that under nearest-rank `p99 == max` for `n < 100` and callers should not over-read it as a tail-latency signal until `n >= 100`. **Track 4 Nit #3 (read_stats refactor):** **Already closed by PR #21 review-round commit `ef30b5c`** — the 8 `echo | awk` parses became a single `while read` loop. Verified at HEAD (`grep -c "while read -r key val" tools/perf-harness.sh` → 1 hit). **2026-05-07 Track-H NIT-1 (Pending Amendment closure-path indexing):** UNSAFE-2026-0019 / 0020 / 0021 each gain a 2026-05-08 "closure-path indexed" Amendment naming the canonical clearance trigger (B5 Milestone, ADR-0030 entry-point, deadline-arming syscall) explicitly, so a future reader of `unsafe-log.md` alone has the full picture without leaving the file. No semantic change; co-locates information that was previously distributed across `phase-b.md` cross-references. Verification gates re-run on the integration branch: - `cargo fmt --all -- --check` clean - `cargo host-test` 159/159 (25 + 100 + 34) - `cargo host-clippy` clean (-D warnings) - `cargo kernel-clippy` clean - `cargo kernel-build` clean - `tools/perf-harness.sh --iterations=3` runs end-to-end with the new trap + Methodology note + while-read parsing intact This commit + the prior 2026-05-08 review's recommendations close all follow-ups identified by both 2026-05-07 and 2026-05-08 multi-axis reviews. Forward-flagged items (10/12/13 from 2026-05-07) and any review-round bot input on this integration branch remain the only open items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 8, 2026
Closes the orchestrator review's findings on PR #22 (16 actionable items; one item — PR #19 description "180/180" wording — not closeable post-merge because the gh PR is already closed and its description is immutable from this branch). **3 Majors (fix-on-branch):** - M1 path-drift in 2026-05-08 consolidated review L124: `(../../decisions/...)` → `(../../../decisions/...)`. The 4-deep file needs 3 `..` to reach `docs/decisions/`. Other relative paths in the same file already use the correct depth; this was the lone broken outlier. - M2 `commit-style.md:130` anchor: the new "PR-number references" rider linked to `track-g-process.md#min-g3` for "PR-numbering fix-up precedent", but MIN-G3 is about closure-trio smoke-trace AC asymmetry, not PR-numbering. Dropped the wrong anchor; the link now points at the 2026-05-08 review's §"Cross-PR observations" §1 ("PR-numbering hygiene drift recurrence"), which IS the canonical source of the rider. - M3 158→159 narrative drift in `current.md:15` and `phase-b.md:103`: added "Current state (post-PR-#18 hygiene): 159/159" addendum that preserves the historical PR-#17-merge-moment claim of 158/158 (factually accurate at the time) while disambiguating from the current 159 baseline that PR #18's `25826a1` brought us to. **7 Minors (substance + governance polish):** - `hal.md` §Mmu present-tense overclaim: `MapperFlush` extension is T-016 deliverable, not current `main`; added a "tense hedge" blockquote making the post-T-016 framing explicit and acknowledging that the current trait surface is still pre-`MapperFlush`. - ADR-0027 §(a) MMIO range mismatch: `0x0902_0000` (the UART's actual end-of-MMIO) vs `0x0920_0000` (the 2 MiB-block-aligned end of the bootstrap's identity-map for that block). Reworded to explicitly cover the block-aligned superset; clarified the unmapped slack is harmless under device-nGnRnE. - ADR-0027 §(a) `TCR_EL1.A1` forward-flag: `A1=0` documented (selects which TTBR holds the ASID; v1 keeps `A1=0` because only `TTBR0_EL1` is active); the future high-half ADR (ADR-0033) gets named as the decider for `A1=1` migration. - `track-3-pr-20-governance.md:31` quoted-excerpt path: the embedded `(../../.claude/skills/write-adr/SKILL.md)` link in a quoted ADR-0027 passage was broken from track-3's depth (5-deep). Restructured the quote into a `> blockquote` with link targets stripped; preserves quote fidelity while avoiding broken-link rendering. - Glossary: added entries for `mmu_bootstrap` and `.boot_pt` — identifiers introduced by ADR-0027 / T-016 that future readers will hit and want a 1-line definition for. - `tools/perf-harness.sh` `export LC_ALL=C` at script entry: forces period-decimal + ASCII-digit awk output regardless of host locale. Without this, `LC_ALL=tr_TR.UTF-8` would emit `5,169` (decimal comma) in baseline reports — valid in tr_TR, mis-parsed in en_US. - 2026-05-08 consolidated review §Follow-up backlog: each of the 15 items now carries an inline ✅ + closing-commit-SHA annotation, matching the [2026-05-07 review's `8b6147d`-style closure-status pattern](docs/analysis/reviews/code-reviews/ 2026-05-07-pr-12-to-17-multi-axis-review.md). Item 8 (PR #19 description "180/180" wording) annotated as "not closeable post-merge" with rationale. **5 Nits (refinements; one was already closed):** - ADR-0027 §Simulation Step 1 row: collapsed the awkward `L2_low[64..72]` + `L2_low[72]` notation into the cleaner half-open `L2_low[64..73]` (= 9 indices = 9 entries) and added a footnote clarifying the half-open Rust convention used throughout the ADR. memory-management.md mermaid diagram updated similarly. - ADR-0027 §References framing: tightened "first ADR drafted under §Simulation; this is the second" to "first ADR drafted under the §Simulation rule (recovery-primitive subject); this ADR is the first non-recovery-primitive state-machine ADR drafted under the same rule" — matches the §Context para 2 framing precisely. - `tools/perf-harness.sh:223` (`QEMU_VERSION` extraction): replaced `head -n 1` with `awk 'NR==1 { print; exit }'` to avoid the `head` SIGPIPE that `set -o pipefail` would propagate. Defensive hardening; observable behaviour unchanged. - `tools/perf-harness.sh:209` watchdog kill: added `kill -0` liveness guard before `kill -KILL`. Before: kill on a dead PID returned ESRCH which `2>/dev/null || true` swallowed silently. After: explicit aliveness check makes the intent self-documenting. - `tools/perf-harness.sh:54-62` trap function: added a one-line comment documenting the idempotency property — both globals empty → no-op; stale PIDs → ESRCH suppressed → safe to fire multiple times. - (Already closed) `unsafe-log.md` 2026-05-08 closure-path Amendments cite Track-H NIT-1: verified — all 3 Amendments (UNSAFE-2026-0019/0020/0021) already include the trace-back link to the prior review's NIT-1. Verification: cargo fmt clean, cargo host-test 159/159, host-clippy clean (-D warnings), kernel-clippy clean, kernel-build clean, perf-harness 3-iter sanity run produces consistent stats with `LC_ALL=C` decimal-period output. 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
Closes the hygiene backlog from the 2026-05-07 PR #12-#17 multi-axis post-merge review (8-track parallel fan-out, verdict Approve, 7 Minor follow-ups for a hygiene PR before ADR-0027 drafting). All 10 in-scope Minor + 2 Nit findings applied; 4 forward-flagged items intentionally deferred.
What landed
Three commits on this branch:
54f6042docs(reviews):read-only artefact commit — the multi-axis review itself (1 consolidated + 8 axis files + README index update).25854a1chore(kernel):hygiene polish onkernel/src/{ipc,sched}/mod.rs— doc-comment forward-rider onipc_cancel_recvfor cap-bearing-state semantics; tightened cancel-block// SAFETY:wording (caller_table is&CapabilityTable, not "exclusive");SchedQueue::new# Panics (compile-time)→# Compile-time errors; newcancel_recv_on_recv_complete_does_not_drop_message_or_captest (158 → 159 host + miri).94a6c0fdocs:doc-side fixes —current.mdpost-T-015 amendment (.text 22,020); 2026-05-07 perf re-baseline gains a "Post-T-015 amendment" section with byte-exact +228 attribution; UNSAFE-2026-0014 3rd + 4th Amendment SHAs back-filled (c30f4ee+7a402cb);unsafe-policy.md §3mechanical-edit exemption for localisation sweeps; ADR-0026 §Revision notes (Simulation chronology + skill-clause reconciliation betweenwrite-adr§10 andsupersede-adr§7); security + performance master-plans gain closure-trio AC cross-reference riders.Findings closed (10 Minor + 2 Nit)
ipc_cancel_recvdoc-rider onSendPending/RecvCompleteno-op + B2+ destroy-drain forward note (consolidated with Track C cap-bearing-state)// SAFETY:wording aligned with UNSAFE-2026-0014 4th Amendment (caller_table is shared, not exclusive)SchedQueue::newrustdoc section renamed# Panics (compile-time)→# Compile-time errorscurrent.md+ perf re-baseline updated to.text 22,020 (+228 vs PR-#16; +112 vs 2026-04-28)cancel_recv_on_recv_complete_does_not_drop_message_or_captest (~25 LOC, fails-before-fix on a regression that blanket-cleared cap-bearing states)write-adr§10 (separate Accept commit) vssupersede-adr§7 (solo-phase combined commit); future introductory state-machine ADRs follow §10write-adr§Simulation rule was retro-extracted from this ADR's table (commit77a578a, 84 sec before ADR-0032 Propose)unsafe-policy.md §3"Mechanical-edit exemption" paragraph; closes the PR #14 Yüksek→High in-place-edit gap without per-entry Amendmentsc30f4ee/7a402cbcurrent.md2026-05-07 audit-status paragraph gains the post-T-015 entry naming the 4th Amendment siteFindings intentionally not in this PR
RecvWaitingunit-variant identity gap) — gating item for ADR-0030 (syscall ABI) + ADR-0019 (multi-waiter); not v1 actionable.These are tracked at the artefact layer (track files + ADR-0032 §Negative + this PR's body) and remain queued for their natural venue.
Test plan
cargo fmt --all -- --checkcleancargo host-clippyclean (-D warnings)cargo kernel-clippycleancargo host-test159/159 (158 → 159 via the new RecvComplete no-op test)cargo +nightly miri test159/159 clean (3.33 + 13.98 + 4.36 s)cargo kernel-buildcleanunsafeblocks; no new audit-log entries; UNSAFE-2026-0014's umbrella unchangedRead order suggestion for review
docs/analysis/reviews/code-reviews/2026-05-07-pr-12-to-17-multi-axis-review.md— what the review founddocs/decisions/0026-idle-dispatch-fallback.md§Revision notes — load-bearing process riderdocs/standards/unsafe-policy.md§3 mechanical-edit exemption — load-bearing standards riderkernel/src/ipc/mod.rsipc_cancel_recvdoc-comment + new test🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation