refactor: share artifact writing and PATH binary lookup - #499
Conversation
|
@codex review Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cc7516eae4
ℹ️ 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".
| // `resolve_validate_target` — otherwise this pre-flight validates one | ||
| // manifest and the install that follows writes another. | ||
| std::fs::read_dir(&path)?; | ||
| let src_manifest = crate::manifest::loader::find_app_manifest(&path).ok_or_else(|| { |
There was a problem hiding this comment.
When the user runs aware app install . (or passes ..), this now always reports that no manifest exists: find_app_manifest begins with root.file_name()?, and dot-component paths have no file name, so it returns None without scanning the directory. The previous inline read_dir scan accepted these common directory arguments; normalize the path before calling the helper or let the helper fall back to scanning when no basename is available.
Useful? React with 👍 / 👎.
| AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) | ||
| })?; | ||
| std::fs::read_dir(&app_dir)?; | ||
| let manifest_path = crate::manifest::loader::find_app_manifest(&app_dir).ok_or_else(|| { |
There was a problem hiding this comment.
When a source folder contains multiple manifests and its selected manifest declares an app id different from the source directory name, looking it up again under apps/<app_id> can select a different file because the helper's preferred <dir-name>.flo candidate has changed. For example, installing bundle/bundle.flo with app: alpha while bundle/alpha.flo is also present validates and copies the former, then this call loads the latter and writes its lockfile; subsequent installed-app discovery also sees the unintended manifest. Reuse the already loaded src_app or preserve the selected source filename rather than rerunning basename-dependent selection after the directory rename.
Useful? React with 👍 / 👎.
|
Both P1 findings were real and are fixed in P1: dot-directory paths. Confirmed: P1: reuse the manifest selected before copying. Confirmed, and your worked example is exactly right — I also made the Both fixes carry integration tests that I verified red on Gates 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: 1158a76ccd
ℹ️ 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 manifest_name = src_manifest.file_name().ok_or_else(|| { | ||
| AwareError::Internal(format!("installed app {app_id} missing .flo/.app file")) | ||
| })?; | ||
| let manifest_path = app_dir.join(manifest_name); |
There was a problem hiding this comment.
When bundle/bundle.flo declares app: alpha beside bundle/alpha.flo, this pins only the immediate lockfile load: install_app_from_path still copies both files into apps/alpha, while run, show, and discover_apps subsequently call find_app_manifest on that directory, whose basename preference selects alpha.flo rather than the installed bundle.flo. Fresh evidence after the claimed fix is that these downstream lookups remain, while the new integration test checks only the lockfile. Ensure the installed directory permanently identifies the originally selected manifest so later commands cannot execute or report the decoy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it reproduces exactly as you describe. My fix pinned the lockfile and nothing else, and the result is arguably worse than what it replaced — the two halves of the installed state now disagree:
$ aware app install $T/bundle # bundle.flo → app: alpha, alpha.flo → app: decoy
✓ installed alpha (lockfile written)
$ ls $AWARE_HOME/apps/alpha
alpha.flo bundle.flo lockfile.yaml
$ grep '^app:' $AWARE_HOME/apps/alpha/lockfile.yaml
app: alpha # ← the manifest that was installed
$ aware app list
warning: app "decoy" is in directory "alpha" — name and `app:` field disagree
decoy 0.1.0 1 0 linear # ← what discovery, run and show actually see
So alpha is locked and decoy is what runs. You are right that the new integration test only checked the lockfile, which is why it went green over this.
I am not attempting a third fix, and I am not merging. Two reasons.
The first is scope. The durable fix is a design decision I should not make unattended: either the installed directory records which manifest is authoritative (a new field in lockfile.yaml that discover_apps honours), or install refuses a source folder carrying two app manifests. Both change the on-disk contract for every installed app and reach well past collapsing duplicated helpers, which is what this PR is for.
The second is that this ambiguity predates the PR. On main, discover_apps already called find_app_manifest with the same <dir-name>.flo preference, and install's inline scan picked whichever manifest read_dir happened to yield — so a two-manifest folder was already resolved inconsistently there, just non-deterministically rather than deterministically. What this PR did was make the inconsistency reliable and legible, which is how you found it.
My recommendation to the maintainer: land duplications 1 and 3 (the artifact writer and the PATH lookup — untouched by any of this), and split duplication 2 out with the manifest-identity question filed as its own issue. I have left the PR open rather than doing that split myself, since after two review rounds the next call is yours, not mine.
Generated by Claude Code
…n each
Three helpers had been written more than once and drifted. Each is now one
implementation both callers use.
1. The `output-path` artifact writer — 4 copies.
`html-report.render`, `ui.render`, `viewer-3d.render` and `ifc.write` each
carried a byte-for-byte copy of "write the rendered bytes to `output-path`
on a real run, report the location and size either way"; `viewer_3d`'s
module doc already described itself as mirroring the other two. Three
emitted the documented `path` alias beside `output-path` and `ui.render`
emitted only `output-path`, so `{{ node.path }}` resolved against three of
the four artifact producers and silently against the fourth. Now
`render::write_artifact`, with `ui`'s manifest declaring the alias its
siblings already declared.
2. The app-manifest selector — 3 inline scans.
`manifest::loader::find_app_manifest` prefers `<dir-name>.flo`, then any
`.flo`, then any `.app`. `resolve_validate_target` was moved onto it and
its comment says the rule is "shared with install" — but install kept three
inline `read_dir` scans that took whatever the filesystem yielded first, so
a directory holding two sources could be validated as one app and installed
as the other inside a single `aware app install`. Each site keeps its
`read_dir` probe, so a permissions or IO failure still surfaces as itself
rather than as "no .flo or .app file".
3. The PATH binary lookup — 2 scans that bypassed `crate::which`.
That module exists to be the one answer to what `Command::new` can launch,
and its doc comment is about this exact drift. Both copies appended `.exe`
and nothing else, so a bridge installed as a `.cmd` shim read as absent on
Windows; `sidecar.rs` also hand-split PATH on `;`, which mishandles a quoted
entry that `std::env::split_paths` handles. Same first answer wherever an
`.exe` exists — strictly a superset.
Considered and left apart, being different abstractions that only look alike:
`truncate_detail` / `truncate_error_detail` (different budgets and suffix
contracts, both deliberate); the two `Envelope` and two `Hit` structs (shared
names, unrelated shapes); `app_lock::find_app_source` (wider extension set,
file-or-dir, serving `app compile`); the read-side and write-side `Provenance`
models (opposite optionality); `builder::{npm,ruby}::extract_surface` (one
signature, two language grammars); and the per-host-version reflected agents
under `20-agents/aeco/architecture/`, whose identical command docs are what
reflection produces, not drift.
Gates from `cli/`: cargo fmt --all --check, cargo clippy --all-targets
-D warnings, cargo test — all pass (1230 unit + integration tests green).
…nstalled Both P1 findings from Codex's review of cc7516e. Collapsing install's three inline manifest scans onto `find_app_manifest` inherited two defects from the helper's basename preference, one of them pre-existing. 1. `aware app install .` reported "no .flo or .app file". `find_app_manifest` opened with `root.file_name()?`, which is `None` for `.` and `..`, so it returned before reading the directory at all. The inline scans it replaced had no such preference and worked. The directory name now comes from the path resolved against the working directory, and a path with no basename falls through to the scan instead of short-circuiting. This also fixes `aware app validate .`, which had the same defect on main — `resolve_validate_target` was moved onto this helper earlier. 2. Install could lock and discover a manifest it never installed. Install copies the source folder to `apps/<app-id>`, which renames the directory, so re-running a `<dir-name>.flo`-preferring selector afterwards asks a different question: a folder `bundle/` holding `bundle.flo` (`app: alpha`) beside an `alpha.flo` was validated and copied as `bundle.flo`, then re-read as `alpha.flo`. The installed manifest is now the one already selected, under its own name — there is no second lookup. The `read_dir`-order fallback is also sorted now. It was unpinned, which is what made the second defect reachable at all, and with no basename to prefer (the `..` case above) it is the only rule left deciding what gets picked up. Both fixes carry integration tests that fail against the code before them: `app_install_accepts_a_dot_path` and `app_install_locks_the_manifest_it_actually_installed`, verified red on the parent commit and green here. Gates from `cli/`: cargo fmt --all --check, cargo clippy --all-targets -D warnings, cargo test — all pass (1232 unit + every integration suite).
pawellisowski
changed the title
1158a76 to
8664245
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Two duplicated helpers now have one implementation shared by every caller:
html-report.render,ui.render,viewer-3d.render, andifc.writenow delegate torender::write_artifact. The shared result reportsoutput-path,path, and byte size consistently;_core/uideclares the existingpathalias used by the other artifact producers.crate::which, preserving the first existing answer while adding the Windows.cmd,.bat, and.comlaunch forms that Rust'sCommandcan execute.The app-manifest selector originally included here was removed from this PR after review exposed a broader identity problem between install and later discovery. That work is tracked separately in #502.
Type of change
Behaviour changes
ui.rendernow emitspathalongsideoutput-path; no field is removed or renamed..exe; existing.exeresolution order is unchanged.Verification
Run from
cli/:cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test— 1,224 unit tests passed, 0 failed, 1 ignored; integration suites passed_core/ui, invokedaware agent invoke ui render, and verified the reported aliases, file existence, HTML prefix, and byte countaware-tekla.cmdon a temporary PATH and verifiedaware sidecar list --jsonclassified it as a legacy installationReview rounds: 3 (self-feeding: 1) — rounds: initial review found manifest-selection defects; follow-up found downstream identity drift; final narrowed-diff review found no issues; outcome: manifest-selector slice split to #502, remaining changes clean in final local Codex review