fix(deploy): referenced secret sets are no longer rejected as not found (PCC-1021 follow-up) - #197
Conversation
The secret set existence check in `pcc deploy` called the per-set endpoint
through `API.secrets_list`, which returns the set's array of key NAMES, then
tested that array for truthiness.
Referenced secret sets (self-hosted regions, PCC-1020) deliberately carry no
key-name rows: Daily stores name + region + readiness and never the contents.
So the API correctly returns `secrets: []` for a healthy referenced set, the
check read that as "does not exist", and the deploy aborted with "Secret set
'X' not found in organization 'Y'". That made every referenced set
undeployable, which blocks the whole self-hosted secrets flow end to end.
Fetch the set itself with `API.secrets_get` and abort only when nothing comes
back. Readiness stays where PCC-1020 put it, in the server-side deploy gate
that does a fresh status check and blocks on `unknown`.
Verified against staging: a referenced set returns
`{"secrets": [], "region": "hush-0", "source": "referenced", "status": "ready"}`
while a managed set in the same org returns 6 key names.
Verified end to end on a live self-hosted regionRan the full flow against staging with this branch installed as the CLI ( The secret set under test is a real referenced set, not a fixture:
Before (released CLI and git main, same code)After (this branch)The deploy went through, and the agent is serving: Both pods reached Two notes from the run:
|
…he output-mode tests Two additions from review: - `secrets set` had the identical existence-check misread: it fetched the set's key names and tested them for truthiness, so an existing referenced set read as nonexistent and the command said "Creating secret set" before hitting the server's cross-backend 409. It now fetches the set itself, and an existing referenced set gets a clear client-side refusal pointing at the cluster as where its contents are managed. Managed and missing sets behave as before. Tests verified red against the previous check. - The output-mode tests failed in any shell exporting FORCE_COLOR (or TTY_COMPATIBLE): rich resolves is_terminal from those before asking the file, so consoles writing to plain buffers claimed to be terminals. conftest now scrubs both at module level, the same seam as the PIPECAT_CONFIG_PATH isolation, making detection depend only on the file the console is handed.
mattshep
left a comment
mattshep
left a comment
There was a problem hiding this comment.
Verified the whole chain independently, since this guard is the last step of the phase-1 self-hosted secrets flow:
- The misread is real. The per-set GET (sandbox
routes/secrets/controller.ts) returns{secrets, region, source, status}, and a referenced set'ssecretsis empty by design (PCC-1020: name + region + readiness only, never contents). The old client tested exactly that array. A genuine 404 also comes back falsy (not_found_is_empty), which is why the check ever looked correct — "missing" and "referenced" were indistinguishable to it. - The fix tests the right thing.
secrets_getreturns the set object orNoneon 404, so existence is decided on the set, not on key visibility. - On your open question: yes, readiness server-side is the intended split.
validateSecretSetReadyruns at all five deploy sites in the sandbox services controllers with a fresh reconcile at deploy time, so a client-side check would only add a stale second opinion. Good call not duplicating it.
Your 4 tests are well-shaped; I confirmed the positive case exercises the guard, and the fix-reverted run goes red.
I've pushed one commit onto the branch (e094dc5) with two additions found while reviewing — shout if you'd rather they travel separately:
secrets sethad the identical misread: an existing referenced set read as nonexistent, so the command said "Creating secret set" and then hit the server's cross-backend name-uniqueness 409. Samesecrets_getfix, plus a clear client-side refusal for referenced sets (their contents are managed in the cluster, e.g. kubectl). Three tests, verified red against the old check.- The output-mode tests were environment-sensitive: any shell exporting
FORCE_COLOR(orTTY_COMPATIBLE) failed 5 of them locally while CI stayed green — rich resolvesis_terminalfrom those env vars before ever asking the file. conftest now scrubs both at module level (the same seam as thePIPECAT_CONFIG_PATHisolation), so the suite result no longer depends on the developer's shell. Full suite: 436 passed.
Agreed on release coupling: secrets reference (#185) has never shipped (PyPI latest is 1.1.0, 08-05), so nobody outside the team can hit this yet — and the broken and fixed paths want to go out in the same release so the subcommand never exists without a working deploy.
Since we've now both contributed to the branch, handing the approval to fresh eyes.
cbrianhill
left a comment
cbrianhill
left a comment
There was a problem hiding this comment.
Approving. The fix is correct and the scope is exactly right.
I traced the central claim rather than taking it on faith, and it holds in a way that makes this even lower-risk than the description suggests: _secrets_list(secret_set=X) and _secrets_get build the identical URL (GET /secrets/{setName} → the sandbox's getSecretSet). So this changes only how the response is parsed — same endpoint, same server-side reconcile, no additional API calls. And getSecretSet returns secrets: set.secrets with Secret included as attributes: ['fieldName'], which independently confirms both the empty-array-for-referenced behavior and the object shape the secrets set overlap check consumes.
I also checked whether there was a third instance of the same misread, since two had already turned up. There isn't: unset and delete go straight to their delete endpoints with no key-name truthiness test, and the image-pull-secret guard matches on name + type off the org-wide list, which is a different response shape. The coverage here is complete.
One thing I specifically went looking for and did not find a problem with — the conftest.py env scrub. Scrubbing environment variables in conftest can quietly paper over real behavior, so I checked console.output_mode: it returns _explicit_output_mode before ever consulting is_terminal, so --output plain was never at risk from FORCE_COLOR. Only the auto-detection path was env-sensitive, and honoring FORCE_COLOR there is the desired production behavior. Right fix, and the comment's reasoning is accurate.
Two non-blocking comments inline: a missing CHANGELOG entry for the secrets set half, and a UX ordering nit where the new refusal lands after the confirmation prompt.
Agreed on the release coupling — secrets reference (#185) has never shipped, so these want to go out together or the subcommand exists without a working deploy. And nice work on the evidence: the live hush-0 before/after plus a fix-reverted red run is more than most PRs this size carry.
| for truthiness, but referenced sets carry no key names by design (Daily | ||
| stores name, region and readiness, never the contents), so every healthy | ||
| referenced set failed the check and no self-hosted deploy could use one. The | ||
| check now fetches the set itself; readiness is still enforced server-side. |
There was a problem hiding this comment.
The entry covers the deploy guard, but not the secrets set fix that came in with e094dc5 — and that one is a user-visible behavior change too: pipecat cloud secrets set <referenced-set> now refuses client-side, where before it printed "Creating secret set" and then failed on the server's cross-backend 409.
This repo keeps a pretty thorough Keep-a-Changelog (secrets reference itself got a full Added entry), so a silently-undocumented command change stands out. Something like:
- `pipecat cloud secrets set` no longer treats an existing referenced secret
set as nonexistent. It had the same key-names misread as the deploy guard,
so it announced "Creating secret set" and then failed on the server's
name-uniqueness check. It now refuses clearly, pointing at the cluster as
the place a referenced set's contents are managed.There was a problem hiding this comment.
Added in 752fd18, essentially your wording with one change: "refuses before asking for confirmation", since after the ordering fix below that is now also true.
Agreed it stood out. The Unreleased section documents the deploy guard in full, so leaving its sibling silent would have implied secrets set was untouched.
| # deploy guard, cli#197): a referenced set carries no key-name rows by | ||
| # design, so testing the key array reads "exists but referenced" as | ||
| # "does not exist". | ||
| set_data, error = await API.secrets_get(org=org, secret_set=name) |
There was a problem hiding this comment.
The refusal is right, but it fires late: this existence check sits after the if not skip_confirm: block, so a user running secrets set against a referenced set gets the full "Secrets to create / modify" panel, answers "Would you like to proceed with these secrets?", and only then is told the command doesn't apply to this set.
The ordering is pre-existing, but the new hard refusal is the first outcome where that confirmation is guaranteed wasted. Worth hoisting the secrets_get lookup above the confirmation block — as a side benefit, existing_set is already what decides the "Creating" vs "Modifying" wording, so the earlier fetch fits naturally there.
Non-blocking; happy for it to be a follow-up.
There was a problem hiding this comment.
Fixed in 752fd18 rather than deferred. Hoisted the whole secrets_get lookup above the if not skip_confirm: block; the diff is a pure block move, with the leading comment rewritten to record why the ordering matters so it does not drift back.
Your framing sharpened what the actual defect was. Every other check in create_set (name, file, key-value syntax, region) already refuses before prompting. The set lookup was the one validation that ran after, purely because it started life as the create-vs-modify decision rather than as a guard. Adding a hard refusal to it is what turned a latent ordering wart into a visible one, and it does read more naturally in the earlier position.
Added a test for the ordering, since none of the existing three would have caught a regression: all of them pass skip_confirm=True. It asserts questionary.confirm is never called, with API.properties never awaited as a second witness (that call exists only to render the panel's region line).
One detail worth recording. Verifying against the pre-hoist code, the test first failed with a TypeError because .ask_async() on a plain MagicMock is not awaitable. That proved the prompt was reached, but for the wrong reason. The mocked prompt now answers yes if it is reached, so the red is:
AssertionError: Expected 'confirm' to not have been called. Called 1 times.
Also worth flagging since it is the trade you are asking for: the lookup now happens before the panel, so a user who aborts at the prompt has made one extra read they previously would not have. Cheap, and the right way round.
One thing I did not take: you noted existing_set is what decides the "Creating" vs "Modifying" wording, which now means the panel title Secrets to create / modify could resolve to the exact verb. I read that as justifying the hoist rather than requesting the change, and it is a separate UX edit, so I left it. Happy to do it if you meant it as a suggestion.
…ngelog Both non-blocking notes from Brian's review: - The `secrets set` refusal fired after the confirmation block, so a user running it against a referenced set saw the full "Secrets to create / modify" panel and answered "proceed?" before being told the command does not apply to that set. The set lookup now runs ahead of the prompt, which also restores the invariant that every refusal in this command happens before any question. It already decided the create-vs-modify wording, so it reads naturally in the earlier position. - Added the missing CHANGELOG entry for the `secrets set` half of e094dc5. It is a user-visible behavior change (it announced "Creating secret set" and then failed on the server's name-uniqueness check) and the Unreleased section documents the deploy guard already. New test covers the ordering: the three existing cases all pass skip_confirm=True, so none of them would have caught a regression here. It asserts questionary.confirm is never called, with API.properties never awaited as a second witness, since that call exists only to render the panel. The mocked prompt answers yes if reached, so the pre-hoist run fails on that assertion rather than on an unawaitable mock. Full suite: 437 passed. ruff format --diff and ruff check clean. pyright on secrets.py reports 0 errors.
3 participants
Found while running the full self-hosted secrets flow end to end on a kind gateway region (
hush-0) against staging, ahead of showing it to customers.The bug
pcc deploycannot deploy an agent that uses a referenced secret set. Every one of them is rejected as missing:The set was healthy at the time:
pipecat cloud secrets listshowed itreadyinhush-0, andsecrets referencehad just succeeded.Why
The check in
_deployfetched the set throughAPI.secrets_list(secret_set=...), which returns the set's array of key names, and then tested that array for truthiness.Referenced sets carry no key-name rows. That is deliberate, from PCC-1020: "No
Secret(key-name) rows — referenced sets are name+region+readiness only; contents are invisible by design." So the API does the right thing and the client misreads it.Confirmed against staging, same org:
secretslengthpipecat-quickstart-secrets-hush-0referencedpipecat-quickstart-secretsmanagedThe API response shape is identical in both cases (
region,secrets,source,status); only the array length differs.This blocks the phase-1 self-hosted secrets flow at the last step. PCC-1021 added
secrets reference, the readiness display, and thesecrets setguard, but the deploy-time existence check predates references and was not revisited.The fix
Use
API.secrets_get, whichapi.pyalready documents for exactly this ("Use this when you need readiness"), and abort only when nothing comes back._secrets_getpassesnot_found_is_empty=True, so a genuinely missing set still returnsNoneand still aborts.Readiness deliberately stays server-side, where PCC-1020 put it: a fresh
getSecretStatusat deploy time that blocks onunknown. This PR does not duplicate that client-side.Tests
tests/test_deploy_referenced_secret_set.py, mirroring the structure oftest_deploy_image_pull_secret.py:secrets: [],source: referenced) passes the guard and reachesAPI.deployNone) still aborts beforeAPI.deployFull suite: 433 passed.
ruff format --diffandruff checkclean.pyrightondeploy.pyis unchanged at 3 pre-existing errors.Note on releasing
secrets referenceitself (#185) is also still unreleased: latest on PyPI is 1.1.0 from 2026-08-05, and #185 merged 2026-08-14. Anyone testing this flow from PyPI hits the missing subcommand first and this bug second, so the two want to ship together.Draft because the release timing is Matt's call, and worth a check that keeping readiness purely server-side is the intended split.