pre-deployment check

[amgb20]

first PR

[amgb20]

@claude please review

[amgb20]

@claude please review

[claude]

Claude finished @amgb20's task in 1m 57s —— View job

---

PR Review: pre-deployment check

• Gather PR context and changed files
• Read CLAUDE.md for repo guidelines (no CLAUDE.md found)
• Review key source files and changes
• Analyze tests, CI, and tooling changes
• Post comprehensive review feedback

---

Summary

This PR is a large pre-deployment setup covering: Prettier/ESLint/Husky tooling, integration tests, CI pipeline expansion, coverage config, Dependabot, and a CHANGELOG. The vast majority of the diff (~2800 lines) is Prettier reformatting of existing code. The functional changes are focused on infra/testing. Overall this is solid work — below are the issues worth addressing before merging to main.

---

Critical Issues

1. integration-real CI job has no continue-on-error: true
.github/workflows/ci.yml — the integration-real job always runs real network requests to mpp.dev. If that external endpoint is temporarily unavailable, all CI runs fail and merges are blocked. This is a hard dependency on an external service.

integration-real:
  needs: lint-and-unit
  ...
  - name: Run integration tests (real mpp.dev endpoint)
    run: npm run test:integration:real
    env:
      MPP_REAL_ENDPOINT_TESTS: "true"

Either add continue-on-error: true to this job, or gate it on a repository secret so it only runs when explicitly enabled. Fix this →

---

Moderate Issues

2. Coverage thresholds exclude commands/** and display/**
packages/cli/vitest.config.ts:14-16:

exclude: ["src/__tests__/**", "src/**/index.ts", "src/commands/**", "src/display/**"],
thresholds: { statements: 80, branches: 75, functions: 75, lines: 80 },

The 80% threshold only applies to utils/ — the most complex business logic (all command handlers and all display renderers) is excluded from coverage enforcement. Consider removing src/commands/** and src/display/** from the exclude list, or explicitly documenting that these are intentionally not covered. Fix this →

3. ESLint ignores pattern is overly broad
eslint.config.mjs:10-14:

ignores: ["**/dist/**", "**/node_modules/**", "**/*.js", "**/*.mjs"],

"**/*.js" ignores all JS files everywhere, which silently excludes any future utility scripts or configs. Since the intent is to skip compiled output, this is better expressed as just "**/dist/**" plus "**/node_modules/**". Fix this →

---

Minor Issues

4. Hardcoded chain ID assertion in real-endpoint test
packages/cli/src/__tests__/integration/real-endpoint.test.ts:54:

expect(details.chainId).toBe(42431);

If the real mpp.dev endpoint ever changes its chain configuration (e.g. adding testnet support), this assertion will fail. Consider a looser check like expect(typeof details.chainId).toBe("number").

5. test:integration:real script hardcodes the env var
packages/cli/package.json:

"test:integration:real": "MPP_REAL_ENDPOINT_TESTS=true vitest run --config vitest.integration.config.ts"

The env var MPP_REAL_ENDPOINT_TESTS is designed as a guard in the test file (process.env.MPP_REAL_ENDPOINT_TESTS === "true"). Embedding it in the script means npm run test:integration:real always bypasses the guard. This is fine by design, but CI then correctly sets it again via the env: block in the workflow — making one of the two redundant. Consider removing the guard env var from the script and relying solely on the workflow env, which makes the test's skipIf behavior more meaningful locally.

6. security audit step uses continue-on-error: true
.github/workflows/ci.yml: The npm audit --omit=dev --audit-level=high step is set to continue-on-error: true. This means high-severity vulnerabilities will never block a merge. Consider making it fail, and only accepting failures when dependencies have no available fixes.

---

Observations (no action required)

• Formatting-only changes are the bulk of the diff: ~2800/3144 lines are Prettier reformats of existing code. The logical changes are clean and minimal. This is fine, but worth noting for reviewers who may be scanning the diff — no logic changes snuck in.
• prepare: "husky" in root package.json: This is the correct setup for Husky v9. Husky automatically disables itself in CI environments (CI=true), so no side effects.
• CHANGELOG.md: Manually maintained. No tooling automation is set up — consider noting this in a contribution guide so it stays up to date.
• Integration test timeout: The vitest.integration.config.ts sets testTimeout: 30_000ms — appropriate for network-dependent tests.
• Dependabot config: Well structured with grouping for dev vs production deps and a weekly schedule.

---

Overall: Approve with the integration-real CI issue addressed (issue #1 above). The rest are quality improvements that can be done in a follow-up.
|