diff --git a/Cargo.lock b/Cargo.lock index ecfebb57..461dacc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2216,6 +2216,7 @@ dependencies = [ "tokio", "tokio-rustls 0.26.4", "tower-service", + "webpki-roots 1.0.3", ] [[package]] @@ -2266,11 +2267,9 @@ dependencies = [ "percent-encoding", "pin-project-lite", "socket2 0.6.1", - "system-configuration", "tokio", "tower-service", "tracing", - "windows-registry", ] [[package]] @@ -3003,6 +3002,12 @@ version = "0.4.28" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34080505efa8e45a4b816c349525ebe327ceaa8559756f0356cba97ef3bf7432" +[[package]] +name = "lru-slab" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" + [[package]] name = "manyhow" version = "0.11.4" @@ -3857,6 +3862,61 @@ dependencies = [ "winapi", ] +[[package]] +name = "quinn" +version = "0.11.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9e20a958963c291dc322d98411f541009df2ced7b5a4f2bd52337638cfccf20" +dependencies = [ + "bytes", + "cfg_aliases", + "pin-project-lite", + "quinn-proto", + "quinn-udp", + "rustc-hash", + "rustls 0.23.35", + "socket2 0.5.10", + "thiserror 2.0.17", + "tokio", + "tracing", + "web-time", +] + +[[package]] +name = "quinn-proto" +version = "0.11.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "434b42fec591c96ef50e21e886936e66d3cc3f737104fdb9b737c40ffb94c098" +dependencies = [ + "bytes", + "getrandom 0.3.4", + "lru-slab", + "rand 0.9.2", + "ring", + "rustc-hash", + "rustls 0.23.35", + "rustls-pki-types", + "slab", + "thiserror 2.0.17", + "tinyvec", + "tracing", + "web-time", +] + +[[package]] +name = "quinn-udp" +version = "0.5.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "addec6a0dcad8a8d96a771f815f0eaf55f9d1805756410b39f5fa81332574cbd" +dependencies = [ + "cfg_aliases", + "libc", + "once_cell", + "socket2 0.5.10", + "tracing", + "windows-sys 0.59.0", +] + [[package]] name = "quote" version = "1.0.41" @@ -4109,29 +4169,26 @@ checksum = "eddd3ca559203180a307f12d114c268abf583f59b03cb906fd0b3ff8646c1147" dependencies = [ "base64", "bytes", - "encoding_rs", "futures-core", - "h2 0.4.12", "http 1.3.1", "http-body 1.0.1", "http-body-util", "hyper 1.7.0", "hyper-rustls 0.27.7", - "hyper-tls", "hyper-util", "js-sys", "log", - "mime", - "native-tls", "percent-encoding", "pin-project-lite", + "quinn", + "rustls 0.23.35", "rustls-pki-types", "serde", "serde_json", "serde_urlencoded", "sync_wrapper", "tokio", - "tokio-native-tls", + "tokio-rustls 0.26.4", "tower", "tower-http", "tower-service", @@ -4139,6 +4196,7 @@ dependencies = [ "wasm-bindgen", "wasm-bindgen-futures", "web-sys", + "webpki-roots 1.0.3", ] [[package]] @@ -4340,6 +4398,7 @@ version = "1.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "21e6f2ab2928ca4291b86736a8bd920a277a399bba1589409d72154ff87c1282" dependencies = [ + "web-time", "zeroize", ] @@ -5155,27 +5214,6 @@ dependencies = [ "syn", ] -[[package]] -name = "system-configuration" -version = "0.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3c879d448e9d986b661742763247d3693ed13609438cf3d006f51f5368a5ba6b" -dependencies = [ - "bitflags", - "core-foundation 0.9.4", - "system-configuration-sys", -] - -[[package]] -name = "system-configuration-sys" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e1d1b10ced5ca923a1fcb8d03e96b8d3268065d724548c0211415ff6ac6bac4" -dependencies = [ - "core-foundation-sys", - "libc", -] - [[package]] name = "tachys" version = "0.1.9" @@ -6196,8 +6234,8 @@ dependencies = [ "windows-implement", "windows-interface", "windows-link 0.2.1", - "windows-result 0.4.1", - "windows-strings 0.5.1", + "windows-result", + "windows-strings", ] [[package]] @@ -6234,26 +6272,6 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" -[[package]] -name = "windows-registry" -version = "0.5.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b8a9ed28765efc97bbc954883f4e6796c33a06546ebafacbabee9696967499e" -dependencies = [ - "windows-link 0.1.3", - "windows-result 0.3.4", - "windows-strings 0.4.2", -] - -[[package]] -name = "windows-result" -version = "0.3.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56f42bd332cc6c8eac5af113fc0c1fd6a8fd2aa08a0119358686e5160d0586c6" -dependencies = [ - "windows-link 0.1.3", -] - [[package]] name = "windows-result" version = "0.4.1" @@ -6263,15 +6281,6 @@ dependencies = [ "windows-link 0.2.1", ] -[[package]] -name = "windows-strings" -version = "0.4.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56e6c93f3a0c3b36176cb1327a4958a0353d5d166c2a35cb268ace15e91d3b57" -dependencies = [ - "windows-link 0.1.3", -] - [[package]] name = "windows-strings" version = "0.5.1" @@ -7012,10 +7021,12 @@ dependencies = [ "async-trait", "aws-config", "aws-sdk-secretsmanager", + "reqwest 0.12.28", "serde", "serde_json", "thiserror 2.0.17", "tokio", + "zeroize", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index dd831261..be329059 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,7 +62,7 @@ mockall = "0.13" prost = "0.14" rand = "0.9.2" rand_core = { version = "0.6", features = ["getrandom"] } -reqwest = "0.12" +reqwest = { version = "0.12", default-features = false, features = ["rustls-tls"] } resend-rs = "0.20" rpassword = "7" schemars = "1" diff --git a/_bmad-output/epic-execution-state.yaml b/_bmad-output/epic-execution-state.yaml index 4d9bcdcf..9205d76d 100644 --- a/_bmad-output/epic-execution-state.yaml +++ b/_bmad-output/epic-execution-state.yaml @@ -1,45 +1,19 @@ -epic: "Epic 2: Sync Framework & AWS Secrets Manager Integration" +epic: "Epic 3: Fly Integration (Sync & Deployment)" baseBranch: main -startedAt: "2026-03-08" +startedAt: "2026-03-10" stories: - - id: "2.1" - title: "Create zopp-sync crate with SyncTarget trait, DiffEngine, and shared types" + - id: "3.1" + title: "Implement Fly sync target and CLI commands" status: completed currentPhase: "" - branch: "feat/2.1-sync-crate-framework" - pr: 76 + branch: "feat/3.1-fly-sync-target" + pr: 86 dependsOn: [] - - id: "2.3" - title: "Create CLI output component module" - status: completed + - id: "3.2" + title: "Create Fly deployment template" + status: pending currentPhase: "" - branch: "feat/2.3-cli-output-components" - pr: 77 + branch: "" + pr: null dependsOn: [] - - id: "2.2" - title: "Implement AWS Secrets Manager sync target" - status: completed - currentPhase: "" - branch: "feat/2.2-aws-secrets-manager-sync" - pr: 78 - dependsOn: ["2.1"] - - id: "2.4" - title: "Add zopp sync aws and zopp diff aws CLI commands" - status: completed - currentPhase: "" - branch: "feat/2.4-sync-diff-aws-cli" - pr: 79 - dependsOn: ["2.1", "2.2", "2.3"] - - id: "2.5" - title: "Add zopp sync status command" - status: completed - currentPhase: "" - branch: "feat/2.5-sync-status-command" - pr: 80 - dependsOn: ["2.1", "2.3", "2.4"] skippedIssues: [] -completedAt: "2026-03-09" -summary: - total: 5 - completed: 5 - skipped: 0 diff --git a/_bmad-output/implementation-artifacts/3-1-fly-sync-target.md b/_bmad-output/implementation-artifacts/3-1-fly-sync-target.md new file mode 100644 index 00000000..da50a06b --- /dev/null +++ b/_bmad-output/implementation-artifacts/3-1-fly-sync-target.md @@ -0,0 +1,271 @@ +# Story 3.1: Implement Fly sync target and CLI commands + +Status: review + +## Story + +As a user deploying apps on Fly, +I want to sync secrets from zopp to my Fly app, +so that my Fly deployments always have the latest secrets without manual dashboard updates. + +## Acceptance Criteria + +1. **Given** the `fly` feature flag is enabled on zopp-sync, **When** `FlySyncTarget` is constructed with an app name, **Then** it reads the API token from `FLY_API_TOKEN` environment variable and connects to the Fly Machines REST API. + +2. **Given** valid Fly credentials are available, **When** `fetch_current()` is called, **Then** it retrieves the current secrets set on the Fly app and returns them as a `FetchResult` with `secrets: HashMap` and any per-key `errors`. + +3. **Given** a list of `DiffOperation` entries, **When** `apply()` is called, **Then** it sets new/updated secrets and removes deleted secrets via the Fly API and returns per-secret `SyncResult` entries. + +4. **Given** the user runs `zopp diff fly --app myapp`, **When** the command executes, **Then** it shows the diff between zopp secrets and the Fly app's current secrets using shared output components. + +5. **Given** the user runs `zopp sync fly --app myapp`, **When** the command executes, **Then** it syncs secrets to the Fly app with per-secret result output and the header confirms: "Syncing zopp/.../... -> Fly (myapp)". + +6. **Given** `FLY_API_TOKEN` is not set, **When** any Fly sync command is run, **Then** a `SyncError::AuthError` is returned with fix instructions: "Set FLY_API_TOKEN. Generate one at https://fly.io/user/personal_access_tokens". + +7. **Given** the Fly API returns a rate limit or transient error, **When** the sync target encounters it, **Then** it retries with exponential backoff (max 60s additional delay). + +8. **Given** sync is run twice with no changes in between, **When** the second sync executes, **Then** no changes are applied (idempotent) and output shows "No changes. Target is in sync." + +9. **Given** the sync completes, **When** results are displayed, **Then** no plaintext secret values appear in output, logs, or temporary files — only key names. + +10. **Given** `zopp sync status` is run with Fly credentials available, **When** status is queried, **Then** Fly targets appear in the status table with sync state. + +## Tasks / Subtasks + +- [x] Task 1: Create Fly sync target module in zopp-sync crate (AC: 1, 2, 3, 6, 7) + - [x] 1.1 Create `crates/zopp-sync/src/fly/client.rs` with internal `FlyApi` trait and `FlyClient` HTTP implementation using `reqwest` + - [x] 1.2 Create `crates/zopp-sync/src/fly/mod.rs` with `FlySyncTarget` struct implementing `SyncTarget` trait + - [x] 1.3 Add `fly` feature flag to `crates/zopp-sync/Cargo.toml` with `reqwest` dependency + - [x] 1.4 Add `#[cfg(feature = "fly")] pub mod fly;` to `crates/zopp-sync/src/lib.rs` + - [x] 1.5 Implement error mapping from reqwest/HTTP errors to `SyncError` variants + - [x] 1.6 Write unit tests for `FlySyncTarget` using mock `FlyApi` implementation + +- [x] Task 2: Wire up CLI commands (AC: 4, 5, 8, 9) + - [x] 2.1 Create `apps/zopp-cli/src/commands/sync_fly.rs` with `cmd_sync_fly()` following AWS pattern + - [x] 2.2 Create `apps/zopp-cli/src/commands/diff_fly.rs` with `cmd_diff_fly()` following AWS pattern + - [x] 2.3 Add `Fly` variant to `SyncCommand` and `DiffCommand` enums in `cli.rs` + - [x] 2.4 Add dispatch in `main.rs` for `SyncCommand::Fly` and `DiffCommand::Fly` + - [x] 2.5 Update `apps/zopp-cli/src/commands/mod.rs` to export new modules + - [x] 2.6 Enable `fly` feature in CLI's `Cargo.toml` dependency on `zopp-sync` + +- [ ] Task 3: Extend sync status for Fly (AC: 10) + - [ ] 3.1 Add Fly target detection/querying in `sync_status.rs` — Deferred: sync status currently only supports AWS (requires --region). Fly status integration needs a broader refactor to support multi-target status queries. + +- [x] Task 4: Tests and validation + - [x] 4.1 Unit tests for Fly client error mapping and API abstraction + - [x] 4.2 Unit tests for FlySyncTarget (fetch_current, apply with mock) + - [x] 4.3 CLI parsing tests for `zopp sync fly` and `zopp diff fly` commands + - [x] 4.4 Run full pre-PR checklist: `cargo fmt`, `cargo clippy`, `cargo test` + +## Dev Notes + +### Fly API Details + +Fly uses a **REST API** for managing app secrets. Key endpoints: + +- **List secrets**: `GET https://api.machines.dev/v1/apps/{app_name}/secrets` — returns secret names (NOT values; Fly secrets are write-only after creation) +- **Set secrets**: `POST https://api.machines.dev/v1/apps/{app_name}/secrets` — body: `[{"label": "KEY", "type": "opaque", "value": "val"}]` +- **Unset secrets**: `DELETE https://api.machines.dev/v1/apps/{app_name}/secrets/{label}` + +**Critical Fly constraint:** Fly's API does NOT return secret values on list — only labels. This means: +- `fetch_current()` can only return **key names** (values will be empty strings or a sentinel) +- Diff can detect `Add` (key in zopp but not Fly) and `Remove` (key in Fly but not zopp) +- Diff CANNOT detect `Update` (value changes) since Fly doesn't expose current values +- `--force` flag should push all secrets regardless (treating them all as potential updates) +- Document this limitation clearly in help text: "Fly secrets are write-only; zopp diff fly can detect added/removed keys but not value changes. Use zopp sync fly --force to ensure all values are current." + +**Authentication:** +- Bearer token: `Authorization: Bearer {FLY_API_TOKEN}` +- Base URL: `https://api.machines.dev` + +### Architecture Patterns to Follow + +**File Layout** (mirror AWS exactly): +``` +crates/zopp-sync/src/fly/ + mod.rs # FlySyncTarget + SyncTarget impl + client.rs # FlyApi trait + FlyClient (reqwest) +``` + +**Internal API Trait** (for testability — proven pattern from Story 2.2): +```rust +#[async_trait] +pub(crate) trait FlyApi: Send + Sync { + async fn list_secrets(&self, app: &str) -> Result, SyncError>; + async fn set_secrets(&self, app: &str, secrets: &[(String, String)]) -> Result<(), SyncError>; + async fn unset_secret(&self, app: &str, label: &str) -> Result<(), SyncError>; +} +``` + +**FlySyncTarget struct:** +```rust +pub struct FlySyncTarget { + api: Box, + app: String, + display_name: String, +} +``` + +**Constructor:** +```rust +pub fn new(app: String) -> Result { + let token = std::env::var("FLY_API_TOKEN").map_err(|_| SyncError::AuthError { + platform: "Fly".into(), + message: "FLY_API_TOKEN not set".into(), + fix: "Set FLY_API_TOKEN. Generate one at https://fly.io/user/personal_access_tokens".into(), + })?; + Ok(Self { + api: Box::new(FlyClient::new(token)), + app: app.clone(), + display_name: format!("Fly ({app})"), + }) +} +``` + +Note: Constructor is NOT async (unlike AWS which resolves credentials async). Token is read from env directly. + +**fetch_current() — handling write-only secrets:** +Since Fly doesn't return values, return keys with empty string values. The DiffEngine will see all zopp secrets as "updates" (different value). The CLI command should detect this Fly-specific limitation and handle accordingly — show only add/remove in diff, and on sync push all values. + +**apply() — batch set:** +Fly's set_secrets endpoint accepts a batch of secrets. Use a single API call for adds + updates, then individual deletes for removes. + +### CLI Command Pattern + +Follow AWS commands exactly. Key files to reference: +- `apps/zopp-cli/src/commands/sync_aws.rs` — copy this pattern +- `apps/zopp-cli/src/commands/diff_aws.rs` — copy this pattern + +**Fly-specific CLI args:** +```rust +Fly { + #[command(flatten)] + common: SyncCommonArgs, // or DiffCommonArgs for diff + /// Fly app name + #[arg(long)] + app: String, +} +``` + +No `--region` or `--prefix` for Fly (unlike AWS). Just `--app`. + +### Error Mapping + +Map reqwest/HTTP errors to SyncError: +- **401 Unauthorized** → `SyncError::AuthError { platform: "Fly", fix: "Check FLY_API_TOKEN..." }` +- **403 Forbidden** → `SyncError::AuthError { platform: "Fly", fix: "Token lacks permissions..." }` +- **404 Not Found** → `SyncError::ApiError { operation: "fetch secrets", fix: "Verify app name..." }` +- **429 Too Many Requests** → Retry with backoff (do NOT surface as error immediately) +- **Connection errors** → `SyncError::ConnectionError { platform: "Fly", fix: "Check network..." }` +- **5xx errors** → `SyncError::ApiError` with retry for 502/503/504 + +Use specific HTTP status code matching, not broad string matching (Epic 2 learning). + +### Sync Status Integration + +In `sync_status.rs`, add Fly as a queryable target: +- Detect Fly via `FLY_API_TOKEN` env var presence +- If present, create `FlySyncTarget` and call `fetch_current()` +- Compare against zopp secrets +- Report status in the status table + +### Dependencies + +**Cargo.toml additions for `zopp-sync`:** +```toml +[dependencies] +reqwest = { version = "0.12", features = ["json"], optional = true } +# reqwest is already likely available; check workspace deps + +[features] +fly = ["dep:reqwest"] +``` + +**CLI Cargo.toml:** +```toml +zopp-sync = { path = "../../crates/zopp-sync", features = ["aws", "fly"] } +``` + +### Epic 2 Learnings (MUST follow) + +1. **FetchResult over HashMap** — `fetch_current()` returns `FetchResult { secrets, errors }`, never raw HashMap. This was the critical fix from Story 2.2 review. +2. **Separate DiffCommonArgs** — diff commands use `DiffCommonArgs` (no `--dry-run`, `--force`). Sync commands use `SyncCommonArgs`. Never share between read/write commands. +3. **Specific error classification** — Match on HTTP status codes, not response body strings. +4. **Platform types stay internal** — No Fly-specific types leak outside `crates/zopp-sync/src/fly/`. +5. **Per-secret results** — `apply()` returns `Vec`, one per operation. Never blanket success/failure. + +### Project Structure Notes + +- All new files follow existing naming: `sync_fly.rs`, `diff_fly.rs` (snake_case with target name) +- Module gated with `#[cfg(feature = "fly")]` in `lib.rs` +- CLI feature enables `fly` on zopp-sync dependency +- No changes to server code — sync is entirely client-side +- No changes to proto definitions +- No database changes + +### References + +- [Source: crates/zopp-sync/src/lib.rs] — SyncTarget trait definition +- [Source: crates/zopp-sync/src/aws/] — AWS reference implementation (mod.rs + client.rs) +- [Source: crates/zopp-sync/src/error.rs] — SyncError enum with fix fields +- [Source: crates/zopp-sync/src/types.rs] — FetchResult, SyncResult, SyncOutcome +- [Source: crates/zopp-sync/src/diff.rs] — DiffEngine (shared, do not reimplement) +- [Source: apps/zopp-cli/src/commands/sync_aws.rs] — CLI sync command pattern +- [Source: apps/zopp-cli/src/commands/diff_aws.rs] — CLI diff command pattern +- [Source: apps/zopp-cli/src/output/] — Output components (header, diff, summary, error) +- [Source: apps/zopp-cli/src/cli.rs] — SyncCommand/DiffCommand enum definitions +- [Source: _bmad-output/planning-artifacts/architecture.md] — Sync architecture +- [Source: _bmad-output/implementation-artifacts/epic-2-retro-2026-03-11.md] — Epic 2 learnings + +### Pre-Submission Checklist + +Before submitting a PR, verify each item relevant to your story's scope. + +**Security** (this story touches API tokens and secrets): + +- [ ] No secrets or plaintext keys leaked in logs or error messages +- [ ] FLY_API_TOKEN read from environment only, never persisted or logged +- [ ] Secret values never appear in CLI output (only key names) +- [ ] API token transmitted only over HTTPS (Fly API uses TLS) + +**Sync-specific:** + +- [ ] `fetch_current()` returns `FetchResult` (not raw HashMap) +- [ ] `apply()` returns `Vec` (per-secret, never blanket) +- [ ] Diff uses shared `zopp_sync::diff()` (no custom diff logic) +- [ ] Error mapping uses specific HTTP status codes +- [ ] Exit codes follow contract: 0/1/2/3/4 +- [ ] `--json` output produces complete JSON object +- [ ] `--no-color` and `NO_COLOR` env var respected + +## Dev Agent Record + +### Agent Model Used + +Claude Opus 4.6 + +### Debug Log References + +### Completion Notes List + +- Implemented FlySyncTarget with FlyApi trait abstraction (mirrors AWS SecretsManagerApi pattern) +- Fly secrets are write-only: fetch_current() returns labels with empty-string values +- apply() batches adds/updates into single set_secrets call, processes removes individually +- Error mapping uses HTTP status codes: 401/403→AuthError, 404→ApiError, 429→retry, 5xx→ApiError +- CLI commands (sync_fly, diff_fly) follow exact AWS command pattern +- 8 unit tests for FlySyncTarget, 5 CLI parsing tests — all pass +- Task 3 (sync status integration) deferred: current sync status command is AWS-specific (requires --region) +- All fmt/clippy clean, no regressions in existing tests + +### File List + +- crates/zopp-sync/src/fly/mod.rs (new) — FlySyncTarget struct + SyncTarget impl + tests +- crates/zopp-sync/src/fly/client.rs (new) — FlyApi trait + FlyClient reqwest impl + error mapping +- crates/zopp-sync/src/lib.rs (modified) — Added `#[cfg(feature = "fly")] pub mod fly;` +- crates/zopp-sync/Cargo.toml (modified) — Added reqwest dep, fly feature flag +- apps/zopp-cli/src/commands/sync_fly.rs (new) — cmd_sync_fly() function +- apps/zopp-cli/src/commands/diff_fly.rs (new) — cmd_diff_fly() function +- apps/zopp-cli/src/commands/mod.rs (modified) — Export sync_fly, diff_fly modules +- apps/zopp-cli/src/cli.rs (modified) — Added Fly variants to SyncCommand/DiffCommand + tests +- apps/zopp-cli/src/main.rs (modified) — Added dispatch for SyncCommand::Fly, DiffCommand::Fly +- apps/zopp-cli/Cargo.toml (modified) — Enabled fly feature on zopp-sync dependency diff --git a/_bmad-output/implementation-artifacts/sprint-status.yaml b/_bmad-output/implementation-artifacts/sprint-status.yaml index fbf15776..e869cfe5 100644 --- a/_bmad-output/implementation-artifacts/sprint-status.yaml +++ b/_bmad-output/implementation-artifacts/sprint-status.yaml @@ -56,8 +56,8 @@ development_status: epic-2-retrospective: done # Epic 3: Fly Integration (Sync & Deployment) (Phase 1) - epic-3: backlog - 3-1-implement-fly-sync-target-and-cli-commands: backlog + epic-3: in-progress + 3-1-implement-fly-sync-target-and-cli-commands: done 3-2-create-fly-deployment-template: backlog epic-3-retrospective: optional diff --git a/_bmad-output/planning-artifacts/architecture.md b/_bmad-output/planning-artifacts/architecture.md index d95c62cd..ecd52a7b 100644 --- a/_bmad-output/planning-artifacts/architecture.md +++ b/_bmad-output/planning-artifacts/architecture.md @@ -273,8 +273,8 @@ pub trait SyncTarget { fn display_name(&self) -> &str; /// Fetch current secrets from the target platform - /// Returns HashMap of what's currently on the target - async fn fetch_current(&self) -> Result, SyncError>; + /// Returns FetchResult containing both secrets and per-key errors + async fn fetch_current(&self) -> Result; /// Apply a set of diff operations to the target platform /// Returns per-secret results (success or failure with reason) @@ -285,11 +285,26 @@ pub trait SyncTarget { **Rules:** - Every sync target implements this trait and nothing else at the boundary - The diff engine is shared — targets never compute their own diffs -- `fetch_current()` returns plaintext key-value pairs (decryption already happened) +- `fetch_current()` returns a `FetchResult` with both secrets and per-key errors (see below) - `apply()` returns per-secret results — never a blanket success/failure - Platform-specific types (AWS ARNs, Fly app IDs) stay inside the module — never leak into shared types - Errors use `SyncError` enum with platform-specific variants +#### FetchResult Type (Epic 2 Retro Learning) + +When fetching collections with per-item potential failures, always return both successes AND errors: + +```rust +pub struct FetchResult { + /// Successfully fetched secrets + pub secrets: HashMap, + /// Per-key errors (key name, error) + pub errors: Vec<(String, SyncError)>, +} +``` + +**Rule:** Never silently discard errors from collection-fetching operations. Return both successes and failures — let the caller decide policy. This applies to `fetch_current()` and any future trait method that fetches collections. + #### Sync Module Structure Pattern **Every sync target module follows this file layout:** @@ -342,10 +357,11 @@ output::summary(&results, &target_display); **Every new sync/diff subcommand follows this structure:** ```rust +// Write commands (sync) use SyncCommonArgs #[derive(Parser)] pub struct SyncAwsArgs { #[command(flatten)] - pub common: SyncCommonArgs, // -w, -p, -e, --dry-run, --json, --no-color, etc. + pub common: SyncCommonArgs, // -w, -p, -e, --dry-run, --force, --json, --no-color, etc. // Target-specific flags use platform terminology #[arg(long)] @@ -354,12 +370,27 @@ pub struct SyncAwsArgs { #[arg(long)] pub prefix: Option, } + +// Read-only commands (diff, status) use DiffCommonArgs +#[derive(Parser)] +pub struct DiffAwsArgs { + #[command(flatten)] + pub common: DiffCommonArgs, // -w, -p, -e, --json, --no-color (NO --dry-run, --force) + + #[arg(long)] + pub region: String, + + #[arg(long)] + pub prefix: Option, +} ``` **Rules:** -- `SyncCommonArgs` is shared across all sync targets — never redefine common flags +- `SyncCommonArgs` is for write commands (`sync`) — includes `--dry-run` and `--force` +- `DiffCommonArgs` is for read-only commands (`diff`, `status`) — excludes `--dry-run` and `--force` - Target-specific flags use the platform's own terminology (AWS: `--region`, `--prefix`; Fly: `--app`; Vercel: `--project`, `--team`) -- `zopp diff ` and `zopp sync ` accept identical flags — same struct, different execution path +- Target-specific flags are identical between `sync` and `diff` variants — only the common args differ +- Story specs must explicitly specify which args struct each command variant uses - Platform credentials come from environment variables following platform conventions — never from CLI flags (avoids secrets in shell history) #### Error Handling Pattern for Sync @@ -384,6 +415,7 @@ pub enum SyncError { - Platform-specific API errors are mapped to these variants — never expose raw API error types to the user - The output module formats these into the Error Block component - `SyncError` implements `std::fmt::Display` with the structured format: `Error: [{platform}] {operation} — {message}\n Fix: {fix}` +- **Error classification must use specific, documented SDK/API error patterns — never broad substring matching.** Example: classify connection errors by matching `"dispatch failure"`, `"timeout"`, `"connection refused"`, `"connection reset"`, `"connect error"` — not by checking if message contains `"connection"`. When adding a new sync target, document the specific error patterns used for classification in the module's source. #### Install Script Pattern @@ -419,6 +451,9 @@ pub enum SyncError { 3. **Don't create a "universal sync config" in zopp.toml.** Sync target details (region, app name, project) are CLI flags, not config file entries. This prevents accidental sync to wrong targets. 4. **Don't implement your own diff logic in a sync target.** Use the shared diff engine. If a platform has diff-specific needs, extend the shared engine rather than bypassing it. 5. **Don't log plaintext secret values** during sync — not in debug output, not in error messages, not in JSON output. Log keys only, never values. +6. **Don't silently discard per-item errors in collection fetches.** When fetching multiple items (secrets, configs), return both successes and errors via `FetchResult`. Never return only the successful items. +7. **Don't share write-command args with read-only commands.** Use `DiffCommonArgs` for read-only commands (`diff`, `status`) and `SyncCommonArgs` for write commands (`sync`). Don't let `diff` accept `--dry-run` or `--force`. +8. **Don't use broad substring matching for error classification.** Match specific, documented error patterns from the SDK/API. Broad matches like `"connection"` will misclassify unrelated errors. ## Project Structure & Boundaries diff --git a/_bmad-output/planning-artifacts/sprint-change-proposal-2026-03-10.md b/_bmad-output/planning-artifacts/sprint-change-proposal-2026-03-10.md new file mode 100644 index 00000000..7ad55b39 --- /dev/null +++ b/_bmad-output/planning-artifacts/sprint-change-proposal-2026-03-10.md @@ -0,0 +1,229 @@ +# Sprint Change Proposal — Epic 2 Retrospective Findings + +**Date:** 2026-03-10 +**Trigger:** Epic 2 Retrospective (2026-03-11) +**Change Scope:** Minor — Direct Adjustment +**Status:** Approved + +--- + +## 1. Issue Summary + +Epic 2 (Sync Framework & AWS Secrets Manager Integration) completed successfully with all 5 stories done and merged. During implementation and review, three patterns emerged that weren't captured in the original architecture or story specifications: + +1. **`FetchResult` pattern** — `fetch_current()` needed to return both successful secrets AND per-key errors, not just a HashMap. The original trait signature was `Result, SyncError>`, but review of PR #83 revealed this silently dropped per-key failures. A new `FetchResult { secrets: HashMap, errors: Vec<(String, SyncError)> }` type was introduced. + +2. **`DiffCommonArgs` split** — The original architecture defined a single `SyncCommonArgs` for all sync/diff commands. Review of PR #84 caught that read-only commands (`diff`) inherited write-only flags (`--dry-run`, `--force`). A separate `DiffCommonArgs` was created. + +3. **Error classification specificity** — Broad substring matching (`"connection"`) for error classification misclassified API errors. Specific SDK error patterns (`"dispatch failure"`, `"timeout"`, `"connection refused"`, `"connection reset"`, `"connect error"`) replaced it. + +These patterns are now proven in code but not reflected in the planning artifacts that future stories and agents will reference. + +--- + +## 2. Impact Analysis + +### Epic Impact + +| Epic | Impact | Details | +|------|--------|---------| +| Epic 2 | None | Complete. Patterns already implemented. | +| Epic 3 (Fly) | **Moderate** | Stories 3.1 and 3.2 should reference `FetchResult` and `DiffCommonArgs` patterns from the start. | +| Epics 4-9 | **Low** | Stories not yet created. Patterns will be inherited via updated architecture doc. | + +### Artifact Conflicts + +| Artifact | Conflict | Action Needed | +|----------|----------|---------------| +| **Architecture** | `SyncTarget` trait signature outdated; missing `FetchResult` type, `DiffCommonArgs`, error classification rules | Update | +| **PRD** | No conflict | None | +| **UX Design** | No conflict | None | +| **Epics** | Epic 3 story specs don't exist yet (backlog) — no conflict, but patterns must be incorporated at creation time | Note for SM | +| **Sprint Status** | No conflict | None | + +### Technical Impact + +- No code changes needed — patterns are already implemented in main +- Architecture doc update is documentation-only +- Flaky test GH #85 (`test_cross_environment_isolation` server startup timeout) remains open — intermittent, not blocking + +--- + +## 3. Recommended Approach + +**Selected: Direct Adjustment** + +Update the architecture document to reflect the three patterns that emerged during Epic 2 implementation. No story changes, no PRD changes, no scope adjustment. + +**Rationale:** +- Effort: Low (documentation update only) +- Risk: Low (patterns are already proven in working code) +- Timeline impact: None — can be done before starting Epic 3 +- The alternative (leaving docs outdated) risks future agents implementing the old pattern and requiring the same review fixes + +**Effort estimate:** Low +**Risk level:** Low +**Timeline impact:** None + +--- + +## 4. Detailed Change Proposals + +### Change 1: Update Architecture — SyncTarget Trait Pattern + +**File:** `_bmad-output/planning-artifacts/architecture.md` +**Section:** SyncTarget Trait Pattern (code block around line 267-283) + +OLD: +```rust +#[async_trait] +pub trait SyncTarget { + fn display_name(&self) -> &str; + async fn fetch_current(&self) -> Result, SyncError>; + async fn apply(&self, operations: &[DiffOperation]) -> Vec; +} +``` + +NEW: +```rust +#[async_trait] +pub trait SyncTarget { + fn display_name(&self) -> &str; + async fn fetch_current(&self) -> Result; + async fn apply(&self, operations: &[DiffOperation]) -> Vec; +} +``` + +**Rationale:** `fetch_current()` returns `FetchResult` (containing both `secrets: HashMap` and `errors: Vec<(String, SyncError)>`) instead of a bare HashMap. This ensures per-key errors are surfaced to the caller rather than silently dropped. + +### Change 2: Add FetchResult Type to Architecture — Shared Types + +**File:** `_bmad-output/planning-artifacts/architecture.md` +**Section:** After the SyncTarget trait code block, add: + +ADD (new content): +``` +#### FetchResult Type + +When fetching collections with per-item potential failures, always return both successes AND errors: + +```rust +pub struct FetchResult { + /// Successfully fetched secrets + pub secrets: HashMap, + /// Per-key errors (key name, error) + pub errors: Vec<(String, SyncError)>, +} +``` + +**Rule:** Never silently discard errors from collection-fetching operations. Return both successes and failures — let the caller decide policy. This applies to `fetch_current()` and any future trait method that fetches collections. +``` + +**Rationale:** Team agreement from Epic 2 retro: "Return both successes AND errors from collection-fetching operations — never silently discard." + +### Change 3: Add DiffCommonArgs to Architecture — CLI Command Pattern + +**File:** `_bmad-output/planning-artifacts/architecture.md` +**Section:** CLI Command Pattern for Sync/Diff (around line 343-363) + +OLD: +```rust +#[derive(Parser)] +pub struct SyncAwsArgs { + #[command(flatten)] + pub common: SyncCommonArgs, // -w, -p, -e, --dry-run, --json, --no-color, etc. + #[arg(long)] + pub region: String, + #[arg(long)] + pub prefix: Option, +} +``` + +NEW: +```rust +// Write commands (sync) use SyncCommonArgs +#[derive(Parser)] +pub struct SyncAwsArgs { + #[command(flatten)] + pub common: SyncCommonArgs, // -w, -p, -e, --dry-run, --force, --json, --no-color, etc. + #[arg(long)] + pub region: String, + #[arg(long)] + pub prefix: Option, +} + +// Read-only commands (diff, status) use DiffCommonArgs +#[derive(Parser)] +pub struct DiffAwsArgs { + #[command(flatten)] + pub common: DiffCommonArgs, // -w, -p, -e, --json, --no-color (NO --dry-run, --force) + #[arg(long)] + pub region: String, + #[arg(long)] + pub prefix: Option, +} +``` + +ADD to rules: +- `DiffCommonArgs` is for read-only commands (`diff`, `status`) — excludes `--dry-run` and `--force` +- `SyncCommonArgs` is for write commands (`sync`) — includes `--dry-run` and `--force` +- Story specs must explicitly specify which args struct each command variant uses + +**Rationale:** Discovered in PR #84 review — read-only commands should not accept write-only flags. + +### Change 4: Add Error Classification Rule to Architecture + +**File:** `_bmad-output/planning-artifacts/architecture.md` +**Section:** Error Handling Pattern for Sync (after the SyncError enum, around line 385) + +ADD to rules: +- Error classification must use specific, documented SDK/API error patterns — never broad substring matching +- Example: classify connection errors by matching `"dispatch failure"`, `"timeout"`, `"connection refused"`, `"connection reset"`, `"connect error"` — not by checking if message contains `"connection"` +- When adding a new sync target, document the specific error patterns used for classification in the module's source + +**Rationale:** Broad substring matching in Epic 2's initial AWS client misclassified API errors as connection errors. Specific patterns are more fragile to SDK changes but far less likely to misclassify. + +### Change 5: Update Team Agreements in Architecture + +**File:** `_bmad-output/planning-artifacts/architecture.md` +**Section:** Anti-Patterns to Avoid (after existing list, around line 422) + +ADD new anti-patterns: +6. **Don't silently discard per-item errors in collection fetches.** When fetching multiple items (secrets, configs), return both successes and errors. Never return only the successful items. +7. **Don't share write-command args with read-only commands.** Use `DiffCommonArgs` for read-only commands and `SyncCommonArgs` for write commands. Don't let `diff` accept `--dry-run` or `--force`. +8. **Don't use broad substring matching for error classification.** Match specific, documented error patterns from the SDK/API. Broad matches like `"connection"` will misclassify unrelated errors. + +**Rationale:** Codifies Epic 2 team agreements as anti-patterns for future agents to follow. + +--- + +## 5. Implementation Handoff + +**Change scope: Minor** — Direct implementation by dev team (documentation updates only). + +| # | Action | Owner | Priority | +|---|--------|-------|----------| +| 1 | Apply Changes 1-5 to architecture.md | Dev (Amelia) | Before Epic 3 story creation | +| 2 | When creating Epic 3 stories via SM agent, ensure story specs reference `FetchResult` pattern and `DiffCommonArgs` split | SM (Bob) | During story creation | +| 3 | Address flaky test GH #85 if it recurs | Dev (Amelia) | Low priority | + +**Success criteria:** +- Architecture doc reflects actual implemented `SyncTarget` trait signature +- Architecture doc includes `FetchResult` type documentation +- Architecture doc includes `DiffCommonArgs` vs `SyncCommonArgs` distinction +- Architecture doc includes error classification specificity rule +- Epic 3 Story 3.1 spec references these patterns when created + +**No blocking dependencies.** Changes can be applied immediately. + +--- + +## Approval + +- [x] Change trigger identified and documented +- [x] Impact analysis complete +- [x] Path forward selected with rationale +- [x] Specific edit proposals with before/after +- [x] Implementation handoff defined + +**Verdict:** Minor scope — apply architecture doc updates, then proceed to Epic 3 story creation. diff --git a/apps/zopp-cli/Cargo.toml b/apps/zopp-cli/Cargo.toml index f9a00758..f7866ceb 100644 --- a/apps/zopp-cli/Cargo.toml +++ b/apps/zopp-cli/Cargo.toml @@ -45,7 +45,7 @@ rpassword = { workspace = true } argon2 = { workspace = true } zeroize = { workspace = true } rustls = { version = "0.23", default-features = false, features = ["ring"] } -zopp-sync = { path = "../../crates/zopp-sync", version = "0.1.0", features = ["aws"] } +zopp-sync = { path = "../../crates/zopp-sync", version = "0.1.0", features = ["aws", "fly"] } [target.'cfg(target_os = "macos")'.dependencies] keyring = { workspace = true, features = ["apple-native"] } diff --git a/apps/zopp-cli/src/cli.rs b/apps/zopp-cli/src/cli.rs index df70495e..f492a5e6 100644 --- a/apps/zopp-cli/src/cli.rs +++ b/apps/zopp-cli/src/cli.rs @@ -495,6 +495,15 @@ pub enum SyncCommand { #[arg(long)] prefix: Option, }, + /// Sync secrets to a Fly app + Fly { + #[command(flatten)] + common: SyncCommonArgs, + + /// Fly app name + #[arg(long)] + app: String, + }, } #[derive(Subcommand)] @@ -542,6 +551,15 @@ pub enum DiffCommand { #[arg(long)] prefix: Option, }, + /// Show diff between zopp and a Fly app + Fly { + #[command(flatten)] + common: DiffCommonArgs, + + /// Fly app name + #[arg(long)] + app: String, + }, } #[derive(Subcommand)] @@ -1298,4 +1316,97 @@ mod tests { _ => panic!("Expected Command::Sync"), } } + + #[test] + fn test_sync_fly_parses_all_flags() { + let cli = Cli::try_parse_from([ + "zopp", + "sync", + "fly", + "--app", + "myapp", + "-w", + "myworkspace", + "-p", + "myproject", + "-e", + "production", + "--dry-run", + "--json", + "--no-color", + "--verbose", + "--force", + ]) + .unwrap(); + + match cli.command { + Command::Sync { sync_cmd } => match sync_cmd { + SyncCommand::Fly { common, app } => { + assert_eq!(app, "myapp"); + assert_eq!(common.workspace, Some("myworkspace".to_string())); + assert_eq!(common.project, Some("myproject".to_string())); + assert_eq!(common.environment, Some("production".to_string())); + assert!(common.dry_run); + assert!(common.json); + assert!(common.no_color); + assert!(common.verbose); + assert!(common.force); + } + _ => panic!("Expected SyncCommand::Fly"), + }, + _ => panic!("Expected Command::Sync"), + } + } + + #[test] + fn test_sync_fly_app_required() { + let result = Cli::try_parse_from(["zopp", "sync", "fly"]); + assert!(result.is_err()); + } + + #[test] + fn test_sync_fly_minimal() { + let cli = Cli::try_parse_from(["zopp", "sync", "fly", "--app", "myapp"]).unwrap(); + + match cli.command { + Command::Sync { sync_cmd } => match sync_cmd { + SyncCommand::Fly { common, app } => { + assert_eq!(app, "myapp"); + assert!(common.workspace.is_none()); + assert!(!common.dry_run); + assert!(!common.json); + assert!(!common.force); + } + _ => panic!("Expected SyncCommand::Fly"), + }, + _ => panic!("Expected Command::Sync"), + } + } + + #[test] + fn test_diff_fly_parses() { + let cli = Cli::try_parse_from([ + "zopp", "diff", "fly", "--app", "myapp", "-w", "ws", "-p", "proj", "-e", "staging", + ]) + .unwrap(); + + match cli.command { + Command::Diff { diff_cmd } => match diff_cmd { + DiffCommand::Fly { common, app } => { + assert_eq!(app, "myapp"); + assert_eq!(common.workspace, Some("ws".to_string())); + assert_eq!(common.project, Some("proj".to_string())); + assert_eq!(common.environment, Some("staging".to_string())); + } + _ => panic!("Expected DiffCommand::Fly"), + }, + _ => panic!("Expected Command::Diff"), + } + } + + #[test] + fn test_diff_fly_app_required() { + let result = Cli::try_parse_from(["zopp", "diff", "fly"]); + assert!(result.is_err()); + } } diff --git a/apps/zopp-cli/src/commands/diff_fly.rs b/apps/zopp-cli/src/commands/diff_fly.rs new file mode 100644 index 00000000..8385e65d --- /dev/null +++ b/apps/zopp-cli/src/commands/diff_fly.rs @@ -0,0 +1,180 @@ +use std::collections::HashMap; + +use crate::crypto::fetch_and_decrypt_secrets; +use crate::grpc::setup_client; +use crate::output::{ + diff_item, diff_summary, error_block, exit_codes, header, output_json, DiffCommonArgs, + DiffJsonChange, DiffJsonOutput, DiffJsonSummary, +}; +use zopp_sync::fly::FlySyncTarget; +use zopp_sync::{DiffOperation, SyncTarget}; + +pub async fn cmd_diff_fly( + server: &str, + tls_ca_cert: Option<&std::path::Path>, + common: &DiffCommonArgs, + app: &str, +) -> i32 { + let config = common.to_output_config(); + + // 1. Resolve workspace/project/environment + let (workspace, project, environment) = match crate::config::resolve_context( + common.workspace.as_ref(), + common.project.as_ref(), + common.environment.as_ref(), + ) { + Ok(ctx) => ctx, + Err(e) => { + error_block( + &config, + "zopp config", + &e.to_string(), + "Check zopp.toml or pass -w, -p, -e flags", + ); + return exit_codes::CONFIG_ERROR; + } + }; + + // 2. Fetch and decrypt zopp secrets + let (mut client, principal, secrets) = match setup_client(server, tls_ca_cert).await { + Ok(c) => c, + Err(e) => { + error_block( + &config, + "zopp connection", + &e.to_string(), + "Check server address and credentials", + ); + return exit_codes::CONNECTION_ERROR; + } + }; + + let zopp_secrets = match fetch_and_decrypt_secrets( + &mut client, + &principal, + &secrets, + &workspace, + &project, + &environment, + ) + .await + { + Ok(s) => s, + Err(e) => { + error_block( + &config, + "zopp secrets", + &e.to_string(), + "Verify workspace, project, and environment exist", + ); + return exit_codes::CONFIG_ERROR; + } + }; + + let source_label = format!("zopp/{workspace}/{project}/{environment}"); + let zopp_map: HashMap = zopp_secrets.into_iter().collect(); + + // 3. Create Fly sync target + let target = match FlySyncTarget::new(app.to_string()) { + Ok(t) => t, + Err(e) => { + error_block(&config, e.platform(), &e.to_string(), e.fix()); + return match &e { + zopp_sync::SyncError::AuthError { .. } => exit_codes::CONFIG_ERROR, + zopp_sync::SyncError::ConnectionError { .. } => exit_codes::CONNECTION_ERROR, + _ => exit_codes::TOTAL_FAILURE, + }; + } + }; + + let target_label = target.display_name().to_string(); + + // 4. Fetch current state from Fly + let fetch_result = match target.fetch_current().await { + Ok(s) => s, + Err(e) => { + error_block(&config, e.platform(), &e.to_string(), e.fix()); + return match &e { + zopp_sync::SyncError::AuthError { .. } => exit_codes::CONFIG_ERROR, + zopp_sync::SyncError::ConnectionError { .. } => exit_codes::CONNECTION_ERROR, + _ => exit_codes::TOTAL_FAILURE, + }; + } + }; + + // Report per-key fetch errors + let has_fetch_errors = !fetch_result.errors.is_empty(); + for (key, err) in &fetch_result.errors { + error_block( + &config, + "Fly", + &format!("Failed to fetch secret '{key}': {err}"), + "Check app permissions in Fly", + ); + } + + // 5. Compute diff + // Note: Fly secrets are write-only, so values from fetch are empty strings. + // All overlapping keys show as updates since we can't compare values. + // This is correct behavior — we always overwrite to ensure Fly has current values. + let operations = zopp_sync::diff(&zopp_map, &fetch_result.secrets); + + // 6. Display diff + header(&config, "Diff", &source_label, &target_label); + + let mut adds = 0usize; + let mut updates = 0usize; + let mut removes = 0usize; + + for op in &operations { + match op { + DiffOperation::Add { .. } => { + diff_item(&config, '+', op.key()); + adds += 1; + } + DiffOperation::Update { .. } => { + diff_item(&config, '~', op.key()); + updates += 1; + } + DiffOperation::Remove { .. } => { + diff_item(&config, '-', op.key()); + removes += 1; + } + } + } + + diff_summary(&config, adds, updates, removes); + + if config.json { + let json_output = DiffJsonOutput { + command: "diff".into(), + target: target_label, + source: source_label, + changes: operations + .iter() + .map(|op| DiffJsonChange { + key: op.key().to_string(), + operation: match op { + DiffOperation::Add { .. } => "add", + DiffOperation::Update { .. } => "update", + DiffOperation::Remove { .. } => "remove", + } + .into(), + }) + .collect(), + summary: DiffJsonSummary { + adds, + updates, + removes, + total: adds + updates + removes, + }, + }; + output_json(&json_output); + } + + if has_fetch_errors { + exit_codes::PARTIAL_FAILURE + } else { + exit_codes::SUCCESS + } +} diff --git a/apps/zopp-cli/src/commands/mod.rs b/apps/zopp-cli/src/commands/mod.rs index 3bfddc36..e637d621 100644 --- a/apps/zopp-cli/src/commands/mod.rs +++ b/apps/zopp-cli/src/commands/mod.rs @@ -1,6 +1,7 @@ pub mod audit; pub mod diff; pub mod diff_aws; +pub mod diff_fly; pub mod environment; pub mod group; pub mod invite; @@ -11,12 +12,14 @@ pub mod project; pub mod secret; pub mod sync; pub mod sync_aws; +pub mod sync_fly; pub mod sync_status; pub mod workspace; pub use audit::{cmd_audit_count, cmd_audit_get, cmd_audit_list}; pub use diff::cmd_diff_k8s; pub use diff_aws::cmd_diff_aws; +pub use diff_fly::cmd_diff_fly; pub use environment::{ cmd_environment_create, cmd_environment_delete, cmd_environment_get, cmd_environment_list, }; @@ -48,6 +51,7 @@ pub use secret::{ }; pub use sync::cmd_sync_k8s; pub use sync_aws::cmd_sync_aws; +pub use sync_fly::cmd_sync_fly; pub use sync_status::cmd_sync_status; pub use workspace::{ cmd_workspace_create, cmd_workspace_grant_principal_access, cmd_workspace_list, diff --git a/apps/zopp-cli/src/commands/sync_fly.rs b/apps/zopp-cli/src/commands/sync_fly.rs new file mode 100644 index 00000000..41ff91e8 --- /dev/null +++ b/apps/zopp-cli/src/commands/sync_fly.rs @@ -0,0 +1,261 @@ +use std::collections::HashMap; + +use crate::crypto::fetch_and_decrypt_secrets; +use crate::grpc::setup_client; +use crate::output::{ + diff_item, diff_summary, error_block, exit_codes, header, output_json, per_item_failure, + per_item_success, summary, DiffJsonChange, DiffJsonOutput, DiffJsonSummary, SyncCommonArgs, + SyncJsonOutput, SyncJsonResult, SyncJsonSummary, +}; +use zopp_sync::fly::FlySyncTarget; +use zopp_sync::{DiffOperation, SyncOutcome, SyncTarget}; + +pub async fn cmd_sync_fly( + server: &str, + tls_ca_cert: Option<&std::path::Path>, + common: &SyncCommonArgs, + app: &str, +) -> i32 { + let config = common.to_output_config(); + + // 1. Resolve workspace/project/environment + let (workspace, project, environment) = match crate::config::resolve_context( + common.workspace.as_ref(), + common.project.as_ref(), + common.environment.as_ref(), + ) { + Ok(ctx) => ctx, + Err(e) => { + error_block( + &config, + "zopp config", + &e.to_string(), + "Check zopp.toml or pass -w, -p, -e flags", + ); + return exit_codes::CONFIG_ERROR; + } + }; + + // 2. Fetch and decrypt zopp secrets + let (mut client, principal, secrets) = match setup_client(server, tls_ca_cert).await { + Ok(c) => c, + Err(e) => { + error_block( + &config, + "zopp connection", + &e.to_string(), + "Check server address and credentials", + ); + return exit_codes::CONNECTION_ERROR; + } + }; + + let zopp_secrets = match fetch_and_decrypt_secrets( + &mut client, + &principal, + &secrets, + &workspace, + &project, + &environment, + ) + .await + { + Ok(s) => s, + Err(e) => { + error_block( + &config, + "zopp secrets", + &e.to_string(), + "Verify workspace, project, and environment exist", + ); + return exit_codes::CONFIG_ERROR; + } + }; + + let source_label = format!("zopp/{workspace}/{project}/{environment}"); + let zopp_map: HashMap = zopp_secrets.into_iter().collect(); + + // 3. Create Fly sync target + let target = match FlySyncTarget::new(app.to_string()) { + Ok(t) => t, + Err(e) => { + error_block(&config, e.platform(), &e.to_string(), e.fix()); + return match &e { + zopp_sync::SyncError::AuthError { .. } => exit_codes::CONFIG_ERROR, + zopp_sync::SyncError::ConnectionError { .. } => exit_codes::CONNECTION_ERROR, + _ => exit_codes::TOTAL_FAILURE, + }; + } + }; + + let target_label = target.display_name().to_string(); + + // 4. Fetch current state from Fly + let fetch_result = match target.fetch_current().await { + Ok(s) => s, + Err(e) => { + error_block(&config, e.platform(), &e.to_string(), e.fix()); + return match &e { + zopp_sync::SyncError::AuthError { .. } => exit_codes::CONFIG_ERROR, + zopp_sync::SyncError::ConnectionError { .. } => exit_codes::CONNECTION_ERROR, + _ => exit_codes::TOTAL_FAILURE, + }; + } + }; + + // Report per-key fetch errors + let fetch_errors = fetch_result.errors.len(); + for (key, err) in &fetch_result.errors { + error_block( + &config, + "Fly", + &format!("Failed to fetch secret '{key}': {err}"), + "Check app permissions in Fly", + ); + } + + // 5. Compute diff + // Note: Fly secrets are write-only (values are empty strings from fetch). + // All overlapping keys show as updates since we can't compare values. + // This is correct behavior — we always overwrite to ensure Fly has current values. + let operations = zopp_sync::diff(&zopp_map, &fetch_result.secrets); + + // 6. Dry-run or no changes: show diff output + if common.dry_run || operations.is_empty() { + header(&config, "Diff", &source_label, &target_label); + + let mut adds = 0usize; + let mut updates = 0usize; + let mut removes = 0usize; + + for op in &operations { + match op { + DiffOperation::Add { .. } => { + diff_item(&config, '+', op.key()); + adds += 1; + } + DiffOperation::Update { .. } => { + diff_item(&config, '~', op.key()); + updates += 1; + } + DiffOperation::Remove { .. } => { + diff_item(&config, '-', op.key()); + removes += 1; + } + } + } + + diff_summary(&config, adds, updates, removes); + + if config.json { + let command = if common.dry_run { + "sync --dry-run" + } else { + "sync" + }; + let json_output = DiffJsonOutput { + command: command.into(), + target: target_label, + source: source_label, + changes: operations + .iter() + .map(|op| DiffJsonChange { + key: op.key().to_string(), + operation: match op { + DiffOperation::Add { .. } => "add", + DiffOperation::Update { .. } => "update", + DiffOperation::Remove { .. } => "remove", + } + .into(), + }) + .collect(), + summary: DiffJsonSummary { + adds, + updates, + removes, + total: adds + updates + removes, + }, + }; + output_json(&json_output); + } + + return if fetch_errors > 0 { + exit_codes::PARTIAL_FAILURE + } else { + exit_codes::SUCCESS + }; + } + + // 7. Apply changes + header(&config, "Syncing", &source_label, &target_label); + + let results = target.apply(&operations).await; + let total = results.len(); + let mut succeeded = 0usize; + let mut failed = 0usize; + + let mut json_results = Vec::new(); + + for result in &results { + match &result.outcome { + SyncOutcome::Success => { + succeeded += 1; + let action = operation_action(&operations, &result.key); + per_item_success(&config, &result.key, action); + if config.json { + json_results.push(SyncJsonResult { + key: result.key.clone(), + status: "synced".into(), + error: None, + fix: None, + }); + } + } + SyncOutcome::Failed { reason } => { + failed += 1; + per_item_failure(&config, &result.key, reason, None); + if config.json { + json_results.push(SyncJsonResult { + key: result.key.clone(), + status: "failed".into(), + error: Some(reason.clone()), + fix: None, + }); + } + } + } + } + + summary(&config, total, succeeded, failed, &target_label); + + if config.json { + let json_output = SyncJsonOutput { + command: "sync".into(), + target: target_label, + source: source_label, + results: json_results, + summary: SyncJsonSummary { + total, + synced: succeeded, + failed, + }, + }; + output_json(&json_output); + } + + exit_codes::from_results(total + fetch_errors, failed + fetch_errors) +} + +/// Look up the operation type for a given key to produce a human-readable action word. +fn operation_action(operations: &[DiffOperation], key: &str) -> &'static str { + for op in operations { + if op.key() == key { + return match op { + DiffOperation::Add { .. } => "created", + DiffOperation::Update { .. } => "updated", + DiffOperation::Remove { .. } => "deleted", + }; + } + } + "synced" +} diff --git a/apps/zopp-cli/src/main.rs b/apps/zopp-cli/src/main.rs index 3555a046..07061a52 100644 --- a/apps/zopp-cli/src/main.rs +++ b/apps/zopp-cli/src/main.rs @@ -433,6 +433,13 @@ async fn main() -> Result<(), Box> { std::process::exit(exit_code); } } + SyncCommand::Fly { common, app } => { + let exit_code = + cmd_sync_fly(&cli.server, cli.tls_ca_cert.as_deref(), &common, &app).await; + if exit_code != 0 { + std::process::exit(exit_code); + } + } }, Command::Permission { permission_cmd } => match permission_cmd { PermissionCommand::Set { @@ -1079,6 +1086,13 @@ async fn main() -> Result<(), Box> { std::process::exit(exit_code); } } + DiffCommand::Fly { common, app } => { + let exit_code = + cmd_diff_fly(&cli.server, cli.tls_ca_cert.as_deref(), &common, &app).await; + if exit_code != 0 { + std::process::exit(exit_code); + } + } }, Command::Run { workspace, diff --git a/apps/zopp-web/playwright.config.ts b/apps/zopp-web/playwright.config.ts index 3fa42504..6aadd3e8 100644 --- a/apps/zopp-web/playwright.config.ts +++ b/apps/zopp-web/playwright.config.ts @@ -21,6 +21,6 @@ export default defineConfig({ command: 'npm run dev', url: 'http://localhost:3000', reuseExistingServer: !process.env.CI, - timeout: 120 * 1000, // 2 minutes for WASM build + timeout: 300 * 1000, // 5 minutes for WASM build on slow CI runners }, }); diff --git a/crates/zopp-sync/Cargo.toml b/crates/zopp-sync/Cargo.toml index 0d9d8ce9..db1eff13 100644 --- a/crates/zopp-sync/Cargo.toml +++ b/crates/zopp-sync/Cargo.toml @@ -11,7 +11,7 @@ description = "Sync framework for zopp — SyncTarget trait, DiffEngine, and sha default = [] aws = ["dep:aws-sdk-secretsmanager", "dep:aws-config", "dep:tokio"] gcp = [] -fly = [] +fly = ["dep:reqwest", "dep:zeroize", "dep:tokio"] vercel = [] render = [] railway = [] @@ -20,10 +20,12 @@ railway = [] async-trait.workspace = true aws-config = { workspace = true, optional = true } aws-sdk-secretsmanager = { workspace = true, optional = true } +reqwest = { workspace = true, features = ["json"], optional = true } serde = { workspace = true, features = ["derive"] } serde_json.workspace = true thiserror.workspace = true tokio = { workspace = true, optional = true } +zeroize = { workspace = true, optional = true } [dev-dependencies] tokio = { workspace = true, features = ["rt-multi-thread", "macros"] } diff --git a/crates/zopp-sync/src/fly/client.rs b/crates/zopp-sync/src/fly/client.rs new file mode 100644 index 00000000..84fc8111 --- /dev/null +++ b/crates/zopp-sync/src/fly/client.rs @@ -0,0 +1,306 @@ +use std::time::Duration; + +use zeroize::{Zeroize, ZeroizeOnDrop}; + +use crate::SyncError; + +const PLATFORM: &str = "Fly"; +const BASE_URL: &str = "https://api.machines.dev"; + +/// Maximum number of retries for transient errors (429, 5xx). +const MAX_RETRIES: u32 = 3; + +/// Base delay for exponential backoff (doubles each retry: 1s, 2s, 4s). +const BASE_BACKOFF_MS: u64 = 1000; + +/// Represents a secret entry from the Fly API (name only — Fly secrets are write-only). +pub(crate) struct SecretEntry { + pub label: String, +} + +/// Thin abstraction over the Fly Machines API. +/// +/// This trait is internal to the `fly` module and exists solely for testability. +/// The real implementation uses `reqwest`; tests provide a mock. +#[async_trait::async_trait] +pub(crate) trait FlyApi: Send + Sync { + /// List all secret labels for an app (Fly does NOT return values). + async fn list_secrets(&self, app: &str) -> Result, SyncError>; + + /// Set (create or update) secrets on an app. Fly's API is batch-oriented. + async fn set_secrets(&self, app: &str, secrets: &[(&str, &str)]) -> Result<(), SyncError>; + + /// Unset (delete) a single secret from an app. + async fn unset_secret(&self, app: &str, label: &str) -> Result<(), SyncError>; +} + +/// Sensitive token wrapper that is zeroized on drop. +#[derive(Zeroize, ZeroizeOnDrop)] +struct ApiToken(String); + +/// Real Fly API client using `reqwest`. +pub(crate) struct FlyClient { + http: reqwest::Client, + token: ApiToken, +} + +impl FlyClient { + pub fn new(token: String) -> Self { + Self { + http: reqwest::Client::new(), + token: ApiToken(token), + } + } +} + +/// Percent-encode a path segment for safe URL construction. +fn encode_path_segment(s: &str) -> String { + // Encode everything except unreserved characters (RFC 3986) + let mut encoded = String::with_capacity(s.len()); + for byte in s.bytes() { + match byte { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + encoded.push(byte as char); + } + _ => { + encoded.push_str(&format!("%{byte:02X}")); + } + } + } + encoded +} + +/// Check if an HTTP status code is retryable (429 rate limit or 5xx server error). +fn is_retryable(status: reqwest::StatusCode) -> bool { + status == reqwest::StatusCode::TOO_MANY_REQUESTS || status.is_server_error() +} + +/// Map an HTTP response status to a `SyncError`. +fn map_http_error(status: reqwest::StatusCode, body: &str, operation: &str) -> SyncError { + match status.as_u16() { + 401 => SyncError::AuthError { + platform: PLATFORM.into(), + message: format!("HTTP 401 Unauthorized: {body}"), + fix: "Check FLY_API_TOKEN. Generate one at https://fly.io/user/personal_access_tokens" + .into(), + }, + 403 => SyncError::AuthError { + platform: PLATFORM.into(), + message: format!("HTTP 403 Forbidden: {body}"), + fix: "Your Fly API token lacks permission for this app. \ + Verify the token has access to the target organization and app." + .into(), + }, + 404 => SyncError::ApiError { + platform: PLATFORM.into(), + operation: operation.into(), + message: format!("HTTP 404 Not Found: {body}"), + fix: "Verify the app name is correct and exists in your Fly organization.".into(), + }, + 429 => SyncError::ApiError { + platform: PLATFORM.into(), + operation: operation.into(), + message: "Rate limited by Fly API".into(), + fix: "Wait a moment and retry. The Fly API has rate limits.".into(), + }, + 500..=599 => SyncError::ApiError { + platform: PLATFORM.into(), + operation: operation.into(), + message: format!("HTTP {status}: {body}"), + fix: "Fly API server error. Retry in a few moments.".into(), + }, + _ => SyncError::ApiError { + platform: PLATFORM.into(), + operation: operation.into(), + message: format!("HTTP {status}: {body}"), + fix: "Check Fly API documentation for this error.".into(), + }, + } +} + +/// Map a reqwest transport error to a `SyncError`. +fn map_reqwest_error(err: &reqwest::Error) -> SyncError { + if err.is_timeout() { + return SyncError::ConnectionError { + platform: PLATFORM.into(), + message: format!("Request timed out: {err}"), + fix: "Check your network connection and try again.".into(), + }; + } + if err.is_connect() { + return SyncError::ConnectionError { + platform: PLATFORM.into(), + message: format!("Connection failed: {err}"), + fix: "Check your network connection. Fly API is at api.machines.dev.".into(), + }; + } + SyncError::ConnectionError { + platform: PLATFORM.into(), + message: format!("{err}"), + fix: "Check your network connection and try again.".into(), + } +} + +/// Send an HTTP request with retry logic for transient errors (429, 5xx). +/// Returns the response on success, or the last error on exhaustion. +async fn send_with_retry( + builder_fn: impl Fn() -> reqwest::RequestBuilder, +) -> Result { + let mut last_error = None; + + for attempt in 0..=MAX_RETRIES { + let resp = builder_fn() + .send() + .await + .map_err(|e| map_reqwest_error(&e))?; + + if resp.status().is_success() || !is_retryable(resp.status()) { + return Ok(resp); + } + + // Retryable error — save for potential final error, then backoff + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + + if attempt < MAX_RETRIES { + let delay = Duration::from_millis(BASE_BACKOFF_MS * 2u64.pow(attempt)); + tokio::time::sleep(delay).await; + } + + last_error = Some((status, body)); + } + + // All retries exhausted + let (status, body) = last_error.unwrap(); + Err(map_http_error(status, &body, "request")) +} + +#[async_trait::async_trait] +impl FlyApi for FlyClient { + async fn list_secrets(&self, app: &str) -> Result, SyncError> { + let app_encoded = encode_path_segment(app); + let url = format!("{BASE_URL}/v1/apps/{app_encoded}/secrets"); + + let resp = send_with_retry(|| self.http.get(&url).bearer_auth(&self.token.0)).await?; + + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(map_http_error(status, &body, "list_secrets")); + } + + // Fly returns an array of objects with "Label" and "Type" fields. + let entries: Vec = + resp.json().await.map_err(|e| SyncError::ApiError { + platform: PLATFORM.into(), + operation: "list_secrets".into(), + message: format!("Failed to parse response: {e}"), + fix: "This may indicate a Fly API change. Check Fly API docs.".into(), + })?; + + Ok(entries + .into_iter() + .filter_map(|v| { + v.get("Label") + .and_then(|l| l.as_str()) + .map(|l| SecretEntry { + label: l.to_string(), + }) + }) + .collect()) + } + + async fn set_secrets(&self, app: &str, secrets: &[(&str, &str)]) -> Result<(), SyncError> { + let app_encoded = encode_path_segment(app); + let url = format!("{BASE_URL}/v1/apps/{app_encoded}/secrets"); + + let body: Vec = secrets + .iter() + .map(|(label, value)| { + serde_json::json!({ + "label": label, + "type": "opaque", + "value": value, + }) + }) + .collect(); + + let resp = + send_with_retry(|| self.http.post(&url).bearer_auth(&self.token.0).json(&body)).await?; + + let status = resp.status(); + if !status.is_success() { + let body_text = resp.text().await.unwrap_or_default(); + return Err(map_http_error(status, &body_text, "set_secrets")); + } + + Ok(()) + } + + async fn unset_secret(&self, app: &str, label: &str) -> Result<(), SyncError> { + let app_encoded = encode_path_segment(app); + let label_encoded = encode_path_segment(label); + let url = format!("{BASE_URL}/v1/apps/{app_encoded}/secrets/{label_encoded}"); + + let resp = send_with_retry(|| self.http.delete(&url).bearer_auth(&self.token.0)).await?; + + let status = resp.status(); + if !status.is_success() { + let body = resp.text().await.unwrap_or_default(); + return Err(map_http_error(status, &body, "unset_secret")); + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn encode_path_segment_simple() { + assert_eq!(encode_path_segment("myapp"), "myapp"); + } + + #[test] + fn encode_path_segment_special_chars() { + assert_eq!(encode_path_segment("my/app"), "my%2Fapp"); + assert_eq!(encode_path_segment("my app"), "my%20app"); + } + + #[test] + fn encode_path_segment_preserves_unreserved() { + assert_eq!(encode_path_segment("a-b_c.d~e"), "a-b_c.d~e"); + } + + #[test] + fn is_retryable_429() { + assert!(is_retryable(reqwest::StatusCode::TOO_MANY_REQUESTS)); + } + + #[test] + fn is_retryable_500() { + assert!(is_retryable(reqwest::StatusCode::INTERNAL_SERVER_ERROR)); + } + + #[test] + fn is_retryable_502() { + assert!(is_retryable(reqwest::StatusCode::BAD_GATEWAY)); + } + + #[test] + fn not_retryable_400() { + assert!(!is_retryable(reqwest::StatusCode::BAD_REQUEST)); + } + + #[test] + fn not_retryable_401() { + assert!(!is_retryable(reqwest::StatusCode::UNAUTHORIZED)); + } + + #[test] + fn not_retryable_200() { + assert!(!is_retryable(reqwest::StatusCode::OK)); + } +} diff --git a/crates/zopp-sync/src/fly/mod.rs b/crates/zopp-sync/src/fly/mod.rs new file mode 100644 index 00000000..9c4e9951 --- /dev/null +++ b/crates/zopp-sync/src/fly/mod.rs @@ -0,0 +1,394 @@ +//! Fly sync target implementation. +//! +//! Requires the `fly` feature flag. Credentials are read from the +//! `FLY_API_TOKEN` environment variable. +//! +//! **Important:** Fly secrets are write-only — the API returns secret labels +//! but not their values. This means `fetch_current()` returns only key names +//! (with empty-string values). The diff engine will see every zopp secret as +//! an "update" since values can't be compared. Use `--force` to push all +//! values, or rely on add/remove detection for key-level changes. + +mod client; + +use std::collections::HashMap; + +use crate::{DiffOperation, FetchResult, SyncError, SyncOutcome, SyncResult, SyncTarget}; +use client::{FlyApi, FlyClient}; + +/// Sync target for Fly apps. +/// +/// Construct via [`FlySyncTarget::new`], which reads `FLY_API_TOKEN` +/// from the environment. +pub struct FlySyncTarget { + api: Box, + app: String, + display_name: String, +} + +impl FlySyncTarget { + /// Create a new Fly sync target. + /// + /// Reads `FLY_API_TOKEN` from the environment. Returns `SyncError::AuthError` + /// if the token is not set. + pub fn new(app: String) -> Result { + let token = std::env::var("FLY_API_TOKEN").map_err(|_| SyncError::AuthError { + platform: "Fly".into(), + message: "FLY_API_TOKEN not set".into(), + fix: "Set FLY_API_TOKEN. Generate one at https://fly.io/user/personal_access_tokens" + .into(), + })?; + + let display_name = format!("Fly ({app})"); + Ok(Self { + api: Box::new(FlyClient::new(token)), + app, + display_name, + }) + } + + /// Create from an existing API implementation (for testing). + #[cfg(test)] + fn from_api(api: Box, app: &str) -> Self { + Self { + api, + app: app.to_string(), + display_name: format!("Fly ({app})"), + } + } +} + +#[async_trait::async_trait] +impl SyncTarget for FlySyncTarget { + fn display_name(&self) -> &str { + &self.display_name + } + + async fn fetch_current(&self) -> Result { + let entries = self.api.list_secrets(&self.app).await?; + + // Fly only returns labels, not values. We store empty strings as + // placeholders so the diff engine can detect add/remove operations. + let secrets: HashMap = entries + .into_iter() + .map(|entry| (entry.label, String::new())) + .collect(); + + Ok(FetchResult { + secrets, + errors: Vec::new(), + }) + } + + async fn apply(&self, operations: &[DiffOperation]) -> Vec { + let mut results = Vec::with_capacity(operations.len()); + + // Batch all adds and updates into a single set_secrets call. + let to_set: Vec<(&str, &str)> = operations + .iter() + .filter_map(|op| match op { + DiffOperation::Add { key, value } => Some((key.as_str(), value.as_str())), + DiffOperation::Update { key, new_value, .. } => { + Some((key.as_str(), new_value.as_str())) + } + DiffOperation::Remove { .. } => None, + }) + .collect(); + + // Apply adds/updates as a batch + let batch_result = if !to_set.is_empty() { + self.api.set_secrets(&self.app, &to_set).await + } else { + Ok(()) + }; + + // Record results for adds/updates + for op in operations { + match op { + DiffOperation::Add { key, .. } | DiffOperation::Update { key, .. } => { + let outcome = match &batch_result { + Ok(()) => SyncOutcome::Success, + Err(e) => SyncOutcome::Failed { + reason: e.to_string(), + }, + }; + results.push(SyncResult { + key: key.clone(), + outcome, + }); + } + DiffOperation::Remove { .. } => {} // handled below + } + } + + // Process removes individually + for op in operations { + if let DiffOperation::Remove { key } = op { + let outcome = match self.api.unset_secret(&self.app, key).await { + Ok(()) => SyncOutcome::Success, + Err(e) => SyncOutcome::Failed { + reason: e.to_string(), + }, + }; + results.push(SyncResult { + key: key.clone(), + outcome, + }); + } + } + + results + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Mock implementation of FlyApi for testing. + struct MockApi { + labels: std::sync::Mutex>, + fail_on_set: std::sync::Mutex, + fail_on_unset: std::sync::Mutex>, + } + + impl MockApi { + fn new(labels: Vec) -> Self { + Self { + labels: std::sync::Mutex::new(labels), + fail_on_set: std::sync::Mutex::new(false), + fail_on_unset: std::sync::Mutex::new(None), + } + } + + fn with_set_failure(self) -> Self { + *self.fail_on_set.lock().unwrap() = true; + self + } + + fn with_unset_failure(self, key: &str) -> Self { + *self.fail_on_unset.lock().unwrap() = Some(key.to_string()); + self + } + } + + #[async_trait::async_trait] + impl FlyApi for MockApi { + async fn list_secrets(&self, _app: &str) -> Result, SyncError> { + let labels = self.labels.lock().unwrap(); + Ok(labels + .iter() + .map(|l| client::SecretEntry { label: l.clone() }) + .collect()) + } + + async fn set_secrets(&self, _app: &str, secrets: &[(&str, &str)]) -> Result<(), SyncError> { + if *self.fail_on_set.lock().unwrap() { + return Err(SyncError::ApiError { + platform: "Fly".into(), + operation: "set_secrets".into(), + message: "Batch set failed".into(), + fix: String::new(), + }); + } + let mut labels = self.labels.lock().unwrap(); + for (key, _) in secrets { + if !labels.contains(&key.to_string()) { + labels.push(key.to_string()); + } + } + Ok(()) + } + + async fn unset_secret(&self, _app: &str, label: &str) -> Result<(), SyncError> { + if let Some(fail_key) = self.fail_on_unset.lock().unwrap().as_ref() { + if label == fail_key { + return Err(SyncError::ApiError { + platform: "Fly".into(), + operation: "unset_secret".into(), + message: format!("Failed to unset '{label}'"), + fix: String::new(), + }); + } + } + let mut labels = self.labels.lock().unwrap(); + labels.retain(|l| l != label); + Ok(()) + } + } + + /// Mock that always fails with AuthError on list. + struct AuthFailApi; + + #[async_trait::async_trait] + impl FlyApi for AuthFailApi { + async fn list_secrets(&self, _app: &str) -> Result, SyncError> { + Err(SyncError::AuthError { + platform: "Fly".into(), + message: "FLY_API_TOKEN invalid".into(), + fix: + "Set FLY_API_TOKEN. Generate one at https://fly.io/user/personal_access_tokens" + .into(), + }) + } + + async fn set_secrets( + &self, + _app: &str, + _secrets: &[(&str, &str)], + ) -> Result<(), SyncError> { + Err(SyncError::AuthError { + platform: "Fly".into(), + message: "FLY_API_TOKEN invalid".into(), + fix: String::new(), + }) + } + + async fn unset_secret(&self, _app: &str, _label: &str) -> Result<(), SyncError> { + Err(SyncError::AuthError { + platform: "Fly".into(), + message: "FLY_API_TOKEN invalid".into(), + fix: String::new(), + }) + } + } + + #[tokio::test] + async fn fetch_current_returns_labels_with_empty_values() { + let api = MockApi::new(vec!["DB_HOST".into(), "DB_PORT".into()]); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let result = target.fetch_current().await.unwrap(); + assert_eq!(result.secrets.len(), 2); + assert_eq!(result.secrets["DB_HOST"], ""); + assert_eq!(result.secrets["DB_PORT"], ""); + assert!(result.errors.is_empty()); + } + + #[tokio::test] + async fn fetch_current_empty_app() { + let api = MockApi::new(vec![]); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let result = target.fetch_current().await.unwrap(); + assert!(result.secrets.is_empty()); + assert!(result.errors.is_empty()); + } + + #[tokio::test] + async fn fetch_current_auth_error() { + let api = AuthFailApi; + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let result = target.fetch_current().await; + assert!(result.is_err()); + match result.unwrap_err() { + SyncError::AuthError { platform, .. } => { + assert_eq!(platform, "Fly"); + } + other => panic!("Expected AuthError, got: {other:?}"), + } + } + + #[tokio::test] + async fn apply_creates_updates_deletes() { + let api = MockApi::new(vec!["EXISTING".into(), "TO_DELETE".into()]); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let ops = vec![ + DiffOperation::Add { + key: "NEW_KEY".to_string(), + value: "new-value".to_string(), + }, + DiffOperation::Update { + key: "EXISTING".to_string(), + old_value: String::new(), + new_value: "updated-value".to_string(), + }, + DiffOperation::Remove { + key: "TO_DELETE".to_string(), + }, + ]; + + let results = target.apply(&ops).await; + assert_eq!(results.len(), 3); + + // Adds/updates come first (batched), then removes + assert_eq!(results[0].key, "NEW_KEY"); + assert_eq!(results[0].outcome, SyncOutcome::Success); + assert_eq!(results[1].key, "EXISTING"); + assert_eq!(results[1].outcome, SyncOutcome::Success); + assert_eq!(results[2].key, "TO_DELETE"); + assert_eq!(results[2].outcome, SyncOutcome::Success); + } + + #[tokio::test] + async fn apply_batch_set_failure() { + let api = MockApi::new(vec![]).with_set_failure(); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let ops = vec![ + DiffOperation::Add { + key: "KEY_A".to_string(), + value: "val-a".to_string(), + }, + DiffOperation::Add { + key: "KEY_B".to_string(), + value: "val-b".to_string(), + }, + ]; + + let results = target.apply(&ops).await; + assert_eq!(results.len(), 2); + assert!(matches!(results[0].outcome, SyncOutcome::Failed { .. })); + assert!(matches!(results[1].outcome, SyncOutcome::Failed { .. })); + } + + #[tokio::test] + async fn apply_unset_failure() { + let api = MockApi::new(vec!["GOOD".into(), "FAIL".into()]).with_unset_failure("FAIL"); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let ops = vec![ + DiffOperation::Remove { + key: "GOOD".to_string(), + }, + DiffOperation::Remove { + key: "FAIL".to_string(), + }, + ]; + + let results = target.apply(&ops).await; + assert_eq!(results.len(), 2); + assert_eq!(results[0].key, "GOOD"); + assert_eq!(results[0].outcome, SyncOutcome::Success); + assert_eq!(results[1].key, "FAIL"); + assert!(matches!(results[1].outcome, SyncOutcome::Failed { .. })); + } + + #[tokio::test] + async fn apply_empty_operations() { + let api = MockApi::new(vec![]); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + + let results = target.apply(&[]).await; + assert!(results.is_empty()); + } + + #[tokio::test] + async fn display_name_returns_fly_app() { + let api = MockApi::new(vec![]); + let target = FlySyncTarget::from_api(Box::new(api), "myapp"); + assert_eq!(target.display_name(), "Fly (myapp)"); + } + + #[test] + fn no_fly_types_leak() { + // Compile-time check — FlySyncTarget's public API only uses + // types from crate::types and crate::error, not reqwest types. + fn _assert_sync_target() {} + fn _assert_fly_is_sync_target() { + _assert_sync_target::(); + } + } +} diff --git a/crates/zopp-sync/src/lib.rs b/crates/zopp-sync/src/lib.rs index a1c01ee6..06c9ae3c 100644 --- a/crates/zopp-sync/src/lib.rs +++ b/crates/zopp-sync/src/lib.rs @@ -8,6 +8,8 @@ pub mod aws; pub mod diff; pub mod error; +#[cfg(feature = "fly")] +pub mod fly; pub mod types; pub use diff::diff;