Add a push store gate by dacorvo · Pull Request #44 · huggingface/funes

dacorvo · 2026-07-01T16:26:34Z

This makes it harder to push to the wrong remote store.

`funes push` now takes an optional `[store]` target, falling back to the active remote as before. Before uploading, it checks whether the local index shares any chunk id with the target. Chunk ids are deterministic transcript coordinates, so a non-empty overlap means "you've contributed here before"; the overlap is already computed as part of the push delta, so the check is free. A share-nothing target — a first publish, a new host of yours, or the wrong store — is gated behind a y/N confirmation (default no). `--yes` skips it; off a terminal the push fails closed rather than guess, so an unattended run can't silently publish to the wrong Hub dataset. Guards against inadvertently leaking private memories to a store you never meant to write to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Extend the gated round-trip so it also exercises the push gate against the real remote — the two overlap states it distinguishes are exactly what the round-trip already walks: - declining a first publish (empty remote, nothing shared) must abort and upload nothing — asserted with a declining Confirm plus a follow-up store_ids showing the remote is still empty; - an append to a store that already shares chunks must not prompt at all — a declining Confirm is passed and asserted never consulted. Verified live: decline leaves the remote untouched, accept publishes, and the overlapping append proceeds without a prompt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Adds a confirmation “no-overlap” gate to push to reduce the risk of publishing a local index to an unintended remote store, including CLI wiring (--yes) and test coverage for the gate’s behavior.

Changes:

Introduce push::Confirm and a must_confirm gate that aborts publishing when the remote shares no chunks unless explicitly confirmed.
Extend the CLI push command to optionally target a specific store and to bypass confirmation via --yes, with a fail-closed interactive prompt.
Update the live push round-trip integration test and README to cover/describe the new gate behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/push_round_trip.rs	Adds assertions for declining/accepting the no-overlap confirmation gate and ensuring append does not prompt.
src/push.rs	Adds `Confirm`, `must_confirm`, and gates publish operations before uploads when overlap is empty.
src/main.rs	Wires CLI options (`[store]`, `--yes`) and adds an interactive, fail-closed confirmation prompt.
README.md	Documents new `push` behavior (optional target store, confirmation gate, `--yes`, fail-closed off-terminal).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    // Guard against publishing to a store this index shares nothing with — a first push here, a new
+    // host of yours, or the wrong store. The overlap is already in hand (see [`must_confirm`]), so
+    // this costs no extra round-trip. Only the real-upload paths reach here; the nothing-to-push
+    // paths returned above.
+    if must_confirm(local_ids.len(), to_push.len()) && !confirm.proceed(&target.label(), to_push.len()) {


+    let declined = funes::push::run_push(Store::parse(&uri), false, Confirm::Ask(decline)).await;
+    let after_decline = funes::push::store_ids(&Store::parse(&uri)).await;


…ne via a Hub query Addresses PR #44 review feedback: - push: resolve parse_hf and the HF token *before* the no-overlap confirmation, so a bad URI or a missing token fails immediately rather than after the user has answered "publish anyway?" for a push that could never proceed. - test: replace the error-swallowing store_ids check with repo.get_paths_info over the scratch prefix. A successful Ok([]) proves nothing was published; a transport failure is Err and fails the test loudly. store_ids returned empty on any open error — which after a declined first publish is the (expected) not-found path — making the old assertion near-tautological and open to transient false positives. Verified live: decline publishes nothing, accept publishes, and the overlapping append proceeds without a prompt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

dacorvo

The comments are overly descriptive, and not contextual enough. They leak the whole imploementaiton details although they are inserted at different nested levels of abstraction.

dacorvo · 2026-07-02T06:43:35Z

+    /// Publish the local index's new chunks to a remote store on the HF Hub — the active remote by
+    /// default, or `[store]` to publish elsewhere. `index` only writes locally, so this is the one
+    /// step that uploads. A publish to a store your index shares no chunks with (a first push, a new
+    /// host, or the wrong store) asks for confirmation first; `--yes` skips it.


Suggested change

/// Publish the local index's new chunks to a remote store on the HF Hub — the active remote by

/// default, or `[store]` to publish elsewhere. `index` only writes locally, so this is the one

/// step that uploads. A publish to a store your index shares no chunks with (a first push, a new

/// host, or the wrong store) asks for confirmation first; `--yes` skips it.

/// Publish the local index's new chunks to a remote store on the HF Hub — the active remote by

/// default, or `[store]` to publish elsewhere.

dacorvo · 2026-07-02T06:45:21Z

+            yes,
+            force_reindex,
+        } => {
+            let target = match store {


Suggested change

let target = match store {

let remote = match store {

dacorvo · 2026-07-02T06:45:33Z

                }
                Err(e) if push::is_read_only(&e) => Err(anyhow!(
-                    "{remote} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"
+                    "{target} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"


Suggested change

"{target} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"

"{remote} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"

dacorvo · 2026-07-02T06:48:00Z

+/// How a push handles a target the local index shares no chunks with — a first publish, a new host
+/// of yours, or (the case this guards) the wrong store. Consulted at most once, only when there are
+/// rows to publish and the overlap is empty, and always before anything is uploaded.


Suggested change

/// How a push handles a target the local index shares no chunks with — a first publish, a new host

/// of yours, or (the case this guards) the wrong store. Consulted at most once, only when there are

/// rows to publish and the overlap is empty, and always before anything is uploaded.

/// How a push handles a target the local index shares no chunks with.

dacorvo · 2026-07-02T06:49:49Z

+    /// Ask before publishing. Called with the target label and the number of chunks about to be
+    /// pushed; returns whether to proceed. The CLI wires this to a stdin y/N prompt that fails closed
+    /// off a terminal.


Suggested change

/// Ask before publishing. Called with the target label and the number of chunks about to be

/// pushed; returns whether to proceed. The CLI wires this to a stdin y/N prompt that fails closed

/// off a terminal.

/// Ask before publishing. Called with the target label and the number of chunks to be pushed.

dacorvo · 2026-07-02T06:52:22Z

+/// Whether a push must be confirmed first: there are rows to publish (`to_push > 0`) and the local
+/// index shares no chunk with the remote (`to_push == local` ⇔ `|local ∩ remote| = 0`) — a first
+/// publish, a new host of yours, or the wrong store. `to_push` is `local − remote`, so it can never
+/// exceed `local`; equality means every local chunk is new to the remote.


Suggested change

/// Whether a push must be confirmed first: there are rows to publish (`to_push > 0`) and the local

/// index shares no chunk with the remote (`to_push == local` ⇔ `|local ∩ remote| = 0`) — a first

/// publish, a new host of yours, or the wrong store. `to_push` is `local − remote`, so it can never

/// exceed `local`; equality means every local chunk is new to the remote.

/// Whether a push must be confirmed first: there are rows to publish and the local index shares

/// no chunk with the remote — a first publish, a new host of yours, or the wrong store.

dacorvo · 2026-07-02T06:53:37Z

+    // Guard against publishing to a store this index shares nothing with — a first push here, a new
+    // host of yours, or the wrong store. The overlap is already in hand (see [`must_confirm`]), so
+    // this costs no extra round-trip. Only the real-upload paths reach here; the nothing-to-push
+    // paths returned above.


Suggested change

// Guard against publishing to a store this index shares nothing with — a first push here, a new

// host of yours, or the wrong store. The overlap is already in hand (see [`must_confirm`]), so

// this costs no extra round-trip. Only the real-upload paths reach here; the nothing-to-push

// paths returned above.

// When required, ask for confirmation before publishing.

Per PR #44 review: the comments repeated the same "first publish / new host / wrong store" rationale at the Confirm enum, the must_confirm predicate, and the gate call site, and leaked the set arithmetic into a doc comment. - Keep the rationale in one place — must_confirm, which defines the concept — and drop it from the enum/variant docs and the call site (now just "ask for confirmation before publishing"). - Drop the |local ∩ remote| arithmetic from must_confirm's doc. - Trim the Push CLI doc to what the command does. - Rename the push target local back to `remote`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

dacorvo and others added 2 commits July 1, 2026 17:49

dacorvo requested a review from Copilot July 1, 2026 17:01

Copilot started reviewing on behalf of dacorvo July 1, 2026 17:01 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

dacorvo commented Jul 2, 2026

View reviewed changes

dacorvo merged commit f79a62d into main Jul 2, 2026
7 checks passed

dacorvo deleted the push-store-gate branch July 2, 2026 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add a push store gate#44

Add a push store gate#44
dacorvo merged 4 commits into
mainfrom
push-store-gate

dacorvo commented Jul 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dacorvo left a comment

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

dacorvo Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let declined = funes::push::run_push(Store::parse(&uri), false, Confirm::Ask(decline)).await;
		let after_decline = funes::push::store_ids(&Store::parse(&uri)).await;

	"{target} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"
	"{remote} is read-only for your token — recall can read it, but publishing needs write access (check your HF token)"

Uh oh!

Conversation

dacorvo commented Jul 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dacorvo left a comment

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

dacorvo Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants