feat: env-driven ob sync-config bootstrap (change 0011)#4
Merged
Conversation
Adds the SyncConfigEnv type and loadSyncConfigEnv validator that resolves the OB_SYNC_FILE_TYPES, OB_SYNC_EXCLUDED_FOLDERS, OB_SYNC_MODE, OB_SYNC_CONFLICT_STRATEGY, OB_SYNC_DEVICE_NAME, and OB_SYNC_CONFIGS variables. Each field is `string | undefined` so the supervisor can later distinguish "unset" (skip flag) from "empty" (forward verbatim as the upstream "empty to clear" sentinel). Invalid enum values fail fast as ConfigError (exit 78) before any vault directory is touched. Plumbs the result through Config.syncConfigEnv and updates every existing Config-literal test fixture so tsc stays clean. The supervisor wiring that consumes these values lands in a follow-up commit.
New module `src/obsidian/syncconfig.ts` translates a validated SyncConfigEnv into an `ob sync-config` argv (`buildSyncConfigArgs`) and runs the spawn through the same retry/backoff envelope as `ensureVaultSetup`: max 5 attempts, 1s/x2/cap 60s, throws-as-transient. `buildSyncConfigArgs` returns null when every field is unset so the supervisor can skip the spawn entirely (preserves on-disk values). Empty strings are forwarded verbatim as the upstream "empty to clear" sentinel. Flag order matches the spec table: file-types, excluded-folders, mode, conflict-strategy, device-name, configs. Reuses DEFAULT_BACKOFF and backoffDelay from setup.ts so the two orchestrators stay in lockstep on retry tuning. SyncConfigPermanentError mirrors SetupPermanentError's shape so the supervisor wiring (next commit) can handle them identically.
…ange 0011) The supervisor now invokes applyVaultSyncConfig between ensureVaultSetup and the continuous `ob sync` child. When every OB_SYNC_* var is unset the call is a logged no-op (preserves prior behavior). Otherwise the spawn order on a fresh vault is sync-status → sync-setup → sync-config → sync, matching the change doc's order-of-operations scenario. A SyncConfigPermanentError marks the vault `failed` with lastError and prevents `child.start()` from being called for that vault — a sync-config failure on one vault never blocks others (vaults are processed serially but per-vault failure stays per-vault). Re-exports SyncConfigPermanentError so callers can recognise the error class. New tests cover: (1) the all-vars-unset no-op (default), (2) the order-of-operations scenario with OB_SYNC_FILE_TYPES set, (3) permanent sync-config failure marking the vault failed and blocking the sync child, and (4) non-Error throws stringifying into lastError via the catch-handler.
Adds the six OB_SYNC_* variables to the README's Configuration table with allowed values, "unset preserves" / "empty clears" semantics, and a worked example explaining the typical .json/.txt/.docx fix (OB_SYNC_FILE_TYPES=image,audio,pdf,video,unsupported). Mirrors the documentation in .env.example as a commented OB_SYNC_* block so operators can copy-paste the file as a starting point. Marks the change-doc task checkboxes off; the PR-number row is left for the parent agent to fill once the PR exists.
…nc-config API Collapses ~50 LOC of duplication between sync-setup and sync-config orchestrators onto a single retry primitive in src/obsidian/backoff.ts. - New `runWithBackoff` owns the probe → spawn → exit-code → retry loop; throws inside an attempt are synthesised to -1 in one place. Both `ensureVaultSetup` and `applyVaultSyncConfig` now delegate. - `Backoff` (renamed from `SetupBackoff`, alias kept) plus `drain` move to backoff.ts and are re-exported. - `applyVaultSyncConfig(vault, deps, env)` now mirrors `ensureVaultSetup`'s signature; `log` lives inside `SyncConfigDeps` as `logger`. - `SyncConfigEnv` enum constants get proper union types (`SyncMode`, `SyncConflictStrategy`, `SyncFileType`, `SyncConfigKey`); validators rewritten as `parseSync*` helpers shaped like `parseLevel`/`parseProvider`. - Drop WHAT-narrating comments in the touched files; keep WHY only.
📝 WalkthroughWalkthroughAdds env-driven per-vault ChangesSync Configuration Bootstrap
Sequence DiagramsequenceDiagram
participant Supervisor
participant Setup
participant SyncConfig
participant Backoff
participant ObCLI
Supervisor->>Setup: ensureVaultSetup(vault)
alt already configured
Setup-->>Supervisor: configured
else not configured
Setup->>Backoff: runWithBackoff(runSetupOnce)
Backoff->>ObCLI: run "ob sync-setup"
ObCLI-->>Backoff: exit (0/!=0)
Backoff-->>Setup: result (ok/cancelled/failure)
Setup-->>Supervisor: setup completed / throws SetupPermanentError
end
Supervisor->>SyncConfig: buildSyncConfigArgs(cfg.syncConfigEnv, vaultPath)
alt args === null
SyncConfig-->>Supervisor: no-op (skip)
else args present
SyncConfig->>Backoff: runWithBackoff(runSyncConfig)
Backoff->>ObCLI: run "ob sync-config" with args
ObCLI-->>Backoff: exit (0/!=0) or throw
Backoff-->>SyncConfig: ok / cancelled / final failure
alt final failure
SyncConfig-->>Supervisor: throw SyncConfigPermanentError
Supervisor-->>Supervisor: mark vault failed, set lastError, skip spawn
else ok
SyncConfig-->>Supervisor: success
end
end
Supervisor->>ObCLI: spawn "ob sync --continuous" (if not failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4 +/- ##
========================================
Coverage 99.90% 99.91%
========================================
Files 59 61 +2
Lines 4404 4592 +188
========================================
+ Hits 4400 4588 +188
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds an env-var-driven, per-vault ob sync-config bootstrap step (Change 0011) to let operators configure Obsidian Sync behavior via OB_SYNC_* at startup, without interactive pod access.
Changes:
- Introduces
src/obsidian/backoff.tsand refactorssync-setupretries to use the sharedrunWithBackoff/drainhelpers. - Implements
src/obsidian/syncconfig.ts(buildSyncConfigArgs+applyVaultSyncConfig) and wires it into the supervisor init sequence betweensync-setupandsync --continuous. - Adds
loadSyncConfigEnvvalidation/plumbing in config, plus comprehensive unit tests and operator docs (README.md,.env.example, change doc task updates).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/server.test.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/relevance/eval.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/obsidian/syncconfig.test.ts | New unit tests covering env→argv, spawn/no-op, retries, and cancellation for sync-config. |
| test/obsidian/index.test.ts | Extends supervisor integration tests for sync-config ordering and permanent-failure behavior. |
| test/obsidian/backoff.test.ts | New unit tests for shared backoff/retry primitive and drain. |
| test/mcp/helpers.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/indexer/index.test.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/http/routes.test.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/http/helpers.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/http.test.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/embeddings/index.test.ts | Updates test config fixture to include new required syncConfigEnv. |
| test/config/syncConfig.test.ts | New unit tests for loadSyncConfigEnv validation/normalization behavior. |
| test/config.test.ts | Adds tests ensuring loadConfig plumbs and surfaces OB_SYNC_* validation. |
| src/obsidian/syncconfig.ts | New ob sync-config orchestration: argv build + backoff-wrapped execution + permanent error. |
| src/obsidian/setup.ts | Refactors setup to use shared backoff/drain/runWithBackoff; re-exports Backoff types. |
| src/obsidian/index.ts | Wires applyVaultSyncConfig into per-vault init before spawning sync --continuous. |
| src/obsidian/backoff.ts | New shared retry primitive (Backoff, DEFAULT_BACKOFF, runWithBackoff, drain). |
| src/config/index.ts | Adds SyncConfig env types + loadSyncConfigEnv validation; plumbs into Config. |
| README.md | Documents OB_SYNC_* env vars and their unset vs empty-string semantics. |
| docs/changes/0011-sync-config-bootstrap.md | Marks core tasks complete for Change 0011 (but status/index tracking still pending). |
| .env.example | Documents optional OB_SYNC_* variables and semantics. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/config/index.ts`:
- Around line 238-248: The parseCsvSubset function validates CSV tokens but
returns the original raw string (so "image, pdf" retains whitespace); change it
to return the normalized, validated CSV by joining the trimmed tokens (use the
existing tokens array) so callers receive a clean comma-separated string
(preserve the early-return for empty string). Update the return in
parseCsvSubset to return tokens.join(",") after validation.
In `@src/obsidian/backoff.ts`:
- Around line 82-92: In the catch block that sets code = -1 (when an exception
is treated as transient), capture and preserve the last thrown error (e) into a
variable like lastError so it survives retries instead of being lost; then, when
creating/throwing the permanent error after retry exhaustion, pass that
preserved lastError into the permanent-error constructors so callers can inspect
the original ENOENT/raw error (update the catch around opName/vaultSlug/attempt
i and the code-path that constructs permanent errors to include lastError).
- Around line 100-103: The backoff sleep currently awaits sleep(delay) and only
re-checks shouldStop() after the full delay, which can block stop() for the
entire backoff window; change the wait to be stop-aware by replacing the direct
await sleep(delay) with a cancellable/wakeable wait (e.g. Promise.race between
sleep(delay) and a stop signal) or a small-tick loop that checks shouldStop()
periodically and breaks early; locate the backoff loop using
backoffDelay(backoff, i - 1), shouldStop(), sleep(), and logger.info(`${opName}
backing off`, { vault: vaultSlug, delayMs: delay }) and ensure that if
shouldStop() becomes true during the backoff you abort the sleep and return
immediately so stop() latency is bounded.
In `@test/obsidian/index.test.ts`:
- Around line 690-730: Update the test "non-Error throw from a sync-config
sub-step stringifies into lastError" to assert that the actual non-Error thrown
value ("raw sync-config failure") is included in the supervisor's recorded
lastError (in addition to the existing "permanently" check); locate the spawner
mock (sp.run) that throws the literal string and the supervisor instance (sup)
whose vault state / lastError is inspected (sup.get("v")?.lastError), then add
an expectation that lastError contains the stringified thrown value so the test
fails if the raw error is lost.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31938038-3f0c-42fc-9f51-9a079e57b00b
📒 Files selected for processing (21)
.env.exampleREADME.mddocs/changes/0011-sync-config-bootstrap.mdsrc/config/index.tssrc/obsidian/backoff.tssrc/obsidian/index.tssrc/obsidian/setup.tssrc/obsidian/syncconfig.tstest/config.test.tstest/config/syncConfig.test.tstest/embeddings/index.test.tstest/http.test.tstest/http/helpers.tstest/http/routes.test.tstest/indexer/index.test.tstest/mcp/helpers.tstest/obsidian/backoff.test.tstest/obsidian/index.test.tstest/obsidian/syncconfig.test.tstest/relevance/eval.tstest/server.test.ts
Per Copilot review on PR #4: change 0011's task list is fully checked off (only the post-merge changelog flip remained), so flip the change doc Status header and both docs/index.md / docs/index.yml entries to `complete`, matching the convention used for changes 0007–0010. Mark the final task as done with the merged PR number.
- parseCsvSubset: return the trimmed/normalized CSV instead of the raw
string so " image, pdf " no longer survives into `ob sync-config`
argv as whitespace, turning a config typo into a runtime failure.
- runWithBackoff: preserve the most recent thrown error across retry
exhaustion via a new optional `lastError` field on the failure
result, and propagate it through both `SetupPermanentError` and
`SyncConfigPermanentError` constructors so launch-time ENOENT/raw
values surface in the supervisor's `lastError` instead of being
flattened to a generic permanent-failure message.
- runWithBackoff: slice the inter-attempt sleep into ≤250ms chunks
with a `shouldStop()` poll between each slice so a stop signal
arriving mid-window aborts promptly instead of holding `stop()`
open for up to `capMs`.
- Strengthen the supervisor's non-Error-throw test to assert the raw
thrown value ("raw sync-config failure") survives the chain — the
existing "permanently" check passed even when the original error
was lost.
- Update existing backoff/setup/syncconfig retry tests to reflect the
sliced-sleep timing (sum of slices unchanged; each slice ≤250ms).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config/index.ts (1)
41-47: ⚡ Quick winTighten
SyncConfigEnvfield types to preserve validation guarantees across modules.After validation,
modeandconflictStrategycan be typed as unions instead of genericstring; this prevents accidental invalid values when constructingSyncConfigEnvoutsideloadSyncConfigEnv.Proposed typing refinement
export interface SyncConfigEnv { readonly fileTypes?: string; readonly excludedFolders?: string; - readonly mode?: string; - readonly conflictStrategy?: string; + readonly mode?: SyncMode | ""; + readonly conflictStrategy?: SyncConflictStrategy | ""; readonly deviceName?: string; readonly configs?: string; } -function parseSyncMode(raw: string): string { +function parseSyncMode(raw: string): SyncMode | "" { // Empty string is the upstream "empty to clear" sentinel — pass through. if (raw === "") return raw; - if ((VALID_SYNC_MODES as readonly string[]).includes(raw)) return raw; + if ((VALID_SYNC_MODES as readonly string[]).includes(raw)) return raw as SyncMode; throw new ConfigError( `OB_SYNC_MODE must be one of ${VALID_SYNC_MODES.join("|")} (or empty), got "${raw}"`, ); } -function parseSyncConflictStrategy(raw: string): string { +function parseSyncConflictStrategy(raw: string): SyncConflictStrategy | "" { if (raw === "") return raw; - if ((VALID_SYNC_CONFLICT_STRATEGIES as readonly string[]).includes(raw)) return raw; + if ((VALID_SYNC_CONFLICT_STRATEGIES as readonly string[]).includes(raw)) { + return raw as SyncConflictStrategy; + } throw new ConfigError( `OB_SYNC_CONFLICT_STRATEGY must be one of ${VALID_SYNC_CONFLICT_STRATEGIES.join("|")} (or empty), got "${raw}"`, ); }Also applies to: 221-236
🤖 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 `@src/config/index.ts` around lines 41 - 47, The SyncConfigEnv interface currently uses generic string types for mode and conflictStrategy which weakens validation; change those fields to the appropriate string union types (e.g., replace mode: string and conflictStrategy: string with the exact allowed unions or an existing enum type used elsewhere) so only valid values accepted, update any other identical interface instance noted (the same interface block around the other occurrence) and ensure loadSyncConfigEnv's returned values conform to the new unions (adjust exports/imports/tests/types using SyncConfigEnv or cast/validate accordingly).
🤖 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.
Nitpick comments:
In `@src/config/index.ts`:
- Around line 41-47: The SyncConfigEnv interface currently uses generic string
types for mode and conflictStrategy which weakens validation; change those
fields to the appropriate string union types (e.g., replace mode: string and
conflictStrategy: string with the exact allowed unions or an existing enum type
used elsewhere) so only valid values accepted, update any other identical
interface instance noted (the same interface block around the other occurrence)
and ensure loadSyncConfigEnv's returned values conform to the new unions (adjust
exports/imports/tests/types using SyncConfigEnv or cast/validate accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c2eb37-7135-4fee-b1af-f73dccd6d9c7
📒 Files selected for processing (9)
src/config/index.tssrc/obsidian/backoff.tssrc/obsidian/setup.tssrc/obsidian/syncconfig.tstest/config/syncConfig.test.tstest/obsidian/backoff.test.tstest/obsidian/index.test.tstest/obsidian/setup.test.tstest/obsidian/syncconfig.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/obsidian/backoff.ts
- test/obsidian/index.test.ts
- test/obsidian/backoff.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements change 0011: an env-var-driven
ob sync-configstep that runs once per vault betweensync-setupandsync --continuous, so operators can configure file types, excluded folders, sync mode, conflict strategy, configs, and device name withoutkubectl exec'ing into the pod.OB_SYNC_*env vars (FILE_TYPES,EXCLUDED_FOLDERS,MODE,CONFLICT_STRATEGY,DEVICE_NAME,CONFIGS) validated at startup; invalid enums fail fast withConfigError(exit 78).unset→ flag omitted (preserve on-disk value);""→ flag forwarded verbatim ("empty to clear" upstream semantic).ensureVaultSetup: 5 attempts, 1s/×2/cap 60s, throws treated as transient. Permanent failure marks the vaultfailedand skips theob syncspawn.runWithBackoff+drain+Backofftypes intosrc/obsidian/backoff.ts, eliminating ~50 LOC of duplication between setup and sync-config.Spec / change references
Test plan
bun run test:covpasses locally with 100% line + function coverage on everysrc/file (823 pass / 0 fail / 4 pre-existing env-gated skips, 55 src/ files at 100%)bun run typecheckcleanbunx biome check src/ test/cleanbuildSyncConfigArgsreturnsnull; supervisor skips the spawnOB_SYNC_EXCLUDED_FOLDERS=""→["sync-config", "--path", "/p", "--excluded-folders", ""]OB_SYNC_MODE→ConfigErrorwith exit 78, names the var and lists valid valuesobchild is spawnedsync-setup→sync-config→sync --continuoussync-configfailure recovers (exit 1 then exit 0; both invocations use same argv; child only spawned after success)sync-configfailure (5 consecutive non-zero exits) marks vaultfailed;sync --continuousnot spawned; other vaults unaffectedSummary by CodeRabbit
New Features
Improvements
Documentation
Tests