feat/api-dx2
needs attentionviewing older commit7113f2e · fullPR #262reviewed 2026-07-06 18:01 UTC5H · 6M · 7L · 4I- 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')
- 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+61−3apps/platform/src/api/utils+42−3apps/platform/src/api/contracts+9−1packages/api/src/schemas/public+20−9packages/api/src/types/public+19−0domains/utility/src/bill+8−1
- 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.
Findings · 21
correctness4
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.
`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.
`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`.
`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
`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.
`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`).
`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.
`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
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(...)`.
`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.
`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.
`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
`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.
`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.
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.
`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.
`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.
`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
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`.
`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.
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
- d2ff8e5needs attentionincremental1H · 2M · 4L2026-07-07 21:51
- 2ad6e01safeincremental0H · 1M · 1L2026-07-07 21:15
- 4ea2976needs attentionincremental0H · 1M · 4L2026-07-07 20:20
- fe4cf9fneeds attentionincremental3H · 8M · 7L2026-07-07 14:53
- d82dbe8safeincremental0H · 0M · 0L2026-07-06 18:16
- 7113f2eneeds attentionfull5H · 6M · 7L2026-07-06 18:01current