From 9e5af895ef61a8f05b5dbe8f1605f429309aebf8 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Thu, 17 Sep 2026 15:43:48 +0200 Subject: [PATCH 1/2] fix: wire sync's -project flag to actually project onto the board (#159) Sync's per-provider loop pushed the issue but never called ProjectOntoBoard, so -project (and openspec/specsync.yml's board target) reported BoardConfigured: true while silently doing nothing to the board. Restores the call, mirroring pull.go's existing usage, and reports the plan through ItemResult.Board so -dry-run finally previews it. A projection error is now a non-fatal stderr warning instead of being swallowed. --- .../wire-sync-board-projection/proposal.md | 86 +++++++++ .../wire-sync-board-projection/tasks.md | 55 ++++++ sync.go | 14 ++ sync_test.go | 169 ++++++++++++++++++ 4 files changed, 324 insertions(+) create mode 100644 openspec/changes/wire-sync-board-projection/proposal.md create mode 100644 openspec/changes/wire-sync-board-projection/tasks.md create mode 100644 sync_test.go diff --git a/openspec/changes/wire-sync-board-projection/proposal.md b/openspec/changes/wire-sync-board-projection/proposal.md new file mode 100644 index 0000000..4291405 --- /dev/null +++ b/openspec/changes/wire-sync-board-projection/proposal.md @@ -0,0 +1,86 @@ +# Wire sync's -project flag to actually project onto the board + +## Why + +`sync -project owner/N` (or `openspec/specsync.yml`) resolves a +`BoardTarget`, threads it through `Options.Project`, and `Sync` reports +`ItemResult.BoardConfigured` from it — but never calls `ProjectOntoBoard`. +Only `pull` does (`pull.go:134`). The result: `sync -project` runs clean, +reports success, `BoardConfigured: true`, and silently does nothing to the +board. `printBoardPlan` (`cmd/specsync/main.go:887`) prints nothing useful +either, since it's handed a permanently-zero-value `BoardPlan` — so there's +no error, no warning, just quiet non-function exactly matching the +documented, expected behavior in shape (`BoardConfigured` says true) while +doing none of it. + +**This is a regression, not a gap.** `git log -S ProjectOntoBoard -- sync.go` +shows `Sync` used to call it, inline in a per-provider `syncOne` helper, with +a hand-rolled three-way-merge dance (query current board state, compare +against a saved binding, decide push-local vs. respect-human-move vs. +report-conflict) before deciding whether to call `ProjectOntoBoard` at all. +Commit `4177dc7` ("multi-provider sync (fan-out)") restructured `Sync` into +its current per-provider loop and dropped that whole block — `Board:` and +the `ProjectOntoBoard` call vanished from the diff; `BoardConfigured: +opts.Project.Configured()` and the `Board BoardPlan` field on `ItemResult` +were left behind, reporting stale/empty data ever since. No test caught it: +`board_test.go` only calls `ProjectOntoBoard` directly, never through +`Sync`; `TestBoardTargetCarriesStatusMapping` (added for an earlier, +narrower version of this same class of bug — `BoardTarget.StatusMapping` +reaching the CLI) only checks target construction, not that projection runs. + +**The fix is smaller than the regression, though.** `ProjectOntoBoard` +itself (`board.go:98`) has been rewritten since the fan-out refactor into a +fully self-contained operation: it resolves the board schema, detects human +moves in both directions (`HumanMovedToDone`/`HumanMovedToActive`), +protects against clobbering a human-set Status via +`LastWrittenOptionID`-based reasoning, and saves its own board binding +(`changeDir` param) — internally, on every call. None of the external +three-way-merge machinery the old `syncOne` had to hand-roll is needed +anymore; `pull.go` already proves this by calling `ProjectOntoBoard` with a +single line. Restoring the call to `Sync` is that same single line, not a +rebuild of the old logic. + +## Proposed Changes + +- In `Sync`'s per-provider loop (`sync.go`), after `Push` succeeds, call + `bp.ProjectOntoBoard(ctx, opts.Project, ref, item, opts.DryRun, c.Dir)` + when `opts.Project.Configured()` and the provider implements + `BoardProjector` — mirroring `pull.go`'s existing call exactly. It already + handles dry-run internally (a zero-API preview), so no extra dry-run + branching is needed here. +- Capture the first successful plan into `ItemResult.Board` (first-provider- + wins, matching the existing `firstRef`/`firstCreated` pattern for + URL/created reporting) so `printBoardPlan` finally has real data. +- A `ProjectOntoBoard` error is non-fatal to the change's sync (the issue + itself already pushed successfully) but printed as a warning to stderr — + silence is exactly the failure mode this proposal exists to remove, so a + new failure path here must not repeat it. + +## Non-Goals + +- **Not** touching `DepSync`'s existing error handling + (`sync.go`, dependency-edge reconcile) even though it has the same + silent-swallow shape (`if _, err := DepSync(...); err != nil { /* Log and + continue */ }` — no logging actually happens). Same bug family, separate + fix, separate PR. +- **Not** reintroducing the old inline three-way-merge dance from before + the fan-out refactor — superseded by `ProjectOntoBoard`'s own internal + handling, per the Why section. +- **Not** per-provider board reporting in `ProviderResult` for multi-board + fan-out (multiple providers simultaneously implementing `BoardProjector`) + — first-provider-wins is enough today since GitHub is the only + `BoardProjector` implementation that exists. + +## Open Questions + +1. Should a `ProjectOntoBoard` failure also skip `DepSync` and any later + post-push step for that provider in the same run, or are they + independent enough to both attempt regardless? Leaning independent — + they touch unrelated state (board vs. issue cross-links). + +## Release Notes + +Fixed: `sync -project` (and `openspec/specsync.yml`'s board target) now +actually projects issues onto the configured GitHub Projects board — it +previously reported `BoardConfigured: true` and silently did nothing. +`pull` was unaffected; this restores parity for `sync`. diff --git a/openspec/changes/wire-sync-board-projection/tasks.md b/openspec/changes/wire-sync-board-projection/tasks.md new file mode 100644 index 0000000..3a1b3ea --- /dev/null +++ b/openspec/changes/wire-sync-board-projection/tasks.md @@ -0,0 +1,55 @@ +# Tasks + +- [x] 1. In `Sync`'s per-provider loop (`sync.go`), after `ref, perr := + prov.Push(...)` succeeds, add: when `opts.Project.Configured()` and + `prov` implements `BoardProjector`, call + `bp.ProjectOntoBoard(ctx, opts.Project, ref, item, opts.DryRun, c.Dir)`. + Track the first successful plan into a `boardPlan BoardPlan` local + (mirroring the existing `firstRef`/`firstCreated` pattern), and set + `ItemResult.Board: boardPlan` in the `res.Items = append(...)` call + that already sets `BoardConfigured`. + Validation: a fixture `Sync` call with a configured `BoardTarget` + against a `BoardProjector`-implementing fake provider results in a + `ProjectOntoBoard` call being made (mirrors `pull_test.go`'s existing + coverage of the same call via `Pull`) and `ItemResult.Board` non-zero. + +- [x] 2. A `ProjectOntoBoard` error is non-fatal: print + `fmt.Fprintf(os.Stderr, "specsync: warning: board projection for %s + failed: %v\n", c.Slug, err)` and continue the rest of that provider's + post-push steps (dependency sync, etc.) rather than `continue`-ing + past them. Add the `os` import to `sync.go`. + Validation: a fixture where the fake `BoardProjector` returns an error + still completes the sync (issue created/updated, no early exit) and + the warning is observable (captured stderr, or the function signature + exposes it for the test — match whatever pattern existing warnings in + this codebase use, e.g. `pull.go`'s intake-transition warning). + +- [x] 3. Regression test proving the specific regression this proposal + describes: `Sync` with `Options.Project` configured against a + `BoardProjector` fake provider must result in exactly one + `ProjectOntoBoard` call per change per provider — not zero (the bug), + not more than one. + +- [x] 4. Dry-run coverage: `Sync` with `DryRun: true` and a configured board + target still calls `ProjectOntoBoard` (it handles dry-run internally, + zero API calls) and the resulting `ItemResult.Board` is non-zero, so + `-dry-run` output actually previews the board plan instead of the CLI + printing nothing under "board:". + +- [x] 5. Manual/live verification: ran `specsync -dry-run -change + wire-sync-board-projection -project androidand/2` against a real + GitHub Projects board with both the unpatched and patched binary. + Unpatched: only the static "would ensure the issue is on the board" + line printed (StatusName/AssigneeLogin always empty). Patched: all + three preview lines printed (add/status/assignee), confirming the + exact silent-no-op this proposal describes and its fix. Did not + perform a real (non-dry-run) write against that board — it is an + unrelated project (BrickNow Prioritization), not a low-stakes test + board for this repo, so a live mutation was skipped pending a real + target. + +- [x] 6. No documentation change needed for accuracy — `README.md:592-599` + ("sync + project onto board 6") and `site/features.json`'s "Projects + board sync" entry already correctly describe the intended behavior; + this proposal makes reality match what's already documented, not the + other way round. diff --git a/sync.go b/sync.go index a3e262e..5c45808 100644 --- a/sync.go +++ b/sync.go @@ -3,6 +3,7 @@ package specsync import ( "context" "fmt" + "os" "strings" ) @@ -175,6 +176,7 @@ func Sync(ctx context.Context, opts Options) (Result, error) { var firstRef Ref var firstCreated bool var firstClosedDeferred string + var boardPlan BoardPlan providerResults := make([]ProviderResult, 0, len(providers)) for _, prov := range providers { @@ -260,6 +262,17 @@ func Sync(ctx context.Context, opts Options) (Result, error) { firstClosedDeferred = ref.ClosedDeferred } + if opts.Project.Configured() { + if bp, ok := prov.(BoardProjector); ok { + plan, berr := bp.ProjectOntoBoard(ctx, opts.Project, ref, item, opts.DryRun, c.Dir) + if berr != nil { + fmt.Fprintf(os.Stderr, "specsync: warning: board projection for %s failed: %v\n", c.Slug, berr) + } else if boardPlan == (BoardPlan{}) { + boardPlan = plan + } + } + } + if opts.DryRun { // A dry run must never mutate local state. } else { @@ -344,6 +357,7 @@ func Sync(ctx context.Context, opts Options) (Result, error) { Flips: allFlips, TitleSuggestion: suggestion, BoardConfigured: opts.Project.Configured(), + Board: boardPlan, ClosedDeferred: firstClosedDeferred, Providers: providerResults, }) diff --git a/sync_test.go b/sync_test.go new file mode 100644 index 0000000..90d095f --- /dev/null +++ b/sync_test.go @@ -0,0 +1,169 @@ +package specsync + +import ( + "bytes" + "context" + "os" + "path/filepath" + "testing" +) + +// boardCapturingProvider is a fake WorkProvider that also implements +// BoardProjector, so Sync's board-projection call can be observed without a +// real GitHub round-trip. +type boardCapturingProvider struct { + ref Ref + plan BoardPlan + err error + calls int + lastItem WorkItem +} + +func (p *boardCapturingProvider) Name() string { return "github" } +func (p *boardCapturingProvider) Push(context.Context, WorkItem, *Ref) (Ref, error) { + return p.ref, nil +} +func (p *boardCapturingProvider) Find(context.Context, string) (*Ref, error) { return nil, nil } + +func (p *boardCapturingProvider) ProjectOntoBoard(_ context.Context, _ BoardTarget, _ Ref, item WorkItem, _ bool, _ string) (BoardPlan, error) { + p.calls++ + p.lastItem = item + return p.plan, p.err +} + +func configuredTarget() BoardTarget { + return BoardTarget{Owner: "acme", Number: 1} +} + +func TestSyncProjectsOntoBoardWhenConfigured(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "changes", "c1", "proposal.md"), "# C1\n") + prov := &boardCapturingProvider{ + ref: Ref{Provider: "github", ID: "1", URL: "https://example.test/1"}, + plan: BoardPlan{ProjectID: "PVT_1", AddedToBoard: true}, + } + + res, err := Sync(context.Background(), Options{OpenSpecDir: root, Provider: prov, Project: configuredTarget()}) + if err != nil { + t.Fatalf("Sync: %v", err) + } + if prov.calls != 1 { + t.Fatalf("expected exactly 1 ProjectOntoBoard call, got %d", prov.calls) + } + if len(res.Items) != 1 { + t.Fatalf("expected 1 item, got %d", len(res.Items)) + } + item := res.Items[0] + if !item.BoardConfigured { + t.Error("expected BoardConfigured true") + } + if item.Board == (BoardPlan{}) { + t.Error("expected ItemResult.Board to carry the projected plan, got zero value") + } + if item.Board.ProjectID != "PVT_1" { + t.Errorf("Board.ProjectID = %q, want PVT_1", item.Board.ProjectID) + } +} + +func TestSyncSkipsBoardProjectionWhenUnconfigured(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "changes", "c1", "proposal.md"), "# C1\n") + prov := &boardCapturingProvider{ref: Ref{Provider: "github", ID: "1"}} + + res, err := Sync(context.Background(), Options{OpenSpecDir: root, Provider: prov}) + if err != nil { + t.Fatalf("Sync: %v", err) + } + if prov.calls != 0 { + t.Fatalf("expected no ProjectOntoBoard calls when Project is unconfigured, got %d", prov.calls) + } + if res.Items[0].BoardConfigured { + t.Error("expected BoardConfigured false") + } +} + +func TestSyncBoardProjectionErrorIsNonFatal(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "changes", "c1", "proposal.md"), "# C1\n") + prov := &boardCapturingProvider{ + ref: Ref{Provider: "github", ID: "1", URL: "https://example.test/1"}, + err: os.ErrInvalid, + } + + var stderr bytes.Buffer + restore := redirectStderr(t, &stderr) + defer restore() + + res, err := Sync(context.Background(), Options{OpenSpecDir: root, Provider: prov, Project: configuredTarget()}) + if err != nil { + t.Fatalf("Sync must not fail when board projection errors: %v", err) + } + if res.Updated != 1 && res.Created != 1 { + t.Fatalf("expected the issue push itself to still count as created/updated, got %+v", res) + } + if len(res.Items) != 1 || res.Items[0].URL == "" { + t.Fatalf("expected the change to still be pushed despite the board error: %+v", res.Items) + } + restore() + if !bytes.Contains(stderr.Bytes(), []byte("board projection for c1 failed")) { + t.Errorf("expected a warning naming the change on stderr, got: %s", stderr.String()) + } +} + +func TestSyncDryRunStillProjectsOntoBoard(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "changes", "c1", "proposal.md"), "# C1\n") + prov := &boardCapturingProvider{ + ref: Ref{Provider: "github", ID: "0"}, + plan: BoardPlan{ProjectID: "PVT_1", AddedToBoard: true}, + } + + res, err := Sync(context.Background(), Options{OpenSpecDir: root, Provider: prov, Project: configuredTarget(), DryRun: true}) + if err != nil { + t.Fatalf("dry-run Sync: %v", err) + } + if prov.calls != 1 { + t.Fatalf("expected ProjectOntoBoard to run under -dry-run (it handles dry-run internally), got %d calls", prov.calls) + } + if res.Items[0].Board == (BoardPlan{}) { + t.Error("expected -dry-run to preview a non-zero board plan") + } +} + +// redirectStderr temporarily replaces os.Stderr so a test can capture warnings +// printed via fmt.Fprintf(os.Stderr, ...). Returns a restore func; safe to +// call more than once. +func redirectStderr(t *testing.T, into *bytes.Buffer) func() { + t.Helper() + orig := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = w + done := make(chan struct{}) + go func() { + buf := make([]byte, 4096) + for { + n, rerr := r.Read(buf) + if n > 0 { + into.Write(buf[:n]) + } + if rerr != nil { + close(done) + return + } + } + }() + restored := false + return func() { + if restored { + return + } + restored = true + os.Stderr = orig + w.Close() + <-done + r.Close() + } +} From 9325fa534f118bcc882f1fce5dadd9449f02033c Mon Sep 17 00:00:00 2001 From: AndreasS Date: Thu, 17 Sep 2026 16:20:51 +0200 Subject: [PATCH 2/2] test: fix TestPull_CannotResolveFromBranch's branch-name isolation (#159) Relied on whatever branch the test suite happened to run from instead of overriding currentBranchFn like every other branch-resolution test, so it failed whenever run from a feat/--named branch (i.e. this branch). --- pull_test.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/pull_test.go b/pull_test.go index a7dd222..5388dcc 100644 --- a/pull_test.go +++ b/pull_test.go @@ -644,12 +644,24 @@ func TestPull_RequiresIssueForNonGithub(t *testing.T) { type stubNonGithubProvider struct{} -func (s *stubNonGithubProvider) Name() string { return "beads" } -func (s *stubNonGithubProvider) Find(context.Context, string) (*Ref, error) { return nil, nil } -func (s *stubNonGithubProvider) Push(context.Context, WorkItem, *Ref) (Ref, error) { return Ref{}, nil } -func (s *stubNonGithubProvider) Get(context.Context, string) (FetchedItem, error) { return FetchedItem{}, nil } +func (s *stubNonGithubProvider) Name() string { return "beads" } +func (s *stubNonGithubProvider) Find(context.Context, string) (*Ref, error) { return nil, nil } +func (s *stubNonGithubProvider) Push(context.Context, WorkItem, *Ref) (Ref, error) { return Ref{}, nil } +func (s *stubNonGithubProvider) Get(context.Context, string) (FetchedItem, error) { + return FetchedItem{}, nil +} func TestPull_CannotResolveFromBranch(t *testing.T) { + // Force a branch name that never matches the feat/- pattern, + // regardless of what branch this test suite actually happens to be + // checked out on (e.g. a real feat/- branch, which would + // otherwise make this test try to resolve issue for real and fail + // with an unrelated JSON-parse error instead of the branch-mismatch + // error it's meant to assert on). + prev := currentBranchFn + currentBranchFn = func() (string, error) { return "main", nil } + defer func() { currentBranchFn = prev }() + dir := t.TempDir() // On a branch that doesn't match the pattern, Pull should error. _, err := Pull(context.Background(), PullOptions{