Repository navigation
fix(phala): consume normalized CLI inventory - #51
Conversation
Phala 1.1.20 emits cvmName/appId, not raw name/id. Both exact-name matching and update identity were reading absent fields. Use the normalized application id for in-place updates and keep release no-create fail-closed. Exercise the actual deployment shell with the CLI schema and malformed-ID cases.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
![burnt-tribunal[bot]](https://avatars.githubusercontent.com/in/4692562?s=60&v=4){kind=small}
There was a problem hiding this comment.
Tribunal Summary
No findings survived review, and nothing in the diff warrants holding this PR. You correct the Phala deployment workflow to consume normalized CLI inventory, validate application IDs, and update existing CVMs while preserving candidate-only creation. The updated shell-execution tests cover the corrected schema and rejection paths for duplicate names, missing or malformed IDs, and raw-API records.
Confidence Score: 5/5
The workflow change is narrowly scoped, preserves duplicate-name rejection and candidate-only creation, and has shell-execution coverage for successful updates and invalid inventory. No surviving findings or additional merge blockers were identified in the supplied diff.
Changes:
- Select existing CVMs by normalized cvmName and deploy updates using app_ identifiers
- Validate application IDs as 40 lowercase hexadecimal characters before deployment
- Update inventory fixtures and extend failure-path coverage while retaining release no-create assertions
Reviewed changes
Tribunal reviewed 2 of 2 changed files.
Important Files Changed
| File | Overview |
|---|---|
.github/workflows/phala-deploy.yml |
Use normalized cvmName/appId inventory and validate application IDs before targeting existing CVMs for updates |
Other Files Changed
| File | Overview |
|---|---|
tests/workflows.test.mjs |
Align shell-execution fixtures with normalized CLI inventory and test duplicate names, invalid IDs, and release no-create behavior |
🗺️ Phala deployment selects normalized inventory, validates app IDs, and preserves candidate-only creation.
flowchart TD
A["phala cvms list"] --> B["jq: exact cvmName matches (changed)"]
B --> C{"Match count?"}
C -->|"Multiple"| X["Error and exit"]
C -->|"One"| D["jq: read appId (changed)"]
D --> E{"40 lowercase hex characters? (new)"}
E -->|"No"| X
E -->|"Yes"| F["target: --cvm-id app_ID (changed)"]
C -->|"Zero"| G{"TARGET_ROLE is candidate?"}
G -->|"No, never create release"| X
G -->|"Yes"| H["target: -n CVM_NAME"]
F --> I["phala deploy --wait"]
H --> I
What you do with these findings feeds back into how this reviewer behaves — committing a suggestion makes it bolder, rejecting noise makes it quieter. Reply to any finding with a reason to teach it faster.
Replied: this reports review availability, not a code finding. Copilot has not reviewed this head; no billing or quota settings changed.
Replied: checked the reviewed diff at |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Problem and confirmed cause
Part of DO-513.
The first TeeLS release attempt stopped before deployment: run 40218564159. Both its preflight and this reusable workflow treated
phala@1.1.20 cvms list --jsonas raw CVM API{name,id}records. That CLI actually normalizes rows to{cvmName,appId,status,uptime}. The exact-name selector therefore reports zero even for a visible, correctly named CVM; simply repairing the caller would leave this deployment step broken too.Published package provenance:
phala@1.1.20, gitHead502a06186f27f36eda2b85c9d97f19ce73648755; normalizer source. Its deploy--cvm-idaccepts application identifiers, not only CVM UUIDs.Change
cvmNamematches and obtain the normalizedappId.--cvm-id app_<id>.The old tests passed because their fabricated raw-API shape matched the broken jq selectors; the corrected fixture fails on the old workflow.
Verification
node --test --test-name-pattern='Phala never creates' tests/workflows.test.mjs: RED on old selectors, GREEN after fix, RED again with selectors reverted, GREEN restored.pnpm run check: formatting, 87 tests and actionlint pass.pnpm run build: succeeds, generated bundle unchanged.Floor exceptions
The mechanical guard reports three removed assertions in
tests/workflows.test.mjs. Each is replaced, not dropped:has no idbecomeshas no valid app id; the error assertion covers missing, duplicate and invalid-ID failures;existing.idbecomes the actualapp_<existing.appId>update argument. New negative cases strengthen these gates. No ignore-file or checker suppression added.Integration and limits
The paired caller fix is teels-internal#118; it deliberately remains a draft until this fix is released and the consumer pin advances. This draft does not deploy or change credentials. A separately approved central release and consumer pin advance are required before TeeLS can execute it; do not point consumers at an unpublished SHA or weaken either no-create guard. Existing policy-tool and internal quality pins are unchanged because neither dependency changed. CI-key workspace visibility is still a separate read-only preflight, not a proven credential failure. No production dispatch/rerun is authorized by this PR.