Development - #1
Merged
Merged
Conversation
…nd typed reviews Introduces the repo-native planning and tracking layer that ADR-0013 pins: the roadmap (the plan, what we intend to do) lives at docs/roadmap/; the analysis (the work, tracked per task and review) lives at docs/analysis/. ## Structure - docs/roadmap/README.md — purpose, conventions, navigation. - docs/roadmap/current.md — "we are here" pointer. - docs/roadmap/phases/ — one file per phase (A–I), split so detail can grow per phase without a single document becoming unnavigable. Detailed breakdown for A, B, C, D; medium detail for E, F, G; light detail for H, I. - docs/analysis/README.md — cross-cutting explanation of tasks and reviews and how they relate to the roadmap and ADRs. - docs/analysis/tasks/ — per-phase folders (phase-a/ through phase-i/), each with its own task index. Task IDs (T-NNN) are sequential across the whole project. - docs/analysis/reviews/ — four review types, each with its own folder: business-reviews/, code-reviews/, security-reviews/, performance-optimization-reviews/. Each has a README (triggers, when to conduct) and a master-plan.md (multi-agent procedure with roles, merge step, acceptance criteria, output template). ## First task docs/analysis/tasks/phase-a/T-001-capability-table-foundation.md is opened as the first roadmap task — status Ready, milestone A2. It has a full user story and eleven acceptance criteria, including that ADR-0014 (capability representation) is Accepted before implementation code lands. ## Skills - .claude/skills/start-task/SKILL.md — creates a new task file in the right phase folder, assigns the next T-NNN, updates current.md on status transition. - .claude/skills/conduct-review/SKILL.md — produces a review artifact following the selected type's master plan (business / code / security / performance-optimization). Skill index updated; CLAUDE.md "Where to find things" table updated. ## ADR-0013 update Rewritten from the pre-structure version to reflect the final split: roadmap vs. analysis, per-phase task folders, per-type review folders with master plans, multi-agent review support. ## Verified - cargo fmt --all -- --check clean. - cargo host-test passes 34/34 (no regression). - Every file's frontmatter / header follows the conventions it describes. Refs: ADR-0013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rogress Pins the capability table's representation before T-001's implementation lands: index-based arena with generation-tagged handles (rejects intrusive linked list — no heap; rejects parent-only table — O(n) revocation; rejects epoch-based lookup — overkill for single-core v1). Supporting types: CapHandle(index, generation) detects use-after-revoke at lookup time; CapRights is a hand-rolled bitfield (DUPLICATE, DERIVE, REVOKE, TRANSFER v1 rights); Capability is move-only (not Copy, not Clone); CapObject(u64) is a placeholder until Milestone A3 introduces real kernel objects. Slot layout carries parent / first_child / next_sibling indices so cascading revocation is an O(descendants) subtree walk with a hard cap of MAX_DERIVATION_DEPTH = 16. Table capacity CAP_TABLE_CAPACITY = 64 per task. Zero `unsafe` targeted in the capability core — the most security-sensitive kernel subsystem stays in safe Rust. Task side-effects: - T-001 frontmatter Status → In Progress; ADR-0014 dependency marked Accepted; review-history row added. - docs/analysis/tasks/phase-a/README.md index updated. - docs/roadmap/current.md now reads "In Progress" and names the development branch. - docs/decisions/README.md indexes ADR-0014. Refs: ADR-0001, ADR-0013, ADR-0014, T-001 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands ADR-0014's capability representation: an index-based arena with
generation-tagged handles, an embedded derivation tree (parent,
first_child, next_sibling indices), and explicit cascading revocation.
No `unsafe`. No heap. Bounded at CAP_TABLE_CAPACITY = 64 slots per task
with MAX_DERIVATION_DEPTH = 16.
## New module: `umbrix_kernel::cap`
- `cap::mod` — top-level types: `Capability` (move-only, not `Copy` or
`Clone`; `Debug` derived for test ergonomics), `CapKind` (Task /
Endpoint / Notification / MemoryRegion v1 variants), `CapObject(u64)`
(placeholder until Milestone A3 replaces with typed references),
`CapError` (`#[non_exhaustive]`: CapsExhausted, InvalidHandle,
WidenedRights, InsufficientRights, DerivationTooDeep).
- `cap::rights` — `CapRights` bitfield with v1 rights (`DUPLICATE`,
`DERIVE`, `REVOKE`, `TRANSFER`) and `BitOr` / `BitAnd` / `BitOrAssign`
operators. Hand-rolled rather than bitflags-crate dependency per the
open question in ADR-0014.
- `cap::table` — `CapHandle { index, generation }`, `CapabilityTable`,
and five operations: `insert_root`, `cap_copy`, `cap_derive`,
`cap_revoke`, `cap_drop`, plus `lookup`. `cap_revoke` is an iterative
BFS collecting descendants into a fixed-size stack array, then freeing
each — zero recursion, bounded worst-case. Stale handles (slot freed,
generation bumped) always return InvalidHandle.
## Kernel crate attribute change
`#![no_std]` → `#![cfg_attr(not(test), no_std)]` so the standard
`cargo test` harness works on host. Kernel production builds remain
strictly `no_std`.
## Tests
27 new host tests in `kernel::cap` covering:
- CapRights union / intersection / difference / narrowing / raw round-trip.
- Empty table shape; root insert / lookup.
- Drop invalidates handle; double-drop is InvalidHandle; freed slot is
reused with a bumped generation.
- cap_copy with same / narrower / widened rights; DUPLICATE-missing
path; peer-of-child shares parent.
- cap_derive with narrower rights; DERIVE-missing; widened rights;
depth cap enforced at exactly MAX_DERIVATION_DEPTH.
- cap_revoke removes only descendants; cascades depth-three; REVOKE
missing; leaf noop; stale handle; peer preservation.
- Table exhaustion returns CapsExhausted; freed slot reusable.
- Sibling-list integrity when dropping the middle child.
## Local verification
- `cargo fmt --all -- --check` clean.
- `cargo host-clippy` clean (`-D warnings`).
- `cargo kernel-clippy` clean (aarch64 build including BSP).
- `cargo host-test` passes 61/61 (27 new kernel tests + 34 existing
test-hal tests; no regression).
## Task status
T-001 transitions Ready → In Progress → In Review. Commit ready for
maintainer PR review from `development` to `main`.
Refs: ADR-0001, ADR-0013, ADR-0014, T-001
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g updates Plan C from the three-document design review: kernel remains AI-neutral, userspace is where any future AI-integrated features live, and the kernel / HAL deliberately leave four hooks open so a later Phase J can build the AI-native userspace layer without touching the kernel's shape. ## ADR-0015 New Accepted ADR laying out the full rationale for the userspace-only, kernel-neutral AI posture. Rejects three alternatives (full AI in kernel, semantic microkernel with AI-adjacent trusted services, no-position) with specific reasons (TCB explosion, determinism loss, prompt-injection attack surface at the privileged boundary, formal-verification incompatibility, proprietary-weight blob issue). Enumerates the four hooks — TEE in the HAL (Phase G), NPU driver trait (Phase H or later), userspace → scheduler hint syscall (Phase C or later), capability intent-extension point (Phase E or later) — and explicitly records what this ADR forbids (LLM in any privileged path, NL syscalls, semantic IPC, neural scheduler, closed-weight blobs). ## Phase J (sketch) New long-horizon phase for the AI-native userspace layer. Six sketched milestones (userspace inference runtime, semantic file indexer, intent-analysis policy service, natural-language shell, scheduler-hint daemon, TEE-backed on-device LLM). Opt-in; Umbrix must be fully usable without Phase J. Detail stays light until activation. ## Supporting updates - phases/README.md — Phase J row added, dependency sketch extended with `G -. opt-in .-> J` and `E -. opt-in .-> J`. - phases/phase-g.md — new Milestone G1.5 (TEE support in the HAL), pairs naturally with measured boot; justified both for Phase G's security goals and as the first of ADR-0015's four hooks. - phases/phase-e.md — E5 ADR-0040 option list extended to mention log-structured FS (flash-friendly wear-levelling) as a candidate. - phases/phase-a.md — A6 exit bar gains a baseline performance-review artifact: first entry in analysis/reviews/performance-optimization- reviews/, capturing v0.0.1 numbers (kernel size, idle memory, IPC round-trip, context-switch overhead, boot time) so later reviews have a reference point. - analysis/tasks/phase-j/README.md — stub task index. - decisions/README.md — ADR-0015 row. Each hook remains a placeholder; their concrete ADRs land at the phase that delivers them. None of the above adds code today. Refs: ADR-0001, ADR-0004, ADR-0013, ADR-0015 Co-Authored-By: Claude Opus 4.7 (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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 45 seconds. ⌛ 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 (4)
📝 WalkthroughWalkthroughAdds roadmap and analysis documentation (phases, tasks, reviews), two Claude skills ( Changes
Sequence DiagramssequenceDiagram
actor Agent
participant Skills as "Claude Skill"
participant Roadmap as "docs/roadmap/current.md"
participant Analysis as "docs/analysis/"
participant Git as "git"
Agent->>Skills: invoke start-task
Skills->>Roadmap: verify phase/milestone exists
alt milestone missing
Skills-->>Agent: require ADR / halt (ADR needed)
else milestone exists
Skills->>Analysis: scan phase-*/T-*.md for next T‑NNN
Skills->>Analysis: copy TEMPLATE.md → phase-X/T-NNN.md
Skills->>Analysis: fill frontmatter and required sections
Skills->>Analysis: update phase-X/README.md index
alt status == "In Progress"
Skills->>Roadmap: update current.md active task (handle prior active)
end
Skills->>Git: commit docs changes (specified commit-style)
Skills-->>Agent: return new task handle
end
sequenceDiagram
actor Agent
participant Skills as "Claude Skill"
participant Roadmap as "docs/roadmap/current.md"
participant Analysis as "docs/analysis/reviews/"
participant Roles as "Role agents"
participant Git as "git"
Agent->>Skills: invoke conduct-review (trigger)
Skills->>Analysis: read review master-plan for selected type
Skills->>Analysis: locate prior review (if any)
Skills->>Roles: dispatch role executions (parallel or sequential)
par Roles run
Roles->>Roles: produce assigned section outputs
end
Skills->>Roles: run merge step (reconcile sections → artifact)
Skills->>Analysis: write dated review artifact and update type README index
alt business review side-effects
Skills->>Roadmap: update docs/roadmap/current.md or pathfinder outputs
end
Skills->>Git: commit docs (include required trailers)
Skills-->>Agent: review artifact complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 introduces a comprehensive roadmap and planning process for the Umbrix project, including new documentation structures for tasks and reviews, as well as the initial implementation of the capability subsystem. The feedback highlights several inconsistencies where legacy planning references (e.g., "Phase 4c", "Phase 4b") were not updated to match the new roadmap. Additionally, there is a technical inaccuracy in the ADR documentation regarding the number of pointers per slot, and a suggestion to improve error handling in the capability table's revocation logic where a misleading error variant is currently used for an unreachable logic path.
|
|
||
| 9. **Commit** per [`commit-style.md`](../../../docs/standards/commit-style.md): | ||
| - Scope: `docs`. | ||
| - Message: `docs(roadmap): <type>-review <scope>` — e.g. `docs(roadmap): business-review A2-completion`. |
There was a problem hiding this comment.
In the commit message example, docs(roadmap): <type>-review <scope>, the scope is roadmap. However, the new documentation structure places reviews under docs/analysis/reviews/. To maintain consistency with the new file structure, it might be clearer to use a scope like docs(analysis) or docs(review) for review-related commits.
| - **Status:** In Review | ||
| - **Created:** 2026-04-20 | ||
| - **Author:** @cemililik | ||
| - **Dependencies:** none (Phase 4c — bootable kernel — is complete) |
There was a problem hiding this comment.
The dependency is listed as none (Phase 4c — bootable kernel — is complete). This appears to reference a previous project planning structure. With the new roadmap introduced in this pull request, it would be more consistent to reference Milestone A1 — Bootable skeleton as the dependency.
| - **Compile-time table size.** Changing `CAP_TABLE_CAPACITY` requires a rebuild. Mitigation: the constant is exposed and revisited when a real use-case demands more; for v1 `64` is adequate. | ||
| - **Generation overflow.** A `u32` generation counter wraps after ~4 × 10⁹ free-reuse cycles of the same slot. Mitigation: on overflow, the slot is permanently marked as used-and-inaccessible (the address stays in the table but no new handle can match). Documented; if this becomes a real scenario, a v2 ADR raises generation to `u64`. | ||
| - **Hard depth cap (16).** Some future subsystem may want deeper. At that point, the cap is loosened in a follow-up ADR with a stated reason. | ||
| - **Four sibling pointers per slot add ~4 bytes each.** Sixty-four slots × ~32 bytes per slot = ~2 KiB per task. Acceptable for v1. |
There was a problem hiding this comment.
The documentation states: "Four sibling pointers per slot add ~4 bytes each." This appears to be inaccurate. The SlotEntry struct contains three pointers for tree linkage (parent, first_child, next_sibling), not four. Clarifying this detail would improve the accuracy of this important design document.
|
|
||
| **Exit bar:** `bsp-pi5`, `bsp-jetson` (CPU-only), and one RISC-V BSP each boot and run the Phase A / B / E subset on real hardware. | ||
|
|
||
| **Scope:** Prove that the HAL abstraction is real. Each new BSP is mostly additive and stresses the HAL interfaces written in Phase 4b. |
There was a problem hiding this comment.
The text mentions "HAL interfaces written in Phase 4b". This seems to be a reference to a legacy planning system. To align with the new documentation structure, this should probably refer to a phase from the new roadmap, such as Phase B, where relevant HAL interfaces are developed.
Comment on lines
+335
to
+338
| if desc_len >= CAP_TABLE_CAPACITY { | ||
| // Should be unreachable since descendants ≤ table size. | ||
| return Err(CapError::CapsExhausted); | ||
| } |
There was a problem hiding this comment.
The comment // Should be unreachable indicates that this path signifies a logic error. While returning an Err is necessary due to the deny(clippy::panic) lint, using CapError::CapsExhausted is misleading. This error variant implies the table is full, not that an internal buffer for descendant collection has overflowed during revocation.
For better clarity and maintainability, consider adding a debug_assert! to catch this during testing. If an error must be returned in release builds, a more descriptive error or at least a comment explaining the use of CapsExhausted in this context would be beneficial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
docs/analysis/tasks/TEMPLATE.md (1)
22-22: Optional: Consider varying sentence structure.Three consecutive sentences begin with "What" which slightly impacts readability. The static analysis tool flagged this repetition.
♻️ Proposed rewrite for improved flow
-<One or two paragraphs. What situation motivates this task? What would go wrong if it were skipped? What has happened before this in the project? Link related ADRs, prior tasks, architecture documents.> +<One or two paragraphs. What situation motivates this task? What would go wrong if it were skipped? Include relevant project history and context. Link related ADRs, prior tasks, architecture documents.>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/tasks/TEMPLATE.md` at line 22, The paragraph in TEMPLATE.md contains three consecutive sentences that all start with "What", harming readability; update that placeholder paragraph by rephrasing at least two of the sentences to vary sentence openings and improve flow (e.g., replace "What situation motivates this task?" with "Describe the situation that motivates this task.", change "What would go wrong if it were skipped?" to "Explain the consequences if it is skipped.", and turn "What has happened before this in the project?" into "Summarize prior work or linked ADRs and tasks."), preserving the same content and links to ADRs/prior tasks.docs/roadmap/phases/phase-b.md (1)
154-157: Optional readability tweak in the open-questions list.Three consecutive bullets start with “Whether…”. Consider varying phrasing slightly to improve scanability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap/phases/phase-b.md` around lines 154 - 157, Three successive bullets all begin with "Whether…", hurting scanability; update the three items referencing ADR-0023 and ADR-0024 so each starts with a different verb phrase (e.g., "Choose between ELF and raw binary (ADR-0023).", "Decide if syscalls should be synchronous-only or include async variants (ADR-0024).", "Determine whether the initial userspace is its own binary target or embedded via include_bytes! (ADR-0023).")—keep the ADR IDs and original meaning, only vary the opening phrasing for better readability.docs/analysis/reviews/code-reviews/master-plan.md (1)
56-70: Optional wording polish in checklist bullets.A few consecutive bullets use the same sentence opener; small rewording would improve readability, but this is non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/reviews/code-reviews/master-plan.md` around lines 56 - 70, Polish the repeated checklist bullet phrasing under the "4. Documentation" section by varying sentence openers and sentence structure for the bullets "Every new public item has a rustdoc comment.", "Every `unsafe fn` has a `# Safety` section.", "Every new `Error` variant is listed in an `# Errors` section where applicable.", and the two conditional bullets starting "If the change affects…". Rephrase one or two bullets to start with a different verb or clause (e.g., "Ensure a rustdoc comment exists for every new public item", "Add a `# Safety` section for each `unsafe fn`", "Document new `Error` variants in an `# Errors` section", and "Update docs/architecture when architecture is affected" or "Create a guides entry for user-facing workflow changes") so the checklist reads less repetitive while preserving the original checks and meaning.kernel/src/cap/mod.rs (1)
63-64:CapObject(pub u64)exposes raw object bits.The inner
u64ispub, which means any kernel code can construct or mutateCapObjectvalues directly — making it easy to forge an "object reference" outside the capability subsystem. Since this is explicitly a v1 placeholder to be replaced in Milestone A3 with typed references (per ADR-0014), consider making the field private now with aconst fn new(u64)/raw(&self) -> u64pair to keep construction auditable before A3 lands. Low risk today (kernel-internal only), but tightens the API boundary early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/cap/mod.rs` around lines 63 - 64, CapObject currently exposes its inner u64 as a public field allowing arbitrary construction/mutation; make the tuple field private (remove pub) and add a const fn new(id: u64) -> Self constructor and a pub const fn raw(&self) -> u64 accessor so callers can only create/read values via these functions; update any call sites that previously constructed CapObject(...) or accessed .0 to use CapObject::new(...) and .raw() respectively to keep construction auditable ahead of the Milestone A3 typed-ref replacement..claude/skills/conduct-review/SKILL.md (1)
48-52: Commit message scope/subject looks mismatched for non-business reviews.The template
docs(roadmap): <type>-review <scope>places every review type under theroadmapsubject, but code/security/performance reviews do not live indocs/roadmap/— they live indocs/analysis/reviews/<type>/. Considerdocs(analysis): <type>-review <scope>(ordocs(roadmap)only for business reviews since those also touchdocs/roadmap/current.md). Aligning the subject with the actual path will makegit log --grepmore useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/conduct-review/SKILL.md around lines 48 - 52, Update the commit-message template that currently uses "docs(roadmap): <type>-review <scope>" so the subject matches the actual docs path for non-business reviews; change it to "docs(analysis): <type>-review <scope>" (or make roadmap only for business reviews) and update the examples and guidance in SKILL.md where the template string appears to recommend using the correct subject for code/security/performance reviews so git logs and grep align with docs/analysis/reviews/<type>/..claude/skills/start-task/SKILL.md (1)
24-27: Race condition on T-NNN assignment across concurrent task creation."Highest existing T-NNN + 1" computed by scanning the filesystem is racy if two task-creation flows run in parallel (e.g., two agent sessions, or a PR branch +
main). Consider either: (a) requiring the skill to only run on a rebased branch and explicitly note "re-run if a newer T-NNN appears at merge time," or (b) reserving IDs via a lightweight ledger file. A footnote documenting the single-writer assumption would be enough for v1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/start-task/SKILL.md around lines 24 - 27, The T-NNN assignment is racy when multiple task-creation flows run concurrently; update the SKILL.md "Assign the next T-NNN" step to either (a) document the single-writer assumption and require running on a rebased branch with an explicit "re-run if a newer T-NNN appears at merge time" note, or (b) describe an alternative reserve-by-ledger approach (e.g., a lightweight ledger file that a single writer updates atomically) and reference the filenames/patterns involved (T-NNN and docs/analysis/tasks/phase-*/T-*.md) so implementers know where to apply it; pick one approach for v1 (prefer the single-writer footnote) and make the instruction text precise.docs/roadmap/phases/phase-d.md (1)
170-172: Clarify SD image blob policy reference.Line 172 calls the question "resolved" by ADR-0004 ("below the kernel" is out of blob-policy scope), but it is still listed under "Open questions." Consider moving this to a "Resolved" note or a non-open section to avoid implying it is unsettled — this is consistent with the learning that Umbrix rejects proprietary blobs in the kernel but tolerates them below it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap/phases/phase-d.md` around lines 170 - 172, The bullet about "Whether the SD image includes the Pi's closed-source firmware blobs" is marked under Open questions but already answered by ADR-0004; update the document so this item is not listed as open: either move that bullet into a "Resolved" or "Notes" section, or add a parenthetical "Resolved per ADR-0004: closed-source blobs below the kernel are allowed" next to the bullet; reference the exact phrase "Whether the SD image includes the Pi's closed-source firmware blobs" and cite ADR-0004 in the moved/resolved text to make the resolution explicit.kernel/src/cap/table.rs (2)
334-367: BFS overflow reportsCapsExhausted, which is misleading.The
desc_len >= CAP_TABLE_CAPACITYguards at lines 335 and 356 can only fire if the derivation tree contains a cycle or a node is reached twice — i.e., an internal invariant violation, not a capacity problem. ReturningCapError::CapsExhaustedconflates a bug with a legitimate allocation failure a caller might try to recover from. Consider adebug_assert!(since this is genuinely unreachable under a correct tree) or a distinct variant likeCapError::Internal. Happy to defer this if a broader error-taxonomy rework is already planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/cap/table.rs` around lines 334 - 367, The BFS loop in cap::table (the descendant collection code using descendants, desc_len, CAP_TABLE_CAPACITY and returning CapError::CapsExhausted) treats an internal invariant violation (cycle / duplicate node) as an allocation failure; change the two guards that check desc_len >= CAP_TABLE_CAPACITY to assert internal invariants instead of returning CapsExhausted — e.g., replace the early Err(CapError::CapsExhausted) in both the sibling-traversal and child-expansion loops with debug_assert!(desc_len < CAP_TABLE_CAPACITY, "derivation tree contains cycle or duplicate node") or, if you prefer an explicit runtime error, return a new variant like CapError::InternalCycleDetected (update CapError enum and call sites accordingly) so callers can distinguish genuine capacity exhaustion from internal bugs.
248-257: Redundant handle validation incap_derive.
self.entry_of(src)?at line 249 already callsresolve_handle(src)internally and returns the entry; the extraself.resolve_handle(src)?at line 250 re-walks the same validation purely to recover the index. You can get the index fromentry_ofby returning it alongside, or simply by reusingsrc.indexafterentry_ofsucceeds (validation already checked bounds/generation/populated).♻️ Minor simplification
- let (kind, rights, parent_index, parent_depth) = { - let entry = self.entry_of(src)?; - let idx = self.resolve_handle(src)?; - ( - entry.capability.kind(), - entry.capability.rights(), - idx, - entry.depth, - ) - }; + let (kind, rights, parent_index, parent_depth) = { + let parent_index = self.resolve_handle(src)?; + let entry = self.entry_of(src)?; + ( + entry.capability.kind(), + entry.capability.rights(), + parent_index, + entry.depth, + ) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/cap/table.rs` around lines 248 - 257, In cap_derive the call self.resolve_handle(src)? is redundant because entry_of(src)? already validates and resolves the handle; remove the extra resolve_handle call and obtain the index from the result of entry_of (or reuse src.index after entry_of succeeds). Update the block that builds (kind, rights, parent_index, parent_depth) to take idx from the returned entry (or src.index) instead of calling self.resolve_handle, keeping references to entry.capability.kind(), entry.capability.rights(), and entry.depth unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/tasks/phase-a/T-001-capability-table-foundation.md`:
- Line 8: The dependency metadata line contains a stale numeric/lettered phase
label ("Phase 4c — bootable kernel"); update the dependency to use the lettered
roadmap naming by replacing the string "Phase 4c — bootable kernel" with the
corresponding lettered phase (e.g., "Phase C — bootable kernel") so the
Dependencies metadata for the task matches the current phase naming convention
used elsewhere in the roadmap.
In `@docs/decisions/0013-roadmap-and-planning.md`:
- Around line 45-56: Update ADR-0013’s phase-range text and example listing so
it reflects the current roadmap scope: change the “A–I” mention to “A–J” in the
descriptive line for the phases/ section, add an entry for phase-j.md in the
enumerated list (with its appropriate detail level), and confirm README.md and
current.md remain referenced as the index and “we are here” pointer respectively
so the ADR no longer references a stale phase range.
In `@docs/decisions/0014-capability-representation.md`:
- Around line 196-200: The ADR currently claims an overflowed generation causes
a slot to be "permanently marked as used-and-inaccessible" but the Slot layout
(generation: u32, entry: Option<SlotEntry>, next_free) lacks that state; update
the document to specify the concrete mitigation: either (1) add a persistent
poison indicator to the slot representation (e.g., a poisoned: bool field or
reserve sentinel generation value like u32::MAX) and state the invariant that
alloc() (or the allocation path that examines generation/next_free/entry) must
skip poisoned slots, or (2) explicitly mark this mitigation as unimplemented and
remove the claim that slots are made inaccessible until a v2 change; reference
CAP_TABLE_CAPACITY, generation, entry: Option<SlotEntry>, next_free and the
alloc path when describing which symbols need to enforce skipping/poisoning.
- Line 240: The markdown link for ADR-0017 in the "Badge semantics." sentence is
pointing to `.` (an empty target); update that link in the "Badge semantics."
line so it either points to the correct ADR file name (e.g., replace `(.)` with
`(0017-ipc-endpoints.md)` or the actual reserved path) or remove the hyperlink
and change the text to plain "ADR-0017 (to be written in A4)"; locate the phrase
"Badge semantics. Per-derivation discriminators for IPC endpoints..." and edit
the `(.)` target accordingly.
In `@docs/roadmap/current.md`:
- Around line 17-19: Remove volatile execution details (exact test counts and
deep implementation notes) from the T-001 / ADR-0014 entries in current.md and
replace them with a concise pointer-style summary: note that T-001 landed on
development, reference ADR-0014 and the new capability types
(cap::CapabilityTable, cap::CapHandle, cap::CapRights, cap::Capability,
cap::CapError) without numeric/runtime specifics (drop the 61/34/27 test counts
and the explicit CAP_TABLE_CAPACITY value), and add a single link to the
task/review artifact for evidence and further details; keep one short note that
the module is not yet wired into run but avoid implementation detail.
In `@docs/roadmap/phases/phase-a.md`:
- Line 53: Update the status of the task entry "T-001 — Capability table
foundation" in the Phase A roadmap so it matches the source-of-truth status ("In
Review"); locate the list item for T-001 in phase-a.md and change its trailing
status text from "Ready." to "In Review." to keep task statuses consistent.
- Line 68: The roadmap maps Milestone A3 to ADR-0015 ("Kernel object storage")
but ADR-0015 is already used by the AI integration decision; update the ADR
reference used by Milestone A3 to a unique, correct ADR id across all A3
occurrences (the three instances mapping A3 → ADR-0015 at the “Kernel object
storage” entry), and then update the ADR index/ledger and any cross-references
so ADR-0015 remains the AI-integration decision and the new ADR id points to the
kernel object storage decision; search for the symbols "Milestone A3",
"ADR-0015", and "Kernel object storage" to locate and change every reference
consistently.
In `@docs/roadmap/README.md`:
- Line 5: Update the textual phase counts: in the top-level README replace the
phrase "Phase A through Phase I" with "Phase A through Phase J" (the line
containing "1. **Where are we going?** — [`phases/`](phases/) (Phase A through
Phase I)"). In the phases README replace the phrase "Nine phases" with "Ten
phases" (the opening line that currently states "Nine phases") so both files
consistently reflect phases A–J.
In `@kernel/src/cap/rights.rs`:
- Around line 36-43: CapRights::from_raw currently accepts arbitrary u32 bits
which lets callers pass reserved/unknown bits and can weaken contains()/subset
checks; change from_raw to either mask incoming bits to a defined KNOWN_BITS
mask (e.g., apply bits & KNOWN_BITS) or return Result to error on any unknown
bits, and update the function signature and docs accordingly; locate and modify
the CapRights::from_raw constructor and ensure callers that depend on raw
construction (ABI boundary code) handle the new masked/validated value or the
Result, keeping contains() semantics unchanged.
In `@kernel/src/cap/table.rs`:
- Around line 382-393: cap_drop currently unlinks and frees a slot without
touching descendants, orphaning children; change cap_drop (in table.rs) to first
check the slot's first_child and if Some(...) return a new CapError::HasChildren
instead of proceeding, avoiding orphaning and preserving ADR-0014 invariants;
update the CapError enum to include HasChildren, update any callers/tests to
expect this error, and add a unit test "cap_drop on an interior node" that
creates a parent with at least one child and asserts cap_drop(parent_handle)
returns CapError::HasChildren (alternatively, if you prefer cascade free or
reparenting, implement the corresponding logic inside cap_drop similar to
cap_revoke or by reparenting children to the grandparent, and add the same test
to lock behavior).
---
Nitpick comments:
In @.claude/skills/conduct-review/SKILL.md:
- Around line 48-52: Update the commit-message template that currently uses
"docs(roadmap): <type>-review <scope>" so the subject matches the actual docs
path for non-business reviews; change it to "docs(analysis): <type>-review
<scope>" (or make roadmap only for business reviews) and update the examples and
guidance in SKILL.md where the template string appears to recommend using the
correct subject for code/security/performance reviews so git logs and grep align
with docs/analysis/reviews/<type>/.
In @.claude/skills/start-task/SKILL.md:
- Around line 24-27: The T-NNN assignment is racy when multiple task-creation
flows run concurrently; update the SKILL.md "Assign the next T-NNN" step to
either (a) document the single-writer assumption and require running on a
rebased branch with an explicit "re-run if a newer T-NNN appears at merge time"
note, or (b) describe an alternative reserve-by-ledger approach (e.g., a
lightweight ledger file that a single writer updates atomically) and reference
the filenames/patterns involved (T-NNN and docs/analysis/tasks/phase-*/T-*.md)
so implementers know where to apply it; pick one approach for v1 (prefer the
single-writer footnote) and make the instruction text precise.
In `@docs/analysis/reviews/code-reviews/master-plan.md`:
- Around line 56-70: Polish the repeated checklist bullet phrasing under the "4.
Documentation" section by varying sentence openers and sentence structure for
the bullets "Every new public item has a rustdoc comment.", "Every `unsafe fn`
has a `# Safety` section.", "Every new `Error` variant is listed in an `#
Errors` section where applicable.", and the two conditional bullets starting "If
the change affects…". Rephrase one or two bullets to start with a different verb
or clause (e.g., "Ensure a rustdoc comment exists for every new public item",
"Add a `# Safety` section for each `unsafe fn`", "Document new `Error` variants
in an `# Errors` section", and "Update docs/architecture when architecture is
affected" or "Create a guides entry for user-facing workflow changes") so the
checklist reads less repetitive while preserving the original checks and
meaning.
In `@docs/analysis/tasks/TEMPLATE.md`:
- Line 22: The paragraph in TEMPLATE.md contains three consecutive sentences
that all start with "What", harming readability; update that placeholder
paragraph by rephrasing at least two of the sentences to vary sentence openings
and improve flow (e.g., replace "What situation motivates this task?" with
"Describe the situation that motivates this task.", change "What would go wrong
if it were skipped?" to "Explain the consequences if it is skipped.", and turn
"What has happened before this in the project?" into "Summarize prior work or
linked ADRs and tasks."), preserving the same content and links to ADRs/prior
tasks.
In `@docs/roadmap/phases/phase-b.md`:
- Around line 154-157: Three successive bullets all begin with "Whether…",
hurting scanability; update the three items referencing ADR-0023 and ADR-0024 so
each starts with a different verb phrase (e.g., "Choose between ELF and raw
binary (ADR-0023).", "Decide if syscalls should be synchronous-only or include
async variants (ADR-0024).", "Determine whether the initial userspace is its own
binary target or embedded via include_bytes! (ADR-0023).")—keep the ADR IDs and
original meaning, only vary the opening phrasing for better readability.
In `@docs/roadmap/phases/phase-d.md`:
- Around line 170-172: The bullet about "Whether the SD image includes the Pi's
closed-source firmware blobs" is marked under Open questions but already
answered by ADR-0004; update the document so this item is not listed as open:
either move that bullet into a "Resolved" or "Notes" section, or add a
parenthetical "Resolved per ADR-0004: closed-source blobs below the kernel are
allowed" next to the bullet; reference the exact phrase "Whether the SD image
includes the Pi's closed-source firmware blobs" and cite ADR-0004 in the
moved/resolved text to make the resolution explicit.
In `@kernel/src/cap/mod.rs`:
- Around line 63-64: CapObject currently exposes its inner u64 as a public field
allowing arbitrary construction/mutation; make the tuple field private (remove
pub) and add a const fn new(id: u64) -> Self constructor and a pub const fn
raw(&self) -> u64 accessor so callers can only create/read values via these
functions; update any call sites that previously constructed CapObject(...) or
accessed .0 to use CapObject::new(...) and .raw() respectively to keep
construction auditable ahead of the Milestone A3 typed-ref replacement.
In `@kernel/src/cap/table.rs`:
- Around line 334-367: The BFS loop in cap::table (the descendant collection
code using descendants, desc_len, CAP_TABLE_CAPACITY and returning
CapError::CapsExhausted) treats an internal invariant violation (cycle /
duplicate node) as an allocation failure; change the two guards that check
desc_len >= CAP_TABLE_CAPACITY to assert internal invariants instead of
returning CapsExhausted — e.g., replace the early Err(CapError::CapsExhausted)
in both the sibling-traversal and child-expansion loops with
debug_assert!(desc_len < CAP_TABLE_CAPACITY, "derivation tree contains cycle or
duplicate node") or, if you prefer an explicit runtime error, return a new
variant like CapError::InternalCycleDetected (update CapError enum and call
sites accordingly) so callers can distinguish genuine capacity exhaustion from
internal bugs.
- Around line 248-257: In cap_derive the call self.resolve_handle(src)? is
redundant because entry_of(src)? already validates and resolves the handle;
remove the extra resolve_handle call and obtain the index from the result of
entry_of (or reuse src.index after entry_of succeeds). Update the block that
builds (kind, rights, parent_index, parent_depth) to take idx from the returned
entry (or src.index) instead of calling self.resolve_handle, keeping references
to entry.capability.kind(), entry.capability.rights(), and entry.depth
unchanged.
🪄 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: 1ac989cf-2125-4249-a1ca-0839478e09be
📒 Files selected for processing (49)
.claude/skills/README.md.claude/skills/conduct-review/SKILL.md.claude/skills/start-task/SKILL.mdAGENTS.mdCLAUDE.mddocs/analysis/README.mddocs/analysis/reviews/README.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/business-reviews/master-plan.mddocs/analysis/reviews/code-reviews/README.mddocs/analysis/reviews/code-reviews/master-plan.mddocs/analysis/reviews/performance-optimization-reviews/README.mddocs/analysis/reviews/performance-optimization-reviews/master-plan.mddocs/analysis/reviews/security-reviews/README.mddocs/analysis/reviews/security-reviews/master-plan.mddocs/analysis/tasks/README.mddocs/analysis/tasks/TEMPLATE.mddocs/analysis/tasks/phase-a/README.mddocs/analysis/tasks/phase-a/T-001-capability-table-foundation.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-c/README.mddocs/analysis/tasks/phase-d/README.mddocs/analysis/tasks/phase-e/README.mddocs/analysis/tasks/phase-f/README.mddocs/analysis/tasks/phase-g/README.mddocs/analysis/tasks/phase-h/README.mddocs/analysis/tasks/phase-i/README.mddocs/analysis/tasks/phase-j/README.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0014-capability-representation.mddocs/decisions/0015-ai-integration-stance.mddocs/decisions/README.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/phases/README.mddocs/roadmap/phases/phase-a.mddocs/roadmap/phases/phase-b.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-f.mddocs/roadmap/phases/phase-g.mddocs/roadmap/phases/phase-h.mddocs/roadmap/phases/phase-i.mddocs/roadmap/phases/phase-j.mdkernel/src/cap/mod.rskernel/src/cap/rights.rskernel/src/cap/table.rskernel/src/lib.rs
|
|
||
| The roadmap is **the plan** — what we intend to build and in what order. It answers two questions: | ||
|
|
||
| 1. **Where are we going?** — [`phases/`](phases/) (Phase A through Phase I). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify phase count across roadmap documentation
echo "=== Checking phases/README.md phase count ==="
rg -n "Nine phases|phase-[a-j]\.md" docs/roadmap/phases/README.md
echo -e "\n=== Checking phase file existence ==="
fd -e md "phase-" docs/roadmap/phases/ -x echo "Found: {}"
echo -e "\n=== Checking all references to phase ranges ==="
rg -n "Phase [A-Z].*through.*Phase [A-Z]|phases [A-Z].*[A-Z]" docs/roadmap/Repository: cemililik/UmbrixOS
Length of output: 2173
Update phase count references for consistency.
Line 5 says "Phase A through Phase I" but Phase J exists in docs/roadmap/phases/README.md. Additionally, the opening line of phases/README.md states "Nine phases" when there are actually 10 phases (A through J). Update both references:
- Line 5 of
README.md: Change "Phase A through Phase I" to "Phase A through Phase J" - Line 3 of
phases/README.md: Change "Nine phases" to "Ten phases"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/roadmap/README.md` at line 5, Update the textual phase counts: in the
top-level README replace the phrase "Phase A through Phase I" with "Phase A
through Phase J" (the line containing "1. **Where are we going?** —
[`phases/`](phases/) (Phase A through Phase I)"). In the phases README replace
the phrase "Nine phases" with "Ten phases" (the opening line that currently
states "Nine phases") so both files consistently reflect phases A–J.
External-codebase reading notes (e.g. the Writing-an-OS-in-Rust study) are kept locally under docs/analysis/technical-analysis/ to inform design conversations but are not intended to ship with the repository. Gitignore the folder so it stays out of git while remaining available on the maintainer's machine. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ved patterns
Three overlapping sets of changes landed together because the renumber
cascade and the review feedback both touch phase-a.md.
ADR renumber cascade. ADR-0015 was planned for A3 "Kernel object storage"
in phase-a.md's ledger but ended up taken by the AI-integration decision
that landed out of sequence. Shift every reserved ADR id by +1 starting at
0015 to restore a contiguous, non-colliding reservation range: A3 becomes
ADR-0016, A4 IPC→0017, A4 Badge→0018, A5 Scheduler→0019, A5 Cpu v2→0020,
then Phase B 0021–0026, Phase C 0027–0031, Phase D 0032–0036,
Phase E 0037–0042, Phase F 0043–0046, Phase G 0047–0051,
Phase H 0052–0054, Phase I 0055–0057. Existing cross-references to
ADR-0015 as AI-integration (phase-g, phase-j) are preserved.
WOSR-derived pattern notes. Three patterns worth naming in advance so the
relevant milestone's ADR, when it lands, explicitly weighs them:
* phase-a A3 — consider a fixed-size-block allocator per kernel-object
kind (same shape as the existing CapabilityTable arena).
* phase-b B2 — Mmu trait mutations return a typed "must-acknowledge"
flush token (analogous to MapperFlush in the x86_64 crate); silent
drop produces a compile-time warning.
* phase-c C3 — introduce Cpu::without_interrupts(|| …) backed by DAIF
manipulation; every spinlock an IRQ handler can touch must be
acquired inside it.
PR review feedback. Inline and nitpick comments from the T-001+Plan-C PR:
* T-001 dependency wording: "Phase 4c" → "Milestone A1"; status
"Ready" → "In Review" in phase-a.md.
* current.md tightened — volatile test counts and capacity constants
replaced by a pointer to the task file.
* ADR-0013 phases/README phase-range A–I → A–J; phase-j.md added to
the listed tree.
* ADR-0014 generation-overflow mitigation marked unimplemented in v1
with an explicit sentinel/poison plan; three-pointer tree-link count
corrected from four; stale (.) link on "Badge semantics" rewritten as
plain text ("a future ADR to be written in A4").
* roadmap README "A–I"→"A–J"; phases README "Nine"→"Ten".
* phase-h legacy "Phase 4b" reference → "Phase A" (scope and
open-question bullet).
* phase-d firmware-blob question moved from "Open" to "Resolved" with
an ADR-0004 citation.
* phase-b open questions reworded to vary sentence openers.
* TEMPLATE.md Context paragraph reworded (three "What" openers
replaced).
* code-review master-plan Documentation bullets rephrased to vary
sentence openers.
* conduct-review SKILL.md commit scope: "roadmap" for business
reviews, "analysis" for code/security/performance reviews.
* start-task SKILL.md documents the single-writer assumption and
re-run-if-collision convention for T-NNN allocation.
Refs: ADR-0013, ADR-0014, ADR-0015
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five findings from the T-001 review, each verified against the current code and addressed. All 63 host tests pass (29 kernel, 34 test-hal); host-clippy and kernel-clippy are clean; rustfmt is clean. * CapRights::from_raw masks to known bits. A new associated constant CapRights::KNOWN_BITS names the union of every defined right; from_raw applies `bits & KNOWN_BITS` so a caller cannot smuggle reserved bits past contains() / subset checks. A new test `from_raw_masks_unknown_bits` covers the masking and the "reserved bits collapse to EMPTY" case. * cap_drop refuses to orphan descendants. A new CapError::HasChildren variant is returned when cap_drop is called on an interior node; callers must cap_revoke the subtree first. This keeps the derivation-tree invariant from ADR-0014 auditable and leaves cascade semantics to cap_revoke. A new test `cap_drop_on_interior_node_returns_has_children` covers the refusal and the "revoke-then-drop works" follow-up. * CapObject encapsulated. The inner u64 is private; construction goes through `CapObject::new(id)` and reads through `CapObject::raw()`, so every touch site is auditable ahead of the Milestone A3 typed-ref replacement. All in-kernel test call sites and the ADR-0014 snippet are updated accordingly. * cap::table BFS treats invariant violations as bugs, not capacity errors. The two desc_len overflow guards in cap_revoke now use `debug_assert!` plus a release-build `break` instead of returning CapError::CapsExhausted. A cycle or duplicate node in the derivation tree is an internal bug (the descendant count cannot exceed CAP_TABLE_CAPACITY); CapsExhausted implied user-visible capacity exhaustion, which was misleading. * cap_derive redundant resolve_handle call removed. entry_of(src) already validates and resolves the handle; the second self.resolve_handle(src)? on the next line was duplicate work. The block now reads parent_index straight from src.index after entry_of succeeds. Refs: ADR-0014 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
docs/roadmap/phases/phase-h.md (1)
41-41: Prefer tighter wording: “inaccessible”Line 41 can be simplified to “Jetson's GPU / NPU are inaccessible.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap/phases/phase-h.md` at line 41, Replace the sentence "Release notes are explicit: Jetson's GPU / NPU are not accessible." with the tighter wording "Jetson's GPU / NPU are inaccessible." so the phrasing is concise; update the exact string in the docs file where that sentence appears.docs/decisions/0013-roadmap-and-planning.md (2)
22-22: Capitalize "Markdown" as a proper noun."Markdown" is the name of the formatting language and should be capitalized.
✏️ Proposed fix
-- **Task-as-user-story.** Each concrete unit of work is a short markdown document in a consistent format. The three-part narrative (role, capability, benefit) is flexible enough for kernel-internal work where the "user" is another subsystem. +- **Task-as-user-story.** Each concrete unit of work is a short Markdown document in a consistent format. The three-part narrative (role, capability, benefit) is flexible enough for kernel-internal work where the "user" is another subsystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/0013-roadmap-and-planning.md` at line 22, Update the sentence that currently reads "**Task-as-user-story.** Each concrete unit of work is a short markdown document in a consistent format." to capitalize "Markdown" as the proper noun so it reads "...short Markdown document..."; locate the string "Task-as-user-story." in the docs/decisions/0013-roadmap-and-planning.md and replace "markdown" with "Markdown".
41-41: Add language specification to code fence.The folder layout code block should specify a language identifier for consistent rendering and to satisfy markdown linters.
📝 Proposed fix
-``` +```text docs/ ├── roadmap/ — the plan, in order of execution🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/0013-roadmap-and-planning.md` at line 41, The code fence in the folder layout block inside docs/decisions/0013-roadmap-and-planning.md lacks a language identifier; update the opening triple backticks for that snippet (the fenced block showing the "docs/" folder layout) to include a language tag such as "text" (i.e., change ``` to ```text) so markdown renderers and linters consistently handle and format the snippet.docs/analysis/reviews/code-reviews/master-plan.md (1)
54-56: Optional: Vary sentence structure for stylistic improvement.Three consecutive bullet points begin with "If," which static analysis flagged. The current structure is clear and functional, but you could optionally vary the phrasing for stylistic polish.
✍️ Alternative phrasing (optional)
- Is the public API of the change exercised? -- If the change is a fix, is there a regression test that fails before and passes after? -- If a new `Error` variant was added, is there a test that provokes it? -- If the change is behavioural, is there a QEMU smoke that demonstrates the behaviour? +- For fixes: is there a regression test that fails before and passes after? +- For new `Error` variants: is there a test that provokes it? +- For behavioural changes: is there a QEMU smoke that demonstrates the behaviour?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/reviews/code-reviews/master-plan.md` around lines 54 - 56, The three consecutive bullets that all start with "If" should be rewritten to vary sentence openings for stylistic polish: update the three list items (the bullets about regression tests, new Error variant tests, and QEMU smoke tests) so one or two begin with alternative phrasing such as "When", "Ensure", or "Confirm that" while keeping the original meaning and test checklist content; edit the three bullet texts (the lines mentioning regression test, Error variant test, and QEMU smoke) to use varied sentence structure but preserve their intent and clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/tasks/phase-a/T-001-capability-table-foundation.md`:
- Line 29: The document uses invalid Rust module path notation
"umbrix-kernel::cap" in two places; update those occurrences to the canonical
Rust identifier form "umbrix_kernel::cap" so references like the
`CapabilityTable` type and the cap module match the rest of the docs (replace
"umbrix-kernel::cap" with "umbrix_kernel::cap" wherever it appears, including
the lines referencing the `CapabilityTable` type and the roadmap linkage).
In `@docs/roadmap/phases/phase-h.md`:
- Line 61: The document incorrectly names the closure target as "Pi 4" in Phase
H; update the Phase H wording to "Pi 5" to match Milestone H1 and the `bsp-pi5`
references—specifically replace the "Pi 4" occurrence in the Phase H description
with "Pi 5" so the phrase and business review sentence align with `bsp-pi5` and
Milestone H1.
- Around line 45-50: The candidate list names ESP32-C6 which lacks an MMU but
this milestone validates the Mmu abstraction; update the H3 candidate selection
to only include MMU-capable RISC-V boards (e.g., SiFive HiFive variants) or
explicitly split the milestone into MMU vs non-MMU paths via an ADR; adjust the
text around ADR-0054 and the mention of Mmu / Cpu / IrqController / Timer to
either constrain candidates to MMU-capable hardware or add a new ADR that
documents separate MMU/non-MMU validation tracks and their acceptance criteria.
---
Nitpick comments:
In `@docs/analysis/reviews/code-reviews/master-plan.md`:
- Around line 54-56: The three consecutive bullets that all start with "If"
should be rewritten to vary sentence openings for stylistic polish: update the
three list items (the bullets about regression tests, new Error variant tests,
and QEMU smoke tests) so one or two begin with alternative phrasing such as
"When", "Ensure", or "Confirm that" while keeping the original meaning and test
checklist content; edit the three bullet texts (the lines mentioning regression
test, Error variant test, and QEMU smoke) to use varied sentence structure but
preserve their intent and clarity.
In `@docs/decisions/0013-roadmap-and-planning.md`:
- Line 22: Update the sentence that currently reads "**Task-as-user-story.**
Each concrete unit of work is a short markdown document in a consistent format."
to capitalize "Markdown" as the proper noun so it reads "...short Markdown
document..."; locate the string "Task-as-user-story." in the
docs/decisions/0013-roadmap-and-planning.md and replace "markdown" with
"Markdown".
- Line 41: The code fence in the folder layout block inside
docs/decisions/0013-roadmap-and-planning.md lacks a language identifier; update
the opening triple backticks for that snippet (the fenced block showing the
"docs/" folder layout) to include a language tag such as "text" (i.e., change
``` to ```text) so markdown renderers and linters consistently handle and format
the snippet.
In `@docs/roadmap/phases/phase-h.md`:
- Line 41: Replace the sentence "Release notes are explicit: Jetson's GPU / NPU
are not accessible." with the tighter wording "Jetson's GPU / NPU are
inaccessible." so the phrasing is concise; update the exact string in the docs
file where that sentence appears.
🪄 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: 7ad1f93a-0b9d-4dd2-b7c4-f2148e76ba77
📒 Files selected for processing (23)
.claude/skills/conduct-review/SKILL.md.claude/skills/start-task/SKILL.md.gitignoredocs/analysis/reviews/code-reviews/master-plan.mddocs/analysis/tasks/TEMPLATE.mddocs/analysis/tasks/phase-a/T-001-capability-table-foundation.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0014-capability-representation.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/phases/README.mddocs/roadmap/phases/phase-a.mddocs/roadmap/phases/phase-b.mddocs/roadmap/phases/phase-c.mddocs/roadmap/phases/phase-d.mddocs/roadmap/phases/phase-e.mddocs/roadmap/phases/phase-f.mddocs/roadmap/phases/phase-g.mddocs/roadmap/phases/phase-h.mddocs/roadmap/phases/phase-i.mdkernel/src/cap/mod.rskernel/src/cap/rights.rskernel/src/cap/table.rs
✅ Files skipped from review due to trivial changes (12)
- .gitignore
- docs/roadmap/phases/phase-g.md
- docs/analysis/tasks/TEMPLATE.md
- docs/roadmap/current.md
- docs/roadmap/phases/phase-i.md
- docs/roadmap/README.md
- docs/roadmap/phases/phase-c.md
- docs/roadmap/phases/README.md
- .claude/skills/conduct-review/SKILL.md
- docs/decisions/0014-capability-representation.md
- docs/roadmap/phases/phase-b.md
- docs/roadmap/phases/phase-f.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/skills/start-task/SKILL.md
- kernel/src/cap/mod.rs
- kernel/src/cap/table.rs
Inline and nitpick findings from a follow-up review pass, each verified against the current text: * T-001: crate path written as "umbrix-kernel::cap" used a Cargo-name hyphen where a Rust module path requires an underscore. Both occurrences (acceptance-criterion bullet and approach step 2) now read "umbrix_kernel::cap", matching phase-a.md. * phase-h closure sentence said "Pi 4, Jetson, RISC-V" — but Milestone H1 introduces bsp-pi5, not a second Pi 4 BSP. Corrected to "Pi 5, Jetson, RISC-V". * phase-h H3 candidate list included ESP32-C6, which lacks an MMU and therefore cannot exercise the `Mmu` trait the milestone is there to validate. Narrowed the candidate set to MMU-capable boards (SiFive HiFive Unmatched/Unleashed, StarFive VisionFive 2), and explicitly noted that MMU-less RISC-V microcontrollers are out of scope for H3 and belong to a separately-scoped future milestone if that work is ever done. * phase-h Jetson acceptance criterion: tightened "Release notes are explicit: Jetson's GPU / NPU are not accessible." to "... are inaccessible." * ADR-0013: "short markdown document" → "short Markdown document" (Markdown is the proper name of the format). * ADR-0013: folder-layout code fence now has an explicit `text` language tag, matching documentation-style.md's rule that fenced blocks must carry a language identifier. * code-reviews master-plan: three consecutive "If …" bullets in the test-coverage section reworded to vary sentence openers while preserving the original checks. Refs: ADR-0013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 21, 2026
PR #1 merged T-001 (capability table foundation) into main. Transition the bookkeeping: * T-001 status: In Review → Done; review history records the merge. * Milestone A2 marked done (2026-04-21). * Phase-a task index reflects Done status. * current.md repoints: last-completed milestone is A2, last-completed task is T-001, active milestone is A3, active task is the T-002 draft. The first business review (A2 completion) is the next trigger. Refs: ADR-0013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 21, 2026
Integrate every follow-up from the 2026-04-21 Phase-A code and security reviews into phase-b.md. No review item remains in an ad-hoc ledger; each is mapped to a specific B0..B6 milestone, and every decision that must close during Phase B is 🚩-flagged in its milestone and collected under the plan's "Open questions" section. New milestone **B0 — Phase A exit hygiene** prepends the original EL-drop pipeline and absorbs: - Security-review blockers #1 / #2 / #3 — UNSAFE-2026-0012 raw-pointer refactor, cross-table revocation policy, scheduler deadlock panic → idle task + typed error — as ADR-0021 / ADR-0022 / ADR-0023. - Code-review follow-ups: architecture docs for kernel-objects / IPC / scheduler; missing tests (ReceiverTableFull, slot-reuse with pending transfer cap, returns-Err replacements for should-panic tests); const {assert!(N>0)} type-level invariants on SchedQueue and CapabilityTable; debug_assert → typed-error hardening. - Non-blocking tracking items, placed in their natural milestone: generation wrap (B2 flag), fault containment scope (B5 flag → Phase E for full supervisor), Capability::Debug redaction (B5 before console_write syscall), write_bytes TX timeout (B6 flag), QEMU-smoke CI gate (B6 flag), cargo-vet init (B6 flag), TaskArena local → global StaticCell migration (bundled with T-006), boot.s early DAIFSet (B1 BSP checklist), TPIDR_EL0 save-set watch (TLS trigger, no decision yet). ADR numbers shifted: the pre-review 0021..0026 (EL-drop, MMU, AS, loader, syscall ABI, initial syscalls) become 0024..0029 so the B0 blockers can claim 0021..0023. Final numbers are assigned when the ADR is actually written per ADR-0013. current.md updated: active phase B, active milestone B0, first task to open is T-006 (raw-pointer scheduler API refactor). Also: docs/analysis/temp/ added to .gitignore as a local-only transient workbook directory (follow-up ledgers, drafts, scratch notes that do not belong in git history). Refs: ADR-0013 Code-Review: docs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.md Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 21, 2026
Expose the scheduler's IPC bridge (ipc_send_and_yield / ipc_recv_and_yield) in a form that never holds a live &mut to EP_ARENA / IPC_QUEUES / TABLE_* across cpu.context_switch — closing UNSAFE-2026-0012 (#1 Phase-B blocker from the 2026-04-21 security review) before any later milestone adds more shared state. TaskArena migration to a StaticCell is bundled in the same task to avoid a second round of BSP static-cell churn. 7 acceptance criteria; ADR-0021 writing is the first step inside the task. Status In Progress; current.md points at T-006; phase-b/README and phase-b.md updated. Refs: ADR-0013 Audit: UNSAFE-2026-0012 Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 26, 2026
…b66c Self-audit of T-009's second-read fix-up commit (39fb66c) found that Review 1's Yüksek #1 was half-implemented. The recommendation read: > Either (a) explicitly detect/handle EL2 ..., or (b) assert early > (and document) that boot environment guarantees EL1 before these > reads (and add a runtime check that panics with a clear error > if not). Commit 39fb66c took option (b) but landed only the documentation half (ADR-0012 cite, EL precondition wording, removal of "unconditional" claim) — the parenthetical "and add a runtime check" was silently skipped. This commit closes the runtime-check half. Implementation: - QemuVirtCpu::new gains an MRS CurrentEL read at the head of the function, before any generic-timer access. The 2-bit Exception-Level field is shifted out and asserted == 1; a mismatch panics with the observed EL ("must run at EL1 per ADR-0012; observed EL{n} instead"). - The cost is one MRS + one compare per CPU construction. Negligible. The assertion runs once per kernel boot. - One new audit entry: UNSAFE-2026-0016 (boot-time CurrentEL self-check), shaped like UNSAFE-2026-0007 (read-only system register at EL≥1, no state mutation, options(nostack, nomem)). The doc-comment on QemuVirtCpu::new was reorganised so the # Panics section names both invariant violations (EL≠1 and CNTFRQ_EL0==0) with their respective audit tags. The CNTFRQ MRS's SAFETY comment now notes the EL-precondition reasoning is "not just documentation but a checked invariant" — closing the gap between paper claim and runtime behaviour. Lesson — recorded in the T-009 task file's review-history row as the adjustment to carry forward — review recommendations with AND/OR structure should be treated as full asks: documentation + runtime check is the (b) option's complete shape, not "either of the two". Verification: 124 host tests green, miri 124/124 clean, all clippy/ fmt/build/YAML gates clean. QEMU smoke shows the new EL=1 assertion passes (we are at EL1, as ADR-0012 specifies) and the eight-line boot trace continues unchanged. Refs: ADR-0012, ADR-0010, T-009 second-read Review 1 Yüksek #1 Audit: UNSAFE-2026-0016 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 27, 2026
…le SAFETY) Verified each finding; all five applied as suggested. Inline #1 — boot.md halt-loop syntax in Mermaid diagram + body text - Line 41 (Mermaid sequence diagram): `halt: wfe; b .-` → `halt_unsupported_el: wfe ; b halt_unsupported_el`. The named-label form matches the actual asm in `bsp-qemu-virt/src/boot.s` and removes the malformed `.-` token. - Line 16 (text description): `wfe; b .-` halt loop wording corrected in two places — the EL3 path now reads "named-label `wfe`-loop (`halt_unsupported_el: wfe ; b halt_unsupported_el`)" and the kernel_entry-return defensive halt now reads `wfe ; b 2b` (matching the local-label loop at the bottom of `_start`). Inline #2 — UNSAFE-2026-0017 GAS halt-loop syntax (Amendment) - §Operation said "halt via `wfe; b -1b`". `-1b` is not valid GAS syntax — `1b` is the back-reference to local label `1:`, but a leading `-` is meaningless there. Real asm uses `b halt_unsupported_el`. - §Rejected alternatives → "Halt on EL3 with a panic frame" said "`wfe; b .-` is the visible silence". `b .-` is a similar malformed token (`.` is the current address; `b .-` with no offset is not a valid branch target). - Both occurrences corrected via an Amendment block per unsafe-policy.md §3 (the entry was committed in f289d4d / T-013; in-place edits to a committed entry's body are forbidden). The behaviour the audit describes is unchanged; only the prose's asm rendering was wrong. Outside-diff #4 — bsp-qemu-virt/src/cpu.rs SAFETY blocks for CNTFRQ_EL0 (`new()`) + CNTVCT_EL0 (`now_ns()`) - Both blocks claimed "boot.s performs no EL transition" — true before T-013, stale after. Updated to reflect ADR-0024: `boot.s` drives an EL2 → EL1 transition when the firmware/emulator delivers at EL2, falls through at EL1, halts at EL3. The runtime EL-check via `tyrne_hal::cpu::current_el()` (audited under UNSAFE-2026-0018) is the checked invariant pinning `CurrentEL == 1` at this point. Both SAFETY paragraphs now name UNSAFE-2026-0017 (the boot.s sequence) explicitly, and document (a) why-unsafe-is- required, (b) invariants, (c) rejected alternatives per CLAUDE.md rule 2. Nitpick — UNSAFE-2026-0018 cfg-gating wording (Amendment) - §Invariants relied on → "Cfg-gating" said "user code reading `CurrentEL` would trap or yield `EL0` with no useful information". The "or yield `EL0`" alternative is wrong: per ARM ARM §D11.2 / §C5.2, `MRS x, CurrentEL` at EL0 is undefined — the system register is not accessible at EL0 and the read raises an Undefined Instruction exception (which becomes `SIGILL` on hosted Unix-like targets such as `aarch64-apple-darwin`). There is no fallback EL0 read. Corrected via Amendment block (same §3 reasoning as UNSAFE-2026-0017 above). Nitpick — T-008 task file:120 review-history release-note structure - Single dense paragraph split into structured bullets using `<br/>•` separators inside the table cell: New docs / Updated docs / Scope discipline / Verification / State transition. Same factual content, scannable. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel- build clean; host-test 143 / 143 green. Refs: ADR-0024 Audit: UNSAFE-2026-0015, UNSAFE-2026-0017, UNSAFE-2026-0018 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 27, 2026
Verified each open item from the second review pass. All applied; the deferred Q1 settled with a strict-§3 discipline call. Q1 (HCR_EL2 Amendment) — strict discipline picked over carve-out The first review-round (commit 9a8e312) added the HCR_EL2 literal- write rationale to UNSAFE-2026-0017's §Invariants relied on as an in-place body edit. The second review-round flagged this as asymmetric against V13/V14 (which went via Amendment) and offered two paths: (1) codify a same-PR-mutable carve-out in unsafe-policy.md §3, or (2) convert V7 to an Amendment. Picked Path 2 (strict). Reasoning, in detail: - The asymmetry within the PR is the actual problem; Path 1 does not fix it because V13/V14 went via Amendment despite being same- PR. The carve-out would need a sub-rule "additions in-place, corrections via Amendment" — a gray-zone rule that just opens more gray zones. - Precedent: UNSAFE-2026-0006's 2026-04-23 Amendment for the post- T-009 QemuVirtCpu struct shape is exactly this pattern — adding to an existing entry's content via Amendment. Strict §3 reading matches the precedent. - The §3 text "must not be rewritten once committed" is unambiguous when read literally. The strict reading is what the rule says. - "Merge boundary" as the locking event introduces a second ambiguity (PR merge ≠ commit, and not observable from inside the audit log). Strict introducing-commit boundary is auditable. Mechanics: removed the HCR_EL2 paragraph from §Invariants relied on (reverting to f289d4d's introducing-commit shape); added a new Amendment block at the end of UNSAFE-2026-0017 with the same content + a "Discipline note for future readers" paragraph documenting the introducing-commit boundary so the next PR does not stumble into the same trap. The existing 2026-04-27-second-review-round Amendment (GAS halt-loop syntax correction) stays as-is, now placed after the HCR_EL2 Amendment in chronological order (first review-round commit 9a8e312 → HCR_EL2; second review-round commit 39dd978 → GAS-syntax). Orta #1 — T-008 + T-013 task-file checkbox flips Same discipline T-011 received in 9a8e312, applied to T-008 and T-013. Every delivered AC + DoD item flipped to [x] with parenthetical delivery notes. T-013's two QEMU smoke items (AC "Tests — QEMU smoke at default config" / "QEMU smoke at -machine virtualization=on" + DoD's QEMU smoke line) are *intentionally* left at [ ] — settled item 8 from the second review pass: those gates are deferred to the maintainer / CI runner and the PR test plan checklists them explicitly. T-008's "Cross-references go both ways" DoD item is now [x] thanks to Düşük #3 below. Düşük #1 — boot.s:21 leading-comment typo Same wfe; b .- malformed token that V13 corrected via Amendment in UNSAFE-2026-0017 also appeared in bsp-qemu-virt/src/boot.s line 21 (the leading comment block, not the actual asm — the asm uses the named-label form correctly). In-source comment, not audit-log body; in-place fine. Replaced with the named-label loop description so the comment matches the asm. Düşük #2 — coverage rerun report follow-up note The 2026-04-27 coverage report's tables describe T-011's 761af95-tip state. The second review-round's drop_first_child_… test fix (in 9a8e312) bumped cap/table.rs regions from 97.46 % → 97.60 % (+0.14 pp), with a corresponding +0.04 pp on the workspace total (96.33 % → 96.37 %). The original tables are intentionally not rewritten — the report describes T-011's tip; the post-fix delta lives in a follow-up note appended to the report. Both AC gates (sched ≥ 90 %, workspace ≥ 96 %) remain comfortably met. Düşük #3 — bidirectional ADR ↔ architecture-doc cross-references T-008's DoD line 88 ("Cross-references go both ways") was deferred ("ADRs cited from architecture docs are the same ADRs whose §References sections cite the new architecture docs — or will cite, in their next rider"). Closed in this commit with five short §Revision notes riders pointing back at the architecture docs: - ADR-0010 (Timer trait) → architecture/hal.md Timer subsection. - ADR-0017 (IPC primitive set) → architecture/ipc.md. - ADR-0019 (Scheduler shape) → architecture/scheduler.md. - ADR-0020 (ContextSwitch + Cpu v2) → architecture/scheduler.md. - ADR-0022 (Idle task + typed Deadlock) → architecture/scheduler.md. ADR-0021 already had a direct architecture/scheduler.md citation (observed by the second review pass), so it does not need a rider. Each rider is one short paragraph dated 2026-04-27, framed "pointer to architecture doc" — same shape as ADR-0013's 2026-04-27 pointer-to-ADR-0025 rider. ADR-0019's rider also notes that its previously-open "Idle task" question was settled by ADR-0022. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel-build clean; host-test 143 / 143 green. Refs: ADR-0010, ADR-0017, ADR-0019, ADR-0020, ADR-0022, ADR-0024 Audit: UNSAFE-2026-0017 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
Two findings from the parallel approval review of f9c145c. One Orta (must-fix this round) + one Düşük (cheap to bundle). Orta #1 — B0 security review §8 + Verdict structural alignment with master plan The 8-axis structure followed master-plan §1..§7 names but §8 was named "Sign-off" rather than "Threat-model impact" (the master plan's defined axis 8). The verdict was folded into §8 instead of sitting in a separate ## Verdict section using the master plan's Approve / Changes requested / Escalate vocabulary. A reviewer walking the master plan template-by-template would not find their checklist. Substantive fix: - Rename ## 8. Sign-off → ## 8. Threat-model impact. - Answer the master plan's threat-model question explicitly. B0's arc does have two real threat-model shifts that the prior shape buried: • ADR-0024 promotes EL1-only execution from defense-in-depth to a structural property. Pre-T-013, the kernel relied on firmware/emulator delivering at EL1 with UNSAFE-2026-0016 as a last-line guard. Post-T-013, boot.s actively drives EL1 regardless of entry EL; UNSAFE-2026-0016 is the post-condition, not the only defence. Threat-model implication: the "we run at EL1" assumption is now an enforced invariant, not a hoped- for one. • ADR-0021 reshapes the aliasing model the kernel proves correctness against. Pre-T-006, &mut Scheduler<C> aliasing across context switches was latent UB (UNSAFE-2026-0012) relying on the optimiser. Post-T-006, the raw-pointer bridge plus momentary-&mut discipline is Stacked-Borrows-checked by Miri on every test run. Threat-model implication: memory- safety story shifted from "hope the compiler doesn't exploit our UB" to "checked invariant the test gate enforces." - Move two pre-existing items (cross-table revocation gap, generation overflow) under a "Threat-model items inherited from Phase A (unchanged severity)" sub-bullet of §8 — they are genuinely threat-model items, not sign-off bullets, so this is the right home for them post-rename. - Keep the two positive observations (Audit-log Amendment discipline; arch-docs as security multiplier) under §8 as "Positive observations about the review surface itself" — the review-surface framing makes them sub-bullets of §8 honestly, not orphaned sign-off prose. - Add separate ## Verdict section ending with **Approve.** keyword per master-plan vocabulary. Verdict prose preserved from the original §8 closing paragraph. Citation preservation: T-012:44 cites "B0 closure consolidated security review §8 recommendation ('Architecture docs are a security multiplier')". The §8 recommendation sub-bullet is preserved verbatim under the new heading; the citation continues to resolve. No T-012 edit needed. Düşük #1 — B0 retro Appendix sourcing line tightened The Appendix introduction read "The T-009 mini-retro's second- follow-up note pre-loaded six agenda items for this retro." More accurate: items 1, 2, 4, 5 originate in the second follow-up's substantive paragraphs; item 3 (ADR-0024 first-week rider rate) originates in the first follow-up; item 6 (stale-link CI check) was a routing decision in the second follow-up's Adjustments- status snapshot rather than a fresh preloading. Rephrased the introduction to acknowledge "two follow-up notes plus the original Adjustments section" with a per-item provenance parenthetical. The six rows themselves are unchanged; net delta ("six items in → four closed + two routed forward") still holds. Out of scope for this fix-round (per the approval review): - Düşük #2 (UNSAFE-2026-0006 §1/§8 placement) — reviewer noted this is a prompt-expectation mismatch, not a delivery gap; "note logged for completeness" was the verdict. Skipped. - Open questions — none from this approval review. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel-build / host-test (143 / 143) all green; doc-only changes confirmed via git diff --stat (29 lines net). Refs: ADR-0021, ADR-0024 Approval-Review: independent agent, 2026-04-27, f9c145c tip Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
Code (3 fixes):
1. `bsp-qemu-virt/src/exceptions.rs::irq_entry` — both inline SAFETY
blocks (`(*GIC.0.get()).assume_init_ref()` at line 139 and the
`msr cntv_ctl_el0` write at line 169) expanded to the full triplet
required by `unsafe-policy.md` §1: why `unsafe` is needed, the
exact invariants relied on (named symbols: `GIC`, `QemuVirtGic`,
`kernel_entry`, `assume_init_ref`, `CNTV_CTL_EL0`, EL1 + non-VHE
per ADR-0024 + UNSAFE-2026-0017), and why safer alternatives were
rejected (`Mutex`/`RefCell`/`OnceCell`-`Option` for the GIC borrow;
typed-register wrapper crates for the system-register write). The
previous comments covered invariants but elided rejected
alternatives, leaving them non-conforming under `§1`.
2. `bsp-qemu-virt/src/exceptions.rs::TrapFrame` — added a const
compile-time guard `const _: () = assert!(size_of::<TrapFrame>() ==
192);` immediately after the struct. The asm `stp` sequence in
`vectors.s` writes through fixed offsets and reserves exactly 192
bytes; a drift between the asm and the `repr(C)` would corrupt
saved registers on every IRQ. The guard fails the build before
that can ship. (Nitpick.)
3. `hal/src/timer.rs::ns_to_ticks_rounds_up_on_subtick` — new unit
test exercising freq = 3 Hz, ns = 333_333_334 → expected 2 ticks
(one tick = 333_333_333.333… ns; ns is 1 ns past tick 1, so the
ceiling tick count is 2). Pairs with a boundary check at ns =
333_333_333 → 1 tick. Existing `ns_to_ticks` tests use frequencies
that divide evenly into 1e9 ns/s, where ceiling and floor agree —
without this test a regression from `div_ceil` to floor division
would silently violate ADR-0010's "reaches or exceeds deadline_ns"
contract and the existing suite would not catch it. (Nitpick.)
Documentation (5 fixes):
4. `docs/architecture/exceptions.md` line 15 — replaced "ADR-0021 will
likely gain an Amendment when the handler is wired" with a
present-tense statement that ADR-0021 carries the 2026-04-28
Amendment recording the IRQ-frame extension, including a deep-link
to the ADR's §Revision notes anchor.
5. `docs/architecture/exceptions.md` line 205 — corrected the
ADR-0021 Amendment date from `(2026-04-27)` to `(2026-04-28)` to
match the actual amendment block in
`docs/decisions/0021-raw-pointer-scheduler-ipc-bridge.md`.
6. `docs/architecture/exceptions.md` lines 160-167 — the prose at
§"Generic-timer IRQ wiring" still listed step 3 as
`Calls sched::on_timer_irq(...)`, contradicting the shipped
ack-and-ignore body in `irq_entry`. Re-numbered to match the code:
ack → mask `CNTV_CTL_EL0` → `gic.end_of_interrupt(IrqNumber(27))`
→ return; added a paragraph explicitly preserving the v1 guarantee
that IRQ entry does not borrow scheduler state, with a link to the
ADR-0021 2026-04-28 Amendment that lays out the discipline for any
future scheduler-touching IRQ handler.
7. `docs/architecture/exceptions.md` line 244 — open-question bullet
for the ADR-0021 Amendment shape rephrased to past tense ("T-012
shipped the 2026-04-28 Amendment"); the residual open question is
now whether the first scheduler-touching IRQ handler should add a
follow-up Amendment recording the activation site, per the
discipline note already in the existing Amendment body.
8. `docs/decisions/0021-raw-pointer-scheduler-ipc-bridge.md` Amendment
§Revision notes — the cited IRQ-handler signature
`extern "C" fn(_frame: *mut TrapFrame)` updated to
`unsafe extern "C" fn(_frame: *mut TrapFrame)` to match the shipped
handler (PR #10 fix #1 already made `irq_entry` an `unsafe fn`;
the ADR text had drifted). New parenthetical explains that the
`unsafe fn` qualifier is part of the contract — the function has
caller-side preconditions documented in its `# Safety` section that
are now visible at the type level, with a link to
`bsp-qemu-virt/src/exceptions.rs::irq_entry`.
9. `docs/roadmap/current.md` line 31 — removed the "Mid-T-012 a code
review on the GIC + vector-table commit before WFI activation
lands" intermediate-gate clause from the "Next review trigger"
sentence; WFI activation already landed in `b4ed68c`, so the
intermediate gate is moot. The B1-closure trigger and the
2026-05-04 ADR-0024 rider-rate measurement remain.
Findings deliberately not actioned:
- Reviewer's request to "add a short comment next to irq_entry
and/or TrapFrame noting that changes touching capabilities/IPC/
memory/crypto must be flagged for explicit security review as
described in the ADR" — that contract lives in CLAUDE.md (§"Before
starting work" item 4) and `unsafe-policy.md` §"Review", and the
function-level doc-comment of `irq_entry` already cites the
relevant audits and ADR-0021. Adding an inline reminder would
duplicate policy that already lives in the canonical locations
and contradicts CLAUDE.md's "Default to writing no comments" rule
for non-load-bearing prose.
Gates: cargo fmt / host-clippy / kernel-clippy / kernel-build all
clean. host-test 149 / 149 (was 148; +1 new
`ns_to_ticks_rounds_up_on_subtick`). The TrapFrame compile-time size
guard is exercised by every kernel build.
Refs: T-012, ADR-0010 (rounding test lock-in), ADR-0021 (Amendment
date + signature consistency), `unsafe-policy.md` §1 (SAFETY-comment
triplet)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 6, 2026
…findings) Address inline + overall comments on PR #14. Two of three findings applied; the third (architectural suggestion) skipped with reason. ## Applied 1. **`docs/guides/run-under-qemu.md:38`** — `cd TyrneOS` after `git clone https://github.com/cemililik/Tyrne.git` was a real bug-risk: the URL sweep in commit `2fc870e` updated the clone URL but the next-line `cd` target was a bare directory name not matched by the `cemililik/TyrneOS` → `cemililik/Tyrne` substitution. `git clone Tyrne.git` creates a `Tyrne/` directory; the `cd` target now matches. *(Flagged by sourcery-ai-bot inline + coderabbitai-bot inline.)* 2. **`SECURITY.md:25`** — *"Everything in the `TyrneOS` repository is in scope."* — same class of bug as #1 (bare repo name not caught by URL-only sed). The bots did not flag this one; surfaced by a defensive grep across the tree for any remaining bare `TyrneOS` references after the fix in #1. Replaced with `Tyrne`. Post-fix verification: `grep -rln 'TyrneOS' . --include='*.md' --include='*.toml' --include='*.rs' --include='*.s' | grep -v 'docs/analysis/reviews/' | grep -v 'docs/analysis/technical-analysis'` returns zero hits — the only remaining `TyrneOS` mentions in the tree are the intentionally-preserved review-snapshot evidence noted in `2fc870e`'s commit message. ## Skipped (with reason) 3. **sourcery-ai-bot's "switch to repo-relative paths" suggestion** — this is a forward-looking architectural change (move every rustdoc / review reference from `https://github.com/cemililik/Tyrne/blob/main/...` to repo-relative paths like `../../docs/decisions/0001-...md`) that would prevent future rename sweeps. Reasonable suggestion, but: - Out of scope for PR #14 (this PR is the *current* rename sweep, not a structural rework). - Touches the same ~26 source files plus every architecture / ADR cross-reference (~hundreds of links) — a separate mass-update similar in shape to commit `10e3351`. - Some link forms (rustdoc footers from kernel/src/* up to docs/) need careful path-hop calculations and may render differently under `cargo doc` vs GitHub markdown — design choice that warrants its own ADR or review. Recommended path: open a follow-up roadmap task post-B1-closure to evaluate "rustdoc footer link form (absolute URL vs repo- relative)" as part of γ or a doc-hygiene phase. Not blocking this PR. Refs: PR #14, comprehensive-review-2026-05-06, Track J §J-NB1 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
… 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 10, 2026
3-bot review round (sourcery + gemini + coderabbit; qodo rate-
limited). 13 inline findings on the T-017 implementation arc.
Triage: 1 build failure (fmt), 4 Major (1 from 3 bots, 1 from 2,
2 standalone), 6 Minor, 2 Medium-to-defer.
Verified each against current code:
Applied:
1. **CI fmt failure** — `cargo fmt --all` reformatted 9 sites in
bsp-qemu-virt/src/main.rs + kernel/src/mm/pmm.rs where I'd
manually broken lines that the rust-toolchain.toml-pinned
nightly-2026-01-15 rustfmt collapses to single lines (e.g.,
`let x = a.method().method()` style). No semantic change;
pure whitespace.
2. **Overlapping reserved ranges in Pmm::new** (Major; flagged
independently by sourcery + gemini + coderabbit). If two
reserved ranges overlap, the per-range bit-set loop sets the
shared frame's bit once but each entry increments
`reserved_count` separately, leaving the cached counter
inconsistent with the bitmap. `stats().free_frames` could
report 0 while `alloc_frame()` still finds a free frame —
classic counter-drift. Added:
- **PmmError::OverlappingReservedRanges** variant
(`#[non_exhaustive]`, additive).
- **O(R²) pairwise overlap check** in `Pmm::new` after the
in-extent / alignment validations, before any bitmap
mutation. Two half-open ranges [a, b) and [c, d) overlap
iff `a < d && c < b`. For R ≤ 8 (v1), ≤ 28 comparisons —
trivially cheap.
- **`new_rejects_overlapping_reserved_ranges`** host test
covers three scenarios: classic-overlap (rejected),
duplicate-range (rejected), touching-but-not-overlapping
half-open ranges `[a, b) + [b, c)` (correctly NOT rejected
— boundary semantics).
3. **stats().total_frames anchored against extent** (sourcery
Medium). Previously derived as `free_count + reserved_count +
allocated_count`; if any sub-counter drifts, the total
silently re-establishes internal consistency rather than
surfacing the drift. Now derived from `self.extent.frame_count()`
directly so counter bugs become stats-vs-bitmap mismatches
the existing `stats_parity_with_bitmap_bit_count` test
catches.
4. **PMM_BITMAP_BYTES round-up** (coderabbit Minor). For QEMU
virt's 32 768 frames the old `/8` and new `.div_ceil(8)`
produce the same 4 096 bytes (no v1 bug). Forward-defensive
for future BSPs with non-multiple-of-8 frame counts where the
floor form would underallocate the last bitmap byte. Used
stable `usize::div_ceil` directly per clippy::manual_div_ceil
suggestion.
5. **alloc_frame SAFETY block — inline rejected-alternatives**
(coderabbit Major). Per CLAUDE.md unsafe-policy.md §1: every
unsafe block must state (a) why unsafe is needed, (b)
invariants upheld, (c) why safer alternatives were rejected.
The previous SAFETY comment had (a) + (b) but punted (c) to
the audit-log entry. Now inlines a four-alternative summary
(slice-from-raw-parts-mut + lazy-zero-on-page-fault +
skip-zero + volatile_set_memory) with one-line rejection
reasons each. The fifth alternative (Mmu::zero_frame trait
method) stays in UNSAFE-2026-0026's full Rejected-alternatives
discussion to keep the inline block bounded; the SAFETY
comment names the audit entry for the full record.
6. **T-017 stale UNSAFE outcome text** (coderabbit Minor).
Lines 20 / 87 / 88 / 124 / 134 still claimed
"UNSAFE-2026-0001 Amendment / no new audit entry" — pre-
adjudication wording from the ADR-0035 §Dependency chain
step 5's deferred-verdict era. Now uniformly "UNSAFE-2026-0026
(new audit-log entry per the adjudication-deferred verdict)"
with the same Rejected-alternatives summary the new entry
carries.
7. **UNSAFE-2026-0026 contradictory verification sentence**
(coderabbit Minor). Status line said both "v1's demo never
calls `alloc_frame`" AND "the site is reached on every boot".
The two refer to different "sites": Pmm::new construction
(reached every boot, prints the banner) vs. the
`core::ptr::write_bytes` zero-fill block (the actual unsafe
site, runtime-unexercised because no v1 caller hits
`alloc_frame`). Reworded to make the construction-vs-zero-
fill distinction explicit per coderabbit's suggestion.
8. **current.md downstream "next steps" sections** (coderabbit
Minor). Lines 78-79 ("Next task to open: T-016 implementation"
+ "Next review trigger: B2 closure trio") were stale —
T-016 closed two PRs ago, B2 closed too. Updated to point
at ADR-0028 + T-018 (next task) + B3 milestone closure trio
(next review trigger) with the correct forward-flag (T-018
will be UNSAFE-2026-0026's first runtime exercise).
Skipped with reason:
A. **`bitmap` array on the function stack in `Pmm::new`**
(gemini Medium). `let mut bitmap = [0u8; N]` creates a stack
frame proportional to `N`. For v1 N = 4096 → trivially fits
in the 64 KiB boot stack; for future Pi-4-class BSPs with
N = 131072 (4 GiB RAM → 128 KiB bitmap), this overflows the
v1 stack. The fix is an in-place `Pmm::init_in(slot: &mut
MaybeUninit<Self>, ...)` API that writes through the
StaticCell directly rather than via a stack-allocated value.
That's a larger API change; v1 doesn't need it and the cost
of bundling it here is non-trivial. Tracked as a known
future-BSP concern in T-017's review-history; opens with the
first BSP whose `N` exceeds the boot stack (likely the
Pi-4 BSP arc).
B. **PA → kernel pointer cast in alloc_frame** (gemini Medium).
The `(extent.start.0 + idx * PAGE_SIZE) as *mut u8` cast
relies on the identity-mapped v1 layout (ADR-0027). The
SAFETY block + UNSAFE-2026-0026's invariant (3) already
document this explicitly. The high-half migration
(ADR-0033 placeholder) will need a `phys_to_virt` helper at
this exact site — the placeholder ADR's §Simulation table
will name it as the first migration step. No code change in
T-017's scope; the identity-only assumption is the v1
design.
Test count rises 15 → 16 (new `new_rejects_overlapping_reserved_ranges`
pinning sourcery+gemini+coderabbit's #1 finding). Workspace
total 199 → 200 host tests.
Verification:
- cargo fmt --check clean (CI ran with
nightly-2026-01-15;
matches local now)
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings (used
`usize::div_ceil` per
clippy::manual_div_ceil)
- cargo test 200/200 host (was 199; +1
overlap test)
- cargo +nightly miri test 15/15 PMM tests clean
(Stacked Borrows clean
against the unchanged
zero-fill path)
- cargo kernel-build clean
- QEMU smoke stable; new banner reads
"tyrne: pmm initialized
(32603 frames available;
165 reserved)" — the +1
reserved-frame delta vs.
prior commit (32604 + 164)
is the kernel-image-section
growth from this commit's
code additions rounding up
one more frame at
__stack_top_aligned
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0026
5 tasks
cemililik
added a commit
that referenced
this pull request
May 14, 2026
…5 valid, 2 skipped) Bot review-round on PR #30 (gemini-code-assist + sourcery-ai; coderabbit rate-limited; qodo paused). 7 findings total — 5 valid + 2 invalid (gemini PR-scope confusion). **Applied (5):** 1. **G-1b — current.md L50 outdated signature.** Active-task bullet still cited `task_create_from_image(...) -> Result<TaskHandle, TaskLoaderError>` from the original ffd712c draft; the pre-Accept #1 review (fd91567) shifted to `load_image(...) -> Result<LoadedImage, LoadError>` but current.md was never updated. Fix: L50 rewritten with the current canonical signature + the "LoadedImage opaque descriptor, not TaskHandle" framing + the typed `LoadError` enum variants + the cap_drop-not-cap_revoke rollback note + UNSAFE-2026-0027 audit-entry forward-flag. 2. **G-2 — ADR-0029 §Negative consequences `.data` write-fault risk now explicit.** The original "no per-section permissions" bullet stated the constraint but didn't make the executable- without-write consequence concrete. Fix: rewrote to state plainly that v1 image region maps `USER | EXECUTE` (no `WRITE`), so a real binary's `.data` section would permission-fault on the first write — v1 userspace is effectively restricted to code + read-only data. Hand-coded placeholder (`mov w0, #42; ret`) and B6's planned minimal "hello" have no `.data`, so the constraint is non-blocking for the v1 demo trajectory. 3. **G-3 — T-019 §Simulation step 5 pseudo-code inaccuracies.** Three issues: (a) `frame_pa` used as pointer but `pmm.alloc_frame ()` returns `PhysFrame` struct — should be `frame.as_usize() as *mut u8`; (b) `copy_len` undefined — should be `core::cmp::min (PAGE_SIZE, image.len() - i*PAGE_SIZE)` for tail truncation on the partial last page; (c) tail-zeroing relies on UNSAFE-2026- 0026's PMM zero-init contract for bytes copy_len..PAGE_SIZE. Fix: pseudo-code rewritten with the correct conversions + new `task_loader::tests::tail_zeroing_on_partial_last_page` row-7 verification test added. 4. **S-1 — cap_drop vs cap_revoke explanation consolidation.** Sourcery flagged that the cap_drop/cap_revoke + leak baseline reasoning was repeated in 3 places (§Context leak-path-closure paragraph, §Acceptance criteria MapFailed bullet, §Rollback contract intro). Fix: §Rollback contract intro stays canonical (most authoritative location); §Context paragraph trimmed to a forward-reference; §Acceptance criteria MapFailed bullet shortened to a one-line summary + pointer to the canonical section. Drift risk on the cap_drop reasoning now sits in one place to maintain. **Skipped (2):** 5. **G-1a — "B3 closure files missing from this PR."** Skipped — gemini's claim is wrong. The B3 closure files (`2026-05-14-B3-closure.md` in business / security / perf review folders) landed in PR #29 (merged 2026-05-14 as commit `b425dc1`) and exist on `main`. PR #30's diff is additive to main; linking from PR #30's files to existing main files is fine and the links resolve correctly. 6. **G-4 — "Link to B3 closure adjustments broken."** Same skip reason as G-1a. Link `../../analysis/reviews/business-reviews/ 2026-05-14-B3-closure.md#adjustments` from `docs/roadmap/phases/phase-b.md` resolves to the file on `main`. **Sourcery-2 (B5/B6 forward-refs depth) also skipped:** judgement call rather than correctness issue. The forward-flags for MemoryRegionCap, destroy path, syscall ABI, ADR-0033 high-half, and ADR-0034 section permissions serve a scope-clarity purpose (they explicitly enumerate what B4 does NOT do), which is valuable for the implementer reading T-019 cold. Aggressive trimming would lose information that scopes the work explicitly. The §Out of scope section already concentrates most of these refs; minor consolidation would help marginally but isn't load-bearing. No code changes; docs-only sweep across 3 files. Refs: ADR-0029 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
This was referenced May 29, 2026
cemililik
added a commit
that referenced
this pull request
May 30, 2026
…t.md ENTRY/linker reconcile, hard direct-map asserts Addresses the PR #36 round-3 code review. Each finding was verified against current code; only still-valid issues changed, the rest skipped with reason. The migration mechanism is unchanged. - ADR-0033: append-only Revision-notes rider reconciling the SHIPPED single linear offset (KERNEL_HIGH_HALF_OFFSET = 0xFFFF_FFFF_0000_0000, canonical KBASE = 0xFFFF_FFFF_4008_0000) against the body's two-offset / 0xFFFF_FFFF_8008_0000 model. Original body left intact per ADR-0025 §Rule 2; the rider explains why the two PA<->VA offsets collapse to one and names ADR-0027 Option C's prospective base as superseded. - boot.md: ENTRY(_start_phys) reconciled (body previously contradicted itself with ENTRY(_start)); Stage-1 entry-point wording fixed; "Linker script responsibilities" rewritten to the real link-high/load-low form (KBASE + AT(), no MEMORY{} block, .text.vectors 2 KiB-aligned). - hal/mmu: phys_to_kernel_va / kernel_va_to_phys now use assert! (was debug_assert!) so an out-of-window PA/VA fail-stops in release (CLAUDE.md #1) rather than wrapping to a wild pointer; added # Panics sections so clippy::missing_panics_doc stays green (assert! is not exempt; debug_assert! was). The check cannot fire in v1 (QEMU PA < 4 GiB). - mmu_bootstrap: high_half_activate's two SAFETY comments gained explicit "Safer alternatives rejected" clauses, matching the sibling mmu_bootstrap() blocks. Audit entries UNSAFE-2026-0022 / 0023 already cover these writes via their T-022 Amendments (no audit-log change needed). - main.rs: timer-banner comment corrected — BOOT_NS is captured POST high-half migration (excludes MMU-activation + migration cost), not pre-MMU. - tools/smoke.sh: --timeout now requires a strictly positive integer; rejects "0", which disables timeout(1)/alarm() and would let the WFI-idling kernel hang the run forever. Skipped (with reason): the migration trampoline / TTBR0-free / panic-handler unsafe blocks in main.rs already carry conforming SAFETY comments + audit IDs (incl. UNSAFE-2026-0031), and the referenced audit entries already exist with T-022 Amendments — no change needed. Gates green: cargo fmt, host + kernel clippy -D warnings, 340 host tests, kernel build, QEMU smoke (gated PASS; -d int,unimp fault-clean, high-half active -> all tasks complete). Refs: T-022, ADR-0033, ADR-0025, PR #36 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 31, 2026
…xt) (#37) * docs(security): T-022 high-half migration security review — Approve Standalone, time-separated (2026-05-31) security pass over the T-022 / ADR-0033 high-half kernel migration, discharging the "awaiting explicit security review" flag current.md carried for UNSAFE-2026-0031. Verdict: Approve — eight axes pass (cryptography N/A), no live finding, no fix required. The migration is a structural kernel/user isolation *strengthening*: the kernel becomes absent from the TTBR0_EL1 regime an EL0 task runs on (removing the Meltdown substrate, replacing a standing per-descriptor AP invariant with structural absence). UNSAFE-2026-0031 + the 0022/0023/0024 T-022 Amendments are policy-conformant and second-reviewer-signed; the migration is smoke-verified fault-clean. The review-round-3 hardenings (fail-fast assert! range guards + regime-correct panic-handler UART alias) are verified sound and cannot misfire in v1 (incl. the no-double-panic property on the panic path). Forward-flagged (non-blocking, pre-B6): the three T-021 carry-forward gates (gate #1 — per-task console_write window + per-page user-VA->kernel-VA translation — is the single most important pre-B6 gate) and ADR-0034 (kernel-image section W^X; privileged-side gap, EL0-unreachable). Refs: T-022, ADR-0033 Security-Review: @cemililik (+ Claude Opus 4.8 (1M context) agent) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(adr): propose ADR-0037 — EL0 entry context Sibling ADR to ADR-0033 (which scoped itself to the migration and named the EL0 context a separate B6 task). Settles how an EL0 task's first entry is expressed and how it integrates with the cooperative scheduler. Decision (Option 1): reuse the existing Aarch64TaskContext + a one-shot enter_el0 ERET trampoline; carry (user_entry, user_sp) in the x19/x20 callee-saved slots the cooperative switch already restores; lr = enter_el0, sp = kernel stack. SP_EL1 (gate #2) is closed by construction — the task enters EL0 from its own kernel context, so a later +0x400 trap lands on that kernel stack. D2 settled: neither add EL0 fields to the kernel-object Task NOR a separate TaskContext struct — the 168-byte Aarch64TaskContext layout (and the security-critical context_switch_asm offsets) stay UNCHANGED, the exact drift the T-022 security review flagged. v1 simplification: SPSR_EL1 = 0x3C0 (EL0t, DAIF masked — cooperative, no preemption). Opens T-023 (Draft) in the same commit per ADR-0025 §Rule 1. Introduces UNSAFE-2026-0032 (the enter_el0 asm; security-sensitive → second-reviewer). Refs: ADR-0037, T-023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(adr): Accept ADR-0037 — EL0 entry context Careful re-read (write-adr §10) passed in a separate commit from Propose: forward-references grounded (T-023 exists; downstream consumers deliberately absent as consumers-not-dependencies, per ADR-0033's pattern), dependency chain complete, negative consequences carry real mitigations, the §Simulation table walks the EL0 first-entry + trap state machine. Refs: ADR-0037 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(task): T-023 — EL0 entry context (init_user_context + enter_el0 trampoline + per-task SP_EL1) Implements ADR-0037 (the B6 EL0 entry-context decision) — the dormant mechanism a userspace task needs to drop to EL0 on first dispatch. Closes T-021 carry-forward gate #2 (SP_EL1). No runnable EL0 task yet (that is the B6 wire-up, after gate #1 / gate #3); the mechanism is dormant and the QEMU trace is byte-stable. What lands: - HAL: `ContextSwitch::init_user_context(ctx, user_entry, user_sp, kernel_stack_top)` — additive sibling of `init_context`; its `# Safety` states the EL0-trap kernel-stack size contract (must hold the ~272-byte trap frame + handler call tree per +0x400, stronger than init_context's). - BSP: `QemuVirtCpu::init_user_context` seeds the *existing* Aarch64TaskContext (x19=user_entry, x20=user_sp, lr=enter_el0, sp=kernel_stack_top) — no struct/layout change, so context_switch_asm offsets are untouched (D2) — plus the `enter_el0` `#[unsafe(naked)]` ERET trampoline. - Scheduler: `Scheduler::add_user_task` (mirrors add_task, calls init_user_context; debug_asserts kernel_stack_top non-null + 16-aligned — gate #2 belt-and-braces). - test-hal: FakeContextSwitch::init_user_context + FakeTaskContext {is_user,user_sp}; host tests for the test-hal recorder and the scheduler route. All four ContextSwitch implementors updated (BSP + test-hal + scheduler's FakeCpu/ResetQueuesCpu). - Audit: new UNSAFE-2026-0032 (the enter_el0 ERET asm; second-reviewer-flagged). 2026-05-31 review-round (1 HIGH + 3 Medium + 4 nits) folded in BEFORE this commit: - HIGH — enter_el0 now SCRUBS the register file (x0–x30 + v0–v31) before ERET. The cooperative switch restores only callee-saved regs, so without the scrub the first EL0 instruction would observe kernel pointers/state in x0/x1/x8 (context ptrs / kernel stack), x30 (kernel code addr), and the caller-saved SIMD — a disclosure orthogonal to ADR-0033's AP/UXN memory isolation. Recorded as a load-bearing invariant (UNSAFE-2026-0032) and in ADR-0037 §Revision notes (CLAUDE.md #1). - SPSR_EL1 written via the register form (mov x8,#0x3c0; msr spsr_el1,x8) — the immediate MSR form is PSTATE-fields-only. - gate #2 reconciled (by-construction + the debug_assert); EL0-trap stack-size contract documented; test-scope claim narrowed (the host test pins the scheduler route via the fake — the real QemuVirtCpu slot write + the scrub are audit + kernel-build verified, the BSP being bare-metal). Docs synced: ADR-0037 §Revision notes (rider, append-only per ADR-0025 §Rule 2), T-023 (AC + review-history + row-to-verification mapping), hal.md + scheduler.md (ContextSwitch surface gains init_user_context / add_user_task), current.md (B6-opening banner + active-task/working-branch). Gates green: cargo fmt; host + kernel clippy -D warnings; 342 host tests (+2: test-hal init_user_context recorder, scheduler add_user_task route); kernel build (the 67-instruction scrub asm assembles); QEMU smoke byte-stable + fault-clean (mechanism dormant); cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt clean (0 UB). Security-relevant (the EL1→EL0 first-entry trust-boundary primitive); the ADR + task design review-round is folded in, the formal code security pass on UNSAFE-2026-0032 remains a DoD item before a real EL0 task is wired. Refs: T-023, ADR-0037, UNSAFE-2026-0032 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(task): T-023 review-round 2 — FPCR/FPSR/TPIDR scrub, offset_of! asserts, user_sp + AS-active guards Folds in the 2026-05-31 second review-round against the committed implementation (verdict: Approve-with-nits). 2 Low + 4 nits, all verified against the code and addressed. - Low-1 — enter_el0's scrub omitted the FP-control/status + EL0 thread-ID register classes. It zeroed x0–x30 + v0–v31 but NOT FPCR/FPSR/TPIDR_EL0/ TPIDRRO_EL0 — all EL0-readable, with architecturally-UNKNOWN reset values on real HW (Pi 4), so "clean, defined register file" was over-claimed off-QEMU (FPCR governs EL0 rounding/FZ/default-NaN; Linux zeroes FPCR/FPSR on exec). enter_el0 now also `msr {fpcr,fpsr,tpidr_el0,tpidrro_el0}, xzr`. No current secret leak (kernel does no FP arithmetic, never writes TPIDR), but the EL0 environment is now defined on real HW, not just QEMU's zeroed reset. - Low-2 — the implicit x19/x20/lr/sp hand-off was pinned only by size_of == 168; a layout-preserving field reorder (e.g. fp/lr swap) would silently corrupt both context_switch_asm and the enter_el0 hand-off. Added offset_of! asserts for all five offsets (0/80/88/96/104) — hardens the pre-existing context_switch_asm coupling too. - Nit — add_user_task now debug_asserts user_sp 16-byte alignment (it becomes SP_EL0; SCTLR_EL1.SA0 faults a misaligned EL0 stack), symmetric with the kernel-stack assert. - Nit — the AS-must-be-active obligation (TTBR0 installed + EPD0 cleared at first dispatch) is now stated on the caller-facing # Safety of both init_user_context and add_user_task (it previously lived only on enter_el0's doc, which the future task_create_from_image author would not read). - Nit (process honesty) — the review-round-1 rider shipped WITH bbc42e8, after ADR-0037's Accept (546febc): the careful-re-read missed the scrub HIGH; recorded in ADR-0037 §Revision notes per ADR-0025 §Rule 2. - Nit — "67-instruction scrub" wording dropped (it is 63 register-zeros + 4 special-register writes); docs reworded. Docs synced: UNSAFE-2026-0032 (scrub scope), ADR-0037 §Revision notes (second rider), T-023 (AC + review-history), hal.md, current.md. Gates green: cargo fmt; host + kernel clippy -D warnings; 342 host tests; kernel build (offset_of! asserts + the new MSRs assemble); QEMU smoke byte-stable + fault-clean (mechanism still dormant); cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt clean (0 UB). Refs: T-023, ADR-0037, UNSAFE-2026-0032 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(task): T-023 review-round 3 — fix non-existent SHA, init_count doc drift, T-023 status/ACs A multi-agent adversarial review of the whole T-023 arc (b6549d7..HEAD; 5 lenses × per-finding skeptic verification) found NO code/security/correctness/ asm defect — the register scrub (incl FPCR/FPSR/TPIDR), the offset_of! asserts, the SPSR register form, the dormancy + gate ordering, all four ContextSwitch implementors, and the host tests were verified correct. The only confirmed findings were three doc-only consistency defects, all fixed: - The T-022 security review (2026-05-31) cited ADR-0033's Accept as `db392c1`, which does not exist (transposed digit) — corrected to `db892c1`. - `FakeContextSwitch::init_count()`'s rustdoc said "init_context calls" but the shared counter is now also incremented by `init_user_context`; doc broadened to match (mirrors the already-updated `FakeTaskContext.initialized` field doc). - T-023 was still `Status: Draft` with all nine ACs unchecked despite being implemented + gates-green; moved to "In Review (… awaiting explicit security review …)" per the T-022 house convention and checked the nine satisfied ACs. Gates re-confirmed: cargo fmt, host clippy -D warnings, 342 host tests (kernel binary unchanged → the prior byte-stable QEMU smoke + Miri-0-UB still hold). Refs: T-023, ADR-0037 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(test-hal): T-023 review-round 4 — init_context clears prior user markers on slot reuse FakeContextSwitch::init_context (the kernel path) seeded initialized/ entry_addr/stack_top but left is_user / user_sp untouched, so a context slot re-seeded user-task -> kernel-task would report stale is_user=true / a stale user_sp. init_context now fully re-seeds a kernel context: clears is_user and user_sp. Adds a regression test (init_context_clears_prior_user_markers_on_reuse). Test-double-only correctness; no runtime/kernel effect. Skipped (with reason) from the same review: - "msr {fpcr,fpsr,tpidr_el0,tpidrro_el0}, xzr leaks SP" — INVALID. For MSR/MRS, Rt=31 is XZR (the X[]/ZR accessor), not SP (the 31=SP rule is for load/store base + SP-form arithmetic). Verified: kernel-build assembles these cleanly, and the shipped `msr ttbr0_el1, xzr` disassembles as `msr TTBR0_EL1, xzr` (d518201f) and is smoke-clean (TTBR0 -> 0, would fault if SP). No change. - "factor the enter_el0 scrub into a .irp/macro" — SKIPPED. The explicit per-register form is deliberately maximally auditable for a security-critical scrub (every scrubbed register visible); the register set is fixed; no defect. - "give init_user_context a default impl (unimplemented!/delegate)" — SKIPPED. A required trait method already fails loudest (compile error) if an implementor omits it; unimplemented! would defer that to a runtime panic (against the kernel's panic-free discipline + kernel-clippy denies panic), and delegating to init_context would silently produce a broken EL0 entry. Gates: cargo fmt; host clippy -D warnings; 343 host tests (+1); Miri 0 UB (kernel binary unchanged -> prior byte-stable QEMU smoke holds). Refs: T-023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced May 31, 2026
cemililik
added a commit
that referenced
this pull request
May 31, 2026
T-025 — Mmu::translate + per-task user-access translation (B6 gate #1)
This was referenced May 31, 2026
cemililik
added a commit
that referenced
this pull request
Jun 1, 2026
… + exits (#42) * feat(el0): T-028 — run the first real EL0 userspace task (B6 wire-up) Tyrne's first real EL0 userspace task runs end-to-end: load_image -> task_create_from_image (resolve the Task cap -> TaskHandle + the AS cap -> AddressSpaceHandle) -> seed USER_TASK_TABLE with a DebugConsole/CONSOLE_WRITE cap at handle 0 (HELLO_CONSOLE_CAP, asserted) -> add_user_task (with a USER_TASK_STACK SP_EL1). The +0x200 EL1-stub fail-closed smoke is retired (its property is host-tested) in favour of the live +0x400 round-trip. The cooperative scheduler had no termination path, so task_exit's Terminate was a dormant no-op (the EL0 task ran past task_exit -> BRK -> panic). Added Scheduler::task_exit_current (-> !: start_prelude + the extracted, shared run_dispatched tail — drops the exiting task, dispatches the next, abandons the syscall frame) and wired syscall_entry's Terminate -> task_exit_current + Reschedule -> yield_now (+ a 'userspace task exited' report). start()'s dispatch tail is now run_dispatched, shared (no duplicated context-switch). QEMU smoke PASS: ERET->EL0 @0x800000 -> console_write SVC (+0x400) -> gate-#1 buffer translate -> 'hello from userspace' -> ERET->EL0 -> task_exit SVC -> 'userspace task exited' -> 'all tasks complete'; only the 2 expected EL0 SVC exceptions, zero new fault class. Gates #1/#2/#3 + EL0 entry + high-half kernel reachability + EPD0-on-activation all proven at runtime. EL0-boundary security review = Approve (the T-024/T-026 carry-forward DoD gate; docs/analysis/reviews/security-reviews/2026-06-01-T-028-el0-userspace-wireup.md): 5 adversarial lenses / 9 agents across the eight axes, 0 confirmed exploitable defects (4 lenses clean, 2 findings verified non-exploitable); SEC-T028-01 object-lifecycle-on-exit forward-flagged. Audit: UNSAFE-2026-0008 Amendment (context_switch-abandon tail extracted + task_exit_current caller) + UNSAFE-2026-0029 Amendment (the +0x400 path + live control-plane arms). Gates: fmt; host + kernel clippy -D warnings; host tests 259 kernel (+start_prelude_dispatches_next_ready_overwriting_current) / 46 hal / 58 test-hal / 3 doc; kernel build; tools/smoke.sh --int PASS; Miri --workspace --exclude tyrne-bsp-qemu-virt 0 UB. All B6 functional steps (1-6) done; only step 7 (the closure trio) remains. Refs: T-028, ADR-0039 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(el0): T-028 PR #42 review-round — assert yield_now Result + doc typo - syscall_entry Reschedule arm: debug_assert!(yield_now's Result is Ok) instead of discarding it — its only Err (NoCurrentTask) is gate-#3-precluded, so a broken invariant surfaces in debug rather than being masked by the OK_STATUS returned to EL0. - security-review doc typo: 'return at EL1' -> 'return to EL1'. - Skipped (with reason): clearing the exiting task to a dedicated terminal TaskState in task_exit_current — this is the SEC-T028-01 forward-flag the security review verified non-exploitable in v1 (orphan handle unreachable; only Blocked is scanned by state; arena slot not reused); a terminal state + its reclamation semantics is the successor lifecycle ADR's scope. Re-validated: fmt; kernel build + clippy; tools/smoke.sh --int PASS (hello still greets + exits cleanly, no fault). BSP-only + doc change (host-test/Miri unaffected). Refs: T-028 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- 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.
Summary by CodeRabbit
New Features
Documentation