feat: remove profiles support - #1183
Conversation
📝 WalkthroughWalkthroughProfile support was removed from decoding, graph construction, checks, remote requests, standard-library declarations, configuration warnings, and documentation. Package and stack checks remain supported. Legacy wire profile records are ignored. ChangesProfile data and graph processing
Check and transport scope
Compatibility and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/session_sop.rs (1)
250-273: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
--profilesfrom parser text.The method documentation and the unknown-flag error still list
--profiles. The parser now rejects that flag. Update both messages to list only supported flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/session_sop.rs` around lines 250 - 273, Update the session check method’s documentation and the unknown-flag error in the argument parser to remove `--profiles`; list only the currently supported `--packages`, `--stacks`, and `--fix` flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mctx/src/env.rs`:
- Around line 153-163: Update the argument parsing loop in env.rs to reject
unknown dash-prefixed tokens such as --profiles instead of adding them to
filter_names. Preserve name filtering for non-flag tokens and keep the existing
check_all behavior; alternatively reuse CheckOpts::from_args so parsing matches
the session_sop transport.
In `@crates/mfile/src/lib.rs`:
- Around line 296-298: Update File::hydrate_task_defaults to stop copying
Defaults::profile into task defaults, while retaining the profile field for
deserialization compatibility. Remove stale profile references from the
hydration documentation and leave other task-default assignments unchanged.
In `@crates/stdlib/minimal-ncl/minimal.ncl`:
- Around line 200-204: Update the Layer documentation near the profiles field to
state that profiles are retained as legacy compatibility data, accepted from old
spec files, and ignored during ingestion; remove the implication that profiles
are ingested.
- Around line 195-197: Restore the active profile compatibility binding in the
minimal standard library, using the existing identity function signature and
behavior. Leave the internal 'Profile type tag unchanged, then run the loader
test through the repository’s just recipe to verify the imported profile calls
resolve.
---
Outside diff comments:
In `@crates/minimald/src/session_sop.rs`:
- Around line 250-273: Update the session check method’s documentation and the
unknown-flag error in the argument parser to remove `--profiles`; list only the
currently supported `--packages`, `--stacks`, and `--fix` flags.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 084b9587-6255-4ab2-8486-7f139e202cb3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
crates/check/src/lib.rscrates/check/src/profile.rscrates/decode/src/lib.rscrates/decode/src/profiles.rscrates/graph/src/graph.rscrates/graph/src/loader.rscrates/graph/src/wire.rscrates/mctx/src/env.rscrates/mfile/src/lib.rscrates/mfile/src/tasks.rscrates/minimald/src/session_sop.rscrates/minimald/src/sessions.rscrates/mip/src/cmd_check.rscrates/remote-client/src/lib.rscrates/remote-proto/build.rscrates/remote-proto/protos/res/check.protocrates/stdlib/Cargo.tomlcrates/stdlib/minimal-ncl/minimal.ncldocs/concepts/profiles.mddocs/reference/cli-mip.mddocs/reference/minimal-dot-toml.mddocs/reference/sandbox-operations.mddocs/reference/tasks.md
💤 Files with no reviewable changes (6)
- crates/decode/src/profiles.rs
- crates/check/src/profile.rs
- docs/concepts/profiles.md
- crates/minimald/src/sessions.rs
- crates/mip/src/cmd_check.rs
- crates/remote-client/src/lib.rs
| /// Removed feature, emits warning if set. | ||
| #[serde(default)] | ||
| pub profile: Option<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop applying the removed default profile.
Defaults::profile is documented as removed, but File::hydrate_task_defaults still copies it into every task. This preserves the removed default-profile inheritance and causes downstream task hydration to warn for each task. Keep the field for deserialization compatibility, but remove the assignment and the stale profile references from the hydration documentation.
Proposed fix
- if let Some(default_profile) = &self.defaults.profile
- && task.profile.is_none()
- {
- task.profile = Some(default_profile.clone());
- }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mfile/src/lib.rs` around lines 296 - 298, Update
File::hydrate_task_defaults to stop copying Defaults::profile into task
defaults, while retaining the profile field for deserialization compatibility.
Remove stale profile references from the hydration documentation and leave other
task-default assignments unchanged.
| # # Profiles were removed 2026-08-06, left to avoid breaking old spec-files | ||
| # profile | Dyn -> Dyn = | ||
| # fun spec => spec, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n crates/stdlib/minimal-ncl/minimal.ncl | sed -n '170,215p'
printf '%s\n' '--- references to profile/Profile and target file ---'
rg -n --glob '!target/**' --glob '!node_modules/**' '\bprofile\b|\bProfile\b|minimal-ncl|minimal\.ncl' .
printf '%s\n' '--- available just recipes ---'
just --list 2>&1 || trueRepository: gominimal/minimal
Length of output: 34427
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- profile fixture and nearby test ---'
cat -n crates/decode/src/load.rs | sed -n '585,640p'
printf '%s\n' '--- profile-related Nickel files ---'
git ls-files | rg '(^|/)(profile\.ncl|.*profile.*\.ncl)$' || true
printf '%s\n' '--- exact Nickel profile type-annotation references ---'
rg -n --glob '*.ncl' '\|\s*Profile\b|\bProfile\b|\bprofile\s*\{' .
printf '%s\n' '--- justfile test recipes ---'
cat -n justfile | sed -n '1,80p;240,310p'
printf '%s\n' '--- decode test declarations around the fixture ---'
rg -n -C 8 'Make a profile called rust|profile_dir\.join|profile \{' crates/decode/src/load.rsRepository: gominimal/minimal
Length of output: 12408
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remainder of loader test ---'
cat -n crates/decode/src/load.rs | sed -n '626,705p'
printf '%s\n' '--- checked-in profile specs ---'
for f in .minimal/profiles/demo/profile.ncl .minimal/profiles/dev/profile.ncl; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
cat -n "$f"
fi
done
printf '%s\n' '--- read-only compatibility verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
stdlib = Path("crates/stdlib/minimal-ncl/minimal.ncl").read_text()
active_profile_binding = re.search(
r"(?m)^\s*profile\s*\|\s*Dyn\s*->\s*Dyn\s*=",
stdlib,
) is not None
commented_profile_binding = re.search(
r"(?m)^\s*#\s*profile\s*\|\s*Dyn\s*->\s*Dyn\s*=",
stdlib,
) is not None
profile_calls = []
profile_type_annotations = []
for path in Path(".").rglob("*.ncl"):
text = path.read_text(errors="replace")
if re.search(r'\blet\s*\{[^}]*\bprofile\b[^}]*\}\s*=\s*import\s+"minimal\.ncl"', text):
profile_calls.append(str(path))
if re.search(r"\|\s*Profile\b", text):
profile_type_annotations.append(str(path))
print(f"active profile binding: {active_profile_binding}")
print(f"commented profile binding: {commented_profile_binding}")
print("legacy profile-import fixtures:")
for path in profile_calls:
print(f" {path}")
print("Nickel Profile type annotations:")
for path in profile_type_annotations:
print(f" {path}")
PYRepository: gominimal/minimal
Length of output: 3591
Restore the active profile compatibility binding.
.minimal/profiles/demo/profile.ncl, .minimal/profiles/dev/profile.ncl, and crates/decode/src/load.rs import and call profile. The commented binding causes these specs to fail with an unknown identifier. No Nickel spec uses Profile as a type annotation; 'Profile in Tys is an internal type tag. Run the loader test through the repository’s just recipe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/stdlib/minimal-ncl/minimal.ncl` around lines 195 - 197, Restore the
active profile compatibility binding in the minimal standard library, using the
existing identity function signature and behavior. Leave the internal 'Profile
type tag unchanged, then run the loader test through the repository’s just
recipe to verify the imported profile calls resolve.
Source: Coding guidelines
| Layer | doc "Implementation detail: the set of build-specs and profiles to be ingested." = { | ||
| ty | Tys = 'Layer, | ||
|
|
||
| builds | Array BuildSpec, | ||
| profiles | Array Profile, | ||
| profiles | Array Dyn, # Profiles were removed 2026-08-06. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document retained profiles as ignored compatibility data.
The Layer documentation still describes profiles as data “to be ingested”. This change retains profiles as Array Dyn for old spec-file compatibility, while profile ingestion was removed. State that legacy profile records are accepted and ignored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/stdlib/minimal-ncl/minimal.ncl` around lines 200 - 204, Update the
Layer documentation near the profiles field to state that profiles are retained
as legacy compatibility data, accepted from old spec files, and ignored during
ingestion; remove the implication that profiles are ingested.
e2de3fe to
e89d8bf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Yeets profiles into the sun once and for all.
Note
Remove profiles support from check pipeline, CLI, wire format, and decode/graph layers
--profilesCLI flag frommipandminimald, removes theprofilesfield fromCheckRequestproto (field 6 reserved), and stops emitting profile frames in wire streams.profilesarray type toArray Dynfor compatibility with existing spec files that still declare profiles.mfilevalidation whendefaults.profileortask.profilefields are set, noting removal in 0.5.1.Macroscope summarized e89d8bf.
Summary by CodeRabbit
Breaking Changes
--profilesoption is no longer available.Documentation
Maintenance