Skip to content

fix(scripts): cdk-floors check does not enforce floor monotonicity across the peer graph #377

Description

@laazyj

Problem / use case

ADR-0008 states the invariant:

Floors are monotonic with the peer graph — a package's floor is the max of its own aws-cdk-lib usage and its @composurecdk peers' floors, which holds automatically because a package can only load once its peers do.

That holds for a real install, but not for the declared manifest, and the declared manifest is what we publish. Nothing computes the max; cdk-floors.json is entirely hand-authored, and check() (scripts/cdk-floors.mjs:98-129) only compares the manifest against each package.json:

for (const [pkg, { floor, peerFloors }] of Object.entries(floors)) {
  const peers = JSON.parse(readFileSync(pkgJsonPath(pkg), "utf8")).peerDependencies ?? {};
  if (peers["aws-cdk-lib"] !== `^${floor}`) { /* mismatch */ }
  // …no comparison against the floors of pkg's own @composurecdk peers
}

So the invariant survives only as prose that each dependent hand-copies into its gatedBy string. Four entries currently restate a peer's floor rather than deriving it:

  • ses — "so ses cannot install below route53's own floor of 2.225.0"
  • cloudfront — "also peers @composurecdk/s3 at 2.123.0"
  • dynamodb — "peers @composurecdk/cloudwatch at 2.93.0"
  • apigateway, events — "reached transitively via @composurecdk/cloudwatch's alarm-threshold-basis.ts"

#376 is the first change to actually move a floor that another package inherits (route53 2.216.0 → 2.225.0, dragging ses with it). Both entries had to be edited by hand, and had the ses one been missed, npm run cdk-floors:apply and npm run cdk-floors:check would both have passed while @composurecdk/ses published aws-cdk-lib: ^2.216.0 — a range under which its own route53 peer does not compile. The consumer, not CI, finds that out.

Why the existing gates don't reliably cover it

enforce is the gate that could catch it, but only incidentally and only sometimes:

  • It shards by declared floor, so an under-declared dependent lands in the wrong shard. In the missed-ses scenario, ses sits in the 2.216.0 shard, nx builds its route53 dependency there (testdependsOn: ["^build", "typecheck"]), route53's tshy build fails to compile against 2.216.0, and the shard goes red — good, but the diagnostic is a TypeScript error inside a different package than the one whose floor is wrong.
  • It only covers what the dependent's suite compiles and executes. A dependent that peers a package but never triggers the version-gated path in a built artefact can still pass.
  • It is the expensive gate: a from-scratch npm install per floor, one CI job per shard. check is the cheap one that runs in the main job and in verify, and it is the one that is blind here.

The result is that a mechanical, statically-checkable invariant is enforced by review attention.

Proposed solution

The peer graph is already machine-readable — every cross-package edge is a peerDependencies entry (ses → core, route53), which is exactly how nx builds its project graph today. Three options, cheapest first:

1. Assert monotonicity in check (minimal). Walk each package's @composurecdk/* peers, and fail when a declared floor is lower than a peer's declared floor:

cdk-floors check failed — floor lower than a peer's:
  ses declares ^2.216.0 but peers @composurecdk/route53 (^2.225.0).
  Raise ses to 2.225.0, or lower route53.

Keeps the manifest hand-authored and every number reviewable; adds no new concepts. Catches the missed-dependent case in the cheap gate with an error that names the right package.

2. Derive the effective floor in apply/check (recommended). Compute effective(pkg) = max(own, effective(peers…)) over a topological walk, write that to package.json, and have check compare against it. Manifest entries then declare only each package's own APIs, and gatedBy prose stops duplicating peers' rationale — ses's entry would say "SES APIs are all ≤ 2.93.0" and nothing about route53, because the tooling knows. Also fixes groupByFloor, so enforce shards by the floor that will actually be published. Costs: the manifest no longer literally shows the published range (mitigate by printing derived values in apply's output and in check's success line), and a cycle guard is needed, though the graph is a DAG today.

3. Add a monotonicity unit test instead of tooling. Same assertion as (1) but as a test rather than a gate on apply. Cheapest to write, but it splits floor logic across scripts/ and a test suite; only worth it if we want the check to run without invoking the script.

Recommendation: (2), with (1) as the fallback if keeping the manifest literal is preferred. Either way docs/adr/0008 should be amended — a short note that the peer-graph max is computed and enforced, replacing the current "holds automatically" claim, which is only true of installs.

Out of scope: the peerFloors lockstep alphas (e.g. @aws-cdk/aws-neptune-alpha) — those track aws-cdk-lib releases, not the workspace graph, and their existing handling is correct.

Alternatives considered

  • Leave it as prose and rely on review. What we do today. It survived until the first inherited-floor bump; there is no reason to expect the next one to be as visible, since a forgotten dependent produces no local symptom at all — the workspace always installs the latest CDK.
  • Rely on enforce alone. Late, expensive, shard-dependent, and it reports the failure in the wrong package (see above).
  • Collapse to one library-wide floor. Rejected by ADR-0008 for good reason: it would drag cloudformation (2.1.0) up to route53's 2.225.0.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions