Move documentation integrity into Rust xtask - #61
Conversation
Code Lawyer activity summary
All seven findings were resolved through isolated RED → GREEN cycles. The complete local debug/release, formatting, Clippy, MSRV, fuzz, dependency-policy, audit, pinned documentation-tool, external refusal, and whitespace matrix is green. No public Keep API or durable-format surface changed. @codex review please. Confirm the issue-#47 parity and bounded-execution claims, with particular attention to fail-closed parsing and process cleanup. |
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
WalkthroughThe change moves documentation and workflow integrity checks from Python into Rust ChangesRepository integrity migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146c419826
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtask/src/git_inventory/process.rs (1)
54-81: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCleanup/join failures mask the primary detected error instead of preserving it.
Three sites in
start_git(54-59, 61-66, 71-80) runcleanup_child(&mut child, operation)?;beforereturn Err(<the actual Pipe/Run error>)— ifcleanup_childitself fails, the?discards the intended error entirely. Same pattern incollect_git_result:diagnostic_worker.join()...?andstop?both run ahead ofpaths?/status?, so a cleanup/join failure during an already-failedpathsread (the "simultaneous failures" case) silently replaces the real root cause with a cleanup-failure message.
bounded_process.rs::cleanupin this same PR already solves this correctly by boxing the primary error (ProcessError::Cleanup { primary: Box::new(primary), .. }).GitInventoryErrorshould gain an equivalent variant so this module doesn't regress diagnostic precision relative to its sibling.🔧 Proposed fix sketch (mirrors bounded_process.rs::cleanup)
+ Cleanup { + primary: Box<Self>, + action: &'static str, + source: io::Error, + },-fn cleanup_child(child: &mut Child, operation: &'static str) -> Result<(), GitInventoryError> { - let stop = request_stop(child, operation); - let wait = child.wait().map_err(|source| GitInventoryError::Run { - operation, - action: "wait for", - source, - }); - stop?; - wait.map(|_| ()) -} +fn cleanup_child( + child: &mut Child, + operation: &'static str, + primary: GitInventoryError, +) -> GitInventoryError { + if let Err(source) = request_stop(child, operation) { + return GitInventoryError::Cleanup { primary: Box::new(primary), action: "stop", source }; + } + if let Err(source) = child.wait() { + return GitInventoryError::Cleanup { primary: Box::new(primary), action: "wait for", source }; + } + primary +}Also applies to: 89-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtask/src/git_inventory/process.rs` around lines 54 - 81, Preserve the primary git error when cleanup or diagnostic-reader joining also fails. Add a cleanup-wrapping variant to GitInventoryError, mirroring ProcessError::Cleanup, and update start_git’s stdout/stderr pipe and diagnostic-worker failure paths to return the original Pipe/Run error while recording cleanup failure. In collect_git_result, ensure paths/status read errors remain primary when diagnostic_worker.join or stop fails, wrapping the cleanup failure instead of replacing the detected error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtask/src/bounded_process.rs`:
- Around line 44-91: Refactor capture by extracting child spawning and
stdout/stderr pipe acquisition into a small spawn_piped helper, preserving the
existing ProcessError mappings and cleanup behavior. Update capture to call this
helper so its remaining reader setup, waiting, collection, and result
construction stay within the review guideline; do not change wait_for_child
behavior.
- Around line 20-91: add rustdoc for every listed public boundary symbol: in
xtask/src/bounded_process.rs lines 20-91 document ProcessOutput, status, and
capture, including blocking behavior, deadline semantics, and error taxonomy; in
xtask/src/bounded_process/error.rs lines 8-36 document ProcessError and each
variant’s trigger; in xtask/src/git_inventory/error.rs lines 11-49 document
GitOutputUnit, GitInventoryError, and each variant’s trigger; in
xtask/src/git_inventory/path_stream.rs lines 21-26 document read_paths framing
and limits, and lines 66-77 document GitPath, GitPath::new, and
GitPath::as_bytes; in xtask/src/git_inventory/process.rs lines 28-32 document
paths, including blocking, bounded-output, and error semantics.
In `@xtask/src/bounded_process/reader.rs`:
- Around line 51-59: Redesign the timeout path in the reader flow around
deadline.remaining, recv_timeout, and handle.join so a timeout cannot return
while the reader thread remains blocked. Ensure cleanup closes or otherwise
invalidates the pipe and causes descendants retaining inherited handles to
release it, then always join and reap the reader thread before propagating the
timeout error; preserve normal receive and panic handling.
In `@xtask/src/bounded_process/tests.rs`:
- Around line 47-49: Remove the direct stdout fixture write in the affected
test, including the StdoutLock, write_all, and flush calls; alternatively, add
the required documented test exception if the output is essential. Keep the
test’s existing behavior otherwise unchanged.
In `@xtask/src/documentation_integrity/dependabot.rs`:
- Around line 83-111: Fix directories parsing in block_scopes so it stops
before, rather than consumes, the first line that does not match the
directory-item prefix; avoid map_while on the shared iterator and preserve that
rejected line for the outer loop to inspect. Add a regression test covering a
comment or blank line followed by another directory declaration, ensuring all
scopes are extracted.
In `@xtask/src/documentation_integrity/error/display.rs`:
- Around line 22-111: Reduce DocumentationError::fmt below the logical-line cap
by extracting the repository-file variants into a dedicated dispatch helper and
the Tool* variants into another helper, mirroring source_path and
repository_root. Move only the corresponding match-arm formatting and dispatch
from fmt, preserving all existing messages and behavior.
In `@xtask/src/documentation_integrity/node_toolchain.rs`:
- Around line 145-156: Update require_provenance to validate the complete
accepted package graph rather than only checking resolved and integrity
presence. Compare packages deterministically against the trusted expected graph,
requiring exact package paths, versions, resolved URLs, and integrity values,
and reject missing or extra entries including altered lock artifacts.
In `@xtask/src/documentation_integrity/workflow_contract.rs`:
- Around line 17-31: Replace raw substring checks in admit with parsed YAML
documentation-job steps and validate only actual run fields, rejecting comments,
quoted decoys, and every Python execution form; update
xtask/src/documentation_integrity/workflow_contract.rs lines 17-31. In
xtask/src/documentation_integrity/node_toolchain.rs lines 95-114, normalize
shell commands to ignore heredoc/comment text and whitespace or continuation
variants, require the real npm ci invocation, and reject actual npm install
invocations. In xtask/src/documentation_integrity/workflow_contract/tests.rs
lines 18-35, add a fixture containing commented required commands without
executable equivalents and assert admission is refused.
In `@xtask/src/fuzz_campaign/execution.rs`:
- Around line 37-45: Update the OutputMode::Inherit branch in the
plan.output_mode() match to use a deadline-aware inherited-status helper instead
of bounded_process::status, passing plan.deadline(). Ensure the helper
terminates and reaps the child when the deadline expires while preserving
inherited output and existing status/error behavior.
In `@xtask/src/git_inventory/path_stream.rs`:
- Around line 131-170: Separate the empty-path error in admit_path from the
unterminated-output error in finish: add or use a dedicated GitInventoryError
variant such as EmptyPath for current.is_empty(), while retaining OutputFraming
for trailing bytes. Update the corresponding error display in
git_inventory/error.rs so each variant accurately describes its malformed-input
shape.
In `@xtask/src/source_structure.rs`:
- Line 28: After the source scan completes and `source_violations` is produced,
call `verify_source_root(&source_root, repository_root)?` before returning or
continuing with the result. Anchor this revalidation in the command flow using
`RepositoryRoot::open` and `source_violations`, matching the existing pattern in
`documentation_integrity::check`.
---
Outside diff comments:
In `@xtask/src/git_inventory/process.rs`:
- Around line 54-81: Preserve the primary git error when cleanup or
diagnostic-reader joining also fails. Add a cleanup-wrapping variant to
GitInventoryError, mirroring ProcessError::Cleanup, and update start_git’s
stdout/stderr pipe and diagnostic-worker failure paths to return the original
Pipe/Run error while recording cleanup failure. In collect_git_result, ensure
paths/status read errors remain primary when diagnostic_worker.join or stop
fails, wrapping the cleanup failure instead of replacing the detected error.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e2477d2-5bf2-462a-923f-d5a095be5648
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.github/workflows/ci.yml.gitignoreCHANGELOG.mdCONTRIBUTING.mddocs/Documentation Standards.mddocs/dependencies/documentation-toolchain.mddocs/dependencies/serde-and-serde-json-1.0.229-1.0.151.mdscripts/check_markdown.pyscripts/check_workflows.pyscripts/test_check_markdown.pyscripts/test_dependabot_coverage.pyscripts/test_documentation_integrity.pyscripts/test_documentation_refusals.pyscripts/test_workflow_corpus.pyxtask/Cargo.tomlxtask/src/bounded_process.rsxtask/src/bounded_process/deadline.rsxtask/src/bounded_process/error.rsxtask/src/bounded_process/reader.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/tool.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/fuzz_campaign.rsxtask/src/fuzz_campaign/execution.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/src/fuzz_campaign/process/error.rsxtask/src/fuzz_campaign/target.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/git_inventory.rsxtask/src/git_inventory/error.rsxtask/src/git_inventory/path_stream.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/git_inventory/process.rsxtask/src/git_inventory/process/tests.rsxtask/src/main.rsxtask/src/repository_file.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_error.rsxtask/src/source_structure/source_kind.rsxtask/src/source_structure/tests.rsxtask/src/task_error.rsxtask/tests/cli_contract.rsxtask/tests/cli_contract/documentation_tools.rsxtask/tests/documentation_cli_contract.rsxtask/tests/source_policy_contract.rs
💤 Files with no reviewable changes (9)
- scripts/test_check_markdown.py
- xtask/src/fuzz_campaign.rs
- xtask/src/fuzz_campaign/process/error.rs
- scripts/test_documentation_refusals.py
- scripts/test_workflow_corpus.py
- scripts/test_documentation_integrity.py
- scripts/check_markdown.py
- scripts/check_workflows.py
- scripts/test_dependabot_coverage.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Runtime fuzz smoke
- GitHub Check: Rust quality gates
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use stable, pinned Rust with edition 2024;
cargo fmt --checkand Clippy with-D warningsmust pass.
Files:
xtask/src/source_structure/pure_rust_tests.rsxtask/tests/documentation_cli_contract.rsxtask/src/git_inventory.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity.rsxtask/src/bounded_process/deadline.rsxtask/src/source_structure/source_kind.rsxtask/src/documentation_integrity/tool.rsxtask/Cargo.tomlxtask/src/fuzz_campaign/target.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/corpus.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/tests/cli_contract/documentation_tools.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/git_inventory/error.rsxtask/tests/source_policy_contract.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/bounded_process/error.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/fuzz_campaign/execution.rsxtask/src/bounded_process/reader.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/task_error.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/main.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/tests/cli_contract.rsxtask/src/repository_file.rsxtask/src/git_inventory/path_stream.rsxtask/src/source_structure/source_error.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process.rsxtask/src/source_structure.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/source_structure/tests.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not useunwrap,expect,panic!,todo!,unimplemented!,dbg!, stdout/stderr printing, unchecked indexing, lossy casts, or unsafe code. Unsafe requires a dedicated crate with documented invariants, tests, and measured necessity.
Use checked arithmetic for externally influenced values andTryFrom/try_frominstead of potentially lossyasconversions.
Public functions must not take boolean parameters; use enums instead. Prefer typed newtypes for IDs, lengths, offsets, generations, and namespaces.
Parse and validate untrusted bytes before admitting them as trusted types; make illegal states unrepresentable where practical.
Prefer concrete types, synchronous core APIs, and traits only at real substitution boundaries; do not introduce async without demonstrated consumer need.
Do not let HashMap iteration order affect identity, serialization, tests, or behavior.
Use hexagonal architecture: domain and port modules own invariants and semantic capabilities, dependencies point inward, and core/ports must not import adapters, filesystems, networks, CLIs, runtimes, or application policy.
Keep codecs at ingress and egress boundaries. Ports exchange semantic or validated types, not serializer-owned JSON, CBOR, or wire values.
Keep files near 200 lines, review files over 300, and do not exceed 500; keep functions near 20 logical lines, review over 40, do not exceed 60; limit nesting to 3 and parameters to 5.
Do not create genericutils.rs,helpers.rs,common.rs,misc.rs,shared.rs,manager.rs,service.rs,types.rs, ormodels.rs; name files after the concept they own.
Keep items private by default; usepub(crate)unless external consumers require public visibility. Public validated types must have private fields and checked constructors, and consequential staged work, plans, commits, and results should use#[must_use].
UseOptionfor normal absence, typed errors by boundary, preserved error sources, and expected/observed state in...
Files:
xtask/src/source_structure/pure_rust_tests.rsxtask/tests/documentation_cli_contract.rsxtask/src/git_inventory.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity.rsxtask/src/bounded_process/deadline.rsxtask/src/source_structure/source_kind.rsxtask/src/documentation_integrity/tool.rsxtask/src/fuzz_campaign/target.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/corpus.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/tests/cli_contract/documentation_tools.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/git_inventory/error.rsxtask/tests/source_policy_contract.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/bounded_process/error.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/fuzz_campaign/execution.rsxtask/src/bounded_process/reader.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/task_error.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/main.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/tests/cli_contract.rsxtask/src/repository_file.rsxtask/src/git_inventory/path_stream.rsxtask/src/source_structure/source_error.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process.rsxtask/src/source_structure.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/source_structure/tests.rs
**/*.{rs,json,cbor}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,json,cbor}: Identity, persistence, comparison, tests, and protocol behavior must be deterministic and independent of iteration order, host state, clocks, locale, and serializer defaults.
Boundary JSON and CBOR must use a named canonical profile with golden fixtures; reject duplicate fields and noncanonical identity-bearing encodings. Hash only typed, domain-separated canonical bytes.
Files:
xtask/src/source_structure/pure_rust_tests.rsxtask/tests/documentation_cli_contract.rsxtask/src/git_inventory.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity.rsxtask/src/bounded_process/deadline.rsxtask/src/source_structure/source_kind.rsxtask/src/documentation_integrity/tool.rsxtask/src/fuzz_campaign/target.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/corpus.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/tests/cli_contract/documentation_tools.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/git_inventory/error.rsxtask/tests/source_policy_contract.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/bounded_process/error.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/fuzz_campaign/execution.rsxtask/src/bounded_process/reader.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/task_error.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/main.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/tests/cli_contract.rsxtask/src/repository_file.rsxtask/src/git_inventory/path_stream.rsxtask/src/source_structure/source_error.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process.rsxtask/src/source_structure.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/source_structure/tests.rs
**/*.{rs,bin}
📄 CodeRabbit inference engine (AGENTS.md)
Durable formats must define magic bytes, versioning, canonical encoding, explicit endianness, bounds, checksums, and golden fixtures; reject trailing bytes, duplicate fields, noncanonical encodings, overflow, invalid ordering, excessive depth, and unknown mandatory flags. Do not use arbitrary Serde or Rust struct output as a durable format.
Files:
xtask/src/source_structure/pure_rust_tests.rsxtask/tests/documentation_cli_contract.rsxtask/src/git_inventory.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity.rsxtask/src/bounded_process/deadline.rsxtask/src/source_structure/source_kind.rsxtask/src/documentation_integrity/tool.rsxtask/src/fuzz_campaign/target.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/corpus.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/tests/cli_contract/documentation_tools.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/git_inventory/error.rsxtask/tests/source_policy_contract.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/bounded_process/error.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/fuzz_campaign/execution.rsxtask/src/bounded_process/reader.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/task_error.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/main.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/tests/cli_contract.rsxtask/src/repository_file.rsxtask/src/git_inventory/path_stream.rsxtask/src/source_structure/source_error.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process.rsxtask/src/source_structure.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/source_structure/tests.rs
🧠 Learnings (1)
📚 Learning: 2026-07-27T22:37:16.896Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 49
File: src/layout/record_length.rs:29-29
Timestamp: 2026-07-27T22:37:16.896Z
Learning: This repository targets Rust 1.96 (per `Cargo.toml` `rust-version` and `rust-toolchain.toml`). When writing or reviewing Rust code, only use APIs/language features stabilized in Rust 1.96 or earlier. Avoid using newer std/library APIs that wouldn’t be available on Rust 1.96 (e.g., you may rely on `u64::is_multiple_of` since it’s stabilized by 1.96).
Applied to files:
xtask/src/source_structure/pure_rust_tests.rsxtask/tests/documentation_cli_contract.rsxtask/src/git_inventory.rsxtask/src/fuzz_campaign/execution/error.rsxtask/src/documentation_integrity/tool/tests.rsxtask/src/documentation_integrity/execution/external_tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity.rsxtask/src/bounded_process/deadline.rsxtask/src/source_structure/source_kind.rsxtask/src/documentation_integrity/tool.rsxtask/src/fuzz_campaign/target.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/corpus.rsxtask/src/bounded_process/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/tests/cli_contract/documentation_tools.rsxtask/src/fuzz_campaign/target/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/corpus/tests.rsxtask/src/git_inventory/error.rsxtask/tests/source_policy_contract.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/bounded_process/error.rsxtask/src/documentation_integrity/dependabot/tests.rsxtask/src/documentation_integrity/dependabot/manifest.rsxtask/src/fuzz_campaign/execution.rsxtask/src/bounded_process/reader.rsxtask/src/documentation_integrity/node_toolchain/tests.rsxtask/src/documentation_integrity/error.rsxtask/src/task_error.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/main.rsxtask/src/documentation_integrity/node_toolchain/unique_json.rsxtask/src/fuzz_campaign/execution/tests.rsxtask/tests/cli_contract.rsxtask/src/repository_file.rsxtask/src/git_inventory/path_stream.rsxtask/src/source_structure/source_error.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process.rsxtask/src/source_structure.rsxtask/src/git_inventory/path_stream/tests.rsxtask/src/source_structure/tests.rs
🪛 LanguageTool
docs/Documentation Standards.md
[uncategorized] ~553-~553: The official name of this software platform is spelled with a capital “H”.
Context: ...d. The dedicated documentation job in .github/workflows/ci.yml installs the pinned t...
(GITHUB)
🔇 Additional comments (49)
xtask/src/git_inventory.rs (1)
1-9: LGTM!xtask/src/git_inventory/path_stream/tests.rs (1)
5-83: LGTM!xtask/src/git_inventory/process/tests.rs (1)
5-28: LGTM!xtask/src/documentation_integrity/corpus/tests.rs (1)
1-178: LGTM!docs/dependencies/serde-and-serde-json-1.0.229-1.0.151.md (1)
1-74: LGTM!xtask/src/bounded_process.rs (1)
64-91: 🩺 Stability & AvailabilityNo change needed: early-return paths do not leave ReaderWorker threads detached.
ReaderWorkerowns a non-detachedJoinHandle<()>, and dropping the partially constructedReaderWorkeronstdout_reader.drop()clears that handle without callinghandle.join(). Threads must be joined in normal reader completion, but Rust does not auto-joinJoinHandlehandles; these early-return paths return before the worker is expected to stop, and the spawned reader threads are already joined at successfulcollect()or can be restructured if this code is not expected to live until successful collection.xtask/src/bounded_process/deadline.rs (1)
7-43: LGTM!xtask/src/bounded_process/tests.rs (1)
15-45: LGTM!Also applies to: 52-110
xtask/src/fuzz_campaign/execution.rs (1)
15-17: LGTM!Also applies to: 78-82
xtask/src/fuzz_campaign/execution/error.rs (1)
6-60: LGTM!xtask/src/fuzz_campaign/execution/tests.rs (1)
7-7: LGTM!Also applies to: 84-90, 106-110, 129-136
xtask/src/fuzz_campaign/target.rs (1)
16-18: LGTM!Also applies to: 53-53
xtask/src/fuzz_campaign/target/error.rs (1)
8-8: LGTM!Also applies to: 113-115
docs/dependencies/documentation-toolchain.md (1)
34-48: LGTM!CHANGELOG.md (1)
13-19: LGTM!.gitignore (1)
9-10: LGTM!xtask/src/repository_file.rs (1)
1-6: LGTM!Also applies to: 17-37, 49-55, 65-90
xtask/src/source_structure.rs (1)
3-15: LGTM!Also applies to: 46-46, 76-77, 86-105, 119-136, 207-209
xtask/src/documentation_integrity.rs (1)
1-51: LGTM!xtask/src/documentation_integrity/repository_text/tests.rs (1)
1-52: LGTM!xtask/src/documentation_integrity/error.rs (1)
1-140: LGTM!xtask/Cargo.toml (1)
12-19: LGTM!Also applies to: 29-32
xtask/src/main.rs (1)
11-36: LGTM!Also applies to: 52-56, 112-125
xtask/src/task_error.rs (1)
8-8: LGTM!Also applies to: 18-18, 42-42, 68-69, 81-81, 108-112
.github/workflows/ci.yml (1)
85-87: LGTM!Also applies to: 101-107
CONTRIBUTING.md (1)
41-52: LGTM!docs/Documentation Standards.md (1)
55-55: LGTM!Also applies to: 537-589
xtask/src/source_structure/pure_rust_tests.rs (1)
1-14: LGTM!xtask/src/source_structure/source_error.rs (1)
12-30: LGTM!Also applies to: 41-44, 60-73, 75-93
xtask/src/source_structure/source_kind.rs (1)
1-24: LGTM!xtask/src/source_structure/tests.rs (1)
65-107: LGTM!Also applies to: 111-136, 139-179, 183-204, 208-267, 297-311, 314-328
xtask/src/documentation_integrity/contributor_contract.rs (1)
1-87: LGTM!xtask/src/documentation_integrity/dependabot.rs (1)
1-82: LGTM!Also applies to: 113-177
xtask/src/documentation_integrity/dependabot/manifest.rs (1)
1-64: LGTM!xtask/src/documentation_integrity/dependabot/tests.rs (1)
1-93: LGTM!xtask/src/documentation_integrity/repository_text.rs (1)
1-51: LGTM!xtask/src/documentation_integrity/node_toolchain/tests.rs (1)
1-175: LGTM!xtask/tests/source_policy_contract.rs (1)
4-4: LGTM!Also applies to: 19-21
xtask/src/documentation_integrity/corpus.rs (2)
1-63: LGTM!Also applies to: 83-174
64-82: 🎯 Functional CorrectnessNo change needed:
paths()already returnsBTreeSet<GitPath>.The corpus selection path preserves deterministic ordering through
BTreeSet.difference()before admission, so this concern does not apply.xtask/src/documentation_integrity/tool.rs (1)
1-77: LGTM!xtask/src/documentation_integrity/node_toolchain/unique_json.rs (1)
1-131: LGTM!xtask/src/documentation_integrity/execution.rs (1)
1-172: LGTM!xtask/src/documentation_integrity/execution/external_tests.rs (1)
1-80: LGTM!xtask/src/documentation_integrity/execution/tests.rs (1)
1-171: LGTM!xtask/src/documentation_integrity/tool/tests.rs (1)
1-64: LGTM!xtask/tests/cli_contract.rs (1)
81-95: LGTM!xtask/tests/cli_contract/documentation_tools.rs (1)
1-127: LGTM!xtask/tests/documentation_cli_contract.rs (1)
1-21: LGTM!
|
@codex review please\n\n## Activity Summary\n\n| Priority | Finding | Commit | Outcome |\n| --- | --- | --- | --- |\n| P1 | Installer admission trusted a reachable substring | b8c7157 | Complete-file BLAKE3 admission now binds the exact reviewed installer bytes. |\n| P2 | Deadline cleanup leaked descendants and reader threads | ee3a18a | Dedicated process groups are terminated through safe Rustix APIs and every reader is joined. |\n| P2 | Pure-Rust admission missed pyw and executable extensionless Python | ba4fd74 | pyw variants and bounded executable Python or PyPy shebangs now receive typed refusal. |\n\nAll three original review threads are resolved with inline RED/GREEN evidence.\n\nLocal verification is green: all/default-minimal workspace checks; all-target Clippy with warnings denied; debug and release suites; five doctests; Rust 1.96; fuzz formatting, check, and Clippy; root and fuzz dependency policy; root and fuzz RustSec audits; both pinned external documentation regressions; pinned cargo xtask verify; whole-tree whitespace validation. The current debug xtask suite reports 174 passed and 2 intentionally ignored; both ignored tests pass when invoked with the pinned tools. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba4fd74b7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Self-review addendum@codex please confirm this newly exposed portability defect.
|
Self-review correctionThe earlier socket-path diagnosis was disproved before commit: shortening the fixture path did not change the isolated |
Activity Summary — current head@codex review please.
All originating review threads are resolved. Current-head local validation is green: repository structure and conformance gates; formatting; all-feature and minimal checks; Clippy with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e8a962763
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtask/src/documentation_integrity/corpus.rs`:
- Around line 80-89: Update the corpus verification flow around `source.verify`,
`Self::read`, and `membership_change` to revalidate retained sources against the
re-inventoried state before successful completion. Ensure same-path replacements
cannot pass based only on path membership, while preserving the existing
`CorpusChanged` error for membership changes.
In `@xtask/src/fuzz_campaign/workflow_tests.rs`:
- Around line 78-84: Update the workflow validation around the build and run
checks to inspect only executable run-step scalar values, not arbitrary workflow
text. Parse the workflow steps, identify actual commands matching “cargo xtask
fuzz build” and “cargo xtask fuzz run,” then preserve the assertion that the
build command precedes the run command.
In `@xtask/src/source_structure.rs`:
- Around line 10-17: Extract the duplicated admission/refusal logic from
inventory_violations and source_violations_with_modes into a shared
admit_regular helper that accepts source_root, the relative path, and
tracked_mode, returning the admitted regular source or the existing error.
Replace both loops’ repeated let-else unwrap blocks with
admit_regular(source_root, relative.as_path(), tracked_mode)? while preserving
their current error behavior.
In `@xtask/src/source_structure/pure_rust_tests.rs`:
- Around line 193-220: Move the shared run_git helper and its sanitized
environment configuration into a common test-support module alongside
TestDirectory, then update pure_rust_tests.rs,
documentation_integrity/execution/tests.rs, and
documentation_integrity/corpus/replacement_tests.rs to import and reuse it.
Remove the duplicated local definitions while preserving the existing bounded
execution and git environment contract.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea3ab32b-d542-4575-be79-e604601063be
📒 Files selected for processing (33)
.github/workflows/ci.ymlCHANGELOG.mdxtask/src/bounded_process/process_group/tests.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/documentation_integrity/dependabot.rsxtask/src/documentation_integrity/error.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/git_inventory/process.rsxtask/src/git_inventory/process/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/python_source.rsxtask/src/source_structure/python_source/environment.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/source_error.rsxtask/src/source_structure/source_file.rsxtask/src/source_structure/source_inventory.rsxtask/src/source_structure/tests/replacement.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Rust quality gates
- GitHub Check: Runtime fuzz smoke
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use stable, pinned Rust with edition 2024;
cargo fmt --checkand Clippy with-D warningsmust pass.
Files:
xtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/error.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/tests/replacement.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/source_structure/python_source.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process/process_group/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_inventory.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/source_structure/source_error.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/source_structure/source_file.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/source_structure/python_source/environment.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/dependabot.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not useunwrap,expect,panic!,todo!,unimplemented!,dbg!, stdout/stderr printing, unchecked indexing, lossy casts, or unsafe code. Unsafe requires a dedicated crate with documented invariants, tests, and measured necessity.
Use checked arithmetic for externally influenced values andTryFrom/try_frominstead of potentially lossyasconversions.
Public functions must not take boolean parameters; use enums instead. Prefer typed newtypes for IDs, lengths, offsets, generations, and namespaces.
Parse and validate untrusted bytes before admitting them as trusted types; make illegal states unrepresentable where practical.
Prefer concrete types, synchronous core APIs, and traits only at real substitution boundaries; do not introduce async without demonstrated consumer need.
Do not let HashMap iteration order affect identity, serialization, tests, or behavior.
Use hexagonal architecture: domain and port modules own invariants and semantic capabilities, dependencies point inward, and core/ports must not import adapters, filesystems, networks, CLIs, runtimes, or application policy.
Keep codecs at ingress and egress boundaries. Ports exchange semantic or validated types, not serializer-owned JSON, CBOR, or wire values.
Keep files near 200 lines, review files over 300, and do not exceed 500; keep functions near 20 logical lines, review over 40, do not exceed 60; limit nesting to 3 and parameters to 5.
Do not create genericutils.rs,helpers.rs,common.rs,misc.rs,shared.rs,manager.rs,service.rs,types.rs, ormodels.rs; name files after the concept they own.
Keep items private by default; usepub(crate)unless external consumers require public visibility. Public validated types must have private fields and checked constructors, and consequential staged work, plans, commits, and results should use#[must_use].
UseOptionfor normal absence, typed errors by boundary, preserved error sources, and expected/observed state in...
Files:
xtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/error.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/tests/replacement.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/source_structure/python_source.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process/process_group/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_inventory.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/source_structure/source_error.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/source_structure/source_file.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/source_structure/python_source/environment.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/dependabot.rs
**/*.{rs,json,cbor}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,json,cbor}: Identity, persistence, comparison, tests, and protocol behavior must be deterministic and independent of iteration order, host state, clocks, locale, and serializer defaults.
Boundary JSON and CBOR must use a named canonical profile with golden fixtures; reject duplicate fields and noncanonical identity-bearing encodings. Hash only typed, domain-separated canonical bytes.
Files:
xtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/error.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/tests/replacement.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/source_structure/python_source.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process/process_group/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_inventory.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/source_structure/source_error.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/source_structure/source_file.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/source_structure/python_source/environment.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/dependabot.rs
**/*.{rs,bin}
📄 CodeRabbit inference engine (AGENTS.md)
Durable formats must define magic bytes, versioning, canonical encoding, explicit endianness, bounds, checksums, and golden fixtures; reject trailing bytes, duplicate fields, noncanonical encodings, overflow, invalid ordering, excessive depth, and unknown mandatory flags. Do not use arbitrary Serde or Rust struct output as a durable format.
Files:
xtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/error.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/tests/replacement.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/source_structure/python_source.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process/process_group/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_inventory.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/source_structure/source_error.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/source_structure/source_file.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/source_structure/python_source/environment.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/dependabot.rs
🧠 Learnings (1)
📚 Learning: 2026-07-27T22:37:16.896Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 49
File: src/layout/record_length.rs:29-29
Timestamp: 2026-07-27T22:37:16.896Z
Learning: This repository targets Rust 1.96 (per `Cargo.toml` `rust-version` and `rust-toolchain.toml`). When writing or reviewing Rust code, only use APIs/language features stabilized in Rust 1.96 or earlier. Avoid using newer std/library APIs that wouldn’t be available on Rust 1.96 (e.g., you may rely on `u64::is_multiple_of` since it’s stabilized by 1.96).
Applied to files:
xtask/src/documentation_integrity/error/display/refusal.rsxtask/src/documentation_integrity/corpus/replacement_tests.rsxtask/src/source_structure/python_source/environment/long_options.rsxtask/src/documentation_integrity/contributor_contract.rsxtask/src/documentation_integrity/error.rsxtask/src/fuzz_campaign/workflow_tests.rsxtask/src/source_structure/python_source/environment/tests.rsxtask/src/source_structure/tests/replacement.rsxtask/src/documentation_integrity/repository_text/tests.rsxtask/src/documentation_integrity/repository_text.rsxtask/src/documentation_integrity/execution/snapshot.rsxtask/src/source_structure/python_source.rsxtask/src/git_inventory/process.rsxtask/src/bounded_process/process_group/tests.rsxtask/src/source_structure.rsxtask/src/source_structure/pure_rust_tests.rsxtask/src/source_structure/source_inventory.rsxtask/src/documentation_integrity/corpus/source_witness.rsxtask/src/source_structure/source_error.rsxtask/src/documentation_integrity/corpus.rsxtask/src/documentation_integrity/workflow_contract/tests.rsxtask/src/documentation_integrity/node_toolchain.rsxtask/src/documentation_integrity/execution/tests.rsxtask/src/documentation_integrity/execution/corpus_guard.rsxtask/src/source_structure/source_file.rsxtask/src/documentation_integrity/error/display.rsxtask/src/documentation_integrity/execution.rsxtask/src/documentation_integrity/workflow_contract.rsxtask/src/source_structure/python_source/environment.rsxtask/src/git_inventory/process/tests.rsxtask/src/documentation_integrity/dependabot.rs
🔇 Additional comments (35)
xtask/src/source_structure/python_source/environment/tests.rs (1)
44-57: LGTM!xtask/src/documentation_integrity/contributor_contract.rs (1)
14-21: LGTM!xtask/src/git_inventory/process.rs (1)
10-10: LGTM! Confirms the previously-flagged Git-failure-precedence issue is resolved —inventory_outputnow checksoutput.succeededbefore decoding stdout.Also applies to: 52-63
xtask/src/git_inventory/process/tests.rs (1)
12-15: LGTM!Also applies to: 68-89
xtask/src/source_structure/pure_rust_tests.rs (1)
7-10: LGTM!Also applies to: 123-148
xtask/src/source_structure/python_source.rs (1)
111-112: LGTM!xtask/src/source_structure/python_source/environment.rs (1)
3-9: LGTM! Traced the abbreviation dispatch against the newlong_optionstable end-to-end (including the--spl=,--ignore-e,--unstest cases) — correctly resolves topython3and correctly fails closed on invalid/ambiguous long options.Also applies to: 67-81, 118-122
xtask/src/source_structure/python_source/environment/long_options.rs (1)
1-101: LGTM! Verified the abbreviation-matching logic against GNU coreutilsenv.c's actualstruct optiontable and value-mode semantics (required_argument/optional_argument/no_argument/split) — matches correctly. The table omits--version, but confirmed viaenv(1)that--version(like the modeled--help) causesenvto exit before ever invoking the target command, so this omission has no exploitable effect on Python-shebang detection.xtask/src/source_structure/source_inventory.rs (1)
27-27: LGTM! Tracked-mode record parsing is properly defensive (checked offsets, rejects malformed/unmerged/duplicate entries, revalidates the path viaadmit_inspection_path).Also applies to: 51-54, 70-129
xtask/src/source_structure/tests/replacement.rs (1)
13-15: LGTM! Traced the newnonregular_executable_candidate_is_refusedtest end-to-end againstinventory_violations— correctly proves the nonregular-candidate bypass is closed.Also applies to: 31-35, 57-61, 86-90, 140-170
xtask/src/source_structure/source_error.rs (1)
12-24: LGTM!Also applies to: 53-68, 113-120
xtask/src/source_structure/source_file.rs (1)
19-26: LGTM!Also applies to: 45-84, 133-168
.github/workflows/ci.yml (1)
137-139: LGTM!CHANGELOG.md (1)
31-56: LGTM!Also applies to: 67-68, 77-80, 117-126
xtask/src/bounded_process/process_group/tests.rs (1)
160-165: LGTM!Also applies to: 183-206
xtask/src/documentation_integrity/corpus.rs (1)
9-11: LGTM!Also applies to: 49-59, 126-143, 177-177
xtask/src/documentation_integrity/corpus/replacement_tests.rs (1)
24-24: LGTM!Also applies to: 38-62
xtask/src/documentation_integrity/corpus/source_witness.rs (1)
4-34: LGTM!Also applies to: 45-75, 108-147, 169-170
xtask/src/documentation_integrity/repository_text/tests.rs (1)
15-15: LGTM!Also applies to: 30-52
xtask/src/documentation_integrity/workflow_contract/tests.rs (1)
89-103: LGTM!Also applies to: 153-153
xtask/src/documentation_integrity/workflow_contract.rs (2)
115-147: 🎯 Functional CorrectnessFail-closed else-branch matches the previously accepted fix.
The
else { return Err(contract(...)) }branch here is exactly the fix already confirmed addressed in a prior review round for the "unclassified step mapping silently excluded" finding — correctly implemented.
24-28: LGTM!xtask/src/documentation_integrity/dependabot.rs (1)
23-31: LGTM!xtask/src/documentation_integrity/error.rs (2)
19-151: LGTM!
159-201: LGTM!xtask/src/documentation_integrity/error/display.rs (1)
27-87: LGTM!Also applies to: 121-167
xtask/src/documentation_integrity/error/display/refusal.rs (1)
7-23: LGTM!xtask/src/documentation_integrity/execution.rs (3)
166-212: LGTM!
30-39: LGTM!
52-58: 🩺 Stability & AvailabilityInspect whether cleanup errors are discarded on check failures.
This depends on the final
run_with(&mut runner, ...)/runner.close()call graph and whethercombine_checks(result, cleanup)is preferred over early short-circuiting.xtask/src/documentation_integrity/execution/corpus_guard.rs (1)
12-89: LGTM!xtask/src/documentation_integrity/execution/snapshot.rs (1)
42-267: LGTM!xtask/src/documentation_integrity/execution/tests.rs (1)
155-183: LGTM!Also applies to: 210-254
xtask/src/documentation_integrity/node_toolchain.rs (1)
24-33: LGTM!xtask/src/documentation_integrity/repository_text.rs (1)
12-94: LGTM!
Activity SummaryAll nine review threads are resolved (eight unique findings; the corpus-identity race was reported twice). Each actionable finding has an isolated commit and targeted evidence.
Current-head local evidence is green:
@codex review please — confirm the current head |
There was a problem hiding this comment.
💡 Codex Review
keep/xtask/src/fuzz_campaign/execution.rs
Lines 94 to 98 in 1254cdc
Stop the fuzz campaign after a terminal interruption
When a campaign has multiple target plans and the operator sends one SIGINT, SIGTERM, SIGHUP, or SIGQUIT, the process guard converts and consumes it as ProcessError::Interrupted, but this branch merely records one target failure and continues the loop. Subsequent targets can therefore run for their full per-target deadlines after a Ctrl-C or CI cancellation request; propagate interruption immediately instead of aggregating it like an ordinary target failure.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| source_process_directory, | ||
| &materialized, | ||
| )?; | ||
| verify_sources(source_root, source_process_directory, corpora, &config)?; |
There was a problem hiding this comment.
When .markdownlint-cli2.yaml or a non-Markdown link target changes after snapshot creation, this is its final identity check: DocumentationSnapshot retains neither the configuration witness nor namespace-file witnesses, while CorpusGuardedRunner::verify checks only Markdown and workflow corpora. The tools can therefore validate stale snapshot bytes and return success even though the current repository now has different lint policy or a missing/replaced link target; retain and revalidate all snapshotted inputs through the final success boundary.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
| fn cleanup_readers(self, error: ProcessError) -> ProcessError { | ||
| let mut child = self.child; | ||
| let error = cleanup_process(&mut child, error); | ||
| join_readers(self.stdout, self.stderr, error) |
There was a problem hiding this comment.
When a tool forks a grandchild that calls setsid while retaining stdout or stderr, the deadline expires and cleanup_process kills only the original process group, after which this unconditional join_readers waits forever for EOF from the escaped grandchild. Thus captured documentation, Git-inventory, and fuzz commands can exceed their advertised complete-operation deadline indefinitely; teardown needs a bounded way to close or cancel the reader workers even when process-group ownership is lost.
Useful? React with 👍 / 👎.
| fn command_index(commands: &[String], expected: &str) -> Option<usize> { | ||
| commands.iter().position(|command| { | ||
| command.lines().any(|line| { | ||
| line.trim().strip_prefix(expected).is_some_and(|suffix| { |
There was a problem hiding this comment.
In the checked ci.yml and fuzz-scheduled.yml jobs, this still accepts any line inside a run scalar, including heredoc data: an earlier step containing cat <<'EOF' followed by cargo xtask fuzz build --profile smoke and EOF satisfies command_index, although no build executes before the later run step. The regression gate can therefore remain green after the required build, corpus check, or minimization command becomes inert; require the reviewed commands as exact executable step bodies rather than searching arbitrary shell lines.
AGENTS.md reference: AGENTS.md:L103-L105
Useful? React with 👍 / 👎.
| let status = wait_for_child(program, &mut self.child, deadline, &self.interrupts)?; | ||
| refuse_exceeded(program, "stdout", self.limits.stdout_bytes(), &stdout).and_then(|()| { | ||
| refuse_exceeded(program, "stderr", self.limits.stderr_bytes(), &stderr) | ||
| })?; |
There was a problem hiding this comment.
When a child starts a same-group background descendant with stdout and stderr closed, emits oversized output, and exits, both readers reach EOF and wait_for_child reaps the group leader before this output-limit check fails. No cleanup then runs, so the background descendant continues indefinitely and the waitable identity that made group termination safe has already been consumed; detect late output or interrupt failures and terminate the group before reaping the leader.
Useful? React with 👍 / 👎.
| fn admitted_steps(workflow: &str) -> Result<Vec<DocumentationStep>, DocumentationError> { | ||
| let documents = YamlLoader::load_from_str(workflow).map_err(|source| { |
There was a problem hiding this comment.
The new workflow and Dependabot admission paths parse externally influenced YAML through YamlLoader, but a repository-wide check of fuzz/fuzz_targets finds no target exposing either YAML admission function; only the new JSON parser received a fuzz boundary. Malformed aliases, nesting, duplicate-key interactions, and scalar edge cases therefore lack the required parser fuzz evidence, so expose the production YAML admission boundary and register a bounded target for it.
AGENTS.md reference: AGENTS.md:L118-L118
Useful? React with 👍 / 👎.
| @@ -0,0 +1,68 @@ | |||
| # Dependency Admission: yaml-rust2 0.11.0 | |||
|
|
|||
| - Status: Accepted for repository-task workflow admission only | |||
There was a problem hiding this comment.
This admission is labeled and justified as workflow-only, but documentation_integrity/dependabot.rs also uses yaml-rust2 to parse .github/dependabot.yml in the production repository task. The record consequently omits the second admitted input, its policy-validation purpose, and its failure boundary even though its own final section requires reopening when admitted YAML use changes; update the dependency admission so the reviewed scope matches the code.
AGENTS.md reference: AGENTS.md:L143-L143
Useful? React with 👍 / 👎.
1 participant
Closes #47.
Problem
Documentation and workflow verification was split between Rust and seven Python programs, so
cargo xtask verifydid not own the complete repository integrity boundary.Invariant affected
Repository verification must select deterministic source corpora, admit exact tools and dependency graphs, bound external work, preserve precise refusals, and contain no Python scripts.
Approach
Move corpus selection, Dependabot coverage, Node lock and installer admission, contributor command laws, tool execution, Markdown links, and workflow linting behind
cargo xtask documentation-integrity-check. Routecargo xtask verifyand CI through that command, share bounded Git/file/process adapters, and remove the superseded Python files.Alternatives rejected
Retaining parallel Python orchestration preserves divergent coverage. Reimplementing Markdown, anchor, or workflow parsers in Keep would expand the parser surface; exact-version external tools remain the admitted boundary.
Failure modes
Typed refusals cover malformed or duplicate JSON, unsafe/non-UTF-8/nonregular paths, changed repository roots, empty corpora, dependency or installer drift, unavailable or wrong-version tools, bounded-output violations, process deadlines, simultaneous checker failures, broken fragments, and invalid workflows.
Tests added
Rust laws cover the former positive and negative Python cases plus stronger duplicate-field, installer-spoof, simultaneous-failure, and descendant-held-pipe regressions. The full debug and release workspace suites, doctests, all/minimal Clippy, Rust 1.96, fuzz checks, root/fuzz dependency policy and audits, pinned-tool verification, external malformed-input tests, and whole-tree whitespace check pass locally.
Benchmark impact
No production storage path changes. No benchmark impact is expected or measured.
Format and API compatibility
No content identity, durable format, or public
keepAPI changes. The new dependencies are optional, exactly pinned, and confined to the privatextaskrepository-task feature.Recovery implications
The checks are read-only. They do not publish or repair repository state, so interruption leaves no authoritative state requiring recovery.
Security implications
The boundary disables tool network access during validation, refuses duplicate JSON fields and unlocked installer substitutions, bounds captured output and execution time, and preserves exact dependency/tool admission.
Checklist
AGENTS.mdand the Keep Rust Engineering Standard.