Identify Starr, folder, and hook instances by map key

[davidnewhall]

Summary

• Starr apps, folders, webhooks, and command hooks are identified by a TOML/JSON map key instead of slice index, so deleting one instance cannot renumber the rest or steal UN_SONARR_0_*.
• Dual-read legacy [[sonarr]] arrays as keys "0", "1", …. PUT writes maps. File commit strips envUsed fields and drops env-only slugs; live ParseENV overlay stays. Live GET includes env keys and redacts env secrets.

Test plan

• go test ./pkg/unpackerr/ ./pkg/configdef/
• Load an existing [[sonarr]] file plus UN_SONARR_0_URL: instance key is 0, env still overlays.
• PUT /api/config/readarr with {} while UN_READARR_0_* is set: file has no Readarr, live still has the env instance, TOML does not gain the env URL or API key.
• Live GET blanks env apiKey / passwords; file GET does not include env-only slugs.

Made with Cursor

[Copilot]

🟡 Changes recommended

Startup overlays can discard file fields, generated example TOML is invalid, and the OpenAPI unions reject valid payloads.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Migrates configurable instances from indexed slices to stable, slug-keyed maps while retaining legacy array input.

Changes:

• Adds map serialization, cloning, validation, environment overlay, and secret-redaction support.
• Updates runtime polling, hooks, folders, config APIs, and tests.
• Updates configuration generation, OpenAPI, examples, and documentation.

File summaries

File	Description
pkg/unpackerr/webhook.go	Adapts hook processing to maps.
pkg/unpackerr/start_test.go	Updates folder validation tests.
pkg/unpackerr/starrpoll.go	Adapts Starr polling to maps.
pkg/unpackerr/starrpoll_test.go	Updates polling tests.
pkg/unpackerr/queue_actions_test.go	Updates queue-action fixtures.
pkg/unpackerr/openapi.json	Documents map-based APIs.
pkg/unpackerr/instancemap.go	Implements legacy-compatible instance maps.
pkg/unpackerr/instancemap_test.go	Tests map decoding and slugs.
pkg/unpackerr/folder.go	Adapts folder validation and polling.
pkg/unpackerr/envstrip.go	Handles environment-owned map fields.
pkg/unpackerr/configput.go	Applies map-based configuration PUTs.
pkg/unpackerr/configput_test.go	Tests persistence and environment overlays.
pkg/unpackerr/configdump.go	Renders mapped runtime configuration.
pkg/unpackerr/configdump_test.go	Updates configuration dump tests.
pkg/unpackerr/configclone.go	Adds deep map cloning.
pkg/unpackerr/configapi.go	Returns map-shaped API payloads.
pkg/unpackerr/configapi_test.go	Updates API fixtures.
pkg/unpackerr/cnfgfile_test.go	Updates configuration round-trip tests.
pkg/unpackerr/apps.go	Changes live instance fields to maps.
pkg/unpackerr/api_test.go	Updates API hook fixtures.
pkg/configdef/validate.go	Validates the named section kind.
pkg/configdef/start.go	Defines named section metadata.
pkg/configdef/live.go	Renders named TOML tables.
pkg/configdef/live_test.go	Tests named-table rendering.
pkg/configdef/help.go	Generates indexed environment examples.
pkg/configdef/docusaurus.go	Documents named headers.
pkg/configdef/definitions.yml	Converts repeatable sections to named maps.
pkg/configdef/config.go	Generates named TOML headers.
pkg/configdef/compose.go	Handles named environment settings.
INTERNALS.md	Documents map lifecycle and locking.
examples/unpackerr.conf.example	Updates generated TOML examples.
examples/docker-compose.yml	Refreshes generated metadata.
.github/copilot-instructions.md	Updates map terminology.

Review details

Suppressed comments (1)

pkg/unpackerr/openapi.json:319

• The PUT union has the same overlapping-schema problem: a valid map matches the generic object branch and often both instance-map branches, while a Starr array can also match the hook-array branch. Under OpenAPI 3.0 oneOf requires exactly one match, so validators and generated clients reject valid requests. Use anyOf or make the alternatives mutually exclusive.

        "oneOf": [

• Files reviewed: 33/33 changed files
• Comments generated: 5
• Review effort level: Balanced

---

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

[qwen-pr-bot]

The delta does what it claims: I verified the startup merge keeps file-only fields while applying the UN_* overlay (the new sonarr test reproduces it — env URL live, file api_key/name/paths live, env URL stays out of the written file), and the example TOML now parses with live [webserver]/[folders] headers. One thing still blocks: the peel-direction note on envstrip.go:277 is unchanged, and with the new startup merge its consequence is now live behavior — see inline.

Executed validation: go build ./... and go test -count=1 ./... (all packages pass at fa8f6e4), plus a throwaway in-package harness running unmarshalConfig and stripEnvFromFolders with UN_FOLDER_watch_EXTRACT_PATH / UN_FOLDER_watch_DELETE_AFTER, at both c92b3a6 and fa8f6e4.

[qwen-pr-bot]

The peel-direction fix is exactly what the earlier comment asked for — longest-first now agrees with cnfg's map-mode resolution, and the new tests settle the extract_path scenario. The merge of main is mechanical and holds up. One blocking residual in the same layer: envstrip derives its field set from toml tags while cnfg reads xml tags, and those drift on folders' exclude_paths, so the documented EXCLUDE_PATH_ env var no longer sticks to live (and can take a sibling file entry with it) — details and repro in the inline comments.

Executed validation at 0ef0da3: go build ./..., go test -count=1 ./... (all packages pass), go test -count=1 -race ./pkg/unpackerr/ (pass), plus throwaway in-package harnesses driving the real startup and putFolders flows with both EXCLUDE_PATH spellings set, an instrumented putFolders to locate each mutation, and direct cnfg.ParseENV checks of res.Used for each spelling. All harness files were deleted; no repository code was modified for this review.

[Copilot]

🟡 Changes recommended

Environment stripping can erase valid file-owned settings, and the new OpenAPI unions reject supported payloads.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

pkg/unpackerr/envstrip.go:60

• Using Path as the retention test drops every file-owned folder option when only the required path comes from UN_FOLDER_<slug>_PATH. A PUT can therefore erase extract_path, deletion settings, limits, and exclusions from disk, so they disappear after restart. Retain partial entries that still contain persistable non-env configuration instead of treating every env-owned path as an env-only slug.

		if item == nil || strings.TrimSpace(item.Path) == "" {
			delete(items, key)

pkg/unpackerr/envstrip.go:87

• When Command is supplied by UN_CMDHOOK_<slug>_COMMAND, this deletes the whole file entry after stripping that field. File-owned events, excludes, timeout, and other options are then lost on the next restart. Drop only truly env-only entries, not every partial entry whose required command is env-owned.

		if cmd && strings.TrimSpace(item.Command) == "" {
			delete(items, key)

pkg/unpackerr/envstrip.go:92

• An env-owned webhook URL causes this to discard all other file-owned hook fields, including a file token, events, template, and exclusions. The live preview works until restart, but the persisted data is gone. Retain meaningful non-env fields and only remove entries that are actually env-only.

		if !cmd && strings.TrimSpace(item.URL) == "" {
			delete(items, key)

pkg/unpackerr/openapi.json:320

• oneOf makes normal map responses fail schema validation: a Starr map also matches HookMap (neither instance schema forbids extra properties) and the unconstrained generic object branch. Since multiple alternatives match, generated validators reject the payload. Use anyOf here, or make the alternatives mutually exclusive.
  pkg/unpackerr/openapi.json:329
• The PUT union has the same overlapping-schema problem: Starr and hook arrays/maps can match each other's permissive item schemas, and object forms also match the generic object branch, violating oneOf's exactly-one requirement. Use anyOf or add discriminating constraints.
  pkg/unpackerr/openapi.json:332
• The description promises legacy folder arrays, and InstanceMap.UnmarshalJSON accepts them inside the folders wrapper, but this branch references FoldersSection, whose folder property only permits FolderMap. OpenAPI clients therefore cannot send the supported legacy {..., "folder": [...]} shape. Add a PUT-specific folders schema whose folder is a map-or-array union.

• Files reviewed: 36/37 changed files
• Comments generated: 1
• Review effort level: Balanced

[qwen-pr-bot]

The peel now agrees with cnfg, which is the right fix — but PeelMapKey drops the slice index, so this zeros the whole exclude_paths out of the file commit when only one index was env-set. I reproduced it: file exclude_paths = ["/a", "/b"] + UN_FOLDER_watch_EXCLUDE_PATH_0=/c, then an interval-only folders PUT rewrites the file with no exclude_paths, and /b is gone for good after the next restart (live was already just [/c] before the PUT). For a slice field whose env var carries an index, skip the whole-field zero here (or zero only the specified index) — the overlay already makes env win live, and the file would keep the indices env never touched.

[qwen-pr-bot]

The tag-drift item is resolved — delegating to cnfg.PeelMapKey with ENVTag is the right call, and the corrected test spellings (EXCLUDE_PATH_0, unindexed PATHS) match what cnfg actually resolves. One remaining blocker, inline on envstrip.go: an indexed env var on a slice field wipes the entire field from the file on a routine PUT, losing sibling indices the env var never specified. The cnfg pseudo-version bump is a pure export refactor (verified by diffing the module zips; no ParseENV behavior change).

Executed validation at 057e5eb: go build ./...; go test -count=1 ./... (all packages pass); go test -count=1 -race ./pkg/unpackerr/ ./pkg/configdef/ (pass); plus throwaway in-package harnesses — direct cnfg.PeelMapKey calls against SonarrConfig/FolderConfig, and the full putFolders sequence (strip → env overlay → file write → restart) with the indexed exclude_path env var. All harness files deleted; no repository code modified.

[davidnewhall]

This sucks. Wrong fix.

[davidnewhall]

Replaced by #729. PUT writes the request body as the file document; live is clone(fileConfig)+ParseENV. No peel of env-owned fields.