Implement get_deployment_payload and delete_deployment_payload operations

[kriszyp]

Both operations have been documented (operations-api reference + 5.1 release notes) and enum-declared since 5.1, but were never registered — calling them returned Operation 'delete_deployment_payload' not found (#1893, reported from a Fabric quota-cleanup workflow).

What this adds

• get_deployment_payload (components/deploymentOperations.ts) — streams the stored deployment tarball back as raw bytes with content-type: application/octet-stream + a content-disposition filename, via the existing Readable+headers pipe path in serverHandlers.js (the get_backup mechanism). Deliberately never base64: payloads can be hundreds of MB, and a base64 round-trip materializes multi-hundred-MB V8 strings (hard ERR_STRING_TOO_LONG near ~400MB). The stream is marked preCompressed so the already-gzipped tar isn't re-gzipped.
• delete_deployment_payload — nulls payload_blob on the row and re-puts it; the RecordEncoder retained-blob check unlinks the local blob file and the replicated null makes every peer drop its copy too (the same cluster-wide reclaim mechanism as the automatic post-deploy retention drop from feat(deploy): auto-drop large deployment payload blobs after a successful deploy #1496). Row metadata + event_log are retained as the audit trail, with a payload_dropped event recording deleted_by. Guards: 409 on non-terminal deployments (their blob may still be the replication channel peers install from); idempotent success (freed_bytes: 0) when the payload is already gone; 404 for unknown ids.
• Registration + authorization for both, mirroring get_deployment's super_user permission shape.

Hardening found by review (in this PR)

• serverHandlers.js gzip pipe now forwards source-stream errors. .pipe() doesn't propagate them, so an async read error on a returned stream (e.g. get_backup's file stream) fired 'error' with no listener → uncaughtException → whole worker exits. Latent before this PR; blob streams made it much more reachable. Now the error destroys the gzip stream and Fastify aborts just that response.
• MCP operations adapter returns a clean isError for streaming (Readable) results instead of JSON.stringify-ing stream internals, and destroys the stream so file-backed sources release fds. delete_deployment_payload added to DESTRUCTIVE_OPERATIONS. (Neither payload op is in the MCP default-allow surface; this covers explicit mcp.operations.allow opt-ins, and get_backup as well.)

Considered decisions (flagging for review)

• Delegability: both ops use get_deployment's permission shape, meaning an SU can grant them to a role via its operations allowlist (gate-2). Considered self-enforcing super_user like the secrets ops (tarballs can embed secrets), but delegation requires a deliberate per-op SU grant, and a non-SU cleanup-automation principal is exactly the delete_deployment_payload operation not implemented #1893 use case. Happy to tighten if reviewers disagree.
• Rollback/redeploy-by-reference is deliberately out of scope — that belongs with the two-phase deploy work in feat(deploy): two-phase stage/activate for deploy_component (draft/RFC) #1849 (design comment there).

Verification

• Unit: 105 passing across the four touched test files (new: handler behaviors, gzip error-forwarding, preCompressed pass-through, MCP streaming guard).
• Live smoke (scratch instance from this branch's dist): deployed 1.2MB payload → real blob file on disk; get_deployment_payload byte-identical round-trip (sha256 match) with and without Accept-Encoding: gzip; delete_deployment_payload → freed_bytes correct, blob file physically unlinked, payload_blob_present:false, payload_dropped audit event, idempotent second call, 404s on reclaimed/unknown.

Cross-model coverage: codex ✓ / gemini ✓ / Harper-domain pass ✓ (thorough), + final-artifact pass after the review fixes. All blockers/concerns from review addressed above.

Docs: companion PR documentation#600 adds the delete response shape, terminal-status requirement, and raw-bytes (not base64) clarification.

Fixes #1893

🤖 Generated with Claude Code (Opus 4.8)

[gemini-code-assist]

Code Review

This pull request introduces support for retrieving and deleting deployment payloads. It adds two new operations: get_deployment_payload (which streams the stored tarball as raw bytes) and delete_deployment_payload (which deletes the payload while retaining metadata and event logs for auditing). It also updates the server handlers to bypass recompression for pre-compressed streams and forward stream errors to prevent worker crashes. The review feedback suggests using pipeline from node:stream instead of .pipe() when chaining streams in serverHandlers.js to ensure proper stream destruction and prevent resource leaks if a client disconnects.

[claude]

Reviewed; no blockers found.

[kriszyp]

Reply to the components/mcp/tools/operations.ts thread (posting as a top-level comment — a direct review-reply collided with your in-progress pending review):

Good catch, and confirmed the mechanism exactly: Readable.from()'s _destroy calls iterator.return(), and per the async-generator spec, calling .return() on a generator that never had .next() called resolves it straight to done without entering the body — so readTxn.done() never runs.

Fixed in 9ba73f1 with your first option: STREAMING_OPERATIONS (get_backup, get_deployment_payload) now short-circuits before chooseOperation/processLocalTransaction are even called, so the fd/txn are never opened for a rejected MCP call. Kept the instanceof Readable check as a defense-in-depth backstop for an op outside that set unexpectedly returning a stream (distinct message so the two paths are distinguishable), but that's belt-and-suspenders now, not the real fix. New test asserts chooseOperation is never reached for both known streaming ops.

Thread marked resolved. — Claude (Opus 4.8)

[kriszyp]

Reply to the claude-bot finding on components/deploymentOperations.ts:251 (handleDeleteDeploymentPayload spreading row) — posting top-level since a direct review-reply collides with the in-progress pending review.

Investigated by instrumenting the live code path rather than trusting either the finding or my own earlier smoke test at face value, since they seemed to disagree. Booted a scratch instance from this branch, deployed a payload, and logged row.constructor.name, Object.keys(row), Object.keys({...row}), and row.getId right before the delete write. Results:

ctor: "RecordObject"
ownKeys: ["deployment_id","project","package_identifier","payload_hash","payload_size",
          "payload_blob","status","phase","event_log","peer_results","origin_node",
          "restart_mode","started_at","completed_at","user","rollback_of","credentials",
          "error","__updatedtime__","__createdtime__"]
spreadKeys: <identical to ownKeys>
getIdFn: "undefined"   // no such method

table.get(id) here — a direct programmatic call on the Table object (databases.system.hdb_deployment), not a Resource/REST-mediated request — returns a plain decoded storage record (RecordObject) with every declared attribute as a genuine own property, not a TableResource instance backed by the assignTrackedAccessors prototype getters the finding described (that mechanism applies to the Resource/HTTP-request layer, a different code path than a direct table.get() call). {...row} therefore does carry every field, data[this.primaryKey] resolves correctly on .put(), and there is no getId() to fall back on because none is needed. The finding's premise doesn't hold for this call site — the earlier live round-trip test (byte-identical payload, correct audit event, idempotent re-delete, all against the same row) is consistent with this, not contradicting it as it first appeared.

No code change from this thread. — Claude (Opus 4.8)

[kriszyp]

Reply to cb1kenobi's finding on utility/operation_authorization.ts:299 (get_deployment_payload delegability) — posting top-level since a direct review-reply collides with the in-progress pending review.

Agreed, and this was exactly the open question I flagged in the PR description ("Happy to tighten if reviewers disagree"). Fixed in fba0fcb: get_deployment_payload now self-enforces super_user in the handler, mirroring secretOperations.requireSuperUser — it can no longer be delegated to a non-SU role via the operations allowlist (gate-2), regardless of what a role grants. delete_deployment_payload is intentionally left as-is per your scoped recommendation — the #1893 non-SU cleanup-automation use case only needs delete, which never exposes source/secrets.

Verified live against a real non-SU role granted both ops via add_role: get_deployment_payload now 403s for it, delete_deployment_payload still succeeds for it, and an SU caller still gets a byte-identical download. New unit tests cover the forbidden path for both no-hdb_user and non-SU-role callers.

Thanks for catching this — nice catch on the asymmetry with stripBlob. — Claude (Opus 4.8)

[cb1kenobi]

LGTM