Repository navigation
fix: name migration path in removed-profiles warning - #1300
norrietaylor merged 3 commits into
Conversation
The warning for a `profile =` declaration stated only that profiles were removed in 0.5.1, leaving no remediation. A config relying on a profile for its packages then composes without them and fails later as `command not found` / exit 127, with nothing naming the cause. Point each warning at where the packages should move: `[session]` packages for `[defaults]profile`, and `[tasks.<name>] packages` for a per-task profile.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe pull request updates deprecated profile warnings. The warnings now identify the replacement package fields for defaults and task configurations. ChangesProfile warning updates
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change only clarifies where packages should be declared when a removed profile is detected; no actionable merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the change, the reason for it, and the affected configuration paths. It also lists verification commands and results. It does not use the template's exact Summary and Testing headings and omits the Checklist, but the required information is mostly complete.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…ng-43737fd4787527a3
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: matches the stated intent -- message-only change to two tracing::warn! calls in crates/mfile/src/lib.rs, no behavior change, single file. Clobber: none; this only extends the warnings 70c9b59 ("remove profiles support everywhere") introduced, nothing reverted. Correctness: the per-task warning ([tasks.<name>]profile -> [tasks.<name>] packages) is accurate -- Task carries its own packages field consumed at execution time. The [defaults]profile warning is not: it tells users to move packages to [session] packages, but [defaults].profile was a default applied to every task (per the pre-removal docs and hydrate_task_defaults), and [session] packages is never merged into a task's sandbox (checked Context::task/make_env and Graph::hydrate_task) -- only [stack] build_packages/runtime_packages and each task's own packages are. Following the new guidance for the [defaults]profile case reproduces exactly the command not found/exit 127 failure this PR exists to prevent, just for every task instead of one. See the finding below. Nothing here required a build to determine.
What ran
6 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg for Task/Session struct defs and profile/packages fields in crates/mfile/src/lib.rs, tasks.rs — exit 0 · self-reported
Task (crates/mfile/src/tasks.rs:7) has its own
packages: Vec<String>field, confirming[tasks.<name>] packagesis a valid remedy target. Session (crates/mfile/src/lib.rs:462) also haspackages, but its doc comment scopes it to 'sessions activated on this project', not task sandboxes.
2. Read File::hydrate_task_defaults (crates/mfile/src/lib.rs:1144-1166) — exit 0 · self-reported
Confirms
[defaults].profileis copied onto every task lacking its ownprofile(task.profile = default_profile.clone()) -- i.e. it was a per-task default, never session-scoped.
3. Read Context::task/make_env path (crates/mctx/src/lib.rs:810-838) and Graph::hydrate_task (crates/graph/src/graph.rs:243-273) — exit 0 · self-reported
Neither merges
mfile.session.packagesinto a task'spackages/sandbox; only stack build_packages/runtime_packages and the task's ownprofile/packagesare consulted when a task actually runs.
4. git show 70c9b59^:docs/concepts/profiles.md (pre-removal docs for profiles) — exit 0 · self-reported
Original docs state: 'Setting defaults.profile ... will apply the profile to all tasks which do not have a profile explicitly set.' Confirms defaults.profile packages were consumed by task execution, not the interactive session.
5. rg for other occurrences of the same 'move its packages to [session] packages' guidance in the repo — exit 0 · self-reported
Only one occurrence (the [defaults]profile warning touched by this diff); the sibling per-task warning correctly points at [tasks.] packages, so this is local to one line, not a repeated pattern.
6. Check for secrets, Cargo.lock/.minimal/minimal.toml changes, and revert of recent merges via git log — exit 0 · self-reported
Diff touches only crates/mfile/src/lib.rs warning strings; no dependency files changed, no secrets found, nothing reverted.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head 4bed22cd5489 · verdict COMMENT · 1 finding(s), 1 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
| tracing::warn!( | ||
| "[defaults]profile = \"{p}\" defined, but profiles were removed in 0.5.1; move its packages to [session] packages" | ||
| ); |
There was a problem hiding this comment.
HIGH · correctness · confidence 0.78 · in File::warn_unknown_fields
This warning tells users to move packages from [defaults]profile to [session] packages, but that's the wrong target and reintroduces the exact failure this PR is meant to fix. [defaults].profile was a per-task default: File::hydrate_task_defaults (crates/mfile/src/lib.rs:1145-1149) copies it onto every task that doesn't set its own profile, and the pre-removal docs (docs/concepts/profiles.md, removed in 70c9b59) confirm 'Setting defaults.profile ... will apply the profile to all tasks which do not have a profile explicitly set.' [session] packages (crates/mfile/src/lib.rs:462) is a separate, session-scoped concept -- it's only consumed by the daemon's interactive session composition (crates/minimald/src/session_host.rs, crates/sessions/src/daemon/composer.rs). Nothing in the task-execution path (Context::task/make_env in crates/mctx/src/lib.rs:810-838, Graph::hydrate_task in crates/graph/src/graph.rs:243-273) reads session.packages into a task's package set -- only [stack] build_packages/runtime_packages and the task's own packages are. So a user who follows this new guidance for a former [defaults]profile gets the packages into their interactive shell but every task still runs without them, hitting the same silent command not found/exit 127 this PR's own description cites as the bug. The sibling per-task warning a few lines down (line ~1077, [tasks.<name>] packages) is correct since Task does carry its own packages field -- this is local to the [defaults]profile case, not a repeated pattern.
What the reviewer checked
docs/concepts/profiles.md (pre-removal, from git show 70c9b59^): "Setting defaults.profile within a codebase minimal.toml will apply the profile to all tasks which do not have a profile explicitly set." Combined with hydrate_task_defaults at crates/mfile/src/lib.rs:1145-1149 (
task.profile = Some(default_profile.clone())) and the absence of anysession.packagesread in crates/mctx/src/lib.rs's task()/make_env or crates/graph/src/graph.rs's hydrate_task.
Hand this to an agent
Act on a code-review finding.
File: crates/mfile/src/lib.rs
Lines: 1041-1043
Category: correctness
Severity: high (reviewer confidence 0.78)
Finding:
This warning tells users to move packages from `[defaults]profile` to `[session] packages`, but that's the wrong target and reintroduces the exact failure this PR is meant to fix. `[defaults].profile` was a *per-task* default: `File::hydrate_task_defaults` (crates/mfile/src/lib.rs:1145-1149) copies it onto every task that doesn't set its own `profile`, and the pre-removal docs (`docs/concepts/profiles.md`, removed in 70c9b59) confirm 'Setting defaults.profile ... will apply the profile to all tasks which do not have a profile explicitly set.' `[session] packages` (crates/mfile/src/lib.rs:462) is a separate, session-scoped concept -- it's only consumed by the daemon's interactive session composition (crates/minimald/src/session_host.rs, crates/sessions/src/daemon/composer.rs). Nothing in the task-execution path (`Context::task`/`make_env` in crates/mctx/src/lib.rs:810-838, `Graph::hydrate_task` in crates/graph/src/graph.rs:243-273) reads `session.packages` into a task's package set -- only `[stack] build_packages`/`runtime_packages` and the task's own `packages` are. So a user who follows this new guidance for a former `[defaults]profile` gets the packages into their interactive shell but every task still runs without them, hitting the same silent `command not found`/exit 127 this PR's own description cites as the bug. The sibling per-task warning a few lines down (line ~1077, `[tasks.<name>] packages`) is correct since `Task` does carry its own `packages` field -- this is local to the `[defaults]profile` case, not a repeated pattern.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
[worker-iterate:addressed]
Addressed: the [defaults]profile warning now points to each task's [tasks.<name>] packages (or [stack] build_packages/runtime_packages), not [session] packages — confirmed hydrate_task_defaults copies defaults.profile onto every task while session.packages is not read by the task-execution path.
Generated by Worker iterate agent (side-repo-ops operator)
|
Commit pushed:
|
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: Small, focused diff — two tracing::warn! message strings in crates/mfile/src/lib.rs, matching the stated intent (name the remedy, not just the removal). No behavior change, no clobber of recent work.
Correctness: Verified against hydrate_task_defaults (crates/mfile/src/lib.rs:1144-1166) that [defaults]profile is a per-task default and that [stack] build_packages/runtime_packages are merged into task packages — the new message text correctly names [tasks.<name>] packages and [stack] build_packages/runtime_packages as the remedy. This also resolves the one open review thread on this PR, which had flagged an earlier revision for pointing at [session] packages (the wrong, session-level target). That thread's concern no longer applies to the current diff.
No issues found. Nothing here needed a build to check.
What ran
3 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. Read hydrate_task_defaults to check the new warning's target advice — self-reported
crates/mfile/src/lib.rs:1144-1166 confirms defaults.profile is copied onto every task lacking its own profile (hydrate_task_defaults), and stack.build_packages/runtime_packages are merged into task.packages. The new [defaults]profile warning text ('move its packages to each task's [tasks.] packages (or [stack] build_packages / runtime_packages, which apply to every task)') matches this behavior exactly.
2. Compared new warning text against the prior review thread on this PR — self-reported
threads.json records a HIGH-confidence prior finding that an earlier revision of this same warning pointed users at '[session] packages', which is wrong because [defaults]profile is per-task, not session-level, and session.packages is not read by the task-execution path. The current diff no longer mentions [session] packages at all and instead names [tasks.] packages / [stack] build_packages/runtime_packages, which is the correct remedy. The prior finding is resolved by this revision.
3. Verified field names referenced in the new message text exist — self-reported
Stack::build_packages and Stack::runtime_packages (crates/mfile/src/lib.rs:399-411) and Session::packages (crates/mfile/src/lib.rs:462-466) exist as written; no typos or stale field names in the new warning strings.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head f8f5cc51cab6 · verdict COMMENT · 0 finding(s), 0 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABNvbC-w
Profiles were removed in 0.5.1, but
mfilestill warns when a config declaresprofile =. The message named only the removal, not the remedy, so a config that relied on a profile for its packages would compose without them and fail later ascommand not found/ exit 127 — with nothing pointing at the cause. Each warning now names where the packages belong:[session] packagesfor[defaults]profile, and[tasks.<name>] packagesfor a per-task profile. Message text only; no behavior change. (The repo's own staleprofile =declarations and.minimal/profiles/files were already removed onmain, so only the warning text remained.)Verification
cargo fmt --all --check — clean
cargo clippy --workspace --locked -- -D warnings — passed, 0 warnings
cargo build --workspace --locked — passed
cargo test --workspace --locked — passed, 0 failed
Summary by CodeRabbit