chore: quality gates — CI, coverage thresholds, UI tests, static analysis, docs - #2
Merged
Merged
Conversation
Until now the verify gate only existed when someone ran it locally. Four jobs: verify matrix on ubuntu+macos (typecheck, lint, tests, knip, coverage with bunfig thresholds), TUI PTY smoke+e2e via expect, compiled-binary smoke on ubuntu only (macOS excluded deliberately — the Bun unsigned-binary SIGKILL issue from the design watchlist), and a live-model smoke that runs solely on main pushes with the ANTHROPIC_API_KEY secret so PRs never see the secret and per-PR token spend stays zero; it skips quietly when the secret is absent. Dependabot groups the AI SDK packages (they version in lockstep — see the @ai-sdk/openai v3/v4 spec mismatch) and dev tooling, weekly, plus github-actions updates.
Coverage stays opt-in (--coverage) so the local verify loop stays fast; CI runs the enforcing pass. Bun applies coverageThreshold per file, not to the total — discovered empirically: totals of 93/96% still fail at 0.80 because three tool files sit at 66.67% function coverage. The threshold starts at 0.60, just under today's worst file, and ratchets to 0.80 in the coverage-gap commit later in this PR. Documented in the file so nobody 'fixes' it back to a total-based mental model.
… knip exactOptionalPropertyTypes and noImplicitOverride are cheap now and painful to adopt later. The fallout was uniform: internal interfaces whose optional properties legitimately receive computed possibly-undefined values gain an explicit `| undefined`, while external types (Ink props, MCP SDK transport params, AI SDK streamText) get conditional spreads at the call site so we never pass an explicit undefined into someone else's optional. noExplicitAny is promoted from warn to error (already clean), and knip joins the gate for dead files, exports, and dependencies — the one static-analysis category nothing else covered in a workspace. It runs as `bun run knip` in the CI test job.
The argv loop lived as top-level script code interleaved with process.exit calls — untestable without spawning a process. It is now a pure function returning run/help/error, unit-tested for every flag, value-missing and flag-as-value errors, command/flag ordering, and the usage text; the entrypoint just switches on the result. Behavior is unchanged (same messages, same exit codes).
The CLI package had zero automated coverage. These render the real App against a real kernel + scripted provider (nothing mocked below the terminal): command palette (/help, unknown command), a complete prompt with streamed text, permission approval and completed tool render, permission rejection marking the tool failed while the turn continues, the todo checklist, and the /mode indicator. Two Ink-under-test quirks are encoded in the helpers rather than sprinkled as sleeps: a chunk containing \r is treated by TextInput as a paste (Enter must be its own keystroke), and writes that land before TextInput's stdin listener attaches — right after first render or after the input remounts post-turn — are dropped, so type() settles first. Verified stable across repeated runs.
The render smoke lived outside the repo as a scratch file — the only check that the terminal UI actually draws was unreproducible. Both scripts now live in scripts/ and run in CI's tui-smoke job: smoke-tui.exp starts the real entrypoint under a PTY, asserts banner + input prompt, and exits via /exit. e2e-tui.exp drives a complete scripted conversation through the genuine Ink UI — typed prompt, streamed assistant text, permission approval keystroke, completed tool output — against the tui-scripted fixture (real kernel + scripted provider, no network). Complements the ink-testing-library tests: those exercise components in-process; these prove the same flows through an actual pseudo-terminal. knip immediately earned its keep: expect is declared as a system binary and CliArgs lost an export nothing imported.
Targeted tests for the uncovered branches the report pointed at: every tool's title() (including malformed-input throws that exercise the loop's fallback), runtime exec spawn failure, todo_write without its loop hook, corrupt settings JSON, unknown-tool calls, a tool throwing mid-execution, provider stream errors producing turn.failed, mid-turn cancellation synthesizing results for pending tool calls (via a hand-rolled slow provider), replay's orphan-result/unknown-event/ missing-tool paths, and the store's plan-replacement and mode handling. PermissionBridge becomes a factory function instead of a class: bun's function-coverage counter never credits a class whose only members are field initializers, which held the file at 50% regardless of tests. The factory reads better anyway. Totals now 96.5% functions / 97.8% lines with every file at or above 0.80/0.80 — the bunfig threshold ratchets from 0.60 to 0.80 as planned.
CONTRIBUTING.md captures the workflow that was previously tribal knowledge: the verify gate and what CI enforces beyond it, branch/commit conventions (conventional commits with why-bodies, no-squash merges), the test layout, and MINERVA_DATA_DIR isolation for manual runs. docs/PROTOCOL.md is the wire reference the M2 GUI and any external frontend will build against: framing, both transports, every ACP-core method with params/results, all session/update variants, permission options and outcome semantics (including the cancelled-outcome and transport-failure rules), the minerva/* extensions, and the versioning policy for what does and does not bump PROTOCOL_VERSION. CHANGELOG.md starts at 0.1.0 (Keep a Changelog), with the quality-gates work under Unreleased. README gains the CI badge and doc links.
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds strict TypeScript settings, CLI parsing, permission-bridge changes, TUI and kernel coverage, CI workflows, smoke tests, dependency automation, protocol documentation, and contribution and release documentation. ChangesRepository validation and contracts
CLI and automation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Marking it --external made the compile succeed but deferred resolution to runtime, where the single-file executable has no node_modules — 'Cannot find package react-devtools-core' on launch (caught by the new release-smoke job on its very first run). Ink's devtools hook is now a real devDependency of the CLI so the bundle is self-contained; it only activates under DEV=true at runtime.
The tui-smoke job failed with 'no banner' after 20 silent seconds — the expect blocks only handled the match and timeout, so a process that died instantly was indistinguishable from one that never drew. Both scripts now handle eof explicitly, and the job first runs the entrypoint headless (--help) so bun/module resolution failures surface as their own step with a real error message instead of a PTY timeout.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
77-92: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against silent skip masking real failures in live-smoke.
The
live-smokejob correctly restricts to main-branch pushes, andscripts/live-smoke.tsexits 0 whenANTHROPIC_API_KEYis unset. However, if the secret is never configured, this job always passes without actually testing anything. Consider adding a comment or step that surfaces whether the secret was present, so the team can distinguish "skipped" from "passed" in CI logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 77 - 92, Add an explicit step in the live-smoke job before running scripts/live-smoke.ts that reports whether ANTHROPIC_API_KEY is configured, using a masked presence check without printing the secret; clearly label the no-secret case as skipped and the configured case as running, while preserving the existing secure behavior.packages/cli/test/app.test.tsx (1)
94-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a settle delay before permission keystrokes.
The
typehelper at line 54 waits 50ms before writing to avoid dropped keystrokes when TextInput remounts. However,ui.stdin.write("y")at line 97 (andui.stdin.write("n")at line 123) write immediately afterwaitForconfirms the prompt is visible. If the permission prompt's stdin listener isn't attached when the frame renders, the keystroke could be dropped. Adding a shortawait Bun.sleep(50)before these writes would match thetypehelper's defensive pattern and reduce flakiness risk.♻️ Proposed fix
await waitFor(() => (ui.lastFrame() ?? "").includes("Permission required"), "permission"); expect(ui.lastFrame()).toContain("echo ui-e2e"); + await Bun.sleep(50); ui.stdin.write("y");Apply the same pattern at line 123:
await waitFor(() => (ui.lastFrame() ?? "").includes("Permission required"), "permission"); + await Bun.sleep(50); ui.stdin.write("n");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/app.test.tsx` around lines 94 - 98, Add a 50ms settle delay before the direct permission responses in the test: insert await Bun.sleep(50) after the permission prompt wait and before ui.stdin.write("y"), and apply the same delay before the corresponding ui.stdin.write("n") in the other permission test. Match the existing type helper timing pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 52-53: Fix the YAML syntax in the sanity-check step named “Sanity
— entrypoint runs headless in this job context” by changing its run command to a
block scalar (|), preserving the existing command and grep pattern so the
colon-space sequence is parsed safely.
In `@CHANGELOG.md`:
- Around line 10-12: Update the CHANGELOG entry to use the standard “macOS”
capitalization instead of “macos” in the CI platform list.
In `@packages/cli/package.json`:
- Around line 22-24: Update the `ink-testing-library` dependency in
`packages/cli/package.json` to a major version compatible with the project’s
`ink@^6` and React 19 versions, or remove it if no CLI tests use it; verify the
affected CLI test imports and dependency lockfile are consistent.
In `@packages/cli/test/app.test.tsx`:
- Around line 17-30: Update renderTui to retain the MinervaKernel returned by
createKernel, expose or register it for cleanup, and close it from an afterEach
hook so every test disposes the kernel and its transports/MCP connections.
In `@scripts/live-smoke.ts`:
- Around line 20-50: Wrap the live smoke test workflow beginning with client
initialization and ending after the success check in a try/catch. In the catch
block, call clearTimeout(timeout), log the actual error with clear context, and
exit with status 1; retain the existing success-path timer cleanup and
validation. Use the existing timeout, client.initialize, client.newSession, and
client.prompt symbols to locate the changes.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 77-92: Add an explicit step in the live-smoke job before running
scripts/live-smoke.ts that reports whether ANTHROPIC_API_KEY is configured,
using a masked presence check without printing the secret; clearly label the
no-secret case as skipped and the configured case as running, while preserving
the existing secure behavior.
In `@packages/cli/test/app.test.tsx`:
- Around line 94-98: Add a 50ms settle delay before the direct permission
responses in the test: insert await Bun.sleep(50) after the permission prompt
wait and before ui.stdin.write("y"), and apply the same delay before the
corresponding ui.stdin.write("n") in the other permission test. Match the
existing type helper timing pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c99c185a-a20d-49dd-9f6d-8d27b93ac448
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.github/dependabot.yml.github/workflows/ci.ymlCHANGELOG.mdCONTRIBUTING.mdREADME.mdbiome.jsonbunfig.tomldocs/PROTOCOL.mdknip.jsonpackage.jsonpackages/cli/package.jsonpackages/cli/src/app.tsxpackages/cli/src/args.tspackages/cli/src/index.tsxpackages/cli/src/permission-bridge.tspackages/cli/test/app.test.tsxpackages/cli/test/args.test.tspackages/cli/test/fixtures/tui-scripted.tsxpackages/cli/test/permission-bridge.test.tspackages/client/src/store.tspackages/client/test/store.test.tspackages/kernel/src/agent-loop.tspackages/kernel/src/events.tspackages/kernel/src/kernel.tspackages/kernel/src/mcp.tspackages/kernel/src/permissions.tspackages/kernel/src/replay.tspackages/kernel/src/runtime.tspackages/kernel/src/settings.tspackages/kernel/src/tools/types.tspackages/kernel/test/gaps.test.tspackages/protocol/src/types.tspackages/providers/src/ai-sdk.tspackages/providers/src/types.tsscripts/e2e-tui.expscripts/live-smoke.tsscripts/smoke-tui.exptsconfig.json
Comment on lines
+22
to
+24
| "@types/react": "^19", | ||
| "ink-testing-library": "^4.0.0", | ||
| "react-devtools-core": "^7.0.1" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the ink version declared in the CLI package or root package.json
fd package.json packages/cli --max-depth 1 --exec cat {} \; | jq '.dependencies.ink, .devDependencies.ink'
# Also check root
cat package.json | jq '.dependencies.ink, .devDependencies.ink'Repository: hutusi/minerva
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/cli/package.json =="
cat -n packages/cli/package.json | sed -n '1,120p'
echo
echo "== root package.json =="
cat -n package.json | sed -n '1,220p'
echo
echo "== lockfiles mentioning ink-testing-library / ink / react =="
rg -n '"ink-testing-library"|"ink"|"react"' -g 'pnpm-lock.yaml' -g 'package-lock.json' -g 'yarn.lock' -g 'npm-shrinkwrap.json' .
echo
echo "== package manifests in repo mentioning ink-testing-library =="
rg -n '"ink-testing-library"|ink-testing-library' -g 'package.json' .Repository: hutusi/minerva
Length of output: 1884
🌐 Web query:
npm ink-testing-library 4.0.0 peerDependencies ink 6 React 19
💡 Result:
The npm package ink-testing-library version 4.0.0 does not officially list ink 6 or React 19 in its peerDependencies [1][2]. While attempts have been made by the community to update ink-testing-library for compatibility with Ink 6 and React 19 [3], these changes were not merged into the 4.0.0 release, which was published on May 22, 2024 [1][4]. Users have reported significant compatibility issues when using ink-testing-library 4.0.0 with newer versions of Ink (such as v5 and v6) [3][5], particularly regarding input handling (useInput) and React 19's synchronous effect execution [3][5]. Consequently, ink-testing-library 4.0.0 is generally considered incompatible with Ink 6 and React 19 in its current state [3][5].
Citations:
- 1: https://registry.npmjs.org/ink-testing-library
- 2: https://app.unpkg.com/ink-testing-library@4.0.0
- 3: Update ink to v6 vadimdemedes/ink-testing-library#28
- 4: https://www.npmjs.com/package/ink-testing-library
- 5: stdin.write() does not trigger useInput callbacks in Ink v5 vadimdemedes/ink-testing-library#29
Align ink-testing-library with ink@^6 packages/cli/package.json:22-23 pins ink-testing-library@^4.0.0, which doesn’t match the current ink@^6 / React 19 stack. Update it to a compatible major or remove it if the CLI tests no longer need it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/package.json` around lines 22 - 24, Update the
`ink-testing-library` dependency in `packages/cli/package.json` to a major
version compatible with the project’s `ink@^6` and React 19 versions, or remove
it if no CLI tests use it; verify the affected CLI test imports and dependency
lockfile are consistent.
The step's unquoted run scalar contained 'Usage: minerva' — the colon-space made the workflow file unparseable, failing the run in zero seconds. Block scalar sidesteps YAML's flow-scalar colon rule; the file is now validated locally before pushing.
The tui-smoke job timed out waiting for the banner while the app was demonstrably alive (the new eof branch never fired and the headless sanity step passed). The pattern 'Minerva \u00b7' contains a UTF-8 middle dot; under the runner's C locale Tcl's encoding mangles it and the match never fires — locally, with a UTF-8 locale, it always did. Patterns are now plain ASCII and the spawned process gets LANG=C.UTF-8 so the middle dot renders consistently either way.
Root cause of the tui-smoke timeouts: Ink detects CI (ci-info reads CI, GITHUB_ACTIONS, ...) and disables interactive rendering, drawing only on unmount — so under expect's PTY the process was alive and intentionally silent. The whole point of these scripts is that expect IS the terminal, so they now unset the CI markers for the spawned process. The earlier eof-vs-timeout diagnostics and headless sanity step are what narrowed this from 'no banner' to 'alive but not drawing'.
On the Linux runner the spawned PTY reports 0x0; Ink wraps to the reported width, emitting one character per line, so no multi-character pattern could ever match (macOS PTYs default to 80x24, which is why the same scripts passed there and locally). stty_init pins 120x40 for both platforms.
Four of five findings actioned (the fifth, the YAML block-scalar fix, had already landed in an earlier commit): - app.test.tsx now retains each MinervaKernel and closes it in afterEach, so transports don't leak across tests. - ink-testing-library is pinned to exactly 4.0.0 with a comment: it predates ink 6 / React 19 and no compatible major exists (upstream PRs unmerged), so our passing suite is the compatibility proof and an unreviewed 4.x bump must not slip in via install. Upgrading, as the review suggested, is not currently possible. - live-smoke wraps its flow in try/catch for a labeled failure message. Note the review's premise was slightly off: Bun exits immediately on an unhandled top-level rejection rather than hanging until the timer — the wrap improves diagnostics, not liveness. - CHANGELOG: macOS capitalization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
v0.1 merged with a solid local verify gate (typecheck + biome + tests) but nothing enforcing it: no CI, the only TUI check lived outside the repo, the CLI package had zero automated coverage, and the workflow/protocol knowledge existed only in heads. This hardens all of that before M2 (Tauri GUI) piles new code on top.
What
.github/workflows/ci.yml): verify matrix on ubuntu+macos (typecheck, lint, tests, knip, per-file coverage thresholds), TUI PTY smoke+e2e via expect, compiled-binary smoke on ubuntu (macOS excluded: known Bun unsigned-binary SIGKILL, see DESIGN watchlist), and a live-model smoke that runs only on main pushes with theANTHROPIC_API_KEYsecret — PRs never see the secret and it skips quietly when absent. Plus Dependabot (AI SDK packages grouped — they version in lockstep).bunfig.toml): bun enforces them per file, not on the total (discovered empirically); started at 0.60, ratcheted to 0.80 within this PR after closing gaps. Totals now 96.5% functions / 97.8% lines.parseCliArgswith unit tests; full-stack Ink UI tests via ink-testing-library (real kernel + scripted provider under the components); PTY scripts committed toscripts/including a genuine pseudo-terminal e2e of a complete conversation with permission approval.exactOptionalPropertyTypes+noImplicitOverride, BiomenoExplicitAnyat error, knip in the gate (it immediately caught an unlisted binary and a dead export).PermissionBridgebecame a factory — bun's function-coverage counter never credits field-initializer-only classes.CONTRIBUTING.md(workflow, gate, test layout),docs/PROTOCOL.md(the wire reference M2 builds against),CHANGELOG.md(0.1.0 + unreleased), README badge/links.Verification
This PR's own CI run is the deliverable proving itself — all jobs on both OSes, with live-smoke correctly skipped (PR context). Locally:
bun run verifygreen (110 tests),bun test --coveragepasses the 0.80 floor,bun run knipclean, both PTY scripts pass.Summary by CodeRabbit