Deprecate reading Edge App id from manifest, prefer EDGE_APP_ID env var

[nicomiguelino]

Summary

• get_app_id now reads the EDGE_APP_ID environment variable first (trimmed; whitespace-only is treated as unset) and uses it when present, taking priority over screenly.yml.
• If EDGE_APP_ID isn't set, behavior is unchanged: the id is read from the manifest's id field, but the CLI now prints a deprecation warning each time it falls back to the manifest, pointing at EDGE_APP_ID.
• This is a single choke point (EdgeAppCommand::get_app_id), so the change and warning apply consistently across deploy, delete, rename, instance list/create, etc.
• Docs updated in docs/EdgeApps.md under the manifest id reference to describe the new precedence and deprecation.

Not covered in this PR

• edge-app create / edge-app create --in-place still decide whether to create a new app purely based on whether screenly.yml already has an id; they don't yet consult EDGE_APP_ID.
• edge-app delete clears the manifest's id via clear_app_id, but doesn't touch or warn about a stale EDGE_APP_ID left set in the environment afterward.

Test plan

• cargo test, all tests pass except a pre-existing, unrelated local failure (authentication::tests::test_read_token_correct_token_is_returned, which reads the machine's real ~/.screenly.d token file)
• cargo clippy --all-targets -- -D warnings, clean
• cargo fmt --check, clean
• Added unit tests: env var takes priority over manifest, falls back to manifest when unset, trims whitespace, and treats a whitespace-only env var as unset

[Copilot]

Pull request overview

This PR updates the Edge Apps CLI to prefer an EDGE_APP_ID environment variable over reading the app id from screenly.yml, while warning that manifest-based IDs are deprecated.

Changes:

• Add EDGE_APP_ID env var precedence in EdgeAppCommand::get_app_id (trimmed; whitespace-only treated as unset).
• Emit a deprecation warning when falling back to the manifest id.
• Add unit tests for env var precedence/whitespace handling and update docs to describe the new behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/commands/edge_app/app.rs	Reads app id from EDGE_APP_ID first; warns on manifest fallback; adds tests for precedence and trimming.
docs/EdgeApps.md	Documents EDGE_APP_ID precedence and deprecation of reading id from screenly.yml.

Suppressed comments (3)

src/commands/edge_app/app.rs:1713

• This test removes EDGE_APP_ID without restoring its previous value, and does so without synchronization; that can leak state into other tests and cause flakiness under parallel execution. Capture and restore the previous value under a test lock.

        env::remove_var(EDGE_APP_ID_ENV);
        let app_id = command.get_app_id(Some(temp_dir.path().to_str().unwrap().to_string()));

src/commands/edge_app/app.rs:1725

• These lines set/remove a global environment variable without synchronization or RAII restoration, which can make the suite flaky under parallel test execution. Prefer envtestkit::lock::lock_test() + envtestkit::set_env(...) to isolate the change and restore the previous value automatically.

        env::set_var(EDGE_APP_ID_ENV, "  01ENVOVERRIDEXXXXXXXXXXXXX  \n");
        let app_id = command.get_app_id(Some(temp_dir.path().to_str().unwrap().to_string()));
        env::remove_var(EDGE_APP_ID_ENV);

src/commands/edge_app/app.rs:1737

• These lines set/remove a global environment variable without synchronization or restoration, which can race with other tests (including the other new EDGE_APP_ID tests) when run in parallel. Use envtestkit's test lock and RAII env setter to avoid cross-test interference.

        env::set_var(EDGE_APP_ID_ENV, "   ");
        let app_id = command.get_app_id(Some(temp_dir.path().to_str().unwrap().to_string()));
        env::remove_var(EDGE_APP_ID_ENV);

---

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

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[sergey-borovkov]

Two blocking issues where the env var wins over an id the caller already stated, plus some smaller ones.

Blocking

1. MCP publish_from_html can deploy to the wrong app

src/mcp/tools/edge_app.rs:271-286 writes a temp screenly.yml whose id is the explicit (or remembered) app id, reads app_id back from it, then calls command.deploy(...). deploy re-resolves through get_app_id, which now prefers EDGE_APP_ID.

If the MCP server process has EDGE_APP_ID in its environment (a Claude Desktop env block, or inherited from the launching shell), the generated HTML is published as a new revision of the env app — silently overwriting an unrelated app — while the tool reports the intended app_id as success.

An id passed explicitly by the caller should take precedence over the env var.

2. edge-app delete strips the id of an app it never deleted

src/cli.rs:955 resolves the id (now the env var) and deletes that app, then src/cli.rs:990 unconditionally runs clear_app_id(manifest_path) and prints "App id cleared from manifest."

With EDGE_APP_ID=A exported and ./myapp/screenly.yml containing id: B:

screenly edge-app delete --path ./myapp

deletes app A and wipes id: B from the manifest. App B still exists on the platform, but the directory no longer links to it. clear_app_id should only run when the id actually came from the manifest.

Non-blocking

The new tests make the rest of the suite flaky. set_env mutates the process-wide environment, and envtestkit::lock::lock_test() only serializes tests that also call it. test_deploy_without_app_id_should_fail (app.rs:1784, asserts MissingAppId), test_deploy_should_send_correct_requests (app.rs:1037, httpmock paths keyed on the manifest id), and the setting/instance tests all reach get_app_id without taking that lock, so in parallel with the four new tests they observe EDGE_APP_ID=01ENVOVERRIDEXXXXXXXXXXXXX and fail with unmatched mocks or an unexpected Ok. Compounding it: temp_env (app.rs:2072, utils.rs:1066+) uses its own global lock, entirely separate from envtestkit's, so the two crates don't serialize against each other.

create / create_in_place ignore EDGE_APP_ID, producing an orphaned app. With the env var set, edge-app create --in-place still calls api.create_app, writes the new id into screenly.yml (app.rs:196-199) and prints success — after which every deploy / setting / instance in that directory targets the env app instead. The new app is live, billed, and unreachable from where it was created. create_in_place also still gates on manifest.id.is_some() (app.rs:187) without consulting the env var. The description lists this as out of scope, but it's a user-visible wrong outcome rather than just a missing feature.

--path is no longer validated when the env var is set. The early return in get_app_id precedes transform_edge_app_path_to_manifest(&path)?, so edge-app rename --path /does/not/exist --name x (cli.rs:1008), edge-app instance list --path <wrong dir> (cli.rs:1133) and edge-app setting list (setting.rs:11) now succeed against the env app instead of erroring on the bad path. A typo'd --path silently operates on a different app.

test_..._when_env_var_is_not_set never exercises the unset branch. set_env(EDGE_APP_ID_ENV, "") sets the variable to an empty string, so std::env::var returns Ok("") — the same path as the whitespace-only test three cases below. The genuinely-absent path stays untested; temp_env::with_var_unset (already a dev-dependency, used at src/mcp/tools/edge_app.rs:905) covers it.

The deprecation warning fires twice per deploy. deploy calls get_app_id at app.rs:217, then update_entrypoint_value at app.rs:270 -> set_setting (setting.rs:26) -> get_app_id again, so remote-entrypoint apps print the banner twice on every deploy.

[Copilot]

🟡 Changes recommended

The MCP edge-app tool path currently prioritizes the manifest id over EDGE_APP_ID, which can deploy to the wrong app when both are set.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[sergey-borovkov]

Thanks for the quick turnaround on 4fb0538 — I re-reviewed against it rather than the original head.

Confirmed fixed

• delete now guards clear_app_id on app_id_override().is_none(), so a manifest id is no longer wiped for an app that was never deleted.
• --path is validated before the env-var early return.
• The deprecation warning is wrapped in a Once, so it fires once per process.
• The set_env tests are gone, replaced by pure normalize_app_id_override unit tests — including the genuinely-absent case. No more cross-test env leakage; the suite is green and stable locally (233 passed).
• deploy takes an explicit app_id and the MCP tool threads it through.

Blocking: the version payload still loses the app id

This one is on me — I missed it in the first pass, and it breaks the flow this PR is built to enable.

create_version builds its body from EdgeAppManifest::prepare_payload, which sources app_id from manifest.id:

("app_id", &manifest.id),
...
.filter_map(|(key, value)| value.as_ref().map(|v| (*key, json!(v))))

The filter_map drops the key entirely when manifest.id is None. So as soon as a user does what the deprecation notice tells them to — remove id: from screenly.yml, set EDGE_APP_ID — the POST /v4/edge-apps/versions goes out with no app_id at all. Every other call in deploy correctly uses the resolved actual_app_id; this is the one that doesn't.

4fb0538 widens the blast radius: create now writes id: None into the manifest when EDGE_APP_ID is set, so every app created that way hits this on its first deploy.

It isn't caught today because test_deploy_should_send_correct_requests uses a manifest that still has an id.

Fix that worked for me — pass the id deploy has already resolved:

         let final_revision = if needs_new_version {
-            let revision =
-                self.create_version(&manifest, generate_file_tree(&local_files, edge_app_dir))?;
+            let revision = self.create_version(
+                &actual_app_id,
+                &manifest,
+                generate_file_tree(&local_files, edge_app_dir),
+            )?;

     fn create_version(
         &self,
+        app_id: &str,
         manifest: &EdgeAppManifest,
         file_tree: HashMap<String, String>,
     ) -> Result<u32, CommandError> {
         let mut json = EdgeAppManifest::prepare_payload(manifest);
+        json.insert("app_id", json!(app_id));
         json.insert("file_tree", json!(file_tree));

Worth a regression test with an id-less manifest asserting the payload still carries the resolved id — I verified such a test fails without the change above and passes with it.

Non-blocking, your call

update_entrypoint_value ignores deploy's explicit id. deploy -> update_entrypoint_value(path) -> set_setting(path, ...) -> get_app_id(path) re-resolves from the env var, so the explicit app_id parameter is bypassed on that branch. Unreachable right now, since the MCP tool writes EntrypointType::File and update_entrypoint_value skips that arm — but it's a trap for the next caller that passes an explicit id for a remote-entrypoint app.

create / create_in_place now validate-and-reuse instead of creating. A reasonable answer to the orphaned-app problem, but worth confirming it's the semantics you want: with EDGE_APP_ID set, MCP publish_from_html called with a brand-new name no longer creates an app — it republishes over the env app and remembers that name -> id mapping. If that's intended, the deprecation docs should probably say so outright.

[Copilot]

🟡 Changes recommended

There is a confirmed precedence bug in src/mcp/tools/edge_app.rs that can cause deployments to target the wrong app when both manifest id and EDGE_APP_ID are present.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/mcp/tools/edge_app.rs:288

• app_id_override is applied after the manifest id here, which makes the manifest take precedence when both are present. Since this code then passes the chosen id into deploy(Some(app_id)), an exported EDGE_APP_ID will be ignored and the tool can deploy to the wrong app.

        let app_id = EdgeAppManifest::new(&manifest_path)
            .map_err(|e| format!("Failed to read Edge App id: {}", e))?
            .id
            .or_else(app_id_override)
            .ok_or_else(|| "Edge App id missing after create".to_string())?;

        let revision = command
            .deploy(Some(app_id.clone()), Some(path), Some(false))
            .map_err(|e| format!("Failed to deploy Edge App: {}", e))?;

docs/EdgeApps.md:297

• This says the CLI prints a deprecation warning "each time" it falls back to the manifest, but get_app_id uses a Once, so it will only warn once per process/CLI invocation. Either adjust the docs to match the implementation, or remove the Once if repeated warnings are desired.

> **Deprecated:** Reading the `id` from `screenly.yml` is deprecated. Set the `EDGE_APP_ID` environment variable instead — for example, via a repo secret in CI or a local `.env` file loaded into your shell. When `EDGE_APP_ID` is set, it takes priority over the manifest's `id` field. The CLI still falls back to reading `id` from `screenly.yml` for now, but prints a deprecation warning each time it does.

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[sergey-borovkov]

4486fa4 addresses everything from the last round. Checked out and verified locally:

• The version payload blocker is fixed. create_version takes the resolved app id and inserts it over whatever the manifest had, and deploy passes actual_app_id. The new end-to-end test with an id-less manifest is a real regression test — I confirmed it fails when the json.insert("app_id", ...) line is removed.
• update_entrypoint_value now honors the explicit id, threading it through set_setting. cli.rs and instance.rs pass None and keep resolving via get_app_id, which is the right behavior for those paths.
• The create / create_in_place semantics are documented, including that a "new" name republishes over the EDGE_APP_ID app instead of creating one. That was the part most likely to surprise someone, so spelling it out in the docs is the right call.

cargo test is green (234 passed) and cargo clippy --all-targets is clean.

One nit, not blocking: test_deploy_with_explicit_app_id_and_no_manifest_id_should_send_correct_requests is a ~340-line copy of test_deploy_should_send_correct_requests, differing only in manifest.id and the explicit id argument. Worth folding into a shared helper next time either one needs a mock updated, but not worth holding this up.

Nice work on the turnaround.