feat(models): add SSH/helper schema and HasFilesystemAccess helper - #1917
feat(models): add SSH/helper schema and HasFilesystemAccess helper#1917nitrobass24 wants to merge 12 commits into
Conversation
…solver Backend interface (17 methods) abstracts filesystem operations so services can work against local or remote backends. Local backend is a thin adapter over os.*/hardlinktree/reflinktree/hardlink/fsutil. Pool resolves instance ID to the appropriate backend based on the instance's filesystem access configuration.
NewPool now panics on nil instanceStore or local backend to fail fast at construction time. Package comment updated to reflect that only Local and Noop implementations exist (Remote is planned).
filepath.WalkDir passes d=nil when the root stat fails (documented in io/fs.WalkDirFunc). Our pre-walk os.Stat check has a TOCTOU window if the root is removed between the check and the walk's internal stat.
Migration 077 (sqlite) / 078 (postgres): 16 new columns on instances for SSH credentials (encrypted at rest) and helper metadata (version, capabilities, allowed roots, deploy timestamp). Instance model extended with SSH/helper fields. Get and List queries updated via shared scanInstance helper to read the new columns from instances_view. HasFilesystemAccess returns the filesystem access mode for an instance (local > helper > none). Test schemas consolidated into testInstanceSchema constant to avoid maintaining 7 inline copies.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds SSH/helper columns and view migration; extends Instance model and scan logic; introduces fsops types/errors and Backend interface; implements local/noop backends, Pool resolver, filesystem-access helper, and comprehensive unit tests. ChangesFilesystem Operations & SSH Remote Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pool
participant InstanceStore
participant LocalBackend as Local Backend
participant NoopBackend as Noop Backend
participant OS as OS/Filesystem
Client->>Pool: GetBackend(ctx, instanceID)
Pool->>InstanceStore: Get(ctx, instanceID)
InstanceStore-->>Pool: Instance
alt HasLocalFilesystemAccess
Pool->>Client: return Local Backend
Client->>LocalBackend: WalkDir(ctx, root, opts)
LocalBackend->>OS: filepath.WalkDir(root)
OS-->>LocalBackend: entries
LocalBackend-->>Client: emits WalkEntry channel
else Access Denied
Pool->>Client: return Noop Backend
Client->>NoopBackend: Stat(ctx, path)
NoopBackend-->>Client: ErrNoFilesystemAccess
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/fsops/local/local.go (1)
116-125: ⚡ Quick winHandle
DirEntry.Info()errors explicitly inReadDir.Line 116 discards
e.Info()errors, which can silently mask filesystem issues. Keep the fallback behavior if intended, but branch on the error explicitly.✅ Minimal fix
- info, _ := e.Info() + info, infoErr := e.Info() de := fsops.DirEntry{ Name: e.Name(), IsDir: e.IsDir(), IsSymlink: e.Type()&os.ModeSymlink != 0, Mode: e.Type().Perm(), } - if info != nil { + if infoErr == nil && info != nil { de.Mode = info.Mode().Perm() }As per coding guidelines: "
**/*.go: Prefer explicit error handling over silent failures in Go code."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/fsops/local/local.go` around lines 116 - 125, In ReadDir, don't ignore the error returned by e.Info(); capture it (e.g., info, err := e.Info()) and branch explicitly: if err != nil then keep the existing fallback (use e.Type().Perm() for de.Mode) but handle or annotate the error (return/wrap/log according to surrounding function behavior), otherwise set de.Mode = info.Mode().Perm(); update references around fsops.DirEntry, e.Info(), and de to reflect this explicit error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/fsops/backend.go`:
- Around line 21-95: The Backend interface is too large; split it into focused
sub-interfaces (each ≤5 methods) and make the original Backend a composition of
them. Create small domain interfaces such as ReadStat (Stat, StatBatch, Lstat,
LstatBatch, FileID), ReadDirWalk (ReadDir, WalkDir), FSState (Statfs,
SameFilesystem), Mutator (MkdirAll, Remove), TreeOps (HardlinkTree, ReflinkTree,
RemoveTree), Capabilities (SupportsReflink), and Diagnostic (Info, HealthCheck);
update implementations and mocks to implement those smaller interfaces and then
define type Backend interface { ReadStat; ReadDirWalk; FSState; Mutator;
TreeOps; Capabilities; Diagnostic } so callers can depend on narrower interfaces
where appropriate.
In `@internal/fsops/local/local_test.go`:
- Around line 439-440: The test calls os.Stat(src1) and ignores the returned
error before using srcFi in os.SameFile, which can mask failures; update the
test around the srcFi variable (where os.Stat(src1) is called) to capture the
error, assert/require that err == nil (e.g., require.NoError or an explicit
if/assert) before passing srcFi to os.SameFile(fi1), so the Stat failure is
surfaced and the SameFile check is only run on a valid FileInfo.
In `@internal/models/instance.go`:
- Around line 547-549: The three json.Unmarshal calls that decode
helperCapabilities/helperAllowedRoots/helperReflinkRoots into
instance.HelperCapabilities, instance.HelperAllowedRoots and
instance.HelperReflinkRoots currently ignore errors; change each call to check
the returned error, and on failure return (or propagate) a wrapped error with
context identifying which payload failed to decode (e.g., "unmarshal
helperCapabilities", "unmarshal helperAllowedRoots", "unmarshal
helperReflinkRoots") so the caller fails fast instead of returning a partial
instance state.
- Around line 50-65: The custom JSON codec in MarshalJSON and UnmarshalJSON is
omitting the newly added SSH and helper fields (e.g., SSHHost, SSHPort,
SSHUsername, SSHAuthType, SSHKeyEncrypted, SSHKeyPassphraseEncrypted,
SSHPasswordEncrypted, SSHHostKey, HelperPath, HelperVersion, HelperCapabilities,
HelperAllowedRoots, HelperReflinkRoots, HelperPlatform, HelperDeployedAt,
HelperLastActivityAt); update the anonymous/manual structs used inside
MarshalJSON and UnmarshalJSON to include those same fields with the correct json
tags and pointer/time types so they are preserved across API boundaries, and
ensure UnmarshalJSON assigns incoming values back to the instance struct fields
(and preserves absent/omitted fields consistently).
---
Nitpick comments:
In `@internal/fsops/local/local.go`:
- Around line 116-125: In ReadDir, don't ignore the error returned by e.Info();
capture it (e.g., info, err := e.Info()) and branch explicitly: if err != nil
then keep the existing fallback (use e.Type().Perm() for de.Mode) but handle or
annotate the error (return/wrap/log according to surrounding function behavior),
otherwise set de.Mode = info.Mode().Perm(); update references around
fsops.DirEntry, e.Info(), and de to reflect this explicit error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 886aadf4-452e-4d67-b1f4-0e3e7a954362
📒 Files selected for processing (16)
internal/database/migrations/077_add_remote_helper.sqlinternal/database/postgres_migrations/078_add_remote_helper.sqlinternal/fsops/backend.gointernal/fsops/errors.gointernal/fsops/local/local.gointernal/fsops/local/local_test.gointernal/fsops/local/statfs_unix.gointernal/fsops/local/statfs_windows.gointernal/fsops/noop.gointernal/fsops/pool.gointernal/fsops/pool_test.gointernal/fsops/types.gointernal/models/filesystem_access.gointernal/models/filesystem_access_test.gointernal/models/instance.gointernal/models/instance_test.go
| _ = json.Unmarshal([]byte(helperCapabilities), &instance.HelperCapabilities) | ||
| _ = json.Unmarshal([]byte(helperAllowedRoots), &instance.HelperAllowedRoots) | ||
| _ = json.Unmarshal([]byte(helperReflinkRoots), &instance.HelperReflinkRoots) |
There was a problem hiding this comment.
Don’t ignore helper JSON decode failures.
Line 547-Line 549 silently swallow malformed DB payloads and return partial instance state. Fail fast with context on decode errors.
🔧 Suggested fix
- _ = json.Unmarshal([]byte(helperCapabilities), &instance.HelperCapabilities)
- _ = json.Unmarshal([]byte(helperAllowedRoots), &instance.HelperAllowedRoots)
- _ = json.Unmarshal([]byte(helperReflinkRoots), &instance.HelperReflinkRoots)
+ if err := json.Unmarshal([]byte(helperCapabilities), &instance.HelperCapabilities); err != nil {
+ return nil, fmt.Errorf("decode helper_capabilities: %w", err)
+ }
+ if err := json.Unmarshal([]byte(helperAllowedRoots), &instance.HelperAllowedRoots); err != nil {
+ return nil, fmt.Errorf("decode helper_allowed_roots: %w", err)
+ }
+ if err := json.Unmarshal([]byte(helperReflinkRoots), &instance.HelperReflinkRoots); err != nil {
+ return nil, fmt.Errorf("decode helper_reflink_roots: %w", err)
+ }As per coding guidelines: "Prefer explicit error handling over silent failures in Go code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = json.Unmarshal([]byte(helperCapabilities), &instance.HelperCapabilities) | |
| _ = json.Unmarshal([]byte(helperAllowedRoots), &instance.HelperAllowedRoots) | |
| _ = json.Unmarshal([]byte(helperReflinkRoots), &instance.HelperReflinkRoots) | |
| if err := json.Unmarshal([]byte(helperCapabilities), &instance.HelperCapabilities); err != nil { | |
| return nil, fmt.Errorf("decode helper_capabilities: %w", err) | |
| } | |
| if err := json.Unmarshal([]byte(helperAllowedRoots), &instance.HelperAllowedRoots); err != nil { | |
| return nil, fmt.Errorf("decode helper_allowed_roots: %w", err) | |
| } | |
| if err := json.Unmarshal([]byte(helperReflinkRoots), &instance.HelperReflinkRoots); err != nil { | |
| return nil, fmt.Errorf("decode helper_reflink_roots: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/models/instance.go` around lines 547 - 549, The three json.Unmarshal
calls that decode helperCapabilities/helperAllowedRoots/helperReflinkRoots into
instance.HelperCapabilities, instance.HelperAllowedRoots and
instance.HelperReflinkRoots currently ignore errors; change each call to check
the returned error, and on failure return (or propagate) a wrapped error with
context identifying which payload failed to decode (e.g., "unmarshal
helperCapabilities", "unmarshal helperAllowedRoots", "unmarshal
helperReflinkRoots") so the caller fails fast instead of returning a partial
instance state.
CI fixes: - gofmt formatting in scanInstance - Add 16 SSH/helper columns to TestMigrationsApplyFullSchema expected schema Review fixes: - Check json.Unmarshal errors in scanInstance instead of silently ignoring them - Add SSH/helper fields to MarshalJSON (secrets redacted) and UnmarshalJSON so they round-trip through the API
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/models/instance.go (1)
166-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnmarshalJSON still drops several newly added SSH/helper fields.
Line 166-Line 174 and Line 225-Line 240 only decode/apply a subset (
helperAllowedRoots), while the marshal path emits more helper fields. This can silently discard client-provided values during JSON round-trips.🔧 Suggested patch
var temp struct { @@ SSHAuthType *string `json:"sshAuthType,omitempty"` + SSHHostKey *string `json:"sshHostKey,omitempty"` SSHPassword string `json:"sshPassword,omitempty"` SSHKey string `json:"sshKey,omitempty"` SSHKeyPassphrase string `json:"sshKeyPassphrase,omitempty"` + HelperPath *string `json:"helperPath,omitempty"` + HelperVersion *string `json:"helperVersion,omitempty"` + HelperCapabilities []string `json:"helperCapabilities,omitempty"` HelperAllowedRoots []string `json:"helperAllowedRoots,omitempty"` + HelperReflinkRoots []string `json:"helperReflinkRoots,omitempty"` + HelperPlatform *string `json:"helperPlatform,omitempty"` + HelperDeployedAt *time.Time `json:"helperDeployedAt,omitempty"` + HelperLastActivityAt *time.Time `json:"helperLastActivityAt,omitempty"` @@ if temp.SSHAuthType != nil { i.SSHAuthType = *temp.SSHAuthType } + if temp.SSHHostKey != nil { + i.SSHHostKey = *temp.SSHHostKey + } + if temp.HelperPath != nil { + i.HelperPath = *temp.HelperPath + } + if temp.HelperVersion != nil { + i.HelperVersion = *temp.HelperVersion + } + if temp.HelperCapabilities != nil { + i.HelperCapabilities = temp.HelperCapabilities + } if temp.HelperAllowedRoots != nil { i.HelperAllowedRoots = temp.HelperAllowedRoots } + if temp.HelperReflinkRoots != nil { + i.HelperReflinkRoots = temp.HelperReflinkRoots + } + if temp.HelperPlatform != nil { + i.HelperPlatform = *temp.HelperPlatform + } + if temp.HelperDeployedAt != nil { + i.HelperDeployedAt = temp.HelperDeployedAt + } + if temp.HelperLastActivityAt != nil { + i.HelperLastActivityAt = temp.HelperLastActivityAt + }Also applies to: 225-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/models/instance.go` around lines 166 - 174, The custom UnmarshalJSON implementation on the Instance model currently only reads a subset of the new SSH/helper fields (e.g., it only handles helperAllowedRoots) which causes fields like SSHHost, SSHPort, SSHUsername, SSHAuthType, SSHPassword, SSHKey, SSHKeyPassphrase, HelperAllowedRoots and LastConnectedAt to be dropped on decode; update the Instance.UnmarshalJSON method to parse and assign all these struct fields (matching the JSON tags shown in the struct: SSHHost, SSHPort, SSHUsername, SSHAuthType, SSHPassword, SSHKey, SSHKeyPassphrase, HelperAllowedRoots, LastConnectedAt) from the incoming JSON (or via an intermediate alias struct) so round-trips preserve client-provided values and keep behaviour consistent with MarshalJSON.
🧹 Nitpick comments (1)
internal/fsops/pool_test.go (1)
77-125: ⚡ Quick winAdd an explicit helper-configured pool test case.
Please add a case where
Instance{SSHHost: ..., HelperDeployedAt: ...}is passed toGetBackendso helper-mode routing behavior is pinned by tests (backend selection or explicit unsupported error, depending on intended contract).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/fsops/pool_test.go` around lines 77 - 125, Add a focused test named like TestPool_HelperMode that creates a fakeInstanceStore entry with Instance{ID: X, SSHHost: "host", HelperDeployedAt: time.Now()} and constructs a Pool via NewPool(store, helperFakeBackend) (reuse or add a fakeBackend that identifies itself as the helper backend), call pool.GetBackend(ctx, X) and then assert the behavior that your project intends for helper-mode: if helper routing is supported assert no error and backend.Info(ctx).Kind == "helper"; if helper routing should be rejected assert GetBackend returns the explicit helper-unsupported error your code exposes (replace with the actual error identifier). This pins the helper-mode routing behavior using the existing symbols GetBackend, NewPool, Instance.SSHHost, Instance.HelperDeployedAt, fakeInstanceStore and backend.Info.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/fsops/local/local.go`:
- Around line 253-260: The Remove method on Backend currently ignores
RemoveOptions.IgnorePaths and calls os.RemoveAll(path) unconditionally for
recursive deletes; change Remove (method Remove on type Backend) to check
opts.IgnorePaths and, as a safe short-term fix, refuse/return an error when
opts.Recursive is true and opts.IgnorePaths is non-empty (instead of calling
os.RemoveAll), so protected paths can't be wiped; later you can replace this
guard with a proper traversal that deletes children while skipping entries in
RemoveOptions.IgnorePaths.
In `@internal/fsops/pool.go`:
- Around line 55-61: GetBackend currently only checks
instance.HasLocalFilesystemAccess and falls back to noopBackend{}, which
silently downgrades helper/SSH-configured instances; update GetBackend to detect
the helper/SSH-enabled flag on the instance (e.g.,
instance.HasHelperFilesystemAccess or instance.SSHConfigured) before returning
noopBackend and return the proper remote/helper backend (e.g., p.helper or
p.remote backend) instead of noopBackend; if the backend field/property does not
yet exist on the pool (e.g., p.helper or p.remote), add and use it so
helper-configured instances are routed to the helper backend rather than
noopBackend while leaving the HasLocalFilesystemAccess branch returning p.local
unchanged.
---
Duplicate comments:
In `@internal/models/instance.go`:
- Around line 166-174: The custom UnmarshalJSON implementation on the Instance
model currently only reads a subset of the new SSH/helper fields (e.g., it only
handles helperAllowedRoots) which causes fields like SSHHost, SSHPort,
SSHUsername, SSHAuthType, SSHPassword, SSHKey, SSHKeyPassphrase,
HelperAllowedRoots and LastConnectedAt to be dropped on decode; update the
Instance.UnmarshalJSON method to parse and assign all these struct fields
(matching the JSON tags shown in the struct: SSHHost, SSHPort, SSHUsername,
SSHAuthType, SSHPassword, SSHKey, SSHKeyPassphrase, HelperAllowedRoots,
LastConnectedAt) from the incoming JSON (or via an intermediate alias struct) so
round-trips preserve client-provided values and keep behaviour consistent with
MarshalJSON.
---
Nitpick comments:
In `@internal/fsops/pool_test.go`:
- Around line 77-125: Add a focused test named like TestPool_HelperMode that
creates a fakeInstanceStore entry with Instance{ID: X, SSHHost: "host",
HelperDeployedAt: time.Now()} and constructs a Pool via NewPool(store,
helperFakeBackend) (reuse or add a fakeBackend that identifies itself as the
helper backend), call pool.GetBackend(ctx, X) and then assert the behavior that
your project intends for helper-mode: if helper routing is supported assert no
error and backend.Info(ctx).Kind == "helper"; if helper routing should be
rejected assert GetBackend returns the explicit helper-unsupported error your
code exposes (replace with the actual error identifier). This pins the
helper-mode routing behavior using the existing symbols GetBackend, NewPool,
Instance.SSHHost, Instance.HelperDeployedAt, fakeInstanceStore and backend.Info.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 42206c07-6fa2-4905-98e8-026cd0b1e458
📒 Files selected for processing (17)
internal/database/db_test.gointernal/database/migrations/077_add_remote_helper.sqlinternal/database/postgres_migrations/078_add_remote_helper.sqlinternal/fsops/backend.gointernal/fsops/errors.gointernal/fsops/local/local.gointernal/fsops/local/local_test.gointernal/fsops/local/statfs_unix.gointernal/fsops/local/statfs_windows.gointernal/fsops/noop.gointernal/fsops/pool.gointernal/fsops/pool_test.gointernal/fsops/types.gointernal/models/filesystem_access.gointernal/models/filesystem_access_test.gointernal/models/instance.gointernal/models/instance_test.go
The local backend's RemoveAll does not support skipping paths. Return an error instead of silently ignoring IgnorePaths, so callers get a clear failure rather than deleting protected paths.
|
Marking this as draft until pre-req PRs are Merged. |
filepath.WalkDir return value was discarded. If the walk fails (permission denied, I/O error), the channel now receives a final entry with the error so callers know the walk didn't complete. Context cancellation is not surfaced as an error.
Context cancellation now takes precedence over ErrNoFilesystemAccess so callers get the correct error instead of a misleading "no filesystem access" when the request was actually cancelled.
Replaces the deployable-agent direction from the remote-helper design (PR #1913): a single SSH connection per instance carrying the SFTP subsystem plus opt-in exec channels, with capabilities probed at connect. The key's own restrictions pick the tier — an internal-sftp key gets a degraded-but-working mode (no file identity, so hardlink features switch off visibly), an exec-capable key gets full functionality. Documents the op mapping, security model, slimmed schema scope for #1917, and the open questions (file identity wire form, SameFilesystem without fsids, BSD remotes).
The Schema section still said 'host-key fingerprint' — a leftover from the pre-security-review draft that contradicted the Security section's requirements (marshaled public key + algorithm under AEAD with host+port AAD; fingerprints are display-only). Since the Schema section scopes #1917, the stale wording would have steered the migration toward a column that cannot support the HostKeyAlgorithms constraint or tamper detection. Also names the motivating deployment for the AAD binding: Postgres with the database on a different host from sessionSecret, where a DB-write attacker without app-host access is realistic — and notes the mechanism stays the product's single AES-GCM/sessionSecret pattern with an AAD argument the existing stores simply haven't passed.
Summary
Adds the database schema and model fields needed for remote helper support on instances.
Schema (migration 077/078)
instances: SSH credentials (host, port, username, auth type, encrypted key/password/passphrase, host key) and helper metadata (path, version, capabilities, allowed roots, reflink roots, platform, deployed_at, last_activity_at)instances_viewto include new columnsModel changes
Instancestruct extended with SSH/helper fieldsscanInstancehelper extracts shared scan logic between Get and List (was duplicated across both)HasFilesystemAccess(inst)returns(FilesystemMode, bool)— local takes precedence over helperTest cleanup
testInstanceSchemaconstantTest plan
go build ./...— cleango test -race -count=1 ./internal/models/...— all passSummary by CodeRabbit
New Features
Tests