Skip to content

fix(ci): execute coherence tests and preserve runner failures - #704

Merged
proffesor-for-testing merged 4 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/coherence-ci-test-execution
Sep 22, 2026
Merged

proffesor-for-testing merged 4 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/coherence-ci-test-execution

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

CI could report success without completing its checks: the coherence command used a Vitest 4 option that prevents test collection, while the shared Vitest wrapper converted nonzero exits into success after seeing a passing test summary. The configured junit coverage reporter also failed after the tests passed.

Use the supported coherence worker flag and propagate test failures. Preserve every shared-runner exit, including timeouts, and use the same strict wrapper for MCP integration. Keep the separate JUnit test reporter and generate valid JSON, JSON summary and HTML coverage reports. Guard optional PR comments for fork tokens while retaining test jobs and report artifacts.

Verification

  • Final published head 8152bc57: all 27 checks pass. Full coverage job: 23,601 tests passed / 62 skipped; 975 files passed / six skipped. Coverage artifacts upload successfully; actual line coverage is 65.42%, below the existing advisory 80% comparison. No reporter errors or success normalization occur.

  • Exact published head 8152bc57: Coherence Verification passes 224 tests / nine files; strict MCP integration passes 59 tests / nine files, with no exit-status normalization.

  • Exact coherence workflow command: original script exits 1 with Unknown option --maxForks and collects no tests; corrected script passes 224 tests across nine files, exit 0. The existing memory/GC settings are retained.

  • Nine shell-wrapper regressions: unpatched 7 fail / 2 pass; patched 9 pass. They execute the actual wrapper with a controlled runner and cover successful argument forwarding, nonzero exits after a passing summary, failure before a summary, and a real timeout after all tests report passing.

  • Wrapper regressions plus existing fork-comment guard tests: 21 pass. The 12 reporting cases keep fork reporting jobs available while skipping unauthorized comments, retain same-repository comments, and prevent comments on non-PR events.

  • Actual Vitest 4.1.4 through the corrected wrapper: 18 tests pass, exit 0, with valid coverage-final.json, coverage-summary.json, HTML and test JUnit artifacts in a fresh temporary fixture. Coverage was restricted to the exercised formatter module for this report-generation check; this is not a full coverage run.

  • Negative report-generation control: the same 18 tests pass and JUnit reports zero failures, but an intentionally invalid coverage reporter still produces exit 1 through the wrapper.

  • Shell tests run with real Bash/GNU timeout where available. A stock-macOS PATH control explicitly skips the nine shell cases when GNU timeout is absent; evaluating the same prerequisite check for Linux confirms missing tools fail instead of skipping.

  • Actionlint passes for the coherence and MCP workflows with optional ShellCheck/Pyflakes integrations disabled. Shell syntax and whitespace checks pass. ESLint does not cover these paths under the existing configuration: tests are ignored and vitest.config.ts is outside its configured TypeScript project.

npm run test:safe -- tests/integrations/coherence/ tests/learning/coherence-integration.test.ts --reporter=verbose
npx vitest run tests/unit/scripts/ci-vitest-run.test.ts tests/unit/scripts/fork-pr-comments.test.ts --bail=0 --reporter=verbose

The full repository coverage job has now completed on this exact head, as recorded above.

Failure modes

  • Coherence tests never collect: the exact original command reproduces the unsupported flag; the corrected command runs the existing nine-file suite. Workflow and npm-script path filters now include changes to their own configuration.
  • A post-test failure becomes a green job: shell cases preserve exits 1, 2, 124, 137, 143 and 255 after a passing summary. The actual invalid-reporter control independently verifies a real post-test exit 1 is retained.
  • Timeout is mistaken for successful completion: the actual timeout regression must return 124 even after a passing summary. MCP integration uses that same wrapper and retains its 480-second limit; report/artifact steps remain available after failure.
  • Invalid or missing configured coverage output: the actual formatter coverage run verifies JSON, JSON summary, HTML and JUnit outputs. The JUnit test reporter remains separate from coverage reporting. This change does not alter or claim enforcement of the workflow's printed coverage threshold.
  • Fork comments fail despite completed checks: the 12 existing actual-YAML guard tests exercise fork, same-repository and non-PR cases. Generation, uploads and test failure semantics are unchanged.
  • Shell-only tests add an unsupported local prerequisite: capability qualification explicitly skips only off Linux when Bash/GNU timeout is missing. Linux treats missing prerequisites as a failure; both branches are checked as described above.

No production API, runner assertion, timing limit, or test exclusion is changed by this follow-up.


Required check (issue #401)

  • Every failure mode mentioned in this PR description has either (a) a test that exercises it, or (b) a linked tracking issue.

  • Affects published API or CLI surface: no; changes are development scripts, CI workflow behavior and test configuration.

  • Touches the init flow / npm-publish.yml / tests/fixtures/init-corpus/: no.

@proffesor-for-testing proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with a full reproduction: on main, npm run test:safe fails immediately on the unknown --maxForks flag and continue-on-error hid it, so the coherence job never executed a test; junit is not a valid istanbul coverage reporter, so coverage runs died after passing. This PR fixes both, the wrapper now propagates real exit codes, and all checks on the head are green with 224 coherence tests actually running. Codex adversarial pass: no findings. One known trade-off: the wrapper no longer masks the historical post-test vitest hang; if that returns we fix the handle leak rather than reinstating the mask.

@proffesor-for-testing
proffesor-for-testing merged commit fe3f4c5 into proffesor-for-testing:main Sep 22, 2026
31 checks passed
@proffesor-for-testing

Copy link
Copy Markdown
Owner

Thank you, @rudycelekli! This one caught a genuine false green: the coherence job had never actually run a test since the --maxForks flag stopped being valid, and the CI wrapper was separately converting post-test failures and timeouts into success. Both are fixed, the coverage summary the threshold step reads now really exists, and the fork-comment guards keep contributor PRs green. Merged. One follow-up we noticed for later, not part of this PR: coherence.yml still runs coherence-check.mjs with || true and defaults USED_FALLBACK to true, so a crash there is indistinguishable from a fallback pass.

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.

2 participants