Repository navigation
utils, dastest/tests: suites merge by topic; lint exe; shape gate per root - #4495
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change spans multiple intertwined areas (test-shape gates, CI suite composition, lint caching/exe behavior, and CMake install wiring) where correctness hinges on cross-platform CI execution to surface path, line-ending, and cache/mtime edge cases and avoid coverage or caching regressions.
No open issues 🎉
What changed in this PR
This pull request extends the “merge suites by topic + described tests + enforced shape” model from tests/ to the other in-repo test roots (utils/ suites and dastest/tests). It centralizes shape rules, updates gates and per-root indexes, and includes supporting tooling changes (notably for lint and MCP C++ tooling) to improve performance and maintainability.
Changes:
- Centralize test-tree shape enforcement in
dastest/test_shape.dasand invoke it from each test root’sREVIEW.das(including utils “tests-only” mode). - Extend
test-auditwith--tests-onlyand--index <path>and verify per-root indexes (e.g.,tests/README.md,utils/TESTS.md,dastest/tests/README.md). - Reshape/merge suites in
utils/anddastest/testsby topic and ensure tests/fixtures carry descriptions/claims. - Improve tooling behavior/performance: standalone lint exe cache behavior and faster MCP C++
compile_commands.jsondiscovery.
Open review comments to address:
- Moderate:
utils/daspkg/uncategorized.das— assert cleanup succeeded so regressions don’t pass silently. - Moderate:
utils/lint/tests/_lint_cli.das— freshness detection forbin/das-lint.exemay miss dependencies outside the current probe set, causing stale exe usage. - Nit:
utils/lint/tests/_lint_cli.das— typehostasarray<string>for clarity and stability. - Moderate:
utils/mcp/tools/cpp_compile_check.das— make compile DB selection deterministic (directory iteration order is filesystem-dependent).
| File | Review |
|---|---|
| utils/watchdog/tests/test_tool_front.das | Reviewed |
| utils/REVIEW.md | Reviewed |
| utils/REVIEW.das | Reviewed |
| utils/mcp/uncategorized.das | Reviewed |
| utils/mcp/uncategorized_no_aot.das | Reviewed |
| utils/mcp/tools/cpp_compile_check.das | Reviewed |
| utils/mcp/ROADMAP.md | Reviewed |
| utils/mcp/REVIEW.md | Reviewed |
| utils/lint/tests/style048_if_chain_to_match.das | Reviewed |
| utils/lint/tests/style047_hand_written_ordering.das | Reviewed |
| utils/lint/tests/style046_match_arm_expression.das | Reviewed |
| utils/lint/tests/style045_condition_interval_merge.das | Reviewed |
| utils/lint/tests/style044_legacy_match_arms.das | Reviewed |
| utils/lint/tests/style044_call_named_match.das | Reviewed |
| utils/lint/tests/style043_merge_declarations.das | Reviewed |
| utils/lint/tests/style042_expression_body.das | Reviewed |
| utils/lint/tests/style041_flag_return.das | Reviewed |
| utils/lint/tests/style040_duplicate_region.das | Reviewed |
| utils/lint/tests/style039_ascii_strings.das | Reviewed |
| utils/lint/tests/style038_macro_body.das | Reviewed |
| utils/lint/tests/style038_function_length.das | Reviewed |
| utils/lint/tests/style037_cyclomatic_complexity.das | Reviewed |
| utils/lint/tests/style035_uint_char_compare.das | Reviewed |
| utils/lint/tests/style034_addr_cast_sugar.das | Reviewed |
| utils/lint/tests/style033_concat_run.das | Reviewed |
| utils/lint/tests/style032_clone_init_push_from.das | Reviewed |
| utils/lint/tests/style031_table_insert_run.das | Reviewed |
| utils/lint/tests/style030_unused_require.das | Reviewed |
| utils/lint/tests/style029_transitive_require.das | Reviewed |
| utils/lint/tests/style028_self_method_arrow.das | Reviewed |
| utils/lint/tests/style027_for_comprehension.das | Reviewed |
| utils/lint/tests/style026_nested_unsafe.das | Reviewed |
| utils/lint/tests/style025_unsafe_block_narrow.das | Reviewed |
| utils/lint/tests/style024_redundant_unsafe.das | Reviewed |
| utils/lint/tests/style023_bitfield_int_cast_test.das | Reviewed |
| utils/lint/tests/style022_bitfield_or_assign.das | Reviewed |
| utils/lint/tests/style021_jv_table_insert.das | Reviewed |
| utils/lint/tests/style020_from_jv_to_coalesce.das | Reviewed |
| utils/lint/tests/style019_clamp.das | Reviewed |
| utils/lint/tests/style018_bool_compare.das | Reviewed |
| utils/lint/tests/style017_bool_return.das | Reviewed |
| utils/lint/tests/style016_adjacent_guards.das | Reviewed |
| utils/lint/tests/style015_long_comment_private.das | Reviewed |
| utils/lint/tests/style014_long_comment.das | Reviewed |
| utils/lint/tests/style013_struct_field_init.das | Reviewed |
| utils/lint/tests/style012_array_push_instead_of_literal.das | Reviewed |
| utils/lint/tests/style005_braced_terminator.das | Reviewed |
| utils/lint/tests/style002_body_from_macro_file.das | Reviewed |
| utils/lint/tests/perf033_loop_container_moved.das | Reviewed |
| utils/lint/tests/perf032_exact_size.das | Reviewed |
| utils/lint/tests/perf031_slice_in_loop.das | Reviewed |
| utils/lint/tests/perf030_move_assign_leak.das | Reviewed |
| utils/lint/tests/perf029_parallel_array_zip.das | Reviewed |
| utils/lint/tests/perf028_hot_path_io.das | Reviewed |
| utils/lint/tests/perf027_hot_path_env.das | Reviewed |
| utils/lint/tests/perf026_hot_path_alloc.das | Reviewed |
| utils/lint/tests/perf025_redundant_string_in_interp.das | Reviewed |
| utils/lint/tests/perf024_redundant_clone_to_annotated.das | Reviewed |
| utils/lint/tests/perf023_clone_into_qmacro.das | Reviewed |
| utils/lint/tests/perf022_for_push_bulk.das | Reviewed |
| utils/lint/tests/perf021_ternary_cast_hoist.das | Reviewed |
| utils/lint/tests/perf020_redundant_cast.das | Reviewed |
| utils/lint/tests/perf019_static_if_no_fold_leak.das | Reviewed |
| utils/lint/tests/perf019_int_cast_collapse.das | Reviewed |
| utils/lint/tests/perf018_index_only_for_loop.das | Reviewed |
| utils/lint/tests/perf017_length_zero.das | Reviewed |
| utils/lint/tests/perf016_ternary_abs.das | Reviewed |
| utils/lint/tests/perf015_ternary_minmax.das | Reviewed |
| utils/lint/tests/perf014_char_class_range.das | Reviewed |
| utils/lint/tests/perf013_inc_dec.das | Reviewed |
| utils/lint/tests/perf011_get_ptr_field.das | Reviewed |
| utils/lint/tests/perf010_get_ptr_null.das | Reviewed |
| utils/lint/tests/perf009_redundant_move_return.das | Reviewed |
| utils/lint/tests/perf009_pending_move_scoping.das | Reviewed |
| utils/lint/tests/perf007_string_das_string.das | Reviewed |
| utils/lint/tests/perf006_push_without_reserve.das | Reviewed |
| utils/lint/tests/perf006_nested_unknown_length.das | Reviewed |
| utils/lint/tests/module_cache/project_resolution.das | Reviewed |
| utils/lint/tests/module_cache/project_options.das | Reviewed |
| utils/lint/tests/module_cache/project_errors.das | Reviewed |
| utils/lint/tests/module_cache/_project_common.das | Reviewed |
| utils/lint/tests/lint031_const_strip_write.das | Reviewed |
| utils/lint/tests/lint030_argument_order.das | Reviewed |
| utils/lint/tests/lint029_by_ref_arg_mutation.das | Reviewed |
| utils/lint/tests/lint024_widened_product.das | Reviewed |
| utils/lint/tests/lint023_unread_var_argument.das | Reviewed |
| utils/lint/tests/lint022_unused_private_symbol.das | Reviewed |
| utils/lint/tests/lint021_64bit_never_consumed.das | Reviewed |
| utils/lint/tests/lint020_range_narrowing.das | Reviewed |
| utils/lint/tests/lint018_mem_size_narrowing.das | Reviewed |
| utils/lint/tests/lint017_long_counterpart_cast.das | Reviewed |
| utils/lint/tests/lint016_redundant_string_clone.das | Reviewed |
| utils/lint/tests/lint015_free_floating_unary.das | Reviewed |
| utils/lint/tests/lint014_unwritten_var_argument.das | Reviewed |
| utils/lint/tests/lint012_static_if_arm.das | Reviewed |
| utils/lint/tests/lint010_dead_store.das | Reviewed |
| utils/lint/tests/lint009_if_same_branches.das | Reviewed |
| utils/lint/tests/lint008_ternary_same_branches.das | Reviewed |
| utils/lint/tests/lint007_same_operands.das | Reviewed |
| utils/lint/tests/lint006_division_by_zero.das | Reviewed |
| utils/lint/tests/lint004_underscore_used.das | Reviewed |
| utils/lint/tests/lint003_pass_mutable.das | Reviewed |
| utils/lint/tests/lint002_unused_variable.das | Reviewed |
| utils/lint/tests/lint001_unreachable_code.das | Reviewed |
| utils/lint/tests/lint_module_rule_overrides.das | Reviewed |
| utils/lint/tests/_lint_cli.das | Reviewed |
| utils/lint/tests/_lint_child.das | Reviewed |
| utils/lint/REVIEW.das | Reviewed |
| utils/lint/README.md | Reviewed |
| utils/lint/main.das | Reviewed |
| utils/jobque-timeline/test_tl_launch.das | Reviewed |
| utils/jobque-timeline/README.md | Reviewed |
| utils/internal/test-release/test_utils_phase.das | Reviewed |
| utils/internal/test-release/test_classify.das | Reviewed |
| utils/internal/test-audit/main.das | Reviewed |
| utils/internal/review-md/uncategorized.das | Reviewed |
| utils/internal/review-md/test_scan.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_used_var_1.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_used_struct_2.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_used_struct_1.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_used_func_1.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_used_enum_1.das | Reviewed |
| utils/internal/requirefix/testdata/_fixture_public_1.das | Reviewed |
| utils/internal/preflight/tests/test_verify_pool.das | Reviewed |
| utils/internal/preflight/tests/test_lane_groups.das | Reviewed |
| utils/internal/preflight/tests/test_compile_db.das | Reviewed |
| utils/internal/preflight/main.das | Reviewed |
| utils/internal/pr-babysit/uncategorized.das | Reviewed |
| utils/internal/dasweb-verify/test_verify_core.das | Reviewed |
| utils/internal/dasweb-playground/test_playground_config.das | Reviewed |
| utils/internal/dasweb-buildd/uncategorized.das | Reviewed |
| utils/internal/dasweb-buildd/uncategorized_persistent_heap.das | Reviewed |
| utils/internal/dasweb-buildd/test_buildd_config.das | Reviewed |
| utils/internal/dasweb-buildd/REVIEW.md | Reviewed |
| utils/internal/dasweb-buildd/README.md | Reviewed |
| utils/internal/dasllama-ladder/test_ladder_config.das | Reviewed |
| utils/internal/dasllama-ladder/test_deploy_caddy.das | Reviewed |
| utils/internal/das-herd/watcher/tests/uncategorized.das | Reviewed |
| utils/internal/das-herd/watcher/tests/uncategorized_gc_persistent_heap.das | Reviewed |
| utils/internal/das-herd/watcher/tests/test_file_inspection.das | Reviewed |
| utils/internal/das-herd/watcher/tests/test_destructive_confirm.das | Reviewed |
| utils/internal/das-herd/PTY_HOST_DESIGN.md | Reviewed |
| utils/internal/das-herd/HERDER_FIXROUND.md | Reviewed |
| utils/internal/das-herd/EDITOR_ARC_PLAN.md | Reviewed |
| utils/internal/das-herd/chunk1/tests/uncategorized.das | Reviewed |
| utils/internal/das-herd/CHANGELIST_ACTIONS_PLAN.md | Reviewed |
| utils/internal/arch-extract/tests/test_arch_extract.das | Reviewed |
| utils/gen1-to-gen2/tests/uncategorized_rtti_strict_smart_pointers_false.das | Reviewed |
| utils/gen1-to-gen2/tests/test_stmt.das | Reviewed |
| utils/gen1-to-gen2/tests/test_make.das | Reviewed |
| utils/gen1-to-gen2/tests/test_engine.das | Reviewed |
| utils/gen1-to-gen2/README.md | Reviewed |
| utils/daspkg/uncategorized.das | Reviewed |
| utils/daspkg/test_release_workspace.das | Reviewed |
| utils/daspkg/test_package_runner_errors.das | Reviewed |
| utils/daspkg/REVIEW.md | Reviewed |
| utils/daspkg/README.md | Reviewed |
| utils/daspkg/_daspkg_test_common.das | Reviewed |
| utils/dasllama-server/uncategorized.das | Reviewed |
| utils/dasllama-server/uncategorized_persistent_heap_stack_524288.das | Reviewed |
| utils/dasllama-server/uncategorized__dasllama_internal_stack_524288.das | Reviewed |
| utils/dasllama-server/test_user_state.das | Reviewed |
| utils/dasllama-server/test_slot_served.das | Reviewed |
| utils/dasllama-server/test_openai_server_translate.das | Reviewed |
| utils/dasllama-server/test_openai_server_speech_multi.das | Reviewed |
| utils/dasllama-server/test_openai_server_mtp.das | Reviewed |
| utils/dasllama-server/test_openai_server_audio.das | Reviewed |
| utils/dasllama-server/test_exchange_client.das | Reviewed |
| utils/dasllama-server/test_constraints.das | Reviewed |
| utils/dasllama-server/test_catalog_download.das | Reviewed |
| utils/dasllama-server/test_cache_probe.das | Reviewed |
| utils/dasllama-server/REVIEW.md | Reviewed |
| utils/dasllama-server/README.md | Reviewed |
| utils/dasllama-convert/test_convert.das | Reviewed |
| utils/daslang/tests/test_tool_dispatch.das | Reviewed |
| utils/daslang/tests/test_relocated_host.das | Reviewed |
| utils/dascov/tests/_simple_linear.das | Reviewed |
| utils/dascov/tests/_simple_branch.das | Reviewed |
| utils/dascov/tests/_nested_branch.das | Reviewed |
| utils/dascov/tests/_multi_func.das | Reviewed |
| utils/dap/test_dap_bridge.das | Reviewed |
| utils/common/tests/uncategorized.das | Reviewed |
| utils/common/tests/test_git_signature.das | Reviewed |
| utils/benchctl/REVIEW.md | Reviewed |
| tests/README.md | Reviewed |
| tests/dastest/uncategorized.das | Reviewed |
| tests/ARCHITECTURE.md | Reviewed |
| skills/tdd_audit.md | Reviewed |
| skills/mcp_tools.md | Reviewed |
| skills/internal/tests_in_repo.md | Reviewed |
| modules/dasLLVM/fat_mode_plan.md | Reviewed |
| modules/dasLLAMA/tests/CLAUDE.md | Reviewed |
| modules/dasLLAMA/followup_general.md | Reviewed |
| modules/dasLLAMA/CMakeLists.txt | Reviewed |
| doc/reflections/das2rst.das | Reviewed |
| dastest/tests/test_together.das | Reviewed |
| dastest/tests/test_timeout.das | Reviewed |
| dastest/tests/test_temp_dir.das | Reviewed |
| dastest/tests/test_log_color_framing.das | Reviewed |
| dastest/tests/test_failure_ownership.das | Reviewed |
| dastest/tests/test_benchmark_budget.das | Reviewed |
| dastest/tests/REVIEW.md | Reviewed |
| dastest/tests/REVIEW.das | Reviewed |
| dastest/tests/_timeout_fixture/test_sleep.das | Reviewed |
| dastest/tests/_isolated_fixture/test_worker_index.das | Reviewed |
| dastest/tests/_isolated_fixture_pre_dashdash/test_preserved_flag.das | Reviewed |
| dastest/tests/_isolated_fixture_panic/test_panic.das | Reviewed |
| dastest/tests/_isolated_fixture_crash/test_crash.das | Reviewed |
| dastest/tests/_fixture_unknown_annotation_arg.das | Reviewed |
| dastest/tests/_fixture_raised_timeout.das | Reviewed |
| dastest/tests/_fixture_bad_annotation_kind.das | Reviewed |
| dastest/tests/_fixture_bad_annotation_arg.das | Reviewed |
| dastest/tests/_fixture_annotation_args.das | Reviewed |
| dastest/file_census.das | Reviewed |
| daslib/lint_config.das | Reviewed |
| CMakeLists.txt | Reviewed |
| .claude/agents/tdd-auditor.md | Reviewed |
Review effort: Balanced
0752be6 to
9fb275a
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change spans broad, cross-cutting test and tooling infrastructure (test-suite layout, lint standalone-exe selection/caching/versioning, and install/path filtering) whose correctness depends on runtime CI behavior across platforms, so it needs maintainer verification and multi-platform CI validation before approval.
No open issues 🎉
Review effort: Balanced
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A critical active finding remains because the lint CLI “--help” probe is configured with a zero-second timeout, meaning a hung executable could stall the entire test run indefinitely and needs a bounded timeout handling.
1 open issue
Review effort: Balanced
9fb275a to
f368cad
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
A critical test-safety issue remains because the lint CLI probe uses run_and_capture with no timeout, which could hang the entire lint test suite if das-lint.exe --help deadlocks or waits on missing runtime components.
1 open issue
Review effort: Balanced
f368cad to
e0145dd
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A documentation-style comment marker (//!) is used for an internal implementation note inside cpp_locate_compile_db, which conflicts with the project’s comment-style guidance and needs to be corrected to avoid unintended generated docs.
2 open issues
Review effort: Balanced
e0145dd to
ce331fa
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
The change still has active review findings noting that several mid-function //! comments will be emitted as documentation rather than implementation notes, requiring replacement with // per the project's comment-style guidance.
2 open issues
2 resolved since last review
Review effort: Balanced
ce331fa to
324e472
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A critical active finding remains: reset_lint_cache_in does not guard against an empty cache directory, which can lead to operations on "/lint_hash.txt" and unsafe rmdir_rec/mkdir_rec calls on an empty path with potentially destructive effects across multiple call sites.
1 open issue
2 resolved since last review
Review effort: Balanced
324e472 to
50c3dda
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
The lint CLI test’s freshness check can select and run a stale das-lint.exe because it ignores changes in non-lint daslib dependencies required by the lint runner, leaving an active reliability issue in the test suite.
1 open issue
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change is large and cross-cutting across test-suite structure, review gates, lint-cache/versioning, and installation/packaging behavior, so maintainers need to validate it against the full multi-platform CI/runtime matrix and shipped-SDK expectations beyond what a static diff review can confirm.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
5e4ea3b to
8299b1c
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change spans multiple interconnected areas (test-shape gating, lint cache/versioning, and CMake install rules), making its correctness dependent on CI/platform behavior and difficult to validate confidently from the diff alone without a targeted human verification pass.
No open issues 🎉
Review effort: Balanced
8299b1c to
0eac42f
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
This broad rework of test-suite layout/gating and lint execution (including standalone executable, caching, MCP tooling, and CMake install rules) is cross-cutting enough that it warrants human review and multi-platform CI validation to avoid subtle suite-discovery and workflow regressions.
No open issues 🎉
Review effort: Balanced
0eac42f to
9af4f10
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change spans multiple subsystems (test-shape enforcement, lint cache semantics, and CMake/install wiring), so its intended behavior and platform-specific outcomes need human validation via multi-platform CI and runtime checks before approval.
No open issues 🎉
Review effort: Balanced
check_test_shape moves out of tests/REVIEW.das into dastest/test_shape.das so every test root applies the same rules: tests/, utils/ in tests-only mode (a tool's sources and the fixtures its suite feeds it are not tests, and a compile-fail fixture keeps the tool's naming), and a new dastest/tests gate. test-audit gains --tests-only and --index; check_tests_index verifies the three indexes. The census reads a // inside a description as text, skips a daspkg-installed modules/ tree, and --verify ignores line endings. The rule documents and the tdd-auditor say "any test root". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every suite under utils/ and dastest/tests takes the tests/ shape: a category file holds three or more tests, a directory's loose tests sit in its uncategorized.das, a file fenced by its options is named by them, and each [test] carries a description; a test that builds, spawns or waits carries the suite default budget. mcp's 190-test file splits by tool family, daspkg's 138-test file by topic, both over a shared helper module; the 92 lint expect fixtures state their claim in a //! line. Every CI row that named a merged file - utils/CMakeLists.txt, the dasLLAMA lane, the SDK install list - names its successor, and utils/TESTS.md and dastest/tests/README.md are the generated indexes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A lint spawn through the host binary compiles the lint tool before it lints anything, about 2.5 s of a 3 s run, and the suites spawn it several times per test. The standalone exe ran without the result cache because the lint_tool_hash macro, which versions the cache while the runner compiles, never runs in an exe; the exe now versions the cache by its own file (size and modification time) at startup instead, so a rebuilt exe starts a fresh cache. `lint/tests/_lint_cli.das` spawns bin/das-lint.exe when the build left one newer than every lint source and it starts, else the script form with its host flags; a call whose host flags change what the lint sees (a disabled module, the host's native libraries) keeps the script. utils/lint: 177 s -> 53 s. run_utils_tests depends on check_standalone_exes so a lane never meets a half-written exe. A test-only exe never ships: utils/REVIEW.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cpp_compile_check searched `build*/compile_commands.json` through the glob expander, whose `/`-bearing pattern walks the whole tree, build directory included, on every call - 20 to 45 s a call on a tree with an MSVC build and no compile database. The lookup now lists the root's `build*` and `cmake-build-*` children and tests each one's compile_commands.json directly. utils/mcp/test_tools_cpp.das: 160 s -> 83 s. The extended job ran the mcp suite twice: once through the `mcp` row of DAS_UTILS_TO_TEST (run_utils_tests, the whole directory) and once through run_tests_mcp_tools, which names the same files. The row goes; the lane target, which preflight runs by name, stays. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9af4f10 to
47ee85c
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A documentation comment in utils/common/verify_pool.das is misleading because it implies --file verifies everything, but the current logic still skips compile-fail fixtures regardless of changed-mode, so the wording should be clarified to match actual behavior.
1 open issue
Review effort: Balanced
The changed-set verifiers - utils/ast-verify --changed (the CI target), preflight's ast-verify gate and make-pr's - each carried a copy of the file filter. The copy is now `ast_verify_skips` in utils/common/verify_pool.das, and the whole-stack compiles join the compile-fail fixtures and the ast-fuzz breakers in it: utils/dasllama-server/ and utils/dasllama-convert/ pull every llama module into one compile, doc/reflections/das2rst.das requires every module in the tree. Each ran past the 1200 s cap on the macOS extended runner with two siblings in flight (the next slowest file took 46 s), and their own lanes compile them anyway. The run total and the heartbeat ages read get_time_nsec: get_time_usec returns a 32-bit int, which wrapped to -2012 s on a 36-minute run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
47ee85c to
f376613
Compare
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🔵 Needs a closer look
The change spans multiple interconnected areas of the test/CI harness, lint caching/versioning, and CMake install/packaging behavior, so it needs hands-on cross-platform CI and suite runs to rule out regressions in test selection, cache invalidation, timeouts, and installed-SDK contents.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
Behavior change: the suites under
utils/anddastest/teststake the topic-file shape oftests/- a new[test]goes into the tool's category file or that directory'suncategorized.das, never a new file.Why. The
tests/merge (#4489) left 1003 utils tests and 64 dastest tests outside the shape: 1001 undescribed, 24 one-test files, three files past the cap, no gate or index.What changes.
dastest/test_shape.das; every test root'sREVIEW.dascalls them, andcheck_tests_indexverifies three indexes.utils/anddastest/testsmerges by topic and every test states its claim.utils/lint177 s -> 53 s. Test-only; it never ships.compile_commands.jsonwithout walking the tree, and the extended job runs the mcp suite once instead of twice.utils/dasllama-server/is in it: its files compile the whole llama stack, minutes each on the macOS runner.Observable behavior.
run_utils_testshere 698 s -> about 440 s.Where to look.
dastest/test_shape.das,utils/lint/tests/_lint_cli.das,configure_lintinutils/lint/main.das, and the CMake rows that named merged files; the risky spots are the mcp and daspkg splits.Validation, claims, ledger
Validation
#nightly- the utils suites run per PR only throughrun_utils_testsand the lane targets; the nightly's extended checks and the dasLLAMA lane run the rest.dastest/testsandtests/dastest; all green but the known Windows dap red below.utils/lintran green both with the exe (53 s) and without it (script fallback, 177 s)._lint_cli.daspicksbin/das-lint.exeonly when it is newer than every lint source and starts (--help); a call whose host flags change what the lint sees - a disabled module, the host's native libraries - keeps the script. The exe versions its cache by its own size and modification time (standalone_lint_version), since thelint_tool_hashmacro never runs in an exe.run_utils_testsdepends oncheck_standalone_exes. The test-only rule is inutils/REVIEW.md._daspkg_test_common.dasand_tools_common.das; the two files over the 100-test cap (mcp 190, daspkg 138) split by topic into 12 and 9.utils/dap/test_dap_bridge.dason Windows (dap bridge: debug_terminate fails on Windows - DAP connection closed before replying; suite runs on Linux CI only #4492; CI runs it on Linux only, 16 of 16 there); the das-herd watcher protocol test (das-herd watcher: test_watcher_protocol_integration red on Windows at stage=wait-health; the folder runs in no CI lane #4493; no CI lane runs the folder); dasweb-buildd here because the box lacksjq(CI: 13 of 13 on Linux and Windows).tests/REVIEW.dasrequiresdastest/test_shape, which joined the AOT corpus; and the macOS extended checks sawutils/TESTS.mddiffer becauserun_utils_testshad installed find-dupe's daspkg dependency underutils/find-dupe/modules/first - the census now skips themodules/child of any directory holding.das_packageand everything git ignores. The real cause of the macOS mismatch was an unstable sort: the index ordered files by stem, andutils/internalholds oneuncategorized.dasper tool, so equal keys came out in platform order; files sort by path now, headings carry the sub-path, and--verifyprints the first differing line.utils/lint/tests/module_cache/project_cli.dasis one ctest where master had four; on the Windows Debug nightly cell its ten tests spawn Debug lint children for 1775 s, past dastest's 1200 s suite timeout, and the failed ctest sent the lane into its isolated retry (the three-hour Test step on the earlier runs). That cell leaves the ctest out;run_utils_testsstill runs the file. Found by running the sweep in Debug here (52 min, no stall) and reading the ctest line in the live log.--timeoutwatchdog now names the file the sequential sweep was on when it fires: under--failures-onlynothing else in a CI log says where a suite died.utils-testsred while every suite prints green: MSBuild's custom build fails on theerror :lines the refusal tests print by design, as on master.tests-aotis skipped by preflight on this box (its own host pins the runtime DLL);test_aot_subsetand the nightly cover it. The docs gates ran green after enabling dasCrypto in this build (the doc generator loads every module).dastest/testsran on Windows only; Linux and macOS see it in CI.git stashof mine during the pass reverted the tree for two minutes and was restored file by file; every shard re-verified its files afterwards, and the suites ran on the restored tree.Claims - stated, not tested
timeout = 1200was judged from the body (a spawn, a server, a wait), not timed.utils/lintunder the exe passed 188 of 188 here; CI is the first run of that path on Linux and macOS.Not done
modules/dasLLVM/tests,modules/dasVulkan/tests,modules/dasImgui/tests, the small module test folders andmodules/dasLLAMA/testskeep their shape; each is its own PR, dasLLAMA last._dasllama_internaloption isuncategorized__dasllama_internal_stack_524288.das, double underscore included.