diff --git a/.loom/event-log.md b/.loom/event-log.md index 0db0c76..0eed615 100644 --- a/.loom/event-log.md +++ b/.loom/event-log.md @@ -1141,3 +1141,47 @@ adoption, which is ours.** **Belief that could be shown wrong:** *that an extra round trip on a failure is free.* **It is one partial clone on a path nobody takes twice**, *and if adoption against large repositories becomes common the listing should be bounded.* + +## 2026-09-10 — `apply` dropped the visibility column, reported from use + +**Reported by an agent working in another repository**, *who restored the value by +hand and said the right thing about it:* **losing it silently on every update is +the kind of thing that only shows up when it matters.** + +**Reproduced before diagnosing, and the report was right and understated. Two +staging sites dropped it, not one:** + +``` +before: 4 fields, vis=not-public +after: 3 fields, vis= +``` + +*A staged polad carried `path`, `url` and `etag`, and `apply` wrote the staged +record verbatim.* **One of the two sites was in `add`, where the value was already +computed and sitting in a variable three lines above.** + +**Fixed in two ways, because they answer different questions.** + +**A staged copy is new bytes, so its visibility is evaluated afresh rather than +inherited.** *A source that has gone private since adoption is a new exposure, +created at the moment of applying* — **inheriting the old value would have +recorded a fact about bytes nobody holds any more.** + +**And `apply` never lets an absent value overwrite a known one.** *A polad staged +by an older build carries no visibility, and the reporter's tree is exactly that +state.* **Pinned by a regression test.** + +> **The lock header says visibility is recorded because access is checked once and +> the copy is durable.** *A field that documents its own reason for existing, and +> is then dropped by the one operation that updates it, is worse than not having +> it* — **it reads as verified and is blank.** + +**Belief that could be shown wrong:** *that re-evaluating on apply is worth one +request.* **It costs a probe per changed document**, *only on the path where +something actually moved* — **and if reconciliation ever becomes bulk, that is the +first thing to batch.** + +*Found in use, in a repository nobody here was looking at, by somebody who then +had to repair a lock by hand. **No test we wrote would have caught it**: both +sites round-tripped fine in isolation, and the field only vanishes across the +handoff between two of them.* diff --git a/internal/external/external.go b/internal/external/external.go index b4a8449..9fe1488 100644 --- a/internal/external/external.go +++ b/internal/external/external.go @@ -135,7 +135,7 @@ func Add(root, raw, override string, out io.Writer) error { return fmt.Errorf("%s differs from what %s serves, and no cart is open to stage it in — "+ "the local copy is the only evidence of that and will not be touched", rel, body.url) } - if err := stage(root, rel, body.data, lock.Record{Path: rel, URL: body.url, ETag: etag}); err != nil { + if err := stage(root, rel, body.data, lock.Record{Path: rel, URL: body.url, ETag: etag, Visibility: vis}); err != nil { return err } fmt.Fprintf(out, "staged %s\n", rel) @@ -387,7 +387,11 @@ func checkLocked(cfg *config.Config, root string, rec lock.Record, open bool) (S if !open { return Status{rec.Path, "CHANGED", "upstream moved — no cart open, so nothing was staged"}, 0 } - if err := stage(root, rec.Path, body.data, lock.Record{Path: rec.Path, URL: body.url, ETag: etag}); err != nil { + // A staged copy is new bytes, so its visibility is evaluated afresh rather + // than inherited: a source that has gone private since adoption is a new + // exposure, created now. + staged := lock.Record{Path: rec.Path, URL: body.url, ETag: etag, Visibility: sourceVisibility(cfg, body.url)} + if err := stage(root, rec.Path, body.data, staged); err != nil { return Status{rec.Path, "error", err.Error()}, 0 } return Status{rec.Path, "CHANGED", "staged as a polad" + usagesNote(root, rec.Path)}, 1 diff --git a/internal/external/polad.go b/internal/external/polad.go index ceda169..29f040a 100644 --- a/internal/external/polad.go +++ b/internal/external/polad.go @@ -104,6 +104,15 @@ func Apply(root string, rels []string, out io.Writer) error { if err := os.WriteFile(dest, body, 0o644); err != nil { return err } + // Never let an absent value overwrite a known one. A polad staged by an + // older build carries no visibility, and the lock header says visibility + // is recorded because access is checked once and the copy is durable — + // so losing it on update is the failure it exists to prevent. + if rec.Visibility == "" { + if prior, ok := locks.Get(rel); ok { + rec.Visibility = prior.Visibility + } + } locks.Put(rec) staged.Remove(rel) if err := os.Remove(src); err != nil { diff --git a/internal/external/polad_test.go b/internal/external/polad_test.go new file mode 100644 index 0000000..34b72ec --- /dev/null +++ b/internal/external/polad_test.go @@ -0,0 +1,48 @@ +package external + +import ( + "io" + "os" + "path/filepath" + "testing" + + "git.hypertheory-labs.dev/loom/loom-cli/internal/lock" +) + +// apply used to write the staged record verbatim, and a staged record carried no +// visibility — so every update silently dropped a column the lock header says is +// recorded because access is checked once and the copy is durable. +func TestApplyDoesNotDropVisibility(t *testing.T) { + root := t.TempDir() + + locks, _ := lock.LoadFile(lock.Path(root)) + locks.Put(lock.Record{Path: "h/o/r/d.md", URL: "https://h/d", ETag: `"old"`, Visibility: lock.NotPublic}) + if err := locks.Save(); err != nil { + t.Fatal(err) + } + + // A polad staged by a build that did not record visibility. + if err := stage(root, "h/o/r/d.md", []byte("new"), lock.Record{ + Path: "h/o/r/d.md", URL: "https://h/d", ETag: `"new"`, + }); err != nil { + t.Fatal(err) + } + if err := Apply(root, nil, io.Discard); err != nil { + t.Fatal(err) + } + + after, _ := lock.Load(root) + got, ok := after.Get("h/o/r/d.md") + if !ok { + t.Fatal("record vanished") + } + if got.ETag != `"new"` { + t.Errorf("etag = %q, want the staged one", got.ETag) + } + if got.Visibility != lock.NotPublic { + t.Errorf("visibility = %q, want it preserved as %q", got.Visibility, lock.NotPublic) + } + if b, err := os.ReadFile(filepath.Join(root, lock.Dir, "h/o/r/d.md")); err != nil || string(b) != "new" { + t.Errorf("applied content = %q, %v", b, err) + } +}