fix: anchor catalog type/version regex to export block#1121
Conversation
The type and version regexes in populateCatalogFromDir matched the first
`type:` / `version:` property anywhere in the source file. When a model
file had unrelated `type:` properties before the export block (e.g.,
inside helper function calls), the catalog was poisoned with the wrong
type. Anchor both regexes on `export const (model|extension) = {` so
they only extract from the actual model definition.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Clean, well-scoped fix. The regex anchoring approach correctly solves the problem of decoy type:/version: properties matching before the actual export block.
Blocking Issues
None.
Suggestions
-
Regex edge case with nested
type:inside export block — The[\s\S]*?(non-greedy) will match the firsttype:afterexport const model = {. If a model ever has a nested object with atype:property before the top-leveltype:(e.g.,export const model = { schema: { type: "object" }, type: "@test/real" }), it would extract the wrong value. This is unlikely in practice and already an improvement over the previous regex (which matched anywhere in the file), so not blocking — just worth noting. -
Large
deno.lockdiff — The lock file adds ~5k lines of new entries (promptfoo and transitive deps). This appears to be from the lock file catching up with the existingdeno.jsonworkspace declaration rather than a new dependency, so it's fine — just visually noisy.
Overall: solid fix with a good regression test. LGTM.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
-
src/domain/models/user_model_loader.ts:836,844—[\s\S]*?can cross the export block boundary. The non-greedy[\s\S]*?between theexport constanchor andtype:/version:will match across the closing}of the export block iftype:orversion:is absent from the export but present later in the file. Fortype:, this is moot — iftype:is missing from the export, you'd get a bogus type, but thecontinueon line 838 prevents that from persisting (the regex just wouldn't match the expected format). Forversion:, if the export block lacks aversion:property but one exists elsewhere in the file (e.g., in a comment or unrelated object), it would be captured. However, the old regex had the exact same cross-boundary issue, and the comment in the old code acknowledged this is best-effort with correction on subsequent runs. No regression from the old behavior. -
src/domain/models/user_model_loader.ts:824-826vs:836— gate check matchesmodel:(type annotation) but extraction regex does not. The gate on line 824 uses[=:]which matchesexport const model: SomeType = {, but the extraction regex on line 836 uses\s*=\s*\{which would NOT match that pattern (it'd miss the: SomeTypebetween the name and=). If anyone wroteexport const model: ModelDef = { ... }, the gate would pass but extraction would fail, silently skipping the file. Verified that no files in the repo currently use this pattern, so this is theoretical only.
Low
-
src/domain/models/user_model_loader.ts:836— comment inside export block could be matched. If someone wroteexport const model = { // type: "commented-out" \n type: "@real/type" }, the non-greedy match would capture"commented-out". Extremely unlikely in practice and identical behavior to the old regex. -
deno.lock— includes unrelatedpromptfoodependency addition. The lock file diff is ~5700 lines for what appears to be a lock file refresh, not related to the regex fix. Harmless but noisy.
Verdict
PASS — The regex anchoring fix is correct and directly addresses the reported bug. The [\s\S]*? non-greedy scan between the anchor and type: will correctly skip decoy properties before the export block. The edge cases identified are either theoretical (no files use type-annotated exports) or identical to pre-existing behavior. Test coverage is appropriate.
Summary
populateCatalogFromDirto theexport const model/extension = {block, preventing them from matching unrelatedtype:/version:properties earlier in the file (e.g., inside helper function calls or schemas)type:property before the export blockFixes #1110 (code fix only — skill doc changes are separate)
Test Plan
type:/version:before export block → catalog stores the real type, not the decoydeno check,deno lint,deno fmtclean🤖 Generated with Claude Code