Address review: test-only registration, fwd-compat error, confined reads

9e4597e→main· Soph·3w ago·6 files·+114 added/-58 removed

Three reviewer points on the Phase 2 stack:

• fsstore registration is no longer a public symbol on the production package. The factory, its config, the backend-type name, and registerForTesting moved into register_test.go, so the production fsstore package exposes only the Store — a production binary has no way to register the "fs" backend.

• The checkpoints loader keeps DisallowUnknownFields (so typos surface instead of being silently dropped), but it is not forward-compatible: a field added by a newer CLI errors in an older one. Documented that adding a field is a coordinated rollout (ship the reader first), and the decode error now tells the user an unrecognized field may mean the file was written by a newer CLI and to confirm they are on the latest version.

• Settings reads go through an os.Root anchored at the file's parent dir (readConfined) instead of a bare os.ReadFile of an absolute path, so the open cannot be redirected outside that directory by a swapped/symlinked path (TOCTOU). An escaping symlink surfaces as a non-ENOENT error and is handled fail-soft; added a regression test. Note: the rest of the settings package still uses os.ReadFile, so making it consistent is a worthwhile follow-up.

Sessions

e9de15786526View transcript

Changes

6

• cmd/entire/cli

• checkpoint/fsstore

• Mfsstore.go+4/-6

• Dregister.go-44

• Aregister_test.go+47

• Mseam_test.go+1/-1

• settings

• Mcheckpoints.go+42/-7

• Mcheckpoints_test.go+20

3 unmodified lines
4
5
6
7
8
9
7
8
9
10
11
12
13
23 unmodified lines

37
38
39
39
40
41
40
41
42

3 unmodified lines

// of the api/checkpoint contract, and to serve as a worked example for new
// backends.

// It is deliberately NOT registered by production code: only RegisterForTesting
// wires it into the checkpoint registry, so a production binary can never select
// it. As a mirror it receives best-effort write fan-out; it intentionally ignores
// the git-specific blob-hash fields of the contract and stores transcript bytes
// directly, which keeps the example small and makes the contract's remaining
// git leakage concrete.
23 unmodified lines

"github.com/entireio/cli/cmd/entire/cli/jsonutil"
}

// BackendType is the registry type name for the filesystem reference backend.
const BackendType = "fs"

// Store is a JSON-file-backed persistent checkpoint store. One file per
// checkpoint (<root>/<checkpoint-id>.json) holds the root summary plus all
// session content.

Mcmd/entire/cli/checkpoint/fsstore/fsstore.go+4/-6

package fsstore

import (
    "context"
    "encoding/json"
    "errors"
    "fmt"
    "sync"

cp "github.com/entireio/cli/api/checkpoint"
    "github.com/entireio/cli/cmd/entire/cli/checkpoint"
)

// Config is the fsstore backend's settings "config" block.
type Config struct {
    Path string `json:"path"`
}

var registerOnce sync.Once

// RegisterForTesting registers the fsstore backend under its type so tests can
// select it as a checkpoint mirror (or primary in fsstore's own tests). It is
// the only path that adds fsstore to the registry: production code never calls
// it, so a production binary cannot resolve the "fs" backend. Registration is
// process-wide and idempotent (checkpoint.Register panics on duplicates).
func RegisterForTesting() {
    registerOnce.Do(func() {
        checkpoint.Register(BackendType, factory)
    })
}

//nolint:ireturn // must return the contract interface to satisfy checkpoint.Factory
func factory(_ context.Context, _ checkpoint.OpenEnv, cfg json.RawMessage) (cp.PersistentStore, error) {
    var c Config
    if len(cfg) > 0 {
        if err := json.Unmarshal(cfg, &c); err != nil {
            return nil, fmt.Errorf("fsstore: invalid config: %w", err)
        }
    }
    if c.Path == "" {
        return nil, errors.New("fsstore: config.path is required")
    }
    return New(c.Path), nil
}

Dcmd/entire/cli/checkpoint/fsstore/register.go-44

package fsstore

import (
    "context"
    "encoding/json"
    "errors"
    "fmt"
    "sync"

cp "github.com/entireio/cli/api/checkpoint"
    "github.com/entireio/cli/cmd/entire/cli/checkpoint"
)

// backendType is the registry type name for the filesystem reference backend.
const backendType = "fs"

// config is the fsstore backend's settings "config" block.
type config struct {
    Path string `json:"path"`
}

var registerOnce sync.Once

// registerForTesting registers the fsstore backend so tests can select it as a
// checkpoint mirror. It lives in a _test.go file on purpose: the production
// fsstore package exposes no way to add itself to the registry, so a production
// binary can never resolve the "fs" backend. Registration is process-wide and
// idempotent (checkpoint.Register panics on duplicates).
func registerForTesting() {
    registerOnce.Do(func() {
        checkpoint.Register(backendType, factory)
    })
}

Acmd/entire/cli/checkpoint/fsstore/register_test.go+47

```go
// Not parallel: uses t.Chdir so settings + ref resolution target the test repo.
func TestSeam_GitPrimaryWithFsMirror(t *testing.T) {
    RegisterForTesting()
    registerForTesting()

dir := t.TempDir()
    testutil.InitRepo(t, dir)
}

Mcmd/entire/cli/checkpoint/fsstore/seam_test.go+1/-1

5 unmodified lines
6
7
8
9
10
11
12
13
14
15
16
5 unmodified lines

22
23
24
22
25
26
27
28
42 unmodified lines

71
72
73
74
75
76
77
78
79
80
73
81
82
83
84
7 unmodified lines

92
93
94
87
95
96
89
90
91
92
97
98
99
100
101
102
103
104
11 unmodified lines

116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147

5 unmodified lines

"encoding/json"
    "errors"
    "fmt"
    "io"
    "io/fs"
    "log/slog"
    "os"
    "path/filepath"

"github.com/entireio/cli/cmd/entire/cli/logging"
}
5 unmodified lines

// CheckpointsConfig selects checkpoint storage backends: one primary (source of
// truth, serves all reads and writes) and zero or more mirrors (independent
// backends that receive best-effort write fan-out). When absent, the checkpoint
// layer defaults to the built-in git backend with no mirrors.
// layer defaults to the built-in git-branch backend with no mirrors.
type CheckpointsConfig struct {
    Primary BackendConfig   `json:"primary"`
    Mirrors []BackendConfig `json:"mirrors,omitempty"`
} 
42 unmodified lines

var cfg CheckpointsConfig
    dec := json.NewDecoder(bytes.NewReader(raw))
    // DisallowUnknownFields surfaces typos (e.g. "primry") instead of silently
    // ignoring them. The trade-off is that this CLI is not forward-compatible
    // with checkpoints fields added by a newer CLI: an unknown field errors here.
    // Adding a field is therefore a coordinated rollout — ship the reader before
    // any writer emits the field. The error below points users at that cause.
    dec.DisallowUnknownFields()
    if err := dec.Decode(&cfg); err != nil {
        return nil, fmt.Errorf("%w in %s: %w", ErrInvalidCheckpointsConfig, src, err)
        return nil, fmt.Errorf("%w in %s: %w; an unrecognized field can also mean this file was written by a newer CLI — confirm you are on the latest version", ErrInvalidCheckpointsConfig, src, err)
    }
    if err := cfg.validate(); err != nil {
        return nil, err
    }
}

Mcmd/entire/cli/settings/checkpoints.go+42/-7

3 unmodified lines

4
5
6
7
8
9
10
94 unmodified lines

105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129

3 unmodified lines

"context"
    "os"
    "path/filepath"
    "runtime"
    "testing"

"github.com/stretchr/testify/assert"
94 unmodified lines

assert.Equal(t, "git", cfg.Primary.Type)
}

func TestLoadCheckpointsConfig_RejectsEscapingSymlink(t *testing.T) {
    if runtime.GOOS == "windows" {
        t.Skip("symlink creation needs privileges on Windows")
    }
    dir := newCheckpointsSettingsRepo(t)

// A valid checkpoints config that lives OUTSIDE the .entire directory.
    outside := filepath.Join(t.TempDir(), "evil.json")
    require.NoError(t, os.WriteFile(outside, []byte(`{"checkpoints": {"primary": {"type": "git-branch"}}}`), 0o644))
    // Point .entire/settings.json at it via an (absolute) symlink that escapes
    // the directory. The confined read must refuse to follow it, so the config
    // is not picked up and we fail soft to the default.
    require.NoError(t, os.Symlink(outside, filepath.Join(dir, ".entire", "settings.json")))

cfg, err := LoadCheckpointsConfig(context.Background())
    require.NoError(t, err)
    assert.Nil(t, cfg, "an escaping symlink must not be followed; config should fail soft to nil")
}

func TestLoadCheckpointsConfig_ToleratesUnreadableFile(t *testing.T) {
    dir := newCheckpointsSettingsRepo(t)
    // settings.json as a directory makes os.ReadFile fail with a non-ENOENT