mirendev/runtime#1412

This is ready for human review. The three new commits do what phinze's threads asked for. I found one compatibility gap that neither the description nor the docs mention. Separately, pr_status currently reports the branch as not mergeable against main, so it will probably need another rebase before it can land.

New finding: a cluster on an older server will reject a source-root app. The projected .miren/app.toml still contains [build] source_root = "../..". The server reads it through loadAppConfig (servers/build/build.go), which calls appconfig.Parse → decodeAndValidate, and that uses dec.DisallowUnknownFields(). A server built before this PR has no SourceRoot field, so a new CLI deploying a source-root app to it fails at build time with an unknown-field error on build.source_root. Nothing in the CLI warns about this. Every new app.toml key in this repo probably has the same skew, so it isn't unique here. But the description and deployment.md:149 both say the server needs no changes, and that will mislead someone. Two ways to close it:

• Have the client drop source_root from the projected copy. The server never uses it.
• Document that the feature needs a server that understands the key.

Absolute source roots (aa45f79). Validate now rejects an absolute build.source_root (appconfig/appconfig.go:482-484). Every load path runs through decodeAndValidate, so deploy, analyze and gating all fail early. SourceDir no longer has an absolute-path branch, so even an unvalidated value would just be joined under the config dir. TestSourceRoot covers the rejection. Relative ../ traversal is still allowed. You chose a docs warning over a printed resolved-root line, which is a reasonable place to stop; phinze can judge whether it's enough on that thread.

Elixir (4f62021). The app name now has - converted to _ before matching at depth 1. A test case shows my-shop-web selecting a later my_shop_web release, and the guide documents the conversion.

Docs. deployment.md now covers source-root detection and a warning that a root Dockerfile.miren beats both detection and an explicit [services.web] image. That addresses what I raised before and phinze's docs thread.

Why I'm not recommending approval:

• The makeTarWithFilter override path runs on every deploy.
• The Elixir release preference changes what existing umbrella apps build.
• The server-version question above needs a decision.

The open threads from phinze, and my old profile handoff thread, are for people to close.

Inline comments

• docs/docs/deployment.md:149 — The projected app.toml still contains [build] source_root. The server parses it with appconfig.Parse, which calls DisallowUnknownFields() (servers/build/build.go loadAppConfig). So a server built before this PR fails the build on an unknown field, and "needs no new … behavior" doesn't hold across versions. Either strip source_root from the projected copy on the client (the server never uses it), or state the minimum server version here and in the PR description.

---

Verdict: ready

mirendev/runtime#1412

The blocker from my last review is fixed, and I think this is ready for human review.

Missing .miren/ directory entry (fixed in 01d23ea). writeFile in pkg/tarx/tarx.go (lines 329–340) now starts from a synthesized TypeDir header with mode 0755. It switches to the real directory's metadata only when that directory exists under the source root, and a stat error other than not-exist now fails the upload instead of being skipped. That covers the documented layout where only the service has a .miren/.

The tests now check this on the server side:

• TestSourceRootUploads unpacks every layout (with default, without default, gitignored, included) through the real tarx.TarFS, for full, delta-config and cached-config archives.
• It then compares the extracted file contents.
• The delta-config archive for the "without default" layout holds only .miren/ and .miren/app.toml. That was the exact failure case, and it's covered now.
• TestAnalyzeSourceRoot no longer writes a root .miren, so the one end-to-end CLI-to-server test runs the documented layout.

One small behaviour change: parent-directory stat and FileInfoHeader errors that used to be swallowed now abort the archive. For walked files the parent has just been visited, so this only matters if a directory disappears mid-walk. Failing loudly there is reasonable.

Elixir release selection. You've confirmed the switch is intentional. The guide now covers the exact-match preference, the fallback to the first release, the lack of hyphen/underscore normalization, and the warning for existing umbrellas. I'm treating that as settled. The release that gets built and the /app/bin/<release> start command come from the same stack instance in pkg/buildkit, so a matched release can't end up with a mismatched start command.

Why I'm not recommending approval: makeTarWithFilter's override path runs on every deploy, not just source-root ones. It bypasses gitignore and replaces the walked entry in the manifest and in both archive variants, so a human should still read it. The Elixir change also alters what existing umbrella apps build on their next deploy, which is a product call. I've seen nothing wrong in either.

One behaviour worth a reviewer's glance: a service app that relies on auto-detection, without an explicit build.dockerfile, now detects from the source root. If the repo root has its own Dockerfile.miren or a stack marker like go.mod, that's what the service will build. The docs' examples always set dockerfile, so this matches the stated design. It may still be worth a sentence in deployment.md.

The older profile thread is still open. It's a human-review handoff and the redesign made it obsolete, as I said last round, so a person should close it.

---

Verdict: ready

mirendev/runtime#1412

I don't think this is ready yet. The deploy layout the docs recommend will fail on the server. The cause is how the projected .miren/app.toml reaches the build context.

Blocker: no .miren directory entry in the tar when the source root has none. makeTarWithFilter's writeFile writes a parent directory header by calling os.Stat on filepath.Join(dir, dirPath). When that stat fails it continues (pkg/tarx/tarx.go:329). For the override, dir is the source root. If the source root has no .miren/ (the tree in docs/docs/deployment.md, the "without default" case in TestSourceRootUploads, or source_root = "src"), the archive gets .miren/app.toml with no .miren/ entry in front of it. The server unpacks both full and delta uploads through tarx.TarFS. See StreamRegistry.Stage (stream_registry.go:105) and extractTarIntoDir (saga_builder.go). TarFS creates directories only for TypeDir headers, and opens regular files with a plain os.Create(path) (tarx.go:570). So extraction fails with ENOENT before the build starts, and --analyze breaks the same way through build.go:1719. The tests miss this:

• TestSourceRootUploads reads the archive with archive/tar, never TarFS.
• TestAnalyzeSourceRoot, the one real CLI-to-server path, writes a root .miren/app.toml ([[[), so the directory happens to exist.

Fixing it in the writer means emitting a synthesized directory header (e.g. 0755) when the parent doesn't exist under dir. Making TarFS MkdirAll the parent would also be reasonable. Either way, please add a test that unpacks the "without default" layout with tarx.TarFS, or drop the root .miren from TestAnalyzeSourceRoot.

For a human: Elixir release selection (645eda9). This applies to every Elixir build, not just source_root apps. An existing umbrella whose Miren app name matches a release other than the first will build a different release on its next deploy. The Elixir guide still says the release comes from "the releases: entry". Details are inline. This is a judgment call, not a defect.

The rest of the change holds together. runDeploy keeps configDir (targets, gating, parent-directory notice) apart from the source dir (git info, manifest, tar). SourceDir/UploadFiles are nil-safe, so -a app with no local config behaves as it does on main. Once the tar fix lands, the FileOverride path in pkg/tarx/tarx.go still deserves a human read. It bypasses gitignore and replaces the walked entry in the manifest and in both tar variants, and every deploy goes through it.

Inline comments

• pkg/tarx/tarx.go:329 — This breaks the override when the source root has no .miren/ directory, which is the documented monorepo layout. The stat fails, the loop continues, and the archive contains .miren/app.toml with no .miren/ directory entry before it. On the server, tarx.TarFS (used by StreamRegistry.Stage, extractTarIntoDir and the analyze path) only creates directories for TypeDir headers. It writes regular files with a plain os.Create(path) at line 570, so the upload fails with ENOENT. When the parent doesn't exist under dir, emit a synthesized directory header (e.g. mode 0755) instead of skipping it. Alternatively, have TarFS MkdirAll(filepath.Dir(path)) before creating a file. TestSourceRootUploads passes the "without default" case only because it reads the tar with archive/tar. Please add a round trip through tarx.TarFS for that layout.
• pkg/stackbuild/elixir.go:134 — Human judgment needed, not a confirmed bug. This applies to every Elixir build, not only source_root apps. An existing umbrella app named shop with releases: [worker: ..., shop: ...] deployed worker before this change. On its next deploy it will build and start shop, and nothing tells the user. That's probably what they meant all along, but please confirm the switch is intended. If it is, update docs/docs/guides/elixir.md: the Umbrella section and the agent quick-reference line ("the releases: entry in mix.exs") should say a release named after the app is preferred. They should also note that matching is exact. Release keys are atoms with underscores, so an app named my-shop will never match a my_shop release, and every app name in the new monorepo docs is hyphenated.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply) The redesign makes this question obsolete. Named profiles, missingConfigError() and MIREN_PROFILE are all gone. Each app now has its own .miren directory, and -d services/api (or cd) selects it, so an explicit -a with no local config works the same as on main. I'm leaving the thread open because it was a request for human review, and a person should close it.

---

Verdict: not_ready

mirendev/runtime#1412

I think this is still ready for human review. 1cb95a4 settles the follow-up I left at app.go:130, and I found no new problems in the upload or selection paths.

Missing-config guidance. noAppMsg and the TTY confirmation both name appconfig.LocalAppConfigPath() now (app.go:114 and 136). So a mistyped --profile wroker gives "No app configuration found at .miren/wroker/app.toml. Run 'miren init' to create it for app "monorepo"?", not the old message saying the directory isn't set up. The prompt still defaults to yes. It now shows the exact file it will create and the inferred app name, though, so accepting is an informed choice and not a trap. I'm satisfied with that. In CI there's no TTY, and the error includes the selected path, so a mistyped MIREN_PROFILE is easy to diagnose. TestProfileSelection asserts that the non-TTY message contains .miren/missing/app.toml. I also agree with doing this without profile-specific branching, because it keeps Validate simple.

Late failure on deploy. You've chosen to keep source validation at upload time and not add profile-aware checks to deployOpts.Validate. That's your call. The result is still correct, since MakeTar/ComputeManifest return ErrNotExist before anything wrong can be uploaded, so I'm dropping this point.

Docs. The help placeholder is now PROFILE, not <profile>, so MDX won't parse it as JSX. The regenerated command reference picks up the change, and I found no raw .miren/<profile> left in docs/.

I'm still not recommending this skip human review. The PR adds new CLI surface, introduces package-level profile state in appconfig, and changes the deploy upload path. A reviewer should look hardest at three places:

• appconfig/profile.go: the global currentProfile, which is safe only while every caller sets it before concurrent work starts.
• The FileOverride handling in pkg/tarx/tarx.go: the override bypasses gitignore and replaces the walked entry in the manifest, the full tar and the filtered tar.
• The pre-parse of --profile in cli/cli.go, which runs before alias expansion and before mflags parses the real flags.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply) I read 1cb95a4, and it covers my app.go:130 follow-up. The non-TTY error and the confirmation prompt both name the selected path now, and the prompt says which app it will create there. Keeping source validation at upload time rather than in deployOpts.Validate is a reasonable boundary, so I'm not pressing it. I'm leaving this thread open because it started as a request for human judgment, and a person should be the one to close it.

---

Verdict: ready

mirendev/runtime#1412

I think this is still ready for human review. c3337c9 answers the question I raised last round and doesn't break the upload or selection paths.

The fix works. missingConfigError is gone. LoadAppConfigWithPath and LoadAppConfigUnder now return nil, nil for a missing named profile, the same as for a missing default. So -a/MIREN_APP works through the existing AppCentric.Validate path without any special case for profiles. TestProfileSelection covers the explicit-app case for both the working directory and -d. It also asserts that opts.config stays nil, so nothing falls back to .miren/app.toml, and that neither loadLocalAppConfigOrWarn nor WarnLocalAppConfig writes to stderr. On the upload side, TestConfigProfiles asserts that ComputeManifest and MakeTar return os.ErrNotExist for a missing selected file. So deploy -a foo --profile missing can't ship the wrong config; it fails when the tar is built.

Two consequences of treating a missing profile like a missing default. Neither blocks the merge, but you should know about both:

• If the profile name is mistyped and no app name is given, the command now goes down the "this directory isn't set up yet" path. On a TTY that offers miren init, defaulting to yes. Accepting creates .miren/<typo>/app.toml with the app name taken from the directory, and miren deploy then goes on to deploy that new app. Off a TTY the error is the generic "no app configuration found", which doesn't mention the profile. Details are inline.
• With -a set, a missing profile now fails late in runDeploy, as a raw ENOENT from MakeTar. By then the confirmation prompt and the "Deploying" line have already appeared. Against an older server it also happens after CreateDeployment (deploy.go, legacy path), so a deployment record gets created and then marked failed. The outcome is correct, but the error is rougher than the early Validate failure in the first commit. If that matters to you, deployOpts.Validate could stat the UploadFiles(dir) sources up front.

I'm still not recommending this skip human review, for the same reasons as before: it adds new CLI surface and package-level selection state, and it changes the deploy upload path.

Inline comments

• cli/commands/app.go:130 — Since c3337c9, a selected profile that's missing ends up in this branch, and both the message and the prompt assume the directory has never been set up. With a typo like --profile wroker and no -a, a TTY user is told "Looks like this directory isn't set up yet", even though .miren/ already holds other profiles. Accepting the prompt (it defaults to yes) makes initApp write .miren/wroker/app.toml, named by inferAppName(workDir). That is usually the monorepo's directory name, and miren deploy then deploys it as a brand-new app. Without a TTY, the error doesn't mention the profile at all, so a mistyped MIREN_PROFILE in CI is hard to track down.

A small fix: when appconfig.LocalAppConfigPath() isn't the default, put the missing path in noAppMsg (e.g. "no app configuration at .miren/wroker/app.toml"), and either skip the init offer or reword it so it says it will create a new profile. This doesn't block merging, but it's a cheap fix before the docs start pointing people at --profile.

Replies to existing threads

Posted to the PR's comment threads when you submit.

• (reply) I read c3337c9 and it does what was agreed. A missing named profile now loads as nil, nil, so -a/MIREN_APP goes through the normal Validate path. TestProfileSelection covers both the CWD and -d cases. It also checks that nothing falls back to the default config and that neither warning API prints anything. Deploy still can't upload the wrong file, because ComputeManifest/MakeTar return ErrNotExist for the missing override. I've left one follow-up inline at app.go:130 about the init prompt and the error message that a mistyped profile now hits. I'm leaving this thread open: it was a request for human judgment, so a person should close it.

---

Verdict: ready

mirendev/runtime#1412

I think this is ready for human review. I didn't find a correctness defect. With no profile selected, behaviour is unchanged: UploadFiles returns nil, missingConfigError returns nil, and LocalAppConfigPath() resolves to the same .miren/app.toml. That keeps the risk for existing users low.

Upload path (pkg/tarx). The refactored walk is equivalent to the base version; the two nested include/ignore ifs became one. The override is skipped during the walk and appended afterwards, so .miren/app.toml never appears twice, and its parent directory is still emitted when .miren/ is gitignored. ComputeManifest reports the profile file's hash at the canonical path, so delta and fully cached deploys get the right config. runDeploy and analyzeApp both pass the override. The server-side build reads only the constant appconfig.AppConfigPath (servers/build/build.go:1225), not the profile-aware helpers, so running miren server with MIREN_PROFILE set can't redirect it. TestConfigProfileUploads covers full, delta, config-cached, no-default and gitignored layouts. It also checks byte counts and that the repository files are left untouched.

Selection plumbing. cli.Run pre-scans for --profile so alias expansion reads the selected app.toml. Cmd.Invoke then sets the profile again from the parsed global flag. Switching localAppConfig from sync.Once to a cache keyed on the config path means the second set costs nothing when the profile is the same, and the config reloads when an alias switches profile. SourceDir counts path segments rather than hard-coding two Dir calls, which is the right fix for resolvedDir.

One behaviour change for a human to decide (not a defect). Today, an AppCentric command that is given an explicit app name succeeds without any local config; TestAppCentricValidate/"app flag with no app.toml succeeds" asserts this. With a named profile selected but missing, Validate now returns an error at app.go:76-78 before it ever looks at a.App. So MIREN_PROFILE=worker miren logs -a foo, run from a checkout without .miren/worker/, now fails. The same missing profile also triggers the "not found" warning from loadLocalAppConfigOrWarn callers, and from Run's WarnLocalAppConfig on any failing command. That's consistent with how a broken config already behaves, and the PR says a missing profile is an error. It still deserves a deliberate yes, because the new docs suggest setting MIREN_PROFILE at the CI-job level, where it will apply to every miren call in the job.

I'm not recommending this skip human review. It adds new user-facing CLI surface and package-level mutable selection state (appconfig.currentProfile), and it changes the deploy upload path. Those design choices deserve a reviewer's sign-off even though I found nothing wrong in them.

Inline comments

• cli/commands/app.go:77 — Human judgment needed, not a confirmed bug. When a named profile is selected but its app.toml is missing, local.load() / LoadAppConfigUnder now return missingConfigError(), and this line fails the command before a.App is checked. Without a profile, -a myapp (or MIREN_APP) works with no local config at all; TestAppCentricValidate/"app flag with no app.toml succeeds" asserts that contract. Should an explicit app name skip the missing-profile error (keeping it an error only when the profile is the sole source of the app name), or is failing fast the intended behaviour whenever a profile is selected? This matters because the docs encourage exporting MIREN_PROFILE at the CI-job level.

---

Verdict: ready