IfNotExists / LatestRevisionID are silently ignored when CreateRevision=false #31

New issue

Open

opened 2026-10-03 05:04:30 +00:00 by thklein · 1 comment

thklein commented

2026-10-03 05:04:30 +00:00

Owner

Problem

WriteOptions documents that a conflict error is returned when LatestRevisionID is not the latest, and that IfNotExists writes only when the file does not exist (pkg/client/client.go:210-214). IfNotExists is implemented as LatestRevisionID = "-1" (client.go:230).

Both checks live in beginWrite (pkg/client/revisionwrite.go ~line 125), which returns immediately when CreateRevision=false:

if opt.CreateRevision != nil && !*opt.CreateRevision {
    return nil, nil
}

So WriteFile(…, WriteOptions{CreateRevision: &false, IfNotExists: true}) overwrites an existing file with no error, and a stale LatestRevisionID is silently accepted. The comment above beginWrite gives the reason for skipping the revision store (connectors must not break on a corrupt index), but that reason does not cover dropping the caller's precondition.

Concrete failure scenario

A caller that uses IfNotExists as a create-once guard (e.g. "write the default config only if none exists") and also passes CreateRevision=false to avoid history clobbers the user's file on every run.

Fix

Pick one, and document it:

Enforce it: when IfNotExists is set and CreateRevision=false, check existence with contentExists / StatFile and return the ConflictError. When LatestRevisionID is set without revisions, return an error, since there is no revision to compare against.
Reject it: return an error for the combination, so the caller learns it is meaningless.

Silently ignoring it is the one wrong answer. Add tests for both options on each provider.

Where it was found

Review of #27 / #28. Related to basepeak-ai/bpai#1220.

## Problem `WriteOptions` documents that a conflict error is returned when `LatestRevisionID` is not the latest, and that `IfNotExists` writes only when the file does not exist (`pkg/client/client.go:210-214`). `IfNotExists` is implemented as `LatestRevisionID = "-1"` (`client.go:230`). Both checks live in `beginWrite` (`pkg/client/revisionwrite.go` ~line 125), which **returns immediately when `CreateRevision=false`**: ```go if opt.CreateRevision != nil && !*opt.CreateRevision { return nil, nil } ``` So `WriteFile(…, WriteOptions{CreateRevision: &false, IfNotExists: true})` **overwrites an existing file** with no error, and a stale `LatestRevisionID` is silently accepted. The comment above `beginWrite` gives the reason for skipping the revision store (connectors must not break on a corrupt index), but that reason does not cover dropping the caller's precondition. ## Concrete failure scenario A caller that uses `IfNotExists` as a create-once guard (e.g. "write the default config only if none exists") and also passes `CreateRevision=false` to avoid history clobbers the user's file on every run. ## Fix Pick one, and document it: - **Enforce it:** when `IfNotExists` is set and `CreateRevision=false`, check existence with `contentExists` / `StatFile` and return the `ConflictError`. When `LatestRevisionID` is set without revisions, return an error, since there is no revision to compare against. - **Reject it:** return an error for the combination, so the caller learns it is meaningless. Silently ignoring it is the one wrong answer. Add tests for both options on each provider. ## Where it was found Review of #27 / #28. Related to basepeak-ai/bpai#1220.

thklein added the

bug

label

2026-10-03 05:04:30 +00:00

sherpa commented

2026-10-03 05:33:42 +00:00

Member

Confirmed in pkg/client/revisionwrite.go:124-127: beginWrite returns nil, nil immediately when opt.CreateRevision != nil && !*opt.CreateRevision, before the IfNotExists/LatestRevisionID check at lines 134-143 ever runs, so both preconditions are silently dropped whenever CreateRevision=false. A contentExists(ctx, fileName) (bool, error) method already exists on the provider interface (revisionwrite.go:56) and is already used for exactly this kind of existence probe (used around line 82), so implementing the 'enforce it' option is a small, local change: in the CreateRevision=false branch, if opt.IfNotExists is set, call contentExists and return a ConflictError (which WriteFile in client.go:240-242 already converts to FileExistsError) when the file exists; if opt.LatestRevisionID is set to anything other than the IfNotExists sentinel, return an error since there's no revision store to compare against. Tests would need to be added per provider (directory/s3/azure) mirroring the existing skipS3Tests/skipAzureTests gating in directory_test.go.

Questions before this can start:

Enforce (check existence/error on a stale LatestRevisionID) or reject (hard error for CreateRevision=false combined with IfNotExists/LatestRevisionID) — the issue proposes both options equally; which should be implemented?
If LatestRevisionID is set (non-IfNotExists value) together with CreateRevision=false, should that always be a hard error, or only when it's not the "-1" IfNotExists sentinel?

Confirmed in pkg/client/revisionwrite.go:124-127: beginWrite returns `nil, nil` immediately when `opt.CreateRevision != nil && !*opt.CreateRevision`, before the `IfNotExists`/`LatestRevisionID` check at lines 134-143 ever runs, so both preconditions are silently dropped whenever `CreateRevision=false`. A `contentExists(ctx, fileName) (bool, error)` method already exists on the provider interface (revisionwrite.go:56) and is already used for exactly this kind of existence probe (used around line 82), so implementing the 'enforce it' option is a small, local change: in the `CreateRevision=false` branch, if `opt.IfNotExists` is set, call `contentExists` and return a `ConflictError` (which `WriteFile` in client.go:240-242 already converts to `FileExistsError`) when the file exists; if `opt.LatestRevisionID` is set to anything other than the IfNotExists sentinel, return an error since there's no revision store to compare against. Tests would need to be added per provider (directory/s3/azure) mirroring the existing `skipS3Tests`/`skipAzureTests` gating in directory_test.go. **Questions before this can start:** - Enforce (check existence/error on a stale LatestRevisionID) or reject (hard error for CreateRevision=false combined with IfNotExists/LatestRevisionID) — the issue proposes both options equally; which should be implemented? - If LatestRevisionID is set (non-IfNotExists value) together with CreateRevision=false, should that always be a hard error, or only when it's not the "-1" IfNotExists sentinel?

sherpa added the

size/M

clarity/ambiguous

area/client

labels

2026-10-03 05:33:42 +00:00