Skip to content

Run production build in CI - #153

Open
grimicorn-agent wants to merge 8 commits into
mainfrom
agent/ci-run-build
Open

Run production build in CI#153
grimicorn-agent wants to merge 8 commits into
mainfrom
agent/ci-run-build

Conversation

@grimicorn-agent

Copy link
Copy Markdown
Collaborator

What changed

CI ran lint, typecheck (against tsconfig.test.json), and test:ci, but never npm run build — the tsc && tsc-alias step that produces the artifact actually published to npm. tsconfig.json (production build) has different include/paths/rootDir than tsconfig.test.json (typecheck), so a break reachable only through the real build compile could pass CI green and ship broken to npm.

This adds:

  1. npm run build as a CI step, so production-build-only compile breakages fail the PR.
  2. A smoke-test step that runs the built dist/index.js --help and asserts its output, so a build that compiles but produces a broken/empty entry point (e.g. a tsc-alias misconfiguration that silently fails to rewrite @/... path aliases) is also caught. This step:
    • Redirects output to a file and checks it with grep, rather than piping directly into grep -q — GitHub Actions runs steps without pipefail, so a direct pipe would let grep's exit code mask a non-zero exit from the CLI itself.
    • Prints the captured output on failure for diagnosability.
    • Sets XDG_CONFIG_HOME to an isolated temp directory, since src/libs/config.ts initializes a Conf store at module load (before the --help branch is even reached), so the step doesn't depend on/pollute the runner's real config directory.

Why not more

An independent review round also flagged that the published npm tarball currently has no files allowlist and would ship without dist/, and that testing dist/index.js directly (rather than through the packed bin entry) doesn't catch that. Both are out of scope here — PR #152 (issue #146) is already adding a prepack build hook and files allowlist for exactly that, and this PR was scoped to stay out of package.json's publish/dist handling per the issue's instructions.

Where to view

https://github.com/neonpixels-studio/markpost-cli/blob/agent/ci-run-build/.github/workflows/ci.yml

Closes #147

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail

Round 1

Round 2

  • Flagged: smoke test only asserts exit code, not output content (a stripped/broken entry file could still exit 0).
    • Fixed: pipe output through grep -q 'Usage: markpost <command>'.
  • Flagged: the smoke test isn't hermetic — src/libs/config.ts initializes a Conf store at module load, touching the runner's real config directory before the --help branch is even reached.
    • Fixed: set XDG_CONFIG_HOME to an isolated temp path for that step.
  • Flagged (again): packaging/files field / testing through the packed bin entry rather than dist/index.js directly.
  • Flagged: no permissions: block on the job.
    • Skipped: pre-existing, unrelated to this change; not introduced by this diff.

Round 3

  • Flagged: GitHub Actions runs steps without pipefail, so node ... | grep -q ... lets grep's exit code mask a non-zero exit from the CLI itself — the one failure mode the step exists to catch.
    • Fixed: redirect stdout to a file, then grep the file, so the CLI's own exit isn't laundered through the pipe.
  • Flagged: a failure produced zero diagnostic output (grep -q is silent, and stdout never reached the log).
    • Fixed: on grep mismatch, print the captured output before exiting non-zero.
  • Flagged: the asserted string duplicates a literal from src/index.ts:157 with nothing linking them, so a wording change reds CI with no explanation.
    • Fixed: named the step, anchored the grep pattern, and the failure message points at src/index.ts:157 as the likely place to reconcile.

Unresolved after 3 rounds: none — every finding was either fixed or explicitly out of scope (packaging/files field, owned by PR #152; the pre-existing missing permissions: block, unrelated to this change).

@grimicorn grimicorn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix CI failure

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Re: "Fix CI failure" — the only red check here is dependency-audit, and it's the fleet-wide fast-uri advisory (GHSA-5jgf-p345-68v8 and related HIGH advisories) inherited from main, not anything introduced by this PR.

Folding the fast-uri override into every affected PR would create duplicate/conflicting fixes, so per our convention the fix lives in one dedicated PR: #156 (agent/audit-fix-fast-uri), which is fully green (ci, dependency-audit, gitleaks all pass) and already has your review requested.

Once #156 merges to main, this branch's dependency-audit clears on the next merge of main. No code change is needed here to fix CI — the fix is #156. Re-requesting your review with that context.

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

The only red check here is dependency-audit, which is the fleet-wide advisory failure on main — not caused by this PR's diff (it only adds the production build step to CI). The fix is already isolated in its own green PR, #156 ("Fix dependency-audit: bump fast-uri override to clear HIGH advisories"), per the one-dedicated-PR convention for advisory overrides. Once #156 merges, I'll merge main into this branch and dependency-audit will go green. No changes needed on this PR itself.

@grimicorn-agent

Copy link
Copy Markdown
Collaborator Author

Independent code review trail (fixing CI: audit + build step)

Fix applied: merged origin/main into this branch (clean, no conflicts) to pull in PR #156's fast-uri override, which clears the dependency-audit failure. Verified locally: npm audit --audit-level=high → 0 vulnerabilities; npm run lint, npm run typecheck, npm run test:ci (847 tests), npm run build, and the --help smoke test all pass.

Round 1:

  • Flagged: help.txt written to repo root untracked (not gitignored) — fixed, redirected to $RUNNER_TEMP.
  • Flagged: hardcoded src/index.ts:157 line number in the failure message will drift — fixed, now references HELP_TEXT by name instead.
  • Flagged: XDG_CONFIG_HOME env var's effect (Linux-only via conf/env-paths) undocumented and silently depends on runs-on: ubuntu-latest — fixed, added an explanatory comment.
  • Flagged: stderr discarded from the diagnostic — fixed, redirected 2>&1.
  • Flagged (skipped): smoke test runs node dist/index.js directly instead of the packed npm tarball, so it wouldn't catch a files/bin regression like Fix npm publish to ship compiled dist binary #152. Valid, but a redesign beyond this task's scope (fix the CI failure on this PR) — logged as a follow-up suggestion below.

Round 2:

  • Flagged: a crashing build (non-zero exit) aborts the step before the diagnostic cat ever runs, so a real crash prints nothing — fixed, wrapped the node invocation so failure output is always surfaced.
  • Flagged (skipped): re-raised the packed-artifact point — same reasoning, out of scope, tracked below.
  • Flagged (skipped): suggested splitting stdout/stderr into separate files — directly contradicts round 1's own fix (which asked to merge them via 2>&1). Kept the merged-stream version from round 1 rather than flip-flopping between two reviewer opinions with no clear correctness win either way.
  • Flagged (skipped): suggested asserting an exact count of Usage: markpost lines in the help output — more brittle (breaks on any command addition) for marginal extra coverage; out of scope for this task.

Round 3 (final round — 3-round cap reached):

  • Flagged: the crash-diagnostic fix from round 2 collapsed all failure modes (crash, OOM kill, etc.) into one message without the actual exit code — fixed, now captures and reports $?.
  • Flagged (skipped): re-raised the packed-artifact redesign a third time — still out of scope for this PR; logged as a follow-up suggestion.
  • Flagged (skipped): suggested a per-command loop asserting every COMMANDS entry appears in the help text — reasonable idea, but expands the test's scope beyond what this PR set out to add; out of scope here.

Unresolved after 3 rounds: the smoke test still exercises dist/index.js from the working tree rather than the packed/installed tarball, so it wouldn't have caught the exact files/bin misconfiguration that #152 fixed. This was raised consistently across all 3 rounds and is a legitimate gap, but implementing it (pack + global install + rerun) is a meaningfully larger change than "make CI green" — recorded as a follow-up suggestion for the improvement digest rather than bundled into this fix.

Follow-up suggestions

  • Test packed CLI artifact in CI, not the build tree — The new help-smoke-test step in .github/workflows/ci.yml runs node dist/index.js directly from the working tree; replacing it with npm pack + global install of the tarball would actually cover the files/bin misconfiguration class that PR Fix npm publish to ship compiled dist binary #152 fixed, which the current step cannot catch. (suggested: P3, effort: S, evidence: .github/workflows/ci.yml:22-30)

@grimicorn-agent grimicorn-agent added the has-suggestions PR carries follow-up suggestions for the improvement digest label Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-suggestions PR carries follow-up suggestions for the improvement digest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI never runs npm run build, so a production-build-only break ships undetected

2 participants