feat/slug-budget-24
needs attention2c88a05 · incrementalPR #320reviewed 2026-07-16 22:57 UTC2H · 3M · 2L · 2I- Purpose
- Raise the CDK preview branch slug budget from 17 to 24 chars by making conventional type prefixes (feat-/fix-/…) not count against the budget — addressing a real crash where a 17-char slug pushed a Helioscope Lambda name to 65/64 chars.
- Goal
- Allow up to 24 descriptive chars in branch names without losing CDK preview isolation, while keeping deployed (un-branched) resource names completely unchanged.
- Sub-goals
- SG-1: preview-only abbreviation pass (helioscope→helio, subscriber→sub, etc.) + BRANCH_EXACT_ABBREV keeps all fixed parts ≤40 chars
- SG-2: MAX_BRANCH_NAME_LEN raised to 24; boundedBranchSlug strips type prefix before bounding
- SG-3: BranchNameLengthAspect — synth-time assertion that every branched name leaves 24 chars of slug room
- SG-4: BRANCH_EXACT_ABBREV key ordering fix — keyed on raw pre-token string so token-map changes can't silently unkey entries
- SG-5: IAM dual-spelling remediation — Vercel Service User policy now covers both helioscope (un-branched) and helio (branched preview) spellings
- SG-6: Doc sweep — all .claude/rules, skills, workflows, and CLAUDE.md updated to the 24-char descriptive budget
- What
- naming.ts: BRANCH_TOKEN_ABBREV (5 tokens), BRANCH_EXACT_ABBREV key ordering fix, boundedBranchSlug prefix stripping (17→24 budget). BranchNameLengthAspect: new resource types (LogGroup, CloudWatch Alarm, SecretsManager). vercel-service-user.stack.ts: helio-spelled IAM ARN patterns. app twin metrics-explore-core.ts + tests updated to match. preview-provision/deprovision YMLs updated. 6 .claude rule/skill files updated.
- Why
- The 17-char gate was too tight and based on a lie — even a 17-char slug could crash synth (fix-next-check-ui + helioscope materialize-persist = 65/64 chars). Abbreviating the wordy fixed tokens rather than shrinking the user-visible budget is the correct fix.
- Areas
- infra/cdk/src/lib/naming.ts+75−13infra/cdk/src/aspects/branch-name-length.ts+75−0infra/cdk/src/stacks/cicd/vercel-service-user.stack.ts+11−0apps/platform/src/api/utils/metrics-explore-core.ts+17−6apps/platform/src/api/utils/__tests__/metrics-explore-core.test.ts+22−15.github/workflows/preview-provision.yml + preview-deprovision.yml+18−11.claude/rules + .claude/skills (6 files)+25−20infra/cdk/scripts/audit-resource-name-lengths.mjs (new)+69−0
- Blast
- 19 files across infra, platform, and docs. Preview CDK resource names change for branches using abbreviated tokens (helioscope, subscriber, schedule, coordinator, aggregator) — safe because preview stacks are ephemeral. Un-branched (dev/stg/prod) names are completely unchanged.
Findings · 9
correctness2
Twin hardcodes `24` instead of importing `MAX_BRANCH_NAME_LEN`
apps/platform/src/api/utils/metrics-explore-core.ts:64
`naming.ts` uses the exported constant `MAX_BRANCH_NAME_LEN`; the app twin hardcodes `24`. If the budget changes in CDK, the platform computes a different bounded slug → `StartExecution` targets a state machine ARN that does not exist. The cross-reference comment is the only guard. Mitigation: publish `MAX_BRANCH_NAME_LEN` from a shared package (`@batu/shared-kernel` or similar) or at least add an inline `// SYNC-POINT: must match MAX_BRANCH_NAME_LEN in infra/cdk/src/lib/naming.ts` comment on the literal.
Dead `/` branch in `BRANCH_TYPE_PREFIX` regex
apps/platform/src/api/utils/metrics-explore-core.ts
Both copies of `BRANCH_TYPE_PREFIX` use `[-/]` but `boundedBranchSlug` is called only after slugification (which replaces `/` → `-`). The `/` arm is unreachable. Not a bug, but it implies the function accepts raw git refs when it does not.
security1
Pre-existing: SES policy uses `resources: ["*"]`
infra/cdk/src/stacks/cicd/vercel-service-user.stack.ts
Not introduced by this PR. SES does not support resource-level restrictions on `ses:SendEmail` for verified identities at the IAM level, so this is largely an AWS constraint. Tracking as a pre-existing finding.
tests4
CDK `naming.ts` has zero unit tests for `boundedBranchSlug`
infra/cdk/src/lib/naming.ts
The authoritative implementation lives in CDK but all test coverage comes from the app-side twin in `metrics-explore-core.test.ts`. A change to `naming.ts` (prefix list, budget, hash form) has no local test signal — CI would only catch it if the twin diverges and someone notices the ARN mismatch at runtime. Recommend adding `infra/cdk/src/lib/__tests__/naming.test.ts` with the same cases as the platform twin plus the slash-form `feat/foo`.
Degenerate prefix-only input (`feat-`, `fix-`) untested — fallback chain invisible
apps/platform/src/api/utils/__tests__/metrics-explore-core.test.ts
`boundedBranchSlug('feat-')` exercises the three-layer fallback `stripped || branch.replace(/^-+|-+$/g,'') || branch`. The CDK comment explicitly calls this out as a degenerate case but no test pins it. The fallback could be silently dropped in a cleanup pass. Expected: `boundedBranchSlug('feat-') === 'feat'`.
No assertion that `feat/foo` and `fix/foo` produce identical bounded slugs
apps/platform/src/api/utils/__tests__/metrics-explore-core.test.ts
CLAUDE.md documents that two branches differing only by type prefix share physical CDK names and the second deploy fails loudly. This invariant is untested — if the prefix regex diverges between twins the caveat silently breaks. Add: `expect(boundedBranchSlug('feat/my-feature')).toBe(boundedBranchSlug('fix/my-feature'))`.
Exact budget boundary (descriptive length === 25) not covered
apps/platform/src/api/utils/__tests__/metrics-explore-core.test.ts
The old test used a 23-char slug; the new test jumps to "way-too-long". The off-by-one at length 25 (one over budget) is not explicitly pinned. A simple `expect(boundedBranchSlug('feat-' + 'a'.repeat(24))).toBe('a'.repeat(24))` and a length-25 hash case would cover the boundary.
improvement2
IAM patterns hardcode both spellings instead of deriving from `abbreviateForBranch`
infra/cdk/src/stacks/cicd/vercel-service-user.stack.ts
`HelioscopeIntakeS3Policy` and `HelioscopeInvokePolicy` now manually list both `helioscope` and `helio` spellings. The connection to `BRANCH_TOKEN_ABBREV` is in a comment only. `abbreviateForBranch('metrics-helioscope-intake')` already returns `'metrics-helio-intake'`, so the abbreviated ARNs could be computed programmatically, eliminating the drift risk when the abbreviation maps change.
`BranchNameLengthAspect` SecretsManager/Alarm entries may be permanently dormant
infra/cdk/src/aspects/branch-name-length.ts
Neither `AWS::SecretsManager::Secret` nor `AWS::CloudWatch::Alarm` use the `batu-{slug}-` prefix in current stacks (secrets follow `batu/{env}/…` with slashes; no alarms are currently defined). The entries are safe forward-protection but will never fire against existing resources. Worth documenting that these are preemptive guards.