Skip to content

ci: Phase 1 test quality — add reusable Node quality workflow#186

Open
lml2468 wants to merge 1 commit into
mainfrom
feat/test-quality-phase1
Open

ci: Phase 1 test quality — add reusable Node quality workflow#186
lml2468 wants to merge 1 commit into
mainfrom
feat/test-quality-phase1

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented Jun 1, 2026

Summary

Phase 1 test quality CI upgrade for octo-web.

Changes

  • New quality job: Calls Mininglamp-OSS/.github/.github/workflows/reusable-node-quality.yml@main
    • typecheck: tsc --noEmit
    • lint: pnpm lint
    • build: pnpm build
    • has-tests: false — no Vitest tests yet (Phase 2+)

What's NOT changed

  • Existing build job preserved (build + lint + i18n:check)
  • changes preflight job unchanged
  • Trigger conditions unchanged

Dependencies

Checklist

  • Reusable node quality workflow integration
  • has-tests set to false (Phase 2+)
  • Existing jobs preserved
  • YAML validated

- Add quality job calling reusable-node-quality.yml (typecheck + lint + build)
- has-tests: false (no Vitest tests yet, Phase 2+)
- Preserve existing build job (build + lint + i18n:check)
@lml2468 lml2468 requested a review from a team as a code owner June 1, 2026 08:15
@github-actions github-actions Bot added the size/S PR size: S label Jun 1, 2026
Copy link
Copy Markdown
Contributor

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

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

Summary: The PR is relevant to octo-web, but the new reusable quality job will fail for this monorepo as currently wired.

🔴 Blocking

🔴 Critical — .github/workflows/ci.yml:99: the called reusable Node workflow runs npx tsc --noEmit from the repository root by default, but this repo has no root tsconfig.json and no root typecheck script. Its TypeScript configs are package/app-level, so the new Quality / Type Check job will fail on every non-draft code PR or push. Fix by adding a repo-level typecheck entrypoint/config that covers the monorepo, or by extending the reusable workflow to accept a typecheck command and passing the correct command from this repo.

💬 Non-blocking

🟡 Warning — .github/workflows/ci.yml:103: has-tests: false does not match the current repo. There are Vitest tests and an app-level test script, for example apps/web/package.json:37, apps/web/src/__tests__/VoiceFeedback.test.ts:1, and packages/dmworkbase/src/Components/ClawInfoModal/__tests__/ClawInfoModal.test.tsx:1. If Phase 1 intentionally excludes tests, the PR description should not say there are no Vitest tests; otherwise this should be wired to run the existing test suite through an appropriate monorepo test script.

✅ Highlights

The new job preserves the existing changes gate and draft-PR condition from the existing build job, so the trigger behavior is consistent with the current CI design.

Copy link
Copy Markdown
Contributor

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

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

Code Review — PR #186 (octo-web)

Verdict: CHANGES_REQUESTED

This PR wires a new quality job into .github/workflows/ci.yml that calls the shared reusable-node-quality.yml. The intent is good, but as written the job will fail on every run, and it references a reusable workflow that does not yet exist on main. Both must be resolved before merge.

Blocking issues

P1 — The new typecheck step will fail on every PR (red CI)

The reusable workflow runs type checking as:

- name: Type check
  run: npx tsc --noEmit

at the repository root (default working-directory: '.'). octo-web is a Turborepo monorepo and has no root tsconfig.json (confirmed: tsconfig.json, tsconfig.base.json, and jsconfig.json all return 404 at the head SHA; tsconfigs only exist per-package under apps/* and packages/*).

When tsc --noEmit is invoked with no tsconfig and no input files in the working directory, it does not no-op — it prints the help text and exits non-zero:

$ npx tsc --noEmit   # typescript@5.9.3, empty dir
...help text...
EXIT=1

Result: the Quality / Type Check job goes red on every PR that touches code. If quality is added to required checks it blocks all merges; if not, it permanently pollutes CI status. The existing build job never ran tsc, so this is a new, always-failing gate rather than a no-op addition.

Fix options:

  • Add a repo-level typecheck script (e.g. turbo run typecheck) with per-package typecheck tasks, and have the reusable workflow call that script instead of bare npx tsc --noEmit; or
  • Point the reusable workflow at the right working directory / tsconfig path; or
  • Defer enabling the typecheck portion until the monorepo exposes a working root typecheck entry point.

This needs to be validated by actually running the workflow green once, not just by YAML validation.

P1 — Referenced reusable workflow is not on main yet

The job pins:

uses: Mininglamp-OSS/.github/.github/workflows/reusable-node-quality.yml@main

reusable-node-quality.yml does not exist on Mininglamp-OSS/.github@main. It currently lives only in Mininglamp-OSS/.github PR #55 (feat/test-quality-phase1), which is still OPEN / not merged. The PR body acknowledges this dependency, but merging #186 before #55 lands will make the quality job error immediately ("workflow not found"). The input contract itself is fine — package-manager, node-version, and has-tests all match the reusable's workflow_call.inputs — so once #55 merges, the wiring is correct.

Fix: Gate the merge order — land .github#55 first, confirm reusable-node-quality.yml is on main, then merge this. Worth stating explicitly in the merge checklist.

Non-blocking suggestions

P2 — @main is unpinned; inconsistent with this repo's supply-chain posture

Every third-party action in this ci.yml is pinned to a commit SHA with a version comment (e.g. actions/checkout@34e1148... # v4, pnpm/action-setup@b906aff... # v4.3.0). The new reusable workflow is referenced as a mutable @main. For a first-party org repo this is lower risk, but it breaks the repo's own convention and means any change to the reusable silently affects octo-web CI. Consider pinning to a tag/SHA (...@v1 or a commit) once #55 is released.

P2 — Duplication between build and quality

The existing build job already runs pnpm build + pnpm lint (plus i18n:check). The new quality job re-runs build + lint via the reusable. That's roughly double the CI minutes for the same checks, minus i18n. This is acceptable as a transitional Phase-1 step, but the intended end state (per the "Phase 2+" framing) should be to consolidate — otherwise the build and quality jobs overlap indefinitely. Worth a follow-up to fold the existing per-job steps into the reusable so there's a single source of truth.

Nit — has-tests: false is correct for now

Matches the reusable's default and the stated "no Vitest yet" status. The test job is correctly gated behind if: inputs.has-tests, so nothing runs prematurely. No action needed.

Summary

The structure and input contract are correct, but the PR cannot merge as-is: the typecheck step will fail against this monorepo layout (P1), and the reusable workflow it depends on is not yet on main (P1). Resolve the root-tsconfig/typecheck path, land .github#55 first, and ideally pin the reusable reference. Recommend validating with a green run before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants