Push mirroring with on-push fan-out + optional schedule (Fix #623) #624

Merged

crueber merged 2 commits from fix/issue-623 into main

2026-09-16 16:10:04 +00:00

crueber commented

2026-09-16 15:29:13 +00:00

Owner

Decisions: stdlib keygen (no openssh-client in runtime), accept-new without pinned known_hosts, no first-sync on create, schedule OFF default, config+secret sidecars with frozen-list amendment. Full spec: docs/features/13_push_mirror.md.

E2E (scratch stack, file:// upstream, zero auth): push lands upstream refs+objects (ls-remote + clone with history); scheduled daily repo fired via loop; keygen/redaction/failure-recording/~p ETag verified live. Coverage: pushmirror 96.0%, api 95.3%, server 98.4%, store 95.0% (-race green); web 1611 node tests green; vite+esbuild green.

Implements Forgejo #623 end to end: post-hoc push-mirror config (upstream + password/token/SSH-key incl. walhub-generated ed25519 keypair), write-only credentials + scrubbed surfaces, on-push enqueue after landed pushes (new mirror-push-sync task kind), optional preset-cron scheduled sync (default off), git push --mirror transfer, server-publish exclusion (structural), Settings Push mirror tab + badge + SDK, summary push_mirror + ~p ETag. Decisions: stdlib keygen (no openssh-client in runtime), accept-new without pinned known_hosts, no first-sync on create, schedule OFF default, config+secret sidecars with frozen-list amendment. Full spec: docs/features/13_push_mirror.md. E2E (scratch stack, file:// upstream, zero auth): push lands upstream refs+objects (ls-remote + clone with history); scheduled daily repo fired via loop; keygen/redaction/failure-recording/~p ETag verified live. Coverage: pushmirror 96.0%, api 95.3%, server 98.4%, store 95.0% (-race green); web 1611 node tests green; vite+esbuild green.

crueber added 1 commit

2026-09-16 15:29:13 +00:00

Push mirroring with on-push fan-out + optional schedule (Forgejo #623 ) 69c001d548

New package internal/pushmirror (Seam 5 kind mirror-push-sync, Seam 1
repo lanes on both lanes): post-hoc config + secret sidecars
(meta/pushmirror.json Create-once-then-CAS'd, meta/pushmirror-secret.json
CAS'd — frozen-list amendment in 14_extensibility.md), password/token/SSH
auth incl. stdlib ed25519 keygen (no new module/binary; x/crypto/ssh stays
server-transport-only), git push --mirror transfer (04_git.md argv),
Server.OnPush hook from pushPipeline (landed-only, fire-and-forget;
server-side publishes bypass structurally), scheduled loop reusing the
preset-cron model (default off), write-only credentials + scrubbed
surfaces, summary push_mirror + ~p ETag, Settings Push mirror tab + badge
+ SDK (12_web_ui.md). Full decisions in docs/features/13_push_mirror.md.

crueber added 1 commit

2026-09-16 15:46:13 +00:00

Push-mirror review fixes (Forgejo #623 ): scoped refspecs, env-only credentials, keygen/update guards, ETag coverage c91803f684

crueber commented

2026-09-16 15:46:47 +00:00

Author

Owner

Review: PR #624 (Forgejo #623) — verdict: APPROVE with review fixes landed

Reviewed origin/fix/issue-623 (69c001d) in /tmp/walhub-623, hard pass over all 10 areas. Found 1 high + 1 medium-high + 3 small defects, all fixed directly in follow-up commit c91803f (pushed to fix/issue-623). No large rework needed. Remaining notes are accepted-risks/observations, not blockers.

Fix commit `c91803f` (review fixes, 15 files)

[HIGH] R1 — git push --mirror shipped forge-internal refs. refs/pull/N/head is ordinary WAL ref state (published server-side via the WAL funnel, internal/pulls/service.go:438), materialized into the serving copy by every Serve sync — so bare --mirror pushed walhub's own PR heads upstream on every PR-carrying repo (leak, and hosts like GitHub refuse writes to refs/pull/*, which would wedge such repos in permanent "failed"). Transfer is now forced namespace wildcard refspecs + --prune (+refs/heads/*, +refs/tags/* always so emptied namespaces prune; other surviving namespaces when populated; bare two-segment names exact), dropping replace/meta/keep-around always and pull/changes/review/notes by default — the pull direction's FilterRefs discipline reversed (per-namespace wildcards scale; per-ref refspecs would not). Pinned argv + rationale updated in docs/go/04_git.md, 13_push_mirror.md §3, decision (j) appended (law 12; (e) kept, marked superseded-in-shape). New tests: TestPushSkipsInternalRefsAndPrunes, TestPushRefspecsRender.

[MED-HIGH] R2 — shell injection via username in credential helper. credentialArgv interpolated the user-controlled username into the ! shell helper (echo username=<user>); metachars/newlines/$() execute as the server user (import/mirror precedents hardcode x-access-token, so this surface is new here). Both halves now ride child env (WALHUB_PUSHMIRROR_USER/_TOKEN); argv carries only env names. Pinned by TestPushHostileUsernameRidesEnvOnly (fake git binary asserts argv-vs-env separation + no marker execution).

[MED] R3 — keygen bricked non-SSH configs. POST keygen flipped any config to auth_kind=ssh without checking the stored URL scheme (https+ssh fails ValidateTarget at sync time → bricked behind a 200). Now 400 unless upstream is ssh/scp; config untouched. Pinned by TestKeygenRefusesNonSSHUpstream.

[MED] R4 — update wrote secret before validating. Kind-change path SaveSecret'd first, then re-validated kind-vs-URL — a refused update left secret material for a kind the config no longer names. Validation hoisted before any secret write. Pinned by TestUpdateKindMismatchKeepsSecret (fails on old code: hint flips to the new key's).

[SMALL] R5 — ETag missed display fields. pushMirrorHash covered only 6 fields; username/secret-rotation (hint) and repeated identical failures (counter-only change) went stale behind 304. Hash now covers username, has_secret, secret_hint, consecutive_failures; PushMirrorView gains consecutive_failures (additive field, §14.12; mapped in cmd/walhub/pushmirror.go).

Area-by-area (post-fix)

LAW 1 — PASS. go.mod/go.sum diff empty; no x/crypto/ssh client use (only a doc.go comment restating the prohibition); transfer argv pinned in 04_git.md; no package.json change; UI is shared Tailwind classes only (no ui.css diff, no <style>).
SECRETS — PASS. Material only in meta/pushmirror-secret.json; write-only API (presence+last-4); scrubText on runner stderr, fail(), RecordAttempt, HTTP errors, sanitize-on-write; per-fire 0600 key dirs always RemoveAlld via deferred cleanup (incl. write-error paths); http-token refused (ValidateTarget); missing material is a failed outcome via resolveAuth (never silent-anonymous). Upstream URLs cannot smuggle userinfo (NormalizeSource 400s embedded credentials), so task params/notices/views are safe.
TRANSFER — PASS (after R1). Deletion semantics documented (primary, --prune within live namespaces). Ref reconstruction = Serve-sync materialization (pull-refmap reversed). Bulk/control: Runner owns its pool (never the control-plane lane); no LIST (probe-only); on-push enqueue is one exact-key GET off the response (go notify(id) post-report in pushPipeline).
TRIGGER — PASS. Hook fires post-report, landed-only (≥1 ok ref), fire-and-forget, both transports (single funnel). Server-publish exclusion is STRUCTURAL (sync/merge publish via Publish/PublishRefs, never enter pushPipeline) + pinned by TestServerPublishBypassExclusion. Kind mirror-push-sync distinct (own single-flight). Schedule default "" off; preset cron reuse (bundle.ParseSchedule); 1-min loop, own cadence. Note: loop uses blocking SyncNow per due repo — matches the pull-mirror precedent (mirror/sync.go:883); serializing both loops to async is a joint future change, not this PR.
ROUTES/SEAMS — PASS. Repo lanes on both lanes (api+api-browser in Handler.Handle); no top-level twins by design (post-hoc only — the issue's "three twins" note applies to top-level endpoints, of which there are none); ExposedTemplates + api.RegisterExposed from composition (14 §14.12/#272); api.Env.PushMirrorSummary hook (core never imports feature); frozen-list amendment present in 14_extensibility.md:632-653. go list -deps shows only the leaf server/auth types package (same as internal/mirror) — no upward import.
SUMMARY/ETAG — PASS (after R5). push_mirror projection + ~p suffix; pull/push independent (separate sidecars/hooks, TestSummaryPushMirrorIndependentOfPull).
KEYGEN — PASS, verified against real crypto: generated PEM → ssh-keygen -y -f derives the exact public line; ssh-keygen -l fingerprint matches SHA256:… on both key and pub file. (Existing test only did the Go-side round-trip; live verification done in review.)
COVERAGE — PASS. pushmirror 95.7% (-race green); api, server, store, cmd/walhub green incl. -race. Zero pushmirror refs on origin/main (new subsystem — tests fail pre-fix by absence). file:// zero-auth e2e exercised in tests (TestRunPushFileEndToEnd, TestPushFileBare, loop/on-push tests); SSH/HTTPS transfers are shape-covered without network — the PR's live-server claims are manual, plausible, and consistent with the covered shapes.
UI — PASS (code-level). Push-mirror tab reuses Mirror-tab idioms (chip, muted, grid gap-3, flex flex-wrap, pill, shared data-table); status table shows upstream/auth+hint/fingerprint/sync phrasing/last result/next fire; Sync-now + force + removal; SDK carries no credentials. No mobile-specific breakage by construction (wrap/grid, no fixed widths) — but I did NOT drive a browser (no running server in this env); recommend the author attach the desktop+~390px screenshots before merge per AGENTS.md ladder step 8.
TOFU accept-new — FLAGGED, accepted-risk (not blocking). Unpinned hosts use StrictHostKeyChecking=accept-new with the default user known_hosts file: trust lands in ephemeral local disk (law 4 — wiped on restart/redeploy), so each fresh host re-TOFUs with no out-of-band verification surface (no UI shows the accepted host key). MITM window reopens per fresh host. The issue left this to the implementer and decision (i) documents it; for production upstreams recommend pinning known_hosts (field exists). Suggest a follow-up: surface the observed host key/fingerprint in the status view after first sync.

Bottom line

Approve. All acceptance criteria met: post-hoc Settings config (all three auth kinds + keygen), write-only/redacted credentials, on-push enqueue → upstream receives refs+objects (file:// proven in tests), schedule opt-in default-off, status + summary/~p surface, structural publish exclusion, pull/push independence, coverage green. Review fixes in c91803f; only asks: browser screenshots (ladder step 8) + consider the TOFU host-key surface as follow-up.

# Review: PR #624 (Forgejo #623) — verdict: APPROVE with review fixes landed Reviewed `origin/fix/issue-623` (69c001d) in `/tmp/walhub-623`, hard pass over all 10 areas. Found 1 high + 1 medium-high + 3 small defects, all fixed directly in follow-up commit **c91803f** (pushed to `fix/issue-623`). No large rework needed. Remaining notes are accepted-risks/observations, not blockers. ## Fix commit c91803f (review fixes, 15 files) **[HIGH] R1 — `git push --mirror` shipped forge-internal refs.** `refs/pull/N/head` is ordinary WAL ref state (published server-side via the WAL funnel, `internal/pulls/service.go:438`), materialized into the serving copy by every Serve sync — so bare `--mirror` pushed walhub's own PR heads upstream on every PR-carrying repo (leak, and hosts like GitHub refuse writes to `refs/pull/*`, which would wedge such repos in permanent "failed"). Transfer is now forced namespace wildcard refspecs + `--prune` (`+refs/heads/*`, `+refs/tags/*` always so emptied namespaces prune; other surviving namespaces when populated; bare two-segment names exact), dropping `replace/meta/keep-around` always and `pull/changes/review/notes` by default — the pull direction's FilterRefs discipline reversed (per-namespace wildcards scale; per-ref refspecs would not). Pinned argv + rationale updated in `docs/go/04_git.md`, `13_push_mirror.md` §3, decision **(j)** appended (law 12; (e) kept, marked superseded-in-shape). New tests: `TestPushSkipsInternalRefsAndPrunes`, `TestPushRefspecsRender`. **[MED-HIGH] R2 — shell injection via username in credential helper.** `credentialArgv` interpolated the user-controlled username into the `!` shell helper (`echo username=<user>`); metachars/newlines/`$()` execute as the server user (import/mirror precedents hardcode `x-access-token`, so this surface is new here). Both halves now ride child env (`WALHUB_PUSHMIRROR_USER`/`_TOKEN`); argv carries only env names. Pinned by `TestPushHostileUsernameRidesEnvOnly` (fake git binary asserts argv-vs-env separation + no marker execution). **[MED] R3 — keygen bricked non-SSH configs.** `POST keygen` flipped any config to `auth_kind=ssh` without checking the stored URL scheme (https+ssh fails ValidateTarget at sync time → bricked behind a 200). Now 400 unless upstream is ssh/scp; config untouched. Pinned by `TestKeygenRefusesNonSSHUpstream`. **[MED] R4 — update wrote secret before validating.** Kind-change path `SaveSecret`'d first, then re-validated kind-vs-URL — a refused update left secret material for a kind the config no longer names. Validation hoisted before any secret write. Pinned by `TestUpdateKindMismatchKeepsSecret` (fails on old code: hint flips to the new key's). **[SMALL] R5 — ETag missed display fields.** `pushMirrorHash` covered only 6 fields; username/secret-rotation (hint) and repeated identical failures (counter-only change) went stale behind 304. Hash now covers username, has_secret, secret_hint, consecutive_failures; `PushMirrorView` gains `consecutive_failures` (additive field, §14.12; mapped in `cmd/walhub/pushmirror.go`). ## Area-by-area (post-fix) 1. **LAW 1** — PASS. `go.mod`/`go.sum` diff empty; no `x/crypto/ssh` client use (only a doc.go comment restating the prohibition); transfer argv pinned in `04_git.md`; no `package.json` change; UI is shared Tailwind classes only (no `ui.css` diff, no `<style>`). 2. **SECRETS** — PASS. Material only in `meta/pushmirror-secret.json`; write-only API (presence+last-4); `scrubText` on runner stderr, `fail()`, `RecordAttempt`, HTTP errors, sanitize-on-write; per-fire 0600 key dirs always `RemoveAll`d via deferred cleanup (incl. write-error paths); http-token refused (`ValidateTarget`); missing material is a failed outcome via `resolveAuth` (never silent-anonymous). Upstream URLs cannot smuggle userinfo (`NormalizeSource` 400s embedded credentials), so task params/notices/views are safe. 3. **TRANSFER** — PASS (after R1). Deletion semantics documented (primary, `--prune` within live namespaces). Ref reconstruction = Serve-sync materialization (pull-refmap reversed). Bulk/control: Runner owns its pool (never the control-plane lane); no LIST (probe-only); on-push enqueue is one exact-key GET off the response (`go notify(id)` post-report in `pushPipeline`). 4. **TRIGGER** — PASS. Hook fires post-report, landed-only (≥1 ok ref), fire-and-forget, both transports (single funnel). Server-publish exclusion is STRUCTURAL (sync/merge publish via `Publish`/`PublishRefs`, never enter `pushPipeline`) + pinned by `TestServerPublishBypassExclusion`. Kind `mirror-push-sync` distinct (own single-flight). Schedule default `""` off; preset cron reuse (`bundle.ParseSchedule`); 1-min loop, own cadence. Note: loop uses blocking `SyncNow` per due repo — matches the pull-mirror precedent (`mirror/sync.go:883`); serializing both loops to async is a joint future change, not this PR. 5. **ROUTES/SEAMS** — PASS. Repo lanes on both lanes (`api`+`api-browser` in `Handler.Handle`); no top-level twins by design (post-hoc only — the issue's "three twins" note applies to top-level endpoints, of which there are none); `ExposedTemplates` + `api.RegisterExposed` from composition (14 §14.12/#272); `api.Env.PushMirrorSummary` hook (core never imports feature); frozen-list amendment present in `14_extensibility.md:632-653`. `go list -deps` shows only the leaf `server/auth` types package (same as `internal/mirror`) — no upward import. 6. **SUMMARY/ETAG** — PASS (after R5). `push_mirror` projection + `~p` suffix; pull/push independent (separate sidecars/hooks, `TestSummaryPushMirrorIndependentOfPull`). 7. **KEYGEN** — PASS, verified against real crypto: generated PEM → `ssh-keygen -y -f` derives the exact public line; `ssh-keygen -l` fingerprint matches `SHA256:…` on both key and pub file. (Existing test only did the Go-side round-trip; live verification done in review.) 8. **COVERAGE** — PASS. `pushmirror` 95.7% (`-race` green); `api`, `server`, `store`, `cmd/walhub` green incl. `-race`. Zero `pushmirror` refs on `origin/main` (new subsystem — tests fail pre-fix by absence). file:// zero-auth e2e exercised in tests (`TestRunPushFileEndToEnd`, `TestPushFileBare`, loop/on-push tests); SSH/HTTPS transfers are shape-covered without network — the PR's live-server claims are manual, plausible, and consistent with the covered shapes. 9. **UI** — PASS (code-level). Push-mirror tab reuses Mirror-tab idioms (`chip`, `muted`, `grid gap-3`, `flex flex-wrap`, `pill`, shared data-table); status table shows upstream/auth+hint/fingerprint/sync phrasing/last result/next fire; Sync-now + force + removal; SDK carries no credentials. No mobile-specific breakage by construction (wrap/grid, no fixed widths) — but I did NOT drive a browser (no running server in this env); recommend the author attach the desktop+~390px screenshots before merge per AGENTS.md ladder step 8. 10. **TOFU accept-new** — FLAGGED, accepted-risk (not blocking). Unpinned hosts use `StrictHostKeyChecking=accept-new` with the default user known_hosts file: trust lands in ephemeral local disk (law 4 — wiped on restart/redeploy), so each fresh host re-TOFUs with no out-of-band verification surface (no UI shows the accepted host key). MITM window reopens per fresh host. The issue left this to the implementer and decision (i) documents it; for production upstreams recommend pinning `known_hosts` (field exists). Suggest a follow-up: surface the observed host key/fingerprint in the status view after first sync. ## Bottom line Approve. All acceptance criteria met: post-hoc Settings config (all three auth kinds + keygen), write-only/redacted credentials, on-push enqueue → upstream receives refs+objects (file:// proven in tests), schedule opt-in default-off, status + summary/`~p` surface, structural publish exclusion, pull/push independence, coverage green. Review fixes in c91803f; only asks: browser screenshots (ladder step 8) + consider the TOFU host-key surface as follow-up.

crueber referenced this pull request

2026-09-16 15:47:34 +00:00

Push-mirror SSH host keys: persist accept-new trust + surface host-key status #625

crueber commented

2026-09-16 16:09:40 +00:00

Author

Owner

Rendered verification (orchestrator, headless Chromium over CDP against a scratch stack serving this branch): Settings Push-mirror tab at 1280px and 390px, zero console errors on both. Desktop shows upstream/status pill/auth/sync cadence/last synced (relative time)/last result ok, schedule select + username/save, sync-now + force, remove button with explanatory copy. Mobile stacks with no page overflow (tab strip scrolls internally per .repo-tabs). Live on-push sync also confirmed end to end on the rig (push to walhub arrived in a file:// upstream bare repo; status flipped to ok). One investigation note: an apparent frozen last_synced_at during the run turned out to be second-precision truncation on a sub-second sync plus a git-log-on-unborn-master misread — server log confirms push landed and sync stamped in the same second; no defect.

crueber merged commit dba49a8991 into main

2026-09-16 16:10:04 +00:00

crueber referenced this pull request from a commit

2026-09-16 16:10:05 +00:00

Merge pull request 'Push mirroring with on-push fan-out + optional schedule (Fix #623)' (#624) from fix/issue-623 into main

crueber referenced this pull request

2026-09-16 16:10:14 +00:00

Push mirroring: push repos to an upstream (password/token/SSH-key auth, keypair generation) with on-push + optional scheduled sync #623