Add project-scoped feature state stores - #37
Conversation
5c59385 to
1d2f384
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Adds an opt-in project-scoped state store for feature mixins (state: true), mounted at ~/.enclave-feature-state/<feature>/ and keyed by feature + project hash but not by tool, so tools in the same project share it.
The store integration is clean: a new StoreKind, one hoststore case, one path helper, and both backends pick it up through the generalized StorePrep.FeatureStores. Nice side effects are that hoststore.DirFor no longer silently falls through to the config layout for unknown kinds, and feature auth and feature state are no longer mutually exclusive. Owner and project hash both go through validateStoreSegment, and the docs (docs/runtime/stores.md) are accurate about the cross-tool trust channel and the --ephemeral / --auth-scope interactions.
Worth a human look at three spots: the kit-init.sh permission fix in the Dockerfile has no counterpart in runtime-assets/microvm/alpine/build-bundle.sh, where the same umask-dependent copy defeats the empty-manifest feature gate; entrypoint.sh unsets the state root before the initFiles/startup loops, so a stateful service mixin can't locate its store from a startup command; and the restructured --all path in internal/app/cleanup.go introduces a "project" cleanup kind that no --keep filter matches.
make build, make test, and make lint all pass.
| /usr/local/share/enclave/net.sh && \ | ||
| chmod a+r /usr/local/share/enclave/tmux-session.conf | ||
| chmod a+r /usr/local/share/enclave/tmux-session.conf \ | ||
| /usr/local/share/enclave/kit-init.sh |
There was a problem hiding this comment.
The QEMU bundle needs the same fix. build-bundle.sh copies kit-init.sh with plain cp and its chmod line only covers entrypoint/auth-reconcile/net.sh (link), and the later chown -R agent doesn't touch /usr/local/share/enclave. Since the bundle bakes an empty .installed-features manifest specifically to gate mixins off and that gate lives in kit-init.sh, an unreadable copy fails open and runs every baked feature's hooks.
| fi | ||
| unset ENCLAVE_FEATURE_STATE_DIR | ||
| done | ||
| unset ENCLAVE_FEATURE_STATE_ROOT enclave_feature_state_root |
There was a problem hiding this comment.
Unsetting the root here means neither ENCLAVE_FEATURE_STATE_ROOT nor ENCLAVE_FEATURE_STATE_DIR is visible to the initFiles / workspace / startup loops that follow (link). A stateful service mixin declaring commands.startup can then only find its store if a feature-entrypoint.d script planted a symlink first, and initFiles can't reference it at all (envsubst whitelist is WORKDIR/HOME/USER). Either keep the root exported through those loops or say so in the extension docs.
| logx.Infof("%s feature state mounted", util.TitleCase(feat.Name)) | ||
| } | ||
| if mounted { | ||
| mounts.AddEnv(model.EnvFeatureStateRoot, mountRoot) |
There was a problem hiding this comment.
Exporting only the root and letting the entrypoint infer per-feature opt-in from [ -d "$root/$feature_name" ] diverges from how feature auth does it: that one ships a declared map, ENCLAVE_FEATURE_AUTH_MAP (link, consumed at entrypoint.sh#L170). With the directory probe a feature that declared state: true but whose mount got skipped looks identical to one that never opted in.
| } | ||
| } | ||
| if !keepsChild { | ||
| dirs = append(dirs, cleanupDir{Kind: "project", Path: projectDir}) |
There was a problem hiding this comment.
Kind: "project" matches none of the filters in cleanupDirsForRemoval, so this whole-project shortcut is unfilterable by any --keep. It happens to be safe because only history and feature-state gate the projects tree today, but it will silently ignore any future project-scoped keep kind. The keepsChild loop above also reduces to two checks: does the project have a features child, does it have any other child.
| ) | ||
|
|
||
| // FeatureStoreLabel returns the user-facing label for a feature-owned store. | ||
| func (kind StoreKind) FeatureStoreLabel() string { |
There was a problem hiding this comment.
This is log-message formatting hanging off a shared domain type, and it returns "feature store" for config/auth/env, which aren't feature stores. It's only used by the two logx lines in the Docker and QEMU prepare paths; a local helper or a small map in each backend keeps the presentation string out of backend.
| } | ||
| } | ||
|
|
||
| func TestResolveEphemeralStoreDirsSkipsFeatureStateNamespace(t *testing.T) { |
There was a problem hiding this comment.
This sets run.Tool = "features", which is the non---all branch, and a sandbox named features is now rejected at spec load so that can't happen in practice. The branch the guard actually protects is --all, where tools comes from listSubdirs(HostProjectDir(...)). Set CleanupAll: true and add a real tool dir alongside the feature-state tree.
| features: []model.Extension{ | ||
| {Name: "diffity", Ports: []model.PortConfig{ | ||
| {Container: 5391, HostAllocation: model.HostAllocationAuto, Publish: true, Label: "Diffity"}, | ||
| {Name: "preview", Ports: []model.PortConfig{ |
There was a problem hiding this comment.
Renaming the diffity fixture to preview is unrelated to feature state. Separate commit.
There was a problem hiding this comment.
i don't care, this will be done now :)
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
1d2f384 to
a22bc64
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up review of the project-scoped feature state store.
Two commits landed since the last pass: the QEMU bundle now chmods kit-init.sh readable, and the ephemeral-cleanup guard test was rewritten to actually hit the --all branch (it now trips on a feature named config-store, which is exactly the collision the guard exists for). Both look right.
The store integration itself stays small and well placed: one StoreKind, one hoststore case, two path helpers, and both backends pick it up via the generalized StorePrep.FeatureStores. Docs across stores.md, persistence.md, ARCHITECTURE.md, cli-reference.md and extensions/README.md match the code, including the concurrency and cross-tool caveats. make build, make test, make lint pass.
Where a maintainer should focus: the restructured --all path in internal/app/cleanup.go (the "project" kind is unfilterable by any --keep), the entrypoint env lifecycle around the initFiles/startup loops, and the blast radius of enclave cleanup --tool <x>, which now wipes the whole project's feature state including data other tools use. Also worth deciding whether the new cross-tool write channel deserves a mention in docs/security/ rather than only docs/runtime/stores.md. No shipped extension sets state: true yet, so the path is exercised only by tests.
Two new low-severity comments inline; the four earlier open threads still apply.
These previous comments can be resolved as they are now handled:
- kit-init.sh needs the same world-readable fix in the QEMU bundle
- feature-state namespace guard test should cover the --all branch
I can't resolve them myself as I would need write permission on this repository.
| } | ||
| if prep.Auth != nil { | ||
| b.ensureStoreDir(backend.StoreKindAuth, prep.Auth.Key, util.TitleCase(prep.Auth.Key.Owner)+" shared auth store") | ||
| b.ensureStoreDir(prep.Auth.Kind, prep.Auth.Key, util.TitleCase(prep.Auth.Key.Owner)+" shared auth store") |
There was a problem hiding this comment.
Taking the kind from prep.Auth.Kind buys nothing here (prep.Auth is by definition the shared tool auth store) and adds a failure mode: with the new erroring default case in DirFor, a caller that forgets Kind now silently loses the shared auth store, since ensureStoreDir only logs the resolve failure at debug level (link). Same in qemu/prepare.go. Keeping backend.StoreKindAuth hardcoded for this one field would be safer.
| if doc.Name != name { | ||
| return specDocument{}, fmt.Errorf("%s name must be %q", specPath, name) | ||
| } | ||
| if kind == KindSandbox && name == projectFeatureStateDirName { |
There was a problem hiding this comment.
The reservation only fires once the spec is actually loaded. ListTools just checks for a spec file (link), so a user tool dir named features still shows up in --help and completion and only errors when someone selects it. Filtering it there too would make the docs claim ("the sandbox name features is reserved") hold end to end.
What it does
Adds an opt-in state store for features that need to keep project data across sessions and tools. A mixin can declare
state: true; in persistent sessions Enclave creates~/.local/state/enclave/projects/<hash>/features/<feature>/state/and mounts it at~/.enclave-feature-state/<feature>/.While sourcing that feature's setup scripts, the entrypoint exposes the mount as
ENCLAVE_FEATURE_STATE_DIR. It unsets the variable before sourcing the next feature and before starting the agent. Ephemeral sessions do not create or mount feature state.Feature state is represented in both backend store layers, although current QEMU sessions disable mixins and do not mount it. It is independent of auth scope and remains separate from feature auth.
enclave cleanupremoves it with the rest of the project data unless--keep feature-stateis set, including duringcleanup --all.Mixin validation rejects
stateon sandbox specs. The sandbox extension namefeaturesis now reserved because it would collide with the project feature-state namespace. The image build also makeskit-init.shworld-readable, so startup does not depend on the checkout's umask.How to test
Current QEMU sessions disable mixins, so this manual test uses Docker. It creates a temporary
state-probeextension that records which tools start with its state store mounted.Run the setup commands from the repository root.
enclave shellis interactive; run the checks below at the container prompt.Inside the Claude shell:
Start the same extension with Codex:
"$enclave_bin" shell --tool codex --features state-probe --rebuildInside the Codex shell, the shared file should contain both tool names:
Back on the host, clean up and run the automated checks:
The unit tests cover stateless and ephemeral sessions, cleanup retention, and both backend store layers.
Follow-ups
None.
Breaking changes
The sandbox extension name
featuresis now reserved.Review checklist