refactor: extract shared process_runner.cjs from claude and copilot harnesses

[Copilot]

claude_harness.cjs and copilot_harness.cjs each contained an independent runProcess implementation (~50 lines) plus duplicated formatDuration and sleep helpers — identical lifecycle logic that had already begun to drift in minor ways.

Changes

• New process_runner.cjs — shared utility exporting runProcess({ command, args, attempt, log, logArgs }), formatDuration, and sleep

  • log is caller-supplied so each harness keeps its own prefix ([claude-harness] / [copilot-harness])
  • logArgs (optional) accepts a pre-redacted arg list for logging, keeping sensitive values out of logs without coupling redaction logic to the runner

• claude_harness.cjs — removed ~95 lines of local duplicates; updated call site to new object-param API

• copilot_harness.cjs — removed ~100 lines of local duplicates (including now-unused spawn import); moved --prompt/-p redaction from inside runProcess to the call site, passing it as logArgs

• New process_runner.test.cjs — 19 tests covering formatDuration, sleep, and the full runProcess lifecycle: stdout/stderr forwarding, hasOutput tracking, exit code propagation, close-based resolution, spawn error handling, logArgs redaction, and log truncation

// Before — each harness owned its own copy
function runProcess(command, args, attempt) { /* ~50 lines */ }

// After — shared runner; callers supply log fn and redacted args
const result = await runProcess({ command, args: currentArgs, attempt, log, logArgs: safeArgs });

[Copilot]

Pull request overview

This PR refactors the Claude and Copilot harnesses to share a single runProcess implementation (plus formatDuration / sleep) and adds a dedicated test suite for the shared runner.

Changes:

• Added actions/setup/js/process_runner.cjs exporting shared runProcess, formatDuration, and sleep.
• Updated claude_harness.cjs and copilot_harness.cjs to use the new object-parameter runProcess API (with caller-provided logging and optional redacted logArgs).
• Added process_runner.test.cjs covering duration formatting, sleep, and runProcess lifecycle behavior.

Show a summary per file

File	Description
actions/setup/js/process_runner.cjs	Introduces shared process spawning/forwarding/output-collection logic used by both harnesses.
actions/setup/js/process_runner.test.cjs	Adds Vitest coverage for the shared runner utilities and lifecycle behavior.
actions/setup/js/claude_harness.cjs	Removes duplicated helpers and switches to the shared runner with harness-specific logging/redaction.
actions/setup/js/copilot_harness.cjs	Removes duplicated helpers and switches to the shared runner; moves --prompt/-p redaction to call site via logArgs.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

actions/setup/js/process_runner.test.cjs:79

• These tests spawn real Node processes that write to the parent process stdout/stderr via runProcess’s forwarding (process.stdout.write / process.stderr.write). This will make the Vitest output noisy and can interfere with CI logs; consider stubbing process.stdout.write/process.stderr.write (via vi.spyOn) during the runProcess test suite and restoring afterward.

    it("collects stdout output and sets hasOutput", async () => {
      const logs = [];
      const result = await runProcess({
        command: process.execPath,
        args: ["-e", 'process.stdout.write("hello stdout"); process.exit(0)'],
        attempt: 0,
        log: msg => logs.push(msg),
      });
      expect(result.hasOutput).toBe(true);
      expect(result.output).toContain("hello stdout");
    });

• Files reviewed: 4/4 changed files
• Comments generated: 3

[pelikhan]

@copilot review all comments

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 94/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	19
✅ Design tests (behavioral contracts)	19 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	15 (79%)
Duplicate test clusters	0
Test inflation detected	No (1.49:1 ratio)
🚨 Coding-guideline violations	None

---

Test Classification Details

View all 19 test classifications

Test	File	Classification	Notes
formats zero milliseconds as 0s	process_runner.test.cjs	✅ Design	Boundary value (zero)
formats sub-minute durations as seconds only	process_runner.test.cjs	✅ Design	Multiple boundary values
formats exactly one minute	process_runner.test.cjs	✅ Design	Boundary at 60s
formats minutes and seconds	process_runner.test.cjs	✅ Design	Output contract
truncates sub-second precision	process_runner.test.cjs	✅ Design	Edge case: truncation behavior
resolves after the given delay	process_runner.test.cjs	✅ Design	Timing contract
resolves immediately for 0ms	process_runner.test.cjs	✅ Design	Edge case: zero delay
resolves with exitCode 0 for a successful command	process_runner.test.cjs	✅ Design	Happy path: exit code contract
resolves with the actual non-zero exit code on failure	process_runner.test.cjs	✅ Design	Error path: non-zero exit
collects stdout output and sets hasOutput	process_runner.test.cjs	✅ Design	Output capture contract
collects stderr output and sets hasOutput	process_runner.test.cjs	✅ Design	Stderr capture contract
sets hasOutput false when no output is produced	process_runner.test.cjs	✅ Design	Edge case: empty output
logs spawning with logArgs instead of args when provided	process_runner.test.cjs	✅ Design	Arg redaction contract
falls back to args for logging when logArgs is not provided	process_runner.test.cjs	✅ Design	Fallback log contract
uses the attempt number in log messages	process_runner.test.cjs	✅ Design	Attempt tracking contract
resolves with exitCode 1 and hasOutput false when command is not found	process_runner.test.cjs	✅ Design	Error path: missing binary
collects combined stdout and stderr in output	process_runner.test.cjs	✅ Design	Combined stream contract
resolves with durationMs as a non-negative number	process_runner.test.cjs	✅ Design	Return type contract
truncates logArgs to 200 chars in spawn log	process_runner.test.cjs	✅ Design	Security/truncation contract

---

Flagged Tests — Requires Review

No tests flagged. All tests verify observable behavioral contracts using real process execution.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 19 tests (vitest)

No Go test files were added or modified in this PR.

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). The test suite uses real child process execution instead of mocks, covering both happy paths and error scenarios (non-zero exit codes, missing binaries, empty output, boundary values, arg redaction). No coding-guideline violations detected.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §25273924690

🧪 Test quality analysis by Test Quality Sentinel · ● 606.3K · ◷

[github-actions]

✅ Test Quality Sentinel: 94/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 19 tests verify behavioral contracts using real child process execution with solid error path and edge case coverage.

[Copilot]

@copilot review all comments

Addressed all three review comments in commit 9810f54:

1. sleep test flakiness — replaced the wall-clock Date.now() assertion with vi.useFakeTimers() + vi.advanceTimersByTime(1000), so the test is instantaneous and deterministic.

2. Fragile truncation regex — replaced the \S+-based parse (which breaks when execPath contains spaces) with a x+$ anchor that matches the trailing run of x characters directly (our longArg is all x's, so the truncated portion always ends the log line).

3. Double-resolution guard — added a settled boolean + settle() wrapper in runProcess so only the first terminal event (error or close) logs and resolves the promise. Subsequent events are silently dropped.