sixfold-space/madtea

SHA256

Fork 0

classify the destructive operations ADR 0029's table never named #569

New issue

Open

opened 2026-09-21 21:09:48 +00:00 by hexajon · 2 comments

hexajon commented

2026-09-21 21:09:48 +00:00

Owner

Part of #563. This is the child the other four keep running into.

Two policies, both green, neither aware of the other

ADR 0029's per-operation table classifies nine operations. The surface that ships is governed by a second, older policy: internal/parity/destructive_remote_pin_test.go pins roughly fifty delete-shaped MCP operations under the #120 rule that an irreversible operation must be gated behind madt_enable.

ADR 0029 clause 1 rejects that rule by name. A gated domain is not a substitute for omission, because the gate degrades and madt_enable carries no consent step. So the pin test asserts a property the record says is insufficient, and both pass CI.

Unclassified today

Destructive on at least one surface, absent from the table:

packages/delete, which destroys uploaded package bytes. The CLI has no domain gating at all (internal/cmd/packages/packages.go:55).
admin/unadopted-delete, which deletes an on-disk repository.
git-objects/note-delete (internal/mcp/tools_gitobjects.go:64, internal/cmd/git/gitobjects.go:135). The replaced note object becomes unreachable.
releases/delete-asset, ungated on both surfaces (internal/mcp/tools_releases.go:103, internal/cmd/release/release_assets.go:108). ADR 0029 omits release delete because notes and assets are unrecoverable, so deleting assets one at a time is a subset of the destruction the record already refused.
Eight hard deletes with no forge undelete: issue comment, issue attachment, issue comment attachment, PR comment, PR review comment, actions run, actions artifacts, repo git hooks (pinned at internal/parity/destructive_remote_pin_test.go:120-155).
madt_repos action=transfer (internal/mcp/tools_repos.go:161) and action=convert_fork (line 227). Both are one way and neither is delete-shaped, so the pin test cannot see them.
madtea tag -d and tag -f (internal/cmd/git/tag.go:51,55), CLI only. Unlike git branch -d there is no merged refusal, and an annotated or signed tag's own object becomes unreachable. The record reasons about exactly this for the forge tag and classified it omit.
madt_labels force=true (internal/mcp/tools_labels.go:40,98,259) overwrites an existing label definition. Low severity, still unnamed.

The table is incomplete in the safe direction too. reset --hard (internal/cmd/git/reset.go:62-68), checkout -- <files> (checkout.go:59), and stash drop and clear (stash.go:124-128) are correctly absent from every surface and the record never says so.

The Confirmation section cannot see the raw passthrough

ADR 0029 says omission is "verifiable by the absence of the flag in the CLI set and of the operation in the tool schema". The raw API passthrough forwards a caller-supplied method to the forge, and its method allowlist includes DELETE with no path filter (internal/service/api/call.go:195, no guard in internal/mcp/tools_api.go). So the forge delete endpoints behind the omitted operations stay reachable from the agent surface, and the stated verification method cannot detect it.

I am not proposing the passthrough be removed. It is deliberate and useful. The defect is that the record claims a property the surface does not have and never mentions the passthrough at all. Either the record scopes its claim to first-class operations and says why the passthrough is an accepted exception, or the passthrough grows a filter.

Documentation rot

docs/reference/destructive.md reads as an exhaustive list of the omitted deletes and is not. It also never mentions the no-force rule for worktree removal, which is the case that produced the record. Seven code comments still cite the pre-migration path docs/adr/0029-..., including internal/cmdutil/deleterefusal.go:44, and internal/mcp/tools_localgit.go:42-44 still advertises a worktree force path the record deleted.

Acceptance criteria

Every destructive-capable operation on either surface carries a classification: omit, machine-proof, already-safe, or out of scope with a reason.
The #120 gated policy and ADR 0029 are reconciled into one rule, and the pin test asserts that rule rather than the older one.
The record's Confirmation section describes a verification that can actually see the whole surface.
docs/reference/destructive.md matches shipped behaviour and stops reading as exhaustive unless it is.
The stale record paths and the stale worktree force text are corrected.

Part of #563. This is the child the other four keep running into. ## Two policies, both green, neither aware of the other ADR 0029's per-operation table classifies nine operations. The surface that ships is governed by a second, older policy: `internal/parity/destructive_remote_pin_test.go` pins roughly fifty delete-shaped MCP operations under the #120 rule that an irreversible operation must be gated behind `madt_enable`. ADR 0029 clause 1 rejects that rule by name. A gated domain is not a substitute for omission, because the gate degrades and `madt_enable` carries no consent step. So the pin test asserts a property the record says is insufficient, and both pass CI. ## Unclassified today Destructive on at least one surface, absent from the table: - `packages/delete`, which destroys uploaded package bytes. The CLI has no domain gating at all (`internal/cmd/packages/packages.go:55`). - `admin/unadopted-delete`, which deletes an on-disk repository. - `git-objects/note-delete` (`internal/mcp/tools_gitobjects.go:64`, `internal/cmd/git/gitobjects.go:135`). The replaced note object becomes unreachable. - `releases/delete-asset`, ungated on both surfaces (`internal/mcp/tools_releases.go:103`, `internal/cmd/release/release_assets.go:108`). ADR 0029 omits `release delete` because notes and assets are unrecoverable, so deleting assets one at a time is a subset of the destruction the record already refused. - Eight hard deletes with no forge undelete: issue comment, issue attachment, issue comment attachment, PR comment, PR review comment, actions run, actions artifacts, repo git hooks (pinned at `internal/parity/destructive_remote_pin_test.go:120-155`). - `madt_repos action=transfer` (`internal/mcp/tools_repos.go:161`) and `action=convert_fork` (line 227). Both are one way and neither is delete-shaped, so the pin test cannot see them. - `madtea tag -d` and `tag -f` (`internal/cmd/git/tag.go:51,55`), CLI only. Unlike `git branch -d` there is no merged refusal, and an annotated or signed tag's own object becomes unreachable. The record reasons about exactly this for the forge tag and classified it omit. - `madt_labels force=true` (`internal/mcp/tools_labels.go:40,98,259`) overwrites an existing label definition. Low severity, still unnamed. The table is incomplete in the safe direction too. `reset --hard` (`internal/cmd/git/reset.go:62-68`), `checkout -- <files>` (`checkout.go:59`), and `stash drop` and `clear` (`stash.go:124-128`) are correctly absent from every surface and the record never says so. ## The Confirmation section cannot see the raw passthrough ADR 0029 says omission is "verifiable by the absence of the flag in the CLI set and of the operation in the tool schema". The raw API passthrough forwards a caller-supplied method to the forge, and its method allowlist includes DELETE with no path filter (`internal/service/api/call.go:195`, no guard in `internal/mcp/tools_api.go`). So the forge delete endpoints behind the omitted operations stay reachable from the agent surface, and the stated verification method cannot detect it. I am not proposing the passthrough be removed. It is deliberate and useful. The defect is that the record claims a property the surface does not have and never mentions the passthrough at all. Either the record scopes its claim to first-class operations and says why the passthrough is an accepted exception, or the passthrough grows a filter. ## Documentation rot `docs/reference/destructive.md` reads as an exhaustive list of the omitted deletes and is not. It also never mentions the no-force rule for worktree removal, which is the case that produced the record. Seven code comments still cite the pre-migration path `docs/adr/0029-...`, including `internal/cmdutil/deleterefusal.go:44`, and `internal/mcp/tools_localgit.go:42-44` still advertises a worktree force path the record deleted. ## Acceptance criteria - Every destructive-capable operation on either surface carries a classification: omit, machine-proof, already-safe, or out of scope with a reason. - The #120 gated policy and ADR 0029 are reconciled into one rule, and the pin test asserts that rule rather than the older one. - The record's Confirmation section describes a verification that can actually see the whole surface. - `docs/reference/destructive.md` matches shipped behaviour and stops reading as exhaustive unless it is. - The stale record paths and the stale worktree force text are corrected.

hexajon added a new dependency

2026-09-21 21:09:55 +00:00

#563 Epic: the destructive surface has outgrown ADR 0029's classification table

hexajon referenced this issue

2026-09-21 21:11:48 +00:00

destructive.md is generated from a hardcoded string, so it cannot track the surface it describes #570

hexajon commented

2026-09-21 21:26:52 +00:00

Author

Owner

Adding the mechanism behind the classification gap, which the issue body does not yet name.

The pin test does not merely list the ungated deletes. It justifies each one in a trailing comment. There are 24 ungated rows (internal/parity/destructive_remote_pin_test.go:126-155), and all 24 carry a rationale:

"issues/comment-delete":   ungated, // comment - re-postable
"branches-remote/delete":  ungated, // remote branch - recoverable from its tip sha
"files/delete":            ungated, // file contents delete - recoverable from git history
"releases/delete-asset":   ungated, // release asset - re-uploadable

Nothing computes any of them. ADR 0029 clause 2 says the proof must be structural and computed, never an assertion, so 24 operations are licensed by exactly the thing the record excludes.

Two rationales are demonstrably wrong, not merely unproven:

"recoverable from its tip sha" assumes the sha is available. branchesDelete (internal/mcp/tools_branches_crud.go:41-50) fetches no sha before the delete and returns only the branch name. Nothing records the value the rationale depends on.
"recoverable from git history" holds only while the containing ref survives. branches-remote/delete removes a ref with no containment proof, and that operation is two rows below this one in the same list. Each rationale leans on a guarantee the other does not provide.

"Re-postable" is a third case, and a judgment rather than an error. Writing a new comment does not restore the original's id, timestamp, reactions, edit history, or position in a thread. Whether that identity data is unrecoverable work is a product call, so I am not settling it here. The same reading applies to issues/attachment-delete and the two PR comment deletes.

This matters for the fix. Reclassifying the operations one by one leaves the mechanism in place, and the next operation added to the list gets waved through by a fresh comment. Whatever replaces the rationale needs to be a computed check or an explicit recorded decision, not a sentence.

Adding the mechanism behind the classification gap, which the issue body does not yet name. The pin test does not merely list the ungated deletes. It justifies each one in a trailing comment. There are 24 ungated rows (`internal/parity/destructive_remote_pin_test.go:126-155`), and all 24 carry a rationale: ```go "issues/comment-delete": ungated, // comment - re-postable "branches-remote/delete": ungated, // remote branch - recoverable from its tip sha "files/delete": ungated, // file contents delete - recoverable from git history "releases/delete-asset": ungated, // release asset - re-uploadable ``` Nothing computes any of them. ADR 0029 clause 2 says the proof must be structural and computed, never an assertion, so 24 operations are licensed by exactly the thing the record excludes. Two rationales are demonstrably wrong, not merely unproven: - "recoverable from its tip sha" assumes the sha is available. `branchesDelete` (`internal/mcp/tools_branches_crud.go:41-50`) fetches no sha before the delete and returns only the branch name. Nothing records the value the rationale depends on. - "recoverable from git history" holds only while the containing ref survives. `branches-remote/delete` removes a ref with no containment proof, and that operation is two rows below this one in the same list. Each rationale leans on a guarantee the other does not provide. "Re-postable" is a third case, and a judgment rather than an error. Writing a new comment does not restore the original's id, timestamp, reactions, edit history, or position in a thread. Whether that identity data is unrecoverable work is a product call, so I am not settling it here. The same reading applies to `issues/attachment-delete` and the two PR comment deletes. This matters for the fix. Reclassifying the operations one by one leaves the mechanism in place, and the next operation added to the list gets waved through by a fresh comment. Whatever replaces the rationale needs to be a computed check or an explicit recorded decision, not a sentence.

hexajon commented

2026-09-21 21:33:48 +00:00

Author

Owner

Correcting my previous comment on one point, and adding a distinction it flattened.

I wrote that all 24 ungated rows carry a rationale. Enumerated, 21 carry one. Three carry only a bare noun phrase and no justification of any kind:

"issues/time-delete":      ungated, // tracked-time entry
"issues/stopwatch-delete": ungated, // running stopwatch
"repos/avatar-delete":     ungated, // repo avatar

Those three are a separate case, and arguably the weaker one. A wrong rationale can at least be read and disputed. A row that says only what the operation is was classified ungated with nothing recorded about why it is safe to be.

The count matters beyond pedantry. The rows run 126 to 155 and total 24. Three separate passes over this file produced 22, 22, and 25. Whatever replaces the rationale comments should be countable by something other than a person reading a list, or the next audit of it will disagree with this one too.

Correcting my previous comment on one point, and adding a distinction it flattened. I wrote that all 24 ungated rows carry a rationale. Enumerated, 21 carry one. Three carry only a bare noun phrase and no justification of any kind: ```go "issues/time-delete": ungated, // tracked-time entry "issues/stopwatch-delete": ungated, // running stopwatch "repos/avatar-delete": ungated, // repo avatar ``` Those three are a separate case, and arguably the weaker one. A wrong rationale can at least be read and disputed. A row that says only what the operation is was classified ungated with nothing recorded about why it is safe to be. The count matters beyond pedantry. The rows run 126 to 155 and total 24. Three separate passes over this file produced 22, 22, and 25. Whatever replaces the rationale comments should be countable by something other than a person reading a list, or the next audit of it will disagree with this one too.

hexajon referenced this issue

2026-09-21 23:49:09 +00:00

chore(agents): track repo-scoped agent definitions, add the destruction-model auditor #574

hexajon referenced this issue from a commit

2026-09-21 23:49:10 +00:00

chore(agents): add a repo-scoped auditor for the destruction model

hexajon referenced this issue

2026-10-01 18:37:26 +00:00

comment_delete permanently deletes comments with no guard, unlike every other irreversible delete #687