docs(roadmap): route 2026-07-15 full-repo review into a phased backlog#46
Merged
Conversation
A systematic, multi-dimensional review of the entire repository (code, docs, infrastructure) was run at the Phase B/C boundary. Its 241 verified findings and 147 excellence opportunities are recorded as actionable roadmap work rather than a side report: - New bridge phase Phase B.2 — Consolidation & Hardening (phase-b2.md), organized as Tracks B.2-1..B.2-6: CI enforcement, the Phase-B documentation cascade, immediate correctness/robustness fixes, code quality & API polish, unsafe-audit hygiene, and performance micro-fixes. - "Review-derived work items" sections appended to phases C, D, E, G, H (additions only; existing milestone bodies are cross-referenced by ID, not restated). - Phase index (README table + dependency graph) and current.md banner updated; .gitignore ignores the private raw-review output. Every finding is written out in full with its file:line location; nothing links to the git-ignored docs/analysis/private artifacts. Phase A and Phase B files are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes add Phase B.2 as a roadmap consolidation bridge, define its work tracks, route review findings into Phases C, D, E, G, and H, and ignore private analysis files. ChangesRoadmap structure
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 updates the project's roadmap and phase plans following a comprehensive full-repository review completed on 2026-07-15. It introduces a new consolidation and hardening bridge phase (Phase B.2) and appends detailed, review-derived work items and polish opportunities to the roadmaps for Phase C, Phase D, Phase E, Phase G, and Phase H. Additionally, the .gitignore file is updated to exclude private analysis documents. There are no review comments provided, so I have no feedback to offer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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 (3)
docs/roadmap/phases/phase-h.md (2)
139-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge this duplicate map-validation work item.
Lines 87-89 already require extracting shared alignment and flag validation; this polish item repeats the same action. Keep one canonical item and preserve the fidelity rationale there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-h.md` at line 139, Merge the duplicate map-validation roadmap item into the existing requirement at lines 87-89. Retain one canonical item covering shared alignment and DEVICE/EXECUTE flag validation, and preserve the rationale about enforcing decorator failure-mode fidelity.
131-133: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftMake the
IrqControllerAPI change explicitly ADR-gated.Changing
enable/disablefrom()toResultis HAL trait rework, while Phase H states that trait rework is out of scope unless an ADR approves it. Record the chosen contract and migration impact before assigning this as routine H1-H3 hardening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-h.md` around lines 131 - 133, Make the proposed IrqController::enable/disable contract change explicitly dependent on an approved ADR before treating it as Phase H work. Document the selected out-of-range IrqNumber behavior and migration impact, and keep H1–H3 implementation tasks from assuming a Result-based HAL trait change until that ADR is approved.docs/roadmap/phases/phase-d.md (1)
293-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate repeated findings before finalizing coverage counts.
Lines 293-301 explicitly duplicate the preceding GIC CPUID/EOI finding, and Lines 314-317 repeat the migration-target/high-half assertion issue from Lines 263-266. Merge each pair while retaining all locations and ADR notes so the roadmap does not create duplicate implementation work or overstate routed findings.
Also applies to: 314-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-d.md` around lines 293 - 301, Deduplicate the repeated roadmap findings by merging the GIC CPUID/EOI entries around the first finding and the duplicate at lines 293-301, preserving every location, action, gate, and ADR-numbering note. Also merge the migration-target/high-half assertion findings around lines 263-266 and 314-317, retaining all relevant locations and ADR notes, and update coverage counts so each underlying issue is counted once.
🤖 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/roadmap/current.md`:
- Around line 7-8: Reconcile the live-state bullets in current.md with the
banner’s stated status: make Phase C the active phase, identify Phase B.2 as the
recommended next bridge, and remove or clearly mark the B6 active/in-review,
b6-closure branch, and B6-next-task entries as historical. Preserve the banner
and ensure no contradictory current-state records remain.
In `@docs/roadmap/phases/phase-b2.md`:
- Around line 485-486: Fix the undefined Markdown reference in the revised
crate-scope wording near the P6 discussion: either define the p6 reference label
in this document or replace [P6][p6] with a valid inline or relative link, while
preserving the intended link to the P6 BSP interrupt-handling claim.
- Line 673: Update the Phase B.2 roadmap documentation to identify or add an ADR
covering the structural decision to introduce Phase B.2, and link it from the
phase documentation. Remove the claim in the “no ADRs” statement that bypasses
this requirement, while preserving the separate guidance for documenting new
design decisions discovered during execution.
In `@docs/roadmap/phases/phase-c.md`:
- Around line 155-157: Move the PMM concurrency requirement into the C1/C2
milestone planning before the secondary-core gate. Add an explicit item covering
cross-core serialization of shared Pmm frame allocation, and ensure
secondary-core enablement remains gated until a real spinlock or atomic bitmap
implementation exists; do not rely on Cpu::without_interrupts alone.
In `@docs/roadmap/phases/phase-d.md`:
- Line 334: Update the boot memory-map diagram referenced in the boot
documentation from ASCII to an inline Mermaid diagram, following
docs/standards/documentation-style.md before editing. Preserve the
T-022/ADR-0033 link-high/load-low memory layout and ensure the diagram replaces
the existing ASCII representation rather than adding a separate duplicate.
- Around line 219-222: Align the documented Mmu::activate ordering across both
roadmap entries: update docs/roadmap/phases/phase-d.md lines 219-222 and
docs/roadmap/phases/phase-c.md lines 315-317 to consistently specify
flush-then-install, with barriers and TLB invalidation before TTBR0/EPD0
updates, while preserving the existing IRQ/FIQ safety-comment guidance.
In `@docs/roadmap/phases/phase-e.md`:
- Around line 152-158: Revise the roadmap wording so it does not call cap_map
the universal W^X chokepoint. State that the invariant must be enforced in
shared MappingFlags validation or Mmu::map, covering direct lower-level callers,
while retaining cap_map’s WRITE+EXECUTE rejection as defense-in-depth; update
the related bullets to consistently reflect this shared enforcement point.
---
Nitpick comments:
In `@docs/roadmap/phases/phase-d.md`:
- Around line 293-301: Deduplicate the repeated roadmap findings by merging the
GIC CPUID/EOI entries around the first finding and the duplicate at lines
293-301, preserving every location, action, gate, and ADR-numbering note. Also
merge the migration-target/high-half assertion findings around lines 263-266 and
314-317, retaining all relevant locations and ADR notes, and update coverage
counts so each underlying issue is counted once.
In `@docs/roadmap/phases/phase-h.md`:
- Line 139: Merge the duplicate map-validation roadmap item into the existing
requirement at lines 87-89. Retain one canonical item covering shared alignment
and DEVICE/EXECUTE flag validation, and preserve the rationale about enforcing
decorator failure-mode fidelity.
- Around line 131-133: Make the proposed IrqController::enable/disable contract
change explicitly dependent on an approved ADR before treating it as Phase H
work. Document the selected out-of-range IrqNumber behavior and migration
impact, and keep H1–H3 implementation tasks from assuming a Result-based HAL
trait change until that ADR is approved.
🪄 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: c7c8bd58-4bce-4c75-ac2a-15ee4bb6724f
📒 Files selected for processing (9)
.gitignoredocs/roadmap/current.mddocs/roadmap/phases/README.mddocs/roadmap/phases/phase-b2.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-g.mddocs/roadmap/phases/phase-h.md
…activate ordering Applies the PR review comments (all verified still-valid against the files): - current.md: reconcile live-state bullets with the 2026-07-15 banner — Phase C active, Phase B.2 recommended bridge; B6 active-milestone, in-review (PR #43) and working-branch entries marked historical. - phase-b2.md: fix undefined [P6][p6] reference (inline link to architectural-principles.md); drop the "no ADRs of its own" claim — introducing Phase B.2 is a structural roadmap change requiring a governing ADR per ADR-0013, now flagged as a prerequisite placeholder. - phase-c.md: gate secondary-core enablement (C1) on real cross-core Pmm serialization (spinlock or atomic bitmap), noting Cpu::without_interrupts alone is insufficient for cross-core; move the requirement into C1/C2 planning; align Mmu::activate to flush-then-install. - phase-d.md: align Mmu::activate ordering with phase-c (authoritative flush-then-install); reword boot.md memory-map diagram item to convert-to-Mermaid per documentation-style.md; merge two near-duplicate finding pairs (GICv2 CPU-ID/EOI; high_half_alias release-debug_assert), 25 -> 23 findings. - phase-e.md: W^X enforcement belongs at the shared Mmu::map / MappingFlags layer (covers direct loader/bootstrap callers that bypass cap_map); cap_map guard reframed as defense-in-depth. - phase-h.md: fold the duplicate map-validation polish item into Epic 1's BlockMappedMmu finding (4 -> 3 polish); gate the IrqController enable/disable Result-based signature change on an approved ADR before H1-H3 assume it. Validated: 172 relative links + all intra-roadmap anchors resolve; finding/polish counts consistent; no docs/analysis/private references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/roadmap/phases/phase-e.md`:
- Around line 152-158: Make the W^X requirement consistent across both roadmap
findings: choose whether shared validation rejects every WRITE+EXECUTE mapping
or only WRITE+EXECUTE+USER mappings, then apply that same scope to the stated
Mmu::map/MappingFlags validation and cap_map defense-in-depth guard. Explicitly
remove the conflicting “arguably” or undecided kernel-only language so
implementation expectations are unambiguous.
In `@docs/roadmap/phases/phase-h.md`:
- Line 140: Keep the GICv2 architectural max-IRQ constant backend-specific
rather than moving it into the generic tyrne_hal::irq_controller API. Update the
GIC/test-HAL implementation, including FakeIrqController and the real driver
references, to share an implementation-local bound without imposing an ARM-only
limit on the future RISC-V PLIC path.
🪄 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: a093cb72-b546-42ad-860c-55d5fc6c9d36
📒 Files selected for processing (6)
docs/roadmap/current.mddocs/roadmap/phases/phase-b2.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-h.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/roadmap/phases/phase-b2.md
- docs/roadmap/phases/phase-d.md
…IC max-IRQ backend-local - phase-e.md: make the two W^X findings consistently scope to WRITE+EXECUTE+USER (the userspace case), enforced at the shared Mmu::map / MappingFlags::validate() layer with cap_map as defense-in-depth. Kernel-only WRITE+EXECUTE (the ADR-0034-blessed bootstrap RWX block mapping) stays permitted until the per-section remap lands. Removes the earlier unqualified "reject WRITE+EXECUTE" and the undecided "arguably the kernel-only case too" language so implementation expectations are unambiguous. - phase-h.md: keep the GICv2 architectural max-IRQ constant implementation-local — shared between bsp-qemu-virt's real GIC driver and test-hal's GICv2-modelling FakeIrqController — rather than hoisting it into the generic tyrne_hal::irq_controller trait module, which must stay architecture-neutral so the RISC-V PLIC path (Milestone H3) is not saddled with an ARM-only bound. Validated: relative links resolve; no docs/analysis/private references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/roadmap/phases/phase-e.md (1)
186-186: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the canonical
Phase B.2spelling.
Phase B2+is inconsistent with the roadmap’sPhase B.2naming and may reduce searchability or break references.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-e.md` at line 186, Update the roadmap text to use the canonical “Phase B.2+” spelling instead of “Phase B2+” in the endpoint-destroy ADR precondition reference. Preserve the surrounding requirement and all other phase naming unchanged.
🧹 Nitpick comments (1)
docs/roadmap/phases/phase-e.md (1)
140-142: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse the capability kind directly.
rights_and_kind_of(src)isn’t an available API here; split kind validation from the rights check and compare the source capability’s kind againstnew_object.kind().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phases/phase-e.md` around lines 140 - 142, The cap_derive validation must use the capability kind directly because rights_and_kind_of is unavailable. In cap_derive, split the existing rights validation from kind validation, compare src.kind() with new_object.kind(), and return CapError::WrongKind on mismatch while preserving the existing rights behavior.
🤖 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/roadmap/phases/phase-e.md`:
- Around line 154-158: Make shared MMU validation, via Mmu::map or shared
MappingFlags validation, the authoritative rejection point for mappings with
WRITE, EXECUTE, and USER set; return InvalidFlags while continuing to permit
kernel-only RWX mappings. Keep matching checks in the VMSAv8 descriptor encoder
and cap_map as defense in depth, ensuring all direct mapper callers are covered.
---
Outside diff comments:
In `@docs/roadmap/phases/phase-e.md`:
- Line 186: Update the roadmap text to use the canonical “Phase B.2+” spelling
instead of “Phase B2+” in the endpoint-destroy ADR precondition reference.
Preserve the surrounding requirement and all other phase naming unchanged.
---
Nitpick comments:
In `@docs/roadmap/phases/phase-e.md`:
- Around line 140-142: The cap_derive validation must use the capability kind
directly because rights_and_kind_of is unavailable. In cap_derive, split the
existing rights validation from kind validation, compare src.kind() with
new_object.kind(), and return CapError::WrongKind on mismatch while preserving
the existing rights behavior.
🪄 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: 64a3f6da-17ff-4eba-bb13-20556ac8522e
📒 Files selected for processing (2)
docs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-h.md
…Phase B.2 spelling - phase-e.md cap_derive: replace the fictional `rights_and_kind_of(src)` helper (which does not exist in the code) with the real accessors — compare `entry.capability.kind()` against `new_object.kind()` (both -> `CapKind`) and return `CapError::WrongKind` on mismatch, kept separate from the unchanged rights validation (DERIVE-present, no-widening). - phase-e.md W^X: make the shared `Mmu::map` / `MappingFlags::validate()` the authoritative WRITE+EXECUTE+USER rejection point (the one layer every direct mapper caller — cap_map, task loader, bootstrap — passes through), with matching defense-in-depth checks in the VMSAv8 descriptor encoder (`flags_to_descriptor_bits`) and `cap_map`; kernel-only RWX stays permitted. - phase-e.md: "Phase B2+" -> "Phase B.2+" (canonical) in the endpoint-destroy ADR precondition reference. Validated: relative links resolve; no docs/analysis/private references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Routes the findings and excellence opportunities from a 2026-07-15 full-repository review (all code, docs, infrastructure) into the roadmap as an actionable, phased backlog — rather than leaving them in a side report. Phase A and Phase B files are untouched (both phases are closed).
241 verified findings (🔴 2 critical · 🟠 27 high · 🟡 78 medium · ⚪ 113 low · ⚫ 21 info) and 147 excellence/polish opportunities are recorded, every one written out in full with its
file:linelocation.How it's organized
docs/roadmap/phases/phase-b2.md("Phase B.2 — Consolidation & Hardening"), inserted between B and C. Work is grouped as Tracks B.2-1…B.2-6 (named to avoid colliding with Phase B's B0–B6 milestone IDs):StaticCellblanketSync, non-atomic PMM bitmap RMW — harmless single-core, UB once a second core boots) + concurrency/SMP readinessCPTR_EL2, FP/SIMD trap frames)phases/README.mdtable + dependency graph) andcurrent.mdbanner updated..gitignoreignores the private raw-review output.Guarantees
file:line.docs/analysis/private/artifacts — findings are written out in full so remediation is self-contained.git diffshows 0 deletions in those files); Phase A/B unchanged.current.mdbanner.Notes
MappingFlags::DEVICEpolicy, a scheduled perf-CI job) the roadmap directs it through normal ADR governance.🤖 Generated with Claude Code
Summary by CodeRabbit