fix(storage): convention parity#120
Conversation
…tions - Use the shared isInteractive() gate (stdin + stdout TTY, json-aware) instead of stdout-only checks in the zone resolver, link, and zone add - zones update: require field flags whenever non-interactive (json, non-TTY, or --force) instead of only under --output json, and abort the whole interactive edit on a mid-flow cancel rather than applying partial answers - zones update --force now matches zones add: skip prompts and confirmations, use flag values only - storage zone domains commands now resolve the zone like every other storage command: explicit ref, then linked zone, then picker
- Honor --force when resolving the update target: pass it through to resolveStorageZoneInteractive so a missing zone errors instead of opening the picker - Omit ReplicationZones from the interactive editor's settings when no new regions were picked, keeping replication out of the update body - Clarify that flags take full precedence over the interactive editor
- zones remove: double confirmation (yes/no, then type the zone name) unless --force, matching db and scripts delete; also removes a stale .bunny/storage.json that pointed at the deleted zone - New storage unlink command mirroring dns zones unlink - resolveStorageZoneInteractive takes DNS-style opts (output, force, offerLink, ignoreManifest); the picker now offers to link the directory on non-destructive commands, and link reuses the resolver instead of duplicating it - The zone-required error now hints at bunny storage link - Replication confirmation defaults to no: adding a replica is permanent and adds cost
|
@codex review |
🦋 Changeset detectedLatest commit: 9ca267c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
jamie-at-bunny
changed the base branch from
main
to
fix/storage-behavioral-consistency
![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: 9ca267cb30
ℹ️ 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 (!force) { | ||
| const confirmed = await confirm( | ||
| `Unlink from ${existing.name ?? existing.id}?`, | ||
| ); |
There was a problem hiding this comment.
When --output json is used and .bunny/storage.json exists, this branch still opens a confirmation prompt unless --force is also supplied. In CI or other JSON consumers this can block or produce the plain-text Unlink cancelled. success path instead of a JSON payload, violating the command's JSON-output contract; require --force/return a JSON error before calling confirm in JSON mode.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR brings behavioral and convention parity across the storage command group: shared
Confidence Score: 4/5Safe to merge with minor follow-ups; no data loss or correctness issues on the happy path. The core refactoring is solid and matches the documented conventions. The type-to-confirm check in zone removal has an edge case where a
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolveStorageZoneInteractive(client, ref, opts)"] --> B{ref provided?}
B -- yes --> C[resolveStorageZone via API]
B -- no --> D{opts.ignoreManifest?}
D -- no --> E{manifest.id exists?}
E -- yes --> F[fetchStorageZone by manifest ID]
E -- no --> G{isInteractive + !force?}
D -- yes --> G
G -- no --> H["throw UserError: zone required"]
G -- yes --> I[Show interactive picker]
I --> J{User cancels?}
J -- yes --> K["throw UserError: zone required"]
J -- no --> L[fetchStorageZone by picked ID]
L --> M{opts.offerLink?}
M -- yes --> N["confirm: Link this directory?"]
N -- yes --> O[writeStorageManifest + log success]
N -- no --> P[return zone]
O --> P
M -- no --> P
C --> P
F --> P
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["resolveStorageZoneInteractive(client, ref, opts)"] --> B{ref provided?}
B -- yes --> C[resolveStorageZone via API]
B -- no --> D{opts.ignoreManifest?}
D -- no --> E{manifest.id exists?}
E -- yes --> F[fetchStorageZone by manifest ID]
E -- no --> G{isInteractive + !force?}
D -- yes --> G
G -- no --> H["throw UserError: zone required"]
G -- yes --> I[Show interactive picker]
I --> J{User cancels?}
J -- yes --> K["throw UserError: zone required"]
J -- no --> L[fetchStorageZone by picked ID]
L --> M{opts.offerLink?}
M -- yes --> N["confirm: Link this directory?"]
N -- yes --> O[writeStorageManifest + log success]
N -- no --> P[return zone]
O --> P
M -- no --> P
C --> P
F --> P
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { value } = await prompts({ | ||
| type: "text", | ||
| name: "value", | ||
| message: `Type "${zone.Name}" to confirm:`, | ||
| }); | ||
| if (value !== zone.Name) { | ||
| logger.log("Cancelled."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
zone.Name is undefined
When the user presses Ctrl+C on the type-to-confirm prompt, prompts resolves with {}, so value is undefined. For every zone that has a real string name the check undefined !== zone.Name evaluates to true and correctly logs "Cancelled." — but if zone.Name is undefined (allowed by the OpenAPI type), the comparison is undefined !== undefined → false, which silently lets the deletion proceed without the user ever confirming the name. Even though real zones fetched from the API always carry a name today, this is an unintended bypass path in the most destructive command in the file. A simple null-coalescing guard on zone.Name (e.g. zone.Name ?? "") makes the second prompt produce a non-empty string that can never accidentally match an undefined typed value.
| if (!force) { | ||
| const confirmed = await confirm( | ||
| `Unlink from ${existing.name ?? existing.id}?`, | ||
| ); | ||
| if (!confirmed) { | ||
| logger.log("Unlink cancelled."); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| removeManifest(STORAGE_MANIFEST); | ||
|
|
There was a problem hiding this comment.
Every other storage command that needs interactive input (e.g. zone update, zone remove) checks isInteractive(output) first and throws a UserError when running without a TTY and without --force. unlink skips that guard: when called in a non-TTY environment (CI, piped output) without --force, confirm() resolves with undefined, returns false, and the command logs "Unlink cancelled." and exits 0 — even though the manifest is still present. A script that expected the unlink to succeed would silently see the manifest left behind with no error to act on. Adding an isInteractive check (or accepting force in the confirm call) before the prompt would match the pattern used elsewhere.
3 participants
No description provided.