fix(cleanup): resolve four bunyip review findings (BUNYIP-400) #392

Merged

nrupard merged 1 commit from fix/BUNYIP-400-cleanup into main

2026-07-24 17:54:41 +02:00

nrupard commented

2026-07-24 16:35:09 +02:00

Owner

What

Four small, independent issues surfaced by a codebase and cross-app review of bunyip, grouped into one cleanup PR (BUNYIP-400).

Why

Silent audit-write failures hide audit gaps under DB stress: bunyip-api/src/handlers/download.rs discarded the audit-log-write Result on both download paths, unlike crates/bunyip-oci/src/handlers/oci_auth.rs which logs a warn! on the identical failure.
Money should never silently truncate: bunyip-api/src/handlers/webhook.rs cast Stripe's i64 cents as i32, which silently wraps above ~$21.47M.
.env.example documented the pre-cutover SSO topology (mokosh-server as IdP), which is wrong now that bunyip-api is the sole OpenID Provider.
A stale mokosh-server confidential OIDC client registration was soft-deleted with a documented follow-up drop that never landed.

Changes

Audit logging (download.rs): both the DownloadFailedUpstream and DownloadCompleted audit writes now use if let Err(audit_err) = AuditLogRepository::create(...).await { tracing::warn!(error = ?audit_err, "..."); }, matching the oci_auth.rs reference pattern, so a failed audit write is observable instead of swallowed.
Stripe amount range checks (webhook.rs): the four as i32 casts (checkout price lock, checkout amount_total fallback, invoice amount_paid, invoice amount_due) are now checked i32::try_from; an out-of-range value is logged with tracing::error! and the affected write is skipped (checkout price lock) or falls back to 0 rather than wrapping. Bounded fix per the issue; the fuller fix (widen the cents columns to BIGINT and carry i64 end to end) is deferred as out of scope for a cleanup PR.
.env.example bunyip-as-OP: the BUNYIP_OIDC_ISSUER default now points at https://${USER}-bunyip-api.a8n.run and the surrounding comments describe bunyip-api as the OpenID Provider (bunyip-web is a relying party, mokosh-server is a Resource Server only), grounded in bunyip-web/src/config.rs (issuer defaults to the api URL) and compose.dev-sso.yml. The stale /config.json + entrypoint.sh + msp-api.<host> mechanism text and the "register in the mokosh-server repo" instruction were corrected (client registration is just register-dev-clients against bunyip-api).
Drop-stale-client migration: bunyip-api/migrations/20260724000010_drop_stale_mokosh_server_oidc_client.sql deletes the mokosh-server confidential client row (b0000000-0000-4000-8000-000000000001) registered in 20260502000048 and flagged for removal in 20260603000010. The DELETE is idempotent (no-op on an absent row) and does not touch the real mokosh-apps / drillmark seeds.

Tests

Migration version gate (scripts/check-migration-versions.sh): passed (unique and strictly increasing; new stamp 20260724000010 sits after the prior max 20260721000030).
Pinned rust-builder gate (ghcr.io/niceguyit/rust-builder-glibc:v1.0.1-rust1.94-trixie, SQLX_OFFLINE=true): cargo fmt --all --check clean, cargo clippy --workspace --all-targets -- -D warnings clean, cargo test --workspace --all-targets green (493 tests passed, 0 failed). No live Postgres required.

## What Four small, independent issues surfaced by a codebase and cross-app review of bunyip, grouped into one cleanup PR (BUNYIP-400). ## Why - Silent audit-write failures hide audit gaps under DB stress: `bunyip-api/src/handlers/download.rs` discarded the audit-log-write Result on both download paths, unlike `crates/bunyip-oci/src/handlers/oci_auth.rs` which logs a warn! on the identical failure. - Money should never silently truncate: `bunyip-api/src/handlers/webhook.rs` cast Stripe's i64 cents `as i32`, which silently wraps above ~$21.47M. - `.env.example` documented the pre-cutover SSO topology (mokosh-server as IdP), which is wrong now that bunyip-api is the sole OpenID Provider. - A stale mokosh-server confidential OIDC client registration was soft-deleted with a documented follow-up drop that never landed. ## Changes - Audit logging (download.rs): both the DownloadFailedUpstream and DownloadCompleted audit writes now use `if let Err(audit_err) = AuditLogRepository::create(...).await { tracing::warn!(error = ?audit_err, "..."); }`, matching the oci_auth.rs reference pattern, so a failed audit write is observable instead of swallowed. - Stripe amount range checks (webhook.rs): the four `as i32` casts (checkout price lock, checkout amount_total fallback, invoice amount_paid, invoice amount_due) are now checked `i32::try_from`; an out-of-range value is logged with `tracing::error!` and the affected write is skipped (checkout price lock) or falls back to 0 rather than wrapping. Bounded fix per the issue; the fuller fix (widen the cents columns to BIGINT and carry i64 end to end) is deferred as out of scope for a cleanup PR. - `.env.example` bunyip-as-OP: the `BUNYIP_OIDC_ISSUER` default now points at `https://${USER}-bunyip-api.a8n.run` and the surrounding comments describe bunyip-api as the OpenID Provider (bunyip-web is a relying party, mokosh-server is a Resource Server only), grounded in `bunyip-web/src/config.rs` (issuer defaults to the api URL) and `compose.dev-sso.yml`. The stale `/config.json` + entrypoint.sh + `msp-api.<host>` mechanism text and the "register in the mokosh-server repo" instruction were corrected (client registration is `just register-dev-clients` against bunyip-api). - Drop-stale-client migration: `bunyip-api/migrations/20260724000010_drop_stale_mokosh_server_oidc_client.sql` deletes the mokosh-server confidential client row (`b0000000-0000-4000-8000-000000000001`) registered in `20260502000048` and flagged for removal in `20260603000010`. The DELETE is idempotent (no-op on an absent row) and does not touch the real mokosh-apps / drillmark seeds. ## Tests - Migration version gate (`scripts/check-migration-versions.sh`): passed (unique and strictly increasing; new stamp `20260724000010` sits after the prior max `20260721000030`). - Pinned rust-builder gate (`ghcr.io/niceguyit/rust-builder-glibc:v1.0.1-rust1.94-trixie`, `SQLX_OFFLINE=true`): `cargo fmt --all --check` clean, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test --workspace --all-targets` green (493 tests passed, 0 failed). No live Postgres required.

nrupard added 1 commit

2026-07-24 16:35:09 +02:00

fix(cleanup): resolve four bunyip review findings

E2E / Playwright against deployment (pull_request) Successful in 53s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 27m25s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

3f2f7c7d94

Four small, independent issues surfaced by a codebase and cross-app review, grouped into one cleanup change.

1. Audit-write errors were swallowed silently: `bunyip-api/src/handlers/download.rs` discarded the audit-log-write Result via `let _ =` on both the DownloadFailedUpstream and DownloadCompleted paths, unlike oci_auth.rs which logs a warn! on the identical failure. Both sites now log a `tracing::warn!` on Err so silent audit gaps under DB stress are observable.

2. Stripe amount truncation: `bunyip-api/src/handlers/webhook.rs` cast Stripe's i64 cents `as i32` at four sites, which silently wraps above ~$21.47M. Each cast is now a checked `i32::try_from`; out-of-range values are logged (tracing::error!) and skip the write or fall back to 0 rather than wrapping. The bounded fix is the range check; widening the cents columns to BIGINT and carrying i64 end to end is the fuller fix, deferred as out of scope.

3. Stale SSO topology in `.env.example`: the Mokosh-server OIDC block documented the pre-cutover topology (BUNYIP_OIDC_ISSUER defaulting to a mokosh-server URL, comments calling it the mokosh-server IdP and referencing registration in the mokosh-server repo). Post-cutover bunyip-api is the sole OpenID Provider, so the issuer default now points at bunyip-api and the comments describe bunyip-as-OP (grounded in config.rs and compose.dev-sso.yml).

4. Stale mokosh-server OIDC client registration: migration 20260502000048 registered mokosh-server as a confidential OIDC client and 20260603000010 noted it should be dropped in a follow-up that never landed. Added migration 20260724000010 to DELETE the stale client row (b0000000-0000-4000-8000-000000000001).

#BUNYIP-400

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>