docs: plan env-driven obsidian-headless sync-config (change 0011)#3
Merged
Conversation
Adds the Sync-configuration-bootstrap requirements to the obsidian-sync spec and a new change document (0011) that plans how OB_SYNC_* env vars will drive an `ob sync-config` step between sync-setup and continuous sync. Lifts the prior "Per-vault sync mode override is out of scope" constraint and updates the docs indexes.
📝 WalkthroughWalkthroughAdds a design and spec integration for a Sync Config Bootstrap: an env-var-driven startup step (between sync-setup and ChangesSync Config Bootstrap
Sequence DiagramsequenceDiagram
participant Supervisor as Supervisor
participant Startup as Startup
participant VaultManager as VaultManager
participant obCLI as obCLI
Supervisor->>Startup: read OB_SYNC_* env vars
Startup->>VaultManager: enumerate fresh vaults
loop per vault
VaultManager->>Startup: vault ready
Startup->>obCLI: run `ob sync-config` with built args
obCLI-->>Startup: success / transient error / permanent error
alt success
Startup->>VaultManager: mark vault configured
else transient error
Startup->>Startup: retry with backoff
else permanent error
Startup->>VaultManager: mark vault failed (prevent sync-start)
end
end
Startup->>Supervisor: proceed to `ob sync --continuous` for configured vaults
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/changes/0011-sync-config-bootstrap.md`:
- Around line 132-140: The doc references two different signatures for
applyVaultSyncConfig; standardize to a single callable signature across the docs
and examples (update both the function declaration and the example call). Pick
one signature (e.g., applyVaultSyncConfig(vault, deps, log, env) or
applyVaultSyncConfig(vault, deps, env)) and update the definition near "1. Pure
validation..." and the usage in the startSupervisor per-vault init loop (the
example after ensureVaultSetup) to match; also ensure related references to
buildSyncConfigArgs and ensureVaultSetup remain consistent with the chosen
signature.
In `@docs/index.yml`:
- Around line 97-103: Update the index metadata for the entry with id "0011" by
adding "0002" to its depends_on list so the change correctly declares it extends
the supervisor lifecycle from 0002; locate the block for id "0011" (name:
sync-config-bootstrap, spec: obsidian-sync) and replace the empty depends_on: []
with a list containing "0002".
In `@docs/specs/obsidian-sync/index.md`:
- Line 228: The changelog row that currently says "Lifts the prior 'Per-vault
sync mode override is out of scope for v1' constraint" contradicts earlier
language that keeps per-vault overrides out of scope (the phrasing "Per-vault
sync mode override is out of scope for v1"); update the changelog entry text
(the 2026-05-06 Add Sync-configuration-bootstrap line) to explicitly state that
the new sync-configuration-bootstrap adds an env-var-driven bootstrap step and
mappings/validation/retry semantics but does not enable per-vault sync mode
overrides for v1, so the wording does not imply that the out-of-scope constraint
has been lifted.
🪄 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: 2d050390-90e0-4cc0-b0b0-810bdbbec112
📒 Files selected for processing (4)
docs/changes/0011-sync-config-bootstrap.mddocs/index.mddocs/index.ymldocs/specs/obsidian-sync/index.md
There was a problem hiding this comment.
Pull request overview
This documentation-only PR updates the Obsidian Sync spec and change-log to plan and specify an env-driven ob sync-config bootstrap step (new Change 0011) so sync settings can be configured via OB_SYNC_* environment variables between sync-setup and sync --continuous.
Changes:
- Add a new Sync configuration bootstrap requirements section to the Obsidian Sync spec (env-var → flag mapping, validation rules, retry semantics, and scenarios).
- Introduce Change 0011 — Sync Config Bootstrap as the implementation plan (design + tasks + testing requirements).
- Register Change 0011 in
docs/index.ymlanddocs/index.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/specs/obsidian-sync/index.md | Adds spec requirements + scenarios for env-driven ob sync-config, plus error taxonomy and architecture diagram note. |
| docs/index.yml | Adds Change 0011 entry to the documentation index. |
| docs/index.md | Adds Change 0011 row to the changes table. |
| docs/changes/0011-sync-config-bootstrap.md | New change document describing motivation, requirements, design, tasks, and open questions for env-driven sync-config bootstrap. |
- 0011 change doc: standardize applyVaultSyncConfig signature to (vault, deps, log, env) on line 133 to match the call site on line 140 - docs/index.yml: set change 0011 depends_on to ["0002"] (extends the supervisor lifecycle introduced in 0002) - obsidian-sync changelog: clarify that change 0011 replaces the prior global sync-config deferral; per-vault overrides remain out of scope Also includes parallel Copilot-thread fixes already in the working tree (0011 Depends On line 9, docs/index.md row 27, sync-spec lines 97/110).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/specs/obsidian-sync/index.md`:
- Line 68: Change the unconditional requirement to invoke ob sync-config into a
conditional one: update the sentence referencing the supervisor invoking "ob
sync-config --path <dir>" (after "sync-setup" and before spawning "ob sync
--continuous") so it reads that the supervisor MUST invoke "ob sync-config
--path <dir>" with flags derived from environment variables UNLESS all OB_SYNC_*
environment variables are unset (in which case the supervisor MUST NOT invoke
"ob sync-config"). Also adjust the related rules around "ob sync-config" (the
section covering OB_SYNC_* handling, currently around lines 101-104) to match
this single unambiguous rule so both the "sync-setup" paragraph and the
OB_SYNC_* paragraph state the same conditional behavior.
🪄 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: 77c44270-de9c-4bce-9d16-b4be7a6fb776
📒 Files selected for processing (4)
docs/changes/0011-sync-config-bootstrap.mddocs/index.mddocs/index.ymldocs/specs/obsidian-sync/index.md
Resolves CodeRabbit review feedback: the prior wording at line 68 was
unconditional ('MUST invoke') but the rest of the section (and the
'all sync-config vars unset' scenario) requires skipping when every
OB_SYNC_* var is unset. Make line 68 conditional so implementers have
a single unambiguous rule.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/specs/obsidian-sync/index.md (1)
203-204: ⚡ Quick winClarify whether SyncConfig error types are part of the public surface.
The Error taxonomy documents
SyncConfigTransientErrorandSyncConfigPermanentError, and snippet 1 shows that the existing code exports similarSetup*error types fromsrc/obsidian/index.ts. However, the "Public surface to the rest of the app" section (lines 151-173) doesn't mention error exports at all.For consistency and clarity, consider adding a note to the Public surface section specifying whether
SyncConfigTransientErrorandSyncConfigPermanentErrorshould be exported following the established pattern, or whether they're internal-only to the obsidian module.📝 Suggested clarification
Add after line 169 (before the closing ```):
interface Supervisor { list(): VaultStatus[]; get(slug: string): VaultStatus | null; stop(): Promise<void>; } + +// Error types (for callers that need to distinguish error causes) +class SetupPermanentError extends Error { ... } +class SetupTransientError extends Error { ... } +class SyncConfigPermanentError extends Error { ... } +class SyncConfigTransientError extends Error { ... }Or add a prose note below line 173:
- `list()` and `get()` MUST be synchronous, non-blocking reads of in-memory state. - The supervisor MUST NOT expose raw child handles to other modules. +- Error types (`SetupPermanentError`, `SetupTransientError`, `SyncConfigPermanentError`, `SyncConfigTransientError`) MUST be exported to allow callers to distinguish error causes.🤖 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 `@docs/specs/obsidian-sync/index.md` around lines 203 - 204, The docs currently list SyncConfigTransientError and SyncConfigPermanentError in the error taxonomy but do not state whether they are exported on the obsidian public surface; update the "Public surface to the rest of the app" section to explicitly state whether these error types are exported (following the existing pattern of exporting Setup* errors from src/obsidian/index.ts) or are internal-only. Specifically, indicate the intended visibility (exported vs internal), and if exported, add a note listing the exact symbols SyncConfigTransientError and SyncConfigPermanentError so readers know to import them from the obsidian module.
🤖 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 `@docs/specs/obsidian-sync/index.md`:
- Around line 203-204: The docs currently list SyncConfigTransientError and
SyncConfigPermanentError in the error taxonomy but do not state whether they are
exported on the obsidian public surface; update the "Public surface to the rest
of the app" section to explicitly state whether these error types are exported
(following the existing pattern of exporting Setup* errors from
src/obsidian/index.ts) or are internal-only. Specifically, indicate the intended
visibility (exported vs internal), and if exported, add a note listing the exact
symbols SyncConfigTransientError and SyncConfigPermanentError so readers know to
import them from the obsidian module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6caa665f-2784-4ded-8efe-ba22badad4ef
📒 Files selected for processing (1)
docs/specs/obsidian-sync/index.md
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
### Sync configuration bootstraprequirements subsection: env-var →ob sync-configflag mapping, validation rules, retry semantics, and four GIVEN/WHEN/THEN scenarios. Also adds two error-taxonomy rows and notessrc/obsidian/syncconfig.tsin the architecture diagram.Constraintsline that deferred sync-mode override ("OUT OF SCOPE for v1") — this change supersedes it.OB_SYNC_FILE_TYPES,OB_SYNC_EXCLUDED_FOLDERS,OB_SYNC_MODE,OB_SYNC_CONFLICT_STRATEGY,OB_SYNC_DEVICE_NAME,OB_SYNC_CONFIGSmapped one-to-one toob sync-configflags, applied per vault betweensync-setupandsync --continuous, with fail-fast enum validation and exponential-backoff retry on non-zero exit. Empty string clears, unset preserves.docs/index.ymlanddocs/index.mdwith the new change row.Motivation
Production usage revealed the supervisor never invokes
ob sync-config, so the upstream defaults (File types: image, audio, pdf, video) are in force. JSON / TXT / CSV files (e.g. voice transcripts underlogs/transcripts/) created on the pod were never pushed to the cloud, even after the user enabled "All other file types" on desktop — that toggle updates the desktop client, not the pod's local filter. The only fix today iskubectl exec+ manualob sync-config, lost on pod restart. This change documents the env-driven solution.Test plan
Documentation-only PR — no code changes, no tests to run.
#sync-configuration-bootstrapdocs/index.ymlanddocs/index.mdlist change 0011 with statusdraftImplementation will land in a follow-up PR once the plan is reviewed.
Summary by CodeRabbit