Ask a human before a Publication destroys something#1465

merged10

main

approvals/p2-review-digest

openedOct 8, 2026byNNick

View on GitHubGitHub import enabledPR last imported 21d ago

Draft. P2 of the agent-approvals program.

An agent working through Cloud can now be stopped before a Publication destroys something. When Cloud marks a request as needing approval, the Config Store refuses any Publish or manual Deploy whose Review removes a deployed Service, deletes a deployed Volume, detaches a Volume from a deployed Service, or removes a deployed domain. The refusal names what would be destroyed and carries a digest a human approves. Nothing sets the requirement yet (Cloud does in P3), so the CLI and the dashboard behave exactly as on trunk.

What changes

The Publication Review (DiffView) gains effects, the set of Destructive Effects it would cause. Each effect is a kind (removes_service, deletes_volume, detaches_volume, removes_domain), the name a human uses (the Service, the Volume, or the domain's hostname), a node and a path. The refusal reads, for example, "A human must approve this first: remove domain app.example.com". name reads as empty from a Cloud that does not send it, so a newer CLI still runs diff against an older Cloud. Effects compare against what the Environment last applied, so removing a Service that never deployed is not destructive. A detach of a Volume that is also being deleted folds into the delete.

Trusted gains approval, one of NotRequired (the default), Required, or Approved(digest). It lives on Trusted because only Cloud may set it. A caller cannot switch its own approval off.

The digest is <review version>:<hash of organization, environment and effects>. An approval matches only the Review it was made against. If the Working revision moves, the old digest is refused and the refusal carries the fresh one.

The refusal is code approval_required with details {effects, approval, diff}. It carries no retry command; the CLI builds that from approval in P4. The code is an open string (RpcErrorCode::from), so ployz-core needs no new variant.

The gate runs only when the write creates a new Saved revision. Deploying already-Saved State, retrying and starting never ask. Settings edits never ask.

A removal that has not deployed yet asks again on every later Publication that still carries it. Earlier approvals are not remembered.

Tests

ployz-store tests cover each effect kind, a Service roll that destroys nothing, a Publish that waits for exactly its digest, a stale digest refused with the fresh one, a Deploy that removes a deployed Service, and a Deploy or retry of Saved State that asks nothing. Removing the resave check or inverting the digest comparison makes the deployment tests fail.

A small fix rides along: the test backend built 128-character container IDs past 16 Services. It now formats them as {index:064x}.

Perf

No measurable difference. A release-build probe publishes 50 staged changes over 200 deployed Services, 200 times. Trunk median 36.3 ms per publish (33.6 to 45.7 ms), head median 35.4 ms (30.9 to 39.2 ms), five alternated runs each on a loaded machine. Time goes to parsing Service config through serde_json::Value; computing effects is 0.03% of samples.

Live lanes

Run on an Incus verify-server cluster. With approval not required (the only path the CLI reaches until P3), publish and deploy behave as on trunk, and the only output change is "effects": [] in --json diff. diff --json lists the right effect for a Service removal, a Volume delete, a detach, and a domain removal, and lists none for an image or replicas change. Store-level runs cover the refusal and a stale digest refused with the fresh one. The plain-text diff does not summarize effects yet; that lands with the CLI work in P4.

Checks

2 failing · 23 successful

Some checks failed

Failing2

CI readyGitHub
Failed
rust-testsGitHub
Failed

Rrailway-appBotOct 8, 2026

🚅 Environment ployz-pr-1465 in Ployz Dashboard has no services deployed.

Cchatgpt-codex-connectorBotOct 10, 2026

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-11T18:39:13.712169Z	`0a7e3d3`	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Cchatgpt-codex-connectorBotreviewed · 2 inline threadsOct 10, 2026

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6b4ee9df7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

core/crates/ployz-store/src/review/approval.rslines 87–89Outdated · b6b4ee9Open thread

…	…	@@ -0,0 +1,262 @@
	86	+	continue;
	87	+	};
	88	+	let (into, from) = (&mut running.config, &service.config);
	89	+	add_missing(&mut into.routes, &from.routes, \|route\| &route.hostname);

Cchatgpt-codex-connectorBoton lines 87–89Oct 10, 2026

_{_{P2 Badge}} Refresh names when merging in-flight services

When a deployed Service is renamed and that rename is admitted but has not finished, this same-ID branch preserves the Applied State slug and only merges selected configuration collections. If the user then removes the renamed Service, destructive_effects reports the old slug for both name and path, while the Diff row and in-flight target use the new slug; the approval prompt therefore identifies the wrong human-facing Service and no longer names the row shown by diff. Preserve the newest in-flight display name while retaining the union of destructive properties.

Useful? React with 👍 / 👎.

NNickOct 11, 2026

Intended. Until the in-flight rename lands, the old name is the one that is still running, and the digest keys on node IDs rather than names.

core/crates/ployz-store/src/review/approval.rslines 258–260Outdated · b6b4ee9Open thread

…	…	@@ -0,0 +1,262 @@
	257	+	.join(", ");
	258	+	Err(error::approval_required(
	259	+	format!("A human must approve this first: {asked}"),
	260	+	json!({"effects": effects, "approval": digest, "diff": diff}),

Cchatgpt-codex-connectorBoton lines 258–260Oct 10, 2026

_{_{P2 Badge}} Terminate the approval-required message with a period

Whenever approval is required, this newly introduced failure message ends with the final effect name rather than a period. core/CODING_STANDARDS.md requires every failure's first line to be a capitalized sentence ending with a period, so the malformed message propagates to both human and JSON error output; append the final punctuation and update the exact-message tests.

AGENTS.md reference: AGENTS.md:L3-L3

Useful? React with 👍 / 👎.

NNickOct 11, 2026

Punctuation nit with no behavior effect. The message is now at core/crates/ployz-store/src/review/approval.rs:262.

Cchatgpt-codex-connectorBotreviewed · 1 inline threadOct 10, 2026

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91d32c1469

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

core/crates/ployz-store/src/review/approval.rsline 78Outdated · 91d32c1Open thread

…	…	@@ -0,0 +1,262 @@
	75	+	.any(\|mount\| mount.volume_resource_id == volume.resource_id)
	76	+	})
	77	+	});
	78	+	add_missing(&mut deployed.volumes, mounted, \|volume\| &volume.resource_id);

Cchatgpt-codex-connectorBoton line 78Oct 10, 2026

_{_{P2 Badge}} Keep the newest Volume name when merging in-flight state

When a mounted Volume is renamed and that rename is in flight, Applied State already contains the same resource_id, so add_missing discards the in-flight copy and preserves the old name. If the user then removes the renamed Volume, destructive_effects reports delete Volume OLD_NAME with volumes.OLD_NAME, even though the current diff and in-flight target use the new name; update the retained Volume's display name while preserving the union of deployed resources.

Useful? React with 👍 / 👎.

NNickOct 11, 2026

Intended for the same reason as the service rename. The Applied name is what still holds the data, and the digest keys on node IDs.

Added 10 commitsOct 11, 2026

Ask a human before a Publication destroys something01c6b45
Regenerate approvals declarations with Config review fields0a7e3d3
Keep Setting edits out of Approval in the glossary2889331
Read Destructive Effects from a Cloud that sends no name43dfac3
Give each container in a test run its own 64-digit ID5e6df26
Count every Deployment in flight as deployed for approval9f85ac2
Name the Service, Volume or hostname each Destructive Effect destroysc8898f7
Ask for removals against what runs, not what was submittedcec3424
Name Approval and Destructive Effects in the domain glossaryee4c628
Ask for removals of what the Deployment in flight puts in placef8d7259

CcoderabbitaiBotOct 11, 2026

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 47828582-bf35-498c-b36e-e9a52726e0dd

📥 Commits

Reviewing files that changed from the base of the PR and between 1aa34abbbb1cd20cf311916499c4cea8370063ff and 0a7e3d35a017633d897e3e0a086f061e2f2f4ad1.

⛔ Files ignored due to path filters (1)

core/crates/ployz-sdk/generated/payloads.d.ts is excluded by !**/generated/**

📒 Files selected for processing (15)

core/CONTEXT.md
core/crates/ployz-store/src/deployment/admit.rs
core/crates/ployz-store/src/deployment/history.rs
core/crates/ployz-store/src/deployment/mod.rs
core/crates/ployz-store/src/error.rs
core/crates/ployz-store/src/id.rs
core/crates/ployz-store/src/lib.rs
core/crates/ployz-store/src/review/approval.rs
core/crates/ployz-store/src/review/mod.rs
core/crates/ployz-store/src/review/publish.rs
core/crates/ployz-store/src/trusted.rs
core/crates/ployz-store/tests/backend/mod.rs
core/crates/ployz-store/tests/deployment.rs
core/crates/ployz-store/tests/domain.rs
core/crates/ployz-store/tests/review.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Summary

Summary by CodeRabbit

New Features
- Environment reviews now identify destructive changes, including service removal, volume deletion or detachment, and removal of deployed domains.
- Publishing or deploying changes that require approval now checks an approval tied to the reviewed changes. If approval is missing or outdated, the action is refused and the review provides updated change details.
- Deployment reviews account for multiple in-progress deployments when showing affected resources and domains.

Walkthrough

The review now reports destructive effects based on applied and in-flight deployment state. Publish and manual Deploy check approval against the current diff version and effect set before saving or publishing.

Changes

Destructive change review and approval

Layer / File(s)	Summary
Load in-flight deployment state `core/crates/ployz-store/src/deployment/history.rs`, `core/crates/ployz-store/src/deployment/mod.rs`, `core/crates/ployz-store/src/review/mod.rs`	`Head` now records all in-flight deployment targets. The newest target supplies overlaid intent, and cluster-domain lookup combines applied and in-flight deployment data.
Derive destructive effects `core/crates/ployz-store/src/review/approval.rs`, `core/crates/ployz-store/src/review/mod.rs`, `core/crates/ployz-store/tests/review.rs`, `core/crates/ployz-store/tests/domain.rs`, `core/CONTEXT.md`	`DiffView` reports effects for removed services, deleted or detached volumes, and removed routes or managed domains. Tests cover effect calculation and deployed hostname resolution.
Bind approval and enforce checks `core/crates/ployz-store/src/trusted.rs`, `core/crates/ployz-store/src/id.rs`, `core/crates/ployz-store/src/error.rs`, `core/crates/ployz-store/src/lib.rs`, `core/crates/ployz-store/src/deployment/admit.rs`, `core/crates/ployz-store/src/review/publish.rs`, `core/crates/ployz-store/tests/deployment.rs`, `core/crates/ployz-store/tests/review.rs`, `core/crates/ployz-store/tests/backend/mod.rs`	`Approval` supports required, not-required, and digest-approved states. Publish and Deploy check approvals; tests cover stale and repeated approvals, approval bypass, and deployment cases. The backend test fixture formats container IDs as 64-character lowercase hexadecimal values.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature · Unblocks: 18 PRs

Sequence Diagram(s)

mermaid

1	sequenceDiagram
2	participant Publisher
3	participant Publish
4	participant ApprovalCheck
5	participant StateWriter
6	Publisher->>Publish: Submit target and Trusted approval
7	Publish->>ApprovalCheck: Check target and approval
8	ApprovalCheck-->>Publish: Allow or approval_required
9	Publish->>StateWriter: Save target when allowed

Merge Risk | ⚪ Minimal · up to 0a7e3

Merge Risk: ⚪ Minimal · up to 0a7e3

No actionable merge-blocking issue is established for this PR. It is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 0a7e3

The approval mechanism is bound to the reviewed changes, but interrupted deployments can leave resources outside its approval calculation. Protection is not enabled by default, limiting immediate exposure. Recovery handling and the trusted source of approval need validation before activation.

Retained concerns

Medium · security · inferred: The new destructive-effect calculation excludes Unknown and lease-lapsed deployment targets. If execution installed a resource before its confirmation was persisted, recovery publication can omit its removal from the effect set, and Required accepts an empty set. Full deployment reconciles the namespace, so this creates a potential unapproved destructive recovery path. The older Unknown-state handling is unchanged, but its use as an authorization basis is new. Actual runtime exploitation remains unverified, and ordinary callers retain NotRequired behavior until protection is enabled.

Security review details

Security Blast Radius

inferred — The relevant destructive scope is an environment's services, domains, mounted storage, and deleted volumes. A full deployment reconciles the namespace, so incomplete authorization review can affect more than the directly edited service. The evidence does not establish cross-tenant privilege expansion.

Security Findings and Attack Paths

inferred — A caller already authorized to edit and publish an environment could encounter an interrupted deployment whose runtime additions were never confirmed. Those targets are excluded once Unknown or lease-lapsed; a recovery target that removes them may therefore pass Required with no effects. This is a conditional recovery concern, not a reproduced exploit.

Trust Boundaries and Controls

observed — Approval is supplied separately from command payloads through the existing in-process trusted-evidence interface. The CLI Cloud path does not transmit caller-selected Trusted values. The SDK accepts trusted JSON from its host, so authenticating that host and constructing approval evidence remain Cloud responsibilities outside the inspected implementation.

Resilience and Maintainability Implications

observed — Successfully confirmed node outcomes advance Applied State, including partial progress. Cancellation is state-aware, and repeated execution evidence must match the recorded outcome. These controls preserve known outcomes but do not establish what happened before an interrupted confirmation.

Hardening Proposals

proposed — Before activating Required, conservatively include potentially installed Unknown targets or require trusted reconciliation of their outcomes. Validate interruption between runtime mutation and confirmation, lease expiry, recovery publication, and subsequent full deployment. Separately validate that external request data cannot choose approval policy or attest human assent.

Pre-merge checks | ^Passed 4 | ^Failed 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	Warning	Docstring coverage is 61.02% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 14 files. (1 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	Passed	The title clearly and concisely summarizes the main change: requiring human approval before a Publication causes destructive changes.
Description check	Passed	The description is directly related to the changeset and explains the approval gate, destructive effects, digest behavior, scope, tests, and performance results.
Linked Issues check	Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 61.02% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 14 files. (1 skipped: 1 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

Cchatgpt-codex-connectorBotreviewed · 1 inline threadOct 11, 2026

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a7e3d35a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

core/crates/ployz-store/src/deployment/history.rslines 166–167Open thread

…	…	@@ -52,64 +52,122 @@ pub(crate) fn head(tx: &mut dyn Tx, environment: &Environment) -> Result<Head, R
	164	+	for managed in &service.config.managed_hostnames {
	165	+	domains
	166	+	.entry((service.id.clone(), managed.prefix.clone()))
	167	+	.or_insert_with(\|\| cluster.clone());

Cchatgpt-codex-connectorBoton lines 166–167Oct 11, 2026

_{_{P2 Badge}} Preserve every Cluster Domain for an in-flight prefix

When an applied Service serves a generated prefix under one Cluster Domain and an in-flight Deployment puts the same Service/prefix in place under a different Cluster Domain, this map keeps only the applied domain because running is chained first and or_insert_with discards the in-flight value. Removing the generated domain then reports and hashes only the old hostname, even though the in-flight Deployment may still install the new hostname, so a human can approve a destructive set that omits a domain the publication removes; retain all distinct domains for each Service/prefix and emit an effect for each hostname.

Useful? React with 👍 / 👎.

NNickOct 11, 2026

Edge case we accept. It needs a Cluster Domain change while a deploy is in flight, and the next deploy reports the right domain.

CcoderabbitaiBotapproved these changesOct 11, 2026

NNickreviewedOct 11, 2026