Conversation
Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
- Created new detectTests.ts with browser-compatible parseContent function - Extracted pure parsing logic (no fs, path dependencies) - Added helper functions: parseXmlAttributes, parseObject, replaceNumericVariables - Updated common exports to include detectTests functionality - Successfully compiles with TypeScript strict mode Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
- Made crypto.randomUUID browser-compatible with fallbacks - Added detectTests() function as main entry point for test detection - Removed duplicate interface declarations - All 441 tests passing - Module compiles successfully with TypeScript strict mode Co-authored-by: hawkeyexl <5209367+hawkeyexl@users.noreply.github.com>
- Add unit tests to verify the export of functions from the CommonJS and ESM modules. - Implement tests to ensure the `detectTests` function works correctly with both import styles. - Confirm that the results from `detectTests` are identical when using CJS and ESM. - Check for type definitions related to `detectTests` in the generated TypeScript declaration files. - Remove obsolete test files: `temp-element-timeout-test.json` and `temp-network-timeout-test.json`.
|
|
WalkthroughAdds a browser-safe detectTests implementation and tests; exposes detectTests and its types in public exports; replaces Node-only UUID usage with a browser fallback; removes file I/O and path-resolution modules and their tests; updates build scripts to produce CJS and ESM wrappers and adjusts package exports and tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(240,240,255,0.5)
actor Caller
end
rect rgba(220,255,220,0.5)
participant DT as detectTests
participant PC as parseContent
participant VA as Validator
participant LG as Logger
end
Caller->>DT: detectTests({ content, filePath, fileType, config })
DT->>PC: parseContent({ content, filePath, fileType, config })
PC->>LG: log(config, "info"/"warn", message)
PC->>PC: extract inline/markup statements → assemble tests & steps
alt schema conversion/validation needed
PC->>VA: transform & validate steps/tests
VA-->>PC: validation result
end
PC-->>DT: DetectedTest[]
DT-->>Caller: DetectedTest[]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
hawkeyexl
changed the title
detectTests function works correctly with both import styles.
- Confirm that the results from detectTests are identical when using CJS and ESM.
- Check for type definitions related to detectTests in the generated TypeScript declaration files.
- Remove obsolete test files: temp-element-timeout-test.json and temp-network-timeout-test.json.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/detectTests.ts`:
- Around line 366-406: The "testStart" and other switch case bodies leak
block-scoped declarations (e.g., const parsedTest, const parsedStep, let step,
const validation) because their case bodies are not wrapped; wrap each case body
in its own block (add { ... } around the code inside cases like "testStart" and
the step/validation cases around lines where parsedStep/step/validation are
declared) so those const/let bindings are scoped to the case, keep existing
breaks inside the blocks, and ensure any case-local variables (parsedTest,
parsedStep, step, validation) are only used inside their respective blocks.
- Around line 476-506: The code assumes step.httpRequest.request exists when
normalizing headers and body; update the guards to check for the presence of
request before accessing its properties (e.g., ensure step.httpRequest.request
is truthy or use optional chaining) so the blocks that reference
step.httpRequest.request.headers and step.httpRequest.request.body only run when
request is defined; apply this check in both the headers parsing branch and the
JSON body parsing branch (locations referencing step.httpRequest.request.headers
and step.httpRequest.request.body) to prevent TypeError when httpRequest has no
request property.
- Around line 358-419: The ignore flag is toggled in the
"ignoreStart"/"ignoreEnd" cases but never used, so either remove it or enforce
it: update the switch handling for "testStart", "step", and "detectedStep" (the
cases that create/append steps and tests) to check if (ignore) { break; } at the
top of each case to skip processing while inside an ignore block; alternatively,
if ignore blocks are meant to be removed, delete the ignore variable and the
"ignoreStart"/"ignoreEnd" cases (keep testEnd reset logic if still needed).
Ensure references are to the existing ignore variable and the switch cases named
"testStart", "step", and "detectedStep".
- Around line 591-607: The log function treats "warning" and "warn" as different
precedence because normalization happens after computing configLevelIndex and
messageLevelIndex; in function log normalize both the incoming level and
config.logLevel (map "warning" to "warn") before computing logLevels.indexOf so
they are synonyms, remove duplicate "warning" from the logLevels array (keep
only "warn"), and then use the normalizedLevel for the console call
(console[normalizedLevel as 'error'|'warn'|'info'|'debug']) so threshold checks
and output are consistent.
In `@src/validate.ts`:
- Line 35: The dynamic defaults factory for UUID uses a zero-arity function
which violates ajv-keywords' requirement; update
dynamicDefaultsDef.DEFAULTS.uuid to be a one-argument function that returns a
generated UUID (e.g., change the factory from () => getRandomUUID to (_?: any)
=> getRandomUUID()) so the function.length > 0 contract is satisfied and the
UUID is actually produced when invoked; alternatively, if you intended lazy
evaluation keep the parameterized form (args?: any) => () =>
getRandomUUID()—apply the change around dynamicDefaultsDef.DEFAULTS.uuid and
ensure getRandomUUID is called (getRandomUUID()) rather than referenced.
In `@test/detectTests.test.js`:
- Around line 1192-1194: The closing brace that ends the
describe("parseContent", ...) block is in the wrong spot: move the standalone
`});` that currently closes `describe("parseContent", ...)` (just before the
`it("should handle markup regex with no capture groups...")` test) so that it
instead comes after the last `it(...)` that tests parseContent (after line
1371), ensuring all six orphaned `it()` blocks remain inside the
describe("parseContent", ...) block; verify the describe closing brace now
precedes the outer `describe("detectTests module", ...)` continuation and run
tests to confirm scope is correct.
🧹 Nitpick comments (3)
src/validate.ts (1)
12-23: Duplicated UUID fallback logic withsrc/detectTests.ts.
getRandomUUID()here andgenerateUUID()insrc/detectTests.ts(lines 13–22) are identical implementations. Consider extracting a shared helper (e.g., in autils.tsmodule) to avoid maintaining the same code in two places.scripts/createEsmWrapper.js (1)
13-13: ESM wrapper exports must stay in sync withsrc/index.ts.This is a manual, fragile synchronization point. If a future export is added to
src/index.tsbut not mirrored here, ESM consumers silently lose access to it. Consider generating this list programmatically (e.g., by introspectingcjsModulekeys) or at least adding a comment cross-referencingsrc/index.ts.src/detectTests.ts (1)
317-318: Regex constructed from configuration input — low ReDoS risk but worth noting.
statementRegexandpatterncome fromfileTypeconfiguration (developer-controlled, not end-user input). The risk is low, but a malicious or accidental pathological regex in config could cause hangs. Consider documenting thatfileTyperegex patterns should be tested for safety, or add a timeout/complexity check if this will be exposed to untrusted config.Also applies to: 330-331
src/validate.ts
Outdated
| // @ts-ignore - ajv-keywords has incomplete types for dynamicDefaults | ||
| const def = require("ajv-keywords/dist/definitions/dynamicDefaults"); | ||
| def.DEFAULTS.uuid = () => randomUUID; | ||
| dynamicDefaultsDef.DEFAULTS.uuid = () => getRandomUUID; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ajv-keywords dynamicDefaults contract factory function signature
💡 Result:
dynamicDefaults keyword contract (schema shape)
dynamicDefaults must be used at the object-schema level and its value is an object mapping property names to either:
- a string (the dynamic-default function id), or
- an object
{ func: string, args: object }whereargsare passed during schema compilation. [1]
Type shape (informal):
type DynamicDefaultsKeyword =
Record<string, string | { func: string; args?: Record<string, any> }>Notes/constraints:
- It only works when Ajv is configured with
useDefaults. [1] - Properties mentioned in
dynamicDefaultsshould not also be inrequiredin the same schema (validation would fail) because these defaults are applied after validation. [1]
Custom dynamic default “factory” function contract (Ajv Keywords)
To add your own function id, you register it in the exported DEFAULTS table:
- Ajv v8 + ajv-keywords v5:
require("ajv-keywords/dist/definitions/dynamicDefaults").DEFAULTS[2] - Ajv v6 + ajv-keywords v3:
require("ajv-keywords").get("dynamicDefaults").definition.DEFAULTS[3]
Signature/behavior contract (as documented):
- A custom default function is selected by id (
func/ string form), and Ajv differentiates functions by arity (function.length). [1] - If your default accepts parameters from schema (
args), implement it as:(args: any) => /* something usable as default */(documentation examples return a 0-arg function that produces the value). [3]
- If your default does not expect
args, it should still have at least one non-optional argument sofunction.length > 0(per ajv-keywords docs) to avoid ambiguity. [1]
So the practical “factory” shapes you see in docs are:
// parameterized (called at compile time with args; returns generator)
type ParamDefaultFactory = (args: Record<string, any>) => () => any
// non-parameterized (docs discuss arity rules; examples vary by major version)
type NonParamDefault = (...atLeastOneArg: any[]) => anySources: ajv-keywords keyword docs and examples for dynamicDefaults and DEFAULTS registration. [1] [2] [3]
🏁 Script executed:
cat -n src/validate.ts | head -50Repository: doc-detective/common
Length of output: 2030
Fix the dynamicDefaults factory function arity to match the ajv-keywords contract.
The ajv-keywords library requires non-parameterized custom defaults to have function.length > 0 (at least one argument). The current () => getRandomUUID has arity 0 and violates this requirement. Change to (_?: any) => getRandomUUID() (or use the parameterized form (args?: any) => () => getRandomUUID() if lazy evaluation is intended).
🤖 Prompt for AI Agents
In `@src/validate.ts` at line 35, The dynamic defaults factory for UUID uses a
zero-arity function which violates ajv-keywords' requirement; update
dynamicDefaultsDef.DEFAULTS.uuid to be a one-argument function that returns a
generated UUID (e.g., change the factory from () => getRandomUUID to (_?: any)
=> getRandomUUID()) so the function.length > 0 contract is satisfied and the
UUID is actually produced when invoked; alternatively, if you intended lazy
evaluation keep the parameterized form (args?: any) => () =>
getRandomUUID()—apply the change around dynamicDefaultsDef.DEFAULTS.uuid and
ensure getRandomUUID is called (getRandomUUID()) rather than referenced.
There was a problem hiding this comment.
Pull request overview
This PR expands the package’s public surface to include a new detectTests module (and related helpers), ensures the ESM wrapper re-exports the same API as CommonJS, and adds integration/type-definition tests around those exports.
Changes:
- Add new browser-compatible
detectTestsimplementation and export it (plus related helpers/types) from the package entrypoint. - Update the ESM wrapper generator to re-export the new APIs from the CJS build.
- Add/extend integration tests to validate CJS/ESM exports and verify generated
.d.tsfiles fordetectTests.
Reviewed changes
Copilot reviewed 6 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration.test.js | Adds CJS/ESM export assertions and cross-module parity checks for detectTests and helpers. |
| test/detectTests.test.js | Adds extensive unit coverage for detectTests helpers (parseContent, parseObject, etc.). |
| src/validate.ts | Reworks UUID default generation to be browser-compatible via dynamicDefaults. |
| src/index.ts | Exports detectTests and related helper functions/types from the main entrypoint. |
| src/detectTests.ts | Introduces the browser-compatible detectTests implementation and helper utilities. |
| scripts/createEsmWrapper.js | Updates ESM wrapper to re-export detectTests and helper exports from the CJS module. |
| package-lock.json | Lockfile metadata adjustments (peer flags removed on some entries). |
| dist/validate.js | Compiled output for src/validate.ts reflecting UUID/dynamicDefaults changes. |
| dist/validate.js.map | Sourcemap updated for validate.js. |
| dist/validate.d.ts.map | Declaration map updated for validate.d.ts. |
| dist/index.mjs | ESM wrapper updated to re-export new APIs. |
| dist/index.js | Compiled entrypoint updated to export detectTests and helpers. |
| dist/index.js.map | Sourcemap updated for index.js. |
| dist/index.d.ts | Type exports updated to include detectTests and helpers. |
| dist/index.d.ts.map | Declaration map updated for index.d.ts. |
| dist/detectTests.js | Compiled output for new src/detectTests.ts. |
| dist/detectTests.js.map | Sourcemap added for detectTests.js. |
| dist/detectTests.d.ts | Type declarations added for detectTests. |
| dist/detectTests.d.ts.map | Declaration map added for detectTests.d.ts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should export detectTests function via require", function () { | ||
| const { detectTests } = require("../dist/index.js"); | ||
| expect(typeof detectTests).to.equal("function"); | ||
| }); |
There was a problem hiding this comment.
These newly added assertions are inside a file that is wrapped in an async IIFE (awaiting import("chai")). With Mocha running in CommonJS mode (npm test is just mocha), Mocha does not await the IIFE promise while loading the test file, so the describe/it blocks (including these new ones) may never be registered/executed. Switching to synchronous Chai loading and removing the async wrapper will make these integration tests actually run.
src/validate.ts
Outdated
| // @ts-ignore - ajv-keywords has incomplete types for dynamicDefaults | ||
| const def = require("ajv-keywords/dist/definitions/dynamicDefaults"); | ||
| def.DEFAULTS.uuid = () => randomUUID; | ||
| dynamicDefaultsDef.DEFAULTS.uuid = () => getRandomUUID; |
There was a problem hiding this comment.
dynamicDefaultsDef.DEFAULTS.uuid is being set to a function that returns getRandomUUID (the function), not a UUID string. Ajv's dynamicDefaults will then populate stepId/testId with a function value instead of a string, breaking schema validation/consumers. Set it to a generator that returns a string (e.g., assign getRandomUUID directly or return getRandomUUID() depending on the ajv-keywords API).
| dynamicDefaultsDef.DEFAULTS.uuid = () => getRandomUUID; | |
| dynamicDefaultsDef.DEFAULTS.uuid = getRandomUUID; |
| case "detectedStep": | ||
| test = findTest({ tests, testId }); | ||
| if (typeof test.detectSteps !== "undefined" && !test.detectSteps) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
ignoreStart/ignoreEnd toggle the ignore flag, but the detectedStep handler never checks ignore, so ignore blocks currently have no effect. If ignore markers are meant to suppress markup-detected steps, add an if (ignore) break/return guard in the detectedStep case (and/or skip collecting markup matches while ignore is true).
| }); | ||
| if (!valid.valid) { | ||
| console.warn(`Step ${JSON.stringify(step)} isn't a valid step. Skipping.`); | ||
| return; |
There was a problem hiding this comment.
parseContent emits warnings via console.warn regardless of config.logLevel. Since this module introduces a log() helper that respects logLevel, these warnings should go through that helper (or be gated) to avoid noisy output in library/browser consumers and to make logLevel: "silent" effective.
|
|
||
| // Normalize 'warning' to 'warn' | ||
| const normalizedLevel = level === "warning" ? "warn" : level; | ||
|
|
There was a problem hiding this comment.
log() can throw if called with level: "silent" because normalizedLevel becomes "silent" and console["silent"] is undefined. Consider treating message-level "silent" as a no-op (or removing it from the allowed message levels) so the function is safe for any level in logLevels.
| // Treat message-level 'silent' as a no-op to avoid calling an undefined console method | |
| if (normalizedLevel === "silent") return; |
test/detectTests.test.js
Outdated
| (async () => { | ||
| const { expect } = await import("chai"); | ||
| const { | ||
| detectTests, | ||
| parseContent, |
There was a problem hiding this comment.
Wrapping test registration in an async IIFE and awaiting import("chai") means Mocha (running in CommonJS mode via npm test: mocha) will not wait for the async setup while loading the file. The describe/it blocks can be registered too late, causing tests to be skipped or nondeterministic. Prefer synchronous Chai loading (e.g., require("chai")) and remove the async IIFE (or convert the test file to ESM in a way Mocha will await).
- Add block scope to case statements in parseContent for better readability. - Update logging function to normalize 'warning' to 'warn' for consistency. - Replace console warnings with the log function for better logging control. - Modify uuid dynamic default to accept arguments in validate.ts. - Clean up test files by removing unnecessary async IIFE wrappers.
- Delete `files.js`, `index.js`, `resolvePaths.js`, `schemas/index.js`, and `validate.js` as they are no longer needed.
…dencies - Remove the `readFile` function and its associated tests from the codebase. - Delete the `resolvePaths` function and its related tests. - Update `package.json` to remove `axios` from dependencies and add `@apidevtools/json-schema-ref-parser` to devDependencies. - Adjust exports in `index.ts` to remove deprecated functions. - Update tests to reflect changes in exports and remove tests for deleted functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/integration.test.js`:
- Around line 157-178: The test shares a single mutable testInput between two
calls to detectTests (in the "should produce identical detectTests results from
CJS and ESM" test), which can hide mutations; fix it by cloning testInput before
each invocation (e.g., use JSON.parse(JSON.stringify(testInput)) or
structuredClone) so call sites use separate objects for
cjsModule.detectTests(...) and esmModule.detectTests(...), referencing the
existing variables cjsModule, esmModule, and detectTests to locate where to
apply the clones.
🧹 Nitpick comments (3)
test/detectTests.test.js (3)
11-33: Test-scoped constants are defined outside thedescribeblock.
markdownFileTypeandminimalFileType(lines 11–32) are indented as if they belong inside thedescribe("detectTests module", ...)block starting at line 34, but they're actually at module scope. This works due to closure, but the misleading indentation could confuse contributors.Move the constant declarations inside the
describeblock, or de-indent them to make the module-level scope explicit.
168-179: Duplicate test scenario for escaped JSON.The test at line 168 ("should parse escaped JSON (double-encoded)") and the test at line 174 ("should fallback to simple unescaping for escaped JSON") use the identical input string
'{\\"key\\": \\"value\\"}'. The second test has a weaker assertion (.to.not.be.null) and doesn't exercise a different code path from the first. Consider removing the duplicate or crafting an input that actually requires the simple-unescape fallback (where double-parse fails butreplace(/\\"/g, '"')succeeds).
296-396: Console monkey-patching is fragile; consider using Sinon stubs.Sinon is already a devDependency. Using
sinon.stub(console, 'info')would be cleaner and automatically restore viasinon.restore()inafterEach, avoiding manual save/restore of each console method.
- Add tests to normalize logLevel configurations in the log function - Implement tests to ensure messages are not logged when logLevel is set to silent - Add tests to skip detected steps inside ignore blocks in markdown content - Use structuredClone for test input in integration tests to prevent mutation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/detectTests.ts`:
- Around line 248-270: The recursive branch in replaceNumericVariables assigns
the result of replaceNumericVariables(stringOrObject[key], values) back to
stringOrObject[key] which can be null for unresolvable nested properties; change
this so that you capture the recursive result into a variable and if that result
is null delete stringOrObject[key] (matching the string branch behavior)
otherwise assign the non-null result back—this ensures nested properties are
removed rather than set to null.
In `@test/detectTests.test.js`:
- Around line 1153-1167: The test "should skip inline steps inside ignore block"
creates a test with all inline steps placed inside an ignore block which results
in zero steps and the test being filtered out by validation; update the test
content used by parseContent so at least one step exists outside the ignore
block (e.g., add a seed step comment before the "<!-- test ignore -->" block or
include a non-ignored step in the test metadata) so result[0].steps is non-empty
and the assertion after parseContent remains valid.
- Around line 1137-1151: The test fails because tests with zero steps are
removed by validation; update the "should skip detected steps inside ignore
block" test so the seeded test is not empty: add a valid step outside the ignore
block in the content string and adjust the assertion accordingly (use
parseContent result from parseContent and expect result to have one test and
result[0].steps length to reflect the seeded step, or alternatively change the
assertion to expect length 0 if you intentionally want empty tests dropped).
Locate the test by its description and the content variable in the file and
modify the content and the expect(...) checks to match the chosen fix.
🧹 Nitpick comments (5)
src/validate.ts (1)
11-22: Duplicate UUID helper — extract to a shared module.
getRandomUUID()insrc/validate.tsis identical togenerateUUID()insrc/detectTests.ts(lines 13–22). Consider extracting a singleuuid.tsutility and importing it from both files.src/detectTests.ts (2)
317-318: Regex from configuration input — be aware of ReDoS risk.Both
statementRegex(line 317) and markuppattern(line 330) are compiled fromfileTypeconfiguration intonew RegExp(...). If a consumer passes untrusted or adversarial regex patterns, this could lead to catastrophic backtracking. Since this module is designed as a library API, consider documenting thatfileTypepatterns must be trusted input, or adding a timeout/complexity guard if untrusted sources are expected.
13-22: Duplicate UUID helper — same asgetRandomUUIDinsrc/validate.ts.This is a copy of the identical function in
validate.ts. Both should share a single implementation (see comment onsrc/validate.tslines 11–22).test/detectTests.test.js (1)
11-32: Test fixtures declared outsidedescribeblock scope.
markdownFileTypeandminimalFileTypeare declared at module level (outside thedescribeblock starting at line 34). While this works, it's a minor style inconsistency — consider moving them inside thedescribeblock for encapsulation, or marking themconstat module level is fine too since they're alreadyconstand not mutated.test/integration.test.js (1)
231-274: Package.json is re-read in every test.Each test under "Package exports field verification" calls
fs.readFileSync+JSON.parseindependently. Abeforehook or sharedlet packageJsonwould reduce redundancy, though this is a minor nit for test code.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package.json (2)
30-30: Keep the coverage ratchet step in the TDD workflow.Since the script path changed, please confirm CI/dev docs still run
npm run test:coverage:ratchetas part of the TDD steps. Based on learnings: All code changes require TDD—write tests first, verify failures, implement, verify pass, and runnpm run test:coverage:ratchet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 30, Confirm that the TDD workflow and CI/dev documentation still include and invoke the npm script "test:coverage:ratchet" (which runs "node scripts/check-coverage-ratchet.cjs"); update any CI config, README/TDD docs, or npm lifecycle hooks that reference the old path so they run `npm run test:coverage:ratchet` as part of the test/TDD steps (write tests first, see failures, implement, verify passing, then run the ratchet), and verify the script path in package.json matches the actual file location of check-coverage-ratchet.cjs.
3-6: Consider adding amodulefield for ESM-aware tooling.With
"type": "module"and"main": "dist/index.cjs", some bundlers that ignore"exports"but honor"module"may miss the ESM build. Adding a"module"field helps compatibility without changing CJS behavior.♻️ Suggested tweak
"type": "module", "main": "dist/index.cjs", + "module": "dist/index.js",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 3 - 6, package.json currently sets "type": "module" but only exposes a CommonJS entry via "main": "dist/index.cjs"; add a "module" field that points to your ESM bundle (e.g., "module": "dist/index.esm.js" or "dist/index.mjs") so ESM-aware bundlers can pick up the ESM build while leaving "main" unchanged; ensure the referenced filename matches your build output (update the build step that emits the ESM artifact if needed) and keep "type" as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 30: Confirm that the TDD workflow and CI/dev documentation still include
and invoke the npm script "test:coverage:ratchet" (which runs "node
scripts/check-coverage-ratchet.cjs"); update any CI config, README/TDD docs, or
npm lifecycle hooks that reference the old path so they run `npm run
test:coverage:ratchet` as part of the test/TDD steps (write tests first, see
failures, implement, verify passing, then run the ratchet), and verify the
script path in package.json matches the actual file location of
check-coverage-ratchet.cjs.
- Around line 3-6: package.json currently sets "type": "module" but only exposes
a CommonJS entry via "main": "dist/index.cjs"; add a "module" field that points
to your ESM bundle (e.g., "module": "dist/index.esm.js" or "dist/index.mjs") so
ESM-aware bundlers can pick up the ESM build while leaving "main" unchanged;
ensure the referenced filename matches your build output (update the build step
that emits the ESM artifact if needed) and keep "type" as-is.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json
- Add null check for nested objects and delete them if the result is null - Ensure proper handling of object keys during replacement
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 3: Revert the manual change to the "version" field in package.json (the
"version": "4.0.0-beta.0" line) so the file does not contain a committed version
bump; restore the previous value from the last commit or remove the change
entirely and ensure CI/CD/semantic-release remains responsible for versioning,
then commit that revert so future releases are produced by the automated
pipeline.
In `@src/detectTests.ts`:
- Around line 168-212: parseObject can return arrays or primitives from
JSON/YAML/XML parsing which violate the expected Record<string, any> return
type; update parseObject to only return plain objects: after each successful
parse path (parseXmlAttributes, JSON.parse results, YAML.parse results) verify
the value is a non-null object, not an array, and has Object.prototype as its
prototype (or use a helper isPlainObject) and if it fails that guard return
null; also ensure the final fallback no longer returns the original string but
returns null when the input isn't a plain object. Use references: parseObject,
parseXmlAttributes, JSON.parse, YAML.parse when locating the checks to add.
- Around line 314-337: The code currently constructs RegExp from config-driven
values (statementRegex and pattern) without validation; wrap the RegExp creation
in a try/catch and skip/emit a warning for invalid patterns, and additionally
run a safety check (e.g., use a safe-regex check or simple heuristics: max
pattern length, reject nested catastrophic quantifiers like (.+){n} or overly
permissive backtracking constructs) before compiling to avoid ReDoS; apply this
guard wherever new RegExp(statementRegex, "g") and new RegExp(pattern, "g") are
used (references: statementRegex, pattern, fileType.inlineStatements,
fileType.markup) and log which pattern was skipped so maintainers can fix
config.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/detectTests.ts
| { | ||
| "name": "doc-detective-common", | ||
| "version": "3.6.1-dev.2", | ||
| "version": "4.0.0-beta.0", |
There was a problem hiding this comment.
Avoid manual version bumps in package.json.
Please revert this manual version change and let CI/CD handle versioning to prevent drift.
Based on learnings: Do not commit version bumps manually in package.json - CI/CD workflows handle versioning automatically
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 3, Revert the manual change to the "version" field in
package.json (the "version": "4.0.0-beta.0" line) so the file does not contain a
committed version bump; restore the previous value from the last commit or
remove the change entirely and ensure CI/CD/semantic-release remains responsible
for versioning, then commit that revert so future releases are produced by the
automated pipeline.
- Update parseContent to skip empty httpRequest headers - Enhance tests for replaceNumericVariables and parseContent functions
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/detectTests.ts (2)
168-212:parseObjectcan still return arrays and primitives, mismatching its declared return type.
JSON.parse(line 178) andYAML.parse(line 204) can produce arrays, numbers,null, etc. Callers guard withtypeof !== 'object'(lines 376, 538), which catches primitives but not arrays (typeof [] === "object"). Consider addingArray.isArraychecks to the callers or withinparseObjectitself to prevent arrays from flowing into schema validation as test/step objects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detectTests.ts` around lines 168 - 212, parseObject currently returns whatever JSON.parse or YAML.parse yields (including arrays and primitives), which breaks callers expecting plain objects; update parseObject to validate parsed results and only return non-null objects that are not arrays: after each successful parse in parseObject (e.g., results from parseXmlAttributes, JSON.parse, YAML.parse, and any unescaped JSON branches), check typeof result === "object" && result !== null && !Array.isArray(result) and return the result only then, otherwise continue/fall through to return null; alternatively, if you prefer fixing callers, add Array.isArray checks where they currently guard with typeof !== "object" to also reject arrays.
315-357: Config-driven regex patterns are still unguarded against invalid/malicious patterns.
new RegExp(statementRegex, "g")(line 324) andnew RegExp(pattern, "g")(line 337) can throw on malformed patterns and are vulnerable to ReDoS with adversarial input. Consider wrapping intry/catchwith a warning log.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detectTests.ts` around lines 315 - 357, Wrap the dynamic RegExp constructions in try/catch so malformed or adversarial patterns don’t throw or crash detection: when creating new RegExp(statementRegex, "g") inside the statementTypes loop and new RegExp(pattern, "g") inside the fileType.markup processing, catch errors, log a warning including the offending pattern (use your existing logger) and skip that pattern; this prevents exceptions from propagating and skips invalid regexes while leaving the rest of detectTests.ts (the statementTypes forEach and markup.forEach handlers) to continue processing.
🧹 Nitpick comments (2)
src/detectTests.ts (1)
563-579: Consider cloning objects before passing tovalidate.The project convention is to clone objects before validation to avoid mutations. While the current code is likely safe (steps are already cloned via
replaceNumericVariables, and test objects aren't shared), thevalidatecalls at lines 518, 541, and 566 pass objects directly. Defensive cloning would align with the project guideline. Based on learnings: "Always clone objects before validation usingJSON.parse(JSON.stringify(object))to avoid mutations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detectTests.ts` around lines 563 - 579, The validate calls are receiving original objects which can be mutated; clone the object before passing it to validate (e.g., replace the direct argument `test` with a deep clone created via JSON.parse(JSON.stringify(...))) so validation runs on a copy; update the three validate usages (the ones validating steps/tests where variables like `tests`, `step`, and the call to `validate({ schemaKey: "test_v3", object: ... })` are used) to pass the cloned object and then push `validation.object` into `validatedTests` as before.test/detectTests.test.js (1)
11-34: Test fixture constants are defined outside thedescribeblock.
markdownFileTypeandminimalFileType(lines 11–32) are at module scope but indented as if they should be insidedescribe("detectTests module", ...)starting at line 34. Consider moving them inside thedescribeblock for consistent scoping and readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/detectTests.test.js` around lines 11 - 34, Move the test fixture constants markdownFileType and minimalFileType into the describe("detectTests module", ...) block so their scope is local to those tests; locate the top-level const declarations of markdownFileType and minimalFileType and cut/paste them immediately inside the describe callback (or inside a beforeEach if they need re-init per test), and update any references in the file to use the now-local constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/detectTests.test.js`:
- Around line 1105-1130: The test title is misleading because it never calls
replaceNumericVariables: parseContent processes the action "$1" as a string
(string-action branch) so replaceNumericVariables is not exercised; either
rename the test to reflect that it verifies skipping invalid string action names
(e.g., "should skip invalid string action names") or change the action from the
string "$1" to an object action (for example { checkLink: { url: "$1" } }) so
parseContent reaches the object-action branch and invokes
replaceNumericVariables; update the test name or the action accordingly and keep
references to the test case using parseContent and the action value "$1" to
locate the code to change.
- Around line 168-179: The second test is a duplicate and never reaches the
simple-unescape branch; update the "should fallback to simple unescaping" case
that calls parseObject({ stringifiedObject }) to use an input that causes
JSON.parse(JSON.parse(...)) to throw but where simple unescape + JSON.parse
succeeds (e.g. a string whose outer JSON is a quoted string containing escaped
quotes so the first JSON.parse yields a string that is not valid JSON but after
replacing /\\"/g with '"' becomes valid), or simply remove the redundant test
since "should handle escaped JSON where double-parse fails but simple unescape
succeeds" already covers it; locate and modify the test block using the
parseObject call and the duplicated escaped variable.
---
Duplicate comments:
In `@src/detectTests.ts`:
- Around line 168-212: parseObject currently returns whatever JSON.parse or
YAML.parse yields (including arrays and primitives), which breaks callers
expecting plain objects; update parseObject to validate parsed results and only
return non-null objects that are not arrays: after each successful parse in
parseObject (e.g., results from parseXmlAttributes, JSON.parse, YAML.parse, and
any unescaped JSON branches), check typeof result === "object" && result !==
null && !Array.isArray(result) and return the result only then, otherwise
continue/fall through to return null; alternatively, if you prefer fixing
callers, add Array.isArray checks where they currently guard with typeof !==
"object" to also reject arrays.
- Around line 315-357: Wrap the dynamic RegExp constructions in try/catch so
malformed or adversarial patterns don’t throw or crash detection: when creating
new RegExp(statementRegex, "g") inside the statementTypes loop and new
RegExp(pattern, "g") inside the fileType.markup processing, catch errors, log a
warning including the offending pattern (use your existing logger) and skip that
pattern; this prevents exceptions from propagating and skips invalid regexes
while leaving the rest of detectTests.ts (the statementTypes forEach and
markup.forEach handlers) to continue processing.
---
Nitpick comments:
In `@src/detectTests.ts`:
- Around line 563-579: The validate calls are receiving original objects which
can be mutated; clone the object before passing it to validate (e.g., replace
the direct argument `test` with a deep clone created via
JSON.parse(JSON.stringify(...))) so validation runs on a copy; update the three
validate usages (the ones validating steps/tests where variables like `tests`,
`step`, and the call to `validate({ schemaKey: "test_v3", object: ... })` are
used) to pass the cloned object and then push `validation.object` into
`validatedTests` as before.
In `@test/detectTests.test.js`:
- Around line 11-34: Move the test fixture constants markdownFileType and
minimalFileType into the describe("detectTests module", ...) block so their
scope is local to those tests; locate the top-level const declarations of
markdownFileType and minimalFileType and cut/paste them immediately inside the
describe callback (or inside a beforeEach if they need re-init per test), and
update any references in the file to use the now-local constants.
test/detectTests.test.js
Outdated
| it("should parse escaped JSON (double-encoded)", function () { | ||
| const escaped = '{\\"key\\": \\"value\\"}'; | ||
| const result = parseObject({ stringifiedObject: escaped }); | ||
| expect(result).to.deep.equal({ key: "value" }); | ||
| }); | ||
|
|
||
| it("should fallback to simple unescaping for escaped JSON", function () { | ||
| // This string looks like escaped JSON but double-parse fails | ||
| const escaped = '{\\"key\\": \\"value\\"}'; | ||
| const result = parseObject({ stringifiedObject: escaped }); | ||
| expect(result).to.not.be.null; | ||
| }); |
There was a problem hiding this comment.
Duplicate test input — "simple unescaping fallback" test doesn't exercise the intended path.
Lines 168–172 and 174–179 use the identical input string '{\\"key\\": \\"value\\"}'. The first parse attempt (double-encoded JSON) succeeds for both, so the test at line 174 labeled "should fallback to simple unescaping" never actually reaches the simple unescape fallback. To exercise that branch, you need input where JSON.parse('"' + input + '"') followed by JSON.parse(...) fails, but input.replace(/\\"/g, '"') followed by JSON.parse(...) succeeds.
Example input that actually exercises the fallback
it("should fallback to simple unescaping for escaped JSON", function () {
- // This string looks like escaped JSON but double-parse fails
- const escaped = '{\\"key\\": \\"value\\"}';
+ // Double-parse fails (intermediate contains backslash sequences),
+ // but simple unescape produces valid JSON
+ const escaped = '{\\"key\\": \\"val\\\\ue\\"}';
const result = parseObject({ stringifiedObject: escaped });
- expect(result).to.not.be.null;
+ expect(result).to.deep.equal({ key: "val\\ue" });
});Note: The test at line 203 ("should handle escaped JSON where double-parse fails but simple unescape succeeds") already covers this exact scenario, making the test at line 174 redundant in its current form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/detectTests.test.js` around lines 168 - 179, The second test is a
duplicate and never reaches the simple-unescape branch; update the "should
fallback to simple unescaping" case that calls parseObject({ stringifiedObject
}) to use an input that causes JSON.parse(JSON.parse(...)) to throw but where
simple unescape + JSON.parse succeeds (e.g. a string whose outer JSON is a
quoted string containing escaped quotes so the first JSON.parse yields a string
that is not valid JSON but after replacing /\\"/g with '"' becomes valid), or
simply remove the redundant test since "should handle escaped JSON where
double-parse fails but simple unescape succeeds" already covers it; locate and
modify the test block using the parseObject call and the duplicated escaped
variable.
- Add checks to ensure parsed results are objects and not arrays or null in parseObject function. - Update tests to verify that parseObject returns null for JSON arrays and primitives. - Add tests for handling escaped JSON that double-parses or simple-unescapes to arrays. - Improve error handling for malformed regex in inlineStatements and markup during content parsing. - Ensure that tests continue to process normally even when regex patterns are invalid.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/detectTests.test.js (2)
218-224: Weak assertion —to.not.be.undefineddoesn't verify the actual outcome.When both escaped-JSON parse attempts fail, the function falls through to YAML (which will also likely fail on this input) and returns
null. Assertingnot.be.undefinedpasses fornull, so this test doesn't validate the expected behavior. Consider assertingto.be.nullexplicitly.Proposed fix
it("should handle escaped JSON where both parse attempts fail", function () { // Input looks like escaped JSON but neither parse succeeds const input = '{\\"broken json\\"}'; const result = parseObject({ stringifiedObject: input }); - // Falls through to YAML parsing - expect(result).to.not.be.undefined; + // Falls through to YAML parsing, which also fails + expect(result).to.be.null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/detectTests.test.js` around lines 218 - 224, The test uses a weak assertion—when parseObject({ stringifiedObject: '{\\"broken json\\"}' }) falls through and returns null the current expect(...).to.not.be.undefined will still pass; update the test in detectTests.test.js to assert the explicit expected null result by replacing the assertion with expect(result).to.be.null (and update the test description to state that it should return null when both escaped-JSON parse attempts fail) so the behavior of parseObject is properly verified.
11-11: Minor: top-leveldescribehas stray leading whitespace.Line 11 has 2 extra spaces before
describe(...), but it's the outermost block. Cosmetic only.- describe("detectTests module", function () { +describe("detectTests module", function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/detectTests.test.js` at line 11, The top-level test block's declaration describe("detectTests module") has stray leading whitespace; remove the two extra spaces before the describe call so the outermost block is left-aligned (fix in the test/detectTests.test.js file by editing the line containing describe("detectTests module")).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/detectTests.test.js`:
- Around line 218-224: The test uses a weak assertion—when parseObject({
stringifiedObject: '{\\"broken json\\"}' }) falls through and returns null the
current expect(...).to.not.be.undefined will still pass; update the test in
detectTests.test.js to assert the explicit expected null result by replacing the
assertion with expect(result).to.be.null (and update the test description to
state that it should return null when both escaped-JSON parse attempts fail) so
the behavior of parseObject is properly verified.
- Line 11: The top-level test block's declaration describe("detectTests module")
has stray leading whitespace; remove the two extra spaces before the describe
call so the outermost block is left-aligned (fix in the test/detectTests.test.js
file by editing the line containing describe("detectTests module")).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dist/detectTests.d.ts.mapis excluded by!**/dist/**,!**/*.mapdist/detectTests.jsis excluded by!**/dist/**dist/detectTests.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/detectTests.tstest/detectTests.test.js
|
📝 Documentation updates detected! Updated existing suggestion: Update contributor docs for v4.0.0 (core bundling and common refactor) Tip: Adjust how proactive or focused Promptless is in Agent Settings ⚙️ |
Summary by CodeRabbit
New Features
Chores
Tests