feat(core): validation Phase 4 — generated-source contract check (G5)#497
Conversation
Replace the skipped Phase 4 stub with validatePhase4: a static, EXECUTION-FREE check that any AI-materialized generatedSource actually defines the change's declared target/name — the right define*() call, references its name, imports @linchkit/core. Catches AI errors that pass the Phase 2 syntax gate yet are wrong (empty scaffold, wrong target, mismatched name) before human review. - Warn-only by default; ValidationContext.strictGeneratedContract escalates to block. All-declarative proposals (no generatedSource) stay 'skipped'. - SAFETY: never eval/import/transpile-and-run the generated source. A true execution-based dry-run (sandboxed handler run against sample data) is intentionally out of scope — it would require a locked-down sandbox to run untrusted AI code; deferred to a separate sandbox-gated step. - Wired into validateProposal; exported from @linchkit/core/server; barrel surface test + 6 unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: raw includes() could be satisfied by a token appearing only in a comment (// defineAction() or a string literal, letting strict mode false-pass a contract-invalid action. Now strip comments (and, for the call check, string literals) and match real import/call structure via anchored regexes. + test that comment/string-only mentions do not satisfy the contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: substring includes(name) let a different name that merely contains the declared one (e.g. do_thing_v2 for do_thing) satisfy the check. Match on a word boundary (\bname\b) so a contained-but-different name no longer passes — snake_case _ is a word char, so do_thing won't match inside do_thing_v2. + test. Name escaped for safe RegExp interpolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 15 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces Phase 4 validation, a static contract check for AI-materialized ChangesPhase 4 Generated-Source Contract Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces Phase 4 validation, which statically verifies that AI-materialized generated source code conforms to its declared contract (e.g., calling the correct definition helper, referencing its declared name, and importing @linchkit/core). Feedback on the implementation highlights a parsing bug where URLs or slashes inside string literals are incorrectly stripped as comments, which breaks validation. Additionally, the reviewer recommends escaping the dot in the core import regex, pre-compiling regular expressions at the module level to improve performance, and adding a test case to prevent regressions.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/engine/validation-engine.ts (1)
1-717: Splitpackages/core/src/engine/validation-engine.tsto satisfy the 500-line guideline
packages/core/src/engine/validation-engine.tsis ~717 lines (> 500).- Searches for existing tracking issues mentioning “validation-engine” / “validation engine” / “validation-engine.ts” didn’t surface one for splitting this file; consider creating an issue to break it into smaller per-phase/per-validator modules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/engine/validation-engine.ts` around lines 1 - 717, The file validation-engine.ts exceeds the 500-line guideline; split it into smaller modules by extracting Phase 1 runner and each validator into separate files and re-exporting a compact API: create a file for the phase runner (export validatePhase1 and validateProposal), a module for helpers/state resolution (resolveStateMachine, ValidationContext utilities), and separate validator modules for validateEntity, validateAction, validateRule, validateStateDef, validateEventDef, and validateViewDef; update imports/exports so the original symbols (validatePhase1, validateProposal, validateEntity, validateAction, validateRule, validateStateDef, validateEventDef, validateViewDef, resolveStateMachine, ValidationContext) remain available from the engine package, add a short tracking issue referencing “validation-engine.ts” for the refactor, and ensure tests/builds still pass after replacing local references with the new module paths.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.
Nitpick comments:
In `@packages/core/src/engine/validation-engine.ts`:
- Around line 1-717: The file validation-engine.ts exceeds the 500-line
guideline; split it into smaller modules by extracting Phase 1 runner and each
validator into separate files and re-exporting a compact API: create a file for
the phase runner (export validatePhase1 and validateProposal), a module for
helpers/state resolution (resolveStateMachine, ValidationContext utilities), and
separate validator modules for validateEntity, validateAction, validateRule,
validateStateDef, validateEventDef, and validateViewDef; update imports/exports
so the original symbols (validatePhase1, validateProposal, validateEntity,
validateAction, validateRule, validateStateDef, validateEventDef,
validateViewDef, resolveStateMachine, ValidationContext) remain available from
the engine package, add a short tracking issue referencing
“validation-engine.ts” for the refactor, and ensure tests/builds still pass
after replacing local references with the new module paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a02bee2-1ad2-4024-b413-987b9c237835
📒 Files selected for processing (6)
.changeset/g5-phase4-contract.mdpackages/core/__tests__/barrel-public-surface.test.tspackages/core/src/__tests__/engine/validation-phase4.test.tspackages/core/src/engine/validation-engine.tspackages/core/src/engine/validation-phase4.tspackages/core/src/exports/server/engines.ts
…se 4) Address gemini review on #497: - stripComments/stripCommentsAndStrings now walk the source ONCE, string- and comment-aware, so a comment marker inside a string literal (a URL's //, or /* */ split across two strings) is never treated as a comment. The old regex stripper could greedily swallow real code between a /* and */ that each live inside separate string literals. + regression test. - Pre-compile the core-import / require detectors at module level instead of rebuilding them per change; fold the define-call detector into a per-target contract map. (@linchkit/core has no regex metachar, so the literals are exact.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: export { defineAction } from "@linchkit/core" re-exports
without creating a local binding, so a generated action that re-exports then
calls defineAction would fail. The core-import detector now matches import only
(not export-from). + test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: a string literal containing a full import statement could satisfy the core-import check (it ran over the strings-kept view). Strippers are now length-preserving, and the import check confirms each matched import/require keyword sits in REAL code by cross-referencing the same index in the strings-blanked view — a fake import inside a string is blanked there. + test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: the call/name checks were independent, so a source that
calls defineAction for a DIFFERENT action while mentioning the declared name
elsewhere (e.g. `const do_thing = 1; const other = defineAction({name:'other'})`)
passed. The check now requires the declared name be BOUND to the define call —
either `const <name> = defineAction(` (identifier, robust in blanked code) or a
real `defineAction({ … name: '<name>' … })` call (keyword confirmed in real code
via index cross-reference). Replaces the two independent call/name findings with
one tied finding. + tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he call
Address codex review (two gaps):
- The import check now requires the SPECIFIC helper (e.g. defineAction) in the
import clause — `import { defineEntity } from core` no longer satisfies a
defineAction contract.
- The tied name match is bounded to the call's own options object (`[^}]*?` can't
cross the first `}`), so a `name: "<declared>"` in unrelated later code no
longer satisfies the contract.
+ tests for both. These remain best-effort STATIC heuristics (no execution, no
full parse); Phase 2 syntax + the graduation PR's CI build + human review are the
authoritative gates.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: the registered ActionDefinition.name is the defineAction
options-object name: literal, not the variable it is assigned to. Dropped the
boundByConst shortcut (const do_thing = defineAction({ name: 'other' }) registers
'other', not 'do_thing'); the contract is now satisfied ONLY by a real
defineAction call whose own options name the declared action. + test. Simplified
CONTRACT_BY_TARGET to a target→helper-name map.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…equire)
Address codex review: the require() branch accepted any require("@linchkit/core")
without checking the called helper was bound. Generated definition source is ESM
(the materializer prompts for import … from "@linchkit/core"), so the CommonJS
fallback is unnecessary surface — removed it. The import check now requires the
specific helper in a real ESM import clause; a require-based file gets an advisory
import warning (warn-only by default).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address codex review: import type { defineAction } is erased at runtime and
{ defineAction as da } binds da (not defineAction), so neither provides a usable
helper value. The import detector now excludes a leading import type and an
aliased-away helper. Deeper shapes (inline { type x }, namespace import) remain
advisory-only — the graduation PR's CI typecheck is authoritative. + tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Validation Phase 4 — generated-source CONTRACT check (G5). Replaces the
skipped Phase 4 stub with
validatePhase4: a static, execution-free checkthat any AI-materialized
generatedSource(from the #495 materializer) actuallydefines the change's declared target/name — the right
define*()call, areference to its name, and an import from
@linchkit/core. It catches a class ofAI errors that pass the Phase 2 syntax gate yet are wrong (empty scaffold, wrong
target, mismatched name) before a human reviews the candidate.
ValidationContext.strictGeneratedContractescalates toblock. All-declarative proposals (no
generatedSource) stay"skipped".validateProposal(was the M1 skip stub); exported from@linchkit/core/server(+ barrel-surface test).actionis the only materializable target today (matches the materializer'sMATERIALIZABLE_TARGETS); the map is extensible.Safety
EXECUTION-FREE by design ("AI never modifies production directly"): it never
evals,imports, transpiles-and-runs, or otherwise executes the generatedsource — only static string/structural heuristics. A true execution-based
dry-run (running a generated handler against sample/historical data) requires a
locked-down sandbox to run untrusted AI code and is intentionally out of
scope here — deferred to a separate, sandbox-gated step.
Review
3 rounds of codex review converged to clean. Hardenings applied along the way:
// defineAction() or string literal nolonger satisfy the call/import checks (strip comments, and strings for the
call check; match real import/call structure via anchored regexes).
CONTAINS the declared one (
do_thing_v2fordo_thing) no longer passes.Verification
bun run check/bun run typecheck— cleanbun run test— PASS (8 Phase 4 unit tests incl. comment/string andsubstring-name hardening; barrel surface test; packages/core 4170 tests green)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests