ci: Phase 1 test quality — reusable Go + Node quality workflows

[lml2468]

Summary

Phase 1 of test quality CI improvements for the Mininglamp-OSS org.

New reusable workflows

reusable-go-quality.yml

• test jobs: Uses gotestsum instead of bare go test, producing JUnit XML + coverage.out
  • -race -shuffle=on flags for better test reliability
  • Supports MySQL + Redis service containers (optional, for repos like octo-server)
  • Per-package DB reset pattern preserved for service-dependent tests
  • Uploads artifacts (JUnit XML + coverage) on both success and failure
  • Writes PR summary to $GITHUB_STEP_SUMMARY
• diff-coverage job: Uses octocov-action for diff coverage with PR comments
  • Configurable overall threshold (default 60%) and core-package threshold (default 75%)
  • Excludes *_mock.go, *.pb.go, *_gen.go, vendor/, migrations/
• test-lint job: Detects time.Sleep in test files (warning, non-blocking)

reusable-node-quality.yml

• typecheck: tsc --noEmit
• lint: pnpm lint or npm run lint
• test (optional): pnpm test --coverage with vitest coverage report
• build: pnpm build or npm run build
• Supports both pnpm and npm package managers

Checklist

• reusable-go-quality.yml created with all specified inputs
• reusable-node-quality.yml created with all specified inputs
• YAML validation passed
• Follows existing reusable workflow conventions (see reusable-check-sprint.yml)

Related PRs

Individual repo CI upgrades will follow in separate PRs after this is merged.

[Jerry-Xin]

The PR is in scope for this repository, but the Go quality workflow has false-green coverage gates that should be fixed before merge.

🔴 Blocking

• 🔴 Critical — Core package coverage threshold is not enforced. In .github/workflows/reusable-go-quality.yml:369-380, packages below inputs.core-coverage-threshold only write a warning to $GITHUB_STEP_SUMMARY; the step never exits non-zero. That means callers can configure a required core coverage threshold and still get a successful workflow when it is violated.

• 🔴 Critical — coverage-exclude is computed but never applied to octocov. .github/workflows/reusable-go-quality.yml:313-321 builds EXCLUDES, but .github/workflows/reusable-go-quality.yml:323-337 never inserts it into .octocov.yml. As a result, *_mock.go, *.pb.go, generated files, vendor/, and migrations/ are still included despite the advertised input/defaults.

💬 Non-blocking

• 🟡 Warning — The core package coverage matching is likely unreliable for inputs like ./modules/user/.... .github/workflows/reusable-go-quality.yml:361-366 greps go tool cover -func output using the literal input path, but coverage output usually contains module/import-style file paths, not ./... patterns. The fallback also averages percentages rather than calculating weighted coverage.

• 🟡 Warning — In .github/workflows/reusable-node-quality.yml:52-64 and repeated jobs, working-directory is applied to shell commands but actions/setup-node cache has no cache-dependency-path. For subdirectory Node projects, setup-node may look for the lockfile at the repo root and fail or miss caching. Add a dependency path derived from inputs.working-directory.

✅ Highlights

• The PR adds relevant reusable workflows under .github/workflows, matching the repository’s purpose.
• The Go workflow’s split service/non-service jobs are a reasonable workaround for conditional service containers.
• Artifacts are uploaded with if: always(), which is useful for failed test debugging.

[yujiawei]

Code Review — PR #55 (.github)

Independent review of the two new reusable workflows (reusable-go-quality.yml, reusable-node-quality.yml). The intent is solid — gotestsum + JUnit/coverage artifacts, diff coverage via octocov, and a Node typecheck/lint/test/build pipeline are all reasonable additions for org-wide CI. However there are blocking correctness issues plus several deviations from this repo's established workflow conventions.

Verdict: CHANGES_REQUESTED.

1. Verification summary

• ⚠️ Repo's own actionlint status check is failing on this PR's head SHA — see P0-1.
• ❌ Core-package coverage threshold is advertised but never enforced — P1-1.
• ❌ coverage-exclude input is computed but never applied — P1-2.
• ❌ gotestsum tool merge is not a real command — silent fallback corrupts the summary/artifact — P1-3.
• ⚠️ Action references are unpinned (@v4/@v5), contrary to the repo-wide SHA-pinning convention — P2-1.
• ⚠️ Broad top-level permissions: instead of per-job least privilege — P2-2.

2. Findings

🔴 P0-1 — The repo's actionlint check fails on this PR (hard merge blocker)

The org runs workflow-sanity.yml (actionlint + shellcheck) on every PR touching .github/workflows/**, and it treats shellcheck findings as errors (exit 1). This PR's actionlint check is currently red:

• reusable-go-quality.yml:158 — SC2012: first_xml=$(ls junit-*.xml ... | head -1) — use find instead of ls for non-alphanumeric filenames.
• reusable-go-quality.yml:170-173 — SC2129: the four echo ... >> "$GITHUB_OUTPUT" lines should be grouped as { ...; } >> "$GITHUB_OUTPUT".
• reusable-go-quality.yml:250-252 — SC2129: same pattern in the standalone job.
• reusable-go-quality.yml:371 — SC2001: pkg_base=$(echo "$pkg" | sed 's|/\.\.\.$||') — use ${pkg%/...} / parameter expansion instead.

Because actionlint is a required-style gate here, the PR cannot pass CI until these are resolved. None are hard to fix, but they must be fixed.

🔴 P1-1 — Core-package coverage threshold is never enforced (false green)

reusable-go-quality.yml:375-386: when a core package is below core-coverage-threshold, the step only sets all_pass=false and appends a warning to $GITHUB_STEP_SUMMARY. It never exit 1s. A caller can configure a required core threshold, fall below it, and still get a green workflow. Either fail the step on all_pass=false, or document explicitly that this is advisory-only (the input name core-coverage-threshold strongly implies enforcement).

🔴 P1-2 — coverage-exclude is computed but dropped

reusable-go-quality.yml:320-327 builds $EXCLUDES from the input, but the .octocov.yml heredoc at lines 329-343 never references it. As a result the documented defaults (mock, pb.go, _gen.go, vendor, migrations) are silently ignored — generated/vendored files still count toward coverage, skewing the gate. The exclude list needs to be written into the octocov config (e.g. under coverage.paths/exclude, per octocov's schema), and verify the generated YAML is valid (note $EXCLUDES is assembled with literal \n, which won't expand inside a quoted heredoc).

🔴 P1-3 — gotestsum tool merge does not exist

reusable-go-quality.yml:156: gotestsum tool merge junit-*.xml > junit.xml. gotestsum tool only supports slowest and ci-matrix — there is no merge subcommand (verified against gotestsum's CLI). The call always fails, silently triggers the fallback at lines 157-160, and junit.xml ends up containing only the first package's results. Downstream effects:

• The PR summary counts (lines 165-167, parsed from junit.xml with head -1) reflect a single package, not the whole run — misleading "Passed/Failed" numbers.
• The uploaded merged junit.xml artifact is incomplete (the per-package junit-*.xml are still uploaded, so this is recoverable but the named merged file is wrong).

The pass/fail gate itself is safe (it uses the accumulated $fail, not the XML), so this is functional-but-wrong rather than a gate bypass. Replace with a real merge tool (e.g. junit-merge, or have gotestsum write a single junit by running once over ./... where DB-reset isn't needed) or parse counts by summing across junit-*.xml.

🟡 P2-1 — Actions are unpinned, against repo convention

Every existing workflow in this repo SHA-pins actions with a version comment (e.g. actions/checkout@de0fac2e... # v6.0.2, CodeQL/labeler/stale all pinned). This PR uses floating tags: actions/checkout@v4, actions/setup-go@v5, actions/setup-node@v4, pnpm/action-setup@v4, actions/upload-artifact@v4, actions/download-artifact@v4, plus third-party k1LoW/octocov-action@v1 and davelosert/vitest-coverage-report-action@v2. For reusable workflows consumed org-wide this is a supply-chain regression — please pin to commit SHAs to match the rest of the repo. Likewise go install gotest.tools/gotestsum@latest (lines 100, 231) is non-reproducible; pin a version.

🟡 P2-2 — Top-level permissions: is broader than needed

Both files set permissions: { contents: read, pull-requests: write } at the top, granting pull-requests: write to every job — including the Go/Node test, typecheck, lint, and build jobs that only need contents: read. The repo convention is top-level permissions: {} with per-job least privilege (see reusable-codeql.yml, reusable-stale.yml). Only the octocov diff-coverage job actually needs PR write. Please scope pull-requests: write to that job.

🟡 P2-3 — Core-package coverage matching is unreliable

reusable-go-quality.yml:367-372: it greps go tool cover -func output with the literal input (./modules/user/...), but cover output uses module/import-style file paths, so the primary match will usually miss and fall through to the broad fallback, which averages per-file percentages rather than computing statement-weighted coverage. Even once P1-1 is fixed to enforce, the number being compared may be inaccurate. Consider computing weighted coverage from the profile.

🟡 P2-4 — Node setup-node cache may miss for subdir projects

reusable-node-quality.yml: working-directory is applied to the run steps, but actions/setup-node has no cache-dependency-path. For a working-directory other than ., setup-node resolves the lockfile from the repo root and will either fail to cache or warn. Pass cache-dependency-path: ${{ inputs.working-directory }}/<lockfile>.

🟢 Nits / maintainability

• reusable-node-quality.yml:581 hardcodes davelosert/vitest-coverage-report-action, which assumes vitest output; projects on jest/other runners with has-tests: true will break. Worth documenting the vitest assumption.
• pnpm test --coverage (line 574) relies on pnpm forwarding the flag to the underlying script; confirm behavior, or use pnpm test -- --coverage for parity with the npm branch.

3. Recommendation

Address the P0 (fix the four shellcheck findings so actionlint goes green) and the three P1 correctness bugs (enforce or relabel core threshold, actually apply coverage-exclude, fix the non-existent gotestsum tool merge). The P2 convention items (SHA-pin actions, scope permissions per-job) should be fixed before these reusable workflows are adopted org-wide, since every consuming repo inherits them.

4. Additional notes

• The split test-with-services / test-standalone jobs plus the always() && (...success) gate on diff-coverage are correct given GitHub Actions has no conditional services: block — good workaround.
• Uploading artifacts with if: always() for failure debugging is a nice touch.

[lml2468]

🔧 Review Feedback Addressed — 8a6c383

感谢 @Jerry-Xin @yujiawei 的 review！以下是逐项修复说明：

---

P0：shellcheck / actionlint 修复（CI 红）

Issue	修复方式
SC2012 — ls junit-*.xml | head -1	已删除，JUnit 合并改用 Python 脚本（见 P1-3），不再需要 ls fallback
SC2129 — 多个 echo >> "$GITHUB_OUTPUT"	合并为 { ... } >> "$GITHUB_OUTPUT" 分组写入（test-with-services 和 test-standalone 两处）
SC2001 — sed 's|/\.\.\.\$||'	改用 bash 参数展开 pkg_base="${pkg%/...}"

P1-1：core package 覆盖率门槛 enforce

当 all_pass=false 时现在会 exit 1，并在 Step Summary 输出 ❌ 提示。

P1-2：coverage-exclude 写入 octocov 配置

重写了 .octocov.yml 生成逻辑：

• 不再使用 heredoc（避免变量展开问题）
• 改用 { echo ... } > .octocov.yml 逐行拼接
• inputs.coverage-exclude 的 glob 片段现在会生成 coverage.exclude 列表
• 目录型 pattern（以 / 结尾）→ "**/dir**"，文件型 → "**/*suffix"

P1-3：gotestsum tool merge → Python XML merger

gotestsum 没有 merge 子命令。改用内联 Python3 脚本（ubuntu-latest 自带）：

• 合并所有 junit-*.xml 为 junit.xml
• 自动 recompute tests/failures/skipped 汇总属性
• 无额外依赖安装

P2-1：所有 action 已 SHA pin

Action	SHA	Tag
actions/checkout	34e11487...	v4
actions/setup-go	40f1582b...	v5
actions/setup-node	49933ea5...	v4
pnpm/action-setup	b906affc...	v4
actions/upload-artifact	ea165f8d...	v4
actions/download-artifact	d3f86a10...	v4
k1LoW/octocov-action	b3b6ee60...	v1
davelosert/vitest-coverage-report-action	02f3c2e6...	v2

P2-2：permissions 最小化

• 顶层 permissions: {} （两个文件）
• 各 job 按需声明：
  • test-with-services / test-standalone / test-lint / build → contents: read
  • diff-coverage (Go) / test (Node, 需要 coverage report) → contents: read + pull-requests: write

---

请 re-review，谢谢！🙏

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/.github, but two workflow correctness issues need fixing before merge.

🔴 Blocking

• 🔴 Critical — Core package coverage checks do not work with the documented ./modules/user/... input. In .github/workflows/reusable-go-quality.yml:405-410, the script greps go tool cover -func=coverage.out output using the raw input path. Go coverage output uses module import paths like example.com/repo/modules/user/file.go, so grep "^./modules/user/..." and fallback grep "./modules/user" will not match. Any caller setting core-packages: './modules/user/...' will likely get 0 coverage and fail incorrectly. Normalize ./foo/... into a path fragment such as /foo/ or compute coverage directly from coverage.out filenames.

• 🔴 Critical — working-directory breaks Node dependency caching for subdirectory projects. In .github/workflows/reusable-node-quality.yml:52-64 and repeated in the other jobs, defaults.run.working-directory only affects shell steps, not actions/setup-node. With cache: pnpm or cache: npm, setup-node still looks for the lockfile at the repository root unless cache-dependency-path is set. A valid caller using working-directory: frontend will fail before install if the lockfile is under frontend/. Add cache-dependency-path: ${{ inputs.working-directory }}/pnpm-lock.yaml or the npm equivalent, preferably with separate setup steps per package manager.

💬 Non-blocking

• 🟡 Warning — .github/workflows/reusable-go-quality.yml:185-193 stores total tests as test_passed, so summaries can show “passed” equal to total tests even when failures or skips exist. Compute passed as tests - failures - skipped.

• 🟡 Warning — .github/workflows/reusable-go-quality.yml:93-94 installs gotestsum@latest, which makes the org-wide reusable workflow sensitive to upstream releases. Pin a known version.

✅ Highlights

• Actions are pinned by SHA.
• The workflow keeps artifact upload under if: always().
• The relevance and structure match the existing reusable workflow approach in this repository.

[lml2468]

Review: .github PR #55 (re-review)

Verdict: CHANGES_REQUESTED
Commit: 8a6c383
Previous: d473d27 (CI red + 4 P0 blocking)

Previous Blocking Items — Resolution Status

• ✅ Fixed — actionlint CI failure: current checks show actionlint passing at the new head SHA.
• ✅ Fixed — core coverage threshold enforcement: the core coverage step now exits non-zero when any configured core package is below threshold (reusable-go-quality.yml:421-424).
• ⚠️ Partially fixed — coverage-exclude is now written into .octocov.yml (reusable-go-quality.yml:369-371), but the generated defaults still do not exclude common directory/fragments correctly. See new finding P1 below.
• ✅ Fixed — nonexistent gotestsum merge: the old command was replaced with a Python JUnit merge block (reusable-go-quality.yml:148-180).
• ❌ Still broken — core package matching remains unreliable and non-weighted for path inputs like ./modules/user/.... See new finding P1 below.
• ❌ Still broken — Node workflow cache still has no cache-dependency-path for subdirectory projects. See new finding P1 below.
• ✅ Fixed — uses: action references are pinned to commit SHAs.
• ⚠️ Partially fixed — top-level permissions are now {} and most jobs are scoped, but the Node test job still grants pull-requests: write to dependency install and test-script execution. See security finding P1 below.

New Findings

• [P1] .github/workflows/reusable-go-quality.yml:350 and .github/workflows/reusable-node-quality.yml:68 — Workflow inputs are still interpolated directly inside shell run: blocks. Examples include inputs.coverage-exclude, inputs.core-packages, inputs.core-coverage-threshold, and every Node inputs.package-manager branch. Per the review requirement, all ${{ inputs.* }} used by shell scripts must be passed through env: and read as normal shell variables. Direct interpolation can turn a crafted input containing shell syntax such as command substitution into executable script content before Bash runs.
• [P1] .github/workflows/reusable-node-quality.yml:61 — The setup-node cache still does not set cache-dependency-path, repeated in each Node job. With working-directory set to a subdirectory, setup-node will look for the lockfile at the repository root, so caching can miss or fail for the main intended reusable-workflow use case.
• [P1] .github/workflows/reusable-node-quality.yml:120 — The test job grants pull-requests: write before running pnpm install, npm ci, and project test scripts. That gives arbitrary repository test code the same write-scoped token needed only by the later coverage-comment action. Split coverage reporting into a separate job/step boundary that does not execute project scripts, or keep this job at contents: read.
• [P1] .github/workflows/reusable-go-quality.yml:405 — Core package coverage matching is still regex-based against the raw input (grep "^${pkg}") and the fallback averages function/file percentages instead of calculating statement-weighted coverage from the profile. For inputs like ./modules/user/..., the primary match generally misses, and the fallback number can be materially wrong while now enforcing exit 1.
• [P1] .github/workflows/reusable-go-quality.yml:356 — coverage-exclude is injected, but the default fragments generate ineffective globs for directories and broad fragments. For example, vendor becomes **/*vendor, which does not match files under vendor/, and mock only matches paths ending exactly in mock. The advertised defaults should either be real octocov globs (**/vendor/**, **/migrations/**, **/*mock*, etc.) or the input should require callers to pass complete glob patterns without this lossy conversion.
• [P2] .github/workflows/reusable-go-quality.yml:94 — go install gotest.tools/gotestsum@latest remains non-reproducible in an org-wide reusable workflow. This is not an action pinning violation, but it is still a moving tool dependency and should be pinned to a known gotestsum version.

Security

The SHA pinning of uses: actions is fixed, and the top-level workflow permissions are much better. The remaining security blockers are direct ${{ inputs.* }} interpolation in shell scripts and pull-requests: write being available while the Node job runs dependency lifecycle scripts and project tests. Secrets are not newly exposed by this diff, but the token scope should be reduced before merging reusable workflows intended for organization-wide consumption.

[yujiawei]

Code Review — PR #55 (.github)

Independent re-review of the two new reusable workflows (reusable-go-quality.yml, reusable-node-quality.yml) at head 8a6c3837. The previous round's blockers were addressed well: actionlint is green again, all actions are SHA-pinned, top-level permissions: {} with per-job scoping is in place, the non-existent gotestsum tool merge was replaced with a real Python merge that correctly aggregates counts, the core-threshold step now exit 1s, and coverage-exclude is now written into .octocov.yml. Nice work on those.

Two correctness gaps remain that are worth one more cycle before this is adopted org-wide, since every consuming repo inherits them.

Verdict: CHANGES_REQUESTED.

1. Verification summary

• ✅ actionlint / workflow-sanity checks pass on the head SHA.
• ✅ Python JUnit merge aggregates tests/failures/skipped across all per-package suites correctly; heredoc indentation is valid (no IndentationError); the head -1 parse now reads the aggregated root, not the first package. Prior P1 fixed.
• ✅ Actions SHA-pinned, permissions scoped per-job. Prior P2s fixed.
• ❌ coverage-exclude is now wired into octocov, but the generated glob is wrong for 3 of the 5 advertised defaults — P1-1.
• ⚠️ Core-package coverage check now gates (exit 1) on a number derived from unreliable matching — P1-2 (opt-in).
• 🟡 gotestsum@latest, Node cache-dependency-path, vitest-only coverage action — P2/nits.

2. Findings

🔴 P1-1 — coverage-exclude default patterns don't exclude mock / vendor / migrations

reusable-go-quality.yml:354-359: each non-/-terminated fragment p is turned into the glob **/*<p>. octocov's coverage.exclude uses doublestar semantics where * does not cross /, so **/*<p> only matches when the final path segment ends with the literal suffix <p>.

Against the documented default coverage-exclude: 'mock,pb.go,_gen.go,vendor,migrations':

Fragment	Generated pattern	Intended target	Result
mock	**/*mock	internal/mocks/user_mock.go	❌ last segment ends .go, not mock
pb.go	**/*pb.go	api/foo.pb.go	✅ true filename suffix
_gen.go	**/*_gen.go	x/y_gen.go	✅ true filename suffix
vendor	**/*vendor	vendor/x/y.go	❌ last segment y.go
migrations	**/*migrations	db/migrations/001_init.go	❌ last segment 001_init.go

So only the two genuine filename suffixes work; the directory-style fragments (vendor, migrations) and the most impactful one — generated *_mock.go mocks — are silently not excluded. octocov matches both git-root-relative paths and Go module paths, but in both cases the trailing segment is the filename, so the conclusion is the same.

Practical impact: the PR description lists these excludes as a delivered feature, but generated mock files (typically 0% coverage) still count toward the gate, skewing overall coverage downward and making coverage.acceptable more likely to false-RED for adopting repos. (vendor is low-impact in practice since go test ./... already skips vendor/, but mock and migrations matter.)

Suggested fix — distinguish filename-suffix fragments from directory fragments, e.g.:

• directory names → **/<name>/** (and/or <name>/**)
• bare tokens meant to match filenames → **/*<token>*.go (so mock → **/*mock*.go)
• keep explicit .go suffixes (pb.go, _gen.go) as **/*<suffix>

Whatever the mapping, please add a comment documenting the expected fragment forms and verify the generated .octocov.yml actually drops the intended files.

🔴 P1-2 — Core-package coverage check gates on an unreliable number (opt-in path)

reusable-go-quality.yml:405-425: the primary match grep "^${pkg}" uses the raw input pattern (e.g. ./modules/user/...), but go tool cover -func output lines start with module/import-style file paths, so ^./modules/... never matches and execution always falls through to the broader branch. The fallback grep "${pkg_base}" then averages per-function percentages (sum+=$NF; n++ … sum/n) rather than computing statement-weighted coverage. Now that this step does exit 1 (good — that was the prior fix), it is gating on an inaccurate figure, which can both false-pass and false-fail the core gate.

This is gated behind core-packages (default ''), so it only bites callers who opt in — but those are exactly the repos that asked for a stricter core gate, so the number needs to be trustworthy. Consider computing weighted coverage from the profile (e.g. parse coverage.out statement counts per package, or use octocov's own per-path reporting) instead of averaging -func rows. Also note grep "${pkg_base}" treats . as a regex wildcard; anchor/escape it.

3. Recommendation

Address P1-1 (the exclude defaults are the headline feature and 3/5 silently don't work) and P1-2 (don't enforce on an inaccurate core-coverage number). Both are small, localized changes. The P2 items below are non-blocking but worth folding in since this is consumed org-wide.

4. Non-blocking (P2 / nits)

• 🟡 reusable-go-quality.yml:94,255 — go install gotest.tools/gotestsum@latest is non-reproducible; pin a version to match the repo's SHA-pinning convention for the rest of the toolchain.
• 🟡 reusable-node-quality.yml — actions/setup-node enables cache: but has no cache-dependency-path. For working-directory != '.', setup-node resolves the lockfile from the repo root and will warn or miss the cache. Pass cache-dependency-path: ${{ inputs.working-directory }}/<lockfile>.
• 🟢 reusable-node-quality.yml coverage step hardcodes davelosert/vitest-coverage-report-action; projects using jest/other runners with has-tests: true will not get a report. Worth documenting the vitest assumption.
• 🟢 pnpm test --coverage relies on pnpm forwarding the flag to the script; pnpm test -- --coverage would be more robust and match the npm branch.

5. Additional notes

• The split test-with-services / test-standalone jobs plus the always() && (…success) gate on diff-coverage remain a correct workaround for GitHub Actions' lack of conditional services: blocks.
• Per-package DB reset (DROP/CREATE + FLUSHALL) before each package is a sound isolation pattern for the service-backed runs.
• Uploading artifacts with if: always() is a good touch for failure debugging.

[lml2468]

🔧 Review 修复完成 (commit ad5d63b)

已一次性修复所有 Blocking / Warning / Nit 问题：

Blocking 修复

ID	问题	修复方案
B2	go list 假绿 — 进程替换失败被静默吞掉	改为先写入临时文件，检查 go list 退出码和文件非空，再循环读取
B3	core packages coverage 用 bash 解析 go tool cover 输出不准确	完全重写为 Python 脚本，直接解析 coverage.out 的 stmts/count，用 go list 展开 pattern 到 import paths
B4	缺少 quality-gate job，branch protection 无稳定 check name	Go 和 Node workflow 各添加 quality-gate job（if: always()），聚合所有子 job 结果
B5	Node workflow 中 ${{ inputs.package-manager }} 直接注入 shell	全部改为先设 env: PACKAGE_MANAGER，shell 里读 $PACKAGE_MANAGER，并加 allowlist 校验

Warning 修复

ID	问题	修复方案
W1	fork PR 的 octocov comment 会因权限不足报错	添加 fork PR 检测逻辑，fork 时自动 comment.enable: false
W2	coverage-exclude glob 模式不够准确	每个 pattern 同时生成目录 glob **/${p}/** 和文件后缀 glob **/*${p}
W9	gotestsum@latest 不确定性	固定为 gotestsum@v1.12.0
W12	Node setup-node 缺少 cache-dependency-path	所有 4 个 job 的 setup-node 都加上 pnpm-lock.yaml 和 package-lock.json 路径

Nit 修复

ID	问题	修复方案
Nit-1	Go summary "Passed" 数字实为总测试数	改为 total_pass = total_tests - total_fail - total_skip
Nit-3	core coverage 表格缺少 header	Python 脚本中写入完整 Markdown 表头
Nit-4	硬编码 OCTO_MASTER_KEY	已删除，应由调用方通过 secrets 传入

额外改动

• Go workflow 的 Generate octocov config step 也改用 env 变量读取 coverage-exclude 和 coverage-threshold，避免 inputs 直接注入 shell

[Jerry-Xin]

The PR is relevant to this .github repository, but the Go coverage workflow has a blocking correctness issue in the generated octocov configuration.

🔴 Blocking

• 🔴 Critical: .github/workflows/reusable-go-quality.yml:397-.github/workflows/reusable-go-quality.yml:404 generates unsupported octocov keys. diff.coverage.acceptable, comment.enable, and summary.enable are not valid octocov config fields, so the advertised diff coverage gate is effectively not enforced. The fork-PR comment disable logic at .github/workflows/reusable-go-quality.yml:363-.github/workflows/reusable-go-quality.yml:372 also does not actually disable comments because it writes comment.enable instead of using octocov’s supported conditional field. This means the reusable workflow can pass without enforcing the PR’s claimed diff coverage behavior, and may still attempt PR comments when it intended not to.

💬 Non-blocking

• 🟡 Warning: .github/workflows/reusable-go-quality.yml:44-.github/workflows/reusable-go-quality.yml:48 says mocks are excluded, but .github/workflows/reusable-go-quality.yml:381-.github/workflows/reusable-go-quality.yml:383 turns mock into **/*mock, which does not match normal Go mock files like foo_mock.go. Consider defaulting to _mock.go or explicitly generating **/*_mock.go.

• 🔵 Suggestion: .github/workflows/reusable-node-quality.yml:83-.github/workflows/reusable-node-quality.yml:84 always uses npx tsc --noEmit, even for pnpm projects. Using pnpm exec tsc --noEmit for pnpm and npm exec tsc -- --noEmit for npm would better match the selected package manager.

✅ Highlights

• The workflows are correctly scoped to reusable workflow_call use and align with this repository’s purpose.
• The Go workflow consistently uploads test artifacts with if: always(), which is useful for failed test diagnosis.
• The quality-gate jobs provide stable required-check names for downstream branch protection.

[lml2468]

Review: .github PR #55 (R3 re-review)

Verdict: CHANGES_REQUESTED
Commit: ad5d63b
Previous rounds: d473d27 (R1, CI red) → 8a6c383 (R2, 6 P1)

Previous Findings — Resolution Status

1. ⚠️ Partially fixed — Go shell input interpolation hardening

   • Fixed at .github/workflows/reusable-go-quality.yml:358-418: coverage-exclude, coverage-threshold, core-packages, and core-coverage-threshold are now passed through env: and read via environment variables in the shell.
   • Still present at .github/workflows/reusable-go-quality.yml:549-556: the quality-gate shell step still directly interpolates ${{ inputs.services }} in if [ "${{ inputs.services }}" = "true" ]; then.
   • This boolean input has lower exploitability than the prior string inputs, but it does not satisfy the previous hardening rule that all inputs.* used inside run: blocks go through env:.

2. ✅ Fixed — Node shell input interpolation hardening

   • .github/workflows/reusable-node-quality.yml:69-81, 111-133, 162-184, and 217-239 now pass package-manager through env: PACKAGE_MANAGER before shell use.
   • I did not find direct ${{ inputs.* }} interpolation inside Node run: script bodies.

3. ✅ Fixed — setup-node cache dependency path for subdirectory projects

   • cache-dependency-path is now set in all setup-node uses: .github/workflows/reusable-node-quality.yml:65-67, 107-109, 158-160, and 213-215.
   • This should allow callers using working-directory to point setup-node at subdirectory lockfiles.

4. ❌ Still broken — Node test job still grants PR write permission to dependency install and test execution

   • .github/workflows/reusable-node-quality.yml:136-142 still gives the entire test job pull-requests: write.
   • The same job then runs checkout, dependency install, and tests at .github/workflows/reusable-node-quality.yml:147-184 before the coverage reporter action at .github/workflows/reusable-node-quality.yml:186-190.
   • Job permissions apply to every step in the job, so this is not isolated. The untrusted install/test path still runs in a job with PR-write scope. Move the coverage-reporting/commenting step into a separate least-privileged job, or otherwise ensure install/test execute without pull-requests: write.

5. ✅ Fixed — Core package coverage matching and weighting

   • .github/workflows/reusable-go-quality.yml:429-444 now expands configured package patterns through go list -f "{{.ImportPath}}".
   • .github/workflows/reusable-go-quality.yml:449-473 parses coverage.out profile entries by import path and accumulates statement counts.
   • .github/workflows/reusable-go-quality.yml:488-498 computes weighted coverage and fails the step when it is below threshold.

6. ✅ Fixed — coverage-exclude directory glob handling

   • .github/workflows/reusable-go-quality.yml:374-384 now emits directory globs as **/<fragment>/** in addition to filename suffix globs.
   • Directory fragments such as vendor and migrations no longer become ineffective **/*vendor / **/*migrations-only patterns.

New Findings

• None found at P0/P1 beyond the unresolved previous findings above.

Security Assessment

The PR is closer, but I cannot approve this round because the Node workflow still grants pull-requests: write to the same job that runs dependency install and test commands. That keeps the prior privilege-isolation issue open for same-repository PRs and any caller configuration where PR-write scope is available. The remaining Go inputs.services direct interpolation is lower risk because the input is typed boolean, but it is still a missed instance of the requested shell-hardening pattern.

[yujiawei]

Code Review — PR #55 (.github)

Third-round review of the two reusable workflows (reusable-go-quality.yml, reusable-node-quality.yml) at head ad5d63b. Most of the prior blockers are genuinely resolved this round — nice work. One advertised feature is still partially broken, which is worth one more cycle since these workflows are inherited org-wide.

Verdict: CHANGES_REQUESTED.

1. Verification summary (prior findings)

• ✅ actionlint / no-tabs / workflow-sanity checks are green on this head SHA.
• ✅ Non-existent gotestsum tool merge replaced with a Python JUnit merge that aggregates tests/failures/skipped across all per-package suites correctly.
• ✅ Actions SHA-pinned; gotestsum pinned to v1.12.0.
• ✅ Top-level permissions: {} with per-job least privilege.
• ✅ User-controllable string inputs (coverage-exclude, core-packages, package-manager) are now passed via env: and read as shell variables, not interpolated into run: blocks.
• ✅ Core-package coverage check (prior P1) is now correct — rewritten in Python to expand patterns via go list -f '{{.ImportPath}}' and compute statement-weighted coverage by parsing coverage.out directly (no more grep on -func output, no more percentage averaging). Verified the derived import path matches go list output exactly and the parse is crash-safe.
• ✅ Node setup-node now sets cache-dependency-path for pnpm-lock.yaml / package-lock.json in all jobs (resolves the prior subdirectory-cache blocker).
• ✅ coverage-exclude for vendor/, migrations/, *.pb.go, *_gen.go now work via the newly-added **/<p>/** directory glob + **/*<suffix> form.
• ❌ coverage-exclude for mock still does not exclude *_mock.go — see P1 below.

2. Findings

🔴 P1 — mock default in coverage-exclude does not exclude *_mock.go (advertised-but-broken)

reusable-go-quality.yml (Generate octocov config step): for each fragment p, the script emits **/<p>/** and **/*<p>. octocov uses doublestar semantics where * does not cross /. For the default fragment mock this produces:

Generated glob	Matches internal/mocks/user_mock.go?	Why
**/mock/**	❌	requires a path segment literally named mock; real mock dirs are mocks/ (plural), and the file itself is not a directory segment
**/*mock	❌	requires the final segment to end in the literal mock; user_mock.go ends in .go

So generated mock files (typically 0% coverage) still count toward the coverage report. The PR description explicitly lists *_mock.go as excluded, so this is a functional + documentation mismatch. The other four defaults (vendor, migrations, pb.go, _gen.go) are now correct — only mock is broken.

Note this does not fail-open a gate (under-excluding inflates the denominator, making thresholds equal-or-harder, never silently passing), so it is P1, not P0. But for org-wide infra it can cause a coverage gate to false-RED for repos with many generated mocks, which is exactly the friction these excludes are meant to remove.

Fix: distinguish filename-suffix fragments from bare tokens, e.g. map mock → **/*mock*.go (or accept that callers must pass complete glob patterns). Keep .go-suffixed fragments (pb.go, _gen.go) as **/*<suffix> and directory names as **/<name>/**. Please verify the generated .octocov.yml actually drops the intended files and add a short comment documenting the expected fragment forms.

3. Non-blocking (P2 / nits)

• 🟡 Core-coverage fail-open — reusable-go-quality.yml core-packages step exits 0 with only a warning when go list resolves nothing or total_stmts == 0. A typo'd core-packages value silently disables the gate rather than failing. Consider failing (or at least surfacing a louder signal) when core-packages is non-empty but resolves to zero statements.
• 🟡 Node test job token scope — the test job holds pull-requests: write while running pnpm install / pnpm test --coverage (arbitrary project scripts) before the coverage-comment action. The token is PR-write only and GitHub auto-downgrades it to read-only for fork PRs, so this is defense-in-depth, not a hole. Splitting the coverage-comment step into a job that doesn't execute project scripts would be cleaner.
• 🟢 reusable-node-quality.yml hardcodes davelosert/vitest-coverage-report-action; projects on jest/other runners with has-tests: true won't get a report. Worth documenting the vitest assumption.
• 🟢 pnpm test --coverage relies on pnpm forwarding the flag to the script; pnpm test -- --coverage would match the npm branch and be more robust.

4. Additional notes

• The split test-with-services / test-standalone jobs + the always() && (…success) gate on diff-coverage remain a correct workaround for GitHub Actions' lack of conditional services: blocks. The shared test-results artifact name is safe because the two jobs are mutually exclusive via if:.
• Per-package DB reset (DROP/CREATE + FLUSHALL) is a sound isolation pattern for the service-backed runs.
• quality-gate aggregation and artifact upload under if: always() are good touches.

This is close — the single remaining item is a one-line change to the mock default.

[Jerry-Xin]

The PR is in scope for this .github repository, but one workflow failure mode should be fixed before merge.

🔴 Blocking

• 🔴 Critical: Fork PRs with Node tests can fail the required quality gate even when typecheck/lint/test/build pass. In .github/workflows/reusable-node-quality.yml, report-coverage runs on every pull_request when has-tests is true and requests pull-requests: write at line 202. For forked PRs, the token is read-only, so the Vitest coverage comment/report action can fail with insufficient permissions. Because quality-gate treats report-coverage failures as blocking at lines 305-308, external contributor PRs can be blocked by reporting rather than code quality. The Go workflow already handles this pattern by disabling comments for forks; the Node workflow needs the same fork guard, or the coverage report job should be skipped/allowed to succeed for forks.

💬 Non-blocking

• 🟡 Warning: In .github/workflows/reusable-go-quality.yml, invalid core-packages patterns are silently ignored, and if no packages match the job exits successfully at lines 453-455. That makes a typo disable the intended core coverage gate. Consider failing when a non-empty core-packages input matches nothing.

✅ Highlights

• Actions are pinned by SHA and permissions are scoped per job.
• The Go workflow preserves artifact upload on failure and provides a stable quality-gate check.
• The split Go service/non-service jobs are a practical workaround for conditional service containers.

[lml2468]

Summary since last review:

• Go workflow: the quality-gate shell now reads inputs.services, needs.*.result, and github.event_name through environment variables instead of direct expression interpolation. The coverage-exclude glob handling was also refined for Go suffixes, bare tokens, and directory fragments.
• Node workflow: test execution now runs with contents: read only, uploads coverage as an artifact, and a separate report-coverage job owns the pull-requests: write permission for the PR coverage comment. The Node quality gate now includes that reporting job and reads needs results through environment variables.

Previous findings:

• Resolved: [P1] reusable-go-quality.yml quality-gate no longer directly interpolates inputs.services or needs.* expressions inside the shell logic.
• Resolved: [P1] reusable-node-quality.yml no longer grants pull-requests: write to the install/test job; the write permission is isolated to the reporter job after test artifacts are produced.

New findings:

• None blocking. I rechecked run-block expression usage, job permission scopes, pinned third-party actions, and the new artifact handoff. The remaining ${{ }} usages in shell steps are either fixed GitHub metadata, numeric/script-produced outputs, or environment assignments rather than untrusted shell syntax construction.

CI status:

• No tabs in workflow files: pass
• actionlint: pass
• code-review: pending at review time

Final verdict:

• No blocking findings. The prior blockers are resolved and I did not find new blocking security, correctness, or maintainability issues in the incremental changes.

[yujiawei]

Code Review — PR #55 (.github)

Fourth-round review of the two reusable workflows (reusable-go-quality.yml, reusable-node-quality.yml) at head fd96a89. The two items targeted by this round are genuinely fixed — nice work — but two correctness gaps remain, and one of them (an invalid octocov config) was flagged last round and is still present. Since every consuming repo inherits these workflows, the advertised gates need to actually fire before merge.

Verdict: CHANGES_REQUESTED.

1. Verification summary

• ✅ Fixed — mock exclude default now works. reusable-go-quality.yml "Generate octocov config" step maps the bare token mock to both **/mock/** and **/*mock*.go, so generated files like internal/mocks/user_mock.go are now excluded. The three-way fragment branch (*.go suffix / bare token / dotted name) is correct and documented.
• ✅ Fixed — Node privilege isolation. The test job is now contents: read only, and coverage commenting moved to a dedicated report-coverage job (pull-requests: write) that runs only checkout + download-artifact + the pinned vitest-coverage-report-action — no project install/test/build. Untrusted project scripts no longer execute with a PR-write token.
• ✅ actionlint / no-tabs / workflow-sanity checks are green on this head SHA; all uses: are SHA-pinned; gotestsum pinned to v1.12.0; shell inputs read via env:.
• ❌ Still broken — .octocov.yml uses invalid config keys, so the diff-coverage threshold and the fork comment-disable are silently no-ops — P1-1 (flagged in R3, not addressed).
• ❌ New / unresolved — the core-package coverage gate fails open on its two most likely misconfigurations — P1-2.

2. Findings

🔴 P1-1 — Generated .octocov.yml uses unsupported keys → diff-coverage threshold is not enforced

reusable-go-quality.yml, "Generate octocov config" step (the heredoc that writes .octocov.yml, ~lines 400-419):

diff:
  coverage:
    acceptable: ${COVERAGE_THRESHOLD}%   # ❌ diff.coverage.acceptable is not a valid octocov key
comment:
  enable: $COMMENT_ENABLE                # ❌ comment.enable does not exist
summary:
  enable: true                           # ❌ summary.enable does not exist

Verified against octocov's config schema (k1LoW/octocov, config/config.go):

• Coverage supports acceptable — so coverage.acceptable: 60% (line ~406) is valid and the overall-coverage gate works.
• Comment supports hideFooterLink, deletePrevious, updatePrevious, message, if — no enable. Conditional commenting uses comment.if:.
• Summary supports hideFooterLink, message, if — no enable.
• Diff supports path, datastores, if — no coverage subkey. Diff/PR-coverage thresholds are expressed inside coverage.acceptable via the diff expression variable, e.g. acceptable: "current >= 60% && diff >= 0.5%".

octocov unmarshals its config without strict mode, so these unknown keys are silently ignored rather than erroring — the job stays green. Concrete impact:

1. The diff-coverage threshold is not applied. The "Diff Coverage" job only enforces the simple overall coverage.acceptable: 60%. The PR description advertises a configurable diff-coverage gate; as written, changed-line coverage is not gated at all.
2. The fork-PR comment suppression is dead config. The IS_FORK logic computes COMMENT_ENABLE=false and writes comment.enable, which octocov ignores — so the intended "don't comment on fork PRs" behavior never takes effect.

Fix: drop the diff.coverage.acceptable / comment.enable / summary.enable keys. To gate diff coverage, fold it into coverage.acceptable as an expression (current >= <overall>% && diff >= <diff>%); to suppress comments on forks, use comment.if: with a condition (e.g. is_pull_request && env.IS_FORK != 'true') rather than a boolean. Please confirm the generated .octocov.yml actually enforces the intended diff threshold after the change.

🔴 P1-2 — Core-package coverage gate fails open on misconfiguration / build break

reusable-go-quality.yml, "Core packages coverage check" Python step (~lines 427-513). This step is the only mechanism enforcing core-coverage-threshold (default 75%), and it exits 0 in two failure modes:

• go list returns non-zero (~lines 443-455): the result is only consumed under if result.returncode == 0:. If go list fails — a build break in/under a core package, or a typo'd core-packages pattern — core_import_paths stays empty, the code hits if not core_import_paths: ... sys.exit(0), and the gate passes without checking anything.
• total_stmts == 0 (~lines 496-500): if a pattern resolves but no coverage.out line matches the import paths, the step prints a warning and sys.exit(0).

In both cases the root cause is the same: "couldn't measure" is treated as "passed." On org-wide infra this means any repo silently loses its core-coverage gate the moment a core pattern is mistyped or a core package fails to build — exactly when the gate should bite. Fix: fail closed when core-packages is non-empty — exit 1 (with ::error::) on non-zero go list, and on total_stmts == 0.

3. Recommendation

Both blockers are small, localized changes:

1. Replace the three invalid octocov keys so the diff-coverage threshold and fork comment-disable actually take effect (P1-1).
2. Make the core-coverage step fail closed on go list failure and zero statements (P1-2).

The P2/nits below are non-blocking but worth folding in.

4. Non-blocking (P2 / nits)

• 🟡 reusable-node-quality.yml hardcodes davelosert/vitest-coverage-report-action; repos with has-tests: true on jest/other runners won't get a report. Worth documenting the vitest assumption (or making it conditional).
• 🟢 pnpm test --coverage relies on pnpm forwarding the flag to the underlying script; pnpm test -- --coverage matches the npm branch and is more robust.
• 🟢 The report-coverage job checks out the PR ref but executes no project code, so it's safe; the checkout is only needed for working-directory context for the action. Fine as-is — noting for awareness.

5. Additional notes

• The split test-with-services / test-standalone jobs and the always() && (…success) gate on diff-coverage remain a correct workaround for GitHub Actions' lack of conditional services: blocks; the shared test-results artifact name is safe because the two jobs are mutually exclusive via if:.
• Per-package DB reset (DROP/CREATE + FLUSHALL) is a sound isolation pattern for the service-backed runs.
• quality-gate aggregation (both files) is fail-closed and correct; artifact upload under if: always() is a good touch.

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/.github, but the new reusable workflows can fail valid fork PRs because PR-comment reporting is not safely disabled.

🔴 Blocking

• 🔴 Critical: .github/workflows/reusable-go-quality.yml:363-411 computes IS_FORK inside the Generate octocov config shell step, but only exports it for that step. The next Run octocov action does not receive it, so comment.if: "is_pull_request && env.IS_FORK != 'true'" still evaluates as true when env.IS_FORK is unset. On fork PRs, octocov will try to comment with a read-only token and can fail the required diff-coverage / quality-gate path. Persist it through $GITHUB_ENV, pass it as env: to the octocov step, or skip/comment-disable explicitly for forks.

• 🔴 Critical: .github/workflows/reusable-node-quality.yml:195-218 always runs the Vitest coverage PR reporter on pull_request when has-tests is true, with pull-requests: write. Fork PRs do not get write-capable GITHUB_TOKENs, so this reusable workflow can fail otherwise valid OSS contributions. Add a fork guard, set comment-on: none for forks, or split reporting into a safe workflow_run pattern.

💬 Non-blocking

• 🟡 Warning: .github/workflows/reusable-go-quality.yml:413-500 applies coverage-exclude only to octocov, not to the custom core package coverage check. Repos with generated or mock files inside core-packages may fail the core threshold even though the workflow input says those files are excluded.

✅ Highlights

• The workflows follow the repository’s reusable workflow direction and keep permissions scoped per job.
• The Go workflow uploads test artifacts on failure and provides a stable quality-gate job, which is useful for branch protection.

[lml2468]

Re-review — PR #55 (449cf9f)

Incremental review of commit 449cf9f (only reusable-go-quality.yml changed since fd96a89).

Previous findings status

From yujiawei (fd96a89):

1. octocov invalid keys (diff.coverage.acceptable, comment.enable, summary.enable) — ✅ Resolved. Config now uses valid octocov schema: coverage.acceptable with expression syntax (current >= N && diff >= 0), comment.if with condition expression, and summary section removed (octocov defaults to showing summary). Verified against octocov source (config/config.go:260, config/ready.go:112, README line 450).

2. Fork PR comment disable — ⚠️ Partially resolved. The config now uses valid comment.if: "is_pull_request && env.IS_FORK != 'true'" syntax (octocov supports env map via os.Environ() in config/config.go:481,500). However, see P1 below — the env var propagation is broken.

3. Core-packages coverage gate — ✅ Resolved. go list failure now exit 1, no packages matched now exit 1, zero statements now exit 1. All three fail-closed paths verified.

From Jerry-Xin (fd96a89):

4. Core-packages silent fail — ✅ Resolved. Same as #3 above.

5. Node fork PR quality gate failure — ❌ Still open. reusable-node-quality.yml was not changed. report-coverage (line 195) still runs on all pull_request events with pull-requests: write. Fork PRs get a read-only token → vitest-coverage-report-action fails → quality-gate treats failure as blocking (line 306). The Go workflow has a fork guard; the Node workflow needs the same treatment (either skip report-coverage for forks, or treat its failure as non-blocking for forks).

New findings

🔴 P1 — Go IS_FORK env var not propagated to octocov action step

reusable-go-quality.yml:405: export IS_FORK is inside the "Generate octocov config" run: step. In GitHub Actions, each step runs in a separate shell process — export does NOT persist to subsequent steps. The "Run octocov" step (line 410-411) is a separate action step (k1LoW/octocov-action), so IS_FORK will not be in its environment.

octocov's comment.if evaluates env.IS_FORK by reading os.Environ() (verified in config/config.go:498-512). Since the variable is missing, env.IS_FORK will be empty string "", which is != 'true' → the condition evaluates to true → octocov will attempt to post a PR comment even on fork PRs → the comment will fail because the token is read-only.

Fix: Replace export IS_FORK with:

echo "IS_FORK=$IS_FORK" >> "$GITHUB_ENV"

This makes IS_FORK available to all subsequent steps via $GITHUB_ENV.

CI status

• No tabs in workflow files: ✅ pass
• actionlint: ✅ pass
• code-review: pending

Verdict

CHANGES_REQUESTED — Two blocking items remain:

1. Node fork PR quality gate issue (unchanged from prior round)
2. Go IS_FORK env var propagation bug (new finding — the fork guard is syntactically correct but won't work at runtime)

[yujiawei]

Code Review — PR #55 (.github)

Fifth-round review of the two reusable workflows (reusable-go-quality.yml, reusable-node-quality.yml) at head 449cf9f. The two blockers raised last round are both genuinely fixed, verified against octocov and the vitest-action source rather than the PR description. No remaining blockers — the advertised gates now actually fire. A few non-blocking items below are worth folding in but should not hold up the merge.

Verdict: APPROVED.

1. Verification summary

• ✅ Fixed (prev P1-1) — .octocov.yml now uses valid octocov keys.
• ✅ Fixed (prev P1-2) — core-package coverage gate now fails closed.
• ✅ Confirmed non-blocking — Node report-coverage on fork PRs does not break the quality gate.
• 🟡 New, non-blocking — fork comment-suppression (IS_FORK) is dead config due to a cross-step env bug, but it is cosmetic because octocov swallows comment failures.

2. Findings

✅ Prev P1-1 resolved — octocov config keys are valid and the gate enforces

The "Generate octocov config" step now writes a schema-valid .octocov.yml:

coverage:
  paths:
    - coverage.out
  acceptable: "current >= 60 && diff >= 0"
comment:
  if: "is_pull_request && env.IS_FORK != 'true'"

The previously-invalid diff.coverage.acceptable, comment.enable, and summary.enable keys are gone. Verified against octocov source:

• coverage.acceptable, coverage.paths, coverage.exclude, and comment.if are all supported keys.
• The current variable is on a 0–100 percentage scale (config/config.go coverageAcceptable, and the table tests use current >= 60% against 50.0), so the bare current >= 60 (no %) is correct — % is optional and stripped by trimPercentRe.
• The threshold is enforced: c.Acceptable(...) returning an error propagates out of RunE (cmd/root.go), failing the action, which fails diff-coverage, which fails quality-gate. The gate is real now.

✅ Prev P1-2 resolved — core-coverage gate fails closed

The "Core packages coverage check" Python step now sys.exit(1) on all three "couldn't measure" paths: non-zero go list, zero matched patterns, and total_stmts == 0. A mistyped core-packages pattern or a build break in a core package now fails the gate instead of silently passing. Import-path matching (via go list -f {{.ImportPath}} against the dir of each coverage.out entry) is also the correct fix for the earlier literal-./modules/...-grep problem.

✅ Node fork report-coverage — not a gate blocker (was raised as a prior blocker)

report-coverage requests pull-requests: write, which forks don't get. I traced davelosert/vitest-coverage-report-action at the pinned SHA: handleError (src/index.ts) catches 403/404 and emits only core.warning(...) then returns; any other error is re-thrown into the top-level .catch((err) => core.error(err)), which logs but never calls setFailed. The action therefore always exits 0, so on a fork the coverage comment is skipped with a warning and report-coverage still succeeds — the gate is not blocked by reporting. No change required.

🟡 P2-1 (new, non-blocking) — fork comment-suppression is dead config

In the Go workflow's "Generate octocov config" step, IS_FORK is computed and export IS_FORK is called, but export only affects the current step's shell; it does not persist to the separate "Run octocov" step (GitHub Actions requires writing to $GITHUB_ENV for cross-step env). octocov's comment.if: env.IS_FORK != 'true' therefore reads an unset variable (empty string → condition is always true), so commenting is attempted on forks regardless. This is cosmetic only: octocov wraps commentReport and swallows the failure (Skip commenting report to pull request: ...) without failing the run, so the fork case degrades to a logged skip — exactly the intended end state, just via a different path. If you want the config to actually do what it says, write echo "IS_FORK=$IS_FORK" >> "$GITHUB_ENV" instead of export IS_FORK.

3. Non-blocking (P2 / nits, carried over)

• 🟡 coverage.acceptable: "current >= 60 && diff >= 0" — without a configured octocov datastore there is no previous report, so prev defaults to 0 and diff = current - 0 >= 0 is always true. The effective gate is overall coverage only; the diff >= 0 clause is a no-op until a datastore is wired. Worth a comment so future maintainers don't assume changed-line gating is active.
• 🟡 reusable-node-quality.yml hardcodes davelosert/vitest-coverage-report-action; repos with has-tests: true on jest/other runners won't get a coverage report. Worth documenting the vitest assumption.
• 🟢 pnpm test --coverage relies on pnpm forwarding the flag to the underlying script; pnpm test -- --coverage matches the npm branch and is more robust.

4. Additional notes

• All uses: are SHA-pinned; gotestsum pinned to v1.12.0; top-level permissions: {} with per-job least privilege; pull-requests: write correctly isolated to the octocov diff-coverage job and the Node report-coverage job. actionlint and no-tabs checks are green on this head.
• Both Python heredocs (<<'PYEOF') are fed to python3 -; after YAML block-scalar stripping the code starts at column 0, so there is no indentation error despite the in-file visual indent — confirmed by parsing the YAML.
• Split test-with-services / test-standalone + the always() && (…success) gate on diff-coverage remain a correct workaround for GHA's lack of conditional services:; the shared test-results artifact name is safe since the two jobs are mutually exclusive via if:. Per-package DB reset (DROP/CREATE + FLUSHALL) is a sound isolation pattern. JUnit merge via Python and the corrected passed = tests - failures - skipped count are both fine.