refactor: collapse three duplicated abstractions (vector maths, PATH lookup, tree copy) - #399
refactor: collapse three duplicated abstractions (vector maths, PATH lookup, tree copy)#399pawellisowski wants to merge 5 commits into
Conversation
…der::geom `render::ifc` and `render::viewer_3d` consume the same scene JSON and each carried its own copy of the same eleven primitives — dot/cross/length/distance, normalize, the polygon walk, the segment-intersection and simple-polygon tests. The two copies were identical maths in two spellings of a vector: `ifc` used tuples, `viewer_3d` used fixed arrays. `ifc` also carried a *third* cross product (`fn cross`) beside its own `cross3`, and `viewer_3d` restated cross/dot/length inline in its frame guards. Arrays win as the one spelling, so `ifc`'s `Vec2`/`Vec3` are now the shared aliases and its tuple field access becomes indexing. The drift this exposes: `viewer_3d`'s `normalized3` had lost the `is_finite` guard its `ifc` twin kept, so a `1e200` direction overflowed to an infinite length and it answered `Some([0,0,0])` — a unit vector of length zero. The shared version rejects it up front, so that frame now fails as "directions must be nonzero and finite" instead of surviving to fail the orthonormality check by luck. Its regression test is updated to say so.
`runtime::invoker` and `render::blender` each had a `find_on_path`, and the two had drifted in the way that only bites on Windows: `blender`'s omitted the `.com` extension, and appended an extension even to a name that already carried one — so an `AWARE_BLENDER=blender.exe` override searched for `blender.exe.exe` and reported Blender missing. `invoker`'s handled both, so its behaviour is what survives; `blender` gains the fix. `find_in_dirs` moves with it, along with the test that covered it.
Three copies of the same tree walk: `install::local` (shared by `install::rename` and `install::registry`), `commands::voice`, and a `plugins::claude_code` test fixture. Two were byte-identical; `voice`'s differed only in returning `AwareError` and leaving the destination root for its caller to create. Neither edge is behaviour a caller needs kept, so the shared version creates the destination and returns `io::Error`, which `voice` maps at its one call site — no flag, no mode parameter.
|
@codex review Generated by Claude Code |
![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: 23c5015e57
ℹ️ 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".
| let dirs: Vec<PathBuf> = std::env::split_paths(&path).collect(); | ||
| // If the caller already supplied an extension (e.g. "codex.cmd"), don't append a | ||
| // second one — look the name up verbatim. | ||
| let exts: &[&str] = if cfg!(windows) && Path::new(name).extension().is_none() { |
There was a problem hiding this comment.
On Windows, when AWARE_BLENDER uses the explicitly supported bare command form blender-4.2 and PATH contains blender-4.2.exe, Path::extension() returns Some("2"), so this condition searches only for the unsuffixed name. resolve_blender_binary routes that documented override form through this helper, while Blender's previous lookup appended .exe, meaning the override now incorrectly reports that no executable exists. Treat only recognized executable suffixes as already extended, or try the Windows suffixes in addition to the verbatim name.
Useful? React with 👍 / 👎.
| for entry in std::fs::read_dir(src)?.flatten() { | ||
| let from = entry.path(); | ||
| let to = dst.join(entry.file_name()); | ||
| if entry.file_type()?.is_dir() { |
There was a problem hiding this comment.
When a source passed to aware voice install contains a symlink to a directory, its former copier used from.is_dir(), which followed the link and recursively copied the referenced contents. DirEntry::file_type() does not follow symlinks, so the shared copier now takes the file branch and std::fs::copy fails because the source resolves to a directory. Such voice packs therefore regress from installing successfully to returning a copy error; preserve the former follow-and-recurse behavior for this caller or explicitly handle directory symlinks here.
Useful? React with 👍 / 👎.
Two real defects the unification introduced, both caught on review. `which`: the verbatim-lookup rule keyed on *any* extension, and `Path::extension` splits at the last `.`, so the documented `AWARE_BLENDER=blender-4.2` override parsed as extension "2" and stopped resolving to `blender-4.2.exe` on Windows — something `render::blender`'s append-always copy had handled. The rule now keys on the extension being one Windows can actually spawn, and the verbatim name is tried last as a fallback, so every lookup either former copy resolved still resolves. `fs_tree`: `install`'s copy classified entries with `DirEntry::file_type`, which does not follow symlinks, while `commands::voice`'s used `Path::is_dir`, which does. Taking the former would have made a voice pack containing a symlink to a directory stop installing — the entry takes the file branch and `fs::copy` fails on a directory source. The `voice` rule survives instead; since following links makes cycles reachable, the walk now tracks its open ancestors and reports a cycle rather than recursing until the stack dies.
|
Both P2 findings were real and are fixed in
Gates re-run from @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ae5b8c5a
ℹ️ 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".
| // name like "blender-4.2" resolving to "blender-4.2.exe". | ||
| let exts: &[&str] = match () { | ||
| _ if !cfg!(windows) || has_executable_extension(name) => &[""], | ||
| _ => &[".exe", ".cmd", ".bat", ".com", ""], |
There was a problem hiding this comment.
On Windows, including "" makes find_on_path accept an extensionless regular file even though this module explicitly notes that such files are not directly spawnable by CreateProcess. If an earlier PATH directory contains a Unix-style codex, claude, or Blender shim while a later directory contains the usable .cmd/.exe, the directory-first loop returns the unspawnable file and the caller fails instead of reaching the valid executable. Keep the verbatim fallback for Unix or already-spawnable extensions; dotted bare names still resolve by trying the Windows suffixes.
Useful? React with 👍 / 👎.
Second-round Codex finding, and correct: the search is directory-outer, so including "" in the Windows suffix list let an extensionless shim in an early `PATH` entry win over the spawnable `.cmd` in a later one — handing the caller a file `CreateProcess` cannot launch instead of the valid executable further down. Returning an unspawnable name is not a better answer than `None`; it is the same failure moved somewhere the caller cannot diagnose it. Windows now searches the executable suffixes only. Nothing regresses: a dotted bare name like `blender-4.2` still resolves through them, and a name that already carries a spawnable extension is still looked up verbatim. The suffix lists are named constants so the tests exercise the list `find_on_path` really uses — the previous test asserted against a hand-copied list, which is how it passed while the code under it was wrong.
|
Third P2 addressed in The search is directory-outer, so including One thing worth naming, because it is the reason the bug survived my own testing: the test I added asserted against a hand-copied suffix list rather than the one Gates re-run from Note for anyone reading the run log: the local test suite initially reported failures that were All three findings from both rounds are now fixed at the root. None were rejected. @codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf61219084
ℹ️ 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".
| if from.is_dir() { | ||
| visit(&from, &to, ancestors)?; |
There was a problem hiding this comment.
When installing or updating from a registry archive containing a directory symlink whose target exists outside the extraction root (for example, an absolute link to / or a user directory), from.is_dir() follows the link and visit recursively copies that external tree into the installation. This is reachable through custom AWARE_REGISTRY payloads because tar extraction preserves symlinks; the former install copier used entry.file_type() and rejected directory symlinks through fs::copy instead. Keep the follow-links behavior only for the voice-pack caller, or otherwise ensure registry/install copies cannot traverse outside their source root.
Useful? React with 👍 / 👎.
Stopping here — not merging. This one needs a human.Codex reviewed The finding is real, and I am not going to fix it in this run. Two reasons. 1. I am at the review capThis routine may re-request review at most twice after fixes. I have used both rounds ( 2. More importantly — this finding says unification #3 was the wrong callThe repo's own rule for this sweep is that two things which merely look alike must stay apart, and that a shared signature which makes one caller grow a flag to get its old behaviour back is the tell. That is exactly what has happened, and it took three rounds to surface:
Those are two different policies, not one function with drift. Every fix I pushed moved the shared copier toward one caller at the other's expense. The honest read is that Deciding what Recommended dispositionDrop unification #3 and land the other two. Unifications #1 ( Alternatively, if the intended policy is "no copy ever traverses a symlink out of its source root," that is a good change — but it should be its own PR with the trust boundary stated, not a side effect of deduplication. Findings status, for the record: three fixed at the root with tests ( Generated by Claude Code |
|
PR sweeper, fresh pass. Not merging, and not pushing a fix. Codex's fourth P2 on I re-derived the question from
So there is no zero-regression single rule, and that is now demonstrable rather than argued:
Each option is a product decision about the substrate's trust boundary with registry payloads, and three of the four regress a caller that works on The previous pass's recommendation still looks right to me on the evidence: land unifications #1 ( No code was changed in this run. No cross-model review is outstanding — Codex has answered, and its answer is a finding. Generated by Claude Code |
2 participants
Scheduled abstraction sweep. Three things were implemented more than once and had drifted apart; each is now one implementation both callers use.
1. 2D/3D vector maths →
render::geomrender::ifcandrender::viewer_3dconsume the same scene JSON and each carried its own copy of the same eleven primitives:dot3,cross3,length3,distance3,normalized3,polygon_edges,point_in_polygon,point_segment_distance,cross2,segments_intersect,polygon_is_simple_nonzero. Identical maths, two spellings of a vector —ifcused tuples(f64, f64, f64),viewer_3dused arrays[f64; 3].Beyond the pair,
ifccarried a third cross product (fn cross, tuple-typed) beside its owncross3, andviewer_3drestated cross/dot/length inline in two frame guards. Arrays win as the one spelling, soifc'sVec2/Vec3are now the shared aliases and its tuple field access became indexing.This one found a bug.
viewer_3d'snormalized3had lost theis_finiteguard itsifctwin kept. AuDirof[1e200, 0, 0]passesfinite_number, but1e200 * 1e200overflows toinf, so the old code divided by infinity and returnedSome([0, 0, 0])— a "unit" vector of length zero. It only failed at all because that zero vector went on to trip the orthonormality check downstream, by luck. The shared version rejects the non-finite length up front.The user-visible consequence is a changed error message on that input:
directions must be nonzero→directions must be nonzero and finite, which is also more truthful about what is wrong.degenerate_bolt_frame_is_rejected_not_panickedis updated with a comment explaining the change; it still asserts what it was written to assert, that this input is a validation error and not a panic.2.
find_on_path→crate::whichruntime::invoker(findsclaude/codex) andrender::blender(findsblender, or whateverAWARE_BLENDERnames) each had one. They had drifted in ways that only bite on Windows, and — as Codex caught on review — neither copy was wholly right:blender's omitted the.comextension.blender's appended an extension even to a name that already carried one, soAWARE_BLENDER=blender.exesearched forblender.exe.exe.invoker's verbatim rule keyed on any extension.Path::extensionsplits at the last., so the documentedAWARE_BLENDER=blender-4.2form parses as extension"2"— which would have stopped it resolving toblender-4.2.exe, a regressionblender's append-always copy did not have.The unified rule keys on the extension being one Windows can actually spawn (
exe/cmd/bat/com, case-insensitive), and tries the verbatim name last rather than instead — so every lookup either copy resolved still resolves, and both drifted cases are fixed.find_in_dirsmoved with it, along with the test that covered it; six more tests pin the behaviours.3.
copy_dir_recursive→crate::fs_treeThree copies:
install::local(shared byinstall::renameandinstall::registry),commands::voice, and aplugins::claude_codetest fixture. Two were byte-identical.voice's differed in three ways: it returnedAwareError, it left the destination root for its caller to create, and it classified entries withPath::is_dir.The first two are not behaviour any caller needs kept, so the shared version creates the destination and returns
io::Error, whichvoicemaps at its one call site. No flag, no mode parameter.The third one mattered, and Codex caught that the first revision of this PR got it backwards.
Path::is_dirfollows symlinks;DirEntry::file_typedoes not. Unifying oninstall's rule would have made a voice pack containing a symlink to a directory stop installing — the entry takes the file branch andfs::copyfails on a directory source. So thevoicerule survives, which is also whatinstallwould have wanted, since its rule turned such a tree into an error rather than a copy. Following links makes cycles reachable, so the walk tracks the canonical path of every directory open on the recursion stack and returns an error on a cycle rather than recursing until the stack dies.Review
Codex reviewed
23c5015eand raised two P2 findings — thewhichdotted-name case and thefs_treesymlink case above. Both were real defects introduced by the unification, neither was a false positive, and both are fixed at the root in60ae5b8cwith tests. Nothing was rejected or triaged away.Considered and rejected
Two more pairs look like duplicates and are deliberately left apart:
builder::mod::now_isoandruntime::provenance::now_iso— byte-identical (chrono::Utc::now().to_rfc3339()), but they timestamp two unrelated contracts: generated-agent provenance metadata and the runtime's JSONL log envelope. Sharing one function would couple the two formats such that changing one silently changes the other. A four-token expression is not worth that coupling. Left as-is.render::file::abs_pathandrender::blender::abs_path— genuinely identical and a fair candidate, but this PR is capped at three unifications to stay reviewable in one sitting. Deferred rather than rejected.kebab_asciiandopenapi::kebab— already documented in-tree as not the same slugifier. Untouched.Gates
Run from
cli/on the pinned 1.95.0 toolchain, with CI's apt deps (clang libsecret-1-dev libdbus-1-dev pkg-config) installed:cargo fmt --all -- --check— passcargo clippy --all-targets -- -D warnings— passcargo test— pass (42 test binaries green)Substrate check
Nothing here bakes in a host or an extension.
render::geomis scene geometry with no domain meaning;crate::whichis a genericPATHlookup (its one host-shaped comment, about theblenderoverride, describes a caller, not a behaviour);crate::fs_treeis a plain filesystem walk.