Skip to content

docs(design): CI build performance roadmap + implementer notes (#363) - #364

Merged
scottschreckengaust merged 4 commits into
mainfrom
docs/363-ci-build-performance
Jul 29, 2026
Merged

docs(design): CI build performance roadmap + implementer notes (#363)#364
scottschreckengaust merged 4 commits into
mainfrom
docs/363-ci-build-performance

Conversation

@scottschreckengaust

@scottschreckengaust scottschreckengaust commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Documents the CI build-performance roadmap in docs/design/CI_BUILD_PERFORMANCE.md (with its generated Starlight mirror). This satisfies the "keep CI_BUILD_PERFORMANCE.md current" DoD bullet of umbrella issue #363 — it is docs-only and implements no perf lever.

Relates to #363

#363 is an umbrella issue and stays OPEN after this docs PR merges. The remaining performance levers are tracked as separate ADR-003 child issues (see below), not implemented here.

What the doc covers

Review remediation (theagenticguy)

Reviewer supplied measured merge_group data; all figures below cite the runs they came from — 30412723686 (Build 171s, //cdk:test 125.08s / 136 suites), 30409171662 (186s), 30400147506 (148s), all 4-core, 2026-07-28/29. Seven inline comments addressed and replied in-thread:

  1. Stale ledger — split so perf(test): skip redundant jest type-check via isolatedModules (#357) #359 (type-check removal, 2026-06-16) and perf(test): disable Lambda bundling in CDK unit-test synths (~15x per synth) (#366) #371 (Lambda-bundling disable, merged 2026-06-17) each get credit; added a "Current baseline" table (build ~148–186s, //cdk:test ~125s) with the run IDs, replacing the stale ~346s/~298s post-Installation: docker image inspect fails #1 column.
  2. "Paid for twice" scopetsc --build includes src/** only, so src/ went twice→once but test/ went once→zero; noted the eslint tsconfig.dev.json backstop and a deferred tsc --noEmit -p tsconfig.dev.json lever.
  3. Idle-window claim — corrected to ~29s (synth ends +142s vs //cdk:test +171s); named //cdk:synth:quiet as the next binding constraint.
  4. fixed_overhead — restated as 120–220s measured from job start (not ~95s from install).
  5. Shard math — recomputed with measured inputs → 4-way lands at/above the whole build; demoted Docs: specify that you can't use the agent with the canned repo #2 to deferred (revisit trigger //cdk:test > ~250s).
  6. Inverted if: — replaced with GitHub's documented pattern: job if: always() + gate moved into a step keying on failure || cancelled; explained why bare contains() never runs.
  7. Check-run names — corrected to build (agentcore) (build.yml compute_type matrix + ruleset 14980587, confirmed PR chore(deps): actions: bump the all-actions group across 1 directory with 4 updates #672); flagged as a REQUIRED-CONTEXT change, not workflow-only.

Re-review remediation round 2 (theagenticguy)

Re-review confirmed the 7 findings above are fixed + resolved, then opened 3 new inline comments plus a dropped nav finding. All addressed and replied in-thread; corrections cross-checked against per-step job timings (gh api .../jobs/<id> .steps[].started_at/completed_at), not log arithmetic:

  1. Cache-fix is net-neutral-to-slower — install cost is cache-independent (was rec chore(deps): bump defu from 6.1.4 to 6.1.6 in /docs #6's over-promoted premise). Comparing a miss merge_group run (30412723686) vs a hitting pull_request run (30419878840) with the identical yarn.lock hash: Cache * restore 1s→9s, yarn Done in 48.35s55.09s, Install dependencies 56s→60s — the hit path is net slower. Cause: mise.toml [tasks.install] runs yarn install --check-files, which re-verifies every restored file; uv sync installs 127 pkgs in 121ms so a venv cache can't pay for itself. Merge-queue cache misses are also largely structural (PR-run caches are scoped to refs/pull/.../merge). Rec chore(deps): bump defu from 6.1.4 to 6.1.6 in /docs #6 reframed as a --check-files/earn-their-restore investigation and demoted in sequencing; every "cache saves time" / "shaves the job-setup tax" claim removed.
  2. Free Disk Space is the dominant addressable overhead — added as new ledger item chore(project): update structure #7, ahead of chore(deps): bump defu from 6.1.4 to 6.1.6 in /docs #6 (and Installation: docker image inspect fails #1 in sequencing). Per-step share: run 30412723686 (job 90452344735) 148s/220s = 67%; run 30419878840 (job 90474296240) 97s/179s = 54%; run 30418865914 (job 90471207016) 75s/158s = 47%. Framed as the drop-or-narrow experiment the measurement protocol demands; the Node 22.23.1 re-download is a 4-5s smell, no-ENOSPC caveat preserved. Feeds the shard math (cut it → fixed_overhead 120-220s→60-100s).
  3. fixed_overhead breakdown named inline — the Mechanism sentence now reads "120-220s, of which Free Disk Space is 75-148s and install is a cache-independent ~56-60s — so the overhead is mostly addressable." The ~250s shard trigger is made conditional on overhead staying 120-220s (move it down if chore(project): update structure #7 lands).
  4. Dropped nav finding (no thread) — the Architecture sidebar in docs/astro.config.mjs is an explicit list, so the page would not appear in nav (Astro still builds the route, so //docs:build didn't catch it). Added slug entries architecture/ci-build-performance and architecture/bedrock-cost-attribution (the latter was also missing). Confirmed against the generated mirror filenames; astro check + //docs:build (70 pages) both pass.

Docs-only — perf levers are tracked as child issues

The remaining levers are not implemented in this PR. They are proposed as ADR-003 child issues of #363 (for the orchestrator to open), re-ranked per the round-2 findings:

  1. Drop / narrow the Free Disk Space step — 47–67% of per-job overhead; the single biggest addressable pre-build cost. Run as a measured experiment (peak disk + 4-core before/after).
  2. Bump the default runner (4-core → 8/16-core) — via vars.DEFAULT_RUNNER_LABEL; helps both //cdk:test and //cdk:synth:quiet.
  3. Path-filtered builds — make //cdk:test conditional (e.g. dorny/paths-filter) so docs/CLI/agent-only PRs skip it, while build (agentcore) still reports.
  4. Gate coverage to merge_group — skip collectCoverage on pull_request push; keep thresholds enforced on merge_group.
  5. Investigate yarn install --check-files / CI cachesnot a cache-hit-rate win: measured install cost is cache-independent (net-neutral-to-slower on a hit). A "delete or fix --check-files" investigation.
  6. Shard the CDK suitedeferred on current numbers (overhead swamps a ~125s suite); re-cost if //cdk:test exceeds ~250s, and note the trigger moves down if the Free Disk Space step is cut.

Testing

  • mise //docs:sync — no drift (committed Starlight mirror matches the regenerated output).
  • pre-commit run --files ... — sync docs → Starlight mirrors Passed, astro check (docs) Passed, gitleaks Passed.
  • mise //docs:build — full Astro/Starlight build Complete (70 pages); both new sidebar slugs resolve (architecture/ci-build-performance, architecture/bedrock-cost-attribution).
  • Rebased onto latest origin/main; clean, no conflicts. Scoped gitleaks on origin/main..HEADno leaks (the 3 full-history findings are pre-existing aws-account-id false positives unrelated to this diff).

🤖 Generated with Claude Code

@scottschreckengaust
scottschreckengaust requested review from a team as code owners June 16, 2026 22:08
@scottschreckengaust
scottschreckengaust marked this pull request as draft June 16, 2026 22:20
scottschreckengaust added a commit that referenced this pull request Jun 17, 2026
The doc lives in draft PR #364 and exists on neither this branch nor
main, so the three links (disable-bundling.ts, AGENTS.md, review_pr.md)
are dead. Drop them and keep the stable #366 reference, removing the
merge-order dependency on #364 and the link-checker (#300) risk.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isadeks pushed a commit to isadeks/sample-autonomous-cloud-coding-agents that referenced this pull request Jun 18, 2026
… synth) (aws-samples#366) (aws-samples#371)

* perf(test): disable Lambda bundling in CDK unit-test synths (aws-samples#366)

Template.fromStack() triggers a full CDK synth that bundles every
NodejsFunction via esbuild — ~28s for the 413-resource AgentStack. Unit
tests assert on CloudFormation structure, not bundled Lambda code, so the
bundling is pure overhead. A jest setupFiles hook sets
aws:cdk:bundling-stacks: [] via CDK_CONTEXT_JSON, which a bare new App()
picks up with no call-site changes.

Measured: single AgentStack synth 28.7s -> 1.9s (~15x); github-tags.test.ts
~135s -> 9s; full //cdk:test 155s -> 25s (8-core local). All 2177 tests
pass; coverage thresholds unchanged (lines 92.36 >= 91).

Durable regression guards added: AGENTS.md "Common mistakes" entry and a
.abca/commands/review_pr.md checklist item — do not re-enable bundling in
tests unless asserting on a bundled asset, and minimize full-stack synths
(synth once in beforeAll). Both link docs/design/CI_BUILD_PERFORMANCE.md
(PR aws-samples#364).

Refs aws-samples#366. Part of aws-samples#363. Likely supersedes the sharding approach (aws-samples#365) —
to be re-evaluated once this lands.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs(test): correct bundling opt-out to postCliContext (aws-samples#366)

The documented per-test opt-out used constructor `context`, which CDK
silently overwrites with CDK_CONTEXT_JSON in App.loadContext — so
`new App({ context: { 'aws:cdk:bundling-stacks': ['**'] } })` does NOT
re-enable bundling. Only `postCliContext` (applied last) overrides the
global disable.

Corrects the opt-out instruction and precedence comment in all three
documented locations (disable-bundling.ts, AGENTS.md, review_pr.md).
Verified empirically against aws-cdk-lib@2.257.0. Comment/doc-only;
runtime logic unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): clarify bundling opt-out is for asset output, not synth (aws-samples#366)

Disabling bundling does not stop tests from synthesizing —
`Template.fromStack()` still runs a full synth; it only skips the
esbuild asset-bundling step. The `postCliContext` opt-out exists solely
for the rare test that needs real bundled-asset output (asset hash /
S3 key), where an unbundled synth would silently yield a placeholder.
Makes this explicit in disable-bundling.ts and AGENTS.md. Doc-only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): drop dead CI_BUILD_PERFORMANCE.md links (aws-samples#366)

The doc lives in draft PR aws-samples#364 and exists on neither this branch nor
main, so the three links (disable-bundling.ts, AGENTS.md, review_pr.md)
are dead. Drop them and keep the stable aws-samples#366 reference, removing the
merge-order dependency on aws-samples#364 and the link-checker (aws-samples#300) risk.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(test): harden bundling-disable against ambient CDK_CONTEXT_JSON (aws-samples#366)

Two robustness fixes to the global bundling-disable setup file, both from
PR aws-samples#371 review (finding 1 hardening nits):

- Spread BUNDLING_DISABLED_CONTEXT LAST when merging a pre-set
  CDK_CONTEXT_JSON, so an ambient `aws:cdk:bundling-stacks` value can no
  longer silently win and re-enable bundling. This is safe precisely because
  the documented per-test opt-out is `postCliContext` (applied after the env
  var regardless), so the escape hatch still works.
- Guard `JSON.parse` of the pre-set var with try/catch. A malformed value
  previously threw inside setupFiles and failed every test in every file with
  an opaque parse error; now the blame is localized to the offending value.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(cdk): add executable guard that Lambda bundling is disabled (aws-samples#366)

PR aws-samples#371 review finding 1: the ~15× synth speedup rested entirely on the
`disable-bundling.ts` setupFiles entry, but the only regression guards were
honor-system prose (AGENTS.md) and a review_pr.md checklist item. Dropping the
setupFiles wiring, reordering Jest setup, or a CDK rename of the context key
would silently revert the suite to full-bundling synths with no failing check.

Adds two assertions: the synth context carries `aws:cdk:bundling-stacks: []`,
and a bare-App stack reports `bundlingRequired === false`. Verified empirically
against aws-cdk-lib@2.259.0 that without the disable a bare App yields
`bundlingRequired === true`, so these discriminate a real regression.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(cdk): cover the postCliContext bundling opt-out precedence (aws-samples#366)

PR aws-samples#371 review finding 2: the documented per-test opt-out (`postCliContext`)
and the `loadContext` precedence this PR corrected had zero test coverage, so a
future CDK version reordering context application could silently break the one
supported escape hatch.

Locks the contract with two assertions verified against aws-cdk-lib@2.259.0:
`postCliContext: { 'aws:cdk:bundling-stacks': ['**'] }` re-enables bundling
(`bundlingRequired === true`), while constructor `context` does NOT (the env
var set by the global disable clobbers it). Uses a bare `Stack` rather than a
heavy AgentStack since the precedence is a property of CDK context loading, not
of any particular stack.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): reframe bundling as overhead-plus-smoke-test, not "pure overhead" (aws-samples#366)

PR aws-samples#371 review finding 3: "pure overhead" undersold the tradeoff. esbuild
bundling also validates that every Lambda `entry` resolves and the handler
compiles, so disabling it in unit tests means a broken entry path or handler TS
error stops surfacing at `Template.fromStack()` and only fails at real
synth/deploy or in the `agent/` build. The tradeoff is still acceptable — real
synth/deploy bundles, and handler code has its own compile/test under `agent/`
— but the wording now names what unit tests cede. Updated in both places the
phrase lived: the disable-bundling.ts header and AGENTS.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: bgagent <345885+scottschreckengaust@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
isadeks pushed a commit to isadeks/sample-autonomous-cloud-coding-agents that referenced this pull request Jul 28, 2026
… synth) (aws-samples#366) (aws-samples#371)

* perf(test): disable Lambda bundling in CDK unit-test synths (aws-samples#366)

Template.fromStack() triggers a full CDK synth that bundles every
NodejsFunction via esbuild — ~28s for the 413-resource AgentStack. Unit
tests assert on CloudFormation structure, not bundled Lambda code, so the
bundling is pure overhead. A jest setupFiles hook sets
aws:cdk:bundling-stacks: [] via CDK_CONTEXT_JSON, which a bare new App()
picks up with no call-site changes.

Measured: single AgentStack synth 28.7s -> 1.9s (~15x); github-tags.test.ts
~135s -> 9s; full //cdk:test 155s -> 25s (8-core local). All 2177 tests
pass; coverage thresholds unchanged (lines 92.36 >= 91).

Durable regression guards added: AGENTS.md "Common mistakes" entry and a
.abca/commands/review_pr.md checklist item — do not re-enable bundling in
tests unless asserting on a bundled asset, and minimize full-stack synths
(synth once in beforeAll). Both link docs/design/CI_BUILD_PERFORMANCE.md
(PR aws-samples#364).

Refs aws-samples#366. Part of aws-samples#363. Likely supersedes the sharding approach (aws-samples#365) —
to be re-evaluated once this lands.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs(test): correct bundling opt-out to postCliContext (aws-samples#366)

The documented per-test opt-out used constructor `context`, which CDK
silently overwrites with CDK_CONTEXT_JSON in App.loadContext — so
`new App({ context: { 'aws:cdk:bundling-stacks': ['**'] } })` does NOT
re-enable bundling. Only `postCliContext` (applied last) overrides the
global disable.

Corrects the opt-out instruction and precedence comment in all three
documented locations (disable-bundling.ts, AGENTS.md, review_pr.md).
Verified empirically against aws-cdk-lib@2.257.0. Comment/doc-only;
runtime logic unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): clarify bundling opt-out is for asset output, not synth (aws-samples#366)

Disabling bundling does not stop tests from synthesizing —
`Template.fromStack()` still runs a full synth; it only skips the
esbuild asset-bundling step. The `postCliContext` opt-out exists solely
for the rare test that needs real bundled-asset output (asset hash /
S3 key), where an unbundled synth would silently yield a placeholder.
Makes this explicit in disable-bundling.ts and AGENTS.md. Doc-only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): drop dead CI_BUILD_PERFORMANCE.md links (aws-samples#366)

The doc lives in draft PR aws-samples#364 and exists on neither this branch nor
main, so the three links (disable-bundling.ts, AGENTS.md, review_pr.md)
are dead. Drop them and keep the stable aws-samples#366 reference, removing the
merge-order dependency on aws-samples#364 and the link-checker (aws-samples#300) risk.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(test): harden bundling-disable against ambient CDK_CONTEXT_JSON (aws-samples#366)

Two robustness fixes to the global bundling-disable setup file, both from
PR aws-samples#371 review (finding 1 hardening nits):

- Spread BUNDLING_DISABLED_CONTEXT LAST when merging a pre-set
  CDK_CONTEXT_JSON, so an ambient `aws:cdk:bundling-stacks` value can no
  longer silently win and re-enable bundling. This is safe precisely because
  the documented per-test opt-out is `postCliContext` (applied after the env
  var regardless), so the escape hatch still works.
- Guard `JSON.parse` of the pre-set var with try/catch. A malformed value
  previously threw inside setupFiles and failed every test in every file with
  an opaque parse error; now the blame is localized to the offending value.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(cdk): add executable guard that Lambda bundling is disabled (aws-samples#366)

PR aws-samples#371 review finding 1: the ~15× synth speedup rested entirely on the
`disable-bundling.ts` setupFiles entry, but the only regression guards were
honor-system prose (AGENTS.md) and a review_pr.md checklist item. Dropping the
setupFiles wiring, reordering Jest setup, or a CDK rename of the context key
would silently revert the suite to full-bundling synths with no failing check.

Adds two assertions: the synth context carries `aws:cdk:bundling-stacks: []`,
and a bare-App stack reports `bundlingRequired === false`. Verified empirically
against aws-cdk-lib@2.259.0 that without the disable a bare App yields
`bundlingRequired === true`, so these discriminate a real regression.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(cdk): cover the postCliContext bundling opt-out precedence (aws-samples#366)

PR aws-samples#371 review finding 2: the documented per-test opt-out (`postCliContext`)
and the `loadContext` precedence this PR corrected had zero test coverage, so a
future CDK version reordering context application could silently break the one
supported escape hatch.

Locks the contract with two assertions verified against aws-cdk-lib@2.259.0:
`postCliContext: { 'aws:cdk:bundling-stacks': ['**'] }` re-enables bundling
(`bundlingRequired === true`), while constructor `context` does NOT (the env
var set by the global disable clobbers it). Uses a bare `Stack` rather than a
heavy AgentStack since the precedence is a property of CDK context loading, not
of any particular stack.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(test): reframe bundling as overhead-plus-smoke-test, not "pure overhead" (aws-samples#366)

PR aws-samples#371 review finding 3: "pure overhead" undersold the tradeoff. esbuild
bundling also validates that every Lambda `entry` resolves and the handler
compiles, so disabling it in unit tests means a broken entry path or handler TS
error stops surfacing at `Template.fromStack()` and only fails at real
synth/deploy or in the `agent/` build. The tradeoff is still acceptable — real
synth/deploy bundles, and handler code has its own compile/test under `agent/`
— but the wording now names what unit tests cede. Updated in both places the
phrase lived: the disable-bundling.ts header and AGENTS.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: bgagent <345885+scottschreckengaust@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@scottschreckengaust
scottschreckengaust force-pushed the docs/363-ci-build-performance branch 2 times, most recently from 06a1f09 to d32c129 Compare July 29, 2026 00:54
@scottschreckengaust
scottschreckengaust marked this pull request as ready for review July 29, 2026 01:04
@scottschreckengaust

Copy link
Copy Markdown
Contributor Author

✅ Acceptance summary (for the reviewer)

#363 DoD mapped to this change — this PR satisfies the "keep CI_BUILD_PERFORMANCE.md current" bullet of the #363 umbrella only; it is docs-only (source + regenerated Starlight mirror, no code/CI/workflow changes).

  • Every factual CI-claim in the doc re-verified against current origin/main (runner resolution via vars.DEFAULT_RUNNER_LABEL, build on both pull_request + merge_group, isolatedModules, coverage thresholds in contracts/coverage-thresholds.json) — all accurate.
  • /review_pr = approve-with-nits, zero blocking; both nits fixed (wall-time ~170s (4-way); suite count well over a hundred).
  • Uses Relates to #363 (NOT Closes): perf(ci): remaining build-performance levers (shard, coverage-gate, runner, path-filter) — follow-ups to #357 #363 is an umbrella that stays open — the remaining perf levers are tracked as the child issues below.

🔀 Merge guidance (for the reviewer)

Independent — no ordering constraint. Cluster ci-perf-docs (disjoint from every other PR in this batch); touches only docs/design/CI_BUILD_PERFORMANCE.md + its generated mirror. Safe to merge in any order relative to the other batch PRs. Action: review + approve at will; native auto-merge is disabled repo-wide, so this awaits your manual squash-merge.

🤖 orchestrator note (agent) — promotion is orchestrator-driven; merge remains a human action.

@theagenticguy theagenticguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: docs-only, but the numbers it enshrines are stale enough to send the next implementer the wrong way

We verified every load-bearing claim against the repo at 6e7ae0f and against real merge_group run logs. The structure, the required-check reasoning, and the coverage-merge caveat are genuinely good, and the Starlight mirror is byte-identical to node docs/scripts/sync-starlight.mjs output, so the mutation gate will pass. Four things need fixing before merge.

1. The headline numbers describe a build that no longer exists. The doc's ~346s build / ~298s //cdk:test were accurate on 2026-06-16. PR #371 ("disable Lambda bundling in CDK unit-test synths, ~15x per synth", merged 2026-06-17, one day later) cut them again. Measured on 4-core merge_group runs from the last week:

Doc claims Measured 2026-07-28/29
build step ~346s 148s, 171s, 176s, 186s, 192s
//cdk:test ~298s ~125s (run 30412723686: Time: 125.08 s, 136 suites)
//cdk:test share ~86% ~74%

The doc credits the entire 710s->346s win to item #1 and never mentions #371. A reader concludes the long pole is 298s when it is 125s.

2. That staleness inverts the doc's central recommendation. Sharding is ranked first on the strength of a 298s long pole and ~95s per-job overhead. Both inputs are wrong in the direction that kills the case. See the inline comment on the shard table: at today's numbers, 4-way sharding lands each shard above the current whole-build wall time. The doc should either re-measure and re-rank, or state plainly that #2's case depends on numbers that need re-verification first.

3. Two concrete defects in the recommended workflow snippet would break the merge queue if implemented verbatim. Both are inline: the if: condition is inverted, and the aggregate job name does not match this repo's actual required context (build (agentcore), not build).

4. The page will not appear in the docs nav. docs/astro.config.mjs lists the Architecture sidebar entries explicitly rather than autogenerating them, so a new docs/design/*.md needs a matching { slug: 'architecture/ci-build-performance' } entry. Astro will still build the route, which is why mise //docs:build passed. Note BEDROCK_COST_ATTRIBUTION is missing from that list too, so this looks like a repeat oversight worth fixing in the same pass.

One process note: this PR is still a draft. Marking it ready for review would let the required checks report.

Commands used to verify: gh run view <id> --log on runs 30412723686, 30409171662, 30400147506; gh api repos/.../rulesets/14980587; node docs/scripts/sync-starlight.mjs diffed against the committed mirror.

Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
scottschreckengaust and others added 3 commits July 29, 2026 03:28
Records how the build workflow's cost is distributed (//cdk:test was the
~91% long pole), why it was expensive (redundant type-check, fixed in #357
/ PR #359), and the remaining levers (shard, coverage-gate, runner size,
path-filter) with sequence and implementer notes — including the
merge-queue required-check constraints that shape #2 and #5.

Companion to umbrella issue #363; keeps the optimization roadmap as a
durable, reviewable design artifact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Expands the CI build-performance doc with a full design for item #2
(shard the CDK suite): the jest --shard mechanism, a shards-vs-wall-time
table showing fixed overhead dominates past ~4 shards, the fan-out +
aggregate-gate pattern needed to keep `build` a single required context
on merge_group, cross-shard coverage merge before threshold enforcement,
and open questions (synth placement, deploy artifact, shard balance).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nalysis per review (#363)

Remediates @theagenticguy's CHANGES_REQUESTED (7 inline comments) with measured
merge_group data from runs 30412723686 (Build 171s, //cdk:test 125.08s / 136 suites),
30409171662 (186s), 30400147506 (148s):

1. Ledger split so #359 (type-check removal) and #371 (Lambda-bundling disable) each
   get credit; restated current baseline (build ~148-186s, //cdk:test ~125s) with the
   run IDs it came from — not the stale ~346s/~298s post-#1 column.
2. 'Paid for twice' scoped: src/ went twice->once; test/ went once->zero (tsc only
   includes src/**); eslint tsconfig.dev.json backstops narrowly; noted deferred
   tsc --noEmit -p tsconfig.dev.json lever.
3. Idle-window claim corrected: synth ends +142s vs //cdk:test +171s -> ~29s idle, not
   ~200s; named //cdk:synth:quiet as the next binding constraint for sharding.
4. fixed_overhead restated as 120-220s measured from job start (not ~95s from install);
   added cache-miss finding (node_modules/venv/jest miss on every merge_group run) as
   new recommendation #6.
5. Shard math recomputed with measured inputs -> 4-way lands at/above whole build; #2
   demoted to deferred (revisit trigger //cdk:test > ~250s); sequencing re-ranked to
   elevate cache hit-rate (#6), runner size (#4), path filters (#5) over sharding.
6. Aggregate-job if: fixed to GitHub's documented pattern (job if: always(), gate moved
   into a step keying on failure||cancelled); explained why bare contains() never runs.
7. Check-run names corrected to build (agentcore) (build.yml compute_type matrix +
   ruleset 14980587, confirmed PR #672); shard dim yields build (agentcore, N); flagged
   as a REQUIRED-CONTEXT change, not workflow-only.

Rebased onto latest origin/main; regenerated Starlight mirror via docs:sync.

Relates to #363

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

@theagenticguy theagenticguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: all 7 inline findings genuinely fixed. The revision introduced one new wrong recommendation, and one original finding was dropped.

We re-verified every claim in the revised doc against the repo at a91b763 and against fresh run logs, including authoritative per-step timings from gh api .../actions/jobs/<id> rather than log-timestamp arithmetic.

The 7 inline threads are accurately resolved — we resolved them ourselves. Each was checked against the file, not just against the reply:

Thread Claim Verified
Stale ledger #1/#1b split, baseline table with all 3 run IDs ✅ lines 38-60
Type-check scope src/ twice→once, test/ once→zero, eslint backstop, deferred tsc --noEmit lever ✅ lines 27-32
Idle window per-task end offsets, ~29s not ~200s, synth named next constraint ✅ lines 7-21
fixed_overhead boundary job-start measurement, 120-220s, extra steps enumerated ✅ lines 80, 96
Shard math recomputed table, demoted to Deferred, ~250s trigger ✅ lines 100-106
Inverted if: always() on job + gate moved to step ✅ lines 136-149
Check-run names build (agentcore), both corrections, required-context framing ✅ lines 110-123

The Starlight mirror is byte-identical to node docs/scripts/sync-starlight.mjs output, so the mutation gate passes.

What blocks merge now. The revision promoted our cache observation into new recommendation #6 and ranked it first. We only reported that caches miss on merge_group; we never established that fixing that would save time. It does not, and we should have said so the first time rather than leaving an unmeasured aside to be promoted. Measured, same yarn.lock hash on both runs:

caches missing (30412723686) caches hitting (30419878840)
Cache * restore steps 1s 9s
yarn install --check-files Done in 48.35s Done in 55.09s
Install dependencies step 56s 60s

The cache-hit path is slower. mise.toml runs yarn install --check-files, which re-verifies every file and defeats a restored node_modules. So #6 as written would send an implementer after a win that does not exist, which is the same failure mode as the original stale-numbers finding.

The actual dominant cost is the Free Disk Space step: 148s / 97s / 75s out of 220s / 179s / 158s total pre-build time, so 47-67% of the overhead the shard math depends on. It also deletes /opt/hostedtoolcache, after which Setup Node.js re-downloads Node 22.23.1. Details inline.

And one original finding got no reply and no fix: the doc page still will not appear in the site nav. docs/astro.config.mjs is unchanged on this branch and its Architecture sidebar is an explicit list, so docs/design/CI_BUILD_PERFORMANCE.md needs a matching { slug: 'architecture/ci-build-performance' } entry. Astro still builds the route, which is why mise //docs:build passes and nothing catches it. BEDROCK_COST_ATTRIBUTION is still missing from that list too.

These are all small edits. Nothing structural is wrong with the doc anymore, and the sharding design and required-check section are now genuinely good implementer references.

Verified with: gh api repos/.../actions/jobs/{90452344735,90474296240,90471207016}, runs 30412723686 30419878840 30418865914 30409171662 30400147506, and node docs/scripts/sync-starlight.mjs diffed against the committed mirror.

Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
Comment thread docs/design/CI_BUILD_PERFORMANCE.md Outdated
…per re-review (#363)

Re-review round 2 (theagenticguy) surfaced 3 new inline findings plus a
dropped nav finding. Corrections, cross-checked against per-step job timings:

- Cache hits do not save time. Comparing a miss merge_group run
  (30412723686) vs a hitting pull_request run (30419878840) with the
  identical yarn.lock hash: Cache * restore 1s->9s, yarn 48.35s->55.09s,
  Install dependencies 56s->60s. The hit path is net SLOWER because
  mise.toml [tasks.install] runs yarn install --check-files, which
  re-verifies every restored file; uv sync installs 127 pkgs in 121ms so
  a venv cache cannot pay for itself. Rec #6 reframed as a --check-files
  investigation and demoted in sequencing; no more cache-win claims.
- Free Disk Space is the dominant addressable overhead, not cache misses.
  Per-step timings (gh api .../jobs/<id> .steps[]): run 30412723686 (job
  90452344735) 148s/220s = 67%; run 30419878840 (job 90474296240)
  97s/179s = 54%; run 30418865914 (job 90471207016) 75s/158s = 47%.
  Added as new ledger item #7 ahead of #6; feeds the shard math.
- fixed_overhead breakdown named inline: of 120-220s, Free Disk Space is
  75-148s and install is a cache-independent ~56-60s. ~250s shard trigger
  is now conditional on overhead staying 120-220s.
- Nav fix: astro.config.mjs Architecture sidebar is an explicit list, so
  added slug entries architecture/ci-build-performance and
  architecture/bedrock-cost-attribution (both were missing from nav).

Relates to #363

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

@theagenticguy theagenticguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve

All 10 findings across both prior rounds are fixed and verified against the file at 0152332, not against the replies. All 10 threads are now resolved.

This round's 3 threads:

Finding Verified
Rec #6 premise contradicted by measurement ✅ reworded to the --check-files investigation; merge-ref scoping named as structural; miss-vs-hit table cited (restore 1s→9s, yarn 48.35s→55.09s, install 56s→60s); uv sync 121ms; "shaves the 120-220s tax" claim removed; demoted to position 5
Free Disk Space is the real overhead driver ✅ new item #7 ahead of #6 and #1 in sequencing; all three per-step figures with job IDs (67% / 54% / 47%); sourced to gh api .../actions/jobs/<id>; Node re-download framed as a ~4-5s smell; no-ENOSPC caveat preserved; drop-or-narrow experiment with peak-disk reporting
fixed_overhead breakdown nit ✅ Mechanism names 75-148s disk + ~56-60s cache-independent install; re-measure-before-costing instruction added; #2's trigger carries the matching conditional

The nav finding from round one is fixed toodocs/astro.config.mjs now carries architecture/ci-build-performance, and it picked up the bedrock-cost-attribution entry that was missing before. Verified end to end rather than by inspection: the Architecture sidebar is 20 slugs against 20 pages with no orphans in either direction, and dist/architecture/architecture/index.html renders href="/sample-autonomous-cloud-coding-agents/architecture/ci-build-performance/", so the page is genuinely reachable from the nav.

Gates run locally against the branch:

  • node docs/scripts/sync-starlight.mjs → committed mirror byte-identical, so "Fail build on mutation" will pass
  • astro build → 70 pages, Complete
  • astro check → 0 errors, 0 warnings, 0 hints
  • link-check.sh → exit 0
  • Every ](#…) anchor in the doc resolves against the rendered HTML IDs

All 8 checks on the PR are green, including build (agentcore).

One note for whoever picks up #7, carried forward rather than blocking: the Free Disk Space timing varies 75-148s across the three sampled runs, which is wide enough that the 47-67% range is a sample rather than a stable estimate. Worth pulling a larger sample before sizing the experiment, and the doc's measurement protocol already asks for that.

Good revision. The doc now argues against its own original recommendation where the data says so, cites run IDs for every number, and the required-check section is a genuinely useful trap map for the next implementer.

@scottschreckengaust
scottschreckengaust added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit aa3372e Jul 29, 2026
9 checks passed
@scottschreckengaust
scottschreckengaust deleted the docs/363-ci-build-performance branch July 29, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants