Skip to content

preflight under 20 minutes, CI under 35 per job, and table keys hash the same on every rail - #3939

Merged
borisbat merged 17 commits into
masterfrom
bbatkin/preflight-20min
Sep 5, 2026
Merged

preflight under 20 minutes, CI under 35 per job, and table keys hash the same on every rail#3939
borisbat merged 17 commits into
masterfrom
bbatkin/preflight-20min

Conversation

@borisbat

@borisbat borisbat commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Behavior change: tables keyed by a 2- or 3-lane vector or a range kept losing keys after their first grow on every rail, and AOT missed such a key from the first insert; both are fixed, so code that worked around it by re-inserting or avoiding those key types can stop.

Preflight on the M5 box took 32 minutes without the dasLLAMA suites and CI's extended checks took 75. This branch puts both under a budget: a full preflight run fits 20 minutes, a per-PR CI job fits 35. Preflight gates now carry a tier and a reach set. The fast tier runs serially and a red stops the run; the lanes run only under --full, all at once as child processes, so the wall is the longest lane; module gates (imgui, sequence, dasLLAMA) run by name when that module is the work. Two gates are new: a compile sweep of every program root under utils, examples and tutorials, and the utils test suite. A full run with a core change now takes 11.5 minutes here.

CI's job matrices are data in ci/ci_matrix.py, evaluated per event by pre_job and pinned by ci/test_ci_matrix.py, which also runs as a workflow step and as a preflight gate. Per PR, extended checks are two darwin15 jobs, core and modules; the nightly runs every step on linux, darwin15 and windows plus the steps too slow for a PR (tutorial dry-runs, the run form of examples, coverage, the nano cross-compile). The sanitizer cells and windows 64 Debug are nightly-only build cells. CodeQL stays per PR.

The new AOT lane found a master bug: two hash definitions for table keys. The interpreter, the JIT helpers, the JSON scanner and the C API hash a builtin key on its own type, while KeyHash, used by AOT and by every grow's rehash, widened it to a vec4f first. KeyHash now hashes a builtin key as itself and a handled key as its annotation's workhorse, which is what the interpreter hashes in both cases. Three tests pin it on all rails, including a cross-tier one (interpreted write, AOT read) and a C++ cell per key type.

The default module cache had no eviction: this box held 1372 records, 28 GB, an engine root's record being 200 MB and every DAS environment or option variant minting another. The default directory is now capped at DAS_MODULE_CACHE_LIMIT megabytes (4096 unless set, 0 off): after a write the oldest records by mtime go, a record a run read counts as fresh, and an explicit -module-cache path is never pruned.

Where to look: include/daScript/simulate/runtime_table.h (KeyHash), cast.h and jit_abi.h (WrapsBuiltinValue); utils/internal/preflight/main.das (run_lanes, gate_compile_sweep); ci/ci_matrix.py; .github/workflows/extended_checks.yml (the role conditions); src/builtin/module_builtin_ast_serialize.cpp (evictModuleCache).

Validation, claims, ledger

Validation

  • Full AOT sweep on the fixed binary: 13210 tests, 0 failed, 3 skipped (local only; per-PR CI compiles the subset). Master's nightly AOT sweep was red on the same cell this fixes (tests/json/test_sscan_json, int2-keyed table).
  • tests/language/table_vector_keys.das red on the unfixed binary in 3 of 4 cells (int2, float3, uint2 and range; int4 green), 5/5 after on interp, JIT and AOT. tests-cpp/small/test_table_key_hash.cpp red against master's KeyHash in the vector and range rows and the detour case, 36/36 after.
  • preflight --full measured once: 690 s wall; fast tier 3.1 min, lanes tests-aot 513 s (build 402 + run 111), docs 336, utils-tests 186, tests-jit 184, tests-interp 116, tests-cpp 20. Later fixes validated by targeted gates (fast tier 12/12 twice, the three suites once).
  • The darwin per-PR extended_checks jobs and the nightly role all on linux and windows have not run yet: this PR's own CI run is that evidence, and a workflow_dispatch of extended_checks.yml is the nightly's.

Claims - stated, not tested

  • Preflight's orchestration arms (run_lanes, collect_changed_paths, the reach-skip and lane-deferral arms of main, the gate functions' skip and fail arms) have no unit test; the pure halves do (config.das, tests/test_changed_set.das). A spawned-child harness is the follow-up; tests/dastest/test_preflight_config.das is the precedent.
  • The moved steps' portable shells (a read loop for mapfile, perl's alarm for timeout, exit 142 for the alarm) run for the first time on this PR's darwin jobs.

Not done

  • The gate candidates the audits named: a workflows REVIEW.das for continue-on-error and || true on per-PR steps; examples/games/REVIEW.das reporting an inline game list beside a loop; a simulate REVIEW.das pinning the table-key hashing sites, or routing every site through KeyHash.
  • compute_worker_count(n, 0) still defaults to the jobque's thread count (5 on an 18-core M5), so the cpp-syntax sweep and detect-dupe run narrow there.
  • Two tracked generated files come back modified after every cmake build: modules/dasUnitTest/unit_test.das.inc, tutorials/integration/cpp/class_adapters_module.das.inc.
  • The dasLLAMA long tests (plan sec. "dasLLAMA long tests").

🤖 Generated with Claude Code

borisbat and others added 12 commits September 4, 2026 22:16
…, compile-sweep and utils-tests gates; make-pr's default chain slims

A gate carries a tier and a reach set. The fast tier runs serially on every run and a red stops the run; the lanes run only under --full, every lane at once as child preflight processes, so the wall is the longest lane; a module gate runs by name when the module is the work. A gate whose reach no changed path hits skips with the reason. Two gates are new: compile-sweep compiles every program root under utils/, examples/, tutorials/ and the modules' examples and utils, the engine roots serially through one shared module cache; utils-tests runs run_utils_tests. make-pr's default chain drops review-md and ast-verify (preflight owns them) and the advisory dupes report (--only runs it). The preflight skill and the make_pr rows carry the new shape.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…le cache, sizes its pool by cores, and excludes fixture trees

The 25 roots that require dasllama/ each paid a ~24 s engine compile at the tail of the pool; they now run one after another through a single -module-cache so the first pays and the rest deserialize. The light pool is sized by get_total_hw_cores() - get_total_hw_threads() is the jobque's own worker count and answers 5 on an 18-core box. Eight trees whose roots are fixtures, crash probes, or need an external SDK are excluded by prefix, and the gate's detail line reports the pool and serial walls separately.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e AOT gate names its failing files

A captured lane child writes every line through to_log, so `[I] [PASS] tests-jit (183.8s)` reached the parser with `I` as its tag and five green lanes reported as "exited 0 without a verdict". The parser drops a leading level prefix first; the unit test carries the captured shape. The tests-aot gate blamed error[50101] on every red; it now says so only when the output carries that code and otherwise lists the files from dastest's FAILURES block. The plan records the measured full run: 11.5 min wall on the M5, tests-aot the longest lane at 513 s.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…win roles per PR, and the over-budget cells move to the nightly

ci/ci_matrix.py emits the build.yml and extended_checks.yml cells per event; pre_job evaluates it and the fan-out job reads fromJSON(needs.pre_job.outputs.matrix), so the per-PR and nightly sets are two lists a test pins (ci/test_ci_matrix.py) instead of an include list the runner merges. Per PR, extended_checks is two darwin15 jobs, core and modules, each inside the 35-minute budget; a step's condition is matrix.role != '<other>' so the nightly role all runs every step on linux, darwin15 and windows, plus the steps too slow for a PR: tutorial dry-runs, the run form of examples, daslang_static, coverage. The linux-only steps that moved onto darwin lost their bash-4 and coreutils dependencies (a read loop for mapfile, perl's alarm for timeout). build.yml's sanitizer cells and windows 64 Debug are nightly-only and save no sccache slot; extended_checks' slot cap rises from 500M to 1500M so the nightly save holds a complete object set; CodeQL drops its pull_request trigger. The workflows checklist admits the budget narrowing and requires the role spelling; the preflight skill and the plan carry the new lane map.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… a table keeps its keys past the first grow

The value nodes, the JIT helpers, the JSON scanner, rtti and the C API hash a key with hash_function on the key's own type - 8 bytes for an int2, 12 for a float3, 8 for a range - while KeyHash widened such a key to its vec4f workhorse and hashed 16. KeyHash is what AOT's TTable uses and what every grow's rehash uses, so on the interpreter and the JIT an int2-keyed table lost most of its keys once it grew past 8 slots (31 of 40 lookups missed), and under AOT a lookup missed from the first key - the nightly AOT sweep's one red, tests/json/test_sscan_json's int2 cell. KeyHash now takes the workhorse detour only when it changes no bytes (Time, handles, smart pointers, 16-byte vectors). tests/language/table_vector_keys.das grows int2, float3, uint2, range and int4 keyed tables past several grows on all three rails; the simulate headers checklist carries the one-hash rule.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rkflows checklist states the budget move as three rules

ci/test_ci_matrix.py had no caller: it now runs as extended_checks' core-role step and as preflight's fast-tier ci-matrix gate, reached by a change under .github/ or ci/. The workflows checklist folds its definition into the weakening rule, splits the add/change duty and the 35-minute nightly move into rules of their own, and keeps only the weakening residue for the role spelling the test enforces. daslang_static stays a per-PR step in the modules role - nothing local sweeps the static binary, and a step leaves the per-PR path only with a preflight mirror. The table key hash mechanism moves from a header comment into the simulate headers' architecture notes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…and a ci/REVIEW.das gate wires every ci/test_*.py

CodeQL took 20 minutes on the last PR - inside the budget - and no local gate mirrors it, so by the workflows checklist's own rule it keeps its pull_request trigger. The rule guarding ci/test_ci_matrix.py moves to ci/REVIEW.md, the narrowest folder holding its trigger; ci/REVIEW.das fails any ci/test_*.py no workflow step names, which is how a matrix pin would otherwise stop running. The utils checklist's CI-row rule now says on every pull request: with rows moving to the nightly, a row that runs after the merge is not the one the rule meant.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…form-bound nightly steps say so

The rules now bind a per-PR check - a step, a matrix cell, or a pull_request trigger - and admit a narrowing to a role that still runs it on every PR or to the nightly cron; a move to the nightly names its preflight mirror or the platform no per-PR cell has. The nano cross-compile and coverage carry that condition explicitly: nano's arm-none-eabi toolchain is an apt package no darwin cell or developer box has, and coverage is a report. pre_job's matrix step computes the JSON in its own statement so a script failure fails the job instead of feeding fromJSON an empty string.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…st pins the nightly-only steps, and the checklists say what weakening means

tier_runs, sweep_excluded with its table, is_sweep_root_text, is_heavy_sweep_root and failed_files_block move from main.das into config.das, where test_changed_set.das reaches them; the parser test gains the edges the audit named. make-pr's chain policy is in_default_chain in gates.das, with its test. ci/test_ci_matrix.py pins the set of nightly-only steps by name, so moving another step off the per-PR path is a deliberate edit, and covers the tool's dispatch and argument arms. The ci and utils checklists spell out what weakening REVIEW.das means, the ci checklist admits any loosened assertion of the matrix test, the workflows checklist defines a preflight gate in place and shrinks the pages.yml rule to what its gate cannot see. The README describes the tiers, lanes and reach; the plan carries the ledger of what stays untested and the gate candidates the audits named.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…paring byte widths

The interpreter hashes a builtin key as itself and a handled key as its annotation's workhorse, so a byte-width compare was the wrong criterion: a handled type narrower than its workhorse (ImVec2 or Point3 wrapped to vec4f) would have hashed its raw bytes under AOT against the workhorse the interpreter hashes. WrapsBuiltinValue (cast.h, marked for the vectors and ranges in jit_abi.h) is the distinction; a handled type an external module wraps defaults to the detour the interpreter takes. The cross-tier test writes int2 and Point3 keys interpreted past several grows and reads them from AOT, the one shape that sees a rail disagreement; the vector-key test gains EntityId and Point3 cells. The architecture notes and the checklist rule carry the corrected model.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… - one concept per rule, no positional cites

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pe and takes the workhorse detour for a handled one

One cell per key type of heap.h's makeTableKeyValueNode list pins KeyHash to the hash the interpreter's node computes; two handled stand-ins declared the way a module declares them - a 12-byte vector wrapped to vec4f and a 4-byte id wrapped to int32 - pin the detour; a third case pins that a builtin vector or range does not detour. Against master's KeyHash the vector and range rows and the detour case go red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 5, 2026 05:17

Copilot AI 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.

🟡 Changes recommended

KeyHash relies on WrapsBuiltinValue specializations that are not visible from runtime_table.h unless jit_abi.h is included, which risks the vector/range hashing fix not taking effect in common translation units.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request fixes a cross-tier correctness bug where tables keyed by vector (2/3 lanes) and range types could lose keys after the first grow (and AOT could miss keys), and restructures preflight + CI/extended_checks to fit strict wall-time budgets by splitting work into tiers/lanes and data-driven matrices.

Changes:

  • Make KeyHash align with interpreter hashing for builtin vector/range keys, and add coverage across interpreter/JIT/AOT plus a C++ pin test.
  • Rework utils/internal/preflight into fast/lanes/module tiers with reach-based skipping and add a compile-sweep + utils-tests lane.
  • Move CI matrices to ci/ci_matrix.py, pin them with ci/test_ci_matrix.py, and refactor workflows to consume the matrices and enforce role conditions.
File summaries
File Description
utils/REVIEW.md Tightens checklist wording around weakening REVIEW.das and CI-executed assertions.
utils/internal/preflight/tests/test_changed_set.das Adds unit tests for reach logic, compile-sweep filtering, lane report parsing, and tier rules.
utils/internal/preflight/README.md Documents tiers/lanes/module gates, reach skipping, and compile-sweep behavior.
utils/internal/preflight/main.das Implements reach-aware tiers/lanes orchestration, compile-sweep gate, utils-tests gate, and improved AOT failure details.
utils/internal/preflight/config.das Adds pure helpers for reach, sweep root detection/exclusion, lane report parsing, and failure summarization.
utils/internal/make-pr/test_gates.das Adds tests for the new “named-only gate” behavior.
utils/internal/make-pr/main.das Removes review-md/ast-verify/dupes from the default chain unless explicitly requested.
utils/internal/make-pr/gates.das Introduces NAMED_ONLY_GATES and in_default_chain to manage default vs --only gates.
tests/language/test_cross_tier_table_hash.das Expands cross-tier hashing test to cover vector and handled keys across a grow boundary.
tests/language/table_vector_keys.das New regression suite covering vector/range/handled keys surviving grows and erases.
tests-cpp/small/test_table_key_hash.cpp New C++ test ensuring KeyHash matches interpreter/JIT/JSON/C API hashing semantics.
skills/internal/preflight.md Updates authoritative documentation for tiers/lanes, reach sets, and CI mapping.
skills/internal/make_pr.md Updates the make-pr checklist to reflect preflight ownership of review-md/ast-verify and advisory dupes.
plans/ci_preflight_budget.md Records the performance budget, acceptance criteria, and migration plan/ledger.
include/daScript/simulate/runtime_table.h Adjusts KeyHash logic to hash builtin wrapped types as themselves.
include/daScript/simulate/REVIEW.md Adds checklist rules pinning table-key hashing sites and requiring PR-description disclosure for hash changes.
include/daScript/simulate/jit_abi.h Marks vectors/ranges as WrapsBuiltinValue and defines their wrap types.
include/daScript/simulate/cast.h Introduces WrapsBuiltinValue trait (default false).
include/daScript/simulate/ARCHITECTURE.md Adds an architecture section describing table key hashing invariants and why mismatches break on grow.
ci/test_ci_matrix.py Adds pinned tests for CI matrices and workflow role-condition correctness, plus nightly-only step pinning.
ci/REVIEW.md Adds checklist guidance around not weakening the matrix-pin tests and REVIEW.das checks.
ci/REVIEW.das Adds a gate ensuring each ci/test_*.py is wired into workflows.
ci/ci_matrix.py Introduces data-driven build/extended job matrices by event type.
.github/workflows/REVIEW.md Strengthens workflow checklist rules around per-PR checks, nightly moves, and role-guard spelling.
.github/workflows/extended_checks.yml Refactors job matrix to come from ci/ci_matrix.py, splits by role, and moves slow steps to nightly/dispatch.
.github/workflows/build.yml Refactors job matrix to come from ci/ci_matrix.py and marks nightly-only cells to avoid wasting cache quota.
Review details
  • Files reviewed: 26/26 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread include/daScript/simulate/runtime_table.h
… past DAS_MODULE_CACHE_LIMIT megabytes

The default-on cache had no eviction: every DAS environment or option variant minted another record beside the last, and an engine root's record is 200 MB, so a working box held 1372 records and 28 GB. After a writeback the default directory's .dascache files are listed and the oldest by mtime removed until the directory fits the limit (4096 MB unless set; 0 disables), never the record just written; a record a run read is touched, so a record in use is the newest. Only .jitted_scripts/module_cache/ is pruned - an explicit -module-cache path is the user's - and the limit is the one DAS* variable the record key skips. The test runs its children inside a temp directory so the cache they fill and prune is their own.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 05:34
Under DAS_NO_FILEIO the cache is a stub and nothing calls them, and that build treats an unused function as an error.

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

Copilot AI 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.

🔵 Needs a closer look

It simultaneously changes core runtime hashing semantics and CI/preflight orchestration (workflows + new gates), so a final human review should validate system-level implications and rollout safety.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

utils/internal/preflight/config.das:93

  • is_sweep_root_text currently treats any file containing both "[export]" and "def main" anywhere as a program root, which can include files where main is not exported (e.g., [export] on a different symbol) or where "def main" appears in comments/other identifiers. That can make compile-sweep compile unintended files and weaken the gate’s signal.
    utils/internal/preflight/main.das:382
  • collect_changed_paths silently falls back to only diffing vs HEAD when the configured base ref can’t be resolved, unlike collect_changed_files which emits a warning. Because changed_all drives reach-based gate skipping, this can make reach-gated checks skip without any visible explanation that the base was missing.
    utils/internal/preflight/main.das:1166
  • The doc comment above gate_ci_matrix describes compile-sweep trees, but the function actually runs ci/test_ci_matrix.py. This is likely a copy/paste and makes --list-gates/docs harder to trust.
  • Files reviewed: 34/34 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 5, 2026 05:42

Copilot AI 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.

🔵 Needs a closer look

It changes core runtime hashing semantics and significantly restructures CI/preflight orchestration across multiple platforms, requiring final human review despite added test coverage.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

utils/internal/preflight/main.das:1166

  • The doc-comment above gate_ci_matrix is incorrect (it describes the compile-sweep trees, but this function runs ci/test_ci_matrix.py to pin CI matrices/role conditions). This is misleading when scanning gate definitions via --list-gates or reading the file top-down.
  • Files reviewed: 34/34 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

… cast.h

A translation unit that sees the vec4f detour from jit_abi.h now always sees the marks too, and one that includes neither hashes a vector key raw either way, since WrapType is then the primary. The review's concern that runtime_table.h alone would leave the marks default was not a hash difference - the two defaults moved together - but the pair is one header now, so no include order can split it. The ci-matrix gate's doc comment says what the gate runs; the trees comment is back on SWEEP_TREES.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 05:54

Copilot AI 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.

🔵 Needs a closer look

The review found a preflight reach-gating correctness edge case and a CI sccache save-order issue that can undermine the PR’s stated timing/budget guarantees.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

.github/workflows/extended_checks.yml:270

  • The sccache save step runs immediately after building only daslang/daslang_static, before later steps that compile additional targets (e.g., check_dasgen, run_utils_tests, all_utils_exe, and the sequence module targets). This means the saved cache slot can miss a large portion of the objects actually built during the job, reducing cache hit rates and risking the 35-minute budget.
    src/builtin/module_builtin_ast_serialize.cpp:3331
  • moduleCacheLimitBytes() multiplies the parsed DAS_MODULE_CACHE_LIMIT value by 1MB without overflow checking. Extremely large values (or strtoull overflow to ULLONG_MAX) can wrap the byte count, leading to an unexpectedly tiny limit and aggressive eviction. Clamping before multiplication would make the behavior robust.
    utils/internal/preflight/main.das:382
  • collect_changed_paths silently falls back to only git diff HEAD when base cannot be verified, which means a clean working tree on a branch with committed changes can yield an empty changed_all. That can incorrectly skip reach-gated checks (e.g., docs/ci-matrix) even though the branch diverged from base. Consider warning (like collect_changed_files does) and treating an unknown base as "core changed" so reach sets are never skipped due to missing refs.
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The ast-grep tools need the tree_sitter_daslang library and the sgconfig.yml its post-build step stamps; run_utils_tests pulled both in when every step shared one job, and now runs in the core role, so the modules job builds the target itself before the test. On darwin without it ast-grep knew no daslang language and every grep_usage and outline cell failed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 06:20

Copilot AI 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.

🔵 Needs a closer look

There are verified issues in the new preflight sweep scanning and module-cache mtime touch semantics, plus a newly added brittle module-cache eviction test that risks CI instability.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/builtin/module_builtin_ast_serialize.cpp:3457

  • touchFile(readFrom) updates mtime for every cache read, including explicit -module-cache paths. That changes the meaning of an explicit cache file’s timestamp (it becomes last-read, not last-written) even though explicit caches are never pruned. If callers treat the explicit cache as a build artifact, this can create confusing rebuild triggers.
    utils/internal/preflight/main.das:1192
  • collect_das_files recursively descends into every directory under utils/examples/tutorials, even for paths that will later be excluded (SWEEP_EXCLUDED) or rejected by is_sweep_root_path (tests/_aot_generated and any nested "/modules/"). On trees with large excluded subtrees or a junctioned modules/ directory inside examples, this can add significant scan time (or worst-case recurse into the entire repo twice) before any compile work starts.
    tests/module_cache/test_default_cache_path.das:440
  • test_default_cache_evicts_to_the_limit hard-codes DAS_MODULE_CACHE_LIMIT=1MB and then asserts the cache record size falls into a narrow range (2 fit, 3 don’t). That makes the test fragile: harmless changes to the serialized module graph size (or platform-dependent differences) will fail the suite even though eviction behavior is still correct.
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

…runs it

On the darwin runner pip and python3 are different interpreters, so `pip install markdown` left build_news.py without the module; python3 -m pip installs where the script imports, with the user-site and externally-managed fallbacks a Homebrew python needs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 06:42

Copilot AI 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.

🔵 Needs a closer look

There are a couple of concrete, fixable issues in the updated code/tests (stored as review comments) that should be addressed before approval.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

ci/test_ci_matrix.py:138

  • step_conditions() only recognizes step names written as - name: "...". If a future edit adds a nightly-only step with an unquoted - name: ... (a pattern used in other workflows, and already present in build.yml), this test would silently ignore it and fail to pin the per-PR vs nightly split.
    src/builtin/module_builtin_ast_serialize.cpp:16
  • moduleCacheLimitBytes() uses strtoull, but this TU doesn’t include (and relies on transitive includes for both strtoull/strlen). Adding the standard headers explicitly makes the file self-contained and avoids accidental build breaks when upstream headers change.
  • Files reviewed: 33/33 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@borisbat
borisbat merged commit 6d7111c into master Sep 5, 2026
33 checks passed
@borisbat
borisbat deleted the bbatkin/preflight-20min branch September 5, 2026 07:45
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