Repository navigation
feat: discovery containment controls, Morphir home fixes and hermetic tests - #497
Merged
Merged
Conversation
Plan discovery containment controls (MORPHIR_DISCOVERY_BOUNDARY, MORPHIR_ISOLATED), fixes for the global Morphir home being discovered as a project and for native Windows MORPHIR_HOME paths, removal of the legacy Morphir.Tooling WorkspaceDiscovery/ConfigResolver, and hermetic test suites. Refs #494. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Group 1 of add-discovery-containment (#494). Adds tests/Morphir.TestSupport (framework-neutral, not packed) with: - TestSandbox: temp tree with its own home, machine config dirs and work area; Apply confines a child (HOME/USERPROFILE/APPDATA/XDG_*, MORPHIR_HOME, MORPHIR_DISCOVERY_BOUNDARY, MORPHIR_ISOLATED, inherited MORPHIR_* removed) while keeping the real NuGet cache. - ChildProcess: concurrent output capture, a shared concurrency gate (MORPHIR_TEST_MAX_PROCESSES) whose timeout starts once a slot is held, and kill-tree-then-await on timeout or cancellation. - TolerantDirectory: retrying cleanup that logs instead of throwing; LocalInstallationTests now uses it. - MachineTripwire: snapshot of the real machine's Morphir locations; a failing assembly hook fails the run (verified: exit 2). Morphir.TestSupport.Tests (29 tests) runs in the Test target. Documented in docs/content/docs/contributing/qa/test-isolation.md and AGENTS.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Group 2 of add-discovery-containment (#494). - MORPHIR_DISCOVERY_BOUNDARY and MORPHIR_ISOLATED mirror --discovery-boundary and --isolated for config get/list/show/path/explain/set/unset. A flag wins over its variable, which wins over the default (filesystem root, not isolated). Empty values are unset; relative boundaries resolve against the current directory; native Windows paths work. - --isolated accepts true or false; an absent flag (including System.CommandLine's implicit result) defers to the environment. - Invalid MORPHIR_ISOLATED (containment.invalid_value) and a current directory outside the boundary (discovery.boundary, naming the boundary's source) fail with exit 2 before any configuration I/O. Help ignores both. - Both names are reserved from MORPHIR_* configuration mapping. - config path --json reports the effective controls and their sources; the published inspection schema gains an optional containment object. - Containment.Resolve is a pure resolver shared by the commands, with 21 unit tests; 12 BDD scenarios cover inspection and editing. The editing scenarios fail against the previous command and pass with this one. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Group 3 of add-discovery-containment (#494). - project migrate takes its boundary from --discovery-boundary, then MORPHIR_DISCOVERY_BOUNDARY; the current directory and the path argument must lie inside it. - workspace migrate gains --discovery-boundary as an outer limit its directory must lie within. - Both fail with exit 2 and a discovery.boundary diagnostic (naming the boundary's source) before reading or writing. Migration stays isolated by construction, so MORPHIR_ISOLATED is neither read nor validated. - 5 BDD scenarios; the boundary ones fail against the previous commands. Existing migration features pass (31 project, 34 workspace). Docs and help updated; help ignores the variables. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…PHIR_HOME works Group 4 of add-discovery-containment (#494). - Discovery treats primary-named files directly in the global Morphir home as absent and never requests them. Walking up from a directory under the home folder used to find ~/.morphir/morphir.toml through the .morphir/ layout and load it as a global source and as the RootPrimary (and its Defaults projection). DiscoveryRequest gains an optional MorphirHome; IProjectTree carries it (default null), FileSystemProjectTree takes it, and the service decorators forward it, so the edit and migration services need no API change. - ProjectSystem.MorphirHomeDirectory computes the home (MORPHIR_HOME, else home/.morphir); all four native hosts pass it, even when isolated. - MachineLocations normalizes MORPHIR_HOME before testing whether it is absolute, so C:\\Users\\me\\.morphir and \\\\server\\share paths are no longer joined onto the start directory. - Tests: 3 machine-location cases and 5 in-memory discovery cases (red first), plus 2 CLI scenarios that reproduce both #494 bugs and now pass. Fantomas applied to the F# changes. - Spec delta: the relocated-home scenario now starts below the home, where the bug shows. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…covery Group 5 of add-discovery-containment (#494). BREAKING CHANGE: Morphir.Tooling.Configuration.ConfigResolver, WorkspaceDiscovery and TomlParser are removed, with their tests and the unused WorkspaceDiscovery registration in ManagementScenario. They walked to the filesystem root and treated any .git or .morphir directory as a workspace, including the global Morphir home, contradicting morphir-project-system, and could not be contained. No restorable package release ever exposed them. The configuration-resolution delta retires the legacy-compatibility requirement. Use ProjectSystem.Discover / LoadDetailed and the shared resolver, and named profiles instead of the legacy CI mode. - Morphir.Tooling's direct CsToml reference (only TomlParser used it) is removed; it stays transitive at the same version. - OsPaths, ArtifactPathResolver and their tests are unchanged and pass. - User docs updated; historical PRDs and archived changes keep their references. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…aking temp state Groups 6.1 and 6.2 of add-discovery-containment (#494). - Configuration, editing, project and workspace migration steps, CliTestHelper and GenerateCommandTests run children through ChildProcess: a shared concurrency gate whose timeout starts once a slot is held, concurrent output capture, and kill-tree-then-await on timeout. Previously a timed-out child was killed without waiting and its working directory deleted at once, which failed with 'being used by another process'. - The workspace recovery child test host gets its own --results-directory and a 120 s budget. - Every fixture cleans up with TolerantDirectory; the three C# generation classes and two backend loaders that never cleaned up now do. - ArtifactPathResolverTests uses an isolated global root instead of writing the real %APPDATA%\\Morphir\\dist. - The machine tripwire runs as an assembly hook. - ProjectMigrationCommandTests binds a boundary that contains the current directory, and a new test checks that a boundary excluding it fails before the host. Locally on Windows: 4 full runs at about 70 s each with no lock errors or timeouts; the last run is 561/561 green with no leaked morphir-* temp directories. A pre-existing, intermittent config.edit_rolled_back under load remains (File.Replace sharing violation); it also fails on the baseline. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…the repo Group 6.3 of add-discovery-containment (#494). - SharedSteps stores the last result in ScenarioContext; a [ThreadStatic] field is lost across awaits, so a scenario could read another's result or null. The locator, runner and resolver singletons are Lazy<T>. - ExecutableRunner runs through ChildProcess: concurrency gate, concurrent capture, process tree killed and awaited on timeout (previously a plain Kill(), not awaited, errors swallowed). - Repository-root lookups use AppContext.BaseDirectory instead of the current directory. - The manual AOT and trimming publishes go to a temp directory removed after each @aot scenario, run one at a time behind a dedicated gate (parallel publishes collide in src/Morphir/obj), have a 20-minute timeout, and pass arguments as a list. Verified: TestE2E --executable-type trimmed passes 24, skips 1, fails 0, with nothing written to artifacts/test-builds. The AOT steps compile but are manual-only and were not run. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Group 6.4 of add-discovery-containment (#494). LocalInstallationTests read stdout to the end before stderr (a full stderr pipe deadlocks) with no timeout, and VersionSourceTests waited without a limit. Both now run through ChildProcess, with 5- and 2-minute timeouts. The runner's own tests show that a sleeping child is killed within its timeout. All 49 build tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ion IO suites Groups 6.5 and 6.6 of add-discovery-containment (#494). - CarRentalSteps, TranspilerTests and the Myriad host test run dotnet through ChildProcess with MSBuild node reuse disabled; their deletes are tolerant. CarRentalSteps also removes its per-run 0.1.0-test.<guid> package from the NuGet cache after the run (the cache no longer grows per run). - GenerationCompiler retries its compile-directory delete (a local helper, since src/ must not depend on tests/). - The configuration recovery child runs through ChildProcess; the native-coordinator test kills and awaits its child on every failure path. A forced SpinUntil failure now leaves no running child (it previously kept the lease). - The F# package-migration evidence root is removed after a passing check and kept on failure. Verified: CodeGeneration.FSharp 333, Myriad 4, Myriad integration 12, Configuration.IO 451 and SDK 403 tests pass, with no new morphir* temp directories and no cache growth. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Task 7.2: the original #494 sandbox commands now load ~/.morphir/morphir.toml only as Global, and a backslash MORPHIR_HOME exits 0. openspec validate --strict passes. Task 7.1 stays open until CI has run on all three platforms. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Codex Review Summary: ✅ CompletedThis 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: efc3da1453
ℹ️ 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".
Comment on lines
+33
to
+36
| var normalized = ProjectPath.normalize(home); | ||
| environment = environment.Add("MORPHIR_HOME", ProjectPath.isAbsolute(normalized) || normalized.StartsWith("//", StringComparison.Ordinal) | ||
| ? normalized | ||
| : ProjectPath.join(startDirectory, normalized)); |
There was a problem hiding this comment.
When MORPHIR_HOME starts with .., normalizing it before joining it to the start directory discards the leading parent traversal: ProjectPath.normalize("../global") returns "global". An invocation from /work/app therefore loads /work/app/global instead of /work/global, unlike the previous implementation. This also causes discovery to exclude the wrong global-home location. Resolve relative values against startDirectory before collapsing their dot segments.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
System.Collections.Hashtable[4498401933]
Comment on lines
+67
to
+68
| foreach (var key in startInfo.Environment.Keys.Where(k => k.StartsWith("MORPHIR_", StringComparison.OrdinalIgnoreCase)).ToArray()) | ||
| startInfo.Environment.Remove(key); |
There was a problem hiding this comment.
When the test runner inherits MORPHIR_HOME, MORPHIR_ISOLATED, or MORPHIR_DISCOVERY_BOUNDARY from a containing harness, this loop removes those protections without replacing them with a sandbox; HOME, APPDATA, and XDG locations still point at the real machine. The child consequently falls back to real machine locations rather than remaining contained. Apply a per-invocation TestSandbox and use its work directory instead. This violates the explicit Morphir-child isolation requirement in AGENTS.md.
AGENTS.md reference: AGENTS.md:L511-L511
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
System.Collections.Hashtable[4498401947]
Comment on lines
+150
to
+154
| process.Kill(entireProcessTree: true); | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Already exited. |
There was a problem hiding this comment.
If a child exits while a grandchild retains its redirected pipes—for example, /bin/sh -c 'sleep 60 &'—the output wait reaches its timeout after the direct child has already exited. Killing that exited process cannot terminate the now-orphaned grandchild, but this catch suppresses the failure and subsequently reports ProcessExited=true based only on the parent. The descendant remains running after RunAsync throws, violating the process-tree cleanup invariant in AGENTS.md. Track descendants or use an owned process group/job that remains terminable after the direct child exits.
AGENTS.md reference: AGENTS.md:L510-L510
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
System.Collections.Hashtable[4498401952]
Review of #497 (#494). - MORPHIR_HOME: a relative value is joined to the start directory before dot segments collapse, so ../global from /work/app resolves to /work/global again (normalizing first dropped the leading traversal). 3 new cases. - CliTestHelper runs each invocation in its own TestSandbox (fake home and machine locations, sandbox MORPHIR_HOME, boundary and isolation, the sandbox work area as its directory) instead of only stripping inherited MORPHIR_* settings, which let the child fall back to the real machine. - ChildProcess reaches descendants that outlive the direct child while holding its pipes. On Linux the child starts under setsid and a timeout kills its whole process group (verified in WSL: the group kill reaches an orphan after the leader exits); Windows already reaches them through the recorded parent. ProcessExited is true only when the parent exited and both pipes closed. macOS has no setsid, documented. A new test covers a grandchild that would write a marker after its parent exits. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements OpenSpec change
add-discovery-containment(#494): discovery containment controls, two discovery bug fixes, removal of the legacy resolver, and hermetic test suites. Lands before thev0.4.0-alpha.1tag.Containment controls
MORPHIR_DISCOVERY_BOUNDARYandMORPHIR_ISOLATEDmirror--discovery-boundaryand--isolatedforconfig get/list/show/path/explain/set/unset.--isolatedacceptstrue/false, so a command can turn off isolation set in the environment.Fails with exit 2 before any configuration I/O:
MORPHIR_ISOLATED(containment.invalid_value);discovery.boundary, naming where the boundary came from).--helpignores both variables.config path --jsonreports the effective controls and their sources. The published inspection schema gains an optionalcontainmentobject.project migratehonors the boundary, andworkspace migrategains--discovery-boundary. Neither reads machine or personal configuration, soMORPHIR_ISOLATEDdoesn't apply.Both names are reserved, so they never become configuration values.
Bug fixes
~/.morphir/morphir.tomlthrough the.morphir/layout and loaded it asGlobal,RootPrimaryandDefaults. Discovery now receives the Morphir home and never treats primaries directly in it as project locations, even when isolated.MORPHIR_HOME(C:\Users\me\.morphir, UNC paths) is no longer joined onto the current directory, which made everyconfigcommand fail withdiscovery.read_error.BREAKING (Morphir.Tooling API)
Morphir.Tooling.Configuration.ConfigResolver,WorkspaceDiscoveryandTomlParserare removed. They walked to the filesystem root and treated any.gitor.morphirdirectory as a workspace, including the global home. No restorable package ever exposed them. Theconfiguration-resolutionlegacy-compatibility requirement is retired in the spec delta. Replacements:ProjectSystem.Discover/LoadDetailed, and named profiles instead of the CI mode.Hermetic tests
New non-packed
tests/Morphir.TestSupport(29 tests of its own):ChildProcess: a concurrency gate (MORPHIR_TEST_MAX_PROCESSES) whose timeout starts once a slot is held, concurrent output capture, and the process tree killed and awaited on timeout or cancellation.TestSandbox: fake home and machine folders, the containment variables set, inheritedMORPHIR_*removed, and the real NuGet cache kept.TolerantDirectory: cleanup that never fails a passing test.MachineTripwire: an assembly hook that fails the run if the real~/.morphir,%APPDATA%\Morphiror XDG directories change.Every process-heavy suite now uses them. Also fixed:
ArtifactPathResolverTestswrote into the real%APPDATA%\Morphir\dist.SharedStepskept results in a[ThreadStatic]field acrossawaits.being used by another process).artifacts/test-builds.Documented in
docs/content/docs/contributing/qa/test-isolation.mdand AGENTS.md.Test plan
Morphir.Tooling.Testsused to have 18 failures (timeouts and locks) and leak about 54 temp directories per run. It now runs 561 tests in about 70 s, with no lock errors, no timeouts and no leaked directories, and the tripwire is clean.TestAllafterPackAll: every other suite passes. Tooling had one intermittent failure, which three reruns didn't reproduce.Globalonly, and a backslashMORPHIR_HOMEexits 0.openspec validate add-discovery-containment --strict.Known issue
An intermittent
config.edit_rolled_backunder load is a transient Windows lock onFile.Replace. It predates this PR and is filed as #496.🤖 Generated with Claude Code