Merge pull request #41 from xraph/ci/shared-go-ci

ci: run the test matrix through shared go-ci

Author: juicycleff

.github/workflows/go.yml

@@ -13,80 +13,34 @@ permissions:
 
 jobs:
   # Build and test
+  # Gating test legs: ubuntu + macos. Runs through the shared workflow, which
+  # provisions the Node toolchain the generated-client runtime tests need.
   test:
-    name: Test on ${{ matrix.os }}
-    runs-on: ${{ matrix.os }}
-    timeout-minutes: 15
-    strategy:
-      fail-fast: false
-      matrix:
-        os: [ubuntu-latest, macos-latest, windows-latest]
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@v4
-        with:
-          sparse-checkout: |
-            /*
-            !docs/
-          sparse-checkout-cone-mode: false
-
-      - name: Set up Go
-        uses: actions/setup-go@v5
-        with:
-          go-version-file: go.mod
-          check-latest: true
-          cache-dependency-path: |
-            **/go.sum
-
-      - name: Download dependencies
-        run: go mod download
-
-      - name: Verify dependencies
-        run: go mod verify
-
-      - name: Check go.mod tidiness
-        if: matrix.os != 'windows-latest'
-        run: |
-          go mod tidy
-          git diff --exit-code go.mod go.sum || (echo "❌ go.mod or go.sum needs tidying" && exit 1)
-
-      - name: Build
-        run: go build -v ./...
-
-      - name: Set up Node
-        uses: actions/setup-node@v4
-        with:
-          node-version: '20'
-
-      - name: Install TypeScript and esbuild
-        # esbuild is required by the generated-client runtime tests, which
-        # bundle an emitted client and execute it under Node. Without it the
-        # harness falls back to `npx esbuild`, which cannot auto-install
-        # non-interactively in CI ("npx canceled due to missing packages and
-        # no YES option") -- those tests then fail rather than skip, and the
-        # execution coverage they provide is exactly the coverage that catches
-        # runtime/type mismatches the tsc gate cannot see.
-        run: npm install -g typescript@5.8.2 esbuild@0.28.1
-
-      - name: Run tests
-        shell: bash
-        run: |
-          PKGS=$(go list ./... | grep -v '/bk/')
-          go test -v -short -race -timeout=10m -coverprofile=coverage.out $PKGS
-
-      - name: Upload coverage reports to Codecov
-        uses: codecov/codecov-action@v5
-        with:
-          token: ${{ secrets.CODECOV_TOKEN }}
-          files: coverage.out
-
-      - name: Upload coverage
-        if: matrix.os == 'ubuntu-latest'
-        uses: actions/upload-artifact@v4
-        with:
-          name: coverage-report
-          path: coverage.out
-          retention-days: 7
+    uses: xraph/workflows/.github/workflows/go-ci.yml@v1
+    with:
+      go-versions: '["1.26"]'
+      os: '["ubuntu-latest","macos-latest"]'
+      only-test: true
+      node-version: '20'
+      npm-global-packages: 'typescript@5.8.2 esbuild@0.28.1'
+      # `make test` sweeps every module in the repo; CI deliberately tests only
+      # the root module and leaves the rest to build-all-modules.
+      prefer-makefile: false
+    secrets:
+      CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+
+  # Windows is optional and must never block a merge - ci-summary deliberately
+  # leaves this out of its gate, matching the pre-migration policy.
+  test-windows:
+    uses: xraph/workflows/.github/workflows/go-ci.yml@v1
+    with:
+      go-versions: '["1.26"]'
+      os: '["windows-latest"]'
+      only-test: true
+      node-version: '20'
+      npm-global-packages: 'typescript@5.8.2 esbuild@0.28.1'
+      prefer-makefile: false
+      coverage: false
 
   # Build and vet ALL submodules (extensions, examples, cmd)
   build-all-modules:
@@ -206,8 +160,9 @@ jobs:
           args: --timeout=5m --exclude-dirs=bk
         continue-on-error: true
 
+      # bk/ no longer exists, so the old `grep -v '/bk/'` filter was a no-op.
       - name: Run go vet
-        run: go vet $(go list ./... | grep -v '/bk/')
+        run: go vet ./...
 
   # Security scanning
   security:
@@ -300,41 +255,50 @@ jobs:
   ci-summary:
     name: CI Summary
     runs-on: ubuntu-latest
-    needs: [test, build-all-modules, lint, security, build-cli]
+    needs: [test, test-windows, build-all-modules, lint, security, build-cli]
     if: always()
     steps:
       - name: Generate summary
+        env:
+          TEST_RESULT: ${{ needs.test.result }}
+          WINDOWS_RESULT: ${{ needs.test-windows.result }}
+          MODULES_RESULT: ${{ needs.build-all-modules.result }}
+          LINT_RESULT: ${{ needs.lint.result }}
+          SECURITY_RESULT: ${{ needs.security.result }}
+          CLI_RESULT: ${{ needs.build-cli.result }}
         run: |
-          cat >> $GITHUB_STEP_SUMMARY << EOF
-          # CI Results Summary
-          
-          ## Job Status
-          - **Test**: ${{ needs.test.result }} (Windows optional)
-          - **Build All Modules**: ${{ needs.build-all-modules.result }}
-          - **Lint**: ${{ needs.lint.result }}
-          - **Security**: ${{ needs.security.result }}
-          - **Build CLI**: ${{ needs.build-cli.result }}
-          
-          ## Details
-          - **Commit**: ${{ github.sha }}
-          - **Branch**: ${{ github.ref_name }}
-          - **Triggered by**: ${{ github.event_name }}
-          - **Run number**: ${{ github.run_number }}
-          
-          > **Note**: Windows tests are optional and failures won't block CI
-          
-          EOF
-          
-          # Allow Windows test failures
-          if [ "${{ needs.build-all-modules.result }}" != "success" ] || \
-             [ "${{ needs.lint.result }}" != "success" ] || \
-             [ "${{ needs.security.result }}" != "success" ] || \
-             [ "${{ needs.build-cli.result }}" != "success" ]; then
-            echo "❌ **CI Failed** - Please check the logs above" >> $GITHUB_STEP_SUMMARY
+          set -uo pipefail
+          {
+            echo "# CI Results Summary"
+            echo
+            echo "| Job | Result |"
+            echo "|---|---|"
+            echo "| Test (ubuntu, macos) | ${TEST_RESULT} |"
+            echo "| Test (windows, optional) | ${WINDOWS_RESULT} |"
+            echo "| Build All Modules | ${MODULES_RESULT} |"
+            echo "| Lint | ${LINT_RESULT} |"
+            echo "| Security | ${SECURITY_RESULT} |"
+            echo "| Build CLI | ${CLI_RESULT} |"
+          } >> "$GITHUB_STEP_SUMMARY"
+
+          # Windows is explicitly optional and is NOT part of the gate.
+          failed=0
+          for entry in "test:${TEST_RESULT}" "build-all-modules:${MODULES_RESULT}" \
+                       "lint:${LINT_RESULT}" "security:${SECURITY_RESULT}" \
+                       "build-cli:${CLI_RESULT}"; do
+            name="${entry%%:*}"; status="${entry##*:}"
+            if [ "$status" != "success" ]; then
+              echo "::error::Required CI job '$name' did not succeed (result: $status)"
+              failed=1
+            fi
+          done
+
+          if [ "$failed" -ne 0 ]; then
+            echo "**CI failed** - see the job logs above." >> "$GITHUB_STEP_SUMMARY"
             exit 1
-          elif [ "${{ needs.test.result }}" != "success" ]; then
-            echo "⚠️  **Tests have issues (Windows may be failing)** - Please review logs" >> $GITHUB_STEP_SUMMARY
+          fi
+          if [ "$WINDOWS_RESULT" != "success" ]; then
+            echo "**Passed**, with optional Windows tests failing." >> "$GITHUB_STEP_SUMMARY"
           else
-            echo "✅ **All CI checks passed**" >> $GITHUB_STEP_SUMMARY
+            echo "**All CI checks passed**" >> "$GITHUB_STEP_SUMMARY"
           fi
-

extension_runnable.go

@@ -6,7 +6,6 @@ import (
 	"os"
 	"os/exec"
 	"sync"
-	"syscall"
 	"time"
 
 	"github.com/xraph/forge/errors"
@@ -313,9 +312,17 @@ func (e *ExternalAppExtension) Shutdown(ctx context.Context) error {
 		return nil
 	}
 
-	// Send SIGTERM for graceful shutdown
-	if err := e.process.Process.Signal(syscall.SIGTERM); err != nil {
-		e.Logger().Warn("failed to send SIGTERM, forcing kill",
+	// Ask the process to stop cleanly. Platforms without a graceful stop signal
+	// (Windows) go straight to termination rather than logging a failure for a
+	// signal that was never going to be deliverable.
+	if !gracefulStopSupported {
+		e.running = false
+
+		return e.process.Process.Kill()
+	}
+
+	if err := signalGracefulStop(e.process.Process); err != nil {
+		e.Logger().Warn("failed to signal graceful stop, forcing kill",
 			F("name", e.config.Name),
 			F("error", err),
 		)
@@ -389,10 +396,16 @@ func (e *ExternalAppExtension) Health(ctx context.Context) error {
 		return fmt.Errorf("external app %s process is nil", e.config.Name)
 	}
 
-	// On Unix systems, sending signal 0 checks if process exists
-	if err := e.process.Process.Signal(syscall.Signal(0)); err != nil {
-		return fmt.Errorf("external app %s process check failed: %w", e.config.Name, err)
-	}
-
+	// The running flag above is the liveness signal. The monitor goroutine owns
+	// the sole Wait() call and clears the flag under this mutex the moment the
+	// process exits, so it is authoritative.
+	//
+	// This used to additionally probe with Signal(0). That probe failed outright
+	// on Windows, where signals are unsupported, making Health() permanently
+	// return an error for every external app. It also did not do what it claimed
+	// on Unix: a child that has exited but not yet been reaped is a zombie whose
+	// PID still resolves, so Signal(0) succeeds for it, and once the monitor
+	// reaps it the running flag is already false. It could not observe a state
+	// the flag missed.
 	return nil
 }

extension_runnable_helper_test.go

@@ -0,0 +1,119 @@
+package forge
+
+import (
+	"fmt"
+	"os"
+	"os/signal"
+	"strconv"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// The external-app tests need real child processes. Shelling out to `sleep` and
+// `bash` only works where those exist, which excluded Windows and made the suite
+// dependent on whatever happens to be on the runner's PATH.
+//
+// Instead the test binary re-executes itself. os/exec's own tests use this
+// pattern: the child runs TestExternalAppHelperProcess, which dispatches on the
+// arguments after "--" and exits without producing test output. It needs no
+// external tooling, so it behaves identically on every platform.
+
+const helperProcessEnv = "FORGE_EXTERNAL_APP_HELPER"
+
+// helperCommand returns the command and args that re-invoke this test binary in
+// helper mode, plus the environment entry that arms it.
+func helperCommand(args ...string) (command string, cmdArgs []string, env []string) {
+	cmdArgs = append([]string{
+		"-test.run=^TestExternalAppHelperProcess$",
+		"--",
+	}, args...)
+
+	return os.Args[0], cmdArgs, []string{helperProcessEnv + "=1"}
+}
+
+// TestExternalAppHelperProcess is not a real test. It is the entry point for the
+// child processes spawned by the external-app tests; it returns immediately
+// unless the parent armed it via helperCommand.
+func TestExternalAppHelperProcess(t *testing.T) {
+	if os.Getenv(helperProcessEnv) != "1" {
+		return
+	}
+
+	args := helperArgs()
+	if len(args) == 0 {
+		fmt.Fprintln(os.Stderr, "helper: no mode given")
+		os.Exit(2)
+	}
+
+	switch args[0] {
+	case "sleep":
+		helperSleep(args[1:])
+
+	case "writeenv":
+		helperWriteEnv(args[1:])
+
+	case "trap":
+		helperTrap(args[1:])
+
+	default:
+		fmt.Fprintf(os.Stderr, "helper: unknown mode %q\n", args[0])
+		os.Exit(2)
+	}
+
+	// Exit rather than return so the testing framework prints no PASS output
+	// into the parent's captured stream.
+	os.Exit(0)
+}
+
+// helperArgs returns the arguments following "--".
+func helperArgs() []string {
+	for i, a := range os.Args {
+		if a == "--" {
+			return os.Args[i+1:]
+		}
+	}
+
+	return nil
+}
+
+// helperSleep blocks for the given number of seconds.
+func helperSleep(args []string) {
+	seconds := 1.0
+
+	if len(args) > 0 {
+		if v, err := strconv.ParseFloat(args[0], 64); err == nil {
+			seconds = v
+		}
+	}
+
+	time.Sleep(time.Duration(seconds * float64(time.Second)))
+}
+
+// helperWriteEnv writes the value of the named environment variable to a file,
+// followed by a newline. Used to prove ExternalAppConfig.Env reaches the child.
+func helperWriteEnv(args []string) {
+	if len(args) < 2 {
+		fmt.Fprintln(os.Stderr, "helper: writeenv needs <var> <file>")
+		os.Exit(2)
+	}
+
+	if err := os.WriteFile(args[1], []byte(os.Getenv(args[0])+"\n"), 0o600); err != nil {
+		fmt.Fprintf(os.Stderr, "helper: writeenv: %v\n", err)
+		os.Exit(1)
+	}
+}
+
+// helperTrap waits for SIGTERM and exits cleanly, standing in for a child that
+// shuts down gracefully. Unix only — the caller gates on gracefulStopSupported,
+// because Windows has no signal for the parent to send.
+func helperTrap(_ []string) {
+	ch := make(chan os.Signal, 1)
+	signal.Notify(ch, syscall.SIGTERM, os.Interrupt)
+
+	select {
+	case <-ch:
+	case <-time.After(30 * time.Second):
+		// Parent went away without signalling; do not linger on the runner.
+	}
+}

extension_runnable_signal_unix.go

@@ -0,0 +1,18 @@
+//go:build !windows
+
+package forge
+
+import (
+	"os"
+	"syscall"
+)
+
+// gracefulStopSupported reports whether this platform can ask a child process
+// to terminate cleanly rather than killing it outright.
+const gracefulStopSupported = true
+
+// signalGracefulStop asks the process to shut down cleanly. On Unix that is
+// SIGTERM, which a well-behaved child can trap to flush state before exiting.
+func signalGracefulStop(p *os.Process) error {
+	return p.Signal(syscall.SIGTERM)
+}