Skip to content

Reuse DuckDB path for Quack sync#930

Open
cpcloud wants to merge 8 commits into
mainfrom
codex/standalone-quack-sync
Open

Reuse DuckDB path for Quack sync#930
cpcloud wants to merge 8 commits into
mainfrom
codex/standalone-quack-sync

Conversation

@cpcloud

@cpcloud cpcloud commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Quack sync now uses the existing DuckDB configuration and command/API paths instead of a parallel Quack config surface.

AGENTSVIEW_DUCKDB_URL and AGENTSVIEW_DUCKDB_TOKEN drive remote Quack pushes through agentsview duckdb push, and duckdb push --watch preserves continuous sync on the same backend path. The daemon route remains /api/v1/push/duckdb; there is no separate /push/quack, [quack], or AGENTSVIEW_QUACK_* path.

Remote URL-backed DuckDB/Quack pushes now use a per-target sync-state scope derived from a non-secret URL fingerprint, so switching remotes does not reuse another target's duckdb_last_push_at watermark. Local DuckDB file mirrors keep the historical unscoped watermark for compatibility, and duckdb status reads whichever scope matches the configured target.

Quack-specific code remains only at the protocol boundary: DuckDB remote attach/opening and agentsview duckdb quack serve, which starts DuckDB quack_serve. Serve startup projects quack_serve results down to listen fields and all serve startup paths now report auth state without printing bearer token values, because stdout and service-manager output can become logs. duckdb quack serve requires an explicit token from --token, AGENTSVIEW_DUCKDB_TOKEN, or [duckdb].token rather than generating a hidden or persisted token; Quack URL redaction also strips URL userinfo and secret-like query values before attach errors can print them.

duckdb status also uses the config-backed DuckDB/Quack opener while preserving the existing machine-scoped DuckDB session and message counts, so shared mirrors and remote Quack endpoints do not report global totals for the local machine view.

The tradeoff is that Quack is treated as DuckDB remote transport rather than a new storage backend. That keeps query behavior aligned with the DuckDB mirror while still supporting remote Quack push/read workflows through the existing DuckDB surface.

@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (63e8c07)

Medium-severity issue found: duckdb push may now target the configured remote read URL instead of the local mirror file.

Medium

  • cmd/agentsview/archive_write_backend.go:409
    DuckDBPush now uses duckdbsync.NewFromConfig, which prefers DuckDBConfig.URL over Path. Since [duckdb].url / AGENTSVIEW_DUCKDB_URL is documented as the remote read endpoint for duckdb serve, users with that configured may have agentsview duckdb push write to Quack or fail on Quack auth instead of updating the local DuckDB mirror file.
    Fix: Keep the DuckDB push path opening duckCfg.Path explicitly, and reserve URL-aware NewFromConfig for the new Quack push path or require an explicit remote-push mode.

Panel: ci_default_security | Synthesis: codex, 13s | Members: codex_default (codex/default, done, 7m40s), codex_security (codex/security, done, 3m9s) | Total: 11m2s

@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (89e4a5d)

Verdict: Two medium issues need attention before merge.

Medium

  • internal/server/huma_routes_push.go:163
    /api/v1/push/duckdb now uses duckdbsync.NewFromConfig, which honors DuckDBConfig.URL. The daemon client posts the resolved DuckDB config, so agentsview duckdb push will push to a configured Quack URL instead of the local DuckDB path whenever a writable daemon is used, while the local backend explicitly ignores that URL.
    Fix: Keep the DuckDB push route local-only by using duckdbsync.New(duckCfg.Path, local, duckCfg.MachineName, ...) or by clearing duckCfg.URL before constructing the syncer. Reserve NewFromConfig for the Quack route and add daemon-path coverage.

  • internal/server/huma_routes_push.go:222
    /api/v1/push/quack relies on the request body to provide sync_state_target; if omitted, Quack pushes use the unscoped DuckDB watermark/fingerprint keys and can advance or clear the local DuckDB mirror state, causing later DuckDB pushes to skip data.
    Fix: Force or default the Quack push handler’s SyncStateTarget to the Quack scope instead of accepting an empty value from the request.


Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 5m25s), codex_security (codex/security, done, 4m38s) | Total: 10m13s

@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (1ae76d1)

No issues found.


Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 13m4s), codex_security (codex/security, done, 3m42s) | Total: 16m46s

@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (140f31b)

Summary verdict: Changes are mostly clean, with two medium operational regressions to address.

Medium

  • cmd/agentsview/duckdb.go:356
    When duckdb quack serve auto-generates a token, the token is no longer printed or persisted, so the advertised default “generated if omitted” mode starts an authenticated server that clients cannot attach to.
    Fix: Require a configured/flag token when tokens must not be printed, or persist the generated token somewhere private such as a 0600 file/config entry and print only that location.

  • cmd/agentsview/quack.go:127
    quack push --watch only cancels on os.Interrupt; a normal SIGTERM kills the process immediately, bypassing the watch loop’s shutdown flush and cleanup.
    Fix: Include syscall.SIGTERM in the watch context, matching the existing long-running watch/serve commands.


Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 5m42s), codex_security (codex/security, done, 6m48s) | Total: 12m38s

cpcloud added 4 commits June 30, 2026 13:35
Quack now has its own config, environment variables, CLI command group, daemon push route, watch path, and read selector instead of being inferred from PostgreSQL targets. PG sync keeps the PostgreSQL-only URL/schema/machine behavior, while Quack maps to the existing DuckDB backend only at the Quack boundary and derives the internal machine label from the host.

This keeps PG and DuckDB behavior separate while making Quack usable where a read-only mirror or push sync backend is expected.
The standalone Quack path made the shared DuckDB push helper URL-aware so Quack could push to a remote endpoint. That also changed regular duckdb push when users had [duckdb].url configured for remote reads, because the helper preferred URL over Path and could try to attach Quack instead of updating the local mirror file.

Keep DuckDBPush on the explicit local file opener and reserve URL-aware opening for the Quack push path. A regression test covers configs that include both a local DuckDB path and a remote read URL.
The HTTP push daemon still used URL-aware DuckDB construction for duckdb push, so a posted DuckDB config with a Quack read URL could make the daemon target the remote endpoint while the local CLI backend correctly wrote the local mirror file. The daemon route now opens the posted DuckDB path explicitly, preserving existing duckdb push behavior across direct and daemon modes.

Quack push also now owns its sync-state scope at the handler boundary instead of accepting an empty or caller-provided request value. This keeps Quack watermarks separate from the local DuckDB mirror state while preserving project filter handling.
Quack exposes auth_token in the CALL quack_serve result, and SQL-printing launch paths can dump that table into logs. agentsview now starts Quack with the table-function form projected to listen_uri and listen_url, keeping the token as an input argument only instead of requesting it back in the result set.

The DuckDB Quack startup summary also stops printing generated token values and reports only whether the token was generated or configured. The Quack smoke path uses the same non-secret projection, and an fd-level integration test covers process stdout and stderr during startup.
@cpcloud cpcloud force-pushed the codex/standalone-quack-sync branch from 140f31b to e7cbb0c Compare June 30, 2026 17:42
@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (e7cbb0c)

Medium issue found: the auto-generated Quack serve token is not retrievable by clients.

Medium

  • cmd/agentsview/duckdb.go:356
    When duckdb quack serve auto-generates a token, the startup output only says Token: generated, so the default command starts an authenticated Quack server that clients cannot actually attach to because the required token is not retrievable.

    Suggested fix: Require an explicit/configured token for this command, or persist the generated token to a protected local file and tell the user where to retrieve it without printing the secret directly.


Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 5m47s), codex_security (codex/security, done, 3m22s) | Total: 9m15s

Quack sync should not introduce a parallel config, command, or daemon API when the existing DuckDB backend already owns local-file and remote-URL targets. This collapses the branch-added Quack config and push/read surfaces into DuckDBConfig, so AGENTSVIEW_DUCKDB_URL and AGENTSVIEW_DUCKDB_TOKEN drive remote Quack pushes through the normal duckdb push route.

Continuous sync is preserved as duckdb push --watch rather than a top-level quack command, and DuckDB status now opens URL-backed stores through the same config path. Startup output for serve modes now reports that auth is configured without printing bearer token values, because those streams can land in logs.
@cpcloud cpcloud changed the title Add standalone Quack sync path Reuse DuckDB path for Quack sync Jun 30, 2026
@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (b2995f6)

Summary verdict: One medium issue found; security review found no additional medium-or-higher issues.

Medium

  • cmd/agentsview/duckdb.go:379 - duckdb quack serve still generates a token when none is configured, but the generated value is neither printed nor persisted. The default command starts an authenticated Quack server that clients cannot connect to because there is no retrievable token.

    Fix: Either require --token or configured duckdb.token when tokens must not be printed, or persist the generated token somewhere documented and retrievable.


Panel: ci_default_security | Synthesis: codex, 13s | Members: codex_default (codex/default, done, 5m24s), codex_security (codex/security, done, 1m38s) | Total: 7m15s

DuckDB status was moved onto the config-backed store opener so URL-backed Quack endpoints could be queried without a local sync handle. That path accidentally counted every mirrored machine, while the existing DuckDB status output had always reported this machine's session and message counts. Shared mirror files and remote Quack endpoints could therefore show global totals in a per-machine status view.

Keep the config-backed status reader on the same machine-scoped queries as Sync.Status, while still allowing the URL-backed store opener to handle remote Quack endpoints. The regression test seeds another machine into the mirror and verifies the status reader does not include it.
@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (6bc8d81)

Summary verdict: changes have two Medium issues; no High or Critical findings were reported.

Medium

  • cmd/agentsview/duckdb.go:378 - When duckdb quack serve generates a token, the token is neither printed nor persisted. The server starts with an unknown token, so a client cannot attach unless the user supplied a token up front.
    Fix: Require --token/configured token when output must not reveal secrets, or persist the generated token somewhere documented and retrievable.

  • cmd/agentsview/archive_write_backend.go:124 - DuckDB/Quack pushes always use the unscoped DuckDB sync state, so a remote Quack target can reuse a watermark from a different local DuckDB target. If that remote target already has some rows but is stale or partial, incremental push can skip older missing sessions.
    Fix: Derive and pass a stable DuckDB target scope from the destination path/URL, and use that same scope for push/status watermarks.


Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 5m28s), codex_security (codex/security, done, 3m9s) | Total: 8m44s

Starting a Quack server with an auto-generated token is not useful once startup output stops printing secrets, because clients have no safe way to discover that token. Persisting the generated value would also create another durable secret sink just to recover from a logging concern.

Require the existing DuckDB token path instead: --token, AGENTSVIEW_DUCKDB_TOKEN, or [duckdb].token. Startup output continues to report only that a token is configured, so systemd or terminal logs do not receive bearer-token material.
@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (d60dbea)

Remote DuckDB pushes have one medium correctness issue; no high or critical findings.

Medium

  • cmd/agentsview/archive_write_backend.go:364 - DuckDB pushes always pass an empty sync-state target, including the new remote Quack URL path. Since Sync.Push reads the shared duckdb_last_push_at watermark, switching from one DuckDB target to another non-empty but stale target can silently skip unchanged sessions that were pushed to the previous target but are missing from the current one.
    • Fix: Derive a stable target scope/fingerprint from the DuckDB target, at least for duckCfg.URL and ideally path/URL without token, and pass it through SyncStateTarget for both local and daemon pushes. Use the same scope when reporting status.

Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 5m22s), codex_security (codex/security, done, 3m38s) | Total: 9m10s

Remote Quack pushes reuse the DuckDB sync watermark, so different URL-backed targets could share duckdb_last_push_at. Switching targets after pushing another remote could skip unchanged sessions missing from the new target.

Derive a non-secret sync-state scope for URL-backed DuckDB targets and pass it through local, daemon, and server push paths. Local file mirrors keep the historical unscoped key for compatibility, and duckdb status reads the same scoped watermark.

Keep the adjacent token handling explicit: Quack serve help no longer promises generated tokens, and Quack URL redaction now strips URL userinfo plus secret-like query values before errors can print them.
@roborev-ci

roborev-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown

roborev: Combined Review (0e1669d)

Medium issue found.

Medium

  • cmd/agentsview/duckdb.go:58 - duckdb push --watch only handles os.Interrupt. A SIGTERM from systemd, Docker, or another service manager can terminate the process before the watch loop performs its shutdown flush, unlike pg push --watch.
    • Fix: include syscall.SIGTERM in the signal.NotifyContext used for watch runs.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 7m37s), codex_security (codex/security, done, 3m51s) | Total: 11m34s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant