feat(cli): Mill-style out directory with task records and install - #789
Conversation
…scapes Only remove an existing directory at the eject target when this function ejected that entry there before; a directory-valued entry that is new to the target, including one the user already has, is merged into rather than deleted. Removal now also confirms the destination's parent still resolves under the canonicalized target before deleting anything, so a symlinked intermediate directory inside the target cannot redirect a removal outside it. The target itself may still legitimately be a symlink, since both sides are canonicalized before comparing. Adds coverage for: the previous-list entry validation loop, degenerate value entries, merging into a foreign directory on first eject, removing a file no longer produced on a re-eject of an owned directory, the symlinked-intermediate escape attempt, and a symlinked target continuing to work.
Round-1 merged into a user's pre-existing directory on the first eject but then recorded the whole entry name as owned in ejected[target], so a second eject saw the entry in its own history and wiped the directory anyway, taking foreign content with it. ejected[target] now stores the flattened list of individual files a run actually writes under target (flatten_value_files): a file entry contributes itself, a directory entry contributes every file beneath it. A later eject diffs the old and new flattened lists and removes only the files that dropped out, never a directory wholesale, so foreign content survives any number of ejects. The remove_confined branch that used to wipe a directory when its entry was in the previous list is gone; directory entries are always merged into. remove_entry/remove_confined now report whether they actually removed something, so EjectReport.removed only lists files this call really deleted rather than stale bookkeeping for something already gone. Added a doc note on remove_confined's canonicalize check: it is advisory against accidental/stale symlinks, not a security boundary, and is TOCTOU-racy; closing that needs descriptor-relative removal, which is out of scope here.
remove_entry recursed into remove_dir_all for any stale path that happened to be a directory, so a record written by an earlier build of this branch that still lists a directory ENTRY name (rather than a flattened file path) would have that directory wholesale-deleted on the first eject after upgrade, foreign content included. The same happens if a user replaces a file eject wrote with their own directory of the same name and the task later stops producing that entry. remove_entry now refuses to remove anything that resolves to a directory, returning false instead of recursing. This module only ever deletes files it wrote; a directory it did not create as a file, under any bookkeeping scheme past or present, is never its to remove. prune_empty_parents remains the only place a directory is removed, and only once file-by-file removal has left it empty. Also switches the new-vs-old file diff to a HashSet lookup instead of Vec::contains, and bumps the morphir-rust submodule to pick up the ejected field doc/example update describing it as flattened file paths.
Before this change, eject copied over whatever already sat at the destination with no ownership check, then recorded those files as ejected. A later run whose artifact set shrank would then delete foreign content it never wrote, and a first run into a hand-written directory would silently overwrite it. eject now computes the full list of files it is about to write, stats each one, and refuses the whole operation with a CliError::Validation listing every conflicting path if any of them already exists and was not written by eject on a previous run to this target. Nothing is copied or removed when that error fires. Also: `ejected` is now keyed on the canonicalized target instead of the raw `-o` string, so `dist` and `./dist` share one bookkeeping entry instead of shadowing each other. And eject refuses a target that is the task's own `.dest` (or lies inside it), since that would make copy's source and destination alias each other.
run_single_file_compile always writes classic v3 JSON and hardcoded the literal "morphir-ir.json" rather than naming that path through ir_storage::v3_json_descriptor(). It also silently ignored a --config's [ir].layout/[ir].format, which apply on the project-mode compile path but do nothing here. Print one warning line when a config context is present and its [ir] section asks for a layout other than single-file or a format other than json, so the setting's absence of effect is visible instead of silent. The boolean condition is split into ir_storage_settings_apply_to_single_file_compile so it is unit-testable without capturing stderr; the eprintln wiring itself was verified manually with a scratch test exercising a real ConfigContext and observing the warning line, then removed before this commit.
The record section claimed an incomplete .dest without a matching .json is "always treated as incomplete and rerun" — nothing in the CLI reruns a task automatically. Say precisely what happens: it is treated as missing, and the user runs the task again. The eject section's directory-deletion claim was checked against the code and already states the rule precisely (eject never deletes a directory wholesale, only prunes ones its own file removals leave empty), so no change was needed there.
- Rename ir_storage.rs's SINGLE_FILE_STEM to IR_STEM: it also names the document-tree directory, not just the single-file stem. - Make generate/artifacts.rs's MANIFEST_PATH pub(crate) and use it from generate.rs instead of re-literaling ".morphir-generated-artifacts.json". - Delete help.rs's print_gleam_help: #[allow(dead_code)] with no callers, and it used no helper exclusive to it. - Sync examples/morphir.toml's [ir] block with docs/configuration.md by adding the layout and format keys it was missing. run_morphir in tests/cli_integration.rs already routes through morphir_command() (the helper that scrubs MORPHIR_OUT_DIR), so no change was needed there.
Rename the -o post-run copy step from "eject" to "install" throughout the CLI: the eject command module, the record's ejected field (now installed, via the morphir-devkit pointer bump), CLI help text, output JSON keys, human-readable output lines, error messages, tests, and the out-directory and configuration docs. This matches Zig's zig-out install-step naming, which the design doc already used to describe the model. No behaviour changes.
… record prepare_dest used to delete the previous <task>.json outright at the start of every run, carrying its installed map forward only through the record a successful run writes. A run that failed wrote no record, so the map was lost. The routine sequence compile -o dist (succeeds), compile (fails on a source error), fix, compile -o dist (succeeds again) would then have the second -o dist see its own earlier output sitting in dist/ as foreign content it never wrote, and refuse to run — the user had no way out short of deleting dist/ themselves. prepare_dest now leaves a tombstone instead of deleting: when a previous record exists and can be read, it writes back the same record with value and inputs emptied and language and ir set to None, keeping installed and completedAt as they were. generate now treats a tombstone (no ir, empty value) the same as a missing record, rather than reporting "produced no IR descriptor", which stays for a record that has a value but no ir. An unreadable record is still removed outright, as before.
Re-review of the tombstone fix found two latent hazards: - prepare_dest's tombstone kept the record's extra (serde flatten) map, so an unknown field computed from the previous .dest, such as a future inputsHash, would ride onto a tombstone whose .dest is empty. Clear extra on the tombstone. - prepare_dest wrote the tombstone after clearing .dest, not before. A crash or a failed record.write between the two would leave the full previous record, still claiming a real ir, beside an empty .dest, and generate would then fail deep in read_value with a raw I/O error instead of the friendly missing-record message. Writing the tombstone first closes that: the worst case is now the previous SUCCESSFUL record sitting beside its own intact .dest, which every reader already treats as a complete run. install() only rejected a missing record. A tombstone (record present, value empty) would pass, compute an empty file list, delete every previously installed file, and write back an empty ledger. Unreachable today, since every caller runs install right after a successful record.write, but add the guard: an empty value is now rejected with a message naming the task and telling the user to run it first. Several existing unit tests exercised install() with a value that shrinks to nothing as a way to test stale-file cleanup; those now carry an unrelated placeholder entry instead, so cleanup of the files that did drop out of value remains covered without tripping the new guard. Also: the design doc said the tombstone sets language and ir to null; both are skip_serializing_if, so the keys are omitted, not null. Fixed the wording and documented that extra is cleared too, and added a JSON-output integration test asserting the compile envelope carries installed_path when -o is given (the eject-to- install key rename had no test covering the JSON path).
✅ Deploy Preview for angry-raman-7c44f6 canceled.
|
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: 2d2fe2dac0
ℹ️ 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".
| // destination wiped first, whether this is the first time it | ||
| // has been installed here or the tenth, so any foreign content in | ||
| // it survives regardless. | ||
| copy_dir(&source, &destination)?; |
There was a problem hiding this comment.
When an install target contains an intermediate symlink matching a directory-valued entry (for example, dist/morphir-ir -> /outside), copy_dir follows that symlink through create_dir_all and std::fs::copy. A first install can therefore create files outside the requested target; after a previous install has marked those paths as owned, replacing the directory with a symlink also bypasses the conflict check and can overwrite existing outside files. Verify each destination parent resolves beneath canonical_target, as the stale-removal path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ab7a94: every install destination is confined in the pre-flight scan (nearest existing ancestor canonicalised and required to sit under the canonical target) before anything is written; an intermediate symlink pointing outside is refused with a Validation error naming both paths. Symlinks inside the target remain allowed. Unix symlink tests added.
| // Every caller today runs `install` right after writing a successful | ||
| // record, so a tombstone can never actually reach this function, but the | ||
| // guard costs nothing and keeps that true if a future caller changes. | ||
| if record.value.is_empty() { |
There was a problem hiding this comment.
When a backend succeeds with zero artifacts, run_generate writes a valid result record whose value is empty and immediately calls this function for -o; this guard then turns that successful generation into an error. The artifact publisher explicitly supports empty sets, including retiring a previous set, so this also prevents a later empty generation from removing files previously installed at the target. The record needs a distinct completion/tombstone state rather than using empty value for both cases.
AGENTS.md reference: AGENTS.md:L53-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a23d84 (with 2a7dd62 on the library side): the record now carries an explicit tombstone flag. install() refuses only tombstones, so a successful zero-artifact product installs and retires previously installed files; generate treats a tombstone as missing. Tests for both cases.
| if paths.dest.exists() { | ||
| std::fs::remove_dir_all(&paths.dest).map_err(|error| CliError::FileSystem { error })?; | ||
| } | ||
| std::fs::create_dir_all(&paths.dest).map_err(|error| CliError::FileSystem { error })?; |
There was a problem hiding this comment.
When two processes run the same compile or generate task concurrently, both enter prepare_dest without a task-level lock, so one can remove .dest while the other is writing it and both can overwrite the same result record. For generate, the publication lock in generate/artifacts.rs is acquired only later by write_all, so this deletion can even unlink the directory held by an active publisher; for compile, concurrent std::fs::write calls can directly leave corrupt or mismatched IR. Hold one lock across destination preparation, task execution, record publication, and installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 77377a9: prepare_dest takes an exclusive advisory fs2 lock on <task>.lock and returns it as a guard held through the run, the record write, and the install in all three command flows; a second run blocks (with one stderr line) until release. Contention test added.
`copy_dir` and the single-file copy went straight to `create_dir_all` and `fs::copy`, both of which follow a symlink sitting in the install target. A `dist/morphir-ir` pointing at `/outside` therefore sent a first install's whole entry outside `dist`; replacing an already-installed directory with such a symlink slipped past the foreign-content check as well, because every path underneath it is one install wrote itself, and the next run overwrote files outside the target. Every destination now has to resolve inside the target before anything is written, the same rule `remove_confined` already applies to removals: the nearest existing ancestor is canonicalized and checked against the canonical target, and a dangling symlink is refused rather than written through. The check runs in the pre-flight scan, so a refused install leaves the target exactly as it was.
`install` refused any record with an empty `value` on the grounds that it might be a tombstone. That also refused a generate that ran, succeeded, and had nothing to emit — and stopped such a run from retiring the files an earlier run had installed, leaving stale artifacts at the target with a ledger that still claimed them. A tombstone now says so: `prepare_dest` sets `tombstone` on the record it leaves behind, and every successful record leaves it false. `install` refuses a tombstone and nothing else, and `generate` reads the flag instead of guessing from a missing `ir` and an empty `value`. A successful record with an empty `value` installs normally, retiring what the previous run put at the target and recording an empty ledger for it. The unit tests that carried a placeholder entry only to get past the old guard go back to the empty `value` they were written with. Bumps the submodule for the `tombstone` field, which also brings in the member-path confinement, the task id parse fix, and the member-adjacent `out_dir` override fix; the workspace section of the design doc describes the last two.
The install target lock lived at <out root>/install-locks/<hash>.lock, so two different workspaces installing to the same absolute -o directory held two different locks and could both believe they held it uncontested. install_lock_path now resolves the lock under the user-global Morphir home directory instead: <Morphir home>/locks/install/<hash>.lock, keyed only on the canonical target. If the home directory cannot be resolved, it falls back to the old out-root location and says so on stderr. The digest scheme is unchanged. Unit tests now redirect MORPHIR_HOME to a sandbox directory once per test process, so they do not touch the developer's real Morphir home.
A copy failing partway through a multi-file install left the files copied before the failure on disk, but the function returned before writing the ledger, so those files stayed unrecorded. The next install read the old ledger, found files it did not know about, and refused the whole run as foreign content — wedging -o until someone cleared the target by hand. install now tracks every file a copy phase actually writes as it goes. On failure, it deletes whatever this run introduced that was not already owned by an earlier install (an overwritten file that was already ours is left alone), writes the ledger as the previous ledger minus whatever stale removal already deleted, and only then returns the error. Disk and the ledger agree again, so a later install is not wedged by an earlier failure.
Picks up finos/morphir-rust#144's third review round: absolute config context roots after a later chdir, a rejected unrecognized ir.mode value, and validated ir.layout/ir.format values at decode time. The last one means an IrSection can no longer be built through Deserialize with an unrecognized layout, so ir_storage.rs's coverage of IrStorage::from_config's own fallback check now builds that IrSection by hand instead.
Two gaps in the install rollback added in 6b7e7a7: 1. A copy that fails partway through can still create a partial file at its own destination (fs::copy creates the file before it starts writing). That file was never added to copied_files, so rollback never removed it and the ledger never recorded it, leaving it as foreign content that would wedge the next install. 2. roll_back_partial_copy stopped and propagated on its first deletion error, which lost the original copy error and skipped the ledger rewrite, leaving the ledger stale against disk. Rollback is now best-effort: every file this run introduced is attempted regardless of an earlier failure, and the ledger is always rewritten afterward to match what is really on disk, including any file rollback could not remove. When rollback does hit an error, it is folded into the message of the original copy failure rather than replacing it. Also documents the rollback behavior in docs/design/out-directory.md, and adds a test for an owned file replaced by a symlink pointing outside the install target (only the inside-target case was covered).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80ac78fec
ℹ️ 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".
| resolved_new_files, | ||
| target, | ||
| } = context; | ||
| let rollback_errors = roll_back_partial_copy(copied_files, previous_files_lookup, target); |
There was a problem hiding this comment.
When a supported internal symlink maps morphir-ir/x to real/x (or a case-insensitive rename changes a path’s spelling), copied_files contains the spelled path while previous_files_lookup contains the resolved ledger path. This new rollback call therefore treats a previously owned file as newly introduced and deletes it after any later copy failure, while the reconstructed ledger can still claim that deleted file exists. The fresh evidence is the latest rollback path passing these differently normalized collections directly to roll_back_partial_copy; map copied paths to their resolved forms before deciding which files are safe to remove.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 55d7e96: copied paths are mapped to their resolved form before the rollback decides what is safe to remove and before the ledger is rebuilt, so ownership is compared resolved-to-resolved and a previously owned file reached through an internal symlink survives a later copy failure. Unix test added.
| let resolved_new_files = new_files | ||
| .iter() | ||
| .map(|file| resolved_relative(file, target, &canonical_target)) | ||
| .collect::<Result<Vec<String>, CliError>>()?; |
There was a problem hiding this comment.
When the install target contains two supported internal symlinks such as a -> real and b -> real, artifacts a/config and b/config both resolve to real/config. This collection preserves both entries without checking for duplicate resolved destinations, and because conflict detection finishes before copying starts, both copies proceed and the latter silently overwrites the former. Reject duplicate values in resolved_new_files before any target mutation so the installed product cannot lose an artifact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7d7b54f (and 75c50be for case-only collisions on case-insensitive filesystems): after resolving the new file set, two spelled outputs that resolve to one destination are refused before any mutation, with an error listing the colliding paths and their shared destination. Unix test with two symlinks to one directory.
With dist/a and dist/b both linking to dist/real inside the install target, the entries a/config and b/config name the same file on disk. Every check before this one passes each of them individually, so the second copy silently overwrote the first. Refuse the whole install instead, before anything is written, once two spelled output paths resolve to the same destination. The error names every colliding spelling and the destination they share.
copied_files holds the SPELLED paths a copy loop wrote to, while the ledger (previous_files) holds RESOLVED paths. After a failed copy, roll_back_partial_copy and the ledger rebuild both compared these directly, so a file already owned through an internal symlink - dist/morphir-ir/x, say, when morphir-ir links to real and the ledger names real/x - looked unowned. Rollback deleted it as though this run had introduced it, while the rebuilt ledger, hitting the same mismatch, went on claiming it was still there. Map every copied path to its resolved form before either comparison runs, so ownership is judged resolved-to-resolved on both sides.
…er fixes Picks up two fixes from finos/morphir-rust#144: workspace.out_dir is now validated as a confined relative path at configuration load time, and wildcard member expansion (packages/*) now follows directory symlinks instead of silently skipping them.
…ensitive filesystems resolved_relative only resolves the directories above a destination, never the file name itself, so two output entries differing only in the case of their final component - a/Config and a/config - resolved to two different strings even with no symlink involved. On a case-insensitive filesystem (the default on macOS and Windows) those name the same file, so reject_colliding_destinations missed exactly the clobber it exists to prevent. Probe the install target's filesystem once per install - by filesystem identity on an existing entry when there is one, or on a disposable marker under the install lock's own directory when the target is empty - and fold case in the collision key when the probe says the filesystem does not distinguish it. An inconclusive probe is treated as case-insensitive, the safe direction.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75c50be2ad
ℹ️ 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 out = OutContext::resolve(config_context.as_ref(), &options.out, &start_dir); | ||
| let task = TaskId::compile(); | ||
| let prepared = out.prepare_dest(&task)?; |
There was a problem hiding this comment.
When a previous single-file Elm compile succeeded and the next invocation cannot read its source because the file was deleted, became unreadable, or contains invalid UTF-8, read_single_source returns before this preparation step. The old successful record and IR therefore remain consumable, so a later generate can silently use stale IR even though the latest compile failed. Acquire the task lock and write the tombstone before loading the source, as the provider compile path does.
AGENTS.md reference: AGENTS.md:L53-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0ea258f: single-file Elm compile now prepares the task (lock and tombstone) before reading the source, matching the provider path, so an unreadable source can no longer leave the previous record consumable.
| let candidates: Vec<&String> = unmatched.iter().map(|(_, resolved)| *resolved).collect(); | ||
| let renamed = renamed_previous_files(target, &previous_files, &candidates)?; | ||
| let renamed_to: HashSet<&str> = renamed.values().map(String::as_str).collect(); |
There was a problem hiding this comment.
When the next artifact name already exists as a hard link to a previously installed file, this identity lookup classifies the new name as a case-only rename and removes it from the conflict list. Copying the new artifact then truncates the shared inode, also changing the old output; the old name is skipped during stale removal and becomes an untracked leftover. A differently named hard link is foreign content, so rename handling should require actual filesystem case equivalence rather than identity alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8bd5723: identity matching is treated as a case-only rename only when the two spellings are equal under case folding in the same directory; a differently named hard link is a conflict.
| record.installed.insert(key, resolved_new_files); | ||
| record | ||
| .write(&paths.result) | ||
| .map_err(|error| CliError::Config { error })?; |
There was a problem hiding this comment.
If all target copies succeed but this result-record write fails—for example, the install fills the filesystem after the caller's initial record write—the target now contains newly added files while the persisted ledger still contains the previous set or is unreadable. A later install consequently rejects those files as foreign content and remains wedged until they are removed manually. The successful-copy path needs the same recovery treatment as copy failures when ledger publication fails.
AGENTS.md reference: AGENTS.md:L53-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d4ffa2f: a failed ledger write after successful copies is handled like a copy failure — files this run added that were not previously owned are rolled back and the original error is returned — so the next install is not wedged.
The case-only output collision test asked the filesystem whether it distinguishes letter case and then asserted the matching outcome, but on a case-sensitive filesystem the assertion never held, because install's own probe could not reach the same answer. `probe_case_sensitivity_at` treated a case-swapped spelling that is simply not there as "could not tell". On a case-sensitive filesystem the swapped spelling is never there, so every probe was inconclusive, install fell back to assuming case-insensitive, and two outputs that really are two different files were refused as a collision. A missing swapped spelling is now read as the answer it is: the filesystem tells the two names apart. The collision test's case-sensitive branch now also checks the contents of both files and both ledger entries, and a new test pins the probe itself to whatever the host filesystem does, so neither kind of filesystem is assumed.
…lved-path ownership Install used to support a symbolic link inside the `-o` target as an ordinary part of a user's layout. Paying for that meant owning every file by where it RESOLVED rather than by how it was spelled, and then guarding every way that resolution could go wrong: a check that each destination still resolved inside the target, a check that no ledger path had moved since install wrote it, a check that no two spellings landed on one file through links, and a spelled-to-resolved map so rollback could tell a file this run introduced from one it merely overwrote. Four rounds of review found four more edge cases in that machinery. One rule replaces all of it: nothing below the target may be a symbolic link. The target itself may still be one — `-o` pointing at a link is ordinary, and it is canonicalised first — but every component of every path install would write, and of every path its ledger owns, has to be a real file or a real directory. The pre-flight scan walks those components with `symlink_metadata` and refuses the whole install before any mutation, naming the link and suggesting the real directory instead. A link leading out of the target, a link with nothing at the far end, a directory swapped for a link after install created it, and two output paths made one file by a link are all the same refusal now. With one spelling per file, the ledger goes back to plain relative paths and ownership is a string comparison. `resolved_relative`, `resolve_directory`, `reject_moved_ledger_entries`, `symlinked_ancestor`, `confined_destination` and the rollback's spelled-to-resolved map are all gone. Kept as they were: the identity match that recognises a case-only rename of a file install already owns, the case-fold collision check for filesystems that do not distinguish letter case, the canonical-parent check on the removal side, the target lock and the shared input lock, the rollback, the tombstone rules, and the foreign-content refusal.
Rewrite the install section's symlink paragraphs to the one rule that replaced them — nothing below the `-o` target may be a symbolic link — and drop the description of resolved-path ownership that went with the old behaviour. Add a paragraph on member identity: a member is identified by its declared path, and the directory that path names may be a symbolic link leading outside the workspace, followed for reading with one warning. Add a Threat model subsection listing what the install and out-root code actually defends against, and against what it does not claim to be a boundary. Also correct the `ir.mode` note: the alias is now applied inside each configuration layer before merging, so precedence between layers works the ordinary way.
… inode `renamed_previous_files` recognises a file install already owns that has come back under a slightly different name — `Foo.gleam` as `foo.gleam` on a case-insensitive filesystem — by asking the filesystem whether the two paths are the same file. Any same-inode match counted, and a hard link is also the same file: a foreign `mine.txt` the user hard-linked to a file install owns was adopted as a rename of it, so the foreign-content check never ran, and the copy wrote through the shared inode and truncated both names at once. A match now has to be a case-only rename on both counts: the same file by identity, AND the same path but for letter case. Anything else is foreign content and is refused by name. `hard_linked_ledger_entries_are_all_kept_when_one_of_them_matches` proved the old adopt-any-inode behaviour, so it is replaced: one test for the hard link now being refused, and one that keeps the property the old test actually protected — every ledger entry matching one file is remembered, not just the last — set up with case-only names, which can only be two names on a case-sensitive filesystem.
Every copy could succeed and the record that names them still fail to reach disk — the filesystem holding the out root fills up, or the record's directory is not writable. The target was then left holding files no ledger on disk named, and the next install refused the whole run as foreign content it never wrote: the same wedge the copy-failure rollback exists to prevent, reached one step later. A ledger write that fails is now treated like a copy that fails. Everything this run introduced is removed again, best effort, leaving the target as the ledger still on disk describes it, and the original write error is returned. A file this run only overwrote was already owned, so it stays. A file the stale-removal pass deleted cannot be brought back, but a ledger entry whose file is already gone is something every later install handles without complaint.
…he source Single-file Elm compile read its input before calling `prepare_dest`, so a deleted or unreadable source returned early — before the task lock was taken and before the previous record was replaced with a tombstone. The previous run's SUCCESSFUL record was then left on disk beside a `.dest` that no longer matched it, and `generate`, which treats a tombstone as a missing compile but consumes a successful record without question, went on reading IR from a compile that had just failed. The source is now read after `prepare_dest`, which is the order the provider path has always used. A real single-file compile needs the Elm extension, so the test covers the ordering rather than the whole run: the failure comes from reading the source, so the tombstone is only written if `prepare_dest` ran first.
On macOS, realpath preserves the caller's spelling instead of correcting it to the spelling already on disk. So `-o dist` followed by `-o DIST` canonicalize to two different strings for what is really one directory: the second run found an empty previous-files list and refused Morphir's own output as foreign content, and the two spellings held two different lock files. Fix: when the target filesystem does not tell letter case apart, derive both the ledger key and the lock name from the canonical target with its components folded to lower case (a simple Unicode case fold is adequate, since it only decides which entries are the same key, never anything written to disk). When reading the ledger, also look up the un-folded key once, so records written before this fix still resolve; this run writes back only under the folded key, retiring the old one.
A failed std::fs::copy during install, whether for a single file entry or a file inside copy_dir, surfaced as a bare CliError::FileSystem: "File system error / Permission denied (os error 13)", with no indication of which file or where it was headed. Add CliError::Copy, which names the source and destination and keeps the original io::Error as its source, and raise it from both copy sites instead. The rollback behaviour after a failed copy is unchanged.
…the checks Two related hardening fixes: - ir_storage::read_value joined a task record's IR descriptor path onto its base directory unchecked. A hand-edited compile.json with a descriptor path like "../../x" could make it read outside the task's own output. Apply the same confinement rule install::validate_entry already uses for a record's value/installed entries (no absolute path, no "..", no empty or "." segment), factored out as install::confine_relative_path so both call sites share it, and fail with a Validation error naming the record's IR descriptor path. - install() created the -o target directory before checking whether the install would even be allowed to proceed, so a refused install (an escaping entry, a symlink below the target, colliding destinations, foreign content already there) still left a new, empty target directory behind. Move the directory creation to after the lock is held and every pre-flight check has passed. Since std::fs::canonicalize needs the target to exist, add canonicalize_for_install, which canonicalizes the nearest existing ancestor and joins the remaining, not-yet-existing components onto it literally -- correct because install goes on to create exactly those components, with exactly those names, and nothing can already be a symlink where nothing yet exists.
compile's JSON result reported output_path as the IR artifact file inside
.dest (morphir-ir.json, or whatever the configured storage names), on
both the classic single-file path and the v4 native path. generate
already reports the task's canonical .dest directory itself, and that is
what the out-directory design doc's output envelope describes. Make
compile report .dest the same way, on every success and failure branch.
Update crates/integration-tests/tests/elm_extension.rs, which asserted a
path ending in .morphir/out/compile.dest/morphir-ir.json, to assert the
.dest directory instead and read the IR back through
Path::new(&output.output_path).join("morphir-ir.json").
Note in the CHANGELOG's out-directory entry that output_path is the
task's .dest directory for every command.
…er note Three corrections to docs/design/out-directory.md, none of them matching what the code has done since earlier fixes in this series: - The rename-adoption paragraph said a destination is adopted as a rename of a ledger entry on filesystem identity alone. It also has to differ from the old spelling only by letter case, or a foreign file the user hard-linked to one install owns would be adopted as a rename and the copy would corrupt both files through the shared inode. - The threat-model bullet on the task lock said it is held "until the record is written." It is held through the end of the task's own -o install, matching what the earlier paragraph on `<task>.lock` already said. - The members paragraph did not say what happens when a symlinked member is reached by another path: running from inside the symlink's physical target discovers no enclosing workspace, and the project is treated as standalone rather than as that member.
Bring in a3615a7: fix(devkit): apply the ir.mode alias to the environment layer too.
Formatting fallout from the preceding fixes, mostly line-wrapping multi-argument function signatures and one long assertion.
1 participant
Summary
Moves the CLI onto a Mill-style out directory. finos/morphir-rust#144 is merged; the
ecosystem/morphir-rustsubmodule is pinned at its merged commit 473c849.<workspace>/.morphir/outby default. A root module's tasks sit directly under it; a member's under<member path>/. Each task owns<task>.dest(scratch) and<task>.json(result record).--out-dirandMORPHIR_OUT_DIRrelocate the root.compilewrites IR in the storage[ir]asks for (single-file JSON, single-file YAML, or a document tree) and records where in the result.generatereads the IR through that record viaresolve_ir_task, the single seam where transform stages (IR transform stages between compile and generate #786) will slot in.generate -istill accepts a file, a document tree, or a compile-output directory.-ois now an install step, the Zigzig-outmodel: the task runs to its canonical location first, then its declared product is copied into the user directory. Install keeps a per-target ledger of the files it wrote, removes only those when they are no longer produced, never deletes a directory it did not create, refuses to overwrite files it does not own, and survives a failed run through a tombstone record. Single-file Elm compile joins the same scheme; its-onow takes a directory.[workspace].out_dir,[ir].layout,[ir].format; removed keys warn. Docs, config specs, examples, CLI reference, completions, man page, and CHANGELOG updated. Newdocs/design/out-directory.mddescribes the layout, record, and install rules.Design
Spec discussion and rulings are summarised in
docs/design/out-directory.md. Follow-ups: #785 (input hashing), #786 (transform stages), #788 (task graph; install becomes a graph node and the tombstone goes away).Testing
cargo test -p morphirgreen (333 lib, 345 bin, 73 integration; 2 pre-existing ignored tests need external binaries).cargo clippy -p morphir --all-targets -- -D warnings,cargo fmt --check, andmise run ci:validate-docsclean.Known gap: the single-file Elm
-opath has no runnable test here (its end-to-end test is#[ignore]d behindMORPHIR_ELM_EXTENSION_BIN); CI with the real binary covers it.