Add custom digest SDK and CLI#158
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } else if (ch === "?") { | ||
| out += "[^/]"; | ||
| } else if ("\\.+^$|()[]{}".includes(ch)) { | ||
| out += "\\\\" + ch; |
There was a problem hiding this comment.
🔴 Double-escaped regex in JS globToRegExp breaks path matching for patterns with dots
The globToRegExp function in the digest function test runner script (digestFunctionRunnerScript) double-escapes regex special characters. Since the script is embedded in a Go raw string (backtick-delimited), the text "\\\\" on line 657 is delivered literally to JavaScript as "\\\\". JavaScript interprets "\\\\" as the 2-character string \\, so "\\\\" + ch produces \\<ch>. In a regex, \\. means "match literal backslash, then any character" — not "match literal dot".
This causes globToRegExp to produce incorrect regexes for any glob pattern containing ., +, ^, $, |, (, ), [, ], {, or }. For example, the glob *.md generates regex ^[^/]*\\.md$ which requires a literal backslash before d, so it never matches readme.md. The existing test passes only because the pattern /notion/databases/eng-roadmap/* is matched by the endsWith("/*") shortcut in matchesPath, never hitting globToRegExp.
| out += "\\\\" + ch; | |
| out += "\\" + ch; |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in fbb71fe by changing the embedded JS escape to a single regex escape and adding TestDigestFunctionTestMatchesDotGlobPatterns for *.md fixtures.
| func isOOM(err error) bool { | ||
| msg := strings.ToLower(err.Error()) | ||
| return strings.Contains(msg, "out of memory") || strings.Contains(msg, "oom") || strings.Contains(msg, "memory") | ||
| } |
There was a problem hiding this comment.
🟡 isOOM heuristic matches any error mentioning "memory", misclassifying non-OOM errors
The isOOM function at internal/wasmrun/wasmrun.go:327 uses strings.Contains(msg, "memory") as a catch-all, which matches any error whose message contains the substring "memory". This incorrectly classifies non-OOM errors as out-of-memory. For example, engine.go:62 can return "wasmrun: guest module exposes no memory" when a WASM module doesn't export a memory section — this is a module configuration error, not an out-of-memory condition. The isOOM check would classify it as OOM, causing a misleading WarningOOM to appear in digest warnings instead of a generic error warning.
| func isOOM(err error) bool { | |
| msg := strings.ToLower(err.Error()) | |
| return strings.Contains(msg, "out of memory") || strings.Contains(msg, "oom") || strings.Contains(msg, "memory") | |
| } | |
| func isOOM(err error) bool { | |
| msg := strings.ToLower(err.Error()) | |
| return strings.Contains(msg, "out of memory") || strings.Contains(msg, "oom") || strings.Contains(msg, "memory limit") | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in fbb71fe by narrowing OOM detection to out-of-memory/oom/memory-limit wording and adding TestEngineInvokeDoesNotClassifyMissingMemoryAsOOM.
| verify: true, | ||
| now: time.Now, | ||
| } | ||
| if opts.Verify { | ||
| c.verify = true | ||
| } |
There was a problem hiding this comment.
🟡 Cache Options.Verify field is non-functional — verification cannot be disabled
In cache.go:90, c.verify is unconditionally set to true. The subsequent if opts.Verify { c.verify = true } on lines 93–95 only ever sets it to true again. As a result, the Verify field in the public Options struct is dead — setting Options{Verify: false} has no effect, and verification is always enabled. The intent appears to be that verify defaults to true but could be disabled, or alternatively that the opts.Verify assignment should replace the hardcoded default. Either way, the current code makes the Verify option misleading.
| verify: true, | |
| now: time.Now, | |
| } | |
| if opts.Verify { | |
| c.verify = true | |
| } | |
| verify: true, | |
| now: time.Now, | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in fbb71fe by removing the misleading Options.Verify field and dead assignment; the cache now always verifies sidecar integrity as the implementation already required.
1 participant
Summary
Verification