fix(pipeline): preserve dotted TypeScript import basenames - #1811
Conversation
Signed-off-by: Pcristin <xxxokzxxx@protonmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
|
CI triage: the only substantive red job is This PR changes only FQN/import resolution and its regression tests; it does not touch daemon or Windows-guard code. The same cold-storm timeout occurred on the unrelated PR #1245 workflow run, while surrounding runs of this guard passed, so this matches an I tried to rerun only the failed jobs, but GitHub rejected it because contributors do not have repository Actions rerun permission. Could you @DeusData rerun the failed jobs? I have deliberately not added an unrelated code change or empty commit just to retrigger CI. |
DeusData
added
bug
|
Thank you for the careful CI attribution. I checked the diff and current main: this PR is limited to FQN and import resolution plus its regression tests, while the red job is the Windows daemon guard. That failure is outside this change path and matches the existing startup-flake family you linked. You are also correct that contributors cannot rerun repository jobs. This should be rerun from the maintainer side, and there is no need to create an unrelated commit. Thank you for documenting the evidence instead of treating the red aggregate as proof against the patch. |
|
Approved on merit. The one red is not yours, and I can be specific about that. The failure
with run totals This is a known flake we have already diagnosed: the assertion's verdict is fed by a 100 ms wall-clock window, so a loaded runner decides the result rather than the code. We saw the identical signature — same test, same line, same platform — on 28 August on an unrelated NetBSD-only On the changeThe bug is a good one to have found, because the old code encoded a false rule: the text after the final dot is an extension. For Two things I want to credit: Removing the second, generic strip after the relative resolver has normalised the path. Double-stripping is the kind of bug that hides behind a passing test suite for years, because it only bites inputs that survive the first pass with a dot still in them. Noticing that the two stages overlapped is more valuable than the extension list itself. Asserting both edges in the regression — On your lint note
That is a false positive on your side, not drift on ours, and you are the third contributor to hit it this week. Our gate requires the Homebrew LLVM I will raise making that explicit in CONTRIBUTING, since three people have now spent time on it. Before mergeFour checks are still queued. Our Actions pool is servicing roughly one job at a time against 21 queued PR runs, so this is slow for reasons unrelated to your PR. I will merge once it settles, and the |
Maintainer notice: please disregard comments from @adfjadfj16-a11y on this thread@adfjadfj16-a11y is not a maintainer of this project and does not speak for it. That account has posted replies on 17 threads here written in the project's voice — promising merges, announcing that a case has been "escalated to the development team", asking to close issues, and in some threads replying as though it were the author of someone else's pull request. None of those were maintainer decisions, and none of them carried any weight. @DeusData is the only account that gives a maintainer response on this repository. If a comment about the fate of your issue or pull request did not come from @DeusData, it is not a decision, however official it reads. If you were waiting on something because of one of those comments — a promised merge, a review "immediately", a request to close your ticket — I am sorry. That was noise you had no way to identify as noise, and it should not have been on your thread. Your issue or PR is judged on its own merits, and I will answer it here myself. Nothing in this notice reflects on your contribution. Thank you for your patience, and thank you for the work. |
|
The latest red check appears unrelated to this PR's #1682 changes and is caused by an upstream toolchain change. The failing job is It fails during compilation with: The previous LSan run on this PR installed Homebrew LLVM 22.1.8 and passed. This run installed the newly available LLVM 23.1.0 through the workflow's unpinned The PR branch's This likely needs an upstream fix to make the test-only probe counter platform-correct, or a temporary LLVM pin. Another branch update or unrelated change in this PR should not be necessary. |
Dismissed by the maintainer: this account is not a member of the maintainer team and its reviews carry no weight here. Only reviews from DeusData are maintainer decisions.
|
Thank you — that diagnosis is exactly right, and it was the first sighting of a real problem. Run 33745916176 poured Homebrew LLVM 23.1.0 and died at The remaining red on your current head ( |
|
Merged as 3bbabf1 — thank you, and thanks for the patience across the reruns and the LLVM 23 compile detour (that one was ours, fixed in #2027). Dotted TypeScript import basenames now keep their full name through |
The test-lsan-macos leg installs the unversioned `llvm` formula, which tracks Homebrew's current stable. That moved from 22.1.8 to 23.1.0 today, so the leg's compiler now depends on which bottle the runner image draws: run 33745916176 (PR DeusData#1811) poured 23.1.0 and died at compile on a new Clang 23 diagnostic (-Wunused-but-set-global under -Werror); the run an hour earlier poured 22.1.8 and built fine. Same code, two verdicts. Pin the formula (and the `brew --prefix` lookup) to llvm@22 — the version every green run of this leg has used so far. Moving to a newer major becomes a deliberate edit of these two lines instead of an accident of the runner image. The diagnostic itself is fixed independently in DeusData#2027 so main compiles under both majors; this change is about determinism of the leg, not the warning. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…istic lock_registry_absolute_deadline_survives_repeated_wakes has failed on the test-unix (macos-15-intel) leg of three unrelated pull requests in one week -- #1342 (08-28), #1811 (09-02) and #1819 (09-03, run 33799475629, job 100823626965) -- always with the same signature: FAIL tests/test_lock_registry.c:1153: ASSERT(tail_queued) 7648 passed / 1 failed The registry is not racy. cbm_lock_registry_acquire enqueues the waiter synchronously under the registry mutex before any wait, so waiter_count and the attempting count are exact. The defect is in the fixture: it raced two wall-clock windows against each other, both anchored to a timestamp the observer thread took before the tail thread had even been scheduled. deadline_start = cbm_now_ms(); tail.deadline_ms = deadline_start + 200; /* the tail's acquire deadline */ queue_deadline = deadline_start + 100; /* the observer's budget */ Because both windows start before the tail runs, a loaded runner breaks the fixture two different ways: * the tail is scheduled inside its deadline but after the observer's 100 ms budget has expired -- the queued state existed and was simply no longer being looked at; or * the tail is scheduled more than 200 ms late, in which case its deadline has already passed when it finally calls acquire, the pre-registration deadline check in lock_registry_acquire_internal returns BUSY immediately, and the tail never enqueues at all -- the asserted state can then never occur, however long the observer waits. lock_registry is in the parallel wave of run-tests-parallel.sh, not the serial tail, so on a small CI runner it competes with a full wave of sanitized suites -- exactly the scheduling delay both paths need. Widening the observer's budget would only paper over the first path, and the state it waits for is transient by construction: it exists only between the tail's enqueue and the tail's own deadline. So the fixture is rebuilt to observe states that cannot evaporate. * The tail anchors its absolute deadline itself, in its own thread, immediately before the acquire it bounds. Scheduling delay can no longer consume the deadline before the call starts, so the enqueue is unconditional, and elapsed is measured from the tail's own anchor -- it now times the registry instead of timing the scheduler. * Enqueue is exposed as a monotonic counter, cbm_lock_registry_waiter_enqueue_count_for_test, next to the existing test_condition_wait_calls counter it is modelled on. Because the count only ever grows, the observer reads it once after the tail has returned rather than trying to catch a live queue depth: the polling loop, and with it the window, is gone. * The fixture's remaining 500 ms and 600 ms budgets become the file's LOCK_REGISTRY_TEST_TIMEOUT_MS backstop, and each loop exits on the state it waits for instead of on the clock, so the backstop only fires when the product is actually broken. The contract is unchanged: the tail must still return at its absolute deadline (150 <= elapsed < 350 ms for a 200 ms deadline) despite ~40 unrelated cancel broadcasts, still with BUSY and no lease, with the head still holding the attempt. The broadcast loop now starts immediately after the tail is released, so it overlaps the tail's wait at least as much as it did before. Verification, all on macOS arm64 with the sanitized runner: * The mechanism was reproduced locally by delaying only the tail thread after its start gate, with production untouched. A 120 ms delay (the state exists, outside the observer's budget) and a 250 ms delay (the tail never enqueues) each produced exactly one failure, ASSERT(tail_queued) -- the CI signature. * After the rebuild the same injections are green at 120, 250, 400 and 900 ms: an arbitrary scheduling delay no longer decides the verdict. * lock_registry 30/30 green plain and 30/30 green under four CPU hogs; 16/16 inside the saturated 18-job parallel wave. * private_file_lock, lock_registry, daemon, project_lock and the daemon_* suites: 222 passed, 1 skipped. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
3 participants
What does this PR do?
Preserve extensionless dotted JavaScript/TypeScript module basenames such as
featureX.engineduring relative-import resolution.final dot suffix as an extension.
relative resolver has normalized the path.
helperBandSomeTypeIMPORTSedges both targetfeatureX.engine.ts.Refs #1682.
Verification
make -f Makefile.cbm test-focused TEST_SUITES='fqn edge_imports'— 143 passedmake -f Makefile.cbm test-par— 7,577 passed, 0 failed, 8 skipped across 139 suitesgit diff --checkclang-format --dry-run --WerrorThe repository-wide
lint-cibaseline is not clean in untouched files: cppcheckreports existing style findings in
src/store/store.candsrc/cypher/cypher.c,and the formatter reports existing findings in
src/mcp/mcp.candsrc/pipeline/pipeline_incremental.c. No unrelated lint cleanup is included here.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)