chore(repo): mark M1 reached + clear 29 deferred-task hardening items#10
Merged
Conversation
…eached PR #9 merged: the OpenAI/DeepSeek (1.G) and Gemini (1.H) adapters + the ADR-0030 seam-shape amendment, conformance green for all three adapters (1.J) → milestone M1 is reached (2026-06-07). - phase-1: 1.G/1.H headers → Done (PR #9); 1.J → Done · M1 reached; banner + the in-phase M1 milestone row updated; next = 1.K (FallbackChain) ‖ 1.L engine. - roadmap README global milestone spine: M1 → ✅ achieved 2026-06-07. - current.md: date bumped; adapter lane moved to merged; M1 reached; next checkpoint M2. - README/CLAUDE/AGENTS status lines: Phase 0 (M0) → Phase 1 in progress, M1 reached. 1.K and the @relavium/core engine lane remain on the critical path toward M2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two items surfaced by the PR #9 comprehensive review had no tracked home: - Decision: `CapabilityFlags.vision: true` is advertised for Anthropic/OpenAI/ Gemini but `ContentPart` has no `image` arm, so vision is only reachable via the providerOptions escape hatch — decide on a first-class image arm (an ADR-0030-style seam amendment) vs. recording it as providerOptions-only. - CI: enable the reserved live-nightly conformance lane (adapters landed at M1; needs the `schedule:` lane uncommented + provider API keys as CI secrets). Also generalize the intro (items now come from the Phase-0 workflow OR a later per-PR review) and bump the date. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…askedSecret, agent schemas Clears confirmed deferred-tasks.md items whose files are already built (verified against current code by a per-item review): - D4: node:started.nodeType is now the ENGINE_NODE_TYPES enum (node-types.md is the canonical taxonomy), not a free string — an authored-only type (parallel) is rejected. - D5: export MaskedSecretSchema/MaskedSecret — the masked run:started input is a named contract. - S3: one O(n) findDuplicates helper in common.ts, shared by the agent + workflow dup-id checks. - S7: AgentSchema accepts optional input_schema/output_schema (agent-yaml-spec.md), locally defined to keep the agent↔node dependency one-way. - Test gaps (S2/T6/T7/T8): custom-merge node-level acceptance, edge branch-handle grammar, condition default/when + empty-transform invariants, RunSchema non-object inputs rejection. - Doc/comment accuracy (X4/X5/X6/X7) and the round-trip-fixture claim (T9) corrected. Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d S4) The shared contract is the provider-agnostic [0, 2] envelope; Anthropic's API caps temperature at 1. Reject temperature > 1 in the Anthropic adapter with a bad_request LlmError before egress (the clamp stays provider-local — the shared contract is unchanged), and pass a pre-egress guard error through the generate/stream catch unchanged rather than re-classifying it as an unknown SDK error. Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_mode (deferred T2/T3/T5) - T2: snapshot the 0000 migration DDL so a silent schema regeneration is caught (column name/type/notnull/default/pk + CHECK/index). - T3: insert a step_executions row with a non-existent run_id and assert the FK rejects (proves foreign_keys = ON). - T5: assert an in-memory DB keeps journal_mode = 'memory' (WAL is a no-op there). Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d S8) WorkflowInputSchema now rejects a validation key that doesn't apply to the input `type` (e.g. `min` on a string, `max_length` on a number) via a per-type matrix superRefine; bound-ordering stays on InputValidationSchema. The workflow-yaml-spec gains the per-type allowed-keys table. Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ols typecheck) - T1: cwd-tolerant coverage glob (**/src/**) so a package-scoped --coverage run no longer reports a false 0%; enforce the Phase-1 >=90% line+branch floor for packages/llm via a per-glob threshold (core enabled once it lands). X3: fix the inaccurate test-include comment (the build tsconfig DOES exclude *.spec.ts). - C2: incremental tsbuildinfo on the emitting build configs. - C3: typecheck the root/tooling *.config.ts via tsconfig.tools.json + a typecheck:tools step wired into the npm typecheck/ci scripts and CI. - C4: collapse a same-branch push + its PR into one CI concurrency group. - P1: drop declarationMap/sourceMap from the published build (no src in the tarball). Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Seam doc: annotate ContentPart/StopReason as @relavium/shared-owned, re-exported one-way by the seam (S6). workflow-yaml-spec per-type validation matrix (S8). - tech-stack Node-runtime row (X1); database-schema WAL single-writer wording (X2); phase-0 0.B reconciled to the implemented turbo ^build ordering, not composite (D6). - deferred-tasks.md: 29 built-package items checked off after the 2026-06-07 re-verification pass; S1/T4/C6 annotated with why they stay deferred. Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideMarks milestone M1 as reached in roadmap/status docs and resolves 29 previously deferred hardening tasks across shared schemas, LLM adapter, DB tests, tooling/CI config, and documentation, primarily by tightening validation contracts, adding tests, enforcing coverage/Node/runtime constraints, and clarifying ownership/semantics at the seam boundaries. Sequence diagram for Anthropic adapter temperature guardsequenceDiagram
actor Client
participant AnthropicAdapter
participant buildCommonBody
participant AnthropicSDK as Anthropic
Client->>AnthropicAdapter: generate(req, apiKey)
AnthropicAdapter->>buildCommonBody: buildCommonBody(req, apiKey)
alt [req.temperature > MAX_TEMPERATURE]
buildCommonBody-->>AnthropicAdapter: throw LlmProviderError
AnthropicAdapter-->>Client: propagate LlmProviderError
else [req.temperature <= MAX_TEMPERATURE]
buildCommonBody-->>AnthropicAdapter: body
AnthropicAdapter->>AnthropicSDK: messages.create(body, options)
AnthropicSDK-->>AnthropicAdapter: response
AnthropicAdapter-->>Client: LlmResult
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 (16)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR updates CI and tooling (typecheck tooling and coverage), adds adapter guards and secret-scrubbing, tightens shared Zod schemas and workflow validation (including agent $ref support), enforces strict local config schemas, records multimodal ADRs/IPC changes, and expands tests and documentation. ChangesPhase 1 M1 completion and hardening
Estimated code review effort: Possibly related PRs:
Poem:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request marks the completion of Phase 1 Milestone M1 (LLM seam proven) by updating project documentation, resolving numerous deferred tasks, and introducing tooling typechecks. Key code changes include adding type-scoped validation checks for workflow inputs, supporting optional schema metadata in agents, enforcing Anthropic-specific temperature limits, and adding database snapshot and constraint tests. Feedback on these changes highlights a potential runtime crash in the workflow input validation schema if the input type is missing or invalid, as well as a non-CWD-tolerant coverage threshold glob in the Vitest 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.
Comment on lines
+121
to
+122
| const allowed = KEYS_BY_TYPE[input.type]; | ||
| for (const key of Object.keys(input.validation)) { |
There was a problem hiding this comment.
In WorkflowInputSchema's superRefine, KEYS_BY_TYPE[input.type] is accessed directly. If input.type is invalid or missing (which can happen during partial validation or if there are other parsing errors), KEYS_BY_TYPE[input.type] will be undefined. This will cause allowed.includes(key) to throw a TypeError: Cannot read properties of undefined (reading 'includes'), crashing the entire parsing process instead of returning a clean Zod validation error. Adding a defensive check to return early if allowed is undefined prevents this unhandled runtime crash.
const allowed = KEYS_BY_TYPE[input.type];
if (!allowed) {
return;
}
for (const key of Object.keys(input.validation)) {
Comment on lines
+34
to
+35
| 'packages/llm/src/**/*.ts': { lines: 90, branches: 90 }, | ||
| // 'packages/core/src/**/*.ts': { lines: 90, branches: 90 }, // enable once core lands (1.L+) |
There was a problem hiding this comment.
The coverage threshold glob 'packages/llm/src/**/*.ts' is not CWD-tolerant. When running Vitest from within the packages/llm directory (which is common during local package development), the relative path of the files is resolved as src/**/*.ts (without the packages/llm/ prefix). As a result, the 90% coverage threshold won't be matched or enforced. Prefixing the glob with **/ makes it CWD-tolerant and ensures the threshold is always enforced regardless of where the test runner is invoked.
Suggested change
| 'packages/llm/src/**/*.ts': { lines: 90, branches: 90 }, | |
| // 'packages/core/src/**/*.ts': { lines: 90, branches: 90 }, // enable once core lands (1.L+) | |
| '**/packages/llm/src/**/*.ts': { lines: 90, branches: 90 }, | |
| // '**/packages/core/src/**/*.ts': { lines: 90, branches: 90 }, // enable once core lands (1.L+) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
WorkflowInputSchema.superRefine, consider hoisting theKEYS_BY_TYPEmap to a module-level constant and typing it againstInputTypeSchema(e.g.,Record<z.infer<typeof InputTypeSchema>, readonly (keyof InputValidation)[]>) so you avoid re‑allocating it per call and get better compiler help if new input types or validation keys are added. - The new
SchemaMetadataSchemainagent.tsduplicates the JSON-Schema-subset concept fromnode.ts; it could be worth centralizing a shared definition (e.g., incommon.ts) once the engine side lands so the agent- and node-level schema metadata stay in lock-step without introducing a circular dependency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WorkflowInputSchema.superRefine`, consider hoisting the `KEYS_BY_TYPE` map to a module-level constant and typing it against `InputTypeSchema` (e.g., `Record<z.infer<typeof InputTypeSchema>, readonly (keyof InputValidation)[]>`) so you avoid re‑allocating it per call and get better compiler help if new input types or validation keys are added.
- The new `SchemaMetadataSchema` in `agent.ts` duplicates the JSON-Schema-subset concept from `node.ts`; it could be worth centralizing a shared definition (e.g., in `common.ts`) once the engine side lands so the agent- and node-level schema metadata stay in lock-step without introducing a circular dependency.
## Individual Comments
### Comment 1
<location path="packages/shared/src/workflow.ts" line_range="106-109" />
<code_context>
validation: InputValidationSchema.optional(),
})
- .strict();
+ .strict()
+ // Per-type validation-key compatibility (workflow-yaml-spec.md): a numeric bound on a string, or a
+ // *_length on a number, is an authored mistake — reject it. (Bound-ordering is on InputValidationSchema.)
+ .superRefine((input, ctx) => {
+ if (!input.validation) {
+ return;
</code_context>
<issue_to_address>
**suggestion (performance):** Hoist KEYS_BY_TYPE out of the superRefine to avoid per-input allocation and keep the mapping reusable.
Since superRefine runs for every input, rebuilding KEYS_BY_TYPE each time adds unnecessary allocation. Consider defining KEYS_BY_TYPE as a top-level constant near InputTypeSchema so it can be reused (including by future schemas like NodeSchema/AgentSchema) and more easily kept in sync with workflow-yaml-spec.md and unit tests.
Suggested implementation:
```typescript
import { AgentSchema } from './agent.js';
import { NodeSchema } from './node.js';
const KEYS_BY_TYPE: Record<z.infer<typeof InputTypeSchema>, readonly string[]> = {
number: ['min', 'max', 'enum'],
string: ['format', 'pattern', 'enum', 'min_length', 'max_length'],
file_path: ['format', 'pattern', 'enum', 'min_length', 'max_length'],
code_diff: ['format', 'pattern', 'enum', 'min_length', 'max_length'],
secret: ['format', 'pattern', 'enum', 'min_length', 'max_length'],
boolean: [],
};
```
```typescript
.strict()
// Per-type validation-key compatibility (workflow-yaml-spec.md): a numeric bound on a string, or a
// *_length on a number, is an authored mistake — reject it. (Bound-ordering is on InputValidationSchema.)
.superRefine((input, ctx) => {
if (!input.validation) {
return;
}
const allowed = KEYS_BY_TYPE[input.type];
```
If `InputTypeSchema` is declared in a different module or below these imports, ensure it is exported or in scope so that the top-level `KEYS_BY_TYPE` type annotation remains valid. If you later introduce similar schemas (e.g., `NodeSchema`/`AgentSchema`) that need this mapping, reference the same `KEYS_BY_TYPE` constant rather than re-encoding the per-type keys, to keep it aligned with `workflow-yaml-spec.md` and tests.
</issue_to_address>
### Comment 2
<location path="docs/roadmap/current.md" line_range="87" />
<code_context>
-The next checkpoint is global milestone **M1 — LLM seam proven** (see the
-[milestone spine](README.md#global-milestone-spine)).
+Global milestone **M1 — LLM seam proven is reached** (PR #9, 2026-06-07): all three
+adapters pass the shared conformance suite behind the frozen seam. The next checkpoint is
+**M2 — engine end-to-end** (see the [milestone spine](README.md#global-milestone-spine)).
</code_context>
<issue_to_address>
**nitpick (typo):** Rephrase 'M1 — LLM seam proven is reached' for smoother grammar
This reads slightly awkward. Please either move “is reached” outside the bold (e.g. `Global milestone **M1 — LLM seam proven** is reached ...`) or separate it with another dash inside the bold (e.g. `**M1 — LLM seam proven — is reached**`) to improve the sentence flow.
```suggestion
Global milestone **M1 — LLM seam proven** is reached (PR #9, 2026-06-07): all three
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/shared/src/constants.ts (1)
2-8: ⚡ Quick winDocumentation categorization: ENGINE_NODE_TYPES is a directly validated vocabulary, not a parallel authoritative list.
The comment groups "node-type taxonomies" (plural) as parallel authoritative lists, but
ENGINE_NODE_TYPES(added at lines 108-127) is consumed directly viaz.enum(ENGINE_NODE_TYPES)inrun-event.tsline 134, making it a directly validated vocabulary rather than a parallel list. OnlyWORKFLOW_NODE_TYPESfits the "parallel authoritative list" pattern (discriminated union withz.literalper member).Consider clarifying the comment to distinguish the two node-type constants:
- Parallel:
WORKFLOW_NODE_TYPES(mirrored inNodeSchemawithz.literalper variant)- Direct:
ENGINE_NODE_TYPES(consumed viaz.enum)📝 Suggested documentation fix
- * - **Parallel authoritative lists** — run-event names and the node-type taxonomies are a + * - **Parallel authoritative lists** — run-event names and the **authored** node-type taxonomy are a * single source of truth that the discriminated unions re-declare by hand (a `z.literal` - * per member); tests pin the two in lock-step so they never drift. + * per member); tests pin the two in lock-step so they never drift. The **engine** node-type + * taxonomy is a directly validated vocabulary (consumed via `z.enum`).🤖 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/shared/src/constants.ts` around lines 2 - 8, Update the top-of-file comment in constants.ts to correctly categorize the node-type constants: state that WORKFLOW_NODE_TYPES is a "parallel authoritative list" mirrored by NodeSchema via z.literal per variant, while ENGINE_NODE_TYPES is a "directly validated vocabulary" consumed by z.enum (as used in run-event.ts). Mention both constant names (WORKFLOW_NODE_TYPES and ENGINE_NODE_TYPES) explicitly so readers know which pattern each follows.
🤖 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 `@docs/roadmap/phases/phase-1-engine-and-llm.md`:
- Line 10: The status line for "🎯 M1 — LLM seam proven, is reached." contains
awkward grammar; update the M1 status sentence in the document (the line
introduced in PR `#9` referencing "M1 — LLM seam") to a clean phrasing such as "🎯
M1 — LLM seam has been proven." or simply "🎯 M1 — LLM seam proven." so it reads
clearly and grammatically; locate the string "M1 — LLM seam proven" in the file
and replace the existing clause accordingly.
- Line 705: Update the M1 milestone table row that currently reads "LLM seam
proven: 3 adapters pass the conformance suite (fixtures on PR, live nightly; no
vendor type across the seam)" so it does not imply live-nightly is already
established: change the parenthetical to something like "(fixtures on PR;
live-nightly pending/blocked; no vendor type across the seam)" or similar
wording that marks the live-nightly lane as pending. Edit the table cell
containing the exact string "LLM seam proven: 3 adapters pass the conformance
suite (fixtures on PR, live nightly; no vendor type across the seam)" to replace
"live nightly" with a clear pending status.
In `@packages/shared/src/run-event.ts`:
- Around line 65-71: Make MaskedSecretSchema strict to prevent extra fields (and
potential secret leakage): change the schema declaration for MaskedSecretSchema
(the z.object({ secret: z.literal(true), ref: nonEmptyString })) to use
.strict(), and add a unit test that asserts MaskedSecretSchema.safeParse({
secret: true, ref: 'x', raw_value: 'sk-...' }).success is false (e.g., in
run-event.test.ts) to ensure extra fields are rejected.
---
Nitpick comments:
In `@packages/shared/src/constants.ts`:
- Around line 2-8: Update the top-of-file comment in constants.ts to correctly
categorize the node-type constants: state that WORKFLOW_NODE_TYPES is a
"parallel authoritative list" mirrored by NodeSchema via z.literal per variant,
while ENGINE_NODE_TYPES is a "directly validated vocabulary" consumed by z.enum
(as used in run-event.ts). Mention both constant names (WORKFLOW_NODE_TYPES and
ENGINE_NODE_TYPES) explicitly so readers know which pattern each follows.
🪄 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: e0cbf3aa-381d-485e-90c3-2fbb24e4b040
⛔ Files ignored due to path filters (1)
packages/db/src/__snapshots__/client.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
.github/workflows/ci.ymlAGENTS.mdCLAUDE.mdREADME.mddocs/reference/contracts/workflow-yaml-spec.mddocs/reference/desktop/database-schema.mddocs/reference/shared-core/llm-provider-seam.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-0-foundations.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/tech-stack.mdpackage.jsonpackages/db/src/client.test.tspackages/db/tsconfig.build.jsonpackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/tsconfig.build.jsonpackages/shared/src/agent.test.tspackages/shared/src/agent.tspackages/shared/src/common.tspackages/shared/src/constants.tspackages/shared/src/edge.test.tspackages/shared/src/node.test.tspackages/shared/src/node.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.tspackages/shared/src/run.test.tspackages/shared/src/workflow.test.tspackages/shared/src/workflow.tspackages/shared/tsconfig.build.jsontsconfig.tools.jsonvitest.config.ts
…matrix, centralize schema - MaskedSecretSchema is now .strict() so a raw secret value can never ride alongside the masked shape; test pins the extra-field rejection. - WorkflowInputSchema: hoist the per-type validation-key matrix to a module constant (allocated once, typed against InputTypeSchema/InputValidation), and bail defensively when `type` failed its own enum check so the refine can't crash on an undefined key list. - Centralize the JSON-Schema-subset metadata map in common.ts (jsonSchemaMetadataSchema), shared by node.ts OutputSchemaSchema and the agent input/output_schema fields — removes the duplicate definition without an agent↔node cycle. - constants.ts header: ENGINE_NODE_TYPES is a z.enum-validated vocabulary (node:started), WORKFLOW_NODE_TYPES the hand-redeclared NodeSchema list — corrected after the enum change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Smooth the awkward "M1 — LLM seam proven is reached" grammar in current.md and the phase-1 banner. - The M1 milestone-table row no longer implies the live-nightly lane is established: mark it "reserved/pending keys", consistent with the deferred-tasks follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/roadmap/phases/phase-1-engine-and-llm.md (1)
10-11: ⚡ Quick winConsider aligning the live-nightly wording with line 705 for consistency.
Line 11 says "live nightly reserved, pending keys" while line 705 (the milestone table) uses the clearer phrasing "live-nightly lane reserved/pending keys." The latter makes it more explicit that the lane itself has pending status.
Suggested alignment
-> in **PR `#9`** (2026-06-07) — **🎯 M1 (LLM seam proven) is reached.** All three adapters pass one shared -> conformance suite in fixture mode (live nightly reserved, pending keys); no vendor type crosses the +> in **PR `#9`** (2026-06-07) — **🎯 M1 (LLM seam proven) is reached.** All three adapters pass one shared +> conformance suite in fixture mode (live-nightly lane reserved/pending keys); no vendor type crosses the🤖 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-1-engine-and-llm.md` around lines 10 - 11, Update the milestone text under "🎯 M1 (LLM seam proven) is reached." to match the milestone table phrasing by replacing "live nightly reserved, pending keys" with "live-nightly lane reserved/pending keys" so the lane/pending status is explicit and hyphenation is consistent with the table entry; locate the string in the document near the M1 heading and change it accordingly.
🤖 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.
Nitpick comments:
In `@docs/roadmap/phases/phase-1-engine-and-llm.md`:
- Around line 10-11: Update the milestone text under "🎯 M1 (LLM seam proven) is
reached." to match the milestone table phrasing by replacing "live nightly
reserved, pending keys" with "live-nightly lane reserved/pending keys" so the
lane/pending status is explicit and hyphenation is consistent with the table
entry; locate the string in the document near the M1 heading and change it
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3e7e311-07b1-421e-a61c-ac0c236bb258
📒 Files selected for processing (10)
docs/roadmap/current.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/shared/src/agent.tspackages/shared/src/common.tspackages/shared/src/constants.tspackages/shared/src/node.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.tspackages/shared/src/workflow.test.tspackages/shared/src/workflow.ts
✅ Files skipped from review due to trivial changes (1)
- docs/roadmap/current.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/run-event.test.ts
- packages/shared/src/constants.ts
- packages/shared/src/run-event.ts
…ecisions)
- D1 (keep the door open): WorkflowSpec.agents now accepts z.union([AgentSchema,
AgentRefSchema]) — an external `{ $ref: './x.agent.yaml' }` entry parses, matching the
spec. File/path resolution + path-traversal hardening stay the engine's job (the pure,
sync shared schema never reads files); the duplicate-id check skips $ref entries.
- S5 (fail loud): GlobalConfigSchema / ProjectConfigSchema / ChatConfigSchema (and their
nested preferences/defaults objects) are now .strict() — a typo'd committed config key is
rejected at parse, parity with the authored-YAML schemas (ADR-0023).
Refs: deferred-tasks.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`engines` was advisory; enforce it (decided) — an unsupported runtime now fails install with a clear message instead of surfacing as confusing errors later. Pinned versions: .nvmrc (Node 22), package.json engines (Node >=20.11.0, pnpm >=9). Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…turbo inputs) - code-style-typescript.md: ids stay plain `string` (deliberate) — branding adds cross-seam friction for little payoff; revisit via ADR if a real id-mixup class appears (D2). - deferred-tasks.md: rule the seven open decisions — $ref (door kept open), config .strict(), engine-strict, branded ids, turbo inputs (keep default) all settled; LICENSE stays open (HodeTech drafting its own commercial license); the vision item is reframed as a decided direction — **first-class multimodal I/O (image/audio/video, in AND out)** needing a design analysis + ADR + seam amendment before code. Refs: deferred-tasks.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/config.test.ts (1)
48-57: ⚡ Quick winMissing test case for
defaultsobject strictness.The test covers typos in
GlobalConfigSchema(top-level andpreferences),ProjectConfigSchematop-level, andchatnested keys — butdefaultswas also made.strict()inconfig.ts(line 113). Consider adding a case for adefaultstypo:expect(ProjectConfigSchema.safeParse({ defaults: { modell: 'x' } }).success).toBe(false); // nested typo in defaults🤖 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/shared/src/config.test.ts` around lines 48 - 57, Add a test asserting that ProjectConfigSchema rejects unknown keys inside the defaults object: create a safeParse call against ProjectConfigSchema with a payload containing defaults: { modell: 'x' } (note the typo) and expect .success toBe(false); this ensures the defaults object (made .strict() in config) is covered — look for ProjectConfigSchema and the defaults key to locate where to add the assertion in the existing test.packages/shared/src/config.ts (1)
24-65: 💤 Low value
McpServerRegistrationSchemais not strict — inconsistent with the "fail loud" philosophy?The three top-level config schemas and their nested
preferences/defaultsobjects are now strict, butMcpServerRegistrationSchemaremains non-strict. A typo in an MCP server entry (e.g.,autostrat: trueinstead ofautostart) would still be silently dropped.If this is intentional (e.g., forward-compatibility for MCP tooling), consider adding a brief comment noting the exception. Otherwise, adding
.strict()here would complete the "fail loud" parity.🤖 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/shared/src/config.ts` around lines 24 - 65, McpServerRegistrationSchema is not declared strict and will silently ignore unknown keys; update the schema by calling .strict() on the z.object (e.g., change z.object({...}) to z.object({...}).strict()) before .superRefine so typos like "autostrat" fail loudly; if non-strict behavior was intentional, instead add a concise comment above McpServerRegistrationSchema explaining the exception/forward-compatibility rationale.
🤖 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.
Nitpick comments:
In `@packages/shared/src/config.test.ts`:
- Around line 48-57: Add a test asserting that ProjectConfigSchema rejects
unknown keys inside the defaults object: create a safeParse call against
ProjectConfigSchema with a payload containing defaults: { modell: 'x' } (note
the typo) and expect .success toBe(false); this ensures the defaults object
(made .strict() in config) is covered — look for ProjectConfigSchema and the
defaults key to locate where to add the assertion in the existing test.
In `@packages/shared/src/config.ts`:
- Around line 24-65: McpServerRegistrationSchema is not declared strict and will
silently ignore unknown keys; update the schema by calling .strict() on the
z.object (e.g., change z.object({...}) to z.object({...}).strict()) before
.superRefine so typos like "autostrat" fail loudly; if non-strict behavior was
intentional, instead add a concise comment above McpServerRegistrationSchema
explaining the exception/forward-compatibility rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 793aaa6d-f053-4af5-9d18-acc9cdbae43f
📒 Files selected for processing (7)
.npmrcdocs/roadmap/deferred-tasks.mddocs/standards/code-style-typescript.mdpackages/shared/src/config.test.tspackages/shared/src/config.tspackages/shared/src/workflow.test.tspackages/shared/src/workflow.ts
✅ Files skipped from review due to trivial changes (1)
- .npmrc
An MCP registration was the only object in config.ts not declared .strict(), so a typo in a committed key (e.g. `autostrat` for `autostart`) was silently dropped instead of failing loudly. Add .strict() for ADR-0023 parity with the rest of the file, and cover an unknown key inside the strict `defaults` object the typo test had missed. Refs: ADR-0023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The M1 status line read "live nightly reserved, pending keys" while the milestones table reads "live-nightly lane reserved/pending keys". Match the table wording so the lane/pending status and hyphenation are consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deep design analysis (image/audio/video, input AND output) for the @relavium/llm seam, decided as a first-class capability per the maintainer. Produced by a multi-phase analysis (provider research → 3 design proposals → judge panel → synthesis → 3 adversarial reviews) and revised to resolve every blocking review finding, verified against the codebase + pinned SDKs. Core of the decided design: - A media ContentPart arm split like MaskedSecret: an in-flight part may inline base64 (sub-ceiling), a distinct durable/persisted part is handle-only — the compiler proves no bytes reach a durable schema. - A single engine-owned deInlineMedia choke point (the leak-bearing event/output fields are z.unknown(), so leak-freedom is an active emit-time pass, not "by construction"); a content-addressed MediaStore (CAS) holds bytes. - providerRef (Files-API id) is an ephemeral sidecar (ADR-0030 reasoning-signature discipline), never persisted; outputCombinations: ModalitySet[] drives per-model output modalities; raw media is forbidden in tool_result.result; SSRF binds input + output URLs; managed mode reconciled with ADR-0015; the desktop Rust-IPC path needs an ADR-0018 amendment. Status: Draft for maintainer review (would land as ADR-0031, amending ADR-0011's seam shape like ADR-0030). Open questions for the maintainer are listed in §11. Refs: ADR-0011, ADR-0030, deferred-tasks.md (multimodal item) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/analysis/multimodal-io-design-2026-06-07.md (1)
785-960: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove the ADR body to
docs/decisions/0031-...mdand keep this file as analysis-only.Keeping a full ADR draft embedded here creates a second canonical source and invites divergence once edits begin. Please place the ADR text in
docs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mdand replace this section with a short summary + relative link.Based on learnings: “Record non-trivial decisions as ADRs in
docs/decisions/using condensed MADR form” and “One canonical home per artifact.”🤖 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/analysis/multimodal-io-design-2026-06-07.md` around lines 785 - 960, Move the full ADR draft body currently embedded under the ADR-0031 header into a new canonical ADR file named docs/decisions/0031-llm-seam-shape-amendment-multimodal-io.md preserving the metadata (Status, Date, Related) and all sections; in this file keep the content exactly as the draft. In the original analysis file replace the moved body with a short summary paragraph and a relative link pointing to the new docs/decisions/0031-llm-seam-shape-amendment-multimodal-io.md, and ensure any internal references to ADR-0031 (the header, "ADR-0031", and referenced invariants or guardrails) now point readers to the new canonical file. Ensure no duplicate canonical content remains in the analysis file and that the new ADR file includes the condensed MADR format and the same decision text.Source: Learnings
🧹 Nitpick comments (1)
docs/analysis/multimodal-io-design-2026-06-07.md (1)
151-731: ⚡ Quick winAdd a Mermaid diagram for the media flow and de-inline boundary.
This section is dense and architecture-critical; adding one Mermaid diagram (request/input normalization → adapter →
deInlineMedia→ durable/event surfaces, plus desktop IPC branch) will make the guardrail model auditable and align with doc standards.As per coding guidelines: “Use English, kebab-case naming, relative links, Mermaid diagrams, and Conventional Commits across all documentation and code.”
🤖 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/analysis/multimodal-io-design-2026-06-07.md` around lines 151 - 731, Add a Mermaid flow diagram depicting request/input normalization → adapter → deInlineMedia → durable/event surfaces, including the desktop IPC branch and ADR-0018 amendment; place it near the de-inline-media discussion (section 3.7/6.5) and reference the key symbols INLINE_MEDIA_CEILING, MediaSource/ DurableMediaSource, MediaPartSchema/DurableMediaPartSchema, deInlineMedia, StreamChunk(media_start/media_delta/media_end) and the Desktop ADR-0018 IPC note so reviewers can audit the exact boundary, and follow kebab-case naming and a relative link to ADR-0018 per doc guidelines.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@docs/analysis/multimodal-io-design-2026-06-07.md`:
- Around line 785-960: Move the full ADR draft body currently embedded under the
ADR-0031 header into a new canonical ADR file named
docs/decisions/0031-llm-seam-shape-amendment-multimodal-io.md preserving the
metadata (Status, Date, Related) and all sections; in this file keep the content
exactly as the draft. In the original analysis file replace the moved body with
a short summary paragraph and a relative link pointing to the new
docs/decisions/0031-llm-seam-shape-amendment-multimodal-io.md, and ensure any
internal references to ADR-0031 (the header, "ADR-0031", and referenced
invariants or guardrails) now point readers to the new canonical file. Ensure no
duplicate canonical content remains in the analysis file and that the new ADR
file includes the condensed MADR format and the same decision text.
---
Nitpick comments:
In `@docs/analysis/multimodal-io-design-2026-06-07.md`:
- Around line 151-731: Add a Mermaid flow diagram depicting request/input
normalization → adapter → deInlineMedia → durable/event surfaces, including the
desktop IPC branch and ADR-0018 amendment; place it near the de-inline-media
discussion (section 3.7/6.5) and reference the key symbols INLINE_MEDIA_CEILING,
MediaSource/ DurableMediaSource, MediaPartSchema/DurableMediaPartSchema,
deInlineMedia, StreamChunk(media_start/media_delta/media_end) and the Desktop
ADR-0018 IPC note so reviewers can audit the exact boundary, and follow
kebab-case naming and a relative link to ADR-0018 per doc guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 854ab3de-bd50-474f-8056-ba0f4a7775d6
📒 Files selected for processing (4)
docs/analysis/multimodal-io-design-2026-06-07.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/shared/src/config.test.tspackages/shared/src/config.ts
✅ Files skipped from review due to trivial changes (1)
- docs/roadmap/phases/phase-1-engine-and-llm.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/config.test.ts
- packages/shared/src/config.ts
Land the multimodal-I/O seam amendment as a pre-freeze decision in the ADR-0030 mould, with the design finalization, roadmap scheduling, and a review pass. Decisions & ADRs: - ADR-0031 (@relavium/llm seam-shape amendment): media ContentPart/StreamChunk arms, CapabilityFlags.media (input image/audio/video/document + outputCombinations), Usage.mediaUnits, LlmRequest.outputModalities, reserved generateMedia?/pollMediaJob?, and the MediaStore/deInlineMedia/handle model. Records maintainer decisions A1-A9 and the five defaulted open questions. - ADR-0032: desktop Rust-side media de-inline on the egress path, amending ADR-0018 via an additive dated note + Related link (append-only history preserved). Design analysis (multimodal-io-design-2026-06-07.md): - A1-A9 threaded inline; section-11 open questions resolved to decisions/defaults; embedded ADR draft replaced by a section-12 pointer (one canonical home); document=0 inline ceiling (A2/OQ4); a section-3.7 de-inline-boundary Mermaid. Roadmap: - Multimodal sub-spine 1.AD-1.AH (1.m6), with 1.AD landing before the exhaustive consumers 1.K/1.O; seven forward-obligations in deferred-tasks; the desktop ADR-0032 obligation threaded into phase-3. Review pass (multi-agent): 0 blockers/majors; 12 consistency/wording nits resolved (document propagation, section refs, decision tags, date, dependency-matrix edges). Refs ADR-0031, ADR-0032. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The secret-free-LlmError invariant was already tested per-adapter (each adapter has a "never leaks the API key into the surfaced LlmError" test), but it rested on per-adapter discipline and makeLlmError's `message` was only *documented* "already redacted". Add scrubSecrets(), a pure pattern-based backstop applied at the one makeLlmError choke point (structural, not discipline — like the engine's deInlineMedia): it masks API-key prefixes (sk-/sk-proj-/sk-svcacct-/sk-ant-/AIza), URL userinfo, secrets in URL query strings, and Bearer headers, and leaves benign text untouched. Every adapter now also benefits centrally. Unit-tested (positive shapes, benign + boundary negatives, and the makeLlmError integration); all 204 @relavium/llm tests green; typecheck + lint clean. Refs: ADR-0011, security-review.md (secret-free LlmError obligation) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… + standards/roadmap Records the maintainer rulings and forward-obligations from a hardening-analysis pass over the seam / fallback / byte-delivery / security surfaces (validated against the current ADRs; each stands on Relavium's own architecture). A follow-up multi-dimensional review refined accuracy (no blockers). Decisions: - Y3 (media-arm integrity metadata): ADR-0031 amended (dated note) — DurableMediaPart gains an optional byteLength? + audio/video durationMs?; NO checksum (the media://sha256 handle IS the checksum); width/height excluded (render-only). Must land in the 1.AD seam shape. Design analysis 3.2 + deferred-tasks + 1.AD task body updated. - Y2 (score/partial-success fallback): deliberately OUT-OF-SCOPE for Phase 1 (an author/node concern, not the lowest-common-denominator seam / 1.K) — deferred-tasks. - TLS granularity: deferred draft-proposal (keep the global never-disable stance) — deferred-tasks. Standards / skill: - security-review.md: secret-free LlmError as a positive *tested* obligation (asserted by the per-adapter adapter tests + llm-error.test.ts, not the conformance suite); sent-secret redacted in response/error logs; new "Media byte delivery" section (Range/offset/upload-size, single read_media gate); media url-carrier as a reserved 4th SSRF egress path; CGNAT (100.64/10) added to the range enumeration; read_media + media url-carrier added to the mandatory-review list. - testing.md: new "Security-critical primitive tests (direct, negative-case)" section. - logging-and-observability.md: echoed-back-secret redaction clause. - add-llm-adapter skill: secret-free-LlmError done-criterion + dependency-provenance/default-off. Roadmap forward-obligations (phase-1 acceptance): - terminal-event totality (1.N), pure-function / no-string-sentinel fallback (1.K), parser purity + config-strict note (1.L), input-mapping taint + SSRF for outbound-URL tools (1.T), and the multimodal sub-spine extensions (shared SSRF primitive + direct tests at 1.AE; Range/keychain/ terminal-state media sweep at 1.AF; LRO cancel at 1.AG; single read_media gate at 1.AH); derived-URL SSRF re-check. Refs: ADR-0031, ADR-0011, ADR-0028, ADR-0029 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The scrubSecrets test additions in 88fe9a0 had long expect(...) lines that failed the `turbo run format:check` (prettier) CI gate. Prettier-wrapped them; no logic change, tests still green. (eslint + typecheck were run pre-commit but format:check was missed.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-class smell)
scrubSecrets' Bearer pattern used a dense character class [A-Za-z0-9\-._~+/] that tripped
Sonar S5869 ("Remove duplicates in this character class", llm-error.ts:90). On inspection
the class had no real duplicate codepoint (A-Z/a-z/0-9 ranges + the literals -._~+/, each
once), but the dense class is unnecessary: a Bearer token is opaque, so matching \S+ (any
non-whitespace up to the next space) is simpler, removes the class entirely (S5869 cannot
fire), subsumes the =* padding, and is a safer redaction backstop (catches any token shape,
not just base64url). Tests + typecheck + lint + format:check green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/analysis/multimodal-io-design-2026-06-07.md`:
- Around line 3-4: The blockquote starting with "Status: Reviewed and decided —
finalized as [ADR-0031]... + [ADR-0032]..." contains an empty blank line which
triggers MD028; remove the blank line inside that blockquote so the lines
beginning with ">" are contiguous (i.e., ensure the entire blockquote content is
continuous without an intervening blank line) so the markdownlint rule passes.
In `@docs/roadmap/deferred-tasks.md`:
- Line 5: Update the header "Last updated: 2026-06-08" to "Last updated:
2026-06-09" so it matches the newest dated entries in the document (see the
dated entries around the blocks referenced, e.g., the lines containing
2026-06-09 at or near line markers 87 and 127); also scan for any other "Last
updated" occurrences in the same file and bump them to 2026-06-09 to avoid
status drift.
In `@packages/llm/src/llm-error.test.ts`:
- Around line 102-104: The test embeds a contiguous key-shaped literal which
triggers scanners; update the test that calls scrubSecrets so the input string
is built at runtime (e.g., concatenate parts or insert a non-key character) or
replace the literal with an obfuscated placeholder before calling scrubSecrets,
and keep the same assertion that scrubSecrets('...') does not return the key
token; locate the expectation using scrubSecrets and the string containing
"AIza..." in the test and change the input construction so no contiguous
key-like token appears in source while preserving the runtime behavior and
assertion.
In `@packages/llm/src/llm-error.ts`:
- Line 85: The URL userinfo redaction in the replace call inside LlmError
message normalization only matches user:pass@ and misses username-only userinfo
like https://token@host; update the replace pattern used in
packages/llm/src/llm-error.ts (the .replace(...) that redacts URL userinfo) so
it matches either username or username:password before the @ and replaces that
whole userinfo portion with [REDACTED]@, leaving the rest of the URL intact;
implement this by widening the regex to accept an optional :password group
(i.e., match one or more non-separators followed optionally by : and more
non-separators, then @) and keep the same replacement token.
🪄 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: 06fff26c-da26-47ba-b9a8-6b1726bcc609
📒 Files selected for processing (15)
.claude/skills/add-llm-adapter/SKILL.mddocs/analysis/multimodal-io-design-2026-06-07.mddocs/decisions/0018-desktop-execution-and-rust-egress.mddocs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mddocs/decisions/0032-desktop-rust-media-de-inline-amends-0018.mddocs/decisions/README.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/roadmap/phases/phase-3-desktop.mddocs/standards/logging-and-observability.mddocs/standards/security-review.mddocs/standards/testing.mdpackages/llm/src/llm-error.test.tspackages/llm/src/llm-error.ts
✅ Files skipped from review due to trivial changes (3)
- docs/decisions/0018-desktop-execution-and-rust-egress.md
- docs/decisions/README.md
- docs/roadmap/phases/phase-1-engine-and-llm.md
PR #10 review (adversarial): - The Anthropic temperature guard only checked `> 1`, so NaN and negative values (NaN > 1 and -0.5 > 1 are both false) were forwarded to a guaranteed 400. Reject !finite/<0/>max before egress; cover generate (negative + NaN) and the stream path (error chunk, no transport). - scrubSecrets: the URL-userinfo pattern matched only `user:pass@`, missing username-only `https://token@host` — widen the password to optional; add a `Basic <token>` pattern alongside `Bearer`. Obfuscate the key-shaped test literals (built at runtime) so secret scanners don't flag the fixtures. Note that `cause` is internal-only and never on the run-event boundary. All 206 @relavium/llm tests green; typecheck + lint + format:check clean. Refs: ADR-0011, security-review.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nfig carve-out) PR #10 review flagged that config.ts's `.strict()` on Global/Project/Chat/MCP config reverses ADR-0023's explicit "config files left lenient" scope decision with no superseding ADR — a non-negotiable #9 (append-only) gap, with the code comments citing ADR-0023 as if it supported the change. The decision (fail-loud config, maintainer S5) is correct and matches config-spec.md (config is a stable, versioned contract; breaking changes need a migration path, not silent reinterpretation), so this records it properly: - ADR-0033 amends ADR-0023's config-scope carve-out (config schemas are strict too; the authored-YAML decision and RunEvent/RunSchema leniency are unchanged), reconciled with config-spec.md. - Append-only dated "> Amended by ADR-0033" note on ADR-0023 (body unchanged); ADR-0033 indexed. - Repoint the three "ADR-0023 parity" comments in config.ts to ADR-0033. Refs: ADR-0023, config-spec.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ding, MD028, dates) Verified-and-valid review nits, minimal fixes: - ci.yml: add `github.event_name` to the concurrency group so a push and its same-SHA PR no longer collapse into one group (a cancelled run no longer risks blocking the required check). - vitest.config.ts: stop claiming the >=90% coverage floor is CI-enforced — it runs only under `pnpm coverage` from the repo root (threshold glob is root-relative); CI-coverage is pending. - tsconfig.tools.json: comment claimed it typechecks `tools/**`; it doesn't (the tools/ lint fixtures are intentionally broken/excluded) — corrected. - ipc-contract.md: forward note that ADR-0032 (media de-inline frame + read_media) amends this raw-chunk contract at 1.AH; the ADR records the decision, the contract isn't updated yet. - security-review.md: scope the secret-free claim to "no credentials-in-URL" (a public endpoint URL is not itself a secret). - multimodal-io-design.md: fix MD028 (blank line inside the header blockquote). - deferred-tasks.md: bump "Last updated" to 2026-06-09. - workflow.ts: comment that `secret` shares `string`'s validation keys (a secret is a string). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/config.test.ts (1)
60-107: ⚡ Quick winConsider adding an explicit test for unknown keys in MCP server registrations.
The existing MCP tests validate required fields, URL format, SSRF guards, and credential checks, but don't explicitly test that unknown keys in
[[mcp_servers]]entries are rejected by.strict(). While the schema is strict (line 35 in config.ts) and this should work transitively through the array, an explicit test would be consistent with the pattern at lines 48-58 and would document this behavior clearly.✅ Suggested test case
Add after line 107:
it('rejects an unknown key in an MCP server registration (strict — ADR-0033)', () => { expect( GlobalConfigSchema.safeParse({ mcp_servers: [{ name: 'x', transport: 'stdio', command: 'cmd', autostrat: true }], }).success, ).toBe(false); // 'autostrat' typo should be rejected });🤖 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/shared/src/config.test.ts` around lines 60 - 107, Add an explicit unit test that verifies unknown keys are rejected by the strict MCP server schema: update the test file containing GlobalConfigSchema and ProjectConfigSchema tests by appending a new it(...) that calls GlobalConfigSchema.safeParse with an mcp_servers entry containing a typo/extra key (e.g., autostrat) alongside valid fields (name, transport, command) and asserts .success is false; this ensures the strict() behavior on the MCP server entry (defined in the schema referenced as GlobalConfigSchema) is documented and enforced.packages/shared/src/agent.test.ts (1)
79-88: ⚡ Quick winConsider adding a negative test for invalid schema metadata.
The test verifies that valid
input_schemaandoutput_schemaare accepted, but it would strengthen coverage to also verify that malformed schema metadata is rejected (e.g.,input_schema: { type: 'invalid_type' }orinput_schema: "not an object").🧪 Suggested additional test
it('rejects invalid input_schema / output_schema metadata', () => { const min = { id: 'a', model: 'm', provider: 'anthropic', system_prompt: 'p' }; expect(AgentSchema.safeParse({ ...min, input_schema: 'not-an-object' }).success).toBe(false); expect(AgentSchema.safeParse({ ...min, input_schema: { type: 'invalid_json_schema_type' } }).success).toBe(false); });🤖 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/shared/src/agent.test.ts` around lines 79 - 88, Add a negative test to complement the existing "accepts optional input_schema / output_schema (agent-yaml-spec.md)" case by asserting that malformed schema metadata is rejected: create a new test (e.g., it('rejects invalid input_schema / output_schema metadata')) that uses the same minimal base object and calls AgentSchema.safeParse with invalid values such as input_schema: 'not-an-object' and input_schema: { type: 'invalid_json_schema_type' } (and similarly for output_schema), and assert .success is false to ensure AgentSchema rejects bad schema metadata.
🤖 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.
Nitpick comments:
In `@packages/shared/src/agent.test.ts`:
- Around line 79-88: Add a negative test to complement the existing "accepts
optional input_schema / output_schema (agent-yaml-spec.md)" case by asserting
that malformed schema metadata is rejected: create a new test (e.g., it('rejects
invalid input_schema / output_schema metadata')) that uses the same minimal base
object and calls AgentSchema.safeParse with invalid values such as input_schema:
'not-an-object' and input_schema: { type: 'invalid_json_schema_type' } (and
similarly for output_schema), and assert .success is false to ensure AgentSchema
rejects bad schema metadata.
In `@packages/shared/src/config.test.ts`:
- Around line 60-107: Add an explicit unit test that verifies unknown keys are
rejected by the strict MCP server schema: update the test file containing
GlobalConfigSchema and ProjectConfigSchema tests by appending a new it(...) that
calls GlobalConfigSchema.safeParse with an mcp_servers entry containing a
typo/extra key (e.g., autostrat) alongside valid fields (name, transport,
command) and asserts .success is false; this ensures the strict() behavior on
the MCP server entry (defined in the schema referenced as GlobalConfigSchema) is
documented and enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d7dd7e-d83c-4d55-be03-580a08b01d67
⛔ Files ignored due to path filters (1)
packages/db/src/__snapshots__/client.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (53)
.claude/skills/add-llm-adapter/SKILL.md.github/workflows/ci.yml.npmrcAGENTS.mdCLAUDE.mdREADME.mddocs/analysis/multimodal-io-design-2026-06-07.mddocs/decisions/0018-desktop-execution-and-rust-egress.mddocs/decisions/0023-strict-authored-yaml-validation.mddocs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mddocs/decisions/0032-desktop-rust-media-de-inline-amends-0018.mddocs/decisions/0033-strict-config-files-amends-0023.mddocs/decisions/README.mddocs/reference/contracts/ipc-contract.mddocs/reference/contracts/workflow-yaml-spec.mddocs/reference/desktop/database-schema.mddocs/reference/shared-core/llm-provider-seam.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-0-foundations.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/roadmap/phases/phase-3-desktop.mddocs/standards/code-style-typescript.mddocs/standards/logging-and-observability.mddocs/standards/security-review.mddocs/standards/testing.mddocs/tech-stack.mdpackage.jsonpackages/db/src/client.test.tspackages/db/tsconfig.build.jsonpackages/llm/src/adapters/anthropic.test.tspackages/llm/src/adapters/anthropic.tspackages/llm/src/llm-error.test.tspackages/llm/src/llm-error.tspackages/llm/tsconfig.build.jsonpackages/shared/src/agent.test.tspackages/shared/src/agent.tspackages/shared/src/common.tspackages/shared/src/config.test.tspackages/shared/src/config.tspackages/shared/src/constants.tspackages/shared/src/edge.test.tspackages/shared/src/node.test.tspackages/shared/src/node.tspackages/shared/src/run-event.test.tspackages/shared/src/run-event.tspackages/shared/src/run.test.tspackages/shared/src/workflow.test.tspackages/shared/src/workflow.tspackages/shared/tsconfig.build.jsontsconfig.tools.jsonvitest.config.ts
…on (PR #10 review) Two PR-review test gaps: - agent.test.ts: a negative case for input_schema/output_schema — a NON-OBJECT value is rejected (jsonSchemaMetadataSchema is z.record(string, unknown)), and it pins the deliberate permissiveness that an unknown inner JSON-Schema `type` IS accepted (the bag does not deep-validate the schema body; the engine/consumer does — so the reviewer's "reject { type: 'invalid' }" suggestion would assert false behavior). - config.test.ts: assert McpServerRegistrationSchema.strict() rejects a typo'd MCP key (`autostrat`), which the existing strict tests didn't cover for an mcp_servers entry (ADR-0033). @relavium/shared lint + typecheck + test green; prettier clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-internal (PR #10 review) Two verified review findings: - llm-error.ts: the query-string scrub missed `?x-api-key=<opaque>` — an opaque value has no sk-/AIza prefix for the key-shape patterns to catch, so only the param-name rule can mask it. The name group gains `(?:x[-_])?`; near-miss params (`?keyword=`, `?tokenize=`) stay untouched (the `=` must directly follow `key`). New backstop test case (opaque value built via join(); Leakwatch-clean). - types.ts: JSDoc on LlmError.cause stating it is NOT scrubbed by makeLlmError and must never be logged/serialized/put in a run event — the run-event error shape already excludes it; the warning makes the per-site obligation explicit at the type itself. Refs: docs/standards/security-review.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hunk count (PR #10 review) - anthropic/openai/gemini.test.ts: the 'never leaks the API key' tests planted the key only as the generate() key ARGUMENT — the mocked vendor error never echoed it, so they proved the key was never there, not that scrubSecrets strips it (security-review.md's claim: each adapter "plants a secret in a vendor error"). The mock bodies now echo the planted key, and a positive `[REDACTED]` assertion proves the scrub actually fired (a dropped message would be vacuously secret-free). Planted keys lengthened to match the real scrub patterns (sk- needs >=16 trailing chars, AIza >=20 — the old short toys dodged the regex) and built via join() (no contiguous key-shaped literal in source; Leakwatch-clean). openai also asserts kind=auth (the 401 classification path). - types.test.ts: the StreamChunk .each table now covers all 10 union members (the 4 ADR-0030 arms were tested separately but missing from the table) + a count pin (StreamChunkSchema.options length == table length) so an added/removed arm must consciously update both — mirroring the run-event count pin. Refs: docs/standards/security-review.md, ADR-0030 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pes, new internals (PR #10 review) Verified coverage gaps on PR #10's own new behavior: - workflow.test.ts: boolean inputs (the only EMPTY row of VALIDATION_KEYS_BY_TYPE) had zero coverage — `enum` on a boolean is the realistic authored mistake and a regression re-adding it would have passed the whole suite; code_diff/secret now reject numeric bound keys; `{ $ref: '' }` is rejected at the schema (nonEmptyString), not left to resolution. - run-event.test.ts: the authored-only nodeType reject matrix covered just 'parallel'; 'merge' (-> fan_in) and 'human_gate' (-> human_in_the_loop) added per node-types.md's mapping table — human_gate is the easiest to confuse with the engine name. - index.test.ts: the internal-leak check now pins the two new common.ts helpers (findDuplicates, jsonSchemaMetadataSchema). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ch other (PR #10 review) The group key used head_ref for PRs, so two PRs open from the same head branch shared one group and the newer run cancelled the older (the old `github.ref` form was unique per PR). PR runs now key on github.event.number; push runs fall back to ref_name, same as before; event_name still separates a push from its same-SHA PR; main is still never cancelled. The block comment contradicted itself ("a push and its open PR share one group" vs two sentences later "do NOT collapse into one group") — rewritten to describe the actual model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r root-only limit (PR #10 review) - package.json: the `ci` script ran 2 of CI's 6 steps — a local green could pass while CI fails on build/format/fence. It now adds `turbo run build format:check` + `pnpm lint:fence-check`. The DB migration drift gate stays CI-only on purpose: it runs db:generate, which mutates the working tree. (pnpm reserves bare `ci`; invoke as `pnpm run ci`.) - vitest.config.ts: verified empirically that the >=90% per-glob floor fires ONLY on a repo-root run — a package-scoped --coverage run keys the coverage map cwd-relative (`src/...`), and no single glob can match both bases without wrongly binding shared/db package runs to the engine floor. Documented at the thresholds block; tracked as a new deferred-tasks item (resolve when coverage becomes a root-run CI gate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eptance criterion) PR #10 review (panel-confirmed): ADR-0031 §Decision says canonical types live in llm-provider-seam.md, but accepting the ADR left the seam doc without them — the decided shape exists only in the ADR/analysis doc, and the 1.AD task said just "update llm-provider-seam.md" without saying how, outside the acceptance criteria. Now: - llm-provider-seam.md: a short "Seam-shape amendments (ADR-0031) — accepted, lands with 1.AD" placeholder that links to the ADR (feature names only, no restated spec — one-canonical-home) and states none of it is implemented yet. - phase-1 1.AD: replacing that placeholder with the full ADR-0030-mould section is now an explicit task line AND an acceptance criterion, so the doc update cannot silently slip. Refs: ADR-0031 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
7 tasks
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
Three related streams of work after PR #9 merged (M1 — LLM seam proven, reached):
1.G/1.H/1.Jdone and M1 reached across the phase doc, the global milestone spine,current.md, and the repo-level status lines (README/CLAUDE/AGENTS); record two PR Phase 1 — adapters 1.G (OpenAI/DeepSeek) + 1.H (Gemini) + seam-shape amendment (ADR-0030) #9-review follow-ups (vision/image-arm decision, live-nightly enable) indeferred-tasks.md.deferred-tasks.mditem whose files are already built (shared / db / llm / root tooling / docs) was re-verified against current code (per-item) and 29 were resolved; the rest are surfaced as decisions / blocked / explicitly-deferred (checked off in the file).Resolved (29), by area
node:started.nodeType→ENGINE_NODE_TYPESenum; exportMaskedSecret; one O(n)findDuplicatesshared by the agent/workflow dup-id checks;AgentSchemaoptionalinput_schema/output_schema; per-type input-validation-key matrix; test-gap fills (custom-merge node-level, edge branch-handle grammar, condition/transform invariants, RunSchema non-object inputs); comment/claim accuracy.temperature > 1for Anthropic (provider-local; the shared contract stays[0,2]), passing a pre-egress guard error through unchanged.journal_modeassertion.@relavium/llm;incrementaltsbuildinfo;tsconfig.tools.json+typecheck:tools(root/tooling configs now typechecked, wired into CI); per-PR CI concurrency keying; drop published source maps.ContentPart/StopReason; workflow-yaml-spec validation matrix; tech-stack Node-runtime row; database-schema WAL single-writer wording; phase-0 0.B reconciled to the implemented turbo^buildordering.Review-fix pass (2026-06-10, deep multi-agent review)
All adversarially-confirmed findings of the final review round were fixed in-branch:
scrubSecretsbackstop provably fires — positive[REDACTED]assertion; keys built viajoin(), Leakwatch-clean);scrubSecretsalso masks?x-api-key=<opaque>;LlmError.causecarries an explicit unscrubbed-internal JSDoc warning;StreamChunktable covers all 10 members + a count pin.booleanvalidation-matrix row,code_diff/secretnumeric-key rejection,{ $ref: '' }, authoredmerge/human_gatenodeType rejection, and the two new internal helpers in the no-leak check.pnpm run cimirrors the full CI pipeline (DB drift gate stays CI-only — it mutates files); the coverage-floor root-only limit is documented + tracked (verified empirically).llm-provider-seam.mdcarries an ADR-0031 placeholder section, and 1.AD now requires replacing it with the full amendment write-up as an acceptance criterion (the seam doc is the shape's canonical home per ADR-0031 §Decision).Still open (tracked, not in this PR)
LICENSE(HodeTech is drafting its own commercial license).z.unknown()presence check (discriminatedUnion constraint), dist packaging smoke test (build-before-test), localformat:checkvia turbo (recursion/rename nit), coverage floor as a root-run CI gate (new — see the review-fix pass).Scope
Package(s):
@relavium/shared,@relavium/db,@relavium/llm+ root tooling + docs (3 new ADRs + the multimodal analysis). Phase/workstream: Phase 1 post-M1 hardening + the multimodal decision. No new runtime dependency. No engine (packages/core) code.Checklist
pnpm turbo run lint typecheck test build+typecheck:tools+ coverage threshold +prettier --check .+ seam fence all green (pnpm run ci)@relavium/llmseamscan fs, 0 findings on tracked files)scrubSecretschoke point) are provider-local / behind the seam; the shared contract is unchangedRefs: ADR-0011, ADR-0030, ADR-0031, ADR-0032, ADR-0033, docs/roadmap/deferred-tasks.md
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests
Chores