Merge branch 'main' into feat/help-feedback-entry

.github/workflows/ci.yml

@@ -513,11 +513,28 @@ jobs:
         run: node .github/reviewer-routing.test.js
 
   test-focused:
-    name: Test (changed packages)
+    name: "Test (focused: ${{ matrix.shard }})"
     needs: lint
     if: ${{ needs.lint.outputs.changelog_only != 'true' && needs.lint.outputs.docs_only != 'true' && needs.lint.outputs.full_suite != 'true' }}
     runs-on: ubuntu-latest
+    # Each shard owns one bounded slice of the impacted set, so no single job
+    # carries internal/app together with every reverse dependency. The shard
+    # list and per-shard execution below mirror test-race, which runs the same
+    # shards at full-suite scope; release-scripts is included because its
+    # dedicated job only runs at full-suite or release-sensitive scope, and
+    # dropping it here would stop testing test/scripts changes entirely.
     timeout-minutes: 20
+    strategy:
+      fail-fast: false
+      matrix:
+        shard:
+          - app
+          - generators
+          - helpers
+          - cli
+          - smoke
+          - remaining
+          - release-scripts
     steps:
       - name: Check out repository
         uses: actions/checkout@v4
@@ -546,23 +563,77 @@ jobs:
         with:
           go-version-file: go.mod
 
-      - name: Test changed packages and reverse dependencies
+      - name: Select impacted packages for shard
+        id: select
         shell: bash
         env:
-          DWS_PACKAGE_VERSION: 0.0.0-test
+          TEST_SHARD: ${{ matrix.shard }}
         run: |
           set -euo pipefail
           package_output="$(
             ./scripts/ci/changed-test-packages.sh \
-              list "$TEST_BASE_REF" "$TEST_HEAD_REF"
+              list-shard "$TEST_SHARD" "$TEST_BASE_REF" "$TEST_HEAD_REF"
           )"
           if [ -z "$package_output" ]; then
-            echo "No buildable Go package is affected by this revision." \
+            echo "No buildable Go package in shard $TEST_SHARD is affected by this revision." \
               >> "$GITHUB_STEP_SUMMARY"
+            echo "affected=false" >> "$GITHUB_OUTPUT"
             exit 0
           fi
-          mapfile -t packages <<< "$package_output"
-          go test -v -race -count=1 -timeout=15m "${packages[@]}"
+          # The package list travels through a file rather than a step output:
+          # reading it with `mapfile < file` has unambiguous line semantics,
+          # whereas a here-string over a multi-line output would append an extra
+          # empty element if the value ever carried a trailing newline, and an
+          # empty element would reach go test as an empty package argument.
+          printf '%s\n' "$package_output" > "$RUNNER_TEMP/focused-shard-packages.txt"
+          echo "affected=true" >> "$GITHUB_OUTPUT"
+
+      - name: Build
+        if: ${{ matrix.shard == 'remaining' && steps.select.outputs.affected == 'true' }}
+        run: make build
+
+      - name: Install archive tooling
+        if: ${{ matrix.shard == 'release-scripts' && steps.select.outputs.affected == 'true' }}
+        run: sudo apt-get update && sudo apt-get install -y zip unzip
+
+      - name: Test shard with Race Detection
+        if: ${{ steps.select.outputs.affected == 'true' }}
+        shell: bash
+        env:
+          DWS_PACKAGE_VERSION: 0.0.0-test
+          TEST_SHARD: ${{ matrix.shard }}
+        run: |
+          set -euo pipefail
+          mapfile -t packages < "$RUNNER_TEMP/focused-shard-packages.txt"
+          test "${#packages[@]}" -gt 0
+          for package in "${packages[@]}"; do
+            test -n "$package" || {
+              echo "shard package list contains an empty entry" >&2
+              exit 1
+            }
+          done
+          if [ "$TEST_SHARD" = "app" ]; then
+            # A single long-lived app test process retains every constructed
+            # command tree in framework registries. Isolate Schema assembly and
+            # bounded name ranges so each process releases that state on exit.
+            test "${#packages[@]}" -eq 1
+            ./scripts/ci/run-app-race-tests.sh run "${packages[0]}"
+            exit 0
+          fi
+          if [ "$TEST_SHARD" = "release-scripts" ]; then
+            # Mirror the dedicated release-contract job: these suites shell out
+            # to archive tooling and are not race-instrumented there.
+            go test -v -count=1 -timeout=10m "${packages[@]}"
+            exit 0
+          fi
+          # cli/smoke own heavy NewRootCommand / Schema assembly under -race;
+          # give them a dedicated package timeout on slower hosted runners.
+          timeout_budget=12m
+          if [ "$TEST_SHARD" = "cli" ] ||
+             [ "$TEST_SHARD" = "smoke" ]; then
+            timeout_budget=15m
+          fi
+          go test -v -race -count=1 -timeout="$timeout_budget" "${packages[@]}"
 
   test-race:
     name: "Test (race: ${{ matrix.shard }})"
@@ -715,7 +786,7 @@ jobs:
           failed=0
           if [ "$CHANGELOG_ONLY" = true ] || [ "$DOCS_ONLY" = true ]; then
             for shard in \
-              "changed packages:$FOCUSED_RESULT" \
+              "focused shards:$FOCUSED_RESULT" \
               "race shards:$RACE_RESULT" \
               "release scripts:$RELEASE_SCRIPTS_RESULT" \
               "cross-platform compile:$CROSS_PLATFORM_RESULT" \
@@ -744,7 +815,7 @@ jobs:
             release_expected=success
           fi
           for shard in \
-            "changed packages:$FOCUSED_RESULT:$focused_expected" \
+            "focused shards:$FOCUSED_RESULT:$focused_expected" \
             "race shards:$RACE_RESULT:$race_expected" \
             "release scripts:$RELEASE_SCRIPTS_RESULT:$release_expected" \
             "cross-platform compile:$CROSS_PLATFORM_RESULT:success"

scripts/ci/changed-test-packages.sh

@@ -7,16 +7,32 @@ set -eu
 ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd)"
 
 usage() {
-  printf '%s\n' "usage: $0 <changed|list> BASE_REF HEAD_REF" >&2
+  printf '%s\n' \
+    "usage: $0 <changed|list> BASE_REF HEAD_REF" \
+    "       $0 list-shard SHARD BASE_REF HEAD_REF" >&2
   exit 2
 }
 
-[ "${1:-}" = changed ] || [ "${1:-}" = list ] || usage
-[ "$#" -eq 3 ] || usage
-
-MODE="$1"
-BASE_REF="$2"
-HEAD_REF="$3"
+# list-shard narrows the impacted set to one test shard so focused CI runs can
+# fan the same package plan across the shard matrix instead of testing every
+# impacted package in a single long-lived job.
+SHARD=""
+case "${1:-}" in
+  changed|list)
+    [ "$#" -eq 3 ] || usage
+    MODE="$1"
+    BASE_REF="$2"
+    HEAD_REF="$3"
+    ;;
+  list-shard)
+    [ "$#" -eq 4 ] || usage
+    MODE="$1"
+    SHARD="$2"
+    BASE_REF="$3"
+    HEAD_REF="$4"
+    ;;
+  *) usage ;;
+esac
 
 cd "$ROOT"
 git rev-parse --verify --quiet "${BASE_REF}^{commit}" >/dev/null || {
@@ -116,9 +132,29 @@ while IFS= read -r file; do
   [ "$embedded_owner_found" = true ] || continue
 done < "$files"
 
+# emit_selection prints the planned packages, restricted to SHARD when the
+# caller asked for one. Restriction reuses scripts/ci/test-packages.sh as the
+# single source of shard membership, which also means an unknown shard name
+# aborts there instead of being reported as an empty selection: a silently empty
+# shard would let a mistyped CI shard name skip every test and still succeed.
+emit_selection() {
+  selection="$1"
+  if [ -z "$SHARD" ]; then
+    cat "$selection"
+    return 0
+  fi
+  shard_packages="$workdir/shard-packages"
+  "$ROOT/scripts/ci/test-packages.sh" list "$SHARD" > "$shard_packages"
+  LC_ALL=C sort -u "$shard_packages" -o "$shard_packages"
+  LC_ALL=C sort -u "$selection" -o "$selection"
+  # An empty intersection is a normal outcome for a shard no change touched, so
+  # comm's silence must not be treated as an error.
+  LC_ALL=C comm -12 "$selection" "$shard_packages"
+}
+
 LC_ALL=C sort -u "$changed_packages" -o "$changed_packages"
 if [ "$MODE" = changed ] || [ ! -s "$changed_packages" ]; then
-  cat "$changed_packages"
+  emit_selection "$changed_packages"
   exit 0
 fi
 
@@ -133,4 +169,5 @@ while IFS= read -r package; do
 done < "$all_packages" > "$impacted_packages"
 
 cat "$changed_packages" >> "$impacted_packages"
-LC_ALL=C sort -u "$impacted_packages"
+LC_ALL=C sort -u "$impacted_packages" -o "$impacted_packages"
+emit_selection "$impacted_packages"

test/scripts/changed_test_packages_test.go

@@ -129,6 +129,245 @@ func Value() string {
 	}
 }
 
+// shardChangedPackagesShards mirrors the shard names scripts/ci/test-packages.sh
+// asserts complete coverage for in its verify mode. Keeping the list here in
+// full is deliberate: the union assertion below fails if a shard is added
+// upstream without being selected, and an unknown-shard error fails if one is
+// removed, so shard-plan drift cannot silently shrink focused-test coverage.
+var shardChangedPackagesShards = []string{
+	"app",
+	"generators",
+	"helpers",
+	"cli",
+	"smoke",
+	"remaining",
+	"release-scripts",
+}
+
+func TestChangedTestPackagesShardSelectionCoversEveryImpactedPackageExactlyOnce(t *testing.T) {
+	repository := newShardChangedPackagesRepository(t)
+	baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture")
+
+	writeChangedTestPackagesFixture(t, repository, "core/core.go", `package core
+
+func Value() string {
+	return "changed"
+}
+`)
+	headRef := commitChangedTestPackagesFixture(t, repository, "change core")
+
+	impacted := runChangedTestPackages(t, repository, "list", baseRef, headRef)
+	if len(impacted) == 0 {
+		t.Fatal("shard fixture produced no impacted packages")
+	}
+
+	owner := map[string]string{}
+	union := make([]string, 0, len(impacted))
+	for _, shard := range shardChangedPackagesShards {
+		for _, pkg := range runChangedTestPackagesShard(t, repository, shard, baseRef, headRef) {
+			if previous, ok := owner[pkg]; ok {
+				t.Fatalf("package %q selected by both shard %q and %q", pkg, previous, shard)
+			}
+			owner[pkg] = shard
+			union = append(union, pkg)
+		}
+	}
+
+	slices.Sort(union)
+	if !slices.Equal(union, impacted) {
+		t.Fatalf("shard selection union = %q, want every impacted package %q", union, impacted)
+	}
+}
+
+func TestChangedTestPackagesShardSelectionFailsClosedOnUnknownShard(t *testing.T) {
+	repository := newShardChangedPackagesRepository(t)
+	baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture")
+
+	writeChangedTestPackagesFixture(t, repository, "core/core.go", `package core
+
+func Value() string {
+	return "changed"
+}
+`)
+	headRef := commitChangedTestPackagesFixture(t, repository, "change core")
+
+	script := filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh")
+	command := exec.Command(script, "list-shard", "nonexistent", baseRef, headRef)
+	command.Dir = repository
+	output, err := command.CombinedOutput()
+	if err == nil {
+		t.Fatalf("unknown shard unexpectedly succeeded:\n%s", output)
+	}
+	// An unknown shard must abort rather than report an empty selection: a
+	// silently empty shard would let a mistyped CI shard name skip every test
+	// while still reporting success.
+	if !strings.Contains(string(output), "unknown test package shard") {
+		t.Fatalf("unknown shard failure = %q, want fail-closed diagnostic", output)
+	}
+	if strings.TrimSpace(string(output)) == "" {
+		t.Fatal("unknown shard produced empty output instead of a diagnostic")
+	}
+}
+
+func TestChangedTestPackagesShardSelectionIsEmptyForUnaffectedShard(t *testing.T) {
+	repository := newShardChangedPackagesRepository(t)
+	baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture")
+
+	writeChangedTestPackagesFixture(t, repository, "internal/helpers/helpers.go", `package helpers
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " helpers changed"
+}
+`)
+	headRef := commitChangedTestPackagesFixture(t, repository, "change helpers only")
+
+	if selected := runChangedTestPackagesShard(t, repository, "helpers", baseRef, headRef); len(selected) == 0 {
+		t.Fatal("helpers shard selected nothing for a helpers-only change")
+	}
+	// An unaffected shard is a normal outcome and must exit zero with no
+	// output so the CI step can no-op instead of failing the job.
+	if selected := runChangedTestPackagesShard(t, repository, "app", baseRef, headRef); len(selected) != 0 {
+		t.Fatalf("app shard = %q, want empty for a helpers-only change", selected)
+	}
+}
+
+func TestChangedTestPackagesShardSelectionRejectsMissingShardArgument(t *testing.T) {
+	repository := newShardChangedPackagesRepository(t)
+	baseRef := commitChangedTestPackagesFixture(t, repository, "initial shard fixture")
+
+	script := filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh")
+	command := exec.Command(script, "list-shard", baseRef, baseRef)
+	command.Dir = repository
+	output, err := command.CombinedOutput()
+	if err == nil {
+		t.Fatalf("list-shard without a shard argument unexpectedly succeeded:\n%s", output)
+	}
+	if !strings.Contains(string(output), "usage:") {
+		t.Fatalf("missing shard argument failure = %q, want usage diagnostic", output)
+	}
+}
+
+func runChangedTestPackagesShard(t *testing.T, repository, shard, baseRef, headRef string) []string {
+	t.Helper()
+
+	output := runChangedTestPackagesCommand(
+		t,
+		repository,
+		filepath.Join(repository, "scripts", "ci", "changed-test-packages.sh"),
+		"list-shard",
+		shard,
+		baseRef,
+		headRef,
+	)
+	return strings.Fields(output)
+}
+
+// newShardChangedPackagesRepository builds a fixture whose directory layout
+// matches every shard scripts/ci/test-packages.sh knows about, so shard
+// selection can be exercised without depending on the real repository's working
+// tree state. Every package imports core, letting a single core edit mark the
+// whole module impacted.
+func newShardChangedPackagesRepository(t *testing.T) string {
+	t.Helper()
+
+	repository := t.TempDir()
+	for path, contents := range map[string]string{
+		"go.mod": `module example.com/shardfixture
+
+go 1.22
+`,
+		"core/core.go": `package core
+
+func Value() string {
+	return "core"
+}
+`,
+		"internal/app/app.go": `package app
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " app"
+}
+`,
+		"internal/generator/gen/gen.go": `package gen
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " gen"
+}
+`,
+		"internal/helpers/helpers.go": `package helpers
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " helpers"
+}
+`,
+		"internal/cli/cli.go": `package cli
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " cli"
+}
+`,
+		"test/smoke/smoke.go": `package smoke
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " smoke"
+}
+`,
+		"test/scripts/scripts.go": `package scripts
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " scripts"
+}
+`,
+		"pkg/extra/extra.go": `package extra
+
+import "example.com/shardfixture/core"
+
+func Value() string {
+	return core.Value() + " extra"
+}
+`,
+	} {
+		writeChangedTestPackagesFixture(t, repository, path, contents)
+	}
+
+	projectRoot, err := filepath.Abs(filepath.Join("..", ".."))
+	if err != nil {
+		t.Fatalf("resolve project root: %v", err)
+	}
+	for _, name := range []string{"changed-test-packages.sh", "test-packages.sh"} {
+		script, err := os.ReadFile(filepath.Join(projectRoot, "scripts", "ci", name))
+		if err != nil {
+			t.Fatalf("read %s: %v", name, err)
+		}
+		scriptPath := filepath.Join(repository, "scripts", "ci", name)
+		if err := os.MkdirAll(filepath.Dir(scriptPath), 0o755); err != nil {
+			t.Fatalf("create script directory: %v", err)
+		}
+		if err := os.WriteFile(scriptPath, script, 0o755); err != nil {
+			t.Fatalf("copy %s: %v", name, err)
+		}
+	}
+
+	runChangedTestPackagesCommand(t, repository, "git", "init", "-q", "-b", "main")
+	runChangedTestPackagesCommand(t, repository, "git", "config", "user.name", "DWS CI")
+	runChangedTestPackagesCommand(t, repository, "git", "config", "user.email", "dws-ci@example.invalid")
+	return repository
+}
+
 func newChangedTestPackagesRepository(t *testing.T) string {
 	t.Helper()
 

test/scripts/changelog_pr_gate_test.go

@@ -1151,13 +1151,13 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) {
 		"filename.startsWith('pkg/cmdutil/')",
 		"filename === 'skills_embed.go'",
 		"filename.startsWith('test/mock_mcp/')",
-		"name: Test (changed packages)",
+		`name: "Test (focused: ${{ matrix.shard }})"`,
 		"changed-test-packages.sh",
 		"Verify authoritative synthetic merge",
 		`test "$(git rev-parse HEAD^1)" = "$PR_BASE_SHA"`,
 		`test "$(git rev-parse HEAD^2)" = "$PR_HEAD_SHA"`,
 		`echo "TEST_HEAD_REF=$(git rev-parse HEAD)"`,
-		"list \"$TEST_BASE_REF\" \"$TEST_HEAD_REF\"",
+		`list-shard "$TEST_SHARD" "$TEST_BASE_REF" "$TEST_HEAD_REF"`,
 		"needs.lint.outputs.full_suite != 'true'",
 		`name: "Test (race: ${{ matrix.shard }})"`,
 		"name: Test (workflow and release contracts)",
@@ -1217,8 +1217,30 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) {
 	if !strings.Contains(focusedJob, "timeout-minutes: 20") {
 		t.Error("focused test job must allow the scoped race suite up to 20 minutes")
 	}
-	if !strings.Contains(focusedJob, `go test -v -race -count=1 -timeout=15m "${packages[@]}"`) {
-		t.Error("focused race tests must retain enough package-level time for internal/app")
+	// The focused path fans the impacted set across the same shards as test-race
+	// and runs each shard the way test-race runs it, so no single job carries
+	// internal/app together with its reverse dependencies. internal/app keeps its
+	// package-level headroom through the process-isolating helper instead of one
+	// long -timeout, which is strictly stronger: every process releases the
+	// framework registries it populated. release-scripts is asserted because its
+	// dedicated job only runs at full-suite or release-sensitive scope, so losing
+	// it here would silently stop testing test/scripts changes.
+	for _, want := range []string{
+		`if [ "$TEST_SHARD" = "app" ]; then`,
+		`test "${#packages[@]}" -eq 1`,
+		`./scripts/ci/run-app-race-tests.sh run "${packages[0]}"`,
+		`if [ "$TEST_SHARD" = "release-scripts" ]; then`,
+		`go test -v -count=1 -timeout=10m "${packages[@]}"`,
+		"timeout_budget=12m",
+		`if [ "$TEST_SHARD" = "cli" ] ||`,
+		`[ "$TEST_SHARD" = "smoke" ]; then`,
+		"timeout_budget=15m",
+		`go test -v -race -count=1 -timeout="$timeout_budget" "${packages[@]}"`,
+		"- release-scripts",
+	} {
+		if !strings.Contains(focusedJob, want) {
+			t.Errorf("focused test job missing shard contract %q", want)
+		}
 	}
 
 	raceStart := focusedEnd