amadan

carlos / platform Public

hostfiles: the cap exemption covers only a skipped entry not also marked deleted, and superseded() skips a path past the cap (Astra round 4, two P2s)

1. An entry marked both Deleted and Skipped passed the exemption, but
   Restore treats it as a tombstone and walks it. Now only
   Skipped != "" && !Deleted is exempt. Only a crafted manifest holds
   such an entry.
2. superseded(), shared by Restore and Present, walked every ancestor of
   an exempt skipped path: quadratic in depth and not cancellable. It now
   skips a path past the cap. Only a skipped entry can be one, and any
   live entry beneath it is refused.

Tests:
- TestATombstoneMarkedSkippedPastTheCapIsRefused
- TestAVeryDeepSkippedEntryCostsRestoreNoAncestorWalk: a 200,000-byte
  path, where Restore took 15s without the skip.
Each fails under its mutation. Astra's round 4 review is committed
unedited.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude fd7136294ecfce2487dc44ee4040e0ca9c31e01e parent c6dc57d
4 files changed, +86 −4
  • docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP-4.md +35 −0
  • internal/hostfiles/fix5_linux_test.go +43 −0
  • internal/hostfiles/publisher.go +3 −2
  • internal/hostfiles/restore.go +5 −2
diff --git a/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP-4.md b/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP-4.md
new file mode 100644
index 000000000..1b9d1fece
--- /dev/null
+++ b/docs/superpowers/plans/CODEX-REVIEW-HOSTFILES-CAP-4.md
@@ -0,0 +1,35 @@
+**Request changes: two P2 findings. HEAD `c6dc57d0` is not ready to merge.**
+
+1. **P2 — Exempt skipped paths still cause unbounded ancestor processing.** The exemption at [publisher.go:74](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/publisher.go:74) admits arbitrarily deep skipped paths. Both Restore and Present call [superseded():226](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/restore.go:226), which walks every ancestor, including those of skipped entries. Each `path.Dir` cleans the remaining prefix, making this quadratic in depth, with no cancellation check. A valid skipped entry at `strings.Repeat("d/", 100000)+"f"` therefore causes substantial uncancellable work even on an empty destination. Bound or redesign overlap processing while preserving skipped entries’ sequence semantics.
+
+2. **P2 — A skipped flag bypasses the cap on a tombstone.** At [publisher.go:74](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/publisher.go:74), an over-cap entry containing both `Deleted: true` and `Skipped: "too-large"` passes validation. Neither decoding nor `checkEntry` rejects that combination. Restore gives `Deleted` precedence at [restore.go:109](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/hostfiles-cap-live-entries/internal/hostfiles/restore.go:109), then performs the ancestor marking and removal that the revised design intends to forbid. This requires a malformed or crafted manifest; normal writers do not produce it. Exempt only `!e.Deleted && e.Skipped != ""`, or reject contradictory states consistently.
+
+The remaining requested checks:
+
+- Round 3’s findings against `removePastCap` are moot: that implementation is gone.
+- For ordinary skipped entries, the StageDelete no-op is safe: it preserves an unpublished entry and cannot resurrect content. Restore → scan → commit succeeds when the deep file and over-cap directories are absent.
+- An existing deep file or directory still makes scanning—and therefore flushing—fail. That is the existing scan-cap limitation described in the brief as #279-class.
+- CopyManifest, NewFromCopy, and moves’ Establish consistently carry ordinary skipped entries through the shared validation.
+- The census overlay is envelope-only; its hash matches exactly. Plan 9 and Plan 14 legacy units and the legacy seal are unchanged.
+
+Static review only; no tests ran, including the full `internal/host` suite.
+
+---
+
+**FIRST PROMPT:**
+Review the round-4 brief at `c6dc57d0`, read-only; avoid the full internal/host suite.
+
+**WHAT:**
+Assess the revised cap, downstream recovery behavior, census, and merge readiness.
+
+**STATUS:**
+Complete: two P2 findings; request changes.
+
+**NEEDED:**
+Nothing from you for this review.
+
+**RECOMMENDED:**
+Address both findings and add focused regressions before merging.
+
+**SESSION:**
+Safe to close. HEAD and working tree are unchanged; no files or background processes were created.
\ No newline at end of file
diff --git a/internal/hostfiles/fix5_linux_test.go b/internal/hostfiles/fix5_linux_test.go
index 18e13546e..1c826162e 100644
--- a/internal/hostfiles/fix5_linux_test.go
+++ b/internal/hostfiles/fix5_linux_test.go
@@ -243,3 +243,46 @@ func TestAScanKeepsAnInheritedSkippedEntryPastTheCap(t *testing.T) {
t.Fatalf("healthy.txt after the scan: %+v", e)
}
}
+
+// Astra's round 4 (P2 2): an entry marked both deleted and skipped is a
+// tombstone to Restore, so the exemption does not cover it.
+func TestATombstoneMarkedSkippedPastTheCapIsRefused(t *testing.T) {
+ m := Manifest{V: ManifestVersion, Seq: 1, Entries: map[string]Entry{
+ pathOfBytes(MaxPathBytes + 1): {Seq: 1, Deleted: true, Skipped: "too-large"},
+ }, Platform: map[string]Entry{}}
+ if err := RestorableWithinCap(m); !errors.Is(err, ErrTooDeep) {
+ t.Fatalf("RestorableWithinCap: %v", err)
+ }
+ if err := ValidateManifest(m, []string{pfx}); !errors.Is(err, ErrTooDeep) {
+ t.Fatalf("ValidateManifest: %v", err)
+ }
+}
+
+// Astra's round 4 (P2 1): a skipped entry far past the cap costs Restore
+// and Present no walk of its ancestors (superseded skips it), so even a
+// path of 200,000 bytes restores at once.
+func TestAVeryDeepSkippedEntryCostsRestoreNoAncestorWalk(t *testing.T) {
+ ctx := context.Background()
+ s, dir := release.NewDirStore(t.TempDir()), t.TempDir()
+ deep := strings.Repeat("d/", 100000) + "f"
+ inheritPastTheCap(t, s, func(seq int64) map[string]Entry {
+ return map[string]Entry{deep: {Seq: seq, Skipped: "too-large"}}
+ })
+ start := time.Now()
+ m, st, err := Restore(ctx, s, pfx, dir, nil)
+ if err != nil {
+ t.Fatalf("Restore: %v", err)
+ }
+ if took := time.Since(start); took > 2*time.Second {
+ t.Fatalf("Restore took %v over a deep skipped entry", took)
+ }
+ if !slices.Equal(st.Skipped, []string{deep}) {
+ t.Fatalf("restore: %+v", st)
+ }
+ if ok, err := Present(dir, filepath.Join(t.TempDir(), "shop.example.env"), m, true); err != nil || !ok {
+ t.Fatalf("Present: %v %v", ok, err)
+ }
+ if took := time.Since(start); took > 4*time.Second {
+ t.Fatalf("Restore and Present took %v over a deep skipped entry", took)
+ }
+}
diff --git a/internal/hostfiles/publisher.go b/internal/hostfiles/publisher.go
index 517051dc8..366138929 100644
--- a/internal/hostfiles/publisher.go
+++ b/internal/hostfiles/publisher.go
@@ -61,7 +61,8 @@ func Open(ctx context.Context, s release.Store, prefix string) (*Publisher, erro
// the tree's length cap (withinTreeCap): the one rule the readers
// (checkManifest), Open, OpenAt and Commit apply, and the check a caller
// makes before it starts work that will need m restored (a move's cut). A
-// skipped entry is exempt: Restore never touches its path, so a manifest
+// skipped entry (one not also marked deleted, which Restore would treat as
+// a tombstone) is exempt: Restore never touches its path, so a manifest
// written before the cap that holds one past it stays readable and
// continuable. A live entry or tombstone past the cap is ErrTooDeep: no
// restore could recreate or remove it in bounded work.
@@ -71,7 +72,7 @@ func RestorableWithinCap(m Manifest) error { return manifestWithinCap(m) }
// it by this name, which the root-read and record-write censuses pin.
func manifestWithinCap(m Manifest) error {
for path, e := range m.Entries {
- if e.Skipped != "" {
+ if e.Skipped != "" && !e.Deleted {
continue
}
if err := withinTreeCap(path); err != nil {
diff --git a/internal/hostfiles/restore.go b/internal/hostfiles/restore.go
index 7dd4af4b5..8a42b12b0 100644
--- a/internal/hostfiles/restore.go
+++ b/internal/hostfiles/restore.go
@@ -216,11 +216,14 @@ func restore(ctx context.Context, s release.Store, prefix string, origins []stri
// wins and the other is superseded; equal sequences keep the deeper one.
// Restore leaves a superseded entry unwritten and Present does not look
// for it, so what Restore produces is exactly what Present accepts
-// (stage B final review A1). Tombstones take no part.
+// (stage B final review A1). Tombstones take no part, and nor does a path
+// past the cap: only a skipped entry can be one (RestorableWithinCap), and
+// walking its ancestors would cost work without bound. Any live entry
+// beneath such a path is past the cap too, and refused.
func superseded(m Manifest) map[string]bool {
loser := map[string]bool{}
for p, e := range m.Entries {
- if e.Deleted {
+ if e.Deleted || withinTreeCap(p) != nil {
continue
}
for a := path.Dir(p); a != "."; a = path.Dir(a) {