Skip to content

ci(sweettest): split the slow nondominium shard and stop retrying the serial core-flows story - #152

Merged
Soushi888 merged 3 commits into
devfrom
fix/142-ci-shards-e2e-retry
Oct 27, 2026
Merged

Soushi888 merged 3 commits into
devfrom
fix/142-ci-shards-e2e-retry

Conversation

@Soushi888

@Soushi888 Soushi888 commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

SoushAI analysis. Drafted by Soushi's AI assistant, reviewed and posted by @Soushi888.

Intent

Two fixes for #142, one per problem in the issue. The nondominium Sweettest target ran close to 30 minutes, so it now runs as two halves, and the job gets an explicit 45-minute timeout. The e2e "empty lobby" precondition kept failing because Playwright retried the serial core-flows story against the same conductors; that story no longer retries, so the first real failure is the one reported. Measured on this PR's first CI run, the resource split did not pay off and was dropped: resource stays one target.

Changes

Sweettest shards (.github/workflows/build.yml). The matrix becomes include entries, and nondominium runs as two halves (a, b) of the same [[test]] target, selected by libtest name filters in the run step:

  • half a passes the filter list (substring match, any filter selects);
  • half b passes --skip for exactly the same list.

Every test runs in exactly one half, and a new test can never be dropped: it lands in b unless its name matches a filter. Rebalancing means moving one filter. No test file changes.

target half a half b
nondominium lifecycle, transition history, hibernation, deprecation, invalid transitions (9) create, discovery, property regime, membership (9)

timeout-minutes: 45 on the sweettest job: without it the runner default is 360 minutes, so a hung conductor held a runner for six hours. A hang now names itself in the job annotation.

Job names change from sweettest (nondominium) to sweettest (nondominium, a) and (nondominium, b). dev has no branch protection or rulesets (checked through the API), so no required check is orphaned.

Empty-lobby precondition (ui/tests/e2e/specs/core-flows.spec.ts, ui/tests/README.md). test.describe.configure({ retries: 0 }) inside the core-flows serial story, with a comment naming the cause, and a line in the e2e README.

Decisions
  • The resource split was measured and dropped. On this PR's first run (37550976485) the resource halves took 5.5 and 20.1 min, against 12.7 to 15.9 min for the whole target on dev (runs 36675002358, 39905805052, 37839257499). Half b alone outran the whole target and became the path that gates e2e, for the cost of an extra runner. The nondominium halves came in at 17.0 and 14.8 min against 27.5 to 29.2 min whole, a real gain, so that split stays.
  • The 59m51s resource failure was not a timeout. Its annotation on run 32910203007 reads "The hosted runner lost communication with the server", runner starvation. The timeout does nothing for that; it stays the only instance on record.
  • The leaked lobby group is the suite's own retry. In all three failing runs (32914253920, 32916185361, 32917489986) attempt 1 passed the empty-lobby test and failed later, at lifecycle history panel present. Playwright retries a describe.serial group as a whole against the same conductors, so each retry started with attempt 1's E2E Circle group still in the sidebar, and the precondition error replaced the real one. test(v0.1.0): harden e2e and Sweettest coverage for the peer-mesh release #127's clone-cell teardown is not involved.
  • Not taken: tearing down agent 1's clone cells at the start of a retry. The lobby DNA would still hold the previous attempt's profile and NDO announcements, so a retry would still not start clean, and would fail somewhere less legible. What we lose is a retry for a transient failure in the story's first tests; those are still reported accurately.
  • multi-agent.spec.ts is also describe.serial with the same retry exposure. Left alone because the issue does not name it; a candidate for the same one-line change.
How to test
  • CI on this PR: the checks list sweettest (nondominium, a) and (nondominium, b), and one sweettest (resource). Compare their durations with gh run view <id> --json jobs.
  • actionlint .github/workflows/build.yml: exit 0.
  • Split coverage, real libtest semantics: cargo test --package nondominium_sweettest --test nondominium -- --list with each half's filters inside nix develop gives 9 + 9 = 18 tests, 0 overlap, union equal to the full target.
  • Retry masking, red then green, in a throwaway Playwright 1.61.1 project with retries: 2 and a file standing in for conductor state: without the change the real failure is reported as flaky and the precondition error fails on both retries, the exact CI pattern; with retries: 0 the real failure is the only one reported.
  • playwright test --list from ui/ with CI=1: 23 tests in 2 files.

Related

Pre-open review

Run 2026-10-27 through PRReview at db3d80c, re-run after the fixes at dad7fb0 (rebased onto dev after #157).

  • Description: rewritten to the repository template (Intent, then Changes, Decisions and How to test folded, Related), with Related #142 under Related.
  • Merge state: rebased onto dev (808fcd7), no conflicts.
  • Code findings: the resource split cost more than it saved on the PR's own measurement; dropped in dad7fb0. The filter design was checked against the test names: 9/9, no accidental matches.
  • Documentation: passes; ui/tests/README.md documents the retry opt-out, and the workflow comments document the split with its measurements. No file under documentation/ describes the CI matrix.
  • Tests: CI and test configuration only; the PR's own CI run is the measurement.
  • Left open: the same retries: 0 for multi-agent.spec.ts.

…job at 45 min

At two test threads each pair of conductor tests takes about 2.5 min, so the two largest targets dominated wall clock (nondominium 28.6 min, resource 15.4 min of tests on dev d91fe28). Each now runs as two halves selected by libtest name filters in the run step: half a lists its filters once and half b skips exactly those, so every test runs once and new tests are never dropped. No test file changes.

Without timeout-minutes the runner default is 360 min. The only long failure on record (run 32910203007) was the runner losing communication at 57 min, not a timeout, so an explicit 45 min cap makes a hang name itself.

Refs #142
Playwright retries a describe.serial group as a whole against the same conductors, so every retry of core-flows started with the previous attempt's E2E Circle group in the sidebar and failed the empty-lobby precondition by construction. The retry error then replaced the real failure in the report (runs 32914253920, 32916185361, 32917489986 all failed first at the lifecycle history panel). With retries: 0 for this story the first failure is the one reported, and the precondition only fires on genuinely leaked state.

Refs #142
The resource halves measured 5.5 and 20.1 min against 13 to 16 min for the
whole target (run 37550976485), so the split made resource the critical path
and cost a runner. nondominium's halves (17.0 and 14.8 min against 27.5 to
29.2 whole) are a real gain and stay.
@Soushi888
Soushi888 force-pushed the fix/142-ci-shards-e2e-retry branch from db3d80c to dad7fb0 Compare October 27, 2026 17:47
@Soushi888 Soushi888 changed the title ci: split the slow Sweettest shards and stop retrying the serial core-flows story ci(sweettest): split the slow nondominium shard and stop retrying the serial core-flows story Oct 27, 2026
Soushi888 added a commit that referenced this pull request Oct 27, 2026
README and conventions still named pai/conventions.md and the old .claude/
skill location; pai/README named the pre-rename holochain-agent-skill repo.
The shard count goes from the review docs and CONTRIBUTING, since #152 splits
the nondominium target and the number would drift again.
@Soushi888
Soushi888 marked this pull request as ready for review October 27, 2026 18:35
@Soushi888
Soushi888 merged commit c9dbd50 into dev Oct 27, 2026
8 checks passed
@Soushi888
Soushi888 deleted the fix/142-ci-shards-e2e-retry branch October 27, 2026 18:35
Soushi888 added a commit that referenced this pull request Oct 27, 2026
* chore(pai): make the PR review procedure shared and executable

Adds a nondominium-review skill under pai/, distributed by the existing nix
pipeline to .claude/, .cursor/ and .agents/ so any collaborator's assistant
runs the same procedure. It routes to REVIEW.md and CONTRIBUTING.md rather
than restating them, matching the nondominium-domain skill's discipline.

Adds an advisory lint workflow reporting rustfmt status, documents the two
sources a skill can come from, and corrects the await_consistency call shape
in REVIEW.md, which named a signature that does not compile.

* fix(ci): stop the advisory rustfmt job reporting as a failed check

continue-on-error keeps the workflow green but still marks the job failed, so
every PR showed a red X and gh pr checks reported a failure. Our own review
procedure treats a failing check as a merge blocker, so the advisory job was
blocking reviews it was never meant to gate. The job now always exits 0 and
the blocking form is one commented line away.

* docs(contributing): add the merge conflict resolution policy

The governing question when resolving is which side carried semantic intent
and which only restated it. Presentational changes yield to semantic ones, a
conflict where both sides are semantic is a decision between two authors
rather than a merge mechanic, and a resolution is backed by evidence instead
of asserted.

Names what counts as fundamental here (REQ meaning, enforced validation, VF
field names, NDO layer boundaries) and the two hotspots the open branches
actually conflict on. The integrity enum case is the rule at its sharpest:
hdk_entry_types derives indices from declaration order, so tidying the enum
while resolving invalidates every entry already on the DHT.

The review skill now treats conflict resolution as its own check.

* refactor(pai): separate harness-agnostic sources from harness adapters

pai/shared/ now holds everything that describes the project: conventions,
rules (was cursor-rules/, whose content was never Cursor-specific), and
skills (was claude/skills/, though every skill already fanned out to
.claude/, .cursor/ and .agents/). pai/harnesses/ holds only what one tool
needs of its own, which today is Claude Code's settings.json. Cursor gets
no directory because its adapter is a pure transform in nix/cursor-pai.nix.

The test for where a file belongs: would its content change if we dropped
support for a tool? If no, it is shared.

Adds bun run pai:sync for the inner loop. Nix flakes only see git-tracked
files, so a new skill silently does not materialize until it is committed;
this copies pai/shared/skills/ from the working tree so an uncommitted edit
is visible immediately. Nix stays the authority: it owns the pinned flake
inputs and is what CI runs.

Also removes the pai/README.md description of a LoadProjectDocs.hook.ts
SessionStart hook. No such hook exists and settings.json wires none; the
@ mentions in .rules are the whole mechanism. The docs described a second
injection path that was never built.

* feat(pai): add the /nondominium-review project command

A slash command any contributor gets from the repo itself, with nothing
installed from anywhere else. It is deliberately thin: it points at the
shared nondominium-review skill and adds no judgement of its own, so the
procedure a reviewer follows does not depend on which editor they opened.

Lives under pai/harnesses/claude/ because /name is a Claude Code mechanism
rather than a portable one. The existing rsync copies the whole harness
tree, so a command needs no flake.nix change.

If a general-purpose review skill is installed on the machine, the command
tells it to contribute forge mechanics only: the check order and the verdict
shape stay the project's. A review that silently applied another repo's
conventions is the thing this is meant to prevent.

* docs(pai): correct how /nondominium-review resolves per tool

The previous commit claimed /name was a Claude Code mechanism and gave
Cursor nothing. That was wrong. In Cursor, skills ARE the slash-command
mechanism: typing / lists the skills in .cursor/skills/ and .agents/skills/
and attaches the chosen one. Cursor has no .cursor/commands/ directory.

So the portable command is the skill, which we already distribute to all
three trees, and Claude Code is the single harness needing an adapter file
because its /name reads .claude/commands/ instead. Tibi already had the
command in Cursor; what was missing was saying so.

CONTRIBUTING.md now gives the invocation per tool, and pai/README.md
explains why one harness needs a file and the others do not.

* docs(pai): note the one file the new structure does not classify

pai/human_ai_collaboration.md arrived from #132 at the pai/ root. It reads
as harness-agnostic, so pai/shared/ is the likely home, but it is Tibi's
document and moving it as a side effect of this refactor is his call to
make, not mine.

* docs(pai): declare the pai commit scope

Found by running /nondominium-review against this PR: every commit here
uses a pai scope that the CONTRIBUTING.md scope list does not declare, so
the procedure's own merge-criteria check failed on the branch introducing
it. Adding the scope rather than renaming the commits, since pai/ is a real
area of the repo now.

* docs(pai): move human_ai_collaboration.md into pai/shared/

It arrived from #132 at the pai/ root, which the new structure does not
use. The content is harness-agnostic reference material, so it classifies
as shared by the same test as everything else there: its content would not
change if we dropped support for a tool.

Move only, no edits to the document. Nothing referenced the old path except
the README note now replaced by the listing.

Authorised by @Soushi888.

* docs(pai): account for the workspace file in the tree

pai/nondominium.code-workspace is the last thing at the pai/ root. Listed
rather than moved: editors resolve a workspace file by its path, so it is
not ours to relocate for tidiness. The diagram now shows everything that
is actually there, which is the point of a diagram.

* docs(pai): point stale paths at pai/shared and drop the shard count

README and conventions still named pai/conventions.md and the old .claude/
skill location; pai/README named the pre-rename holochain-agent-skill repo.
The shard count goes from the review docs and CONTRIBUTING, since #152 splits
the nondominium target and the number would drift again.
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.

1 participant