Skip to content

FileTransport does not validate syncDir before reading or writingΒ #725

Description

@Rene-Kuhm

πŸ“‹ Pre-flight Checks

  • I have searched existing issues and this is not a duplicate
  • I understand this issue needs status:approved before a PR can be opened

πŸ› Bug Description

First of all, thanks for engram β€” I have been using it daily as my memory
layer across a few projects, and it has become part of how I work.

While running the suite on Windows I ran into something in internal/sync
that I think is worth reporting.

FileTransport assumes syncDir resolves to a directory, so when it does not,
the failure surfaces later and in a shape callers cannot easily act on:

  • If syncDir does not exist, ReadManifest returns a file-read error
    rather than an empty manifest, so a first-run client cannot tell "nothing has
    been synced yet" from a real transport failure.
  • If syncDir exists but is a file rather than a directory, it is accepted,
    and WriteManifest, WriteChunk and ReadChunk then fail with whatever the
    OS returns for that path, which varies across platforms.

TestFileTransportReadManifestNotDir already covers the second case, and it is
currently red on my machine β€” I may well be missing something about how it is
meant to run, so please tell me if that is the case.

πŸ” Steps to Reproduce

go test ./internal/sync/... -run TestFileTransportReadManifestNotDir -v

βœ… Expected Behaviour

  • A missing syncDir yields an empty manifest (Version: 1), matching the
    documented contract of ReadManifest.
  • A syncDir that is not a directory is rejected with a clear error.

❌ Actual Behaviour

--- FAIL: TestFileTransportReadManifestNotDir (0.00s)
    transport_test.go:74: expected error when syncDir is a file
FAIL    github.com/Gentleman-Programming/engram/internal/sync

πŸ“¦ Affected Area

internal/sync β€” FileTransport.

πŸ“Ž Additional Context

I already have a candidate fix if it would help: a small validateSyncDir
helper called from ReadManifest, WriteManifest, WriteChunk and
ReadChunk. It propagates the os.Stat error unchanged so callers can use
os.IsNotExist to tell a missing path from one that is not a directory, and
ReadManifest maps the missing case to an empty manifest.

With it the existing test passes, and I add
TestFileTransportReadManifestMissingDir for the missing-directory path.

That said, you know the codebase far better than I do β€” happy to open a PR if
this is the direction you want, or to drop it entirely if there is a reason it
behaves this way. Just let me know either way, and thanks again for the project.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions