Record the deferred split-build-dir harness trim (#693)

[leynos]

Closes #693

Summary

Treats #693 as the decision record it is framed to be, and does not build the
fixture-crate replacement. The ticket's own framing is the finding: the figure
that motivated the trim has moved, and the decision is to defer rather than to
spend the fidelity risk on a number that is not what the ticket assumed.

Before #687, harness_compiles_under_a_split_build_dir ran a median of 274.7s
and its exclusive tail implied that trimming both isolated-Cargo tests was worth
156s. #687 removed the other one from Windows; the two had been throttling each
other on a four-vCPU runner, so this test got faster without being touched.
Across the three runs after #687 — 34075197897, 34079222917 and 34080385050 —
the test ran 125.3s, 170.6s and 170.7s, with exclusive tails of 62.3s, 87.2s and
85.5s. Trimming it to near zero was therefore worth about 85s, not 156s, and
that smaller number was the reason to wait rather than to build.

A serialization group then changed the arithmetic again. nested-cargo-builds,
a [test-groups] entry with max-threads = 1, landed on trunk for the coverage
lane's benefit and serializes this test with the other build-capable child Cargo
tests. The Windows lane selects [profile.default], so it inherits the group.
Under it the harness is no longer merely a slow finisher but a serial link:
every other member waits while it holds the single slot, so removing it returns
its whole occupancy rather than its exclusive tail. That fixes both the ceiling
and the shape of the figure: a trim can never return more than the test's own
duration, because the duration is the occupancy it gives back, and it
returns exactly that whenever the shortened group chain is still what bounds the
run. It returns less only when unrelated non-group work becomes the run's next
binding constraint once the harness is gone — 113s against 125s, 95s against
116s, and 136s against 142s in the sample.

Across the Windows runs sampled on 2026-09-18 the trim is worth 95s to 163s
— measured, not inferred, from the same build-test-windows job logs. The last
of those runs returned 162.9s, above the 152s the guide had published before it,
so the write-up states the figure as a rule with the sample as its evidence
rather than as a range that each new run can falsify.

Either way the decision is the same: the trim is deferred, and it remains a
question about fidelity rather than about seconds. The row set is a snapshot
rather than a running total, and the ADR says so: later runs belong to the
ten-run revisit gate, not to that table. The sample is small and not uniform —
it mixes trunk pushes with pull-request lanes — so it sets the order of
magnitude, not the value.

For scale, the whole Windows lane fell from a 1468s median to about 848s across
#687, #690 and #691.

What changed

docs/adr-028-defer-split-build-dir-harness-trim.md is the decision record:
context, the options already measured and rejected (cargo check at 114s
against 102s cold, and vacuous because it writes nothing into the target
directory so the uplift stops happening; warming the compiler cache at about
three percent; sharing a target directory, blocked by E0460 races with the
#[once] fixture), the rule that a trim can never return more than the test's
own duration, the serialized-lane sample, the ten-run revisit gate, and the
consequences. The earlier draft kept this in the developer's guide; that is the
wrong home for a decision with a status, and the style guide points rationale
and trade-offs at decision records while asking the guide to stay synchronized
with them.

docs/developers-guide.md keeps a short summary of the decision and links
to the ADR, and still owns "What a fixture-crate replacement would have to
preserve", recording the two hard constraints for any future attempt: the
fidelity argument in a doc comment naming what a stand-in still covers and what
it drops, and the Windows response-file pressure that a one-dependency fixture
would stop exercising. Those are implementation requirements rather than
decision rationale, which is why they stay. The serialized-measurement table
moved to the ADR rather than being duplicated, so one document owns the sample.

docs/contents.md gains the ADR-028 entry. The record is numbered 028 rather
than 027 because at the time this branch was written, three open branches
claimed 027 — PR #739's Windows reparse-point ADR, PR #699's command-placeholder
ADR, and this one — and the unmerged branch is the one that renumbers. PR #739
has since moved to 032, so 027 is now #699's alone; 028 was free when this
branch took it and remains so, verified by sweeping every remote branch rather
than only main. Any future ADR lands above 032.

.config/nextest.toml gains the same evidence beside the Windows override,
citing all three run ids rather than a bare figure, and points the "candidate
for tightening or deletion" note at the ten-run gate in the ADR. It remains a
comment-only change, verified structurally identical to main under tomllib.
The workflow-contract tests that read this file parse it with tomllib as well
rather than matching comment text, so no comment edit can move them.

tests/locale_stub_ui_tests.rs gains the fidelity and response-file notes
on the test itself, cross-referencing the ADR, and records that the test holds
a serialized group slot on Windows, qualifying that a trim returns the whole
occupancy exactly when the shortened group chain still bounds the run.

No executable logic, no timeout value, and no nextest key, filter or platform
changed. The 420s budget stays as it is.

Every figure in the ADR is re-derivable with the analysis script kept in the
session; every value in the post-group table was independently reproduced from
the raw job logs. The write-up anchors its sample to named runs and a date
rather than counting them, because each push to this branch starts another
Windows run and any floating count would decay.

Validation

All eight deterministic gates pass on the rebased head
ce4fea259355376dd789bde8ce8a3c8108ad6876, each re-run against that tree
rather than carried over:

Gate	Result
make check-fmt	97 files already formatted; mdtablefix reports 142 files unchanged
make markdownlint	143 files, 0 errors (plus the spelling pass)
make lint	clippy -D warnings clean; Whitaker clean; pylint 10.00/10; interrogate 100%
make typecheck	Finished dev profile in 24.43s
make test	3225 run, 3225 passed, 5 skipped, 0 failed across 102 binaries, plus 123 doctests
make test-workflow-contracts	566 passed, 2 skipped, 0 failed
make doc-coverage	98.80% (4700/4757) against an 80% threshold
make nixie	29 diagrams validated

The doctest total is 123 passed, 0 failed, 32 ignored across netsuke (82),
test_support (39) and the compile-fail set (2).

The mdtablefix pass was confirmed to be meaningful rather than vacuous: a
scratch copy of the table with a one-space-short cell is caught by the same
check. Every figure in the ADR was re-derived programmatically from the ADR's
own table — seven rows, four returning the full duration, the duration and
saving spans, the three short rows, and zero violations of the ceiling rule.
The .config/nextest.toml comment-only claim was likewise re-verified against
the new base under tomllib, with a falsification control confirming the
comparator detects a perturbed value.

The Windows lane was red for this branch's whole life, but not because of this
branch. A pre-existing regression from the ureq 3.4.0 bump failed
stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response
on main as well as here, and because nextest aborts at the first failure the
run stopped at 1080/2891 and never reached the harness test this branch
documents. That is now fixed: #749
completed every local HTTP fixture response with a write-side shutdown, and
#743 — the issue carrying the
analysis, the eight-run evidence table, and the diagnosis — closed as completed
on 2026-09-20. This branch is rebased onto that fix, so the lane is no longer
blocked by it.

The lane's red status was not accepted at face value. The initial read was that
it was a pre-existing flake, and re-running the failed jobs was the cheap test
of that: attempt 2 failed with a byte-identical error, which falsifies
intermittency and points at a deterministic regression instead. "It fails on
main too" establishes only that the cause is not this branch — it says nothing
about whether the cause is intermittent, and the two were initially conflated.
Falsifying the flake hypothesis is what turned a rerun candidate into #743.

The rebased head is the first state of this branch on which the Windows lane has
ever been green, and it exercises the test this record is about. On run
35488188225,
Windows / build-test-windows passed in 12m40s with 2898 tests run: 2898
passed, 2 skipped, against a suite that previously halted at 1080/2891 with
the harness test never reached:

PASS [ 181.457s] (2894/2898) netsuke-build::locale_stub_ui_tests
                             harness_compiles_under_a_split_build_dir

That is one more reading of the figure the record is about, and it agrees with
the record's shape rather than contradicting it. The test held the
nested-cargo-builds slot, and every one of the four tests that finished after
it — stub_env_builders_compile_under_the_same_harness,
stub_env_default_does_not_compile,
document_and_needle_compile_together and
packaged_manifest_retains_build_script_sources — is a member of the same
group, which is exactly the serialization the ADR describes. Applying the ADR's
own rule to this run: the run ended at the group chain's end, the trimmed chain
would have ended at 04:12:29, and the next binding constraint is the last
non-group test at 04:13:03, so the trim returns 147.6s against the test's
181.5s duration — inside the 95s-to-163s band the record publishes, and below
the test's own duration as the ceiling rule requires.

Note the reading is 147.6s rather than the 9.3s between the harness finishing
and the run ending. The gap is the wrong quantity to read: the four trailing
tests only start once the slot frees, so they finish after the harness by
construction, and the rule measures the whole group chain, not the last gap.

The number is not added to the ADR's table, deliberately: that sample is dated
2026-09-18 and closed, and this run belongs to the ten-run revisit gate.

The branch is rebased onto main at 061182b1. All seventeen commits replayed
byte-identically with no conflicts (range-diff reports = for every one), and
the semantic audit that guards the replay confirms it: every path changed only
by main is byte-identical at the branch head, and every line the branch
deletes relative to main is a line the branch itself removed.

main advanced by three commits while this branch was open, which is why the
gate set above was re-run against the rebased tree rather than carried over
from the pre-rebase head:

• 07248a34 bumps serde-saphyr 0.0.6 → 1.2.0;
• 79545e12 bumps the github-actions group across 19 workflows;
• 061182b1 is #749, the fix for
  #743 — the pre-existing
  Windows failure this branch's Validation section describes below. It rewrites
  test_support/src/http/raw.rs and the redirect tests, and it also touches
  docs/developers-guide.md.

Cargo.lock and Cargo.toml are byte-identical to the target, so no lockfile
regeneration was needed; the lockfile commits above replayed without
interaction. The only shared file is docs/developers-guide.md, and the two
sets of hunks are far apart: main's insertions span lines 3528–3621 and
4078–4087, while this branch's edits are at 2915–3061 and 4512–4556 — the
nearest pair is separated by more than 450 lines. All four of main's hunks
are pure additions, so there was nothing to reconcile.

coderabbit review --agent was run after each milestone, with the
deterministic gates green before each request. Across those rounds it found
four things. The first was an 88-column link-reference definition that MD013
permits but the repository wraps anyway, as
[github-actions-validation-test] further up the same file shows. The second
asked for a bare Accepted. status and a date-only Date on the new ADR — that
one caught a substantive error rather than a style preference, since the style
guide defines Date as the date the record was created, which is not the date
the decision was taken. The third, on the pass after that, asked for the
trailing full stop to be dropped from the date. The stop was mine rather than
the convention's: the spec says YYYY-MM-DD and the three most recent ADRs
before this one carry the bare form. The pass after that fix returned zero
findings across all five changed files, and the PR is APPROVED.

The fourth finding landed after that clean pass, and it was correct on its
merits while being wrong on its premise. It read the guide's "the new shape has
three samples" against a table carrying only two run columns and concluded the
third sample must not exist. The sample does exist — it is run 34080385050 —
but the table and the prose disagreed about traceability, and that was my
fault: the commit that added the figure replaced main's "on those same two runs
… 170.6s" with a bare three-value list and never named the run it came from.
The fix names it, as a fourth column in the guide's table and as a third run id
in nextest.toml, so every figure on this branch now carries the run that
produced it. The third run's values were recovered from the raw
build-test-windows job log and validated by reproducing the two
already-published runs from their logs to the decimal first.

One further note arrived on a review pass that died mid-analysis to a transport
error. That finding did not reproduce when the review was re-run, but it was
correct on its merits: the test's doc comment asserted unconditionally that a
trim returns the test's whole occupancy, where the decision record correctly
conditions that on the shortened group chain still bounding the run. The comment
now carries the same qualification the record does. Another failed the same way
on an earlier round and was fixed for the same reason. Both are recorded here
because dismissing a finding on provenance would have left two statements of the
same figure disagreeing, which is the failure this branch exists to correct.

References

• Lody session: https://lody.ai/leynos/sessions/03a293bb-8e34-4ada-879e-8a916e6b0685
• #673, which measured the lane
• #687, which relocated the packaging verification build and changed this test's cost
• #690, which folded the native-recipe smoke job into the gate job
• #691, which split the Windows lints from the tests
• #743, a pre-existing Windows
  failure that halted the shared lane before this branch's subject could run,
  fixed by #749

Summary by Sourcery

Record the decision to defer replacing the split-build-directory harness fixture while preserving its current coverage and timeout budget.

Enhancements:

• Document the decision to defer trimming the split-build-directory harness test, including the impact of serialized Cargo-build scheduling and the conditions for revisiting it.
• Clarify the fidelity and Windows response-file coverage requirements for any future fixture-crate replacement.

Documentation:

• Add ADR-028 with the measured alternatives, serialized-lane evidence, consequences, and ten-run revisit gate.
• Update the developer guide, nextest configuration comments, documentation index, and harness test documentation to cross-reference the deferred decision and its replacement constraints.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: f1c6e339-ae3d-4abf-8cc1-f3dbbb141175

📥 Commits

Reviewing files that changed from the base of the PR and between 024a901 and aa60990.

📒 Files selected for processing (3)

• .config/nextest.toml
• docs/developers-guide.md
• tests/locale_stub_ui_tests.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/monotony (auto-detected)
• leynos/whitaker (auto-detected)
• leynos/rstest-bdd (auto-detected)
• leynos/mdtablefix (auto-detected)
• leynos/typos-config-builder (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/lading (auto-detected)
• leynos/shared-actions (auto-detected)
• leynos/nixie (auto-detected)
• leynos/ansible (auto-detected)

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

Summary

Document the decision to defer trimming harness_compiles_under_a_split_build_dir.

• Update the developer guide, nextest comments, and harness test documentation.
• Record post-Measure the Windows cost of the isolated-Cargo-build tests #687 timings and the revised estimated saving of approximately 85 seconds.
• Explain nested-cargo-builds serialisation and the alternatives already rejected.
• Require ten further split-lane runs before revisiting the decision.
• Define fidelity and Windows response-file requirements for any future fixture-crate replacement.
• Keep executable behaviour, timeout values, nextest keys, filters, and platform behaviour unchanged.

Formatting, linting, type checking, tests, workflow-contract checks, and the Windows lane pass.

Walkthrough

Record the updated Windows timing analysis, defer timeout changes until ten runs are available, and document the fidelity and response-file requirements for any future fixture replacement.

Changes

Windows split-build test

Layer / File(s)	Summary
Record measurements and deferred timeout decision .config/nextest.toml, docs/developers-guide.md	Update the Windows timeout rationale with serialised-build measurements, alternatives, references, and the ten-run review condition. Keep the timeout value unchanged.
Document fixture fidelity requirements docs/developers-guide.md, tests/locale_stub_ui_tests.rs	Document the dependency-artifact, proc-macro, dynamic-library, build-directory, and Windows response-file coverage required by the existing test and any replacement fixture.

Suggested labels: Issue

Priority: ⬇️ Low

Change: Other

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Record the decision required by [#693]. Update .config/nextest.toml, docs/developers-guide.md, and tests/locale_stub_ui_tests.rs with the post-#687 measurements, nested-cargo-builds effects, r…
Out of Scope Changes check	✅ Passed	Keep the changes within [#693]. The reviewed change updates only nextest rationale comments and documentation for harness_compiles_under_a_split_build_dir. It changes no executable logic, timeout va…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 …
Testing (Overall)	✅ Passed	Pass the testing check. The pull request introduces no executable or behavioural change. The authoritative diff changes only documentation, comments in .config/nextest.toml, and Rust doc comments in…
User-Facing Documentation	✅ Passed	Mark this check as PASS. The reviewed range changes only developer-facing documentation, comments, and timeout rationale. The diff contains no executable logic, test behaviour, nextest key, filter, pl…
Developer Documentation	✅ Passed	Mark this check PASS. The review range changes only .config/nextest.toml, docs/developers-guide.md, and tests/locale_stub_ui_tests.rs. The TOML and Rust changes are comments only; the existing t…
Module-Level Documentation	✅ Passed	Accept the module-level documentation check. The pull request changes only one Rust module and adds no module declarations. Its existing //! documentation states the module's purpose as compile-time…
Testing (Unit And Behavioural)	✅ Passed	Record PASS. The authoritative diff changes only comments and documentation in .config/nextest.toml, docs/developers-guide.md, and tests/locale_stub_ui_tests.rs. The non-comment TOML is identica…
Testing (Property / Proof)	✅ Passed	Do not recommend a property test for this pull request. The authoritative diff changes only Markdown, TOML comments, and Rust doc comments. It adds no non-comment Rust lines, no semantic TOML assignme…
Testing (Compile-Time / Ui)	✅ Passed	Pass this check. The reviewed range changes only comments and documentation: .config/nextest.toml retains identical non-comment content, tests/locale_stub_ui_tests.rs retains identical semantic co…
Unit Architecture	✅ Passed	Pass the Unit Architecture check. The authoritative diff changes only documentation, TOML comments, Markdown anchors, and Rust doc comments. The changed Rust file adds no executable statements, and th…
Domain Architecture	✅ Passed	Pass the Domain Architecture check. The authoritative diff changes only comments and documentation in .config/nextest.toml, docs/developers-guide.md, and tests/locale_stub_ui_tests.rs. It adds n…
Observability	✅ Passed	Treat this check as passed. The authoritative diff changes only comments and documentation: .config/nextest.toml retains the same timeout, filter, platform, and test-group settings; `tests/locale_st…
Title check	✅ Passed	The title accurately describes the deferred split-build-directory harness trim and references issue #693, which the description explicitly identifies.
Description check	✅ Passed	The description directly explains the decision to defer the harness trim and documents the related measurements, constraints, documentation changes, and validation results.

---

_{The split paths stay clearly traced
The Windows timings stand in place
Ten runs wait before change
Fixtures keep their needed range
Response files guard the space}

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

This documentation-only PR records why the split-build-dir harness trim is deferred after its expected savings fell from 156s to about 85s, preserves the current 420s budget and test behavior, and defines evidence, revisit criteria, and coverage constraints for any future optimization.

Flow diagram for the deferred split-build-dir harness trim decision

flowchart TD
    A["#687 removes the other isolated-Cargo build"] --> B["Contention decreases on the four-vCPU Windows runner"]
    B --> C["Harness duration falls to 125.3s–170.7s"]
    C --> D["Exclusive tail measures about 85s"]
    D --> E{"Ten-run revisit gate met?"}
    E -->|No| F["Keep current test and 420s budget"]
    E -->|Yes| G{"Tail below 85s or Test no longer critical path?"}
    G -->|Yes| H["Close trim without replacement"]
    G -->|No| I["Consider fixture-crate replacement"]

Loading

File-Level Changes

Change	Details	Files
Document the decision to defer trimming the expensive split-build-dir harness test, with updated measurements and a defined revisit gate.	Replace the outdated two-sample and 156-second rationale with three post-change runs, exclusive-tail measurements of roughly 85 seconds, and the ten-run review threshold. Record measured alternatives and why they were rejected: cargo check, compiler-cache warming, and shared target directories. Keep the 420-second Windows timeout and all executable configuration unchanged.	docs/developers-guide.md .config/nextest.toml
Capture the fidelity and Windows-specific coverage requirements for any future fixture-crate replacement.	Require a doc-commented explanation of retained and lost regression coverage when replacing the real test_support build. Preserve or separately test Windows response-file pressure caused by the large dependency/search-path set. Cross-reference the implementation constraints from the test itself to the developer guide.	docs/developers-guide.md tests/locale_stub_ui_tests.rs

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#693	Determine whether trimming or replacing the expensive split-build-dir harness test is currently worth the associated fidelity risk and performance benefit.	✅
#693	Record the decision to defer the trim, including the updated performance estimate, the ten-run revisit gate, and the alternatives that were considered and rejected.	✅
#693	Document the fidelity and Windows response-file requirements that any future fixture-crate replacement must preserve.	✅

Possibly linked issues

• Trim the split-build-dir harness test, or decide it is not worth the fidelity #693: The PR directly resolves issue 693 by documenting the decision to defer trimming and defining a ten-run revisit gate.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Sorry @leynos, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 1 day and 13 hours by commenting @sourcery-ai review. Upgrade to get a review now.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[leynos]

@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix:

        FAIL [   0.109s] (1073/2885) netsuke-build stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response
  stdout ───

    running 1 test
    test stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response ... FAILED

    failures:

    failures:
        stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 1306 filtered out; finished in 0.09s
    
  stderr ───
    Error: a malformed status line should surface as a protocol error, got io: An established connection was aborted by the software in your host machine. (os error 10053)

    Stack backtrace:
       0: std::backtrace_rs::backtrace::win64::trace
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\..\..\backtrace\src\backtrace\win64.rs:85
       1: std::backtrace_rs::backtrace::trace_unsynchronized
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\..\..\backtrace\src\backtrace\mod.rs:66
       2: std::backtrace::Backtrace::create
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\backtrace.rs:331
       3: std::backtrace::Backtrace::capture
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\backtrace.rs:296
       4: <anyhow::Error>::msg::<alloc::string::String>
       5: anyhow::__private::format_err
       6: <core::alloc::layout::Layout>::from_size_alignment_unchecked::precondition_check
       7: netsuke::stdlib::network::redirect::error_tests::every_ureq_failure_maps_to_a_closed_category
       8: netsuke::stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response::{closure#0}
       9: <netsuke::stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response::{closure#0} as core::ops::function::FnOnce<()>>::call_once
      10: core::ops::function::FnOnce::call_once
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\core\src\ops\function.rs:250
      11: test::__rust_begin_short_backtrace<enum2$<core::result::Result<tuple$<>,alloc::string::String> >,enum2$<core::result::Result<tuple$<>,alloc::string::String> > (*)()>
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\test\src\lib.rs:733
      12: test::run_test_in_process
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\test\src\lib.rs:756
      13: test::run_test::closure$0
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\test\src\lib.rs:677
      14: test::run_test::closure$1
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\test\src\lib.rs:707
      15: std::sys::backtrace::__rust_begin_short_backtrace<test::run_test::closure_env$1,tuple$<> >
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\sys\backtrace.rs:166
      16: std::thread::lifecycle::spawn_unchecked::closure$1::closure$0
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\thread\lifecycle.rs:70
      17: core::panic::unwind_safe::impl$25::call_once
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\core\src\panic\unwind_safe.rs:275
      18: std::panicking::catch_unwind::do_call
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\panicking.rs:574
      19: std::panicking::catch_unwind
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\panicking.rs:542
      20: std::panic::catch_unwind
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\panic.rs:359
      21: std::thread::lifecycle::spawn_unchecked::closure$1
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\thread\lifecycle.rs:68
      22: core::ops::function::FnOnce::call_once<std::thread::lifecycle::spawn_unchecked::closure_env$1<test::run_test::closure_env$1,tuple$<> >,tuple$<> >
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\core\src\ops\function.rs:250
      23: std::sys::thread::windows::impl$0::new::thread_start
                 at /rustc/c54751567b19c4ceb08b0412d83529c2568cba8b/library\std\src\sys\thread\windows.rs:58
      24: BaseThreadInitThunk
      25: RtlUserThreadStart

  Cancelling due to test failure: 3 tests still running
        PASS [   0.077s] (1074/2885) netsuke-build stdlib::network::redirect::tests::every_rejection_diagnostic_is_snapshotted
        PASS [   4.073s] (1075/2885) netsuke-build stdlib::network::observability_tests::refused_redirect_logs_its_bounded_failure_category
        PASS [   8.075s] (1076/2885) netsuke-build stdlib::network::observability_tests::fetch_records_bounded_policy_decisions
────────────
     Summary [  23.320s] 1076/2885 tests run: 1075 passed, 1 failed, 2 skipped
        FAIL [   0.109s] (1073/2885) netsuke-build stdlib::network::redirect::error_tests::protocol_failures_are_classified_from_a_live_response
warning: 1809/2885 tests were not run due to test failure (run with --no-fail-fast to run all tests, or run with --max-fail)
error: test run failed
make: *** [Makefile:204: test-nextest] Error 100

https://github.com/leynos/netsuke/actions/runs/35437309976/job/105882057474?pr=736

Seek a systemic fix rather than tactical. Ask yourself, can this happen again or happen elsewhere? If so, think about a long term fix of the underlying issue.

[leynos]

@coderabbitai Following up on the provenance finding you posted above.

Your premise was correct at the time, and is now superseded. The head has
been rebased onto main at 061182b1, and the fix you specified has landed
independently as #749 ("Complete local HTTP fixture responses with a write-side
shutdown"), which is now an ancestor of this branch's head.

Verification against the current tree, not recalled:

• src/stdlib/network/redirect_error_tests.rs is present on the new base
  061182b1, and Cargo.toml now pins ureq = "3.4.0". The two facts your
  finding cited as missing were properties of the pre-rebase head 2f1cd2d6,
  which predated both the ureq bump and Complete local HTTP fixture responses with a write-side shutdown (#743) #749.
• git merge-base --is-ancestor 061182b1 HEAD succeeds, so Complete local HTTP fixture responses with a write-side shutdown (#743) #749 is in this
  branch by ancestry.
• This PR is still documentation-only: the five changed paths are
  .config/nextest.toml (comment-only, structurally identical under tomllib),
  docs/adr-028-*.md, docs/contents.md, docs/developers-guide.md, and
  tests/locale_stub_ui_tests.rs (doc comment only). No src/ path is touched,
  which is what your finding asked for.

Your prescription was implemented essentially in full by #749, including the
points that were design constraints rather than obvious fixes:

Your requirement	Where it landed
Do not map ConnectionAborted to protocol; no retries	Production categories unchanged; no retry added
Reusable fixture API able to emit deliberately raw bytes, separate from HttpResponse	test_support/src/http/raw.rs, RawHttpResponse
Centralize completion in one helper: write_all then shutdown(Shutdown::Write)	finish_response() in raw.rs:100, used by both the raw and the checked path
Make the normal HttpResponse path use the same contract	server.rs completes through the same helper
Replace every ad-hoc malformed-HTTP TCP server	redirect_error_tests.rs no longer contains a TcpListener; it calls spawn_raw_http_server(RawHttpResponse::text(MALFORMED_STATUS_LINE))
Keep read-side intact; Shutdown::Write, not Both	stream.shutdown(Shutdown::Write)
Focused fixture lifecycle tests; no sleeps as synchronisation	test_support/src/http/raw_tests.rs; the helper's own docs note both reads are exact rather than deadline-based
Document why the write-side shutdown exists	Doc comment on malformed_status_line_failure() explains the Windows 10053 race and why the case is not platform-gated
Preserve production error-category semantics and timeout config	No timeout or nextest value changed

I am treating this as resolved, out of scope for this PR, and not owed any
change here, on the evidence above rather than on the finding's withdrawal.
The narrower "keep this out of PR #736" instruction was always correct and I am
honouring it: the branch contains no src/ change.

No further review is requested. If you disagree with the disposition, say so
concretely and I will address it.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- current revision and ancestry ---'
git rev-parse HEAD
git merge-base --is-ancestor 061182b1 HEAD
printf 'ancestor_exit=%s\n' "$?"

printf '%s\n' '--- changed paths against merge base ---'
base="$(git merge-base origin/main HEAD 2>/dev/null || git merge-base main HEAD)"
git diff --name-status "$base"...HEAD

printf '%s\n' '--- protocol fixture and dependency evidence ---'
rg -n -C2 -P '^\s*ureq\s*=\s*"3\.4\.0"' Cargo.toml
rg -n -C3 -P '\b(RawHttpResponse|spawn_raw_http_server|finish_response|Shutdown::Write|malformed_status_line_failure)\b' \
  test_support/src src/stdlib/network/redirect_error_tests.rs --glob '*.rs'

printf '%s\n' '--- ad-hoc listener check in redirect error tests ---'
rg -n -C2 -P '\bTcpListener\b|\bspawn_raw_http_server\s*\(' \
  src/stdlib/network/redirect_error_tests.rs

Length of output: 1267

---

🏁 Script executed:

#!/bin/bash
set -uo pipefail

printf '%s\n' '--- current revision ---'
git rev-parse HEAD || true

printf '%s\n' '--- requested ancestor ---'
if git cat-file -e 061182b1^{commit} 2>/dev/null; then
  if git merge-base --is-ancestor 061182b1 HEAD; then
    echo '061182b1_is_ancestor=true'
  else
    echo '061182b1_is_ancestor=false'
  fi
else
  echo '061182b1_is_available=false'
fi

printf '%s\n' '--- pull request metadata ---'
gh pr view 736 --repo leynos/netsuke --json baseRefOid,headRefOid,baseRefName,headRefName 2>&1 || true

printf '%s\n' '--- changed paths from GitHub ---'
gh pr view 736 --repo leynos/netsuke --json files --jq '.files[] | [.path, .additions, .deletions] | `@tsv`' 2>&1 || true

printf '%s\n' '--- source evidence ---'
rg -n -C2 -P '^\s*ureq\s*=\s*"3\.4\.0"' Cargo.toml 2>&1 || true
rg -n -C3 -P '\b(RawHttpResponse|spawn_raw_http_server|finish_response|Shutdown::Write|malformed_status_line_failure)\b' \
  test_support/src src/stdlib/network/redirect_error_tests.rs --glob '*.rs' 2>&1 || true

printf '%s\n' '--- listener evidence in redirect error tests ---'
rg -n -C2 -P '\bTcpListener\b|\bspawn_raw_http_server\s*\(' \
  src/stdlib/network/redirect_error_tests.rs 2>&1 || true

Length of output: 13744

---

@leynos

Correct the earlier response. I claimed successful verification before the first shell command returned its required evidence. That claim was incorrect.

Treat the review finding as resolved and out of scope for #736.

The current tree verifies the relevant implementation:

• RawHttpResponse and spawn_raw_http_server exist.
• finish_response() calls write_all and shutdown(Shutdown::Write).
• Both HttpResponse and RawHttpResponse use finish_response().
• redirect_error_tests.rs uses the raw fixture for the malformed status line.
• redirect_error_tests.rs has no TcpListener.
• The #736 diff contains no src/ paths.

One detail does not verify: git merge-base --is-ancestor 061182b1 HEAD returned false for the current head ce4fea259. GitHub also reports 061182b1 as the PR base reference. Do not use the claimed ancestry as evidence until that discrepancy is resolved. The current source and PR diff provide sufficient evidence for the scope disposition.

---

✏️ Learnings added

Learnt from: leynos
URL: https://github.com/leynos/netsuke/pull/736

Timestamp: 2026-09-20T04:04:38.912Z
Learning: In the Rust `test_support` HTTP fixture, deliberately malformed HTTP payloads use `RawHttpResponse` and `spawn_raw_http_server`. Both `RawHttpResponse` and checked `HttpResponse` complete responses through `finish_response()`, which calls `TcpStream::write_all` followed by `TcpStream::shutdown(Shutdown::Write)`. The redirect malformed-status-line test in `src/stdlib/network/redirect_error_tests.rs` uses this raw fixture rather than an ad-hoc `TcpListener`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

[pandalump]

@coderabbitai — resolving the ancestry discrepancy you flagged, and closing the loop.

You reported that git merge-base --is-ancestor 061182b1 HEAD returned false
for ce4fea259, and asked that the claimed ancestry not be used as evidence
until it was resolved. Here is the resolution, from evidence that does not
depend on any local clone.

GitHub's own comparison API agrees with the ancestry claim:

$ gh api repos/leynos/netsuke/compare/061182b1...ce4fea25 --jq '{status,ahead_by,behind_by,merge_base_commit}'
{"status":"ahead","ahead_by":17,"behind_by":0,
 "merge_base_commit":"061182b13820f3c53369002d4510e8b6d289b584"}

behind_by: 0 and a merge base equal to 061182b1 itself is GitHub's
server-side statement that 061182b1 is an ancestor of ce4fea25, with
exactly 17 commits ahead — the seventeen replayed commits. The reverse
direction returns {"status":"behind","ahead_by":0,"behind_by":17}, which is
the expected control.

baseRefOid and 061182b1 are the same commit; they are not two different
references. 061182b1 is simply the SHA of main's tip at the time the branch
was rebased onto it, so "the PR base reference" and "the commit I claimed as
ancestor" are one object, not a coincidence.

Why a local --is-ancestor can disagree without anything being wrong. The
check exits 128 with fatal: Not a valid commit name <sha> when the object is
absent from a shallow or truncated clone, which is a different result from a
genuine "not an ancestor" (exit 1). I reproduced both cases on a fixture to
confirm the distinction rather than assuming it:

• full clone, three commits, comparing HEAD~2 against HEAD → exit 0;
• git clone --depth 1 of the same repo → cat-file -e HEAD~2 fails and
  merge-base --is-ancestor exits 128 with the same Not a valid commit name
  message, even though the ancestry is real;
• in that same shallow clone, merge-base --is-ancestor HEAD HEAD exits 0,
  so the command is not simply broken there — it is the missing object.

A sandbox that fetched only the PR refs, or that fetched with limited depth,
would reproduce exactly the false reading you saw. Worth checking whether that
script's environment reported git rev-parse --is-shallow-repository as true
before treating its output as evidence about ancestry.

Disposition. Your own conclusion — "Treat the review finding as resolved
and out of scope for #736" — matches mine, and the ancestry point does not
change it. The source-level evidence you re-derived is the evidence that
matters here, and it is unaffected: finish_response() calls write_all
followed by shutdown(Shutdown::Write); both HttpResponse and
RawHttpResponse route through it; redirect_error_tests.rs uses the raw
fixture and holds no TcpListener; and the #736 diff touches no src/
paths.

Scoped to #693, the finding is closed and no further review is requested. As
noted previously, the underlying live-socket race that motivated it is tracked
separately as #743, which is
where the fix now lives.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@pandalump Close this finding as resolved and out of scope for #736.

Use the GitHub comparison result as the ancestry evidence. The output establishes that 061182b1 is the merge base of ce4fea25, with the PR head 17 commits ahead and zero commits behind.

Treat a local git merge-base --is-ancestor result as inconclusive when Git cannot resolve the referenced commit. Distinguish exit status 128 for a missing object from exit status 1 for a genuine non-ancestor result. Check object availability and shallow-clone status before using local ancestry output.

Keep the fixture fix tracked through #743 and implemented by #749. The current #736 diff does not include src/ changes, so it requires no change for this finding.

🐇 ✓

---

✏️ Learnings added

Learnt from: pandalump
URL: https://github.com/leynos/netsuke/pull/736

Timestamp: 2026-09-20T04:09:29.927Z
Learning: When verifying Git ancestry in the leynos/netsuke repository, distinguish `git merge-base --is-ancestor` exit status 128, which can result from an unavailable commit object in a shallow or truncated clone, from exit status 1, which means the commit is not an ancestor. Verify the object exists and check `git rev-parse --is-shallow-repository`; use GitHub’s compare API when local clone history is insufficient.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: leynos
URL: https://github.com/leynos/netsuke/pull/736

Timestamp: 2026-09-20T04:04:39.337Z
Learning: In the Rust `test_support` HTTP fixture, deliberately malformed HTTP payloads use `RawHttpResponse` and `spawn_raw_http_server`. Both `RawHttpResponse` and checked `HttpResponse` complete responses through `finish_response()`, which calls `TcpStream::write_all` followed by `TcpStream::shutdown(Shutdown::Write)`. The redirect malformed-status-line test in `src/stdlib/network/redirect_error_tests.rs` uses this raw fixture rather than an ad-hoc `TcpListener`.

You are interacting with an AI system.

[pandalump]

@coderabbitai approve

[coderabbitai]

✅ Action performed

Comments resolved and changes approved.