diff --git a/docs/decisions/2026-09-15-reposync-extraction-declined.md b/docs/decisions/2026-09-15-reposync-extraction-declined.md new file mode 100644 index 0000000000..8b515cb57f --- /dev/null +++ b/docs/decisions/2026-09-15-reposync-extraction-declined.md @@ -0,0 +1,260 @@ +# ADR-2026-09-15-reposync-extraction-declined: Keep workflow sync and Office config sync as separate packages + +**Status:** accepted +**Date:** 2026-09-15 +**Area:** backend + +## Context + +`internal/workflowsync` syncs workflow definitions from GitHub or GitLab into a +workspace. `internal/office/configsync`, merged in PR #3304 on 2026-09-03, syncs +Office agents, projects, skills, and routines from the same two providers. The +two packages were expected to share an extracted `internal/reposync` library. + +That extraction was proposed before the second domain existed. Round 4 of the +Office config sync spec review found that four of its eleven findings existed +only because the design required extracting the shared library from the shipped +workflow sync package while promising workflow sync stayed byte-identical. Each +was a collision between Office's contract and a shipped workflow sync quirk, +forced by physical code sharing rather than by either domain's requirements. + +The decision then was to build `internal/office/configsync` standalone, reusing +workflow sync's pattern rather than its package, and to revisit the extraction +once the second domain was real rather than predicted. The system design set the +condition for this revisit explicitly, in wording it no longer carries and this +ADR now preserves: "'not worth extracting' is only a defensible verdict once both +halves exist to be compared." Both halves now exist. This ADR is that revisit. + +### The four round-4 predictions, all now shipped + +Verified against `origin/main` at `afeebea2d`. + +| Concern | `internal/workflowsync` | `internal/office/configsync` | +| --- | --- | --- | +| Unchanged verdict | `service.go:238` includes `&& len(warnings) == 0`, so warnings flip the verdict | `reconcile_run.go:214` omits the warnings term | +| Provider default | `models.go:89` defaults an empty provider to GitHub | `models.go:145` rejects an empty provider | +| Run deadline | none anywhere in the package | `service.go:21` defines `runDeadline = 10 * time.Minute`, applied at `service.go:208` | +| Warning render cap (frontend) | `workflow-sync-status-banner.tsx:75` renders all warnings | `office-config-sync-status-card.tsx:77` applies `slice(0, MAX_VISIBLE_WARNINGS)` | + +### Three further shipped divergences, from the design's own list + +The round-4 findings and the system design's divergence enumeration are two +different lists. The design named five points on which the callers diverge, and +three of them are absent from the round-4 table above. All three are shipped: + +| Concern | `internal/workflowsync` | `internal/office/configsync` | +| --- | --- | --- | +| Empty path | `models.go:163-164` rewrites an empty path to `DefaultPath`, so the repository root cannot be configured | `models.go:243-245` treats an empty path as the repository root, which is why `Path` is `*string` here | +| Walk shape | `service.go:249-252` fetches one non-recursive directory listing | `walk.go:163-164` runs a bounded multi-round directory walk | +| Warning retention (backend) | no cap | `reconcile_run.go:18` caps retention at `maxRecordedWarnings = 100` and appends a truncation entry | + +Two accounting notes, so the tally is auditable rather than merely asserted. The +backend retention cap and the frontend render cap are distinct concerns that +happen to share the word "warning"; the design's fifth point is the former, and +round 4's fourth finding is the latter. And the run deadline was *not* among the +design's five: it entered from Office's own `AC-OFFICE-CONFIG-SYNC-004.4a` after +that list was written, so it is a divergence discovered by building, not a +predicted one. + +That is seven shipped divergences, not four. Each is deliberate. Office rejects +an empty provider because it is new and has no pre-provider clients, while +workflow sync's GitHub default is what every pre-existing row implicitly was. +`configsync/models.go:306-312` documents the verdict divergence in the source +itself. + +### The shared surface, itemized + +The genuinely common, collision-free surface is roughly 126 lines. The count is +given per item so a future reader can check it rather than take it on trust, which +only works if the counting rule is stated: **every figure below is physical lines, +including doc comments and internal blank lines, over the span named in its own +row.** Figures are `workflowsync` / `configsync`, non-test. + +| Item | Lines | Span counted | Status | +| --- | --- | --- | --- | +| `Poller` | 81 / 83 | All of `poller.go`, which holds the 60s tick constant, the struct and its four methods, and nothing else | Identical in logic; the only non-comment difference is the logger component string | +| Client provider interfaces + compile-time assertions | 28 / 29 | Both interface declarations plus the `var` assertion block, first doc comment through the closing `)` | Identical but for one comment line (`configsync` documents "Satisfied by github.Service." on both interfaces, `workflowsync` on only one) | +| Provider-neutral directory entry struct | 8 / 6 | The struct with its doc comment, three lines here and one there | Same three fields; unexported `dirEntry` here, exported `DirEntry` there. The declarations themselves are 5 lines on both sides | +| Per-workspace lock helper | 4 / 4 | The function; neither side has a doc comment | Byte-identical | +| Provider constants | 5 / 5 | The `const` block with its doc comment | Byte-identical, comment included | +| **Total** | **126 / 127** | | | + +Three further candidates were examined. Two are **not** part of that surface; +the third is, and is listed here only because it is easy to assume otherwise: + +- **Content hashing.** `workflowsync/service.go:325` (`contentHash`, 8 lines) and + `configsync/reconcile_run.go:441` (`computeRunHash`, 22 lines) share a purpose + (an informational digest, never used to skip reconciliation) but not an + implementation. They take different inputs (a flat file slice versus a walk + result spanning four entity kinds), frame the hash differently (`path` plus + content length versus `kind` plus `path`), and order differently (walk order + versus an explicit sort of skill directories). What is common is the + `sha256` plus `hex` idiom, which is the standard library. +- **The HTTP controller.** Both expose the same four routes (get, set, delete, + force-sync), and there the resemblance stops. Workflow sync's `Controller` + (159 lines) self-mounts on the root engine at `/api/v1/workflow-sync`, takes + the workspace from a `workspace_id` query parameter, and re-authorizes in every + handler by translating a service denial into a 404. Office's `Handler` (111 + lines) is registered onto a router group the caller owns, takes the workspace + from the `:wsId` path parameter, and deliberately does not re-authorize because + `officeWorkspaceScopeMiddleware` has already scope-checked it. The workspace + source, the mount model, and the authorization model all differ. This is a seam, + not shared code, and it is counted in the Decision's tally below. +- **Provider constants** are in the table above: genuinely shareable, and five + lines under the convention stated there. + +Two asymmetries appeared only once the second package was real. The stores are +not the same table under two names: config sync adds an ownership manifest table +and its own index, while workflow sync carries two idempotent `ALTER TABLE` +migration helpers for databases that predate its poll toggle and its GitLab +columns. Office has no legacy rows and will never need them, so a +table-parameterized shared store would carry workflow sync's migration history +into a package Office depends on. The packages are also lopsided: 1,105 versus +4,050 non-test lines, because config sync's bulk is reconciliation across four +entity kinds with no workflow sync counterpart. + +## Decision + +Do not extract `internal/reposync`. `internal/workflowsync` and +`internal/office/configsync` remain separate packages that share a pattern rather +than code. + +A shared layer would have to parameterize the verdict formula, the provider +default policy, the run deadline, the empty-path meaning, the walk shape, the +warning caps (backend retention and frontend render, separately), the store +schema and its migration history, and the HTTP controller's workspace source and +authorization model, in order to save roughly 126 lines that do not collide. +Every one of those seams exists to let the two domains disagree, which means the +abstraction would encode the disagreement rather than remove it. + +The duplicated `Poller` is real but is not a repository-sync concern. A +`Start`/`Stop` lifecycle over a context cancel and a `WaitGroup` appears in at +least five packages: `internal/integrations/healthpoll`, `internal/gitlab`, +`internal/linear`, `internal/workflowsync`, and `internal/office/configsync`. +The shape is common to all five; the specific choice to hold the mutex across +`WaitGroup.Wait()` is shared only by the two sync pollers, while `healthpoll`, +`gitlab` and `linear` release it before waiting and re-acquire it only to clear +`started`. Extracting the lifecycle into a `reposync` package used by two of +those five would put it in the wrong home and leave the other three copies in +place. It is recorded as separate, repo-wide follow-up work rather than folded +into this decision. + +## Consequences + +A change to sync behavior that should apply to both domains must be made twice, +and nothing in the build enforces that. This is accepted: no such change has been +required so far, and the seven divergences above are evidence that most changes +apply to exactly one domain. + +The two packages may drift further apart. **This supersedes three statements in +`docs/specs/office/system-design/config-sync.md`**. That design has been updated +so none of them still reads as pending, which makes this ADR their only record; +they are quoted in full here for that reason: + +- *Purpose and boundaries*: "Extracting the common mechanics is deferred so it + can be designed against two working implementations rather than one working and + one imagined." The deferral is resolved. Nothing is pending. +- *`internal/office/configsync` (new)*: the vocabulary and lifecycle are + identical "by *convention*, enforced by this design and by tests, not by a + shared type". The convention is retired, and it was never actually enforced by + tests: no shipped test compares the two packages' column names, field shapes, + or lifecycle. +- *Prior art and alternatives*: the duplication is "bounded by building Office to + the *same* vocabulary (identical column names, the same `SyncResult` field + shape, the same poll-and-record lifecycle) so a later extraction is a merge of + two working implementations rather than a redesign. A follow-up card carries + the extraction". This ADR is the disposition of that follow-up card. + +All three were reasonable when the extraction was still expected. Now that it is +declined, holding the vocabulary aligned buys nothing and would constrain both +domains for a merge that is not going to happen. Each package owns its contract, +and neither honors a behavior-preservation invariant for the other. + +Both client provider interfaces stay duplicated, but each keeps its +compile-time assertion that the real `github.Service` and `gitlab.Service` +satisfy it, so drift in either integration's workspace-routed methods still +breaks the build in both packages rather than surfacing at wiring time. + +Poller lifecycle duplication remains. That work is tracked as Kandev card `fc386556-8101-49a6-ac22-97b526c66e50` ("Unify backend +Poller Start/Stop lifecycle across 5 packages"), scoped repo-wide and +independent of this decision: whatever home it picks, it does not reopen the +question of a `reposync` library for these two. + +**Revisit this decision if any of the following holds.** Absent one of them, two +similar packages is the intended steady state and no revisit is owed. Nothing in +the build or in CI watches for these; they are checked by whoever is already +editing one of the two packages. The first two are state, readable from the code +at any moment. The third is a count over time, so it needs somewhere to +accumulate or it resets with each reader's memory: the Occurrences list below is +that place. + +- A third repository-sync domain is proposed. Three callers change the + arithmetic: the shared surface is paid for once and amortized across three, + and a third domain is also evidence that the pattern is a genuine abstraction + rather than a coincidence of two. +- The divergence count materially shrinks, for example because a requirement + change aligns the verdict formula and the provider policy. The seam count, not + the line count, is what made this a no. The seven divergences are tabulated in + Context across two tables; re-reading them against the code is the whole check. +- A behavior change is required in both domains at once, twice. The first time + is the cost this ADR accepts; a repeat indicates the domains are coupled in a + way this analysis did not find. Append each occurrence below, with the date, + the change, and both call sites. + +### Occurrences + +None recorded. This list is only as good as the discipline of appending to it, +and it is the sole record: there is no counter anywhere else, so an occurrence +that goes unwritten here is an occurrence that did not happen as far as the next +reader is concerned. + +Appending an occurrence is a post-acceptance edit to this document, so it also +takes the repository's amendment marker: change the Status line to +`accepted (amended YYYY-MM-DD)`. Of the 270 files under `docs/decisions/`, 23 +already use that form verbatim and 15 more use a dated or ADR-referencing +variant of it. The marker records that this document changed, not that the +decision reversed. Without the bump the append leaves the Status line +unchanged, and outside this list that line is the only place the change +surfaces: a human skim reads it directly, and `list-docs.py decisions` echoes +it in the Status column. The `--status accepted` filter is not that signal, +since it matches on the leading word alone and lists an amended document +either way. + +## Alternatives considered + +**Extract `internal/reposync` with a `Domain` seam.** The round-4 findings imply +a minimum of a verdict hook and a provider-policy hook, and the shipped code adds +a deadline policy, an empty-path policy, a walk-shape strategy, two warning-cap +policies, a store/migration seam, and a controller mount/authorization seam. +Rejected: that is nine parameterization points, and they would buy roughly 126 +shared lines of which only the `Poller`'s 81 are logic rather than declarations, +and the `Poller` belongs in a five-package home this library would not serve. The +original design's `Domain` seam had no verdict or provider hook at all, so the +prediction was already wrong about its own shape. + +**Extract only the collision-free surface.** Rejected on its own terms. The +interfaces, the directory entry struct, the lock helper and the provider +constants are declarations rather than logic, so sharing them buys little, and +the one piece with real logic, the `Poller`, has a five-package scope that a +`reposync` package cannot serve. + +**Migrate workflow sync onto config sync's semantics first, then share.** This +would change shipped behavior for existing workspaces: warnings would stop +flipping the unchanged verdict, an omitted provider would start failing, a +configured path of `""` would change meaning from "the default directory" to +"the repository root", and every sync run would acquire a ten-minute deadline it +does not have today. Rejected as a behavior change with no requirement asking for +it. Notably, no shipped test covers "warnings present, zero changes" +(`TestSyncWorkspace_AlwaysReconciles` has no warnings, and +`TestSyncWorkspace_BrokenFileBecomesWarningAndNilExport` never asserts +`Unchanged`), so this migration could regress the verdict in either direction +without failing CI. + +## Related specification + +`docs/specs/office/requirements/config-sync.md` and its siblings define the +Office contract whose acceptance criteria drove most of the divergences above. +`docs/specs/office/system-design/config-sync.md` records the original deferral +and the condition for this revisit; see Consequences for the three statements +that this ADR supersedes. That design's Related decisions section links back +to this ADR. diff --git a/docs/specs/office/system-design/config-sync.md b/docs/specs/office/system-design/config-sync.md index ded2d9e32a..fcfb91403b 100644 --- a/docs/specs/office/system-design/config-sync.md +++ b/docs/specs/office/system-design/config-sync.md @@ -35,9 +35,9 @@ Office config sync is built standalone, as `internal/office/configsync`. It reuses workflow sync's *pattern* — the same field vocabulary, provider-dispatch shape, and poll-and-record lifecycle — but not its *package*: no shared library is extracted, and `internal/workflowsync` is neither modified nor imported. -Extracting the common mechanics is deferred so it can be designed against two -working implementations rather than one working and one imagined; the reasoning -is under [Prior art and alternatives](#prior-art-and-alternatives). +Extracting the common mechanics was deferred until both implementations existed +to be compared. That question has since been settled as a decline, so no +extraction is pending; see [Related decisions](#related-decisions). ## Requirement mapping @@ -79,10 +79,10 @@ the reconciliation, and the HTTP surface. Nothing in it is shared with | `Runner` | Per-workspace lock, authorization, fetch, reconcile, `recordFailure`. | | `Controller` | The four HTTP handlers. | -The field vocabulary, the status columns, and the poll-and-record lifecycle are -deliberately identical to `workflow_sync_configs` — that is the requirement the -task sets — but the identity is by *convention*, enforced by this design and by -tests, not by a shared type. +The field vocabulary, the status columns, and the poll-and-record lifecycle +started out identical to `workflow_sync_configs` — that is the requirement the +task sets — but the identity was convention, never a shared type or a test, and +it is no longer maintained; see [Related decisions](#related-decisions). ### `internal/workflowsync` (unchanged) @@ -140,12 +140,10 @@ of `TestOfficeRouteScopeCompleteness`. ### Config fields -A separate table, with column names, types, and semantics deliberately -identical to `workflow_sync_configs` so a later extraction is a merge rather -than a rename: `workspace_id` (primary key), `provider`, -`repo_owner`, `repo_name`, `project_path`, `branch`, `path`, -`interval_seconds`, `poll_enabled`, `last_synced_at`, `last_ok`, `last_error`, -`last_warnings`, `last_hash`, `created_at`, `updated_at`. +Office's own table: `workspace_id` (primary key), `provider`, `repo_owner`, +`repo_name`, `project_path`, `branch`, `path`, `interval_seconds`, +`poll_enabled`, `last_synced_at`, `last_ok`, `last_error`, `last_warnings`, +`last_hash`, `created_at`, `updated_at`. ### Defaults and the addressable root @@ -432,7 +430,7 @@ provider, and repository for human debugging. Recorded ## Frontend Office gets its own card, -`components/office/settings/office-config-sync-card.tsx`, modelled on +`components/office/settings/office-config-sync-status-card.tsx`, modelled on `components/settings/workflow-sync-status-banner.tsx` but not shared with it. `workflow-sync-status-banner.tsx` is not edited, extracted, or rewrapped. @@ -483,9 +481,8 @@ seam an earlier draft of this design proposed: a shared `internal/reposync` library onto which `internal/workflowsync` would be migrated and Office then built. -**That extraction is deferred, and this design builds Office standalone -instead.** The same note is the reason: it records a **god-object failure** from -a codebase where a `SyncCoordinator` reached 2,506 lines, and warns that the +**This design builds Office standalone instead.** The same note is the reason: +it records a **god-object failure** from a codebase where a `SyncCoordinator` reached 2,506 lines, and warns that the failure begins when a shared coordinator absorbs per-caller variation. The two callers here are already known to diverge on five points, every one of which the shared seam would have had to @@ -499,14 +496,9 @@ differences in a library written before either caller exists is speculative abstraction, which the repository's engineering principles rule out directly. Deferring costs genuine duplication — two poll loops, two config stores, two -status cards — and that cost is accepted rather than argued away. It is bounded -by building Office to the *same* vocabulary (identical column names, the same -`SyncResult` field shape, the same poll-and-record lifecycle) so a later -extraction is a merge of two working implementations rather than a redesign. A -follow-up card carries the extraction and the five collisions above, sequenced -after this ships: doing it first re-imports the coupling this narrowing removed, -and "not worth extracting" is only a defensible verdict once both halves exist -to be compared. +status cards — and that cost is accepted rather than argued away. The extraction +was taken up once both halves shipped, and declined: the five collisions above +are seams, not a library. The vocabulary is no longer held aligned. `synthesis/pbt-from-ears-bridge.md` treats EARS acceptance criteria as **universal properties** - for any input where the trigger holds, the response @@ -554,8 +546,17 @@ in then the settings card. `internal/workflowsync` is not touched at any point, so no step in this order can regress a shipped feature. +## Out of scope + +**Table alignment with `workflow_sync_configs`.** Not held aligned and not +compared here; the three timestamp columns diverge. + ## Related decisions +- [Decline the `internal/reposync` extraction](../../../decisions/2026-09-15-reposync-extraction-declined.md) + is the disposition of the card this design deferred to. It retires three + positions quoted in full there; every sentence stating them is rewritten + above. - [ADR 0031](../../../decisions/0031-office-skill-reference-files.md) defined skill support files and `file_inventory`, which round 2's `references/` request exists to populate.