fix/cfe-silent-ok
needs attentionviewing older commit87450fb · incrementalpre-PRreviewed 2026-07-22 16:34 UTC0H · 7M · 5L- Purpose
- BAT-272: CFE collector was silently reporting success (gobierno_only) when Mi Espacio creds were present but registration failed transiently — triage was checking the wrong thing
- Goal
- Transient Mi Espacio registration failure retries via SFN then grades failed, not silently completed. Strategy label honestly distinguishes deliberate gobierno-only (BAT-174 path) from attempted-and-failed fallback.
- Sub-goals
- SG-1: pure decideMiEspacioFailureAction + recoverable() ctor; gate abort/degrade/none (CONFIRMED)
- SG-2: regression tests pinning decideFinalizeJob grading (CONFIRMED)
- SG-3: gobierno_fallback strategy label keyed on miFailureReason (CONFIRMED)
- SG-4: preview e2e on manual cfe-silent estate — ALL PASS
- SG-N: framework fold-back: MAX_BRANCH_NAME_LEN 17->16, workflow timeout, .branch wipe fix, test setup soft fallback
- What
- Added gobierno_fallback enum value to 3 hand-synced declarations (CollectingStrategy, CollectingStrategySchema, CollectorOutput.strategy). Extracted pure decideStrategyLabel (keyed on miFailureReason). MAX_BRANCH_NAME_LEN 17->16 + docs. Workflow timeout 45->75 min. Targeted .branch wipe in branch-provisioning skill. Utility test setup soft DB fallback.
- Why
- SG-3/SG-N are the tail of the BAT-272 fix: strategy label honesty (SG-3) + learnings folded back into framework files (SG-N). E2E validated on manually-deployed cfe-silent preview estate (CDK preview had never deployed on this branch — off-by-one in MAX_BRANCH_NAME_LEN was the root cause, now fixed).
- Areas
- services/utility/bills/cfe+90−31domains/utility/src+12−0packages/api/src+13−0infra/cdk/src+19−11.github/workflows+21−8.claude/rules+24−11.claude/skills+10−3CLAUDE.md+1−1.branch+135−272
- Blast
- 14 files, +818/-306. No DB migration, no new API surface, no SFN ASL change. Collector behavior change scoped to strategy label on degrade runs only.
Findings · 13
correctness4
Cache-carry success: Mi Espacio succeeded but uploaded nothing new -> still labels gobierno_only
services/utility/bills/cfe/src/handlers/collector.lambda.ts:481
Pre-existing behavior from old ternary. hasMi=false when Mi Espacio succeeded but all periods cached; miFailureReason=null so gobierno_only wins. JSDoc implies honest labeling but this path is unchanged.
gobierno_sufficient is a dead enum value — no runtime path produces it
domains/utility/src/cfe-job/cfe-job.type.ts:66
Pre-existing. CollectingStrategy union includes it; decideStrategyLabel never returns it; CollectorOutput omits it. Two seed rows use it. Cleanup belongs in a follow-up issue.
17-char slug back-compat: naming threshold 17->16 while workflow gate stays >17
infra/cdk/src/lib/naming.ts:349
Deliberate and documented. Any live 17-char preview branch gets hash-renamed physical resources on next CDK deploy. Isolated to ephemeral previews.
Placeholder POSTGRES_URL removes fail-fast on DB-less integration test runs
domains/utility/src/__tests__/setup.ts:17
Mirrors existing CFE service pattern. A lazily-connected integration test could pass without a DB; ECONNREFUSED is less clear than the old throw.
conventions3
12-line JSDoc on decideStrategyLabel violates 'one short line max' rule
services/utility/bills/cfe/src/handlers/collector.lambda.ts:527
CLAUDE.md: no multi-line comment blocks. Core WHY is genuine but compressible to 1-2 lines. Sibling functions share same pattern.
7-line deploy-order comment block in schema file; SSOT already in cfe-pipeline.md
packages/api/src/schemas/cfe-job.schemas.ts:36
Duplicate of cfe-pipeline.md deploy-order rule. A single cross-reference line is sufficient.
Emoji in console.warn copied from sibling — violates no-emoji rule
domains/utility/src/__tests__/setup.ts:19
Copied from services/utility/bills/cfe/__tests__/setup.ts. CLAUDE.md: no emojis in code.
tests4
Missing: miUploaded=true + miFailureReason set — miFailureReason intentionally ignored on success paths not pinned
services/utility/bills/cfe/__tests__/unit/handlers/collector-lambda.test.ts:449
{miUploaded:true, miFailureReason:'error'} -> mi_espacio_direct should be explicitly tested. A branch reorder could silently break the most common success path.
Missing: dual_portal + miFailureReason set — upload-count priority over failure-reason not pinned
services/utility/bills/cfe/__tests__/unit/handlers/collector-lambda.test.ts:449
Current dual_portal test only covers miFailureReason:null. A case with miFailureReason set would document intended priority.
3 success-label assertions bundled in one it() — reduces failure locality
services/utility/bills/cfe/__tests__/unit/handlers/collector-lambda.test.ts:449
dual_portal, mi_espacio_via_registration, mi_espacio_direct each cover a distinct code branch. Split into 3 it() blocks for precise regression signals.
Missing integration pin: gobierno_fallback through updateAfterCollectingShell
domains/utility/src/cfe-job/__tests__/cfe-job.decisions.test.ts
No BAT-272 test passes strategy:'gobierno_fallback' through the shell. If the Drizzle mapping excluded it, the shell would fail at runtime.
improvement2
CollectorOutput["strategy"] is a hand-maintained subset of CollectingStrategy — 3 places to sync
services/utility/bills/cfe/src/handlers/collector.lambda.ts:68
Consider Exclude<CollectingStrategy, 'gobierno_sufficient'>. This PR demonstrated the 3-way sync burden. Would make a future missing-variant a type error.
gobUploaded only materially affects dual_portal arm — naming slightly misleads
services/utility/bills/cfe/src/handlers/collector.lambda.ts:544
gobUploaded does not affect gobierno_fallback vs gobierno_only split. Not a bug — intentional and tested. Awareness only.