← all branches

feat/api-dx2

needs attentionviewing older commit
7113f2e · fullPR #262reviewed 2026-07-06 18:01 UTC5H · 6M · 7L · 4I
The branch
Purpose
Close 4 DX gaps surfaced by a Tiendas Neto customer request ('send me the latest PDFs for these 4 RPUs') end-to-end through the public API.
Goal
Public-API DX round 2: document existing params, fix zip collisions, add collection health visibility, add latest_only file filter.
Sub-goals
  • SG-1: Document force_refresh + period_count on POST /v1/jobs (already worked — discoverability gap)
  • SG-2: Fix filename collisions in /v1/files/zip (multi-RPU bundles had identical period-based entry names)
  • SG-3: Add last_collection + latest_period_end to GET /v1/monitoring (surfaces silently-failing RPUs)
  • SG-4: Add GET /v1/files?latest_only=true (one-call answer to 'give me the latest PDF per RPU')
The changes (whole branch)
What
10 files across api contracts, handlers, validation utils, DB queries, and API types/schemas. All wire changes additive; no migrations.
Why
Addresses a concrete Tiendas Neto customer request and closes gaps that required multiple round-trips through the API.
Areas
apps/platform/src/api/handlers+613apps/platform/src/api/utils+423apps/platform/src/api/contracts+91packages/api/src/schemas/public+209packages/api/src/types/public+190domains/utility/src/bill+81
Blast
10 files, +159/-17. Additive API surface (new fields + new query param); no DB migrations. Public-API consumers see new fields on monitoring rows and new filter option on files list.
latent-idor-risk missing-test-coverage fcis-convention-violations
CI checks· GraphQL access token insufficient for statusCheckRollupcoderabbit· No .coderabbit.yaml in repo

Findings · 21

correctness4

medium

Zip dedup suffix can collide when an existing entry already has the `_N` suffix

apps/platform/src/api/utils/public-v1-file-read.ts:218

If S3 already contains a file whose rpu-prefixed name is `{stem}_2.{ext}` and two copies of `{stem}.{ext}` follow, the suffix formula produces a collision: `files=[{rpu:'123',filename:'2026-05_2.pdf'},{rpu:'123',filename:'2026-05.pdf'},{rpu:'123',filename:'2026-05.pdf'}]` → `['123_2026-05_2.pdf','123_2026-05.pdf','123_2026-05_2.pdf']`. The zip lambda receives two entries at different storagePaths with the same name; one silently overwrites the other. Fix: maintain a `Set<string>` of emitted names and bump until a free slot is found.

low

`latest_only=true` + `rpu` filter applies RPU restriction twice — can diverge under concurrent updates

apps/platform/src/api/utils/public-v1-file-read.ts:100

When `latest_only` is true and `q.rpu` is set, `latestBillIds` is already RPU-filtered; yet `listEnrichedFiles` receives both `billPublicIds: latestBillIds` AND `rpus: q.rpu`, ANDed in SQL. If a contract's `contractNumber` was updated between the two round-trips, a valid billPublicId from round-trip 1 would be silently excluded. Safe fix: don't pass `rpus: q.rpu` to `listEnrichedFiles` when `latestBillIds` is set.

low

`loadHealthByRpu` fires a DB query when `rpus` is empty

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:154

When all visible monitoring rows have `rpu === null`, `rpus` is `[]` but `contractPublicIds` may be non-empty. `listLatestPaymentStatusByContractPublicIds` still fires (it only short-circuits on empty `contractPublicIds`); the result is discarded because the `for (const rpu of rpus)` loop never runs. Guard: `if (rpus.length === 0) return new Map()` at the top of `loadHealthByRpu`.

info

`periodByRpu` may surface wrong contract's latest bill for re-contracted RPUs

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:95

Acknowledged known follow-up in PR description: dedup is per-contract, not per-RPU. Non-crashing. Recorded for completeness.

security4

medium

`listLatestPaymentStatusByContractPublicIds` has no orgId WHERE clause — latent IDOR

domains/utility/src/bill/bill.queries.ts:1048

The query filters only on `inArray(utilityContracts.publicId, contractPublicIds)` with no orgId join. Safety depends entirely on callers having pre-validated the list via `getAccessibleContractPublicIds`. Any future caller that passes an unvalidated list would silently return cross-org data. The sibling `getLatestPerPipelineByRpus` adds `eq(cfeJobs.orgId, orgId)` as defence-in-depth. Recommend adding an orgId parameter and WHERE clause.

medium

`last_collection.error_code` exposes internal CFE scraper error states

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:107

The raw `CfeErrorCode` enum includes `INVALID_CREDENTIALS`, `LOGIN_FAILED`, `SESSION_EXPIRED`, `CAPTCHA_REQUIRED`, `CAPTCHA_SERVICE_UNAVAILABLE`, `RPU_MISMATCH` etc. Exposing `INVALID_CREDENTIALS` to a public API caller reveals that the org's stored CFE credentials are bad — sensitive operational intelligence. Recommend mapping to a curated public enum (e.g. `INVALID_CREDENTIALS/LOGIN_FAILED/SESSION_EXPIRED` → `authentication_failed`; `CAPTCHA_*` → `service_unavailable`; `RPU_MISMATCH` → `rpu_mismatch`).

low

`z.unknown().passthrough()` silently accepts and forwards arbitrary extra fields

packages/api/src/schemas/public/file.public-schemas.ts:138

Intentional forward-compat pattern per api-patterns.md. Risk is that if a future handler refactor spreads the body object, unexpected fields could cause unintended behaviour. Flag for audit if any handler changes from destructuring named fields to iterating/spreading.

info

`loadHealthByRpu` org-scoping is a caller-contract invariant, not query-layer enforcement

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:93

Org-scoping is enforced at two upstream gates before `loadHealthByRpu` is called. Safe in current code; documented to make the dependency chain explicit.

conventions4

high

Handler imports jobQueries directly, bypassing FCIS namespace

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:24

`import { jobQueries } from '@batu/utility-domain/cfe-job'` imports the raw query object from the entity sub-path instead of using `CfeJobFCIS.jobQueries` from `@batu/utility-domain`. api-patterns.md explicitly bans importing domain internals (decisions, queries) in handlers — use the FCIS namespace. Fix: `import { ..., CfeJobFCIS } from '@batu/utility-domain'` and call `CfeJobFCIS.jobQueries.getLatestPerPipelineByRpus(...)`.

high

`loadHealthByRpu` orchestrates DB queries inside a handler module — should be a utility function

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:86

Per FCIS/ADR-016 handlers follow 'withAuth → validate → shell → map response'. `loadHealthByRpu` is a module-level async function running two parallel DB queries in the handler file. This data-fetch orchestration belongs in a utility module co-located with the other public-v1 orchestration utilities (e.g. `public-v1-monitoring-read.ts`), keeping handler files thin.

medium

`public-v1-file-read.ts` calls `BillFCIS.billQueries.*` in the `latest_only` path without a shell boundary

apps/platform/src/api/utils/public-v1-file-read.ts:87

The `latest_only` path calls `BillFCIS.billQueries.listLatestPaymentStatusByContractPublicIds` directly from the orchestration utility. FCIS rule: handlers never call queries directly — always through shells. The FCIS namespace is used correctly (not a raw import), and the file acknowledges the entity_relationships gap in its header comment — medium severity given the existing pattern, but worth a shell extraction in a follow-up.

low

`MonitoringSubscriptionFCIS.subscriptionQueries` called directly in handler body

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:144

Lines 144 and 199 call `subscriptionQueries` directly in the handler body. FCIS namespace is used correctly; this is the established precedent for public-v1 handlers on this surface (service-role read, org-scoped via accessible-contracts). Pre-existing pattern, not introduced by this PR.

tests6

high

`latest_only` + `bill_id` mutual-exclusion rule has no test

apps/platform/src/api/utils/__tests__/public-v1-validation.test.ts:224

The three new `latest_only` paths in `parseFilesListQuery` are entirely uncovered: (1) `latest_only=true` accepted as boolean, (2) `latest_only=true` + `bill_id` returns mutual-exclusion fail, (3) `latest_only=false`/absent behaves as no-filter. This is the only purely functional new logic in the PR and it is directly unit-testable at ~5000/sec with no mocking.

high

`latest_only` non-boolean string rejection path untested end-to-end

apps/platform/src/api/utils/__tests__/public-v1-validation.test.ts:319

`parseBoolean` is exercised generically but `parseFilesListQuery({ latest_only: 'yes' })` is never run through the composed parser. Cheap addition that guards against future refactors silently skipping the boolean gate.

high

ZIP entry deduplication logic has no unit test

apps/platform/src/api/utils/public-v1-file-read.ts:218

The new `seen`-Map dedup algorithm is pure deterministic logic (no DB, no S3). Three edge cases are uncovered: multi-RPU same period → distinct entries; duplicate within same RPU → `_2` suffix; rpu=null → bare filename. The subtle `n+1` off-by-one (original → `_2`, skipping `_1`) would be pinned by a test.

medium

`listPublicFiles` `latest_only` code path has no integration test

apps/platform/src/api/utils/public-v1-file-read.ts:86

The `latest_only` branch (DB query → RPU filter → early-exit vs pass-through to listEnrichedFiles) is untested at both unit and integration level. The early-empty-return when `latestBillIds.length === 0` is a silent correctness guarantee with zero coverage.

medium

`loadHealthByRpu` has no test covering null collect job or missing bills

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:86

Four semantically distinct outcomes are untested: RPU with successful job, failed job with error_code, no prior job (last_collection=null), no bills (latestPeriodEnd=null). `collect.error?.code ?? null` is silently wrong if `error` is present but `code` is missing — a test would catch this shape drift.

low

`billPublicId` propagation from `listLatestPaymentStatusByContractPublicIds` has no test

domains/utility/src/bill/bill.queries.ts:1

No test confirms the query returns `bil_` prefixed public IDs (not internal integer ids) for the new `billPublicId` field. A mismatch would cause `listEnrichedFiles` to receive wrong IDs and silently return no files.

improvement3

low

Zip dedup skips `_1` suffix — document the intent

apps/platform/src/api/utils/public-v1-file-read.ts:221

Counter `n` is 1 for the first duplicate, producing `_2` (original → `_2` → `_3`). If this is intentional (matching macOS/Windows behaviour), a comment prevents future readers from 'fixing' it. If unintentional, the first duplicate should be `_1`.

info

`loadHealthByRpu` `out` Map declared without type parameter

apps/platform/src/api/handlers/public-v1/monitoring.handler.ts:98

`new Map()` without a type parameter. Adding `new Map<string, { lastCollection: ...; latestPeriodEnd: string | null }>()` would catch shape drift at compile time.

info

Mutual-exclusion guard `out.bill_id.length > 0` may be redundant

apps/platform/src/api/utils/public-v1-validation.ts:322

If `bill_id` is only populated when non-empty (the schema enforces `min(1)` for filter arrays), the `.length > 0` check is dead code. A comment or removal reduces noise.

History · 6 commits

  1. d2ff8e5needs attentionincremental1H · 2M · 4L2026-07-07 21:51
  2. 2ad6e01safeincremental0H · 1M · 1L2026-07-07 21:15
  3. 4ea2976needs attentionincremental0H · 1M · 4L2026-07-07 20:20
  4. fe4cf9fneeds attentionincremental3H · 8M · 7L2026-07-07 14:53
  5. d82dbe8safeincremental0H · 0M · 0L2026-07-06 18:16
  6. 7113f2eneeds attentionfull5H · 6M · 7L2026-07-06 18:01current