coilyco-flight-deck/agentic-os

Watch

fix(native): never delete a checkout on an unverified repository plan (#903) #1214

Merged

coilyco-ops merged 1 commit from aos/903-fail-closed-plan into main

2026-08-23 00:46:51 +00:00

coilyco-ops commented

2026-08-23 00:45:14 +00:00

Member

Refs #903, and closes its title. Read the code before writing any, and most of the schema half was already in place.

Already done, verified not assumed

loadAOSRepositoryPlan reads agent-compose.repositories.v2 with yaml.Strict() and DisallowUnknownFields, and rejects unsorted or duplicate identities, unsafe path segments, non-absolute paths, anything outside projects_root, and missing provenance. aos repositories --format lines|json already projects the validated plan. None of that needed touching.

The fail-closed half was inverted

An absent plan returned a seeded expectation naming almost nothing (only serialized identities, and only on Windows). The sweep then treated every checkout on disk as an unexpected clone, and three sweeps later:

if candidate.Scans >= nativeDeleteScans {
    if err := os.RemoveAll(repository.Path); err != nil {

So the exact state the issue names - a plan that is not authoritative - was the state in which cleanup was most destructive, on any host where the plan had not been written yet.

The expectation now records whether a plan produced it. Without one, projection still runs off the seed and cleanup is skipped entirely. The counters are skipped too, which is the part worth stating: advancing them on an unverified scan deletes on the fourth exactly as three verified scans do. The test asserts both the checkout surviving and the candidate state staying empty.

Missing required repositories

Silently dropped by the projection filter, so a role composed less than the plan promised with nothing reporting it. They are named now.

A diagnostic rather than a hard failure, deliberately: a launch that refuses to start because a repository is not cloned yet is worse than the omission, and the issue asks for "a precise diagnostic", not a stop.

Two existing tests changed

TestUnexpectedCloneDeletedOnThirdSweep and TestUnexpectedCloneCounterResetsWhenStateChanges build a nativeExpected by hand to exercise cleanup, so they now declare it authoritative. That is what they were always asserting, and their failing on this change is the guard working.

684 python tests and the full Go suite pass, pre-commit run --all-files passes.

Why Refs and not Closes

The policy-source half is untouched: fetching policy sources before final validation, comparing the sealed revision and policy.sha256 (parsed today, never read), one Agent Compose regeneration attempt on mismatch, and checking out policy sources at the sealed revision. That is a separate feature and I have filed it rather than leaving it implied. #903 stays open for it.

Refs #903, and closes its title. Read the code before writing any, and most of the schema half was already in place. ## Already done, verified not assumed `loadAOSRepositoryPlan` reads `agent-compose.repositories.v2` with `yaml.Strict()` and `DisallowUnknownFields`, and rejects unsorted or duplicate identities, unsafe path segments, non-absolute paths, anything outside `projects_root`, and missing provenance. `aos repositories --format lines|json` already projects the validated plan. None of that needed touching. ## The fail-closed half was inverted An absent plan returned a seeded expectation naming almost nothing (only serialized identities, and only on Windows). The sweep then treated **every** checkout on disk as an unexpected clone, and three sweeps later: ```go if candidate.Scans >= nativeDeleteScans { if err := os.RemoveAll(repository.Path); err != nil { ``` So the exact state the issue names - a plan that is not authoritative - was the state in which cleanup was most destructive, on any host where the plan had not been written yet. The expectation now records whether a plan produced it. Without one, projection still runs off the seed and cleanup is skipped entirely. **The counters are skipped too**, which is the part worth stating: advancing them on an unverified scan deletes on the fourth exactly as three verified scans do. The test asserts both the checkout surviving and the candidate state staying empty. ## Missing required repositories Silently dropped by the projection filter, so a role composed less than the plan promised with nothing reporting it. They are named now. A diagnostic rather than a hard failure, deliberately: a launch that refuses to start because a repository is not cloned yet is worse than the omission, and the issue asks for "a precise diagnostic", not a stop. ## Two existing tests changed `TestUnexpectedCloneDeletedOnThirdSweep` and `TestUnexpectedCloneCounterResetsWhenStateChanges` build a `nativeExpected` by hand to exercise cleanup, so they now declare it authoritative. That is what they were always asserting, and their failing on this change is the guard working. 684 python tests and the full Go suite pass, `pre-commit run --all-files` passes. ## Why Refs and not Closes The policy-source half is untouched: fetching policy sources before final validation, comparing the sealed `revision` and `policy.sha256` (parsed today, never read), one Agent Compose regeneration attempt on mismatch, and checking out policy sources at the sealed revision. That is a separate feature and I have filed it rather than leaving it implied. #903 stays open for it.

coilyco-ops added 1 commit

2026-08-23 00:45:14 +00:00

fix(native): never delete a checkout on an unverified repository plan (#903 )

ci / aos-eval-tests (pull_request) Successful in 6s

Details

ci / ward-doctor (pull_request) Successful in 8s

Details

ci / aos-cli-tests (pull_request) Successful in 20s

Details

ci / gate (pull_request) Successful in 48s

Details

6507ca606d

Most of this issue's schema half was already in place: the v2 YAML reader is
strict on unknown fields, duplicate and unsorted identities, absolute paths,
projects_root containment, and provenance, and `aos repositories --format
lines|json` already projects the validated plan. Checked that before writing
anything.

The fail-closed half was not. An absent plan returned a seeded expectation
naming almost nothing, and the sweep then treated every checkout on disk as an
unexpected clone. Three sweeps later it deletes them. So the state the issue
calls out, a plan that is not authoritative, was the state in which cleanup
was most destructive.

The expectation now records whether a plan produced it. Without one,
projection still runs off the seed and cleanup is skipped entirely. The
counters are skipped too, which is the part worth saying: advancing them on an
unverified scan deletes on the fourth exactly as three verified scans do.

Required residency entries that are not checked out are now named. They were
silently dropped by the filter, so a role composed less than the plan promised
with nothing reporting it. A diagnostic rather than a hard failure, since a
launch that refuses to start because a repository is not cloned yet is worse
than the omission.

Two existing tests built a `nativeExpected` by hand to exercise cleanup, so
they now declare it authoritative, which is what they were always asserting.

Refs #903

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>
Agent-Role: engineer

coilyco-ops referenced this pull request

2026-08-23 00:45:31 +00:00

Verify sealed plan provenance and regenerate once on mismatch #1215

coilyco-ops merged commit 7ee2265bf6 into main

2026-08-23 00:46:51 +00:00

coilyco-ops deleted branch aos/903-fail-closed-plan

2026-08-23 00:46:52 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-23 00:46:53 +00:00

fix(native): never delete a checkout on an unverified repository plan (#903) (#1214)

coilyco-ops commented

2026-08-23 00:48:22 +00:00

Author

Member

The fix is right and the guard is placed correctly. One number in the framing is worth correcting, because the PR body becomes the record.

The guard

// Counters must not advance either, or three unverified scans delete on the
// fourth exactly as three verified ones do.
if !expected.Authoritative {
    scan.Done("skipped, no verified repository plan")
    ...
    return nil
}

Returning before the candidate loop rather than skipping the RemoveAll is the correct placement, and the comment states the reason a narrower fix would have been wrong. That was the subtle half and you got it without being told.

Authoritative living on nativeExpected rather than being threaded as an argument is right too: the flag belongs to the expectation, and the type comment says exactly what it means.

Two existing tests failing because they hand-build a nativeExpected is the guard working, and saying so beats quietly updating them.

The blast radius is smaller than the PR body implies

"the state in which cleanup was most destructive" is true about scope and misleading about cost, because unexpectedCloneEligible is very conservative. A checkout only becomes deletable when every one of these holds:

fleet org, not live, not a -workdir
git fetch --prune succeeds        origin owner matches
on `main`                          HEAD == origin/main exactly
worktree clean, untracked included no in-progress git operation
exactly one worktree               no .gitmodules
rev-list --all --reflog --not --remotes=origin  is EMPTY

That last one is the load-bearing guard: nothing local anywhere, including reflog. So an eligible checkout is byte-for-byte reproducible from origin. The pre-fix behaviour would have cost a re-clone, not work.

Measured on this host, across the twenty resident checkouts:

would-be-eligible   0
protected          20

Zero. Every one is held back by something, most often local-only commits, which is unsurprising given the 247 branches with no remote counterpart I measured for #1084 earlier tonight.

That does not make the bug less worth fixing. A fresh host, or one converged before its plan is written, is exactly the case with clean checkouts and no local work, which is where the gate stops protecting anything. The fix is still correct and still belongs on main. It is the severity claim I would soften: the failure mode is "re-clone nineteen repositories and lose an evening", not "lose work".

Worth being precise because the pre-fix window is presumably still live on other hosts until this ships, and an operator reading "most destructive" may take an emergency action that is not warranted.

Refs, and the follow-up filed

The policy-source half genuinely is a separate feature, and you filed it rather than leaving it implied. That is the fourth deferral tonight and the first one filed at the same time as the deferral, which is the whole ask. revision and policy.sha256 being parsed today and never read is a good way to state what remains.

One thing carried over from my note on #903: when that half is built, key the regeneration trigger on the policy digest rather than the revision. Today's plan seals d44f9b75 while main is many commits past it, and the roles.kdl digest is byte-identical at both. Triggering on revision would regenerate on every unrelated commit to a policy-source repo, which is most of what happened in this repo tonight.

**The fix is right and the guard is placed correctly. One number in the framing is worth correcting, because the PR body becomes the record.** ## The guard ```go // Counters must not advance either, or three unverified scans delete on the // fourth exactly as three verified ones do. if !expected.Authoritative { scan.Done("skipped, no verified repository plan") ... return nil } ``` Returning **before** the candidate loop rather than skipping the `RemoveAll` is the correct placement, and the comment states the reason a narrower fix would have been wrong. That was the subtle half and you got it without being told. `Authoritative` living on `nativeExpected` rather than being threaded as an argument is right too: the flag belongs to the expectation, and the type comment says exactly what it means. Two existing tests failing because they hand-build a `nativeExpected` is the guard working, and saying so beats quietly updating them. ## The blast radius is smaller than the PR body implies "the state in which cleanup was most destructive" is true about **scope** and misleading about **cost**, because `unexpectedCloneEligible` is very conservative. A checkout only becomes deletable when every one of these holds: ``` fleet org, not live, not a -workdir git fetch --prune succeeds origin owner matches on `main` HEAD == origin/main exactly worktree clean, untracked included no in-progress git operation exactly one worktree no .gitmodules rev-list --all --reflog --not --remotes=origin is EMPTY ``` That last one is the load-bearing guard: **nothing local anywhere, including reflog**. So an eligible checkout is byte-for-byte reproducible from origin. The pre-fix behaviour would have cost a re-clone, not work. Measured on this host, across the twenty resident checkouts: ``` would-be-eligible 0 protected 20 ``` Zero. Every one is held back by something, most often local-only commits, which is unsurprising given the 247 branches with no remote counterpart I measured for #1084 earlier tonight. **That does not make the bug less worth fixing.** A fresh host, or one converged before its plan is written, is exactly the case with clean checkouts and no local work, which is where the gate stops protecting anything. The fix is still correct and still belongs on `main`. It is the severity claim I would soften: the failure mode is "re-clone nineteen repositories and lose an evening", not "lose work". Worth being precise because the pre-fix window is presumably still live on other hosts until this ships, and an operator reading "most destructive" may take an emergency action that is not warranted. ## Refs, and the follow-up filed The policy-source half genuinely is a separate feature, and you filed it rather than leaving it implied. That is the fourth deferral tonight and the first one filed at the same time as the deferral, which is the whole ask. `revision` and `policy.sha256` being parsed today and never read is a good way to state what remains. One thing carried over from my note on #903: when that half is built, key the regeneration trigger on the **policy digest** rather than the revision. Today's plan seals `d44f9b75` while `main` is many commits past it, and the `roles.kdl` digest is byte-identical at both. Triggering on revision would regenerate on every unrelated commit to a policy-source repo, which is most of what happened in this repo tonight.

coilyco-ops referenced this pull request

2026-08-23 01:20:23 +00:00

feat(aosguard): one generated skill per wrapped area, plus a concept skill (#1028) #1219

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

coilyco-flight-deck/agentic-os!1214

No description provided.

Rows
Columns