Conversation
Documents the deletionPolicy: Retain trap that leaks EBS disks, the patch-before-delete ordering, the workspace-Kustomization reconcile target, the resource-disappearance poll, the namespace procedures, and the cleanup path for already-orphaned SeiNodes and volumes. Co-authored-by: omnigent <noreply@omnigent.ai>
… on deletionPolicy Post-merge reconciliation targeted `flux-system`, which tracks the platform repo — a workspace-repo merge was verified against a reconciler that never applied it. Route workspace merges to `Kustomization <alias>` in `eng-<alias>` and keep `flux-system` for platform merges. Add Guardrail #10, a PR-based teardown procedure with the patch-before-delete ordering and a resource-disappearance poll, three halt conditions, and the `teardown.md` reference-index entry. Co-authored-by: omnigent <noreply@omnigent.ai>
Recipes #9 (disappearance poll) and #10 (orphaned-SeiNode detection); `bench:teardown` now reconciles the engineer's Kustomization and confirms the objects went away. The SeiNetwork CRD reference documents deletionPolicy as the disk-leak field, its mutability, and the closing window. Chain-flow, comparative-bench, preflight, troubleshooting, and the package README point at the gated procedure. Two evals cover the Retain halt and the clean teardown. Co-authored-by: omnigent <noreply@omnigent.ai>
PR SummaryLow Risk Overview The skill now documents that default Post-merge behavior is corrected: workspace-repo merges must reconcile Flux Teardown is explicitly git-first: set Reviewed by Cursor Bugbot for commit 2218e68. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is made'. ASD-STE100 asks for the active voice.
Descriptive sentence has 29 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'be deleted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 35 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 37 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is preserved'. ASD-STE100 asks for the active voice.
Passive voice: 'are required'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is burned'. ASD-STE100 asks for the active voice.
Passive voice: 'are applied'. ASD-STE100 asks for the active voice.
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
Passive voice: 'be gone'. ASD-STE100 asks for the active voice.
Passive voice: 'are held'. ASD-STE100 asks for the active voice.
'very' is a weasel word!
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'is attached'. ASD-STE100 asks for the active voice.
Passive voice: 'was provisioned'. ASD-STE100 asks for the active voice.
Descriptive sentence has 42 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is governed'. ASD-STE100 asks for the active voice.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| - Defaults to GitOps: renders CRs via `--dry-run`, writes them under `engineers/<alias>/<task>/`, opens a PR, and lets Flux apply on merge — direct apply is a rare, double-confirmed escape hatch. | ||
| - Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and `git rm`-based teardown. | ||
| - Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and PR-based teardown. | ||
| - Gates teardown on the field that leaks disks. A `SeiNetwork` deleted under its default `spec.deletionPolicy: Retain` orphans the validator SeiNodes it generated — they keep running, keep their PVCs, and keep their EBS volumes, and nothing in git or in Flux will ever remove them. The skill reads the policy, patches it to `Delete` before the removal merges, verifies against the engineer's own Flux Kustomization rather than `flux-system`, and polls the resources to gone. |
There was a problem hiding this comment.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| - Defaults to GitOps: renders CRs via `--dry-run`, writes them under `engineers/<alias>/<task>/`, opens a PR, and lets Flux apply on merge — direct apply is a rare, double-confirmed escape hatch. | ||
| - Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and `git rm`-based teardown. | ||
| - Covers the full daily-driver surface: onboarding, chain spinup, RPC fleets, single and comparative benches, status reads, and PR-based teardown. | ||
| - Gates teardown on the field that leaks disks. A `SeiNetwork` deleted under its default `spec.deletionPolicy: Retain` orphans the validator SeiNodes it generated — they keep running, keep their PVCs, and keep their EBS volumes, and nothing in git or in Flux will ever remove them. The skill reads the policy, patches it to `Delete` before the removal merges, verifies against the engineer's own Flux Kustomization rather than `flux-system`, and polls the resources to gone. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
| | "What's running in my namespace" / "what chains do I have" | `seictl network list -n eng-<alias>` for the chains + `seictl node list -n eng-<alias>` for the followers (yaml default; `-o name` for short, `-o jsonpath=...` for one-shot field reads). | | ||
| | "Show me chain X" / "what's the status of X" | `seictl network get <name> -n eng-<alias>` for the network (`.status.phase`); `seictl node get <name>-rpc-<k> -n eng-<alias>` for a follower (`.status.phase`, `.status.endpoint`). | | ||
| | "Tear down chain X" | `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, which cascades to pods / PVCs per k8s deletion propagation. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. See `bench:teardown` recipe in `references/cluster-inspection-recipes.md` for the bench-specific variant. | | ||
| | "Tear down chain X" / "delete my bench" | **PR-based** (see Procedure: tear down). Read `spec.deletionPolicy` on every SeiNetwork in the task dir **first** — on `Retain` (the default) the teardown orphans the validators and leaks their disks. Then `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge → reconcile `<alias>` → poll until the objects are gone. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. | |
There was a problem hiding this comment.
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
| | "What's running in my namespace" / "what chains do I have" | `seictl network list -n eng-<alias>` for the chains + `seictl node list -n eng-<alias>` for the followers (yaml default; `-o name` for short, `-o jsonpath=...` for one-shot field reads). | | ||
| | "Show me chain X" / "what's the status of X" | `seictl network get <name> -n eng-<alias>` for the network (`.status.phase`); `seictl node get <name>-rpc-<k> -n eng-<alias>` for a follower (`.status.phase`, `.status.endpoint`). | | ||
| | "Tear down chain X" | `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, which cascades to pods / PVCs per k8s deletion propagation. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. See `bench:teardown` recipe in `references/cluster-inspection-recipes.md` for the bench-specific variant. | | ||
| | "Tear down chain X" / "delete my bench" | **PR-based** (see Procedure: tear down). Read `spec.deletionPolicy` on every SeiNetwork in the task dir **first** — on `Retain` (the default) the teardown orphans the validators and leaks their disks. Then `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge → reconcile `<alias>` → poll until the objects are gone. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. | |
There was a problem hiding this comment.
Descriptive sentence has 35 words. ASD-STE100 allows 25. Split it.
| | "What's running in my namespace" / "what chains do I have" | `seictl network list -n eng-<alias>` for the chains + `seictl node list -n eng-<alias>` for the followers (yaml default; `-o name` for short, `-o jsonpath=...` for one-shot field reads). | | ||
| | "Show me chain X" / "what's the status of X" | `seictl network get <name> -n eng-<alias>` for the network (`.status.phase`); `seictl node get <name>-rpc-<k> -n eng-<alias>` for a follower (`.status.phase`, `.status.endpoint`). | | ||
| | "Tear down chain X" | `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, which cascades to pods / PVCs per k8s deletion propagation. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. See `bench:teardown` recipe in `references/cluster-inspection-recipes.md` for the bench-specific variant. | | ||
| | "Tear down chain X" / "delete my bench" | **PR-based** (see Procedure: tear down). Read `spec.deletionPolicy` on every SeiNetwork in the task dir **first** — on `Retain` (the default) the teardown orphans the validators and leaks their disks. Then `git rm -r engineers/<alias>/<task>/` **and** remove `<task>` from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with a missing-resource entry). Commit → push → merge → reconcile `<alias>` → poll until the objects are gone. **Teardown does NOT purge the chain-id's S3 genesis artifacts — the chain-id is burned**; a later respin must use a fresh chain-id (see the naming step) or purge the `<chain-id>/` genesis-bucket prefix. | |
There was a problem hiding this comment.
Passive voice: 'is burned'. ASD-STE100 asks for the active voice.
|
|
||
| ### 9. Did the teardown actually remove the resources? | ||
|
|
||
| A Flux reconcile reports success once it issues the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll rather than assert once. |
There was a problem hiding this comment.
📝 [vale] <AgenticWriting.STE-NounCluster> reported by reviewdog 🐶
Noun cluster of more than three nouns: 'Flux reconcile reports success'. Break it with 'of' or 'for'.
| 14. **Poll bench Jobs to terminal** — both `seiload-<COMPARE_RUN_ID>-a` and `-b` to `Complete` or `Failed`. Deadline `<DURATION> * 60 + 660` seconds. | ||
| 15. **Fetch + render** — `aws s3 cp` both reports; extract metrics; render the side-by-side table. On any extraction gap, fall back to the raw-tail format with both S3 paths surfaced. | ||
| 16. **Teardown guidance** — `git rm -r engineers/<alias>/compare-<COMPARE_RUN_ID>/` and remove the entry from `engineers/<alias>/kustomization.yaml` `resources:`. Flux prunes both SeiNetworks, all follower SeiNodes, and both Jobs; child pods/PVCs cascade. | ||
| 16. **Teardown guidance** — run the procedure in `teardown.md` against `engineers/<alias>/compare-<COMPARE_RUN_ID>/`. **Both** SeiNetworks need the `deletionPolicy` gate before the removal PR opens: a comparison under the default `Retain` orphans two validator pools and leaks both sets of EBS disks. After that gate, `git rm -r` the dir, remove the entry from `engineers/<alias>/kustomization.yaml` `resources:`, merge, reconcile `kustomization <alias>` in `eng-<alias>`, and poll both chain-ids to gone. |
There was a problem hiding this comment.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
| ``` | ||
|
|
||
| **Why:** the post-merge reconcile pattern (`flux reconcile kustomization flux-system --with-source -n flux-system`) is the fast path from "PR merged" to "manifests applied in cluster." Without `flux`, the fallback is `kubectl annotate kustomization flux-system reconcile.fluxcd.io/requestedAt=$(date +%s) --overwrite -n flux-system`, which works but doesn't fetch the latest source revision in the same call. | ||
| **Why:** the post-merge reconcile pattern is the fast path from "PR merged" to "manifests applied in cluster." The target depends on which repo merged: a **workspace**-repo merge (every chain, bench, and teardown) goes to `flux reconcile kustomization <alias> -n eng-<alias> --with-source`, and only a **platform**-repo merge (onboarding) goes to `flux reconcile kustomization flux-system --with-source -n flux-system`. Reconciling `flux-system` for a workspace merge reconciles the platform repo and reports success without applying the engineer's change. Without `flux`, the fallback is `kubectl annotate kustomization <name> reconcile.fluxcd.io/requestedAt=$(date +%s) --overwrite -n <namespace>`, which works but does not fetch the latest source revision in the same call. |
There was a problem hiding this comment.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
| ## Deletion — `deletionPolicy` is the disk-leak field | ||
|
|
||
| `spec.deletionPolicy` defaults to `Retain` — it governs whether the controller orphans its generated validator SeiNodes on delete. This is orthogonal to the client-side `--cascade` propagation policy on `seictl network delete`; both apply. | ||
| `spec.deletionPolicy` defaults to **`Retain`**. Under `Retain` the controller does not delete the generated validator SeiNodes on deletion — it **strips their owner reference** and leaves them running. Each orphan keeps its PVC and its EBS disk, garbage collection has no owner reference left to follow, and Flux prune never reaches them because the controller created them and Flux never held them in its inventory. The teardown looks clean and the spend continues. |
There was a problem hiding this comment.
Descriptive sentence has 37 words. ASD-STE100 allows 25. Split it.
| `spec.deletionPolicy` defaults to `Retain` — it governs whether the controller orphans its generated validator SeiNodes on delete. This is orthogonal to the client-side `--cascade` propagation policy on `seictl network delete`; both apply. | ||
| `spec.deletionPolicy` defaults to **`Retain`**. Under `Retain` the controller does not delete the generated validator SeiNodes on deletion — it **strips their owner reference** and leaves them running. Each orphan keeps its PVC and its EBS disk, garbage collection has no owner reference left to follow, and Flux prune never reaches them because the controller created them and Flux never held them in its inventory. The teardown looks clean and the spend continues. | ||
|
|
||
| The storage class is not the lever. A `Delete` reclaim policy releases a disk only when the PVC is deleted, and an orphaned SeiNode never releases its PVC. |
There was a problem hiding this comment.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
…ore removal A kubectl patch against a Flux-owned SeiNetwork is drift. The rendered manifest normally carries the server-defaulted deletionPolicy: Retain, so Flux owns the field and reverts the patch on its next reconcile — typically while the removal PR is still in review. The engineer then merges a teardown they believe is safe and it orphans the validators. Document a policy-PR-then-removal-PR ordering with a reconcile and a read-back of both the live object and the committed file. The live patch stays only for a SeiNetwork no reconcile owns, and carries the re-verify-immediately-before-merge requirement. Also drop spec.resources from the immutable-field list: SeiNetworkSpec carries exactly three immutability rules — genesis, replicas, dataVolume — and has no resources field. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is governed'. ASD-STE100 asks for the active voice.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
|
|
||
| Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.import` on the SeiNode) by design. | ||
|
|
||
| Keep `Retain` only to preserve a validator's disk for forensics after the network goes away, and say so where the choice is made — a retained disk is a cost somebody chose. |
There was a problem hiding this comment.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
|
|
||
| Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.import` on the SeiNode) by design. | ||
|
|
||
| Keep `Retain` only to preserve a validator's disk for forensics after the network goes away, and say so where the choice is made — a retained disk is a cost somebody chose. |
There was a problem hiding this comment.
Passive voice: 'is made'. ASD-STE100 asks for the active voice.
| @@ -0,0 +1,290 @@ | |||
| # Teardown (chain, bench, namespace) | |||
|
|
|||
| Teardown removes an engineer's workloads from `eng-<alias>` through the same PR contract that created them. One ordering rule governs the whole file: **patch `spec.deletionPolicy` to `Delete` on every SeiNetwork you are about to remove, and land that patch before the removal merges.** A SeiNetwork deleted under the default `Retain` orphans its generated validator SeiNodes, and each orphan keeps its PVC and its EBS disk running with nothing left to clean it up. | |||
There was a problem hiding this comment.
Descriptive sentence has 29 words. ASD-STE100 allows 25. Split it.
|
|
||
| `spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable. `SeiNetworkSpec` carries exactly three immutability rules, on `spec.genesis`, `spec.replicas`, and `spec.dataVolume`. An operator can therefore move a SeiNetwork from `Retain` to `Delete`. | ||
|
|
||
| That window closes at deletion. Once a `Retain` deletion has stripped the owner references and removed the parent SeiNetwork, no patch brings the cascade back — the parent is gone and the children are top-level objects. The leftover SeiNodes and PVCs then need the manual cleanup in [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). |
There was a problem hiding this comment.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
|
|
||
| `spec.deletionPolicy` is **mutable** — no CEL validation rule and no webhook makes it immutable. `SeiNetworkSpec` carries exactly three immutability rules, on `spec.genesis`, `spec.replicas`, and `spec.dataVolume`. An operator can therefore move a SeiNetwork from `Retain` to `Delete`. | ||
|
|
||
| That window closes at deletion. Once a `Retain` deletion has stripped the owner references and removed the parent SeiNetwork, no patch brings the cascade back — the parent is gone and the children are top-level objects. The leftover SeiNodes and PVCs then need the manual cleanup in [Find and clean up already-leaked resources](#find-and-clean-up-already-leaked-resources). |
There was a problem hiding this comment.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
The disappearance poll piped kubectl into wc -l, so a Forbidden or a dropped connection yielded zero lines and printed success — the worst possible failure for a verifier, reporting the disks gone exactly when it cannot see them. Capture kubectl's exit status separately and print one of GONE / PRESENT / UNVERIFIED. The loop also used Bash's SECONDS inside an sh fence, where it is unset and the comparison dies with `Illegal number`, skipping the poll outright. Use arithmetic on `date +%s`. Poll the PVCs rather than reading them once, and expect the imported ones to survive: the SeiNode finalizer skips spec.import by design, so zero PVCs is the wrong end state. Inventory now records which nodes carry spec.import, since nothing says so once they are deleted. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| kubectl patch seinode <name> -p '{"metadata":{"finalizers":[]}}' --type=merge | ||
| ``` | ||
|
|
||
| This abandons the PVC and its EBS disk — the finalizer is what deletes the PVC, so removing it is how a stuck teardown becomes a leaked disk. Take it only with the engineer's explicit acceptance, record the PVC name, and follow up with the sweep in `teardown.md` → *find and clean up already-leaked resources*. |
There was a problem hiding this comment.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
… ownership walk EC2 `available` means unattached, not unowned — a volume backing a live Bound PVC reads available the moment its workload stops. `Used By` is current attachment, not ownership. As written the sweep routed legitimate disks into a deletion escalation framed as confirmed garbage. Both signals are now candidates. Ownership resolves by walking volume ID to PV via spec.csi.volumeHandle, PV to PVC via claimRef, then PVC to its workload, with a verdict table that sends any failed hop to UNRESOLVED rather than to confirmed-safe. The tag-confirmation command takes --volume-ids instead of dropping --filters, which would have enumerated other tenants' volumes. Also: no EBS delete from this skill at all; an escalation path for every non-Git resource a workspace PR leaves behind; and the namespace ordering is stated as a preference rather than asserting a cascade failure mode the namespace controller does not have. Co-authored-by: omnigent <noreply@omnigent.ai>
…on git-first Recipe #9 becomes a reusable poll_gone with the exit-status handling and the portable deadline; bench:teardown polls Jobs, ConfigMaps AND pods instead of reading once, since a Job can be gone while its pod is still Terminating. SKILL.md, the SeiNetwork CRD reference, chain-flow, comparative-bench, and the two evals now say the policy lands in git rather than through a live patch, and the teardown step reports GONE / PRESENT / UNVERIFIED. Co-authored-by: omnigent <noreply@omnigent.ai>
| 9. **Two paths wipe a node's chain data — gate both.** `seictl workflow state-sync` is the destructive **paved road**; a mutating `seictl task submit` is the destructive **escape hatch**. Neither is ever the default, and the agent volunteers neither. | ||
| - **`seictl workflow state-sync`** re-bootstraps an existing node by wiping its local chain state (an `rm -rf` on that node's data), optionally with an irreversible `--migration GigaStore --backend <pebbledb|rocksdb>` store change — both tokens are required together, never `--migration` alone. Require explicit engineer sign-off before the non-dry-run apply, verify the target node against the live cluster first, `--dry-run` to inspect, and escalate to the owner — never wipe on agent initiative — for any shared or long-lived `pacific-1`/`atlantic-2` follower. Never commit a workflow CR to the Flux workspace repo (a one-shot, spec-immutable request object; force-delete recovery fights Flux). Full gate in `references/seictl-cli.md` → `seictl workflow state-sync`. | ||
| - **`seictl task submit`** POSTs a raw task straight to one pod's sidecar, and the accepted types include `reset-data`. Submitted that way the wipe runs with **none** of the recipe's protections — no `mark-not-ready` hold, no `stop-seid` first, no ordering, and no adoption pointer telling the controller the node is occupied — so it is strictly more dangerous than the paved road, not a lighter-weight version of it. Prefer `workflow state-sync` for anything the recipe covers; a mutating `task submit` requires explicit sign-off naming node, namespace, and task type. `task get` / `task list` are reads and safe. Full gate in `references/seictl-cli.md` → `seictl task`. | ||
| 10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Land `deletionPolicy: Delete` **in git** and confirm it reconciled onto the live object **before** the removal merges — a bare `kubectl patch` on a Flux-owned SeiNetwork is drift the next reconcile reverts, typically while the removal PR is still in review. After the parent SeiNetwork is gone, nothing restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. |
There was a problem hiding this comment.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
| 9. **Two paths wipe a node's chain data — gate both.** `seictl workflow state-sync` is the destructive **paved road**; a mutating `seictl task submit` is the destructive **escape hatch**. Neither is ever the default, and the agent volunteers neither. | ||
| - **`seictl workflow state-sync`** re-bootstraps an existing node by wiping its local chain state (an `rm -rf` on that node's data), optionally with an irreversible `--migration GigaStore --backend <pebbledb|rocksdb>` store change — both tokens are required together, never `--migration` alone. Require explicit engineer sign-off before the non-dry-run apply, verify the target node against the live cluster first, `--dry-run` to inspect, and escalate to the owner — never wipe on agent initiative — for any shared or long-lived `pacific-1`/`atlantic-2` follower. Never commit a workflow CR to the Flux workspace repo (a one-shot, spec-immutable request object; force-delete recovery fights Flux). Full gate in `references/seictl-cli.md` → `seictl workflow state-sync`. | ||
| - **`seictl task submit`** POSTs a raw task straight to one pod's sidecar, and the accepted types include `reset-data`. Submitted that way the wipe runs with **none** of the recipe's protections — no `mark-not-ready` hold, no `stop-seid` first, no ordering, and no adoption pointer telling the controller the node is occupied — so it is strictly more dangerous than the paved road, not a lighter-weight version of it. Prefer `workflow state-sync` for anything the recipe covers; a mutating `task submit` requires explicit sign-off naming node, namespace, and task type. `task get` / `task list` are reads and safe. Full gate in `references/seictl-cli.md` → `seictl task`. | ||
| 10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Land `deletionPolicy: Delete` **in git** and confirm it reconciled onto the live object **before** the removal merges — a bare `kubectl patch` on a Flux-owned SeiNetwork is drift the next reconcile reverts, typically while the removal PR is still in review. After the parent SeiNetwork is gone, nothing restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. |
There was a problem hiding this comment.
Descriptive sentence has 37 words. ASD-STE100 allows 25. Split it.
| 9. **Two paths wipe a node's chain data — gate both.** `seictl workflow state-sync` is the destructive **paved road**; a mutating `seictl task submit` is the destructive **escape hatch**. Neither is ever the default, and the agent volunteers neither. | ||
| - **`seictl workflow state-sync`** re-bootstraps an existing node by wiping its local chain state (an `rm -rf` on that node's data), optionally with an irreversible `--migration GigaStore --backend <pebbledb|rocksdb>` store change — both tokens are required together, never `--migration` alone. Require explicit engineer sign-off before the non-dry-run apply, verify the target node against the live cluster first, `--dry-run` to inspect, and escalate to the owner — never wipe on agent initiative — for any shared or long-lived `pacific-1`/`atlantic-2` follower. Never commit a workflow CR to the Flux workspace repo (a one-shot, spec-immutable request object; force-delete recovery fights Flux). Full gate in `references/seictl-cli.md` → `seictl workflow state-sync`. | ||
| - **`seictl task submit`** POSTs a raw task straight to one pod's sidecar, and the accepted types include `reset-data`. Submitted that way the wipe runs with **none** of the recipe's protections — no `mark-not-ready` hold, no `stop-seid` first, no ordering, and no adoption pointer telling the controller the node is occupied — so it is strictly more dangerous than the paved road, not a lighter-weight version of it. Prefer `workflow state-sync` for anything the recipe covers; a mutating `task submit` requires explicit sign-off naming node, namespace, and task type. `task get` / `task list` are reads and safe. Full gate in `references/seictl-cli.md` → `seictl task`. | ||
| 10. **Never tear down a SeiNetwork before reading its `spec.deletionPolicy`.** It defaults to `Retain`, and a `Retain` deletion strips the owner reference from every generated validator SeiNode instead of deleting it. The orphans keep running, keep their PVCs, and keep their EBS disks, with nothing left in-cluster that will ever remove them — the teardown reports success while the spend continues. Land `deletionPolicy: Delete` **in git** and confirm it reconciled onto the live object **before** the removal merges — a bare `kubectl patch` on a Flux-owned SeiNetwork is drift the next reconcile reverts, typically while the removal PR is still in review. After the parent SeiNetwork is gone, nothing restores the cascade and the cleanup is manual. Full procedure and the leaked-resource sweep: `references/teardown.md`. |
There was a problem hiding this comment.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
| 1. **Pre-flight** — five gates. Halt on first failure. | ||
| 2. **Inventory what goes away** — `kubectl get seinetwork,seinode -n eng-<alias> -l sei.io/seinetwork=<chain-id>` plus `kubectl get pvc -n eng-<alias>`. Show the engineer the list before touching anything. | ||
| 3. **Gate on `deletionPolicy`** — `kubectl get seinetwork <chain-id> -n eng-<alias> -o jsonpath='{.spec.deletionPolicy}'` for every SeiNetwork in the task dir. `Retain` or empty means halt: removing the manifest orphans the generated validators and leaks their EBS disks (Guardrail #10). | ||
| 4. **Set it to `Delete` in git, then confirm both** — a policy PR that sets `deletionPolicy: Delete` on the SeiNetwork manifest, merged and reconciled, before the removal PR merges. Both the committed file and the live object must read `Delete`. **A bare `kubectl patch` is not enough on a Flux-owned SeiNetwork**: git still declares `Retain`, so the next reconcile reverts the patch, often mid-review, and the removal then merges under `Retain` anyway. The patch is a repair for a SeiNetwork no reconcile owns. Full ordering in `references/teardown.md`. |
There was a problem hiding this comment.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` from `references/cluster-inspection-recipes.md` recipe #9; it captures the exit status separately and uses a `date +%s` deadline (Bash's `SECONDS` is unset under `sh`, where the loop silently never runs). Poll the PVCs too — but expect the **imported** ones to survive, since the SeiNode finalizer skips `spec.import` by design. | ||
| 10. **Report** — what is gone, what remains, and the burned chain-id. An object still `Terminating` past the budget is a real finding: surface the finalizer and the controller log line; never strip a finalizer to make the check pass. An `UNVERIFIED` result is reported as unknown, never as done. |
There was a problem hiding this comment.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
| - `.status.endpoint.tendermintRest` — Tendermint REST URL | ||
| - For pod-targeted connectivity (seiload's WebSocket block collector, etc.), pick one follower — its `.status.endpoint` is already its stable per-node URL. | ||
| 14. **Report teardown** — `git rm -r engineers/<alias>/<task>/` **and** remove the `<task>` entry from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, cascading to pods/PVCs per k8s deletion propagation. | ||
| 14. **Report teardown** — point at `teardown.md`; do not restate it. The load-bearing step comes *before* the `git rm`: read `spec.deletionPolicy` on the SeiNetwork and, if it reads `Retain` (the default), land `Delete` **in the manifest** and let it reconcile first — a live `kubectl patch` is drift Flux reverts on its next pass. A `Retain` teardown orphans the generated validator SeiNodes and leaks their EBS disks, and nothing after the fact undoes that. Then `git rm -r engineers/<alias>/<task>/`, remove the `<task>` entry from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference), merge, reconcile `kustomization <alias>` in `eng-<alias>`, and poll until the CRs and their PVCs are gone. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
| - `.status.endpoint.tendermintRest` — Tendermint REST URL | ||
| - For pod-targeted connectivity (seiload's WebSocket block collector, etc.), pick one follower — its `.status.endpoint` is already its stable per-node URL. | ||
| 14. **Report teardown** — `git rm -r engineers/<alias>/<task>/` **and** remove the `<task>` entry from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference). Commit → push → merge. Flux prunes the SeiNetwork + SeiNodes on next reconcile, cascading to pods/PVCs per k8s deletion propagation. | ||
| 14. **Report teardown** — point at `teardown.md`; do not restate it. The load-bearing step comes *before* the `git rm`: read `spec.deletionPolicy` on the SeiNetwork and, if it reads `Retain` (the default), land `Delete` **in the manifest** and let it reconcile first — a live `kubectl patch` is drift Flux reverts on its next pass. A `Retain` teardown orphans the generated validator SeiNodes and leaks their EBS disks, and nothing after the fact undoes that. Then `git rm -r engineers/<alias>/<task>/`, remove the `<task>` entry from `engineers/<alias>/kustomization.yaml`'s `resources:` list (Kustomize fails to render with an orphan reference), merge, reconcile `kustomization <alias>` in `eng-<alias>`, and poll until the CRs and their PVCs are gone. |
There was a problem hiding this comment.
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
| kubectl get seinetwork <id> -n eng-<alias> -o jsonpath='{.spec.deletionPolicy}' # empty means Retain | ||
| ``` | ||
|
|
||
| **For a Flux-owned SeiNetwork the change goes through git, not `kubectl patch`.** The manifest rendered at spin-up normally carries the server-defaulted `Retain`, so Flux owns the field and reverts a live patch on its next reconcile — often while a removal PR is still in review, which is exactly when the revert does the damage. Set `deletionPolicy: Delete` in `engineers/<alias>/<task>/seinetwork-<id>.yaml`, merge, reconcile, and read back both git and the live object. `kubectl patch seinetwork <id> -n eng-<alias> --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct only where no reconcile owns the object. |
There was a problem hiding this comment.
Descriptive sentence has 43 words. ASD-STE100 allows 25. Split it.
|
|
||
| ### Set it to `Delete` | ||
|
|
||
| **The change must land in git. A `kubectl patch` alone does not survive to merge time.** Flux reconciles `engineers/<alias>/` every 5 minutes against what git declares. The manifest that spun the chain up was rendered from `seictl network apply --dry-run`, which captures the server-defaulted CR, so `deletionPolicy: Retain` is normally written out in the committed file. Flux owns that field, and the next reconcile reverts the patch — typically while the removal PR sits in review. The engineer then merges a teardown they believe is safe, and it orphans the validators anyway. Do not rely on server-side-apply field ownership to keep a patch alive across a reconcile, even where git happens to omit the field. |
There was a problem hiding this comment.
📝 [vale] <AgenticWriting.STE-ParagraphLength> reported by reviewdog 🐶
Paragraph has more than 6 sentences. ASD-STE100 allows 6.
|
|
||
| ### Set it to `Delete` | ||
|
|
||
| **The change must land in git. A `kubectl patch` alone does not survive to merge time.** Flux reconciles `engineers/<alias>/` every 5 minutes against what git declares. The manifest that spun the chain up was rendered from `seictl network apply --dry-run`, which captures the server-defaulted CR, so `deletionPolicy: Retain` is normally written out in the committed file. Flux owns that field, and the next reconcile reverts the patch — typically while the removal PR sits in review. The engineer then merges a teardown they believe is safe, and it orphans the validators anyway. Do not rely on server-side-apply field ownership to keep a patch alive across a reconcile, even where git happens to omit the field. |
There was a problem hiding this comment.
Passive voice: 'was rendered'. ASD-STE100 asks for the active voice.
…be overwritten poll_gone returned the right code but no caller checked it, so an UNVERIFIED first call followed by a clean last call left the block exiting 0 — the original "reports gone when it could not look" bug surviving on the exit-code path. Add a record/VERDICT aggregator where the worst outcome wins, and make every call site use it. Poll PVCs by name rather than by namespace sweep: a sweep matches imported claims and other chains' claims, so a correct teardown reported PRESENT. Explicit names need --ignore-not-found, or the success condition (NotFound) reads as UNVERIFIED. Add expect_present as the mirror assertion for imported claims the controller preserves by design. Recipe #9 is now the single implementation, and the POSIX claim is softened: date +%s is a near-universal extension, not a specified format. Verified under dash: UNVERIFIED-then-GONE aggregates to 2; expect_present returns 0 on all-present and 1 on a missing claim. Co-authored-by: omnigent <noreply@omnigent.ai>
…VC inventory path teardown.md had re-implemented the poll twice instead of calling poll_gone, and the copies had already diverged: the PVC loop emitted no GONE/PRESENT/UNVERIFIED verdict at all and just spun to its deadline printing counts. Both loops also ended in `break`, which succeeds, so the completed while exited 0 even after printing UNVERIFIED. Both call sites now use the shared functions and aggregate through record. The imported-PVC query selected .spec.import, which does not exist — the field is .spec.dataVolume.import.pvcName, so the query matched nothing and every imported claim was silently reclassified as one that must disappear. The inventory now records real node-to-claim identities from the pods' own claimName, classifies imported against controller-managed, and aborts on any API or parse failure. The path is caveated as repo-main API inspection, not the deployed CRD. Close the live-patch exception for Flux-owned networks: a pre-merge re-read narrows the window but does not order against Flux's reconcile, and the losing sequence needs no unusual timing. Path B is now a stacked draft PR, since a merged PR cannot carry a later removal commit. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'are held'. ASD-STE100 asks for the active voice.
'very' is a weasel word!
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is unwanted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 34 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is namespaced'. ASD-STE100 asks for the active voice.
Passive voice: 'was stopped'. ASD-STE100 asks for the active voice.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
Passive voice: 'is preserved'. ASD-STE100 asks for the active voice.
Passive voice: 'being recreated'. ASD-STE100 asks for the active voice.
Passive voice: 'is governed'. ASD-STE100 asks for the active voice.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
|
|
||
| **Either way it works only before deletion.** Once a `Retain` deletion has stripped the owner references and removed the parent, nothing restores the cascade — the leftover SeiNodes and PVCs need manual cleanup (`teardown.md`). | ||
|
|
||
| Under `Delete` the chain runs end to end: SeiNetwork deleted → validators deleted through their owner references → each SeiNode's finalizer deletes its data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. The finalizer skips an **imported** PVC (`spec.dataVolume.import` on the SeiNode) by design. |
There was a problem hiding this comment.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| Both reads must agree on `Delete`. A live object reading `Delete` while git still declares `Retain` is the drift this path exists to close. | ||
| 3. **Removal PR.** Only now `git rm` the task dir, per the procedure below. | ||
|
|
||
| **Path B — stack the removal PR on the policy PR.** Where preparing two changes serially is too slow, write both up front: branch the removal from the policy branch and open its PR as a **draft**. Then merge the policy PR, run the step-2 reads, and only mark the removal ready once both reads say `Delete`. This is still two separately landed changes — the saving is in preparation, not in the ordering. A single PR carrying both the policy edit and the `git rm` is not this path: it merges as one revision, so the policy and the removal reach the cluster in the same reconcile and the ordering never exists. |
There was a problem hiding this comment.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
| Both reads must agree on `Delete`. A live object reading `Delete` while git still declares `Retain` is the drift this path exists to close. | ||
| 3. **Removal PR.** Only now `git rm` the task dir, per the procedure below. | ||
|
|
||
| **Path B — stack the removal PR on the policy PR.** Where preparing two changes serially is too slow, write both up front: branch the removal from the policy branch and open its PR as a **draft**. Then merge the policy PR, run the step-2 reads, and only mark the removal ready once both reads say `Delete`. This is still two separately landed changes — the saving is in preparation, not in the ordering. A single PR carrying both the policy edit and the `git rm` is not this path: it merges as one revision, so the policy and the removal reach the cluster in the same reconcile and the ordering never exists. |
There was a problem hiding this comment.
Descriptive sentence has 37 words. ASD-STE100 allows 25. Split it.
|
|
||
| **Path B — stack the removal PR on the policy PR.** Where preparing two changes serially is too slow, write both up front: branch the removal from the policy branch and open its PR as a **draft**. Then merge the policy PR, run the step-2 reads, and only mark the removal ready once both reads say `Delete`. This is still two separately landed changes — the saving is in preparation, not in the ordering. A single PR carrying both the policy edit and the `git rm` is not this path: it merges as one revision, so the policy and the removal reach the cluster in the same reconcile and the ordering never exists. | ||
|
|
||
| **The live patch is a repair, and it is not available for a Flux-owned network.** `kubectl patch seinetwork <chain-id> -n eng-<alias> --type=merge -p '{"spec":{"deletionPolicy":"Delete"}}'` is correct in exactly one situation: the SeiNetwork is **not** in the workspace repo at all — an escape-hatch direct apply, or an object already orphaned from an earlier teardown — so no reconcile will revert it. Confirm that with the workspace search in [the other resources git never owned](#the-other-resources-git-never-owned) before relying on it. |
There was a problem hiding this comment.
Descriptive sentence has 36 words. ASD-STE100 allows 25. Split it.
| 1. Git declares `Retain`. The engineer patches the live object to `Delete`. | ||
| 2. The pre-merge read returns `Delete`. It is true, and it is already stale. | ||
| 3. Flux reconciles the existing revision — the one that still declares `Retain` — and restores `Retain`. | ||
| 4. The removal merges. The SeiNetwork is deleted under `Retain`. The validators are orphaned. |
There was a problem hiding this comment.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
|
|
||
| With `deletionPolicy: Delete` the chain runs end to end: SeiNetwork deleted → generated validator SeiNodes deleted through their owner references → each SeiNode's finalizer (`sei.io/seinode-finalizer`) deletes the node's data PVC → the storage class's `Delete` reclaim policy releases the EBS volume. | ||
|
|
||
| The finalizer **skips an imported PVC** (`spec.dataVolume.import` set on the SeiNode, naming the claim in `.pvcName`). An imported PVC is preserved by design; its disk is not a leak. See the field-path caveat under inventory step 2 before relying on that path in a query. |
There was a problem hiding this comment.
Passive voice: 'is preserved'. ASD-STE100 asks for the active voice.
| Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. | ||
|
|
||
| 1. **Pre-flight** — the five gates. Halt on first failure. | ||
| 2. **Inventory what goes away and what stays — by name, before anything is deleted.** The verification in step 8 polls *named* claims, so this step produces those names. It has to run first: once the SeiNodes are gone, nothing in the cluster still records which claims were imported and which the controller managed. |
There was a problem hiding this comment.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
| Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. | ||
|
|
||
| 1. **Pre-flight** — the five gates. Halt on first failure. | ||
| 2. **Inventory what goes away and what stays — by name, before anything is deleted.** The verification in step 8 polls *named* claims, so this step produces those names. It has to run first: once the SeiNodes are gone, nothing in the cluster still records which claims were imported and which the controller managed. |
There was a problem hiding this comment.
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
| Teardown follows the same PR contract as spinup: render the change, open a PR, let the engineer merge, verify what Flux did. Never `kubectl delete` a Flux-owned CR — the next reconcile re-applies it and the removal PR never lands. | ||
|
|
||
| 1. **Pre-flight** — the five gates. Halt on first failure. | ||
| 2. **Inventory what goes away and what stays — by name, before anything is deleted.** The verification in step 8 polls *named* claims, so this step produces those names. It has to run first: once the SeiNodes are gone, nothing in the cluster still records which claims were imported and which the controller managed. |
There was a problem hiding this comment.
Passive voice: 'were imported'. ASD-STE100 asks for the active voice.
| 1. **Pre-flight** — the five gates. Halt on first failure. | ||
| 2. **Inventory what goes away and what stays — by name, before anything is deleted.** The verification in step 8 polls *named* claims, so this step produces those names. It has to run first: once the SeiNodes are gone, nothing in the cluster still records which claims were imported and which the controller managed. | ||
|
|
||
| Save this as `inventory.sh` and run it with `sh inventory.sh`. It aborts on the first API or parse failure, because a partial inventory under-reports what must disappear and then reads as a clean teardown later. |
There was a problem hiding this comment.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
… delete scope The ownership walk listed every SeiNode in the namespace without establishing which one referenced the candidate claim, then offered a "confirmed orphan" verdict off that. Hop 3 now resolves the claim to the pod that mounts it and that pod's owner reference, and an empty result is unresolved — a stopped workload is exactly what made the volume read available in the first place. Each hop separates the API call from the parse and checks both: a kubectl-into-jq pipeline exits with jq's status, so a Forbidden read as "no match", which on this walk is the difference between unowned and could-not-look. Missing-claim and no-PV rows are now explicitly unresolved candidates requiring platform review, not bare candidates. Add the tenant guard: claimRef comes from a cluster-wide PV list and can name any namespace, so anything other than eng-<alias> escalates instead of being inspected. Name the namespace and context on every delete, since both CLIs fall back to the current context otherwise. Replace the authorizing grep with one that distinguishes no-match (exit 1, the only status that authorizes a delete) from search error (exit 2+), against a freshly fetched clone. Drop the GNU-only `sed -n '/x/,+3p'`. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'is preserved'. ASD-STE100 asks for the active voice.
Passive voice: 'being recreated'. ASD-STE100 asks for the active voice.
Passive voice: 'is governed'. ASD-STE100 asks for the active voice.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
The Retain eval forbade live patching only when there was no git change AND no pre-merge re-verification, which preserved the exact unsafe exception the docs now close; the happy path let the agent proceed on a live Delete without requiring the committed manifest to agree. Both now require git verification for a Flux-owned object, and the happy path requires named-claim polling with outcome aggregation rather than a namespace PVC sweep. Co-authored-by: omnigent <noreply@omnigent.ai>
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
Descriptive sentence has 37 words. ASD-STE100 allows 25. Split it.
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
📝 [vale] <AgenticWriting.STE-ParagraphLength> reported by reviewdog 🐶
Paragraph has more than 6 sentences. ASD-STE100 allows 6.
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
Descriptive sentence has 34 words. ASD-STE100 allows 25. Split it.
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
Descriptive sentence has 35 words. ASD-STE100 allows 25. Split it.
| **PVC behavior** — verify before deleting on stateful nodes: | ||
| - For **imported** PVCs (`spec.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. | ||
| - For **controller-managed** PVCs (no `spec.import`): the controller's `handleNodeDeletion` path deletes the PVC during teardown. Delete-and-recreate **wipes data**. Safe for ephemeral chains being recreated from genesis; not safe for archive nodes or any chain with state worth preserving. | ||
| - For **imported** PVCs (`spec.dataVolume.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. |
There was a problem hiding this comment.
Passive voice: 'is preserved'. ASD-STE100 asks for the active voice.
| - For **imported** PVCs (`spec.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. | ||
| - For **controller-managed** PVCs (no `spec.import`): the controller's `handleNodeDeletion` path deletes the PVC during teardown. Delete-and-recreate **wipes data**. Safe for ephemeral chains being recreated from genesis; not safe for archive nodes or any chain with state worth preserving. | ||
| - For **imported** PVCs (`spec.dataVolume.import` set on the SeiNode): the PVC is preserved; the recreated SeiNode reuses existing data. | ||
| - For **controller-managed** PVCs (no `spec.dataVolume.import`): the controller's `handleNodeDeletion` path deletes the PVC during teardown. Delete-and-recreate **wipes data**. Safe for ephemeral chains being recreated from genesis; not safe for archive nodes or any chain with state worth preserving. |
There was a problem hiding this comment.
Passive voice: 'being recreated'. ASD-STE100 asks for the active voice.
| ### vs. retained data on delete | ||
|
|
||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted — useful when tearing down a network but keeping a validator's disk for forensics. The hardlink trick above is for **live debugging** while the node continues running. They're complementary, not redundant. | ||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.dataVolume.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. |
There was a problem hiding this comment.
Passive voice: 'is governed'. ASD-STE100 asks for the active voice.
| ### vs. retained data on delete | ||
|
|
||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted — useful when tearing down a network but keeping a validator's disk for forensics. The hardlink trick above is for **live debugging** while the node continues running. They're complementary, not redundant. | ||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.dataVolume.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. |
There was a problem hiding this comment.
Passive voice: 'is deleted'. ASD-STE100 asks for the active voice.
| ### vs. retained data on delete | ||
|
|
||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted — useful when tearing down a network but keeping a validator's disk for forensics. The hardlink trick above is for **live debugging** while the node continues running. They're complementary, not redundant. | ||
| For a SeiNode, whether its PVC survives deletion is governed by `spec.dataVolume.import` (imported PVC = preserved) vs controller-managed (wiped on teardown) — documented under **Phase: Failed** above. A `SeiNetwork`'s `spec.deletionPolicy` (defaults `Retain`) governs whether the controller orphans its generated validator SeiNodes (and thus their PVCs) when the network is deleted. Forensics is the one case where `Retain` is the right answer; on an ordinary teardown it is a disk leak, because the orphaned validators keep running with no owner left to delete them (see `teardown.md`). The hardlink trick above is for **live debugging** while the node continues running. The two are complementary, not redundant. |
There was a problem hiding this comment.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| ``` | ||
| 2. For every SeiNetwork in the inventory, run the `deletionPolicy` gate in [The `deletionPolicy: Retain` trap](#the-deletionpolicy-retain-trap). One `Retain` network is enough to leak a set of disks. | ||
| 3. `git rm -r` every task dir under `engineers/<alias>/`, and reduce `engineers/<alias>/kustomization.yaml` to `resources: []`. Keep that file: deleting it makes the Flux Kustomization fail reconcile with `path not found`, which is the same breakage the onboarding scaffolding PR exists to prevent. | ||
| 4. Open the PR, merge, then run [Verify the teardown](#verify-the-teardown) with no `-l` selector, so the poll covers the whole namespace. |
There was a problem hiding this comment.
blocker — This step now contradicts the section it links to. Verify the teardown polls PVCs by name from ./teardown-inventory/managed-claims.txt and imported-claims.txt — its PVC calls carry no -l to drop — and both line 245 here and cluster-inspection-recipes.md:249 state in bold that a namespace sweep is the wrong check, because imported claims and other chains' claims make it report PRESENT after a correct teardown. Step 1 of this procedure is a display-only kubectl get seinetwork,seinode,job,pvc, so it never generates those name lists: an operator following this step either runs the sweep the same file argues trains the reader to ignore the check, or has no inputs for the verification at all. It is also worst in the case that most needs it — a namespace holding orphans from an earlier Retain teardown, where a sweep can never reach GONE. Point step 1 at the inventory script from Procedure step 2, run over every chain in the namespace, and have step 4 use the same by-name calls.
| res=$1; shift | ||
| deadline=$(( $(date +%s) + 300 )) | ||
| while : ; do | ||
| out=$(kubectl --context harbor get "$res" -n eng-<alias> "$@" -o name 2>&1); rc=$? |
There was a problem hiding this comment.
suggestion — 2>&1 merges stderr into out, and left=$(printf '%s' "$out" | grep -c .) counts every line in it as a remaining object. Any stderr line emitted alongside a successful read — an apiserver deprecation or warning header, a stale-cache notice — inflates the count, so the loop runs to the deadline and reports PRESENT for a namespace that is actually empty; expect_present at line 224 has the same construction and would report MISSING. The direction is fail-safe, but a false alarm is exactly what line 249 says trains the reader to ignore the check. Capture stderr into a separate variable or file so the count sees only stdout, and keep it for the UNVERIFIED message. (Correcting my earlier reviews: I attributed this to kubectl's "No resources found" notice; on the -o name path that is unverified, and the warning-header case is what holds.)
| - **`<chain-tag>` exceeds the 22-char budget** when the `-{a,b}-rpc-<k>` suffix is added. Surface the overflow and ask the engineer to pick a shorter tag. | ||
| - **CR name collision on either side.** Halt before render; surface the existing object's age + labels. | ||
| - **One network reaches `Ready` while the other reaches `Failed`.** The comparison is invalid. Surface the failed side's `.status.plan.failedTaskDetail.error`. The half-teardown is two coordinated edits, **both required** — Flux refuses to apply a kustomization with a missing resource: | ||
| - **First**, land `deletionPolicy: Delete` in the surviving side's SeiNetwork manifest if it reads `Retain`, and let it reconcile before the removal merges (`teardown.md` — a live patch gets reverted). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. |
There was a problem hiding this comment.
suggestion — The gate names the wrong SeiNetwork for the steps it prefixes. This bullet lands deletionPolicy: Delete on the surviving side, but the two steps below git rm the failed side's chain-<a-or-b>/ and the closing line says Flux prunes the SeiNetwork on the failed side. The network whose policy decides whether validators are orphaned is the one being removed, so the gate belongs there; "the failed side needs the same read" leaves the load-bearing case as a secondary check. The verb is right now — swap which side it applies to.
| **Read `references/teardown.md` first.** It carries the `deletionPolicy` trap in full, the namespace procedures, and the sweep for resources that already leaked. | ||
|
|
||
| 1. **Pre-flight** — five gates. Halt on first failure. | ||
| 2. **Inventory what goes away** — `kubectl get seinetwork,seinode -n eng-<alias> -l sei.io/seinetwork=<chain-id>` plus `kubectl get pvc -n eng-<alias>`. Show the engineer the list before touching anything. |
There was a problem hiding this comment.
suggestion — This inventory is a bare kubectl get pvc -n eng-<alias> and step 9 says only "poll the PVCs too", so the always-loaded procedure produces neither the managed nor the imported claim-name list that recipe #9 now requires, and never mentions the record/VERDICT aggregation that keeps an early UNVERIFIED from being masked. An agent working from SKILL.md alone lands on the namespace sweep that teardown.md:245 and recipe #9 both call the wrong check. Point this step at the inventory script in references/teardown.md procedure step 2, and have step 9 name the by-name calls and the verdict aggregation.
What changedThe pull request moved again (merge commit Three of my four blockers are addressed. Path B is now executable — it stacks a draft removal PR on the policy PR and states plainly that a single PR carrying both is not that path ( A correction to my last two reviews: I said 1. BlockingThe namespace-emptying path instructs the check the verification section forbids, using files that path never creates. 2. Non-blocking
3. SummaryThis revision closed Path B, the imported-PVC field path and identifier, and the unselected PVC poll — the three blockers I carried in, one of which was codex's from an earlier dispatch — and the replacements are stronger than what I asked for: claim names read from the pods rather than guessed, a seidroid review · decision Findings: 1 blocking | 14 non-blocking | 4 posted inline | 1 pre-existing |
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
SKILL inventory omits claim classification
Medium Severity
Step 9 now polls controller-managed PVCs by name from the step-2 inventory and forbids a namespace-wide sweep, and the happy-path eval requires capturing imported versus managed claims. Step 2 still runs kubectl get pvc -n eng-<alias> and never classifies those claims, so an agent following SKILL.md inventories the sweep that step 9 says makes a correct teardown report PRESENT.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 45a623c. Configure here.
… reads a checked step expect_present merged stderr into stdout and counted non-empty lines, so one API deprecation warning satisfied want=1 and an absent claim reported PRESERVED. poll_gone had the same mixing, where a warning could make an empty result report PRESENT. Both now keep stderr in a separate sink, count only lines shaped like a resource id, and expect_present matches the returned identities against the requested names rather than counting. Inventory name lists were inlined as $(cat file) into the helper argument list, so a missing or unreadable file collapsed to an empty argument list and the helpers succeeded against an empty namespace — losing the inventory read as a clean teardown, and $? was the helper's status, never cat's. read_inventory is now its own checked step with three distinct states: has-entries, legitimately-empty, and missing-or-unreadable. The empty and unreadable branches are implemented, not described. Every helper runs its API call inside a condition and every caller uses an OR-list, so set -e can no longer terminate inside an assignment before classification or before record runs. Verified under dash and bash, with and without set -eu: stderr warning with an absent claim reports MISSING; warning with empty stdout reports GONE; missing inventory reports UNVERIFIED; empty inventory takes the NOTE branch and leaves the verdict alone; UNVERIFIED then GONE still aggregates to 2. Co-authored-by: omnigent <noreply@omnigent.ai>
…elete context Managed claims came only from pods that currently exist, so a node whose pod was absent contributed nothing — and a provisioned PVC with no pod is exactly the leaked disk this document exists to catch. The inventory now attributes claims per node, computes the nodes that resolve to no storage at all, writes them to unresolved-nodes.txt, exits non-zero, and leaves a status file reading UNRESOLVED that forces the verifier to UNVERIFIED. Pod phase is deliberately not consulted: a Pending pod still declares its volumes. Namespace PVCs no chain attributes are surfaced separately for the leak sweep rather than folded into the managed list. The namespace-emptying path had only a display command and generated none of the named-claim files the verifier consumes; it now runs inventory.sh once per chain-id, each with its own INV directory. Replace the unenforceable "(harbor context)" parenthetical on the direct delete: seictl's documented flags are --kubeconfig and -n only, with no --context, so the direct deletes use kubectl --context harbor, which pins cluster and namespace on the line that deletes. A guarded seictl form is documented for workflows that need it, with its check-to-call window stated. Also: the hop-3 pod parse now has the explicit UNRESOLVED branch the other hops have, and the ownership search exits 0/1/2 so a scripted caller can tell "not in git" from "the search failed". Verified under dash and bash: a node with no pod lands unresolved and stops the run; the happy path flips status to OK; the search returns three distinct exit codes under set -e. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'is told'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is namespaced'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 32 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 47 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
|
|
||
| **And a later success must never overwrite an earlier failure.** Printing `UNVERIFIED` is not enough on its own: a `break` out of a loop, or a bare call whose return code nobody reads, still leaves the block exiting 0. A human sees the warning; a wrapper script or an agent reading `$?` sees success. Every check records its outcome into a running verdict, and the worst one wins. | ||
|
|
||
| **Use `poll_gone`, `expect_present`, and `record` from `cluster-inspection-recipes.md` recipe #9 — do not re-implement them here.** One implementation, one place to fix. Source them, then: |
There was a problem hiding this comment.
blocker — This is the block an operator following teardown.md runs, and it is a generation behind the recipe it cites. It never reads $INV/status, it inlines $(cat ./teardown-inventory/managed-claims.txt) into the arguments, and it uses ; record $? — the two patterns recipe #9 names as defects, one in the words 'losing the inventory reads as a clean teardown'. The new inventory makes the consequence concrete: inventory.sh writes managed-claims.txt before completeness check 1, so an aborted run leaves that file present but missing the unresolved node's claim while status reads UNRESOLVED. This block polls the truncated list, finds nothing, and prints TEARDOWN VERIFIED — precisely the node whose disk may be leaking, reported as verified. Two smaller faults in the same block: it hardcodes ./teardown-inventory while the namespace procedure now directs per-chain INV=./teardown-inventory-<chain-id> directories, and a missing file collapses to an empty argument list that turns poll_gone pvc --ignore-not-found into the namespace sweep this page forbids. Line 256 already says not to re-implement the helpers: replace the block with recipe #9's read_inventory/OR-list version, parameterised on INV, or drop it and link there.
| - **`<chain-tag>` exceeds the 22-char budget** when the `-{a,b}-rpc-<k>` suffix is added. Surface the overflow and ask the engineer to pick a shorter tag. | ||
| - **CR name collision on either side.** Halt before render; surface the existing object's age + labels. | ||
| - **One network reaches `Ready` while the other reaches `Failed`.** The comparison is invalid. Surface the failed side's `.status.plan.failedTaskDetail.error`. The half-teardown is two coordinated edits, **both required** — Flux refuses to apply a kustomization with a missing resource: | ||
| - **First**, land `deletionPolicy: Delete` in the surviving side's SeiNetwork manifest if it reads `Retain`, and let it reconcile before the removal merges (`teardown.md` — a live patch gets reverted). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. |
There was a problem hiding this comment.
suggestion — The gate still names the wrong side. It lands deletionPolicy: Delete in the surviving side's manifest, but the two steps below git rm the failed side's chain-<a-or-b>/ and the closing line says Flux prunes the SeiNetwork on the failed side. The network whose policy decides whether its validators are orphaned is the one being removed, so the gate belongs there; 'the failed side needs the same read' leaves the load-bearing case as a secondary check. The verb is correct now — only the side needs swapping.
| **Read `references/teardown.md` first.** It carries the `deletionPolicy` trap in full, the namespace procedures, and the sweep for resources that already leaked. | ||
|
|
||
| 1. **Pre-flight** — five gates. Halt on first failure. | ||
| 2. **Inventory what goes away** — `kubectl get seinetwork,seinode -n eng-<alias> -l sei.io/seinetwork=<chain-id>` plus `kubectl get pvc -n eng-<alias>`. Show the engineer the list before touching anything. |
There was a problem hiding this comment.
suggestion — This step is still a bare kubectl get seinetwork,seinode plus kubectl get pvc -n eng-<alias>, which captures no imported-versus-managed split and produces none of the named-claim files. Step 9 now correctly tells the agent to poll 'by name from the step-2 inventory' and to aggregate outcomes, so the always-loaded procedure asks its own step 2 for something it does not produce — and an agent working from SKILL.md alone lands on the namespace sweep that teardown.md and recipe #9 both reject. Point this step at inventory.sh in references/teardown.md procedure step 2, and name the status/managed-claims.txt/imported-claims.txt outputs step 9 consumes.
What changedThe pull request moved again (merge commit Two of my three blockers are closed. The inventory script now writes 1. BlockingThe verify block in 2. Non-blocking
3. SummaryThis revision closed the status-gate mismatch and the namespace-sweep contradiction, and went further than I asked on both: the inventory now refuses to declare itself complete when a SeiNode resolves to no storage, which is the leak case, and the namespace path runs a real per-chain inventory instead of a display read. What still blocks is the one seam that has survived three revisions — seidroid review · decision Findings: 1 blocking | 13 non-blocking | 3 posted inline | 1 pre-existing |
…tion The canonical caller in teardown.md re-implemented the whole orchestration under a paragraph telling the reader not to, and never called read_inventory, never read the completeness certificate, and never consumed unresolved-nodes.txt. Everything built in the library was dead code on the path that matters. That is the same defect relocating one layer up for the fourth time: duplicated poll bodies, then the caller chain, then the caller arguments, then a caller that bypassed the fix. Recipe #9 now holds one parameterized verify_teardown carrying the certificate gate, the checked list reads, the empty-list branches as code, the polls and the aggregation. teardown.md contains no verification shell at all — only a call. Single-chain, namespace, and bench teardown all go through that one function; a bench passes `-` for the inventory dir because it owns no claims. The certificate now names its target, so a complete inventory for another chain cannot authorize this one, and it is written only after every check passes. The inventory takes a fresh directory, so a failed refresh cannot leave a stale OK beside half-written lists. Each transformation is its own command: `cut … | sort -u` exits with sort's status, so a failed cut produced a successful empty claim list and the inventory certified itself complete while omitting every managed claim. Namespace teardown checks chain discovery's own status, aggregates every chain into one verdict, and sweeps for unlabelled leftovers. Remove the guarded seictl delete: a current-context check reads mutable state rather than pinning the config the delete consumes, and the explicitly scoped kubectl path already exists. Verified under dash and bash, with and without set -eu: 11 verify_teardown scenarios including a stale cross-chain certificate, an unresolved node, a missing imported claim behind a stderr warning, bench mode, the unlabelled sweep, and multi-chain aggregation where a clean second chain must not cover an unverified first. Plus: a stale certificate does not survive a failed refresh, and a failed cut aborts before certifying. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'are held'. ASD-STE100 asks for the active voice.
'very' is a weasel word!
Descriptive sentence has 34 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 35 words. ASD-STE100 allows 25. Split it.
'usually' is a weasel word!
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is unwanted'. ASD-STE100 asks for the active voice.
Descriptive sentence has 34 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is unresolved'. ASD-STE100 asks for the active voice.
Passive voice: 'was stopped'. ASD-STE100 asks for the active voice.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'is told'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is namespaced'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
🚫 [vale] <write-good.ThereIs> reported by reviewdog 🐶
Don't start a sentence with 'There is'.
Descriptive sentence has 43 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 47 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
|
|
||
| A Flux reconcile reports success once it issues the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll rather than assert once. | ||
|
|
||
| **This block is the only verification code in this skill.** `verify_teardown` is the single entry point: every teardown — one chain, a whole namespace, a bench — calls it and reads its return value. Nothing re-implements the orchestration, because the one defect this whole procedure exists to prevent ("report success when the check could not actually look") has repeatedly survived by reappearing in a second copy of the orchestration one layer up. One copy is the control for that. |
There was a problem hiding this comment.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
|
|
||
| Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX dependencies, all near-universal: `date +%s`, `mktemp -d`, and `kubectl`'s own flags. Bash's `SECONDS` is *not* usable — it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. | ||
|
|
||
| **Three rules hold everywhere below.** Each corresponds to a defect found while reviewing this document, and fixed before it merged. None of them ever ran against a cluster. They are recorded because each would have shipped a verifier that passes when it cannot see the cluster, and because the same defect class kept reappearing until the rule was written down: |
There was a problem hiding this comment.
Descriptive sentence has 32 words. ASD-STE100 allows 25. Split it.
|
|
||
| Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX dependencies, all near-universal: `date +%s`, `mktemp -d`, and `kubectl`'s own flags. Bash's `SECONDS` is *not* usable — it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. | ||
|
|
||
| **Three rules hold everywhere below.** Each corresponds to a defect found while reviewing this document, and fixed before it merged. None of them ever ran against a cluster. They are recorded because each would have shipped a verifier that passes when it cannot see the cluster, and because the same defect class kept reappearing until the rule was written down: |
There was a problem hiding this comment.
Passive voice: 'are recorded'. ASD-STE100 asks for the active voice.
|
|
||
| Written for a portable shell (`dash`, `ash`, `bash`). Three deliberate non-POSIX dependencies, all near-universal: `date +%s`, `mktemp -d`, and `kubectl`'s own flags. Bash's `SECONDS` is *not* usable — it is unset under `sh`, where the comparison dies with `Illegal number` and the loop never runs. | ||
|
|
||
| **Three rules hold everywhere below.** Each corresponds to a defect found while reviewing this document, and fixed before it merged. None of them ever ran against a cluster. They are recorded because each would have shipped a verifier that passes when it cannot see the cluster, and because the same defect class kept reappearing until the rule was written down: |
There was a problem hiding this comment.
Passive voice: 'was written'. ASD-STE100 asks for the active voice.
| **Three rules hold everywhere below.** Each corresponds to a defect found while reviewing this document, and fixed before it merged. None of them ever ran against a cluster. They are recorded because each would have shipped a verifier that passes when it cannot see the cluster, and because the same defect class kept reappearing until the rule was written down: | ||
|
|
||
| 1. **stderr never mixes with resource output.** `2>&1` merges API deprecation warnings into the result, and "count the non-empty lines" then treats one warning as one resource. | ||
| 2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. |
There was a problem hiding this comment.
Passive voice: 'are matched'. ASD-STE100 asks for the active voice.
|
|
||
| Compare that revision to the merge commit SHA. A stale revision means Flux has not applied the removal yet, so any disappearance check below is premature. | ||
|
|
||
| A `Forbidden` on `--with-source` **may** mean the `GitRepository` the Kustomization references sits outside `eng-<alias>`, beyond the engineer's namespace-scoped Role. It may equally be an expired session, a missing EKS access entry, or a Role that never carried the Flux verbs. Read the message before concluding which. Dropping `--with-source` helps only the first cause: reconciling the Kustomization alone applies the revision the source has already fetched, and the source polls on its own schedule. It repairs nothing for an expired session, a missing access entry, or a Role without the Flux verbs — those fail the same way with or without the flag. **The fallback has to succeed on its own terms.** If the reconcile without `--with-source` also fails, you have no reconcile at all: stop, fix the access problem, and do not proceed to the disappearance check, whose result would be `UNVERIFIED` anyway. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
|
|
||
| Compare that revision to the merge commit SHA. A stale revision means Flux has not applied the removal yet, so any disappearance check below is premature. | ||
|
|
||
| A `Forbidden` on `--with-source` **may** mean the `GitRepository` the Kustomization references sits outside `eng-<alias>`, beyond the engineer's namespace-scoped Role. It may equally be an expired session, a missing EKS access entry, or a Role that never carried the Flux verbs. Read the message before concluding which. Dropping `--with-source` helps only the first cause: reconciling the Kustomization alone applies the revision the source has already fetched, and the source polls on its own schedule. It repairs nothing for an expired session, a missing access entry, or a Role without the Flux verbs — those fail the same way with or without the flag. **The fallback has to succeed on its own terms.** If the reconcile without `--with-source` also fails, you have no reconcile at all: stop, fix the access problem, and do not proceed to the disappearance check, whose result would be `UNVERIFIED` anyway. |
There was a problem hiding this comment.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
|
|
||
| ### Confirm the resources disappeared | ||
|
|
||
| A successful reconcile says Flux applied the change. It does not say the objects are gone. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases their PVCs, so poll instead of asserting once. |
There was a problem hiding this comment.
Passive voice: 'are gone'. ASD-STE100 asks for the active voice.
| | `PRESENT` | The API answered and objects remain at the deadline. | Not torn down. Read the finalizers below. | | ||
| | `UNVERIFIED` | The API call failed — `Forbidden`, expired credential, connection error. | **Teardown not confirmed.** Say the check could not run. | | ||
|
|
||
| **A failed API read is never a pass.** A `Forbidden` or a dropped connection returns zero lines, and a check that counts lines without reading the exit status prints "gone" precisely when it cannot see the cluster. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
|
|
||
| **A failed API read is never a pass.** A `Forbidden` or a dropped connection returns zero lines, and a check that counts lines without reading the exit status prints "gone" precisely when it cannot see the cluster. | ||
|
|
||
| **And a later success must never overwrite an earlier failure.** Printing `UNVERIFIED` is not enough on its own: a `break` out of a loop, or a bare call whose return code nobody reads, still leaves the block exiting 0. A human sees the warning; a wrapper script or an agent reading `$?` sees success. Every check records its outcome into a running verdict, and the worst one wins. |
There was a problem hiding this comment.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
`storage:\n` matches skill-package-checks A2's drive-letter pattern as `e:\n`. Same false positive as round 4, same fix. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
|
|
||
| ```sh | ||
| # ============ harbor teardown verification library ======================== | ||
| # Source this, then call verify_teardown. Do not copy pieces of it. |
There was a problem hiding this comment.
SKILL.md still calls old helpers
Medium Severity
Recipe #9 now says verify_teardown is the only entry point and changes poll_gone / expect_present to take the namespace first. SKILL.md step 9 still tells the agent to call poll_gone / expect_present / record directly. That contradicts the skill’s first surface: an agent following SKILL.md skips the certificate and named-claim gates, and the old helper signature binds kinds as the namespace so the check cannot see the cluster.
Additional Locations (1)
Triggered by learned rule: Skill cross-file claim precision: taxonomy and authority statements must match everywhere
Reviewed by Cursor Bugbot for commit ba2c5a0. Configure here.
…ore the verdict, isolate traps Chain discovery checked its API read but not its transformations: `| sort -u` takes sort's status and `|| true` swallowed the rest, so a failed grep or sort produced a successful EMPTY chain list, the per-chain loop ran zero times, and every certificate and claim check was skipped at VERDICT 0. Filter and sort are now separate checked steps where only grep's exit 1 counts as legitimate emptiness. The prose that argued this could not happen carried the same gap — it reasoned only about an API failure — and now names both. Both orphan sweeps piped an unchecked kubectl into jq. A failed read gives jq no input, and jq exits 0 with no output, so an unreadable sweep read as "no orphans". Read and parse are separated and both checked. The namespace path printed NAMESPACE EMPTIED and exited before step 5, so a namespace whose only leftover was a leaked PVC passed: clean discovery, clean CR/pod poll, exit 0. The `-` call is now documented and used as a CR/pod disappearance check only; a new sweep_residual runs first, excludes each chain's imported claims BY NAME as expected survivors, and its result is aggregated before anything is printed or exited. Sourcing the library no longer installs a trap or creates a temp dir: verify_ teardown and sweep_residual are subshell functions owning their own temp dir and traps, so a caller's EXIT handler survives, nothing leaks on return, and a later check cannot inherit a deleted stderr dir. An interrupt exits 130/143, which is above every verdict value, so both verdict switches gained an explicit catch-all rather than ending silently. Also: the inventory abort note said status "stays UNRESOLVED"; the implementation writes no certificate at all. Verified under dash and bash, with and without set -eu: 11 prior scenarios still pass, plus 11 new — grep/sort failures in discovery, all-empty labels as legitimate, residual sweep with a leaked PVC vs an imported survivor, forbidden residual read, unreadable survivors list, and the signature case where the CR/pod check alone verdicts 0 while the residual sweep correctly verdicts 1. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 43 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 47 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
|
|
||
| Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. | ||
|
|
||
| **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, and **no certificate is written at all** — the `status` file simply does not exist, which `read_inventory` reports as `UNVERIFIED`. The verifier reads that file, and also reads `unresolved-nodes.txt` directly, so either one alone is enough to fail the run. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
|
|
||
| Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. | ||
|
|
||
| **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, and **no certificate is written at all** — the `status` file simply does not exist, which `read_inventory` reports as `UNVERIFIED`. The verifier reads that file, and also reads `unresolved-nodes.txt` directly, so either one alone is enough to fail the run. |
There was a problem hiding this comment.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
|
|
||
| Claim names come from the **pods' own `spec.volumes[].persistentVolumeClaim.claimName`**, not from a guessed naming rule — the controller owns how it names a generated claim, and a rule inferred here would desync the moment it changes. | ||
|
|
||
| **A node with no pod resolves to nothing, and that is the leak case, not a nuisance.** The controller reconciles each SeiNode into a StatefulSet (`seinode-crd.md`), so a node whose StatefulSet has no pod — scaled down, unschedulable, evicted — still has its PVC and its EBS volume. The old version of this inventory dropped that node's claim silently and the teardown then verified clean. Check 1 makes the gap executable: the node lands in `unresolved-nodes.txt`, the script exits non-zero, and **no certificate is written at all** — the `status` file simply does not exist, which `read_inventory` reports as `UNVERIFIED`. The verifier reads that file, and also reads `unresolved-nodes.txt` directly, so either one alone is enough to fail the run. |
There was a problem hiding this comment.
Passive voice: 'is written'. ASD-STE100 asks for the active voice.
| 4. Open the PR, merge, then verify **every chain, retaining the worst result**. One `VERDICT` spans the whole namespace, so a clean second chain cannot cover an unverified first one: | ||
|
|
||
| ```sh | ||
| # Same shell as step 1 — the library is already sourced and $VERDICT already |
There was a problem hiding this comment.
suggestion — Steps 4-5 still inherit $chains and $VERDICT from step 1 across opening a PR, review, and a merge, so a fresh shell is the ordinary case. sweep_residual now keeps that from hiding a leak — a leftover claim or a lingering SeiNode still fails the run — so this is no longer blocking. What survives is narrower: with an empty $chains the per-chain loop runs zero times, expect_present never asserts the imported claims survived, and the summary at line 417 still prints 'every chain verified'. Re-derive $chains at the top of step 4 from the ./teardown-inventory-* directories on disk (treating an empty result as UNVERIFIED unless a discovery that succeeded in this shell says otherwise), and the assumption goes away.
| 6. **Commit + push** — branch `feat/eng-<alias>-teardown-<task>`. Message: `feat(eng/<alias>): tear down <task> — chain-id=<chain-id>`. | ||
| 7. **Open the PR** — title `feat(eng/<alias>): tear down <task>`. Body names the chain-id, every CR that goes away, and the `deletionPolicy` value the SeiNetwork now carries. Surface the URL and halt for the merge. | ||
| 8. **After merge — reconcile the workspace Kustomization** — `flux --context harbor reconcile kustomization <alias> -n eng-<alias> --with-source`, then compare `.status.lastAppliedRevision` to the merge SHA. Reconciling `flux-system` here verifies the wrong repo (see Post-merge reconciliation). | ||
| 9. **Poll until the resources disappear, and distinguish three outcomes** — a reconcile only says Flux issued the deletes. Deletion is asynchronous and finalizers hold objects in `Terminating` while the controller releases PVCs, so poll on a budget (~5 min) rather than asserting once. Report exactly one of **`GONE`** (the API answered and matched nothing), **`PRESENT`** (objects remain at the deadline), or **`UNVERIFIED`** (the API call failed). **A failed read is never a pass** — a `Forbidden` or dropped connection returns zero lines, so a check that counts lines without reading `kubectl`'s exit status claims success precisely when it cannot see the cluster. Use `poll_gone` / `expect_present` / `record` from `references/cluster-inspection-recipes.md` recipe #9 — one implementation, called, never re-typed. **Aggregate the outcomes**: a bare call discards its return code, so an early `UNVERIFIED` followed by a clean later read leaves the whole check exiting 0, which is the original bug on the exit-code path. Poll the chain's controller-managed claims **by name** from the step-2 inventory, and assert the imported ones separately — a namespace-wide PVC sweep also matches imported claims and other chains', so a correct teardown reports `PRESENT`. |
There was a problem hiding this comment.
suggestion — This still points the agent at poll_gone / expect_present / record, but recipe #9 now makes verify_teardown and sweep_residual the entry points, with 'Do not copy pieces' written into the block. The always-loaded file names the parts the library asks callers not to assemble themselves. Point step 9 at verify_teardown, and mention sweep_residual for the namespace case.
| - **`<chain-tag>` exceeds the 22-char budget** when the `-{a,b}-rpc-<k>` suffix is added. Surface the overflow and ask the engineer to pick a shorter tag. | ||
| - **CR name collision on either side.** Halt before render; surface the existing object's age + labels. | ||
| - **One network reaches `Ready` while the other reaches `Failed`.** The comparison is invalid. Surface the failed side's `.status.plan.failedTaskDetail.error`. The half-teardown is two coordinated edits, **both required** — Flux refuses to apply a kustomization with a missing resource: | ||
| - **First**, land `deletionPolicy: Delete` in the surviving side's SeiNetwork manifest if it reads `Retain`, and let it reconcile before the removal merges (`teardown.md` — a live patch gets reverted). The failed side needs the same read: a network that never reached `Ready` may still have generated validators to orphan. |
There was a problem hiding this comment.
suggestion — The gate names the wrong side. It lands deletionPolicy: Delete in the surviving side's manifest, but the two steps below git rm the failed side's chain-<a-or-b>/ and the closing line says Flux prunes the SeiNetwork on the failed side. The network whose policy decides whether its validators are orphaned is the one being removed, so the gate belongs there; 'the failed side needs the same read' leaves the load-bearing case as a secondary check. The verb is correct — only the side needs swapping.
Superseded: the latest review found nothing blocking in this change.
What changedThe pull request moved to merge commit My blocker is closed, though not by the fix I proposed. The namespace procedure still inherits Nothing new blocks. 1. BlockingNone. 2. Non-blocking
3. SummaryThe residual sweep is the right answer to the blocker I raised: rather than patch the shell-continuity assumption, it added the check that can contradict "the namespace is empty" — leftover claims, Jobs and ConfigMaps, with imported claims excluded by name — so the verdict no longer rests on a per-chain loop having run. With the discovery transformations now status-checked, the library's traps confined to subshells, and the orphan sweep's read separated from its parse, I have no blocking findings left; what remains are accuracy and consistency notes, chiefly that the namespace summary line still claims every chain was verified when the loop may have run zero times, and that seidroid review · decision Findings: 0 blocking | 15 non-blocking | 3 posted inline | 1 pre-existing |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 604b772. Configure here.
| # An interrupt exits 130/143, which is above every verdict value. Without | ||
| # this branch the case matches nothing and the run ends silently. | ||
| *) printf 'ABORTED %s %s — interrupted or unexpected status %s; treat as unverified\n' \ | ||
| "$vt_ns" "$vt_sel" "$VT_WORST" ;; |
There was a problem hiding this comment.
EXIT trap zeros verifier status
High Severity
Converting verify_teardown and sweep_residual to subshell functions means return now exits a subshell and runs trap 'rm -rf …' EXIT. On dash (the documented target), that cleanup’s status replaces the function’s, so PRESENT or UNVERIFIED becomes 0. Callers then record a pass and report the teardown verified while objects or a failed read remain.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 604b772. Configure here.
…stale survivor list sweep_residual compared only the bare name against the survivors list, with no kind check, so an imported PVC named `data` also exempted job/data and configmap/data — all three kinds reported CLEAR at verdict 0. That is the same full-identity defect already fixed in expect_present, in code that should have reused the hardened matching rather than re-inventing it. Every exemption is now kind-scoped and compares the full `<kind>/<name>` identity that `-o name` prints, matching the shape the kube-root-ca.crt exemption already had. The namespace step built its survivors list with an unchecked `: > file`. In bash without set -e a failed redirection on that builtin leaves the OLD file intact and execution continues, so a previous run's list could authorise this run's exemptions: stale list readable, zero labelled chains, concat loop runs zero times, and a leaked PVC named `data` is exempted while the namespace reports success. The list is now built in a fresh per-namespace directory with every step checked; on failure the run records UNVERIFIED and passes `-` so no exemptions apply, rather than falling back to whatever was on disk. A missing per-chain imported-claims file is now its own explicit UNVERIFIED branch. Narrow the completion message to the kinds actually swept, and extend the sweep to `service` and `cronjob`: a leftover type=LoadBalancer Service bills with no pod running and the per-tenant ResourceQuota caps load balancers. Platform-owned objects stay out of scope — a workspace PR never owned them. Also add the ABORTED catch-all to the single-chain caller, so an interrupt that exits through the trap does not end nonzero in silence. Verified under dash and bash, with and without set -eu — including bash WITHOUT errexit, where the stale-list defect lives: imported pvc `data` alongside job/data and configmap/data now verdicts 1; a leftover Service verdicts 1; a stale list with an unwritable target verdicts 2 and grants no exemption; zero chains with a fresh empty list leaves a pvc unexempted; a missing imported-claims file verdicts 2; a real imported list still exempts correctly. All 22 prior scenarios still pass. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Passive voice: 'is told'. ASD-STE100 asks for the active voice.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is namespaced'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 43 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 47 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
| exit "$VERDICT" | ||
| ``` | ||
|
|
||
| That is the whole verification step. **If you find yourself writing a `poll_gone` line in this file, stop** — a second copy of the orchestration is how this exact defect survived four review rounds, reappearing one layer up each time: duplicated poll bodies, then the caller chain, then the arguments feeding the callers, then a canonical caller that bypassed the fixed library entirely while the paragraph above it said not to re-implement. |
There was a problem hiding this comment.
Descriptive sentence has 65 words. ASD-STE100 allows 25. Split it.
|
|
||
| That is the whole verification step. **If you find yourself writing a `poll_gone` line in this file, stop** — a second copy of the orchestration is how this exact defect survived four review rounds, reappearing one layer up each time: duplicated poll bodies, then the caller chain, then the arguments feeding the callers, then a canonical caller that bypassed the fixed library entirely while the paragraph above it said not to re-implement. | ||
|
|
||
| The empty-list cases are handled inside the function, as code rather than as advice here: an empty `managed-claims.txt` takes a `NOTE` branch instead of calling `poll_gone` with no names, because `kubectl get persistentvolumeclaim` with no arguments lists the whole namespace — the sweep this design exists to avoid. |
There was a problem hiding this comment.
Descriptive sentence has 42 words. ASD-STE100 allows 25. Split it.
|
|
||
| That is the whole verification step. **If you find yourself writing a `poll_gone` line in this file, stop** — a second copy of the orchestration is how this exact defect survived four review rounds, reappearing one layer up each time: duplicated poll bodies, then the caller chain, then the arguments feeding the callers, then a canonical caller that bypassed the fixed library entirely while the paragraph above it said not to re-implement. | ||
|
|
||
| The empty-list cases are handled inside the function, as code rather than as advice here: an empty `managed-claims.txt` takes a `NOTE` branch instead of calling `poll_gone` with no names, because `kubectl get persistentvolumeclaim` with no arguments lists the whole namespace — the sweep this design exists to avoid. |
There was a problem hiding this comment.
Passive voice: 'are handled'. ASD-STE100 asks for the active voice.
|
|
||
| ### A stuck `Terminating` object is a real signal | ||
|
|
||
| If the poll runs out with objects still present, do not report the teardown as done and do not force the objects away. Read why they are held: |
There was a problem hiding this comment.
Passive voice: 'are held'. ASD-STE100 asks for the active voice.
|
|
||
| ## Procedure: empty or remove my namespace | ||
|
|
||
| "Destroy my namespace" means one of two very different things. Ask which before acting. |
There was a problem hiding this comment.
'very' is a weasel word!
| rm -f "$derr" | ||
| ``` | ||
|
|
||
| Run `inventory.sh` **once per chain-id**, each writing its own `./teardown-inventory-<chain-id>`. A namespace usually holds more than one chain, and a single sweep cannot tell one chain's controller-managed claim from another's. Any chain whose `inventory.sh` exits non-zero writes no certificate, and its verification then reports `UNVERIFIED` — emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims no chain attributes land in each run's `unattributed-claims.txt`; take those to the leak sweep in step 5, not to a delete. |
There was a problem hiding this comment.
'usually' is a weasel word!
| rm -f "$derr" | ||
| ``` | ||
|
|
||
| Run `inventory.sh` **once per chain-id**, each writing its own `./teardown-inventory-<chain-id>`. A namespace usually holds more than one chain, and a single sweep cannot tell one chain's controller-managed claim from another's. Any chain whose `inventory.sh` exits non-zero writes no certificate, and its verification then reports `UNVERIFIED` — emptying a namespace on an incomplete inventory is how a leak becomes invisible. Claims no chain attributes land in each run's `unattributed-claims.txt`; take those to the leak sweep in step 5, not to a delete. |
There was a problem hiding this comment.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
| record "$rc" | ||
| ``` | ||
|
|
||
| **Do not print a verdict yet.** The check that can contradict "the namespace is empty" has not run: a namespace whose only leftover is a leaked PVC passes everything above — successful discovery, a clean CR/pod poll, exit 0. |
There was a problem hiding this comment.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
| ``` | ||
|
|
||
| **Do not print a verdict yet.** The check that can contradict "the namespace is empty" has not run: a namespace whose only leftover is a leaked PVC passes everything above — successful discovery, a clean CR/pod poll, exit 0. | ||
| 5. **Sweep the residuals, then decide.** Build the expected-survivors list from every chain's imported claims, sweep what is left, and only then print and exit: |
There was a problem hiding this comment.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
| exit "$VERDICT" | ||
| ``` | ||
|
|
||
| With `surv='-'` the sweep runs with **no** exemptions, so genuinely imported claims are reported as residuals. That is the safe direction and it is not the verdict: `record 2` already fired, and `2` dominates the `1` a residual would raise. |
There was a problem hiding this comment.
Passive voice: 'are reported'. ASD-STE100 asks for the active voice.
…tovers The sweep filters by Kind, not by ownership: its only ownership-aware exemptions are the imported claims and kube-root-ca.crt, so a platform-owned or controller-created Service, CronJob or ConfigMap is reported too. Saying it "deliberately leaves platform-owned objects alone" claimed more certainty than the implementation delivers — the same species of defect this PR exists to correct. Residuals are now candidates requiring ownership resolution, and the unswept kinds are described as unswept rather than as recognised and excluded. Caveat the ownership evidence for Service and CronJob: the platform base listing neither is what the repository declares, not what the cluster holds. Add the ABORTED catch-all to the recipe's single-chain caller, matching both callers in teardown.md. Record one line for the next editor beside the shared helpers: reuse the matching and status handling next door rather than re-deriving it. That was this file's recurring defect, not any single bug. No behaviour change — the verification library is byte-identical to the reviewed revision. Co-authored-by: omnigent <noreply@omnigent.ai>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
vale
Passive voice: 'is ******'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 33 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 30 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is left'. ASD-STE100 asks for the active voice.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 43 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 31 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
Descriptive sentence has 47 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is ********'. ASD-STE100 asks for the active voice.
Descriptive sentence has 27 words. ASD-STE100 allows 25. Split it.
Passive voice: 'is fixed'. ASD-STE100 asks for the active voice.
| 2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. | ||
| 3. **Every command that can fail runs inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before classification, and before the caller records anything. | ||
|
|
||
| **Adding a helper here? Reuse the matching and status handling beside it rather than re-deriving them.** Through this file's review history the recurring defect was never one bug — it was new code re-deriving logic already hardened next door, and arriving without the fix. `sweep_residual` was written comparing bare names in the same review round that `expect_present` was corrected to compare full identities, one function away. |
There was a problem hiding this comment.
Descriptive sentence has 29 words. ASD-STE100 allows 25. Split it.
| 2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. | ||
| 3. **Every command that can fail runs inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before classification, and before the caller records anything. | ||
|
|
||
| **Adding a helper here? Reuse the matching and status handling beside it rather than re-deriving them.** Through this file's review history the recurring defect was never one bug — it was new code re-deriving logic already hardened next door, and arriving without the fix. `sweep_residual` was written comparing bare names in the same review round that `expect_present` was corrected to compare full identities, one function away. |
There was a problem hiding this comment.
Passive voice: 'was written'. ASD-STE100 asks for the active voice.
| 2. **Identities are matched, not counted.** A count says how many lines came back, not whether the resources you asked about are the ones that came back. | ||
| 3. **Every command that can fail runs inside a condition.** Under `set -e` a bare `out=$(kubectl …)` terminates the shell at the assignment — before classification, and before the caller records anything. | ||
|
|
||
| **Adding a helper here? Reuse the matching and status handling beside it rather than re-deriving them.** Through this file's review history the recurring defect was never one bug — it was new code re-deriving logic already hardened next door, and arriving without the fix. `sweep_residual` was written comparing bare names in the same review round that `expect_present` was corrected to compare full identities, one function away. |
There was a problem hiding this comment.
Passive voice: 'was corrected'. ASD-STE100 asks for the active voice.
| -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' | ||
| ``` | ||
|
|
||
| **Zero PVCs is the wrong expectation, and a namespace-wide PVC poll is the wrong check.** The SeiNode finalizer deliberately skips an imported PVC, so imported claims survive by design and other chains' claims are none of this teardown's business. Both make a namespace sweep report `PRESENT` after a correct teardown. `verify_teardown` therefore polls the target's controller-managed claims by name and asserts the imported ones separately, from the lists `teardown.md` inventory step 2 captured **before** the SeiNodes were deleted — afterwards nothing in the cluster still says which claims were which. |
There was a problem hiding this comment.
Descriptive sentence has 39 words. ASD-STE100 allows 25. Split it.
| -o custom-columns='NAME:.metadata.name,PHASE:.status.phase,DELETED:.metadata.deletionTimestamp,FINALIZERS:.metadata.finalizers' | ||
| ``` | ||
|
|
||
| **Zero PVCs is the wrong expectation, and a namespace-wide PVC poll is the wrong check.** The SeiNode finalizer deliberately skips an imported PVC, so imported claims survive by design and other chains' claims are none of this teardown's business. Both make a namespace sweep report `PRESENT` after a correct teardown. `verify_teardown` therefore polls the target's controller-managed claims by name and asserts the imported ones separately, from the lists `teardown.md` inventory step 2 captured **before** the SeiNodes were deleted — afterwards nothing in the cluster still says which claims were which. |
There was a problem hiding this comment.
Passive voice: 'were deleted'. ASD-STE100 asks for the active voice.
| |---|---| | ||
| | Volume → PV → PVC → pod → a SeiNode confirmed orphaned by the signature | Reclaimable. Delete the **SeiNode**, not the volume — see below. | | ||
| | Volume → PV → PVC → pod → a live, wanted workload | **Not garbage.** Leave it. `available` only meant the workload was stopped. | | ||
| | Volume → PV → PVC whose claim is gone, PV `Released` | **Unresolved candidate.** Platform review required. Report the PV, PVC name, and reclaim policy; do not act on it here. | |
There was a problem hiding this comment.
Passive voice: 'is gone'. ASD-STE100 asks for the active voice.
| | `claimRef` names a namespace other than `eng-<alias>` | **Out of scope.** Another tenant's disk. Escalate; do not inspect. | | ||
| | Any hop returned `Forbidden`, errored, or found nothing | **UNRESOLVED.** Escalate as unresolved. Never as confirmed-safe. | | ||
|
|
||
| Only the first two rows are verdicts. Every other row is an escalation, and the platform team is told which row it came from. |
There was a problem hiding this comment.
Passive voice: 'is told'. ASD-STE100 asks for the active voice.
|
|
||
| Only the first two rows are verdicts. Every other row is an escalation, and the platform team is told which row it came from. | ||
|
|
||
| `kubectl get pv` is cluster-scoped and the per-engineer Role is namespaced, so `Forbidden` at hop 1 is the **normal** case for an engineer — an unresolved result, not a clean one. When it happens, hand the volume IDs to the platform team and let them walk the chain; do not substitute the tag data for the walk. |
There was a problem hiding this comment.
Descriptive sentence has 28 words. ASD-STE100 allows 25. Split it.
|
|
||
| Only the first two rows are verdicts. Every other row is an escalation, and the platform team is told which row it came from. | ||
|
|
||
| `kubectl get pv` is cluster-scoped and the per-engineer Role is namespaced, so `Forbidden` at hop 1 is the **normal** case for an engineer — an unresolved result, not a clean one. When it happens, hand the volume IDs to the platform team and let them walk the chain; do not substitute the tag data for the walk. |
There was a problem hiding this comment.
Passive voice: 'is namespaced'. ASD-STE100 asks for the active voice.
|
|
||
| Only the first two rows are verdicts. Every other row is an escalation, and the platform team is told which row it came from. | ||
|
|
||
| `kubectl get pv` is cluster-scoped and the per-engineer Role is namespaced, so `Forbidden` at hop 1 is the **normal** case for an engineer — an unresolved result, not a clean one. When it happens, hand the volume IDs to the platform team and let them walk the chain; do not substitute the tag data for the walk. |
There was a problem hiding this comment.
Descriptive sentence has 26 words. ASD-STE100 allows 25. Split it.


The ticket premise is false. PLT-1216 says teardown leaks disks because a
pruneoption is missing. Prune is already enabled —platform/clusters/harbor/engineers/base/sync.yaml:14, in the shared base that renders all nine engineer reconcilers. It works correctly for everything Flux owns. This PR changes no Flux YAML, noprunesetting, and nothing in the platform repo.What actually leaks the disks:
SeiNetwork.spec.deletionPolicydefaults toRetain. On deletion the reconciler deliberately strips the owner reference from the child SeiNodes rather than deleting them (sei-k8s-controllerinternal/controller/seinetwork/controller.go:135-143). The orphaned validators keep running, each still holding its PVC and its EBS volume. Flux prune cannot reach them — the controller created them, so they were never in Flux's inventory — and garbage collection has no owner reference left to follow. Storage-class reclaim is irrelevant, because reclaim only fires on a PVC that actually gets deleted.gp3-10k-750andgp3are alreadyreclaimPolicy: Delete; nothing needs changing there.This PR does not stop the leak. It is docs/skills only. The load-bearing fix is benchmark SeiNetworks carrying
deletionPolicy: Delete, tracked separately. What this PR does is stop the harbor-dev skill from reporting a successful teardown while disks are still running, and give the operator the procedure that prevents the orphaning in the first place.What this fixes
1. Wrong reconcile target. Teardown verification reconciled
flux-systemand read itslastAppliedRevision. That root Kustomization trackssei-protocol/platformatclusters/harbor— notharbor-engineering-workspace. So teardown verified a reconciler that never touched the engineer's change and reported success regardless. Workspace merges now target FluxKustomization <alias>in namespaceeng-<alias>(verified in-repo:references/onboarding-pr.md:110,references/cluster-inspection-recipes.mdrecipe #8).flux-systemstays correct for platform-repo merges (onboarding), and the split is now a table rather than a single command.2. No resource-disappearance check. A successful reconcile only means Flux issued the deletes. Added an explicit poll of the SeiNetwork/SeiNodes/PVCs on a bounded budget, with the caveat stated: deletion is asynchronous and
sei.io/seinode-finalizerholds a SeiNode until the controller releases its PVC. An object stillTerminatingpast the budget is a reportable finding — stripping the finalizer to make the check pass is exactly how a stuck teardown becomes a leaked disk.3. The
deletionPolicy: Retaintrap was undocumented. Now a guardrail, a procedure step, a halt condition, and a section of the new reference.deletionPolicyis mutable (no CELXValidation, no webhook —SeiNetworkSpeccarries exactly three immutability rules:spec.genesis,spec.replicas,spec.dataVolume), so an operator can patch a live SeiNetwork toDelete. The ordering is the load-bearing part: the policy change must land in git and reconcile before the removal merges. A barekubectl patchon a Flux-owned SeiNetwork is drift — the rendered manifest normally carries the server-defaultedRetain, so Flux owns the field and reverts the patch on its next pass, typically while the removal PR is in review. And once aRetaindeletion has stripped the owner references and removed the parent, nothing restores the cascade at all.4. No "destroy my namespace" procedure. Added, and honest about the boundary: a workspace PR removes the engineer's workloads only. The
Namespaceobject, the three ServiceAccounts, the RBAC, and the Flux Kustomization itself are platform-owned and stay. So does anything created outside git — an escape-hatch apply, or an orphan from an earlierRetainteardown.5. Teardown skipped the PR contract. Spinup goes through a PR; teardown now does too — branch, commit,
gh pr create, merge, reconcile, verify.6. No cleanup path for already-leaked resources. Added. Orphan signature:
sei.io/role=validatorand noownerReferencesand no live parent. Absence of owner references alone is not the signal — a follower fromseictl node applyis a top-level object and legitimately has none. Cleanup iskubectl delete seinodeon the confirmed orphan: its finalizer deletes the PVC and theDeletereclaim policy releases the volume. Disk signals are candidates, never verdicts — EC2availablemeans unattached, not unowned, and a volume backing a liveBoundPVC readsavailablethe moment its workload stops. Ownership resolves by walking volume ID → PV (spec.csi.volumeHandle) →claimRef→ PVC → workload; any hop that isForbidden, errors, or finds nothing escalates as UNRESOLVED, never as confirmed-safe. This skill never deletes an EBS volume.Explicitly not done
prunesetting, no platform-repo change. Prune is correct.deleteonpersistentvolumeclaimsadded to the per-engineer Role. That restriction is deliberate: the controller's finalizer owns PVC lifecycle, PVCs are not in the workspace repo so prune never targets them, and widening the Role would be a security regression that does not fix this bug. The new reference says so in place, so the next reader does not re-propose it.Files
references/teardown.mdRetaintrap and patch-before-delete ordering, the PR-based procedure, the reconcile target + disappearance poll, the namespace procedures, the leaked-resource sweep, and seven halt conditions.SKILL.mdProcedure: tear down (PR-based); three halt conditions; teardown + namespace intent rows; reference-index entry; step-11/step-16 pointers.references/cluster-inspection-recipes.mdTerminatingread) and #10 (orphan detection); recipe #8 states it is the target for workspace merges;bench:teardownreconciles<alias>and confirms the objects went away.references/seinetwork-crd.mddeletionPolicydocumented as the disk-leak field: the orphan mechanism, mutability, the closing window, and what theDeletecascade does end to end.references/ephemeral-chain-flow.mddeletionPolicybefore thegit rm.references/comparative-bench.mdreferences/preflight.mdreferences/troubleshooting-seinode.mdRetainnote distinguishes forensics from a leak.README.mdevals/evals.jsonRetainhalt (empty ≠ safe; forbids the prune/reclaim/Role "fixes") and a clean teardown happy path.Verification
Docs/skills only — no build, no module compile.
GONE/PRESENT/UNVERIFIED, capturingkubectl's exit status separately rather than counting lines from a pipeline. AForbiddenor dropped connection isUNVERIFIED, never a pass. The poll deadline is arithmetic ondate +%s; Bash'sSECONDSis unset undersh, where the comparison fails withIllegal numberand the loop never runs. Both behaviours were reproduced under this host's/bin/shbefore and after the fix.writing/scripts/lint.sh(Vale 3.17.1, aftervale sync): 0 errors on every added line (530 added lines across 9 changed.mdfiles). Warnings remain (sentence length, passive voice) at the same density as the surrounding file; CI'sfail_leveliserror.xreview/scripts/skill-package-checks.sh --skill-dir .claude/skills/harbor-dev: no block-severity failure. Same two pre-existing non-block failures asmain(T2info: missingstate/.gitkeep;A1warn: time-sensitive content inSKILL.md/preflight.md/seictl-cli.md).B1passes —SKILL.mdis 396 lines against the 500 limit.writing/scripts/check-{verifiers,artifact-length,coverage,contract-anchors,anchor-authorities,generated-rules,admission,consumer-scoping,template-deltas}.sh: all pass.scripts/verify-references.sh: no error-class finding.scripts/sync-skills.sh --verify: catalog coverage OK.scripts/tests/skill-package-checks.test.sh: 47 passed, 0 failed.evals.jsonparses; the diff is 49 insertions, 0 deletions.No command in this PR was run against a live cluster — none was available. Every documented invocation is derived from the repo's existing conventions.
Inferred rather than verified
--with-sourceon the workspace Kustomization. TheGitRepositoryit references comes from the platform-repo base (clusters/harbor/engineers/base/sync.yaml), which is not in this repo, so its namespace is unverified. If it sits outsideeng-<alias>, the engineer's namespace-scoped Role returnsForbidden. The docs state that failure mode and the fallback (reconcile the Kustomization alone) rather than asserting the flag always works.kubernetes.io/created-for/pvc/namespaceand.../pvc/nameare the EBS CSI driver's own convention, not something these repos set. The doc tells the operator to confirm the keys against a known-live volume before trusting an empty filter result.references/onboarding-pr.md; no worked offboarding PR exists to check it against. Marked as such in place, and routed to the platform team rather than presented as a self-service runbook.The Kustomization name for fix 1 was verified, not inferred —
<alias>ineng-<alias>, fromreferences/onboarding-pr.md:110and recipe #8.🤖 Generated with Claude Code
Fix round — cross-review findings addressed
All three P1s, the P2, and the three non-blocking items are fixed on this branch. Commits
85cca82,f470156,a65e1b3,8b9ffea.kubectl | wc -ltakeswc's exit status, so aForbiddenyielded zero lines and printed "all objects gone"kubectl's status separately and reports one ofGONE/PRESENT/UNVERIFIED, with distinct exit codes 0/1/2.UNVERIFIEDis a halt condition. Reproduced the false pass under/bin/sh, then reproduced all three outcomes correctly after the fix.Retainfrom git back over the patch, typically mid-reviewSKILL.md,seinetwork-crd.md,ephemeral-chain-flow.md,comparative-bench.md, and both evals.available/Used By: <none>treated as proof of garbage, routing live disks into a deletion escalationspec.csi.volumeHandle→claimRef→ PVC → workload) and a verdict table whose every failed hop lands onUNRESOLVED.kubectl get pvis cluster-scoped, soForbiddenis called out as the normal engineer case and an unresolved result. This skill now never deletes an EBS volume.SECONDSin anshfence skips the poll; PVC and bench checks were single reads; PVC expectation contradicted the imported-PVC exceptiondate +%sdeadline.poll_gonein recipe #9 is reused for CRs, PVCs, pods, and bench Jobs/ConfigMaps — bench now polls pods too, since a Job can be gone while its pod isTerminating. The PVC expectation is no longer zero: controller-managed PVCs must go, imported PVCs must survive, and inventory step 2 now records which nodes carryspec.importbecause nothing says so after they are deleted.SeiNodeTaskWorkflow, hand-applied bench Jobs, controller-managed PVCs, and S3 artifacts — each with an escalation or a guarded path. Anything not in it escalates as unresolved.Forbiddenon--with-sourcestated as diagnosticGitRepository, alongside expired session, missing access entry, or a Role without the Flux verbs. Read the message first.--filters" would enumerate other tenants' volumes--volume-ids <known-live-id>, scoped to one volume the operator already knows.Also corrected, and it was wrong in the original body as well as the doc:
spec.resourceswas listed among the immutableSeiNetworkSpecfields. There are exactly three immutability rules —spec.genesis,spec.replicas,spec.dataVolume— andSeiNetworkSpechas noresourcesfield. Removed from both.Untouched by design, per the review: the reconcile target (A), PR parity (E), the orphan-detection triple, the legitimate
flux-systemcommand inpreflight.md, and the three inference caveats.Gates after the fix round
.mdfiles.skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures asmain(T2info,A1warn), no block failure.B1passes at 396/500 lines.writing/scripts/check-*.sh: pass.verify-references.sh,sync-skills.sh --verify,skill-package-checks.test.sh(47/0): pass.evals.jsonparses; internal anchors resolve.Round 3 — cross-review findings addressed
All nine fixed. Commits
dfc6d90,4b2e6ab,95b3f7c,45a623c. No finding was disputed; findings 1, 2 and 4 were reproduced or re-derived locally before fixing.teardown.mdre-implemented the poll twice instead of calling the sharedpoll_gone, and the copies had already diverged — the PVC loop emitted no verdict at all, just counts until its deadlineUNVERIFIEDstill exited 0 —breaksucceeds, so the completedwhilereturned 0, and no caller checkedpoll_gone's return code eitherrecord/VERDICTaggregation where the worst outcome wins (0 GONE < 1 PRESENT < 2 UNVERIFIED) and every call site records. Confirmed underdash:break-after-UNVERIFIEDdoes exit 0; after the fix,UNVERIFIEDthenGONEaggregates to 2..spec.importdoes not exist — the field is.spec.dataVolume.import.pvcName, so the query matched nothing and every imported claim was reclassified as one that must disappearclaimName, imported vs controller-managed classification, abort on any API or parse failure. Poll named managed claims; assert imported ones withexpect_present. Field path caveated as repo-main API inspection, not the deployed CRD. Also corrected the three pre-existingspec.importprose mentions elsewhere in the skill — see "scope call" below.claimReffollowed without a namespace check; deletes without namespace/context; a failedgrepread as "no manifest"claim_ns != eng-<alias>escalation gate; every delete names--context harborand-n eng-<alias>; the authorizing search runs against a freshly fetched clone and branches ongrep's exit status — only 1 (no match) authorizes a delete, 2+ is unresolved. Verified the three exit codes.sed -n '/Used By/,+3p'is a GNU extension; blanket POSIX claimdash,ash,bash)" and namesdate +%sas a deliberate near-universal extension rather than POSIX.DeletealoneDeletefor a Flux-owned object; the live-patch forbidden signal no longer has an escape clause; the happy path requires named-claim polling with aggregation.--with-sourcefallback claimed to help "whatever the cause"One extra defect found while fixing 4.
jq'suniquesorts by codepoint;commassumes its input is sorted the way the locale collates, and a locale that ignores punctuation orders hyphenated claim names differently. The inventory script now pinsLC_ALL=C.Scope call to flag. Finding 4 was scoped to
teardown.md, but the same wrong path appeared in three pre-existing prose lines introubleshooting-seinode.mdand one inseinetwork-crd.md. Leaving them would have made the skill contradict itself on an API path, so they were corrected tospec.dataVolume.import— prose only, no query. Revert if you would rather keep that out of this PR.Gates after round 3
.mdfiles (onewrite-good.ThereIsintroduced and fixed mid-round).skill-package-checks.sh: 20 pass / 2 fail / 4 skipped — the same two non-block failures asmain(T2info,A1warn).B1passes at 396/500.writing/scripts/check-*.shpass.verify-references.sh,sync-skills.sh --verify,skill-package-checks.test.sh(47/0) pass.evals.jsonparses; internal anchors resolve.jqfilters. This host has nojq, and installing one was out of budget — they are reviewed by inspection only. The shell control flow around them (aggregation,expect_present,set -euabort on a redirected failure,grepexit codes,comm) was executed under/bin/sh.