Code Review

Clean, well-scoped PR. The layer separation is solid — CLI validates input, libswamp threads the override, and the domain resolves the timeout with a clear 4-tier precedence chain. 19 new tests cover the happy paths and edge cases thoroughly.

Blocking Issues

None.

Suggestions

1. CreateDatastoreSyncDepsOptions not re-exported from barrel — src/libswamp/mod.ts (line 946–953) exports createDatastoreSyncDeps but not the new CreateDatastoreSyncDepsOptions interface. The CLI command sidesteps this by passing an inline object literal, but any future consumer that wants to type an options variable would need to import from the internal path ./datastores/sync.ts, violating the libswamp import boundary. Consider adding type CreateDatastoreSyncDepsOptions to the barrel export.

2. JSDoc verbosity — The multi-paragraph JSDoc on SYNC_TIMEOUT_CLI_MAX_SECONDS (datastore_sync.ts:42–52) and parseTimeoutFlag (datastore_sync.ts:55–62) go beyond the project's "default to no comments" convention. The first sentence of each captures the essential "why"; the rest could live in the PR description or design doc. Not blocking since the comments are accurate and the constant/function names are already descriptive.

Adversarial Review

Critical / High

None found.

Medium

1. src/libswamp/datastores/sync.ts:67-75 — CreateDatastoreSyncDepsOptions not re-exported from mod.ts
   The createDatastoreSyncDeps function is re-exported from src/libswamp/mod.ts (line 947), but the new CreateDatastoreSyncDepsOptions interface is not. Any consumer outside the CLI that wants to type the options bag (e.g., a programmatic caller or a future test that constructs deps directly) would need to import from the internal path src/libswamp/datastores/sync.ts, which violates the project's barrel-import convention (CLAUDE.md: "CLI commands and presentation renderers must import libswamp types and functions from src/libswamp/mod.ts"). The CLI itself is fine because it passes an object literal that satisfies the type structurally — so no breakage today, just API surface hygiene.

Low

1. src/cli/commands/datastore_sync.ts:114-116 — Cliffy :integer parser may truncate floats silently.
   parseTimeoutFlag correctly rejects 1.5, but Cliffy's :integer type hint may silently truncate 1.5 → 1 before the value reaches the action handler, meaning the !Number.isInteger(raw) guard never fires for float-like CLI input. This is defense-in-depth territory — the code is correct for all values that actually arrive — but worth noting that the test at line 80 (parseTimeoutFlag(1.5)) exercises a path that may be unreachable from actual CLI invocation. No fix needed; just awareness.

2. src/domain/datastore/datastore_sync_service.ts:96-98 — cause set conditionally, override readonly declaration may shadow Error.cause for undefined callers.
   When options is omitted, this.cause is never explicitly assigned, relying on undefined as the implicit value of the optional property. This is correct in current TypeScript/V8 semantics but is subtly different from Error's own cause mechanism (which expects { cause } in the options bag passed to super()). No practical issue — UserError doesn't forward options to super() anyway — but a future refactor that tries to use super(msg, { cause }) would need to update this path.

Verdict

PASS — Solid, well-scoped PR. The validation logic is thorough, the precedence chain is correct and backward-compatible (the existing resolveSyncTimeoutMs(datastoreConfig) call in repo_context.ts:338 is unaffected), the error message is actionable, and the test coverage hits the right boundaries. The medium finding (missing barrel re-export) is API hygiene, not a correctness issue.

CLI UX Review

Blocking

None.

Suggestions

1. SyncTimeoutError remedy 1 is misleading for implicit-sync contexts.
   The SyncTimeoutError fires from flushDatastoreSync() after every command (see src/cli/mod.ts:1014), not just swamp datastore sync. A user running swamp model run @my-model/method who hits the timeout will see • Rerun with --timeout <seconds> as the first remedy — but that flag doesn't exist on their command. They'll try it, get "unknown flag", and have to read further to find the env var.

   The env var remedy (item 2) is the correct escape hatch for implicit syncs, but it's buried after the inapplicable flag suggestion. Consider either swapping the order (env var first, then flag with a note that it applies only to swamp datastore sync) or adding a parenthetical like: • Rerun with swamp datastore sync --timeout before your command (explicit syncs only). Minor but would save real confusion at 3am during a migration.

2. --timeout flag description is verbose relative to other flags.
   The description is 3 sentences including precedence details: "Override the per-direction sync timeout for this invocation (seconds, max 21600). Wins over SWAMP_DATASTORE_SYNC_TIMEOUT_MS and per-datastore config. Preferred escape hatch for one-off large syncs." Other flags in the codebase (--pull, --push, --version) are single short phrases. The precedence context is valuable but belongs in --help --verbose or the example comment rather than the primary description. Suggested trim: "Per-invocation sync timeout in seconds (max 21600). Overrides config and SWAMP_DATASTORE_SYNC_TIMEOUT_MS."

Verdict

PASS — the --timeout flag is well-designed and discoverable, validation produces clean UserError messages with correct escape-hatch guidance, and the enriched SyncTimeoutError is a meaningful improvement over the old single-line message. The example in --help and the 6h cap with env-var escape hatch are good ergonomic choices.