fix: require confirmation before restoring deleted local files in ht revert - #12
fix: require confirmation before restoring deleted local files in ht revert#12aoruLola wants to merge 6 commits into
Conversation
…revert When a checked-out asset is deleted locally (local_hash == None, base_hash == Some), the revert command now treats this as overwriting a local change, triggering the dangerous-operation prompt unless --yes is provided. Previously this case fell through to false, silently recreating the file without confirmation. Also adds the `ht revert` command to recover a single asset from a previous snapshot.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8606b6d7de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args.to_changeset_id.as_deref(), | ||
| ) | ||
| .await?; | ||
| let asset = find_snapshot_asset(&snapshot, &asset_path)?.clone(); |
There was a problem hiding this comment.
Reverting to a changeset before an asset was added should restore that asset to an absent state, but this lookup makes a missing target entry an error and exits before staging the deletion. In that scenario (ht revert --asset-path X --to <pre-add> with X present in the current workspace), users cannot use the new command to undo an added asset even though deletions are representable in the stage (blob_hash: None, as used by ht remove).
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
|
|
||
| upsert_stage_asset(stage, asset_path, Some(blob_hash.to_string())); |
There was a problem hiding this comment.
When the matched snapshot entry has an asset_id different from its path, this stages only the path/blob hash via StageFile/AssetDelta. On submit the server defaults a missing asset ID to the path, so the revert updates or inserts a path-keyed asset instead of the snapshot asset, leaving the original asset ID unchanged; this affects repos populated through API/checkpoint flows that use stable asset IDs. The revert should carry asset.asset_id into the staged delta.
Useful? React with 👍 / 👎.
1. Stage deletions when target snapshot lacks the asset:
- When reverting to a changeset before an asset was added, the
command now stages a deletion (blob_hash: None) and removes the
local file instead of erroring.
2. Preserve snapshot asset IDs when staging reverts:
- Add asset_id field to AssetDelta with serde skip_serializing_if
- Pass asset_id from snapshot entries through apply_revert_state
and upsert_stage_asset into the staged delta
- On submit the server defaults a missing asset ID to the path,
so this preserves stable asset IDs from API/checkpoint flows
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee163affcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| &asset_path, | ||
| None, | ||
| base_hash.as_deref(), | ||
| None, |
There was a problem hiding this comment.
When the target snapshot lacks this path but the current branch contains it under a stable asset_id different from the path, this missing-target branch stages the deletion with asset_id: None. The submit path defaults a missing id to asset.path and removes that key (crates/server/src/core/versioning.rs:932 and :948), so the stable-id asset remains in the snapshot and the revert-to-absent is effectively not applied. Fresh evidence is that the newly added absent-target branch still passes None as the staged asset id.
Useful? React with 👍 / 👎.
|
|
||
| update | ||
| } | ||
| None => { |
There was a problem hiding this comment.
When --to points to a snapshot before the asset existed, execution enters this branch and completes the delete/stage flow without the !args.keep_lock lock-release call used by the restore branch above. Because --keep-lock is documented as the opt-out, the default ht revert --asset-path X --to <pre-add> leaves the HyperTide lock held after a successful revert and requires a manual ht lock release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d76c72392d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(lock) = state.lock_manager.get_lock(&asset.path) { | ||
| if let Some(lock) = state | ||
| .lock_manager | ||
| .get_lock_with_repo(repo_id, "asset", &asset.path) |
There was a problem hiding this comment.
When upgrading with active locks created by the previous API, those rows are loaded with repo_id == "" (the old lock path used the default repo-less key). This submit-time check now only looks up locks under the requested repo, while the lock API also rejects empty repo IDs, so those legacy locks become invisible until they expire and another user can submit over an asset that was locked before the deploy. Add a migration/cleanup strategy or a temporary fallback check for repo-less locks.
Useful? React with 👍 / 👎.
| &mut workspace, | ||
| &mut stage, | ||
| &asset_path, | ||
| None, |
There was a problem hiding this comment.
When the target snapshot does not contain the asset, this passes None into apply_revert_state, creating a staged deletion. The status path collapses staged state to staged_hash and only classifies a row as staged when that hash is Some, so after this revert ht status reports the asset as deleted/unmodified instead of showing the pending staged deletion that ht submit will apply. Carry an explicit staged-delete marker through status classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f56f11638
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Ok(identity) = rate_limit.auth_manager.validate_access_token(&token).await { | ||
| return format!("principal:{}", identity.owner_id); | ||
| } | ||
| return "anonymous".to_string(); |
There was a problem hiding this comment.
When a request carries an expired or malformed Bearer token, this branch returns the literal anonymous bucket before reaching the peer-IP fallback used for invalid API keys below. In rate-limited deployments, unrelated clients with invalid/expired bearer tokens from different IPs all consume the same global bucket and can 429 each other (or be DoSed), instead of being isolated by the anonymous per-IP bucket.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cac8f085d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let peer_ip = request | ||
| .extensions() | ||
| .get::<ConnectInfo<SocketAddr>>() | ||
| .map(|connect_info| connect_info.0.ip()); |
There was a problem hiding this comment.
In reverse-proxy deployments (the self-hosting docs say to expose the TLS proxy rather than raw :3000), ConnectInfo is the proxy's socket address, not the original client. Because the previous X-Forwarded-For/X-Real-IP handling is gone, all anonymous or invalid-credential clients behind the proxy share one anonymous:<proxy-ip> bucket and can 429 each other. Consider using a trusted-forwarded-header path or proxy-aware configuration for this bucket.
Useful? React with 👍 / 👎.
| } else if let Some(api_key) = api_key { | ||
| if let Ok(identity) = rate_limit | ||
| .auth_manager | ||
| .validate_api_key_identity(&api_key) | ||
| .await |
There was a problem hiding this comment.
With DB-backed auth, persisted API keys that are not in the in-memory cache (for example after a server restart) and random invalid X-API-Key values both call validate_api_key_identity before allow() is checked. A flood of invalid or unique keys therefore still performs one auth lookup per request even after the anonymous bucket is exhausted, so rate limiting no longer shields the database; add a cheap pre-limit/cache or defer DB validation until after a coarse bucket check.
Useful? React with 👍 / 👎.
| base_hash: Option<&str>, | ||
| asset_id: Option<String>, | ||
| ) -> RevertStateUpdate { | ||
| if base_hash == blob_hash { |
There was a problem hiding this comment.
When the target snapshot has the same blob hash at this path but a different asset_id (e.g. an asset was delete/re-added with identical bytes), this hash-only comparison treats the revert as a no-op, removes any staged delta, and even rewrites the local workspace metadata to the old id. The server snapshot remains at the current id, and subsequent CLI updates/deletes can target the wrong asset id, so this needs to compare/stage identity changes as well as blob changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb488ec54c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let payload = LockRequest { file_path: path }; | ||
| let repo_id = match repo_id { | ||
| Some(repo_id) => repo_id.to_string(), | ||
| None => resolve_repo(&profile, None)?, |
There was a problem hiding this comment.
When a user logs in without a default repo and then initializes a workspace with ht checkout --repo repo-a, the lock subcommands have no --repo flag and pass None into this helper, so this new repo-scoped payload path fails with repo not set before contacting the server. Other workspace commands fall back to workspace.repo_id; the lock commands need the same fallback (or an explicit repo option), otherwise locks are unusable in a valid checked-out workspace unless the user separately mutates the profile.
Useful? React with 👍 / 👎.
1 participant
Summary
local_hash == None,base_hash == Some),ht revertnow treats this as overwriting a local change, triggering the dangerous-operation prompt unless--yesis providedfalse, silently recreating the file without confirmationht revertcommand to recover a single asset from a previous snapshotTest plan
deleted_local_file_treated_as_overwriteverifies the match expression logic