Identify Starr, folder, and hook instances by map key - #729
Conversation
… overlay the same slug after a PUT. PUT writes the request body as the file document. Live is clone(fileConfig)+ParseENV; there is no peel of env-owned fields. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
Legacy numeric instances are reordered and the new OpenAPI unions reject valid payloads.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Migrates configured instances from positional slices to stable, slug-keyed maps while retaining legacy array input compatibility.
Changes:
- Adds
InstanceMapJSON/TOML decoding and updates runtime consumers. - Preserves file/live environment-overlay behavior and redacts live secrets.
- Updates configuration generation, API schemas, documentation, dependencies, and tests.
File summaries
| File | Description |
|---|---|
pkg/unpackerr/webhook.go |
Uses keyed hook maps. |
pkg/unpackerr/start_test.go |
Updates folder tests. |
pkg/unpackerr/starrpoll.go |
Polls keyed Starr instances. |
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 instance maps. |
pkg/unpackerr/instancemap_test.go |
Tests map decoding. |
pkg/unpackerr/historyrestore_test.go |
Updates restoration fixtures. |
pkg/unpackerr/folder.go |
Validates keyed folders. |
pkg/unpackerr/configput.go |
Applies map-based PUTs. |
pkg/unpackerr/configput_test.go |
Tests PUT and overlays. |
pkg/unpackerr/configdump.go |
Dumps keyed instances. |
pkg/unpackerr/configdump_test.go |
Updates dump tests. |
pkg/unpackerr/configclone.go |
Deep-clones instance maps. |
pkg/unpackerr/configapi.go |
Returns maps and redacts secrets. |
pkg/unpackerr/configapi_test.go |
Updates config API tests. |
pkg/unpackerr/cnfgfile.go |
Clarifies overlay behavior. |
pkg/unpackerr/cnfgfile_test.go |
Tests map persistence. |
pkg/unpackerr/apps.go |
Changes configuration fields to maps. |
pkg/unpackerr/api_test.go |
Updates API fixtures. |
pkg/configdef/validate.go |
Validates named section kinds. |
pkg/configdef/start.go |
Defines named section kind. |
pkg/configdef/live.go |
Renders named TOML tables. |
pkg/configdef/live_test.go |
Tests named rendering. |
pkg/configdef/help.go |
Updates repeatable environment help. |
pkg/configdef/docusaurus.go |
Documents named headers. |
pkg/configdef/definitions.yml |
Defines named instance sections. |
pkg/configdef/config.go |
Generates named TOML headers. |
pkg/configdef/compose.go |
Handles named environment entries. |
INTERNALS.md |
Documents map architecture. |
go.sum |
Updates dependency checksums. |
go.mod |
Pins map-overlay cnfg. |
examples/unpackerr.conf.example |
Shows named TOML instances. |
examples/docker-compose.yml |
Refreshes generated output. |
.github/copilot-instructions.md |
Updates map terminology. |
Review details
Suppressed comments (1)
pkg/unpackerr/openapi.json:329
- The PUT alternatives overlap as well: every map matches the generic object schema, and URL-only legacy arrays can match both Starr and hook arrays. Consequently valid request bodies fail
oneOfvalidation in OpenAPI clients. This union needsanyOf.
- Files reviewed: 35/36 changed files
- Comments generated: 4
- 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[bot]](https://avatars.githubusercontent.com/in/4697433?s=60&v=4){kind=small}
|
|
||
| if err := unpackerr.validateApp(item.conf(), app); err != nil { | ||
| server := asStarr[T, P](item) | ||
| if err := unpackerr.validateApp(server.conf(), app); err != nil { |
There was a problem hiding this comment.
One rough edge on this path: when env sets UN_SONARR_0_URL without UN_SONARR_0_API_KEY (the file supplies the key) and the user deletes the matching file entry, this rejects the whole PUT with the bare "provided application API Key is invalid, must be at least 32 characters" — nothing indicates it is the env-derived entry "0" and not something in their body. Reproduced: env URL-only, file [sonarr.0] + [sonarr.1], PUT {"1": …} → 400. Pre-PR the same op "succeeded" by stealing the env URL onto the surviving entry, so the rejection is the safe direction; wrapping the error with the key here (fmt.Errorf("%s instance %q: %w", app, key, err)) would make it self-explanatory. putHooks has the same shape.
There was a problem hiding this comment.
Wrapping the error still 400s Save, which we cannot do.
A PUT-body row that omits apiKey because UN_*_API_KEY is set is filled by ParseENV and succeeds. Env overlay slugs that were not in the body and that startup would skip (URL without key, or the reverse) are dropped from live instead of failing the whole save. Remaining 400s now include the instance key (Sonarr instance "0": …). Same drop for incomplete env-only hooks.
| ### Env (`UN_*`) | ||
|
|
||
| Env overlays **live only**. They are not merged into `fileConfig`. Starr / folder / hook PUTs re-apply the overlay onto the live copy after the file-shaped body is written, so env-only list rows survive a save that omitted them; they still never land in `fileConfig` or the TOML. Env-only extra keys/roles exist at runtime until restart unless you add them in the PUT body. General scalars (`UN_INTERVAL`, `UN_PASSWORDS`, …) still overlay only at startup. | ||
| Env overlays **live only**. They are not merged into `fileConfig`. Starr / folder / hook PUTs write the request body as the file snapshot, then overlay live with `ParseENV` (cnfg now overlays existing map entries instead of replacing them). Env-only slugs keep polling. They land in `fileConfig` only if the PUT body included them. PUT `{}` (or a legacy `[]`) clears file instances; live still has `UN_SONARR_uhd_*` / `UN_READARR_0_*`. Env-only extra keys/roles exist at runtime until restart unless you add them in the PUT body. General scalars (`UN_INTERVAL`, `UN_PASSWORDS`, …) still overlay only at startup. The official UI must GET the file section (not `/live`) so it does not persist overlay values. |
There was a problem hiding this comment.
One consequence worth a sentence here: renaming a TOML key that env references (e.g. [sonarr.1] → [sonarr.uhd]) does not move the env identity — after the PUT, live holds both "uhd" (file values, file key) and "1" (env values, env key), so the same server polls twice until the env var is renamed or cleared with UN_SONARR_1=. I verified this by experiment; fine if it's intended, but a client that renames keys in the UI without touching env will hit it.
There was a problem hiding this comment.
Approving.
Reviewed the full pull request at head 6587eda against base 4dc1ea7 (single commit). What it actually contains: (1) Starr/folder/webhook/cmdhook instances become slug-keyed InstanceMaps with dual-read of legacy [[section]] arrays as keys "0", "1", … in both TOML and JSON; (2) a new PUT contract — the request body is the file document, live is clone(fileConfig) + ParseENV, no env-peel safety net; (3) live GET now redacts instance secrets (Starr apiKey/HTTP/native passwords, hook token) even for *, while file GET keeps showing file-stored keys; (4) golift.io/cnfg pinned to the golift/cnfg#48 map-overlay commit; (5) the named kind in configdef, new OpenAPI schemas, regenerated examples, and the test-suite migration.
The renumber/steal fix is sound, and making the map key be the identity is the right core move: delete-safety falls out structurally (a sibling can never inherit a deleted slot's env var), and cnfg's overlay then does the work that #728 was approximating with peel/keepPut.
Executed validation (at head, in a disposable pod; no repo code committed or pushed):
go build ./...andgo vet ./...— clean.go test ./...— all packages pass.go test -race ./pkg/unpackerr/ ./pkg/configdef/— clean.go test ./...inside the pinnedgolift.io/cnfgcommit (6fc2ae3, tip of merged PR #48) — passes, including the empty-value entry-clear path (UN_…_KEY=).- Disposable harness (removed after use): a legacy two-entry
[[sonarr]]file +UN_SONARR_0_URL/API_KEY+PUT {"1": …}→ 200, the env slug survives live as its own entry, only[sonarr.1]is written, no env value reaches the TOML — settling the headline claim for the legacy-array path the repo tests only cover in named/readarr form. Same harness: keyless env slug → 400 (note inline), and a key rename that env references leaves a duplicate env identity (note inline). - The checked-in
examples/unpackerr.conf.examplematches the currentconfigdef.ExampleTOML()output exactly, apart from the generator's timestamp footer.golangci-lintwas not run locally (not in this pod); the public CI on head — gotest on ubuntu/macos/windows, golangci-lint on four OSes, Snyk — is all green as of this review.
Non-blocking notes are in the two inline comments. One housekeeping item that doesn't need a reply: cnfg is pinned as a pseudo-version of an untagged commit; bumping to a real v0.4.1 tag when it gets cut would be the only thing I'd change over time.
… hook objects stay valid.
Live GET redaction now covers Starr passwords and hook tokens; a webhook {} PUT keeps env-only rows.
Co-authored-by: Cursor <cursoragent@cursor.com>
… URL or API key. Save omits env-owned secrets and may omit a slug whose identifying URL is in env; ParseENV still fills complete rows, and incomplete extras are skipped the same way as startup. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the persisted configuration format, runtime ownership paths, API contracts, and environment-overlay dependency behavior across the daemon.
Review details
- Files reviewed: 36/37 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
UN_SONARR_0_*.[[sonarr]]arrays as keys"0","1", …. PUT writes the request body as the file document. Live isclone(fileConfig)+ParseENV(cnfg overlays existing map entries). Env-only slugs still appear on live after{}.keepPutof env-owned fields. A client that PUTs live overlay values persists them. Official UI must GET the file section, not/live.golift.io/cnfgto the map overlay commit (golift/cnfg#48).Test plan
go test ./pkg/unpackerr/ ./pkg/configdef/[[sonarr]]file plusUN_SONARR_0_URL: instance key is0, env overlays URL, file API key survives.PUT /api/config/readarrwith{}whileUN_READARR_0_*is set: file has no Readarr, live still has the env instance, TOML does not gain the env URL or API key.apiKey/ passwords / webhook token; file GET still shows file-stored keys.