apply dropped the visibility column, and add did too
Reported from use in another repository, reproduced before diagnosing, and the report was right and understated: two staging sites built a lock record without visibility, not one, and one of them was in add where the value was already computed three lines above. Fixed twice over, because the two fixes 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, and inheriting the old value would record a fact about bytes nobody holds any more. And apply now never lets an absent value overwrite a known one, which covers polads staged by an older build, the state the reporter is in. Pinned by a regression test. No test we had would have caught it: both sites round-trip correctly in isolation, and the field only vanishes across the handoff between them. 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, reads as verified and is blank. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018UTxuSizozEA8yDitPuris
This commit is contained in:
@@ -1141,3 +1141,47 @@ adoption, which is ours.**
|
|||||||
**Belief that could be shown wrong:** *that an extra round trip on a failure is
|
**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
|
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.*
|
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.*
|
||||||
|
|||||||
Vendored
+6
-2
@@ -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 — "+
|
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)
|
"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
|
return err
|
||||||
}
|
}
|
||||||
fmt.Fprintf(out, "staged %s\n", rel)
|
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 {
|
if !open {
|
||||||
return Status{rec.Path, "CHANGED", "upstream moved — no cart open, so nothing was staged"}, 0
|
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, "error", err.Error()}, 0
|
||||||
}
|
}
|
||||||
return Status{rec.Path, "CHANGED", "staged as a polad" + usagesNote(root, rec.Path)}, 1
|
return Status{rec.Path, "CHANGED", "staged as a polad" + usagesNote(root, rec.Path)}, 1
|
||||||
|
|||||||
Vendored
+9
@@ -104,6 +104,15 @@ func Apply(root string, rels []string, out io.Writer) error {
|
|||||||
if err := os.WriteFile(dest, body, 0o644); err != nil {
|
if err := os.WriteFile(dest, body, 0o644); err != nil {
|
||||||
return err
|
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)
|
locks.Put(rec)
|
||||||
staged.Remove(rel)
|
staged.Remove(rel)
|
||||||
if err := os.Remove(src); err != nil {
|
if err := os.Remove(src); err != nil {
|
||||||
|
|||||||
Vendored
+48
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user