fix(agent-challenge): owned CVM teardown, fail-loud list, staging stack - #51
fix(agent-challenge): owned CVM teardown, fail-loud list, staging stack#51echobt wants to merge 1 commit into
Conversation
Rebase the safety-critical selfdeploy/staging work onto current main without the older 38-commit stack that conflicted with PR #49. - Fail-loud Phala CVM listing via GET /cvms/paginated + X-Phala-Version 2026-06-23; unknown envelopes raise instead of under-reporting as 0 - Loopback http:// only with SELFDEPLOY_ALLOW_INSECURE_LOOPBACK=1 - Staging compose/scripts/docs with owned-only teardown (never account-sweep) - AGENTS.md points operators at the local staging loop first Dropped changes main already covers (shape mismatch formatter, eval default tdx.xlarge, HTTP delete_cvm). Compose pre-artifact pin matching deferred: main's generator does not yet include artifact envs and cannot reproduce daf0 without a broader compose delta.
📝 WalkthroughWalkthroughAdds an isolated local Agent Challenge staging stack using real Phala TDX CVMs, strict paginated CVM parsing, owned-only teardown planning, loopback HTTP controls, pinned staging configuration, end-to-end review/evaluation orchestration, and production compose upgrade documentation. ChangesAgent Challenge staging
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant run_staging.sh
participant AgentChallenge
participant PhalaCloud
participant KeyRelease
Operator->>run_staging.sh: start local staging run
run_staging.sh->>AgentChallenge: submit miner artifact
AgentChallenge->>PhalaCloud: deploy review CVM
run_staging.sh->>AgentChallenge: poll review outcome
run_staging.sh->>KeyRelease: start local KR service
AgentChallenge->>PhalaCloud: deploy evaluation CVM
PhalaCloud->>KeyRelease: request attested secret release
run_staging.sh->>AgentChallenge: poll guest_artifact_proof
run_staging.sh->>PhalaCloud: delete owned CVMs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/challenges/agent-challenge/docs/prod-compose-upgrade.md (1)
1-504: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftReduce this shipping document to a minimal operational note.
This 500-line runbook embeds incident-specific hashes, submission state, host paths, timestamps, and validation evidence. Keep a short upgrade/rollback pointer in product docs and move measured evidence and change-window details to local untracked evidence or the ops change record.
As per coding guidelines: keep shipping documentation minimal and keep validation logs out of product documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/docs/prod-compose-upgrade.md` around lines 1 - 504, Replace the oversized incident-specific runbook with a concise product-documentation note covering only the artifact-aware eval pin upgrade, required authorization, high-level upgrade/rollback pointers, and references to the relevant implementation and operational procedures. Remove measured hashes, submission state, host paths, timestamps, validation logs, and other change-window evidence; direct operators to record those details in local untracked evidence or the ops change record instead.Source: Coding guidelines
🧹 Nitpick comments (8)
packages/challenges/agent-challenge/docs/staging.md (1)
311-313: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the Phala CLI instead of executing
npx phala@latest.This command executes mutable, unreviewed package code and makes teardown verification non-reproducible. Use an approved pinned CLI version or repository-managed toolchain.
As per coding guidelines: treat shell commands and dependency changes as executable surfaces during review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/docs/staging.md` around lines 311 - 313, Replace the teardown command that invokes `npx phala@latest` with an approved pinned Phala CLI version or the repository-managed toolchain. Ensure the command resolves a deterministic, reviewed CLI version rather than mutable latest package code.Source: Coding guidelines
packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cvm_list.py (2)
65-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate
_item_id/_extract_cvm_id.Both walk
_CREATE_CVM_ID_FIELDSidentically; only the miss behavior differs.♻️ Proposed dedupe
def _extract_cvm_id(item: Mapping[str, Any]) -> str: - for name in _CREATE_CVM_ID_FIELDS: - if name not in item: - continue - normalized = _normalize_id(item.get(name)) - if normalized is not None: - return normalized - raise ValueError("Phala create response does not identify the CVM") + found = _item_id(item) + if found is None: + raise ValueError("Phala create response does not identify the CVM") + return found🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cvm_list.py` around lines 65 - 82, Deduplicate the identical field-scanning logic in _item_id and _extract_cvm_id by reusing one helper or having one function delegate to the other. Preserve _item_id’s None result for missing or invalid identifiers, while keeping _extract_cvm_id’s ValueError behavior when no identifier is found.
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAmbiguity is signalled by an error message substring.
phala.resolve_cvm_id_from_listdistinguishes this case withif "multiple CVMs match app_id" in msg. A dedicated subclass (e.g.AmbiguousCvmMatchError(CvmListParseError)) would make that contract robust against wording changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cvm_list.py` around lines 235 - 239, Introduce a dedicated AmbiguousCvmMatchError subclass of CvmListParseError and raise it in the require_unique branch of the CVM resolution logic when multiple matches are found. Update phala.resolve_cvm_id_from_list to detect this exception type instead of searching the error message for “multiple CVMs match app_id”, preserving existing handling for other CvmListParseError cases.packages/challenges/agent-challenge/scripts/staging/cvm_teardown_policy.py (1)
17-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parse_account_cvms_payloadraisesSystemExitfrom a library-style function.Callers that import this module (the tests do) get process-exit semantics instead of a catchable error. Prefer raising
ValueErrorhere and converting toSystemExitinmain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/scripts/staging/cvm_teardown_policy.py` around lines 17 - 39, Update parse_account_cvms_payload to raise ValueError from CvmListParseError instead of SystemExit, preserving the original error message and exception chaining. In main, catch the parsing ValueError and convert it to SystemExit so library callers receive a catchable error while the CLI retains its exit behavior.packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py (1)
48-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAll three branches of the second
elifassign the same value; the dispatch logic is effectively dead.Lines 61-70 collapse to
self._queue = list(payloads). Also note that a genuine bare-list response body (e.g.[{"id": "cvm_1"}]) is misinterpreted as a one-element queue, so the opener can never serve a list body — worth fixing before someone adds such a test.♻️ Proposed simplification
- if isinstance(payloads, list) and payloads and not isinstance( - payloads[0], dict - ): - # list of sequential response bodies - self._queue = list(payloads) - elif isinstance(payloads, list) and all( - isinstance(p, (dict, list)) for p in payloads - ): - # could be one list-body OR queue of bodies — treat multi as queue - # when first element looks like a full response object with items/total - if ( - len(payloads) > 1 - and isinstance(payloads[0], dict) - and ("items" in payloads[0] or "total" in payloads[0]) - ): - self._queue = list(payloads) - elif len(payloads) == 1: - self._queue = list(payloads) - else: - self._queue = list(payloads) - else: - self._queue = [payloads] + # ``payloads`` is either a queue of response bodies or a single body. + self._queue = list(payloads) if isinstance(payloads, list) else [payloads]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py` around lines 48 - 72, Fix _CapturingOpener payload classification: remove the redundant branches in the second elif and distinguish a bare list response body from a queue of response bodies, ensuring a payload such as [{"id": "cvm_1"}] is served as one body while genuine sequential bodies remain queued.packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.py (1)
329-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnreachable fallback branch.
combinedis parsed from a bare list, socombined.total == len(all_items)always. Thereforecombined.total != reported_totalimplieslen(all_items) != reported_total, and the innerifalways raises — lines 336-341 are dead. Collapse to a single check.♻️ Proposed simplification
combined = parse_cvms_list_response(list(all_items)) - if reported_total is not None and combined.total != reported_total: - if len(all_items) != reported_total: - raise CvmListParseError( - "unrecognized CVM list shape: collected items " - f"{len(all_items)} != total {reported_total}" - ) - return CvmListSnapshot( - items=combined.items, - total=reported_total, - ids=combined.ids, - source_shape="paginated-merged", - ) + if reported_total is not None and len(all_items) != reported_total: + raise CvmListParseError( + "unrecognized CVM list shape: collected items " + f"{len(all_items)} != total {reported_total}" + ) return CvmListSnapshot( items=combined.items, total=reported_total if reported_total is not None else combined.total, ids=combined.ids, source_shape="paginated-merged", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.py` around lines 329 - 347, In the paginated response merge logic, simplify the reported_total validation after parse_cvms_list_response(list(all_items)): remove the unreachable inner branch that returns a snapshot, and directly raise CvmListParseError when the merged item count differs from reported_total. Preserve the existing snapshot construction for valid totals, including its total and source_shape values.packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cli.py (1)
204-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-serializing the snapshot into a listing dict re-runs shape validation unnecessarily.
resolve_cvm_id_from_snapshot(snapshot, app_id=identity, require_unique=True)avoids the round-trip through_total_from_mapping, which could reject an otherwise-valid merged snapshot on total/items consistency rules. Note the ambiguity case then raisesCvmListParseErrorrather thanPhalaApiError, so keep that translation in the caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cli.py` around lines 204 - 212, The teardown flow should resolve the CVM identity directly from the parsed snapshot instead of rebuilding a listing dict and invoking resolve_cvm_id_from_list. Replace that call with resolve_cvm_id_from_snapshot(snapshot, app_id=identity, require_unique=True), and preserve caller-level translation of ambiguity-related CvmListParseError into the appropriate RouteClientError or API error.packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py (1)
13-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated staging-policy module loader across two test files. Both modules define an identical
POLICY_PATHand_load_policy()to importscripts/staging/cvm_teardown_policy.py; a singleconftest.pyfixture would keep the path in one place.
packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py#L13-L28: movePOLICY_PATHand_load_policyintotests/conftest.pyand keep only thepolicyfixture consumption here.packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py#L32-L45: import the sharedPOLICY_PATH/policyfixture instead of redefining the loader.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py` around lines 13 - 28, Centralize the duplicated cvm_teardown_policy loader by moving POLICY_PATH and _load_policy into tests/conftest.py and exposing the shared policy fixture. In packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py#L13-L28, remove the local definitions and consume the fixture; in packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py#L32-L45, likewise remove the duplicate loader and use the shared POLICY_PATH/policy fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 96-98: The documentation must consistently define teardown as
owned-CVM-only. In AGENTS.md, replace account-wide zero-count language with
verification that the run-owned CVM count reaches zero, while preserving
paginated verification. In packages/challenges/agent-challenge/docs/staging.md,
update the --down documentation to state that it removes only owned CVMs and
never sweeps account-wide or foreign/production CVMs.
In `@packages/challenges/agent-challenge/docker-compose.staging.yml`:
- Around line 31-32: Remove the unconditional dcap-qvl host bind from the base
staging Compose configuration so the baked /usr/local/bin/dcap-qvl binary
remains available on hosts without the source path. If host overrides are
needed, place this read-only bind in a separate opt-in staging override file
such as docker-compose.staging.dcap.yml.
In
`@packages/challenges/agent-challenge/scripts/staging/config/challenge.env.example`:
- Line 17: Replace the hardcoded value for CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT
with a clearly marked placeholder for the staging host’s externally reachable
IP, preserving port 8701, and add an adjacent comment documenting that operators
must set it to their staging host address.
In
`@packages/challenges/agent-challenge/scripts/staging/config/kr.example/README.md`:
- Around line 9-14: Replace the server-certificate placeholder in the KR staging
README with tested OpenSSL commands that issue a KR server certificate signed by
the generated CA and include the required Subject Alternative Name. Document the
exact generated server key and certificate paths expected by staging, while
preserving the existing CA and golden-key setup.
In
`@packages/challenges/agent-challenge/scripts/staging/config/measurements_source.md`:
- Around line 7-9: Remove the absolute workspace and production-host evidence
paths from the documentation list in measurements_source.md. Retain only
portable provenance such as dates, quote identifiers, and hashes, ensuring the
shipped markdown remains minimal and does not include validation-log locations.
In `@packages/challenges/agent-challenge/scripts/staging/cvm_teardown_policy.py`:
- Around line 250-254: Update the --dry-run argument definition and its handling
in the staging teardown entry point to clearly document that it is accepted for
compatibility and does not change behavior, or remove the flag entirely. Ensure
the help text and surrounding logic do not imply that omitting the flag performs
deletion, and eliminate the no-op unused-variable handling if the flag is
removed.
- Around line 144-162: Propagate the unresolved ids from resolve_delete_ids
through select_teardown_ids and plan_teardown as unresolved_owned, and include
that field in the generated plan output so unverified tracked tokens are
distinguishable from confirmed account ids. Update all callers, including tests
currently using two-value unpacking, to handle the expanded return value while
preserving existing deletion selection.
In `@packages/challenges/agent-challenge/scripts/staging/run_staging.sh`:
- Around line 660-665: Replace the predictable /tmp/hosts.staging.$$ staging in
run_staging.sh lines 660-665 with a mktemp-created file, remove the dead awk
write, and continue filtering and copying the result to /etc/hosts with cleanup.
Apply the same mktemp-based staging fix to the teardown unpin path at lines
506-514; both sites require direct changes using the generated temporary
filename consistently.
- Line 557: Replace the world-readable permissions in run_staging.sh at
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L557-L557 for
challenge_token and review_evidence_encryption_key with group-scoped read access
shared with container uid 10001, using chgrp or equivalent ownership setup. Also
update
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L475-L483 to
replace world read/write access on the staging SQLite database and world
read/write/execute access on its volume directory with group-scoped permissions
shared with the KR process; preserve required functionality without exposing
sensitive materials to other local users.
- Around line 370-409: Move the OWNED_CVMS_FILE pruning block out of the
pre-verification path in the teardown function. Compute owned_left from the
original tracking files and final CVM listing first, return failure when any
owned CVM remains, then prune only targeted IDs confirmed absent from the final
listing after the check succeeds; preserve the existing best-effort handling for
the cleanup write.
- Around line 205-214: Remove direct shell interpolation of remote IDs and file
paths into Python source across the identified unquoted heredocs. Pass each
value through environment variables or another data channel, then read it from
os.environ inside the corresponding Python blocks; update the DELETE request
block around cid and the other heredocs at the referenced staging-script
sections consistently.
- Around line 826-841: Remove the eval-based assignment around the Python JSON
parsing block in the staging result handling. Safely transfer reason_code and
retryable from the parsed response using a file or separate command
substitutions with robust shell-safe handling, preserving the existing defaults
for malformed responses and missing items without allowing API-controlled
reason_code content to execute as shell syntax.
In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.py`:
- Around line 305-324: Update the paginated CVM aggregation loop around
parse_cvms_list_response to track each CVM’s unique ID and raise
CvmListParseError when an ID repeats across pages, before extending all_items.
Preserve normal pagination and total-consistency validation, failing closed
instead of returning duplicate rows when the server repeats a page.
---
Outside diff comments:
In `@packages/challenges/agent-challenge/docs/prod-compose-upgrade.md`:
- Around line 1-504: Replace the oversized incident-specific runbook with a
concise product-documentation note covering only the artifact-aware eval pin
upgrade, required authorization, high-level upgrade/rollback pointers, and
references to the relevant implementation and operational procedures. Remove
measured hashes, submission state, host paths, timestamps, validation logs, and
other change-window evidence; direct operators to record those details in local
untracked evidence or the ops change record instead.
---
Nitpick comments:
In `@packages/challenges/agent-challenge/docs/staging.md`:
- Around line 311-313: Replace the teardown command that invokes `npx
phala@latest` with an approved pinned Phala CLI version or the
repository-managed toolchain. Ensure the command resolves a deterministic,
reviewed CLI version rather than mutable latest package code.
In `@packages/challenges/agent-challenge/scripts/staging/cvm_teardown_policy.py`:
- Around line 17-39: Update parse_account_cvms_payload to raise ValueError from
CvmListParseError instead of SystemExit, preserving the original error message
and exception chaining. In main, catch the parsing ValueError and convert it to
SystemExit so library callers receive a catchable error while the CLI retains
its exit behavior.
In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cli.py`:
- Around line 204-212: The teardown flow should resolve the CVM identity
directly from the parsed snapshot instead of rebuilding a listing dict and
invoking resolve_cvm_id_from_list. Replace that call with
resolve_cvm_id_from_snapshot(snapshot, app_id=identity, require_unique=True),
and preserve caller-level translation of ambiguity-related CvmListParseError
into the appropriate RouteClientError or API error.
In
`@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cvm_list.py`:
- Around line 65-82: Deduplicate the identical field-scanning logic in _item_id
and _extract_cvm_id by reusing one helper or having one function delegate to the
other. Preserve _item_id’s None result for missing or invalid identifiers, while
keeping _extract_cvm_id’s ValueError behavior when no identifier is found.
- Around line 235-239: Introduce a dedicated AmbiguousCvmMatchError subclass of
CvmListParseError and raise it in the require_unique branch of the CVM
resolution logic when multiple matches are found. Update
phala.resolve_cvm_id_from_list to detect this exception type instead of
searching the error message for “multiple CVMs match app_id”, preserving
existing handling for other CvmListParseError cases.
In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.py`:
- Around line 329-347: In the paginated response merge logic, simplify the
reported_total validation after parse_cvms_list_response(list(all_items)):
remove the unreachable inner branch that returns a snapshot, and directly raise
CvmListParseError when the merged item count differs from reported_total.
Preserve the existing snapshot construction for valid totals, including its
total and source_shape values.
In `@packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py`:
- Around line 48-72: Fix _CapturingOpener payload classification: remove the
redundant branches in the second elif and distinguish a bare list response body
from a queue of response bodies, ensuring a payload such as [{"id": "cvm_1"}] is
served as one body while genuine sequential bodies remain queued.
In
`@packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py`:
- Around line 13-28: Centralize the duplicated cvm_teardown_policy loader by
moving POLICY_PATH and _load_policy into tests/conftest.py and exposing the
shared policy fixture. In
packages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py#L13-L28,
remove the local definitions and consume the fixture; in
packages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.py#L32-L45,
likewise remove the duplicate loader and use the shared POLICY_PATH/policy
fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dc68352-0804-4be8-86b7-594bbffff99f
📒 Files selected for processing (27)
AGENTS.mdpackages/challenges/agent-challenge/docker-compose.staging.ymlpackages/challenges/agent-challenge/docs/README.mdpackages/challenges/agent-challenge/docs/prod-compose-upgrade.mdpackages/challenges/agent-challenge/docs/staging.mdpackages/challenges/agent-challenge/scripts/staging/.gitignorepackages/challenges/agent-challenge/scripts/staging/config/challenge.env.examplepackages/challenges/agent-challenge/scripts/staging/config/challenge_token.examplepackages/challenges/agent-challenge/scripts/staging/config/dstack-client-trust.crt.examplepackages/challenges/agent-challenge/scripts/staging/config/eval_allowlist.jsonpackages/challenges/agent-challenge/scripts/staging/config/kr.example/README.mdpackages/challenges/agent-challenge/scripts/staging/config/kr_allowlist.jsonpackages/challenges/agent-challenge/scripts/staging/config/measurements_source.mdpackages/challenges/agent-challenge/scripts/staging/config/pins.jsonpackages/challenges/agent-challenge/scripts/staging/config/review_allowlist.jsonpackages/challenges/agent-challenge/scripts/staging/config/review_evidence_encryption_key.examplepackages/challenges/agent-challenge/scripts/staging/cvm_teardown_policy.pypackages/challenges/agent-challenge/scripts/staging/run_staging.shpackages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cli.pypackages/challenges/agent-challenge/src/agent_challenge/selfdeploy/client.pypackages/challenges/agent-challenge/src/agent_challenge/selfdeploy/cvm_list.pypackages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.pypackages/challenges/agent-challenge/tests/test_phala_create_ack_and_cli_token.pypackages/challenges/agent-challenge/tests/test_phala_cvms_list_parse.pypackages/challenges/agent-challenge/tests/test_selfdeploy_loopback_http_policy.pypackages/challenges/agent-challenge/tests/test_selfdeploy_teardown_http.pypackages/challenges/agent-challenge/tests/test_staging_cvm_teardown_policy.py
| - CVMs are **real** Phala TDX machines (billable). Staging tears down **only CVMs this run owns** (`work/owned_cvms.txt` + per-run track). It never account-sweeps foreign/prod CVMs. Always tear down owned CVMs before you leave. | ||
|
|
||
| Real Phala TDX CVMs + dual attestation flags; always tear down to a verified CVM count of 0 via paginated list (never trust bare `GET /cvms` empty arrays). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the owned-CVM teardown boundary consistently.
The documentation mixes owned-only cleanup with account-wide zero-count and sweep language, which could lead to deletion of foreign or production CVMs.
AGENTS.md#L96-L98: verify that only the run-owned CVM count reaches zero; do not require an account-wide zero.packages/challenges/agent-challenge/docs/staging.md#L102-L102: state that--downremoves only owned CVMs and does not sweep the account.
📍 Affects 2 files
AGENTS.md#L96-L98(this comment)packages/challenges/agent-challenge/docs/staging.md#L102-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 96 - 98, The documentation must consistently define
teardown as owned-CVM-only. In AGENTS.md, replace account-wide zero-count
language with verification that the run-owned CVM count reaches zero, while
preserving paginated verification. In
packages/challenges/agent-challenge/docs/staging.md, update the --down
documentation to state that it removes only owned CVMs and never sweeps
account-wide or foreign/production CVMs.
| # dcap-qvl is baked into the runtime image; host bind is optional fallback | ||
| - /root/.cargo/bin/dcap-qvl:/usr/local/bin/dcap-qvl:ro |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unconditional host bind of dcap-qvl will shadow the baked binary on hosts lacking /root/.cargo/bin/dcap-qvl.
Docker creates an empty directory at a missing bind source, then mounts it over /usr/local/bin/dcap-qvl, turning the baked binary into a directory and failing attestation startup. A "fallback" bind can't be conditional in a compose file — move it to an opt-in override (e.g. docker-compose.staging.dcap.yml) instead.
🔧 Proposed change
- # dcap-qvl is baked into the runtime image; host bind is optional fallback
- - /root/.cargo/bin/dcap-qvl:/usr/local/bin/dcap-qvl:ro
+ # dcap-qvl is baked into the runtime image. If you need a host override,
+ # add it via an extra `-f` override file rather than here — a missing
+ # source path would be auto-created as a directory and shadow the binary.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # dcap-qvl is baked into the runtime image; host bind is optional fallback | |
| - /root/.cargo/bin/dcap-qvl:/usr/local/bin/dcap-qvl:ro | |
| # dcap-qvl is baked into the runtime image. If you need a host override, | |
| # add it via an extra `-f` override file rather than here — a missing | |
| # source path would be auto-created as a directory and shadow the binary. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/docker-compose.staging.yml` around lines
31 - 32, Remove the unconditional dcap-qvl host bind from the base staging
Compose configuration so the baked /usr/local/bin/dcap-qvl binary remains
available on hosts without the source path. If host overrides are needed, place
this read-only bind in a separate opt-in staging override file such as
docker-compose.staging.dcap.yml.
| CHALLENGE_EVAL_APP_KMS_PUBLIC_KEY_HEX=8820793b3116a96b7c8c7daf06b104b14cbf9ee5dd3fb65d8ad53b50cefc7809 | ||
| CHALLENGE_EVAL_APP_MEASUREMENT={"mrtd":"f06dfda6dce1cf904d4e2bab1dc370634cf95cefa2ceb2de2eee127c9382698090d7a4a13e14c536ec6c9c3c8fa87077","rtmr0":"68102e7b524af310f7b7d426ce75481e36c40f5d513a9009c046e9d37e31551f0134d954b496a3357fd61d03f07ffe96","rtmr1":"07e6f51aa763abfe75c3ddfbf4f425fe3f0ceff66d807a75e049303dce9addf68e7218729bd419638af63a370f65878c","rtmr2":"df67e467e60edc1737bcf8e682d48131bfb427f523226aa7f197a7608e9b3784783fa759ef5b28191fa12f9ddb36b858","os_image_hash":"5c6d8f757e3adb0563efc809710076a631442db3b4de02ad32d33fe1994721e0","key_provider":"phala","vm_shape":"tdx.small"} | ||
| CHALLENGE_EVAL_APP_MEASUREMENT_ALLOWLIST=[{"mrtd":"f06dfda6dce1cf904d4e2bab1dc370634cf95cefa2ceb2de2eee127c9382698090d7a4a13e14c536ec6c9c3c8fa87077","rtmr0":"68102e7b524af310f7b7d426ce75481e36c40f5d513a9009c046e9d37e31551f0134d954b496a3357fd61d03f07ffe96","rtmr1":"07e6f51aa763abfe75c3ddfbf4f425fe3f0ceff66d807a75e049303dce9addf68e7218729bd419638af63a370f65878c","rtmr2":"df67e467e60edc1737bcf8e682d48131bfb427f523226aa7f197a7608e9b3784783fa759ef5b28191fa12f9ddb36b858","os_image_hash":"5c6d8f757e3adb0563efc809710076a631442db3b4de02ad32d33fe1994721e0","compose_hash":"0647b4d9b1e3d458b7910638ee187c968835840fe2e65f1f332dbe69c518dfd9"}] | ||
| CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT=84.32.70.61:8701 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the hardcoded host IP with a placeholder.
84.32.70.61:8701 is an operator-specific address baked into a committed template; copying this file verbatim points guest key-release traffic at a foreign host. Use a placeholder and document that it must be the staging host's externally reachable IP.
🔧 Proposed change
-CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT=84.32.70.61:8701
+# Externally reachable address of the host running the staging key-release server.
+CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT=REPLACE_WITH_STAGING_HOST_IP:8701📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT=84.32.70.61:8701 | |
| # Externally reachable address of the host running the staging key-release server. | |
| CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT=REPLACE_WITH_STAGING_HOST_IP:8701 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/challenges/agent-challenge/scripts/staging/config/challenge.env.example`
at line 17, Replace the hardcoded value for CHALLENGE_EVAL_KEY_RELEASE_ENDPOINT
with a clearly marked placeholder for the staging host’s externally reachable
IP, preserving port 8701, and add an adjacent comment documenting that operators
must set it to their staging host address.
| openssl req -x509 -newkey rsa:2048 -nodes \ | ||
| -keyout scripts/staging/config/kr/ca.key \ | ||
| -out scripts/staging/config/kr/ca.crt \ | ||
| -days 365 -subj "/CN=ac-staging-kr-ca" | ||
| # ... issue server cert for your KR host; copy ca.crt to config/kr-server-ca.crt | ||
| openssl rand -out scripts/staging/config/kr/golden.key 32 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the complete KR server-certificate setup.
The command only creates the CA; the # ... issue server cert placeholder leaves the staging setup incomplete. Add tested commands that issue the KR server certificate with the required SAN and document the exact key/certificate paths consumed by staging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/challenges/agent-challenge/scripts/staging/config/kr.example/README.md`
around lines 9 - 14, Replace the server-certificate placeholder in the KR
staging README with tested OpenSSL commands that issue a KR server certificate
signed by the generated CA and include the required Subject Alternative Name.
Document the exact generated server key and certificate paths expected by
staging, while preserving the existing CA and golden-key setup.
| - Review TEE evidence: `/work/baseintelligence/.omo/evidence/ac-attested-review-20260727/review-tee.json` | ||
| - T8 eval KR allowlist dumps: `/work/baseintelligence/.omo/start-work/T8-e2e/kr-meas-diag-20260725T233258Z.txt` | ||
| - Prod KR file (same core): host `/var/lib/base/keyrelease/eval-allowlist.json` on the prod master |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove internal evidence paths from shipped documentation.
The absolute workspace and production-host paths are environment-specific and turn this file into a validation log. Keep portable provenance such as dates, quote identifiers, and hashes, or store raw evidence locations outside the repository.
As per coding guidelines: **/*.md must keep shipping documentation minimal and must not add validation logs to product documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/challenges/agent-challenge/scripts/staging/config/measurements_source.md`
around lines 7 - 9, Remove the absolute workspace and production-host evidence
paths from the documentation list in measurements_source.md. Retain only
portable provenance such as dates, quote identifiers, and hashes, ensuring the
shipped markdown remains minimal and does not include validation-log locations.
Source: Coding guidelines
| # Drop successfully targeted ids from durable owned list (best-effort). | ||
| if [[ -f "${OWNED_CVMS_FILE}" && -n "${will_delete// /}" ]]; then | ||
| local tmp_owned keep d | ||
| tmp_owned="$(mktemp)" | ||
| while read -r id; do | ||
| [[ -n "$id" ]] || continue | ||
| keep=1 | ||
| for d in ${will_delete}; do [[ "$id" == "$d" ]] && keep=0 && break; done | ||
| [[ "$keep" == "1" ]] && echo "$id" | ||
| done <"${OWNED_CVMS_FILE}" >"${tmp_owned}" || true | ||
| mv -f "${tmp_owned}" "${OWNED_CVMS_FILE}" | ||
| fi | ||
| if ! listing="$(phala_get_cvms)"; then | ||
| log "FATAL: post-teardown CVM list failed — count indeterminate (not success)" | ||
| echo '{"count":-1,"ids":[],"indeterminate":true}' | tee "${RUN_DIR}/cvms-final.json" | ||
| return 1 | ||
| fi | ||
| echo "${listing}" | tee "${RUN_DIR}/cvms-final.json" | ||
| local owned_left cnt indeterminate | ||
| cnt="$(python3 -c "import json;print(json.load(open('${RUN_DIR}/cvms-final.json')).get('count',-1))")" | ||
| indeterminate="$(python3 -c "import json;print(bool(json.load(open('${RUN_DIR}/cvms-final.json')).get('indeterminate')))")" | ||
| if [[ "${cnt}" == "-1" || "${indeterminate}" == "True" ]]; then | ||
| log "FATAL: CVM count indeterminate after teardown (count=${cnt})" | ||
| return 1 | ||
| fi | ||
| owned_left="$(python3 -c " | ||
| import json | ||
| from pathlib import Path | ||
| final=set(json.load(open('${RUN_DIR}/cvms-final.json')).get('ids') or []) | ||
| owned=set() | ||
| for p in ('${CVM_TRACK}','${OWNED_CVMS_FILE}'): | ||
| path=Path(p) | ||
| if path.is_file(): | ||
| owned |= {ln.strip() for ln in path.read_text().splitlines() if ln.strip() and not ln.strip().startswith('#')} | ||
| print(' '.join(sorted(owned & final))) | ||
| ")" | ||
| if [[ -n "${owned_left// /}" ]]; then | ||
| log "WARNING: owned CVMs still present after teardown: ${owned_left} (account count=${cnt})" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pruning the owned list before verification defeats the leftover check.
Lines 371-381 drop every targeted id from OWNED_CVMS_FILE, but the owned_left check at 395-405 re-reads that same file. Any CVM whose delete failed (the || true at line 367 swallows it) has already been removed from the owned set, so owned_left is empty and the run reports "teardown OK" while a billable CVM is still alive. Prune only after computing owned_left, and only for ids absent from the final listing.
🔧 Proposed reordering
- # Drop successfully targeted ids from durable owned list (best-effort).
- if [[ -f "${OWNED_CVMS_FILE}" && -n "${will_delete// /}" ]]; then
- local tmp_owned keep d
- tmp_owned="$(mktemp)"
- while read -r id; do
- [[ -n "$id" ]] || continue
- keep=1
- for d in ${will_delete}; do [[ "$id" == "$d" ]] && keep=0 && break; done
- [[ "$keep" == "1" ]] && echo "$id"
- done <"${OWNED_CVMS_FILE}" >"${tmp_owned}" || true
- mv -f "${tmp_owned}" "${OWNED_CVMS_FILE}"
- fi
if ! listing="$(phala_get_cvms)"; then…then, after owned_left is computed and found empty, drop the targeted ids that are confirmed gone from the final listing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/scripts/staging/run_staging.sh` around
lines 370 - 409, Move the OWNED_CVMS_FILE pruning block out of the
pre-verification path in the teardown function. Compute owned_left from the
original tracking files and final CVM listing first, return failure when any
owned CVM remains, then prune only targeted IDs confirmed absent from the final
listing after the check succeeds; preserve the existing best-effort handling for
the cleanup write.
| log "miner zip ok hash=${zip_hash}" | ||
|
|
||
| if [[ "${SKIP_BUILD}" != "1" ]]; then log "building runtime image"; ${COMPOSE} build agent-challenge; fi | ||
| chmod a+r "${CONFIG_DIR}/challenge_token" "${CONFIG_DIR}/review_evidence_encryption_key" 2>/dev/null || true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Permissions are widened to other where group/uid ownership would suffice. Both sites bridge the host↔container uid mismatch by granting world access to sensitive material, exposing it to every local user. Use chgrp plus group bits, or chown to the container uid (10001), in both places.
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L557-L557: replacechmod a+ronchallenge_tokenandreview_evidence_encryption_keywith group-scoped read for the container uid.packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L475-L483: replacechmod a+rwon the staging SQLite DB andchmod a+rwxon its volume directory with group-scoped access shared with the KR process.
As per coding guidelines: "Never expose private keys, wallet mnemonics, API tokens, or full secret values in logs, documentation, or evidence."
📍 Affects 1 file
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L557-L557(this comment)packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L475-L483
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/scripts/staging/run_staging.sh` at line
557, Replace the world-readable permissions in run_staging.sh at
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L557-L557 for
challenge_token and review_evidence_encryption_key with group-scoped read access
shared with container uid 10001, using chgrp or equivalent ownership setup. Also
update
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L475-L483 to
replace world read/write access on the staging SQLite database and world
read/write/execute access on its volume directory with group-scoped permissions
shared with the KR process; preserve required functionality without exposing
sensitive materials to other local users.
Sources: Coding guidelines, Linters/SAST tools
| awk -v h="${PUB_HOST}" 'index($0, h)==0 || $0 !~ (h "$") {print}' /etc/hosts > /tmp/hosts.staging.$$ || true | ||
| # Safer: drop lines ending with the hostname | ||
| grep -v -E "[[:space:]]${PUB_HOST}$" /etc/hosts > /tmp/hosts.staging.$$ || cp /etc/hosts /tmp/hosts.staging.$$ || true | ||
| echo "${PUB_IP} ${PUB_HOST}" >> /tmp/hosts.staging.$$ | ||
| cp -f /tmp/hosts.staging.$$ /etc/hosts | ||
| rm -f /tmp/hosts.staging.$$ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Insecure predictable temp file used to rewrite /etc/hosts. Both /etc/hosts mutation paths stage the new file at /tmp/hosts.staging.$$, a guessable name in a world-writable directory, then copy it over /etc/hosts as root — a symlink/pre-creation race. One mktemp-based helper fixes both.
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L660-L665: drop the deadawkwrite on line 660 and stage the filtered hosts file viatmp="$(mktemp)"before copying to/etc/hosts.packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L506-L514: replace/tmp/hosts.staging.$$in the teardown unpin path with the samemktemp-created file.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 661-661: Building a temp file path in a world-writable directory from the PID ($$) or `` is predictable and racy: an attacker can pre-create or guess the name and win a symlink/race attack. Use mktemp (e.g. `f=$(mktemp)` or `f=$(mktemp /tmp/myapp.XXXXXX)`) so the kernel atomically creates a unique, unpredictable file.
Context: /tmp/hosts.staging.$$
Note: [CWE-377] Insecure Temporary File.
(tmp-file-pid-name-bash)
[warning] 661-661: Building a temp file path in a world-writable directory from the PID ($$) or `` is predictable and racy: an attacker can pre-create or guess the name and win a symlink/race attack. Use mktemp (e.g. `f=$(mktemp)` or `f=$(mktemp /tmp/myapp.XXXXXX)`) so the kernel atomically creates a unique, unpredictable file.
Context: /tmp/hosts.staging.$$
Note: [CWE-377] Insecure Temporary File.
(tmp-file-pid-name-bash)
[warning] 662-662: Building a temp file path in a world-writable directory from the PID ($$) or `` is predictable and racy: an attacker can pre-create or guess the name and win a symlink/race attack. Use mktemp (e.g. `f=$(mktemp)` or `f=$(mktemp /tmp/myapp.XXXXXX)`) so the kernel atomically creates a unique, unpredictable file.
Context: /tmp/hosts.staging.$$
Note: [CWE-377] Insecure Temporary File.
(tmp-file-pid-name-bash)
[warning] 663-663: Building a temp file path in a world-writable directory from the PID ($$) or `` is predictable and racy: an attacker can pre-create or guess the name and win a symlink/race attack. Use mktemp (e.g. `f=$(mktemp)` or `f=$(mktemp /tmp/myapp.XXXXXX)`) so the kernel atomically creates a unique, unpredictable file.
Context: /tmp/hosts.staging.$$
Note: [CWE-377] Insecure Temporary File.
(tmp-file-pid-name-bash)
📍 Affects 1 file
packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L660-L665(this comment)packages/challenges/agent-challenge/scripts/staging/run_staging.sh#L506-L514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/scripts/staging/run_staging.sh` around
lines 660 - 665, Replace the predictable /tmp/hosts.staging.$$ staging in
run_staging.sh lines 660-665 with a mktemp-created file, remove the dead awk
write, and continue filtering and copying the result to /etc/hosts with cleanup.
Apply the same mktemp-based staging fix to the teardown unpin path at lines
506-514; both sites require direct changes using the generated temporary
filename consistently.
Source: Linters/SAST tools
| eval "$(python3 - <<PY | ||
| import json | ||
| from pathlib import Path | ||
| p=Path("${RUN_DIR}/review-result-a${attempt}-${i}.json") | ||
| try: | ||
| o=json.loads(p.read_text()) | ||
| except Exception: | ||
| o={} | ||
| items=o.get("items") if isinstance(o,dict) else None | ||
| it=items[0] if isinstance(items,list) and items else {} | ||
| rc=str(it.get("reason_code") or "") if isinstance(it,dict) else "" | ||
| rt=it.get("retryable") if isinstance(it,dict) else False | ||
| print(f'reason_code={rc!r}') | ||
| print(f'retryable={"true" if rt else "false"!r}') | ||
| PY | ||
| )" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
eval on server-controlled JSON is a command-injection sink.
reason_code comes from the review API response. Python's !r switches to double quotes when the value contains a single quote, so a crafted reason_code breaks out of the shell quoting and executes arbitrary commands in a script that holds the Phala API key and (in this environment) root. Emit the values to a file and read them, or parse with python3 -c into separate command substitutions.
🔧 Proposed change
- eval "$(python3 - <<PY
-import json
-from pathlib import Path
-p=Path("${RUN_DIR}/review-result-a${attempt}-${i}.json")
-try:
- o=json.loads(p.read_text())
-except Exception:
- o={}
-items=o.get("items") if isinstance(o,dict) else None
-it=items[0] if isinstance(items,list) and items else {}
-rc=str(it.get("reason_code") or "") if isinstance(it,dict) else ""
-rt=it.get("retryable") if isinstance(it,dict) else False
-print(f'reason_code={rc!r}')
-print(f'retryable={"true" if rt else "false"!r}')
-PY
-)"
+ _hist_json="${RUN_DIR}/review-result-a${attempt}-${i}.json" \
+ read -r reason_code retryable < <(HIST_JSON="${RUN_DIR}/review-result-a${attempt}-${i}.json" python3 - <<'PY'
+import json, os, re
+from pathlib import Path
+try:
+ o=json.loads(Path(os.environ["HIST_JSON"]).read_text())
+except Exception:
+ o={}
+items=o.get("items") if isinstance(o,dict) else None
+it=items[0] if isinstance(items,list) and items else {}
+rc=str(it.get("reason_code") or "") if isinstance(it,dict) else ""
+rc=re.sub(r"[^A-Za-z0-9_.:-]", "_", rc) or "unknown"
+rt=it.get("retryable") if isinstance(it,dict) else False
+print(rc, "true" if rt else "false")
+PY
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| eval "$(python3 - <<PY | |
| import json | |
| from pathlib import Path | |
| p=Path("${RUN_DIR}/review-result-a${attempt}-${i}.json") | |
| try: | |
| o=json.loads(p.read_text()) | |
| except Exception: | |
| o={} | |
| items=o.get("items") if isinstance(o,dict) else None | |
| it=items[0] if isinstance(items,list) and items else {} | |
| rc=str(it.get("reason_code") or "") if isinstance(it,dict) else "" | |
| rt=it.get("retryable") if isinstance(it,dict) else False | |
| print(f'reason_code={rc!r}') | |
| print(f'retryable={"true" if rt else "false"!r}') | |
| PY | |
| )" | |
| _hist_json="${RUN_DIR}/review-result-a${attempt}-${i}.json" \ | |
| read -r reason_code retryable < <(HIST_JSON="${RUN_DIR}/review-result-a${attempt}-${i}.json" python3 - <<'PY' | |
| import json, os, re | |
| from pathlib import Path | |
| try: | |
| o=json.loads(Path(os.environ["HIST_JSON"]).read_text()) | |
| except Exception: | |
| o={} | |
| items=o.get("items") if isinstance(o,dict) else None | |
| it=items[0] if isinstance(items,list) and items else {} | |
| rc=str(it.get("reason_code") or "") if isinstance(it,dict) else "" | |
| rc=re.sub(r"[^A-Za-z0-9_.:-]", "_", rc) or "unknown" | |
| rt=it.get("retryable") if isinstance(it,dict) else False | |
| print(rc, "true" if rt else "false") | |
| PY | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/scripts/staging/run_staging.sh` around
lines 826 - 841, Remove the eval-based assignment around the Python JSON parsing
block in the staging result handling. Safely transfer reason_code and retryable
from the parsed response using a file or separate command substitutions with
robust shell-safe handling, preserving the existing defaults for malformed
responses and missing items without allowing API-controlled reason_code content
to execute as shell syntax.
Source: Linters/SAST tools
| while True: | ||
| path = f"/cvms/paginated?page={page}&page_size={page_size}" | ||
| raw = self.get(path) | ||
| snap = parse_cvms_list_response(raw) | ||
| if reported_total is None: | ||
| reported_total = snap.total | ||
| elif snap.total != reported_total: | ||
| raise CvmListParseError( | ||
| "unrecognized CVM list shape: total changed across pages " | ||
| f"({reported_total} -> {snap.total})" | ||
| ) | ||
| all_items.extend(snap.items) | ||
| if reported_total <= len(all_items): | ||
| break | ||
| if not snap.items: | ||
| raise CvmListParseError( | ||
| "unrecognized CVM list shape: empty page before total reached " | ||
| f"(have={len(all_items)} total={reported_total})" | ||
| ) | ||
| page += 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pages are merged without identity checks; a server that ignores page yields duplicates.
If the endpoint returns page 1 repeatedly, all_items accumulates duplicate rows until len(all_items) >= reported_total, and the merged snapshot silently reports the same CVM twice — bad input for owned-only teardown planning. Consider tracking seen CVM ids and failing closed on repeats (or asserting the echoed page field matches the requested page).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/challenges/agent-challenge/src/agent_challenge/selfdeploy/phala.py`
around lines 305 - 324, Update the paginated CVM aggregation loop around
parse_cvms_list_response to track each CVM’s unique ID and raise
CvmListParseError when an ID repeats across pages, before extending all_items.
Preserve normal pagination and total-consistency validation, failing closed
instead of returning duplicate rows when the server repeats a page.
1 participant
Summary
main(post-feat(agent-challenge): CVM sizing, token handoff, and Phala eval deploy hardening #49). Only still-needed selfdeploy/staging safety work; no prism/constation/Lium/master/DB stack.Production safety defects addressed
work/owned_cvms.txt+ per-run track), logs the delete plan first, and never account-sweeps.GET /api/v1/cvmscan return[]while inventory is non-empty. Authority isGET /cvms/paginatedwithX-Phala-Version: 2026-06-23. Unknown envelopes raise; indeterminate count is teardown FAILURE (never success with total=0).Changes
cvm_list.py+PhalaCloudClient.list_cvmsvia/cvms/paginated)http://only withSELFDEPLOY_ALLOW_INSECURE_LOOPBACK=1for 127.0.0.1/localhost/::1scripts/staging/(secrets gitignored;.exampleonly) +docs/staging.md+docs/prod-compose-upgrade.mdExplicitly dropped (main already covers / not portable)
format_eval_shape_mismatch_errortdx.xlargedefaultX-Phala-Versionheader presence2026-01-21; this PR pins CLI2026-06-23+ paginated listdaf0f209…without broader compose deltauv.lockStaging / execution proof honesty
daf0f209…was measured withoutCHALLENGE_PHALA_EVAL_ARTIFACT_{URL,TOKEN}inallowed_envs. Proven on submission 13 whose deployencrypted_env_namescontained neither — a matchingguest_artifact_proofis structurally impossible on that pin.3a81feaf…(current image + artifact envs only).Test plan
origin/main(2bfa110c): 3221 passed, 62 failed, 6 errors, 8 skipped (env/docker/keyrelease pre-existing)test_phala_cvms_list_parse, loopback policy, teardown policy, teardown http, create ack)ruff checkclean on touched Python filesnpx phala@latest cvms list --json→ total 0.exampleconfigs)CI
Notes
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests