Skip to content

fix(provision): resolve --list's target the way every other command does - #223

Open
mobileoverlord wants to merge 1 commit into
mainfrom
jschneck/provision-list-target-resolution
Open

fix(provision): resolve --list's target the way every other command does#223
mobileoverlord wants to merge 1 commit into
mainfrom
jschneck/provision-list-target-resolution

Conversation

@mobileoverlord

Copy link
Copy Markdown
Contributor

Problem

With AVOCADO_TARGET set to something other than the config's
default_target, avocado provision --list reports:

[INFO] Stone manifest not found in the SDK volume. The SDK install may not have completed for this target.

on a project that has just finished a clean avocado install && avocado build
for that target. The manifest is present; nothing about the install is
incomplete.

Root cause

ProfilesListCommand::execute resolved the target with its own chain — the
--target flag, then default_target — skipping AVOCADO_TARGET. Every other
command goes through utils::target::resolve_target (flag, env, default), and
so does the interpolation behind Config::load_composed: resolve_target_value
reads the env var when no flag is given.

So the command interpolated its config for the env target and then read the
manifest path for a different one, from the same function. It looked for
/opt/_avocado/<default_target>/sdk/*/stone/stone-<default_target>.json, found
nothing, and attributed the miss to an incomplete SDK install.

The comment above the chain claimed it resolved "the same way the rest of the
CLI does", which is what it stopped doing.

Change

Delegate to resolve_target. The message names the target and what sets one,
because a miss is as often a target mix-up as an incomplete install, and the
human path prints only the reason string — bail_unavailable already carries a
structured target field, but only under --output json.

Scope is this command: runtime provision and the rest already use
resolve_target_required, and a sweep for other direct default_target reads
in src/commands turns up only init (which writes it) and test fixtures.

Verification

cargo fmt --check, cargo clippy --all-targets -- -D warnings clean;
cargo test 3657 passed, 0 failed.

Four tests, over the command's own resolve_target rather than the free
function it delegates to — testing the utility directly would have passed
against the bug:

mutation caught by
restore the flag-then-default_target chain env-beats-default test
message names no target reason-text test

Also covered: an explicit --target still beats the env var, and
default_target still applies when neither is set.

… does

## Problem

With `AVOCADO_TARGET` set to something other than the config's
`default_target`, `avocado provision --list` reports:

```
[INFO] Stone manifest not found in the SDK volume. The SDK install may not have completed for this target.
```

on a project that has just finished a clean `avocado install && avocado build`
for that target. The manifest is present; nothing about the install is
incomplete.

## Root cause

`ProfilesListCommand::execute` resolved the target with its own chain — the
`--target` flag, then `default_target` — skipping `AVOCADO_TARGET`. Every other
command goes through `utils::target::resolve_target` (flag, env, default), and
so does the interpolation behind `Config::load_composed`: `resolve_target_value`
reads the env var when no flag is given.

So the command interpolated its config for the env target and then read the
manifest path for a *different* one, from the same function. It looked for
`/opt/_avocado/<default_target>/sdk/*/stone/stone-<default_target>.json`, found
nothing, and attributed the miss to an incomplete SDK install.

The comment above the chain claimed it resolved "the same way the rest of the
CLI does", which is what it stopped doing.

## Change

Delegate to `resolve_target`. The message names the target and what sets one,
because a miss is as often a target mix-up as an incomplete install, and the
human path prints only the reason string — `bail_unavailable` already carries a
structured `target` field, but only under `--output json`.

Scope is this command: `runtime provision` and the rest already use
`resolve_target_required`, and a sweep for other direct `default_target` reads
in `src/commands` turns up only `init` (which writes it) and test fixtures.

## Verification

`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` clean;
`cargo test` 3657 passed, 0 failed.

Four tests, over the command's own `resolve_target` rather than the free
function it delegates to — testing the utility directly would have passed
against the bug:

| mutation | caught by |
|---|---|
| restore the flag-then-`default_target` chain | env-beats-default test |
| message names no target | reason-text test |

Also covered: an explicit `--target` still beats the env var, and
`default_target` still applies when neither is set.
Copilot AI lite review requested due to automatic review settings August 27, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes avocado provision --list / avocado profiles list target resolution to honor AVOCADO_TARGET (matching the rest of the CLI and config interpolation), preventing false “SDK install incomplete” reports when the manifest exists under the env-selected target.

Changes:

  • Delegate target selection in ProfilesListCommand::execute to utils::target::resolve_target (CLI flag > env > config default).
  • Improve the “missing stone manifest” human-facing reason text to include the resolved target and where it came from.
  • Add regression tests covering env-vs-default and flag-vs-env precedence plus reason-text expectations.
Suppressed comments (1)

src/commands/profiles.rs:195

  • With the method and free function both named resolve_target, this call site is ambiguous at a glance. Use the aliased import name here for clarity.
    fn resolve_target(&self, config: &Config) -> Option<String> {
        resolve_target(self.target.as_deref(), config)
    }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/commands/profiles.rs
use crate::utils::config::Config;
use crate::utils::output::{print_info, OutputLevel};
use crate::utils::output_format::{emit_json_object, OutputFormat};
use crate::utils::target::resolve_target;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0cc0575: imported as resolve_config_target; the method body and its doc name it.

Comment thread src/commands/profiles.rs
Comment on lines +434 to +438
fn default_target_still_applies_when_nothing_overrides_it() {
let config = Config::load_from_str("default_target: imx8mp-evk\n").unwrap();
std::env::remove_var("AVOCADO_TARGET");
assert_eq!(resolve_target(None, &config).as_deref(), Some("imx8mp-evk"));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0cc0575: the test calls cmd(None).resolve_target(&config) like its siblings.

Comment thread src/commands/profiles.rs
/// helpful prose message in human mode. Always exits Ok so the
/// desktop app can distinguish "no profiles yet" from "command
/// failed" via the JSON shape rather than process exit code.
/// The target whose stone manifest `--list` reads.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This method landed between bail_unavailable's doc comment and bail_unavailable itself, so that comment now documents resolve_target. In the generated rustdoc ProfilesListCommand::resolve_target opens with the four lines about the JSON envelope and the always-Ok exit code, and bail_unavailable is left with no documentation.

Moving the method below bail_unavailable restores both. Non-blocking, take it or leave it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 0cc0575: method moved below bail_unavailable, whose doc comment is back where it belongs.

@mobileoverlord

Copy link
Copy Markdown
Contributor Author

Note: the fixes referenced in the thread replies are written and tested locally but not pushed yet - the commit-signing agent stopped answering mid-batch. The replies will be followed up with the real commit SHA once it lands; until then the branch head is unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants