fix(build): repair team-sync + enterprise feature rot and gate them in CI - #424
Conversation
…n CI The `team-sync` (AD-023, 17 commands) and `enterprise` (15 commands) features are dormant command surfaces with zero frontend callers. CI built only `default` and `experimental`, so NOTHING ever compiled them — and they rotted silently into 56 compile errors. Enabling either one was impossible. Root causes, all invisible without a build: 1. Dependency API drift. chacha20poly1305 0.10 -> 0.11 moved to aead 0.6 / crypto-common 0.2, which removed the `aead::OsRng` re-export and `AeadCore::generate_nonce`. team_sync_crypto.rs failed at its import, and because the module never compiled, the `#[tauri::command]` macros in team_sync_commands never expanded — producing 33 cascading "cannot find __cmd__*" errors that masked the real one-line cause. Migrated to the `Generate` trait (`XNonce::generate()`) for the AEAD nonce and `rand::rngs::OsRng` for x25519-dalek, which still pins rand_core 0.6. The two RNG generations are NOT interchangeable; both are system CSPRNGs, so this is an API migration with no change to randomness source or strength. Also replaced the deprecated `XNonce::from_slice` with `TryFrom`. 2. Named re-exports of Tauri commands. Both team_sync_commands/mod.rs and webhooks/mod.rs re-exported their commands BY NAME. `#[tauri::command]` emits a companion `__cmd__<name>` macro beside each function, and `generate_handler!` resolves `module::__cmd__<name>` — a named re-export carries the function and leaves the macro behind. Switched both to glob re-exports, matching the working pattern in ace_commands/mod.rs. 3. rsa 0.10 test break. sso_crypto's test key generator passed `rand::thread_rng()` (rand_core 0.6) to an API now requiring rand_core 0.10. Uses `getrandom::rand_core::UnwrapErr(getrandom::SysRng)` — getrandom is already a direct dependency, so no new crate. Also: 3 sort_by -> sort_by_key(Reverse), 3 Duration::from_secs -> from_mins/ from_hours (verified const-callable on the pinned 1.95.0 toolchain), and removal of 6 genuinely dead imports. Dormant-by-design items carry an explicit #[allow(dead_code)] at their cfg-gated `mod` declaration, matching the convention already used for the stub modules. OrgPolicies.ts is regenerated by ts-rs: it was stale because the enterprise tests had never run. It will now regenerate on every enterprise CI leg. THE STRUCTURAL FIX: three new legs in the validate matrix — team-sync, enterprise, and both together. The combined leg is not redundant: it is the only one that swaps the real `audit` module in for `audit_stub`, a third path neither single-feature leg covers. Without these, this rot recurs. Verified locally, all five legs: clippy -D warnings: default, experimental, team-sync, enterprise, both — clean cargo fmt --check: clean cargo test --lib: 4437 / 4489 / 4549 / 4601 passed, 0 failed Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUeKTKwNmdow8yUk3q8RB2
c156a6e to
0f7b047
Compare
|
Closes the original Verified on the rebased branch ( On Worth noting for the reviewer: the new CI legs deliberately gate |
…ncovered paths (#429) ## What this fixes Four CI gates reported success without doing the work they claim. Each was verified against live run data before anything was changed. ### 1. The integration tests had never run in CI — on any workflow Every `cargo test` in `.github/workflows/` was `--lib`-scoped (`validate.yml:303/308`, `hermetic.yml:167/176`). No `--tests`, no `--all-targets`. So **all 154 real integration tests in `src-tauri/tests/` had never executed in CI**, including all 12 migration tests and the repo's only forward-migration coverage — against a `TARGET_VERSION = 103` migration chain that has no checksums and no downgrade path. **I measured before changing anything.** Full `cargo test --tests` run on this branch's base (`a6ece843`), isolated data dir, exit code 0: | Target | Result | |---|---| | lib unittests | **4,290 passed**, 0 failed, 10 ignored | | `4da` (cli bin) | 13 passed, 0 failed | | `fourda` / `fourda-engine` bins | 0 tests each | | `migration_tests` | **12 passed**, 0 failed | | `pipeline_integration` | **13 passed**, 0 failed | | `source_resilience` | **5 passed**, 0 failed | | `stack_simulation` | **124 passed**, 0 failed | | `victauri_dogfood` | 157 passed, 3 ignored (self-skips without `VICTAURI_E2E=1`) | | **Total** | **4,614 passed across 9 binaries, 0 failed** | **Nothing was broken and nothing had to be excluded.** They are also hermetic by construction, which I verified rather than assumed: `pipeline_integration` uses an in-memory DB (`test_utils::test_db()` → `:memory:`) and `migration_tests` uses `tempfile::tempdir()`. Both workflows now run `--tests` (lib + bins + integration) under the same throwaway-data-dir isolation `hermetic.yml` already used. Two follow-on fixes were required to avoid landing a red gate: - The count floor took `tail -1` of the `test result:` lines. With `--tests` there are **9** test binaries, so it would have read the *last* binary's total (157) and tripped the 2000 floor on every run. It now **sums** all binaries. - A new assertion fails if fewer than 5 test binaries report — so if this is ever re-scoped to `--lib`, it fails loudly instead of silently dropping the integration suite again. >⚠️ **Non-obvious trap for reviewers:** the isolation directory name must keep containing the substring `data`. `src/state.rs::test_get_db_path_points_to_data_dir` asserts the resolved DB path contains `"data"`. Pointing `FOURDA_DATA_DIR` at e.g. `/tmp/4da-hermetic` makes that lib test fail; `…/4da-hermetic-data` passes. I hit this during measurement. Both call sites are commented. ### 2. The hermetic fresh-clone canary never ran outside PRs `fresh-clone` needs the PR-only `changes` job. GitHub skips any job whose `needs` was skipped **unless its `if:` contains a status function** — and `hermetic.yml:94` had none. So push-to-main, the nightly cron and manual dispatch all skipped the clone and reported success: | Trigger | Duration | Result | |---|---|---| | push → main (08-14 03:52) | **9s** | "success" | | push → main (08-13 16:42) | **7s** | "success" | | schedule (08-13 08:21) | **7s** | "success" | | pull_request (real work) | ~19min | success | The file's own comment at `:56-60` claimed these paths "ALWAYS run the full canary". **The nightly cron had never built a single fresh clone.** Fixed with `!cancelled()` — not `always()`, so a cancelled run doesn't spawn a 45-minute cold build. The **identical defect** silently disabled `workflow_dispatch` for Frontend, MCP Server and the entire Rust matrix in `validate.yml`: the `github.event_name == 'workflow_dispatch'` clause on those three jobs had never once fired, while `Validate Success` (`if: always()`) still went green. Same fix. ### 3. A path-filter hole took the whole fleet down today This stopped being hypothetical while this PR was being written: - **#423 was a Rust-only PR.** Its `Frontend` job was skipped by the path filter. - `scripts/check-file-sizes.cjs --ci` ran **only inside the Frontend job** — but it scans `SCAN_DIRS = ['src', 'src-tauri/src']`, i.e. it guards Rust files too. - So #423 merged a **1032-line `src-tauri/src/analysis_rerank.rs`** past the 1000-line hard error threshold, with the gate never executing. - `check-file-sizes.cjs` then exited 1 on `main`, and `.husky/pre-commit:38-41` treats that as blocking — **every developer in the fleet was unable to commit.** A gate that guards Rust files must not be reachable only through a filter that excludes Rust. This PR adds a **`repo-guards` job with no path filter and no `needs:`** — it runs on every PR and dispatch, and carries `check-file-sizes`, `check-no-window-spawns`, `check-release-channel` and the guard self-tests. They are removed from `Frontend` (they were never frontend-specific). Hosted, ~40s, no `pnpm install` needed: all three guards use only node builtins. It is also in `validate-success`'s `needs`, so it actually gates the merge — `repo-guards` is now the only leg guaranteed to have run. **Additionally, "no filter matched" now means RUN, not PASS.** `site/`, `paddle-webhook/`, `mcp-memory-server/`, `editors/vscode/` and `.husky/` matched no filter, while `Validate Success` is the only required check and auto-merge is enabled repo-wide — so a Dependabot bump into the payment webhook, or a PR weakening `.husky/` itself, could merge with nothing run. Added `.github/**` and `.husky/**` explicitly, plus an `uncovered` fail-safe filter that catches anything unrecognised **including directories added in future**. > The `uncovered` filter uses `predicate-quantifier: 'every'`, which is **required** — the default `'some'` ORs the patterns, and a list of negations OR'd together matches every file. Verified against the action's source at the pinned SHA (`src/filter.ts:110-113` → `patterns.every(...)`; `MatchOptions = {dot: true}`, so `.husky/**` matches). ### 4. The guards' own self-tests ran nowhere `pnpm run test:scripts` (53 tests across 6 files) executed in **no hook and no workflow** — nothing verified the guards still detect what they claim. `pnpm run validate` isn't run by CI either (the Frontend job runs its steps individually), so wiring it into `package.json` alone would not have gated it. It is now a step in `repo-guards`. ## Deliberately NOT done - **No branch-protection or ruleset change.** Making `Hermetic Success` required is the correct end state — currently `Validate Success` is the *only* required check in the active `main-protection` ruleset (verified via the API; classic branch protection is disabled). But with ~30 open PRs and Hermetic historically failing on #421, flipping it now would block the fleet. **Recommended as an explicit follow-up** once this lands and Hermetic is observed green on push-to-main for a few days — which, note, is the first time that signal will ever have existed. - **`package.json` untouched.** `test:scripts` was going to be added to the `validate` chain, but a peer worktree agent holds a claim on that file and #418 also edits that exact line. Wiring it into `repo-guards` achieves the real goal (it now runs in CI) without touching the claimed file. - **`analysis_rerank.rs` / `check-file-sizes.cjs` untouched** — a separate agent owns the immediate unblock. This PR fixes the structural cause only. - **No per-package jobs for `site/`, `paddle-webhook/`, `mcp-memory-server/`, `editors/vscode/`.** The `uncovered` fail-safe means they now trigger the generic gate instead of passing silently, but that gate does not *build* them. Dedicated jobs are the right follow-up and belong in their own PR. - **Rust job timeout raised 30 → 45 min.** Not cosmetic: this job now compiles the integration test targets, and Swatinem's cache is only saved from `main`, so until this lands there every PR run pays a cold link for 5 extra binaries. 30 was too tight for that first window, and a timeout on a required gate is a red gate. ## Conflicts with open PRs Checked `gh pr diff --name-only` on every PR touching these files: | PR | Overlap | Notes | |---|---|---| | #387, #350 | none | Dependabot `actions/checkout` SHA pins only — different lines | | #388 | none | `taiki-e/install-action` SHA pin only | | #424 | none | Adds 3 matrix legs at `validate.yml:236-255`; my edits are at 301+ and inside `steps:`. I deliberately did **not** add a matrix key — an integration-floor key would have had to be added to its new legs. Verified `cargo test --tests --features experimental` compiles clean, so its `test-floor: 0` compile-gate legs are unaffected. | | #418 | **1 line** | Both edit `validate-success`'s `needs:`. #418 adds `pr-metadata`, this adds `repo-guards`. Resolution is a union of the two lists — whoever merges second takes both. Flagged rather than pre-empted. | `uncovered` was also deliberately placed *before* the main filter step, to stay clear of the end-of-job boundary #418 inserts a job into. ## Live CI evidence from this PR's own run The first run of this branch already proves the fix, on both platforms: | Check | Result | |---|---| | Fresh clone (ubuntu-22.04) | **pass**, 12m22s | | Fresh clone (windows-latest) | **pass**, 18m21s | | Hermetic Success | **pass** | | Rust (default) | **pass**, 12m41s | | Rust (experimental) | **pass**, 12m02s | The hermetic job log shows **all 9 test binaries executing on both legs** — `migration_tests` 12, `pipeline_integration` 13, `source_resilience` 5, `stack_simulation` 124, `victauri_dogfood` 157, plus lib (4,290 windows / 4,284 ubuntu — a 6-test platform delta, far above the 2000 floor) and the 3 bin targets. **Zero failures.** That is the first time any of those integration tests has run in CI. Rust finished in ~12min against the old 30min cap, so the 45min bump is headroom for the first cold-cache window rather than a response to an observed timeout. ## Verification - Both workflows parse as YAML; all `if:` expressions and filter blocks inspected post-rebase. - `cargo test --tests` measured green in full **before** any workflow edit, and **re-run green after** rebasing onto current `main` (table above) — #421 removed ~54k lines and #423 changed pipeline code between those two runs. - `dorny/paths-filter` negation + `every` semantics confirmed from source at the pinned SHA, not assumed. - `check-no-window-spawns`, `check-release-channel`, `test:scripts` all verified exit 0 locally; `check-file-sizes` correctly exits 1 (the live outage above). - Rebased onto latest `main`; only the two workflow files differ. The `analysis_rerank.rs` unblock (#430) has landed, so `repo-guards` passes; this branch is rebased on top of it. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pply-chain blind spot (#433) ## The premise, verified first An audit lane claimed the quick-xml suppression in `deny.toml:96-112` / `.cargo/audit.toml:15-29` had gone stale. It rested on this justification: > "NO consumer in our tree has a released version against >=0.41 yet" **Confirmed false.** Read straight out of the registry index (`rust_version` and `deps` per published version): | consumer | we had | latest | quick-xml req | zip req | |---|---|---|---|---| | `calamine` | **0.25.0** | 0.36.1 | `^0.31` → **`^0.41`** | `^1.0` → `^8.6` | | `docx-rs` | **0.4.20** | 0.4.22 | `^0.36` → **`^0.41`** (since 0.4.21) | `^0.6.3` → `^8.6` | | `plist` | **1.9.0** | 1.10.0 | `^0.39.2` → **`^0.41`** | — | All three shipped support. The ignores were suppressing a live, fixable advisory pair on a parser that reads **user-supplied `.xlsx` / `.docx`**. ## What changed **quick-xml (RUSTSEC-2026-0194 / -0195) — resolved, not re-justified.** Bumping the three consumers collapses `quick-xml` **0.31.0 + 0.36.2 + 0.39.4 → a single 0.41.0**. Both advisories stop firing on their own, so both ignores are **deleted** from `deny.toml` *and* `.cargo/audit.toml` (they had diverged; both were checked). `RUSTSEC-2023-0071` (`rsa`) left alone as instructed. **`office.rs` needed no edit** — and that is a verified claim, not an absence of errors: - `sheet_names()` and `worksheet_range()` have byte-identical signatures in 0.25 and 0.36. - `Data` still has exactly the same nine variants with the same payloads. `cell_to_string` matches it **exhaustively with no wildcard arm**, so an added variant could not have compiled. - `ExcelDateTime`'s `Display` impl is byte-identical (`write!(f, "{}", self.value)`), so `DateTime` cells format the same. - Same for `docx-rs`: `TableChild` / `TableRowChild` are destructured irrefutably, so a new variant there could not have compiled either. The documented decompression-bomb weakness (the 100 MB cap is on the **compressed** size) is untouched — separate work, not regressed. **zip — partial.** `zip 1.1.4` retired as hoped. **`zip 0.6.6` did not** — it is our own direct `zip = "0.6"`, so retiring it is an 8-major API migration across `osv/cache.rs`, `extractors/archive.rs` and `embeddings_providers/fastembed.rs`. No advisory attaches to it, so it is staleness, not exposure. Left as follow-up rather than smuggled into a security PR. Tree is now `zip` 0.6.6 (ours) + 4.6.1 (tauri-plugin-updater) + 8.6.0 (calamine/docx-rs). **`relay/` — 5 vulnerabilities → 0.** A TLS-terminating server with no Dependabot entry, no cargo-audit, no CI. | crate | change | advisory | |---|---|---| | `rustls-webpki` | 0.103.9 → **0.103.14** | RUSTSEC-2026-0049 / -0098 / -0099 / -0104 (cert validation) | | `spin` | 0.9.8 → **0.9.9** | 0.9.8 was **yanked** | | `anyhow` | 1.0.102 → 1.0.104 | RUSTSEC-2026-0190 | | `event-listener` | 5.4.1 → 5.4.2 | RUSTSEC-2026-0221 | | `rand` | 0.8.5 → 0.8.7 | RUSTSEC-2026-0097 | `rsa 0.9.10` remains with no fix available, and is recorded in a new `relay/.cargo/audit.toml` with evidence that it is **not in the build graph**: it reaches `Cargo.lock` only via sqlx's optional `mysql` backend, which relay never enables — `cargo tree -i rsa` and `cargo tree -i sqlx-mysql` both report *nothing to print*. **Coverage, so it stops recurring.** `dependabot.yml` gains a `cargo` entry for `/relay` (not a `src-tauri` workspace member, so the existing entry never saw it), and `nightly-audit.yml`'s cargo-audit step now loops every `Cargo.lock` in the repo. **Workflow footprint is deliberately limited to those two files** — `validate.yml` is being reshaped by peer PRs and is untouched here. **`relay/Dockerfile`.** `cargo build --release --locked 2>/dev/null || cargo build --release` silently dropped lockfile enforcement and swallowed the reason. Fallback removed. Its base image also had to move **1.82 → 1.95**: the lockfile already required 1.88 via `time 0.3.47` (`jsonwebtoken` → `simple_asn1`), so that image could not have built this crate at all — the fallback was hiding a hard failure, not surviving a soft one. ## Two things found on the way **1. `main` was un-committable — independently confirmed, now fixed by #430.** `scripts/check-file-sizes.cjs` exits 1 on `src-tauri/src/analysis_rerank.rs` (**1032 lines against a 1000 hard limit**, arrived with #423). The gate scans the whole repo rather than staged paths, so `.husky/pre-commit` failed for *every* terminal on *every* commit — including this one. I hit it, diagnosed it, and fixed it the same way a peer did in **#430** (lift the test module into a sibling `analysis_rerank_tests.rs` via `#[path]`, 1032 → 866). #430 landed first, so **that commit has been dropped from this branch by rebase** — this PR now contains only the dependency work. Recording it here as an independent second confirmation of both the diagnosis and the chosen fix. **2. `cargo clippy --all-targets -- -D warnings` does not pass on `main`** (255 pre-existing errors at my branch point, ~all `unwrap_used`/`expect_used` in test code). This is **not** the gate — CI runs `cargo clippy ${{ matrix.cargo-features }} -- -D warnings` *without* `--all-targets`, so the numbers below are from the CI-equivalent invocation. Reported as an observation, not touched. ## Verification | check | result | |---|---| | `src-tauri` `cargo audit` | **exit 0** — zero vulnerabilities, zero warnings | | `src-tauri` `cargo deny check` | **exit 0** — `advisories ok, bans ok, licenses ok, sources ok` | | `relay` `cargo audit` | **exit 0** (was 5 vulns + 4 warnings + 1 yanked) | | `relay` `cargo check --locked --all-targets` | clean | | `cargo clippy -- -D warnings` (CI-equivalent, default) | **exit 0** | | `cargo clippy --features experimental -- -D warnings` | **exit 0** | | `cargo fmt --check` | **exit 0** | | `cargo test --lib` | **4300 passed, 0 failed, 10 ignored** | All re-run after rebasing onto `c1fd348c` (#425, #426, #427, #429, #430, #431 all landed mid-flight). `--features team-sync` and `--features enterprise` fail to compile — **pre-existing rot on `main`** (`chacha20poly1305::aead::OsRng` unresolved, then cascading `__cmd__*` macro failures), which is what #424 exists to repair. My lockfile diff touches no crypto crate. #424 is still open as of this push, and the CI clippy matrix on `main` still carries only the `default` and `experimental` legs — so the two legs verified above are exactly the gate. ### The extractor tests were `#[ignore]`d and had never run There are no `.xlsx`/`.docx` fixtures anywhere in the repo, so `test_real_docx_extraction` / `test_real_xlsx_extraction` were no-ops that returned early. To gain real confidence in an 11-minor-version parser bump I generated **real OOXML documents** — shared strings, an inline string, numeric and boolean cells, paragraphs and a table — confirmed both `#[ignore]`d tests pass against them, and separately asserted the extracted text matches the pre-bump formatting contract exactly: ``` === Sheet: Budget === Hello from 4DA Item | Cost Second paragraph Widget | 42 A1 | B1 Gadget | 3.50 | TRUE ``` That exercises every arm of `cell_to_string` that a document can reach (shared/inline string, integral float → `42`, fractional float → `3.50`, bool → `TRUE`) plus the docx paragraph and table paths. The scratch harness was deleted; **no test-file changes ship in this PR**. ## Deliberately left - **`zip 0.6.6`** — direct dep, 8-major API migration, no advisory. Follow-up. - **`office.rs` decompression bomb** — the 100 MB cap is on the compressed size. Out of scope, not regressed. - **`--all-targets` clippy backlog** — pre-existing, not the CI gate. - **`validate.yml`** — peer-owned right now, untouched on purpose. - **`--features team-sync` / `enterprise`** — pre-existing rot, #424's job. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The problem
team-sync(AD-023, 17 commands) andenterprise(15 commands) are dormant command surfaces with zero frontend callers. CI built onlydefaultandexperimental, so nothing ever compiled them — and they rotted silently into 56 compile errors. Enabling either feature was impossible.This was found while auditing
--all-featuresduring #422 and is fixed here at the root, plus gated so it cannot recur.Root causes
1. Dependency API drift (the real one-liner, hidden behind 33 cascading errors)
chacha20poly13050.10 → 0.11 moved toaead0.6 /crypto-common0.2, which removed theaead::OsRngre-export andAeadCore::generate_nonce.team_sync_crypto.rsfailed at its import — so the module never compiled, so the#[tauri::command]macros inteam_sync_commandsnever expanded, producing 33cannot find __cmd__*errors that buried the actual cause.Migrated to the
Generatetrait (XNonce::generate()) for the AEAD nonce, andrand::rngs::OsRngforx25519-dalek, which still pinsrand_core0.6. The two RNG generations are not interchangeable — verified against the vendored crate sources rather than assumed. Both are system CSPRNGs: this is an API migration, with no change to randomness source or strength. DeprecatedXNonce::from_slice→TryFrom.2. Named re-exports of Tauri commands
Both
team_sync_commands/mod.rsandwebhooks/mod.rsre-exported commands by name.#[tauri::command]emits a companion__cmd__<name>macro beside the function, andgenerate_handler!resolvesmodule::__cmd__<name>— a named re-export carries the function and leaves the macro behind. Switched to glob re-exports, matching the workingace_commands/mod.rs.3.
rsa0.10 test breaksso_crypto's test key generator passedrand::thread_rng()(rand_core 0.6) to an API now requiring rand_core 0.10. Fixed withgetrandom::rand_core::UnwrapErr(getrandom::SysRng)—getrandomis already a direct dependency, so no new crate.Also
sort_by→sort_by_key(Reverse(..)); 3×Duration::from_secs→from_mins/from_hours(verified const-callable on the pinned 1.95.0 toolchain, not just local 1.97)#[allow(dead_code)]at their cfg-gatedmoddeclaration, matching the convention already used for the stub modules. This suppresses lint noise on staged code — it does not suppress compile errors, which is the failure this feature actually had.OrgPolicies.tsregenerated by ts-rs — stale only because enterprise tests had never runThe structural fix
Three new legs in the validate matrix:
team-sync,enterprise, and both together. The combined leg is not redundant — it is the only one that swaps the realauditmodule in foraudit_stub, a third path neither single-feature leg covers. Without these, this rot simply recurs.Verification — all five legs
-D warningscargo test --libcargo fmt --check: clean. 0 failures anywhere.Note for reviewers
webhooks/tests.rsimportedcheck_circuit_breakerbut never used it — removed. That suggests a missing test for the webhook circuit breaker, which is reliability-relevant. Left as a follow-up rather than silently writing tests for a dormant surface in a build-repair PR.