test: live-model e2e for /mindmodel - #102
Open
vtemian wants to merge 7 commits into
Open
Conversation
opencode calls every exported function in a plugin module as a plugin factory and installs each return value as hooks. src/index.ts exported four helpers alongside the plugin, so mergePluginAgents was being invoked as (input, undefined) and threw on Object.entries, which opencode swallowed while still exiting 0: failed to load plugin ... Object.entries requires that input parameter not be null or undefined Hook registration survived only because ESM namespace keys sort alphabetically and OpenCodeConfigPlugin starts with a capital letter, placing it ahead of the throw. Renaming that export, or adding one sorting earlier, would have silently dropped every hook. Move the helpers to plugin-config.ts and leave the entry point exporting the plugin alone.
A small invoicing domain for the live-model e2e to point /mindmodel at. Three services share one parse-and-log-on-reject shape so the pattern discoverer clears its 3-instance threshold, and src/legacy/report-export.ts is the single deliberate deviation the anti-pattern detector reports on. Test files are named *.tests.ts: bun test discovers by filename, so a *.test.ts here would be collected into the main suite and run in CI. Exclude the directory from biome and eslint. It is a self-contained sample project with its own tsconfig, so its @/* alias resolves against the parent repo's paths and every domain type becomes an error type under type-aware linting. Its legacy file also violates the house rules by design.
The unit suite cannot reach prompt adherence: it has no model in it, so nothing catches an agent that writes the wrong thing. This drives a real opencode session over a fixture project and checks the artifacts through the production loader, so a manifest that fails here is one a user could not have loaded either. Specs are named *.e2e.ts because bun discovers tests by filename, not directory. A .test.ts here would run on every PR and call a model. The free tier needs no credentials: opencode zen authenticates as public and leaves only zero-cost models enabled, and the spec asserts cost is zero to keep it that way. MICODE_E2E_MODEL opts into a paid model when a specific one needs reproducing. Also require quoted descriptions in the generated manifest, which this found on its first real run. A description like description: parseX(raw: unknown): X | null is not valid YAML, so parseManifest threw, loadMindmodel returned null and the whole generated mindmodel was unusable while the run still reported success.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
1 issue found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/plugin-config.ts">
<violation number="1" location="src/plugin-config.ts:23">
P2: Plugin startup can execute a newly published MCP implementation without a micode update because these `npx -y` commands resolve mutable package versions. Pin reviewed MCP package versions (and update them deliberately) so API-key-enabled sessions have reproducible, auditable executable dependencies.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (features?.context7 !== false) { | ||
| servers.context7 = { | ||
| type: "local", | ||
| command: ["npx", "-y", "@upstash/context7-mcp@latest"], |
There was a problem hiding this comment.
P2: Plugin startup can execute a newly published MCP implementation without a micode update because these npx -y commands resolve mutable package versions. Pin reviewed MCP package versions (and update them deliberately) so API-key-enabled sessions have reproducible, auditable executable dependencies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/plugin-config.ts, line 23:
<comment>Plugin startup can execute a newly published MCP implementation without a micode update because these `npx -y` commands resolve mutable package versions. Pin reviewed MCP package versions (and update them deliberately) so API-key-enabled sessions have reproducible, auditable executable dependencies.</comment>
<file context>
@@ -0,0 +1,69 @@
+ if (features?.context7 !== false) {
+ servers.context7 = {
+ type: "local",
+ command: ["npx", "-y", "@upstash/context7-mcp@latest"],
+ };
+ }
</file context>
Review found five of the spec's assertions were dead, including the three that replaced an earlier dead one. The subagent failure strings are unreachable by construction: /mindmodel runs as a subtask in a child session and the CLI drops events whose sessionID is not the primary one, so only the orchestrator's final text reaches stdout. Assert exit status and the error event instead, which do. Nothing on the loadMindmodel path writes to log.info or log.error, and both log.warn sites are already covered by an assertion that fails first, so emptiness checks on those channels proved nothing. Enforce the capture helper's own contract in afterEach instead: any line the spec never inspected is a degraded path it did not account for. Rename totalCost to primarySessionCost. spawn_agent gives each subagent its own top-level session, so the ten that do the analysis never reach the stream, and a zero here only ever meant the orchestrator turn was free. Also: the manifest wait was dead, since the process is fully reaped before it starts, and it reported a product failure as a timeout. Both temp dirs leaked whenever spawn threw, because the caller minted the project directory and nobody owned it. And a timer firing beside a clean exit could report a successful run as timed out. Extend the manifest quoting rule to every string value: name carries the same colon hazard, the bracket placeholder made it a YAML array, and an unescaped inner quote ends the value early. All three verified against parseManifest. Stop failing the pre-commit hook on fixture-only commits, where every staged path is excluded and both linters exit non-zero on an empty set.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/e2e/harness.ts">
<violation number="1" location="tests/e2e/harness.ts:170">
P3: A run that the timeout timer killed is reported as NOT timed out whenever the killed process exits 0 (e.g. opencode catching SIGTERM and exiting gracefully). Since the spec asserts `expect(run.timedOut).toBe(false)`, this can turn a genuinely slow/truncated run that was stopped by the timer into a green pass, exactly the slowness regression the generous RUN_TIMEOUT_MS budget is meant to surface. Consider keying on whether the kill actually preceded a clean completion (e.g. tie timedOut to the kill having fired before the drain resolved), not on the numeric exit code alone.</violation>
</file>
<file name="tests/e2e/mindmodel.e2e.ts">
<violation number="1" location="tests/e2e/mindmodel.e2e.ts:47">
P2: In `afterEach`, the `expect(stray).toEqual([])` assertion runs before the `rmSync` cleanup of `run.projectDir` / `run.homeDir`. A stray-log finding is the very failure this assertion exists to trigger, but in that case the throw aborts the rest of `afterEach`, so the per-run temp project and HOME (each potentially tens of MB, and each run taking ~12 minutes) are never removed. Because `afterEach` also runs after a failed `it()` body, this assertion can additionally fire a secondary "stray logs" error that masks the original root-cause failure from a green-body run that happens to have captured logs. Consider restoring/cleaning up in a `finally` (or moving the rmSync calls above the assertion) so cleanup always runs, and treat the stray check as a best-effort diagnostic rather than a hard assertion that can smother the real failure.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const stray = logs?.unread() ?? []; | ||
| logs?.restore(); | ||
| logs = undefined; | ||
| expect(stray).toEqual([]); |
There was a problem hiding this comment.
P2: In afterEach, the expect(stray).toEqual([]) assertion runs before the rmSync cleanup of run.projectDir / run.homeDir. A stray-log finding is the very failure this assertion exists to trigger, but in that case the throw aborts the rest of afterEach, so the per-run temp project and HOME (each potentially tens of MB, and each run taking ~12 minutes) are never removed. Because afterEach also runs after a failed it() body, this assertion can additionally fire a secondary "stray logs" error that masks the original root-cause failure from a green-body run that happens to have captured logs. Consider restoring/cleaning up in a finally (or moving the rmSync calls above the assertion) so cleanup always runs, and treat the stray check as a best-effort diagnostic rather than a hard assertion that can smother the real failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/mindmodel.e2e.ts, line 47:
<comment>In `afterEach`, the `expect(stray).toEqual([])` assertion runs before the `rmSync` cleanup of `run.projectDir` / `run.homeDir`. A stray-log finding is the very failure this assertion exists to trigger, but in that case the throw aborts the rest of `afterEach`, so the per-run temp project and HOME (each potentially tens of MB, and each run taking ~12 minutes) are never removed. Because `afterEach` also runs after a failed `it()` body, this assertion can additionally fire a secondary "stray logs" error that masks the original root-cause failure from a green-body run that happens to have captured logs. Consider restoring/cleaning up in a `finally` (or moving the rmSync calls above the assertion) so cleanup always runs, and treat the stray check as a best-effort diagnostic rather than a hard assertion that can smother the real failure.</comment>
<file context>
@@ -49,8 +39,12 @@ describe("micode /mindmodel against a live model", () => {
+ const stray = logs?.unread() ?? [];
logs?.restore();
logs = undefined;
+ expect(stray).toEqual([]);
// opencode installs @opencode-ai/plugin into the per-run HOME on cold
// start, so leaving it behind leaks tens of MB per spec.
</file context>
|
|
||
| // The timer can fire while a healthy process is already exiting, so a clean | ||
| // exit outranks it: only a kill that actually stopped the run counts. | ||
| const timedOut = killedByTimer && exitCode !== 0; |
There was a problem hiding this comment.
P3: A run that the timeout timer killed is reported as NOT timed out whenever the killed process exits 0 (e.g. opencode catching SIGTERM and exiting gracefully). Since the spec asserts expect(run.timedOut).toBe(false), this can turn a genuinely slow/truncated run that was stopped by the timer into a green pass, exactly the slowness regression the generous RUN_TIMEOUT_MS budget is meant to surface. Consider keying on whether the kill actually preceded a clean completion (e.g. tie timedOut to the kill having fired before the drain resolved), not on the numeric exit code alone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/harness.ts, line 170:
<comment>A run that the timeout timer killed is reported as NOT timed out whenever the killed process exits 0 (e.g. opencode catching SIGTERM and exiting gracefully). Since the spec asserts `expect(run.timedOut).toBe(false)`, this can turn a genuinely slow/truncated run that was stopped by the timer into a green pass, exactly the slowness regression the generous RUN_TIMEOUT_MS budget is meant to surface. Consider keying on whether the kill actually preceded a clean completion (e.g. tie timedOut to the kill having fired before the drain resolved), not on the numeric exit code alone.</comment>
<file context>
@@ -148,27 +146,41 @@ export async function runCommand(
+
+ // The timer can fire while a healthy process is already exiting, so a clean
+ // exit outranks it: only a kill that actually stopped the run counts.
+ const timedOut = killedByTimer && exitCode !== 0;
return { stdout, stderr, exitCode, projectDir, homeDir, timedOut };
</file context>
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.
Closes the gap the unit suite structurally cannot reach: there is no model in it, so nothing catches an agent that writes the wrong thing. Issue #58 was exactly that class, and I could only fix it blind.
Depends on #100 (merged into this branch, so the diff includes it until that lands).
What it does
Drives a real
opencodesession over a fixture project and asserts on the artifacts through the production loader. IfloadMindmodelrejects what the run generated, a real user could not have loaded it either.Deliberately one tier, not two. octto needed a stub provider because its deterministic coverage was thin; micode already has 489 unit tests plus a bundle test that loads the shipped artifact under both runtimes. The gap here is model behaviour specifically, which a stub cannot exercise by definition, since the script decides what the model does. So no
cdp.ts, noentrypoint.sh, no Dockerfile, no stub.It found a real bug on its first run
The model emitted this into the manifest:
A bare YAML scalar containing colon-space is invalid.
parseManifestthrows,loadMindmodelreturnsnull, and the entire generated mindmodel is dead while the run exits 0 and reports success, claiming it wrote 14 files and verified them.Worse than #58, which left things merely inconsistent. The prompt showed the manifest format but never said to quote descriptions, and any description naming a typed signature contains a colon. Fixed by requiring quotes and by making the format example itself contain a colon, so it demonstrates the escape rather than just asserting it.
Cost
Zero. opencode zen authenticates as
publicwith no credentials and leaves only zero-cost models enabled. Five full runs during development,cost: 0on every one, and the spec asserts that to keep it honest.MICODE_E2E_MODELopts into a paid model such asopencode/qwen3.6-pluswhen a specific one needs reproducing.Three traps worth knowing
Filename, not directory. Bun's discovery glob is
**{.test,.spec,_test_,_spec_}.{js,ts,jsx,tsx}. A spec namedmindmodel.test.tsinsidee2e/would still run on every PR and call a model. The.e2e.tssuffix is the protection;bun run checkcount is unchanged at 489.stdin must be closed. opencode blocks reading stdin to EOF when it is not a TTY, so spawning without
stdin: "ignore"hangs forever before the model is contacted.Config goes in HOME, not the project. micode resolves its agents' model through
loadDefaultModel(), which reads only the global config dir. A project-levelopencode.jsonsatisfies opencode but leaves micode's agents on the hardcodedopenai/gpt-5.2-codex, which then fails withModel not found. That is issue #52's root cause, reproduced live.Verification
Five live runs. Final: pass, 13 assertions, 11m55s. Timings varied (382s, 397s, 715s), so the budget is 25 minutes: a slow free model must not read as a product failure.
Two assertions cover #58 rather than one, because
warnNonMarkdownCategoriesonly inspects the manifest's path strings. A run writingstack/frontend.yamlwhile listingstack/frontend.mdraises nothing, so the disk is globbed separately.The fixture is deliberately shaped: three services sharing one error-handling shape to clear the "3+ instances" bar, exactly one deviating
legacy/report-export.tsfor the anti-pattern detector, and no frontend at all so "skip empty categories" is assertable.tests/e2e/fixture/**is excluded from biome and eslint. Not cosmetic: ESLint type-checkstests/**throughtsconfig.eslint.json, whose@/*maps to micode'ssrc/*, so the fixture's own@/domain/typesresolves to nothing and cascades into 17 phantom errors. It is test input data that must contain a rule-violating file by design.