Refuse empty folder paths and name Starr skip errors by instance key

[davidnewhall]

Summary

• Follow-up to Identify Starr, folder, and hook instances by map key #729: a folder env overlay with no path (UN_FOLDER_foo2_DELETE_ORIGINAL=true) used to filepath.Abs("") and watch the working directory. Startup and PUT now require a path; incomplete env leftovers are dropped so saving a different slug does not 400.
• Starr skip logs, fatal validation, and the duplicate-name warning include the instance map key (instance "0", instance "books") so a missing URL or API key is not just "one of your configurations".

Test plan

• go test ./pkg/folders ./pkg/unpackerr
• Start with UN_FOLDER_foo2_DELETE_ORIGINAL=true and no path: process refuses to start (folder "foo2": path is required).
• PUT /api/config/folders for another slug while that env is set: 200, foo2 is not written to the file.
• Missing Readarr URL logs instance "0" (or the real slug) instead of "one of your configurations".

Made with Cursor

[Copilot]

🟡 Changes recommended

Make duplicate-name warning ordering deterministic and add regression coverage for instance keys.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR enforces required folder paths and adds Starr instance keys to diagnostics.

Changes:

• Rejects empty folder paths and prevents watching the working directory.
• Drops incomplete environment-only folder overlays during PUT.
• Includes instance keys in Starr validation and duplicate-name diagnostics.

File summaries

File	Description
pkg/unpackerr/start_test.go	Startup folder validation tests
pkg/unpackerr/starrpoll.go	Starr instance-key diagnostics
pkg/unpackerr/folder.go	Folder path validation
pkg/unpackerr/configput.go	Environment overlay filtering
pkg/unpackerr/configput_test.go	PUT behavior tests
pkg/unpackerr/cnfgfile.go	Starr error logging with instance keys
pkg/unpackerr/cnfgfile_test.go	Starr diagnostic tests
pkg/folders/validate.go	Required path enforcement
pkg/folders/path.go	Empty-path watcher protection
pkg/folders/folder_test.go	Folder path tests

Review details

Suppressed comments (1)

pkg/unpackerr/starrpoll.go:66

• The duplicate-name diagnostic is part of this change, but there is no regression test exercising warnDuplicateStarrNames with slugged map entries. Add a case with two same-name instances (for example keys 0 and books) and assert the warning contains both instance keys, so this user-facing requirement cannot regress unnoticed.

		dupKey := strings.ToLower(name)
		loc := fmt.Sprintf("%s instance %q (%s)", app, slug, cfg.URL)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[qwen-pr-bot]

Approved. Reviewed 4154dbe against 50309c4 (main). Three distinct changes: (1) folders.Check no longer filepath.Abs("")-resolves a pathless folder onto the working directory — that was the real data-loss hazard, a delete_original overlay would have watched CWD; (2) folders now require a path — startup refuses with folder "<slug>": path is required and PUT 400s a body row while dropInvalidEnvOverlay keeps an env-only leftover from 400-ing saves of other slugs; (3) Starr skip logs, fatal validation errors, and the duplicate-name warning name the instance key instead of "one of your configurations".

Executed validation (head 4154dbe): go build ./... and go test ./... green; the new/changed tests pass individually; go test -race ./pkg/folders ./pkg/unpackerr and go vet clean; GitHub CI on the commit is green. Ran the built binary through the test-plan items:

• env-only UN_FOLDER_foo2_DELETE_ORIGINAL=true, no path → exit 1, folder "foo2": path is required.
• config file provides foo2's path plus that env flag; PUT /api/config/folders with only tv → 200, file and /live contain only tv, foo2 written nowhere. One consequence worth knowing: the pre-existing post-PUT self-restart then hits the new startup refusal and exits, so a container with that orphaned env var crash-loops until the var gets a path or is removed. The error names the folder and this is the documented trade-off, so I read it as intended rather than a defect.
• UN_READARR_books_API_KEY without a URL → Missing Readarr URL in instance "books", skipped and ignored. and the process keeps running; a bad instance name is fatal as Readarr instance "movies": ….

One non-blocking doc gap: the Folders PUT paragraph in INTERNALS.md (line 224) doesn't mention the new contract, while the adjacent Starr PUT and Webhooks / cmdhooks PUT paragraphs both document the identical drop-from-live behavior. Suggested sentence to append to the Folders PUT paragraph: "Instance rows require path (empty or whitespace-only is rejected): a PUT-body row 400s, an env-only overlay slug is dropped from live rather than 400, and startup refuses with folder "<slug>": path is required."