Conversation
- Add c8 for coverage reporting with ratchet mechanism - Create test files for openapi, arazzo, utils, resolve, sanitize, telem modules - Add TDD skill and update AGENTS.md with testing strategy - Update CI workflows to run coverage checks - Coverage: 75% statements, 82% branches, 86% functions (225 tests)
- Add tests for fetchFile function with mocked axios - Add tests for replaceEnvs nested env var references - Coverage: 76% statements, 82% branches, 88% functions (232 tests)
Add 29 new tests for previously untested functions: - createRestApiClient: REST API client creation - getJobStatus: job status retrieval - buildFileMapping: DITA file mapping with image refs - searchFileByName: file search by name - uploadFile: file upload with content type detection - resolveFileId: file ID resolution chain - getResourceDependencies: resource dependency retrieval Coverage improvement: - heretto.js: 55.41% → 93.77% - Overall lines: 76.06% → 86.03% - Functions: 88% → 97.4%
- Update coverage thresholds to reflect new test coverage. - Add tests for fileTypes normalization in config. - Implement edge case tests for detectAndResolveTests and resolveTests. - Introduce error handling tests for OpenAPI module. - Add utility tests for qualifyFiles and parseTests functions.
|
Important Review skippedToo many files! 119 files out of 269 files are above the max files limit of 150. You can disable this status message by setting the WalkthroughAdds test coverage infrastructure and enforcement: c8 configuration and thresholds, a coverage ratchet script, coverage-aware npm scripts and CI workflow changes, extensive new/expanded test suites, documentation for TDD/coverage, and an updated Changes
Sequence Diagram(s)sequenceDiagram
actor CI
participant c8 as c8 (coverage)
participant Repo as Repository
participant Script as check-coverage-ratchet.js
participant Threshold as coverage-thresholds.json
CI->>Repo: checkout
CI->>c8: run tests (npm run test:coverage)
c8-->>CI: writes coverage/ (lcov, json, summary)
CI->>Repo: upload artifact (coverage/)
CI->>Script: run scripts/check-coverage-ratchet.js
Script->>Threshold: read thresholds
Script->>Repo: read coverage/coverage-summary.json
Script-->>CI: exit 0 (pass) or exit 1 (fail) -> gate release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @.claude/skills/tdd-coverage/SKILL.md:
- Around line 42-51: The documented coverage thresholds in SKILL.md are
outdated; update the table under "Current thresholds are in
`coverage-thresholds.json`" to match the values in coverage-thresholds.json by
replacing Lines/Statements/Functions/Branches with 87% / 87% / 97% / 84%
respectively so the markdown table reflects the current ratcheted thresholds.
In @.github/workflows/auto-dev-release.yml:
- Around line 95-101: The "Check coverage ratchet" step currently runs
regardless of whether the "Run tests with coverage" step succeeded; add an id to
the test step (e.g., id: run_tests) and change the ratchet step's conditional to
also require the test step succeeded (combine the existing check
'steps.check_changes.outputs.skip_release == 'false'' with
'steps.run_tests.conclusion == 'success'' or 'steps.run_tests.outcome ==
'success'') so the ratchet only runs when tests pass.
In @.github/workflows/integration-tests.yml:
- Line 1: The workflow YAML currently uses CRLF newlines and fails YAMLlint;
convert the file to use LF newlines (Unix style) — e.g., run dos2unix on the
file or set your editor to save with LF, then git add/commit; alternatively add
a .gitattributes entry like "*.yml text eol=lf" and run git add --renormalize to
enforce LF line endings before committing.
In @docs/plans/2026-01-07-test-coverage-strategy.md:
- Around line 1-142: Update the coverage phrasing to be a clear snapshot and fix
two wording nits: change the "Coverage Achieved: 75.47% statements, 82.37%
branches, 86.66% functions" header to "Coverage achieved (as of 2026-01-07):
75.47% statements, 82.37% branches, 86.66% functions" and adjust the final
bullet in the TDD Workflow from "Refactor if needed\nRun `npm run
coverage:ratchet` to verify coverage hasn't decreased" to a single sentence
"Refactor if needed. Run `npm run coverage:ratchet` to verify coverage hasn't
decreased" (locate these strings in the document around the top summary and the
"TDD Workflow" section).
In @package.json:
- Around line 6-16: The "test:all" and "test:all:coverage" scripts are named to
run all tests but currently only run unit tests; update the scripts in
package.json so they include both unit and integration patterns (e.g., run both
src/*.test.js and src/*.integration.test.js or use a combined glob) and preserve
existing options like --timeout and c8 for coverage; specifically modify the
"test:all" and "test:all:coverage" entries so they do not exclude integration
tests (remove the ignore or add the integration pattern) to align behavior with
their names.
In @scripts/check-coverage-ratchet.js:
- Around line 37-63: Validate that loadJSON returned the expected shapes before
computing diffs: ensure thresholds is an object with numeric thresholds[metric]
and coverage.total (current) is an object where current[metric] exists and
current[metric].pct is a number for each metric in metrics; if any metric is
missing or not numeric throw a clear error (or log and process.exit(1))
referencing THRESHOLDS_FILE/COVERAGE_FILE so the script fails fast instead of
producing NaN — implement these checks right after assigning thresholds and
current (use the same symbols thresholds, coverage, current and the metrics
loop) and only compute diff/status when both values are valid.
In @src/arazzo.js:
- Around line 34-37: Fix the typo in the user-visible warning message ("arne't"
→ "aren't") and replace direct console.warn calls with the project logger;
specifically update the block that checks if (!workflow) to call the repo's
logger.warn (e.g., logger.warn or projectLogger.warn) with the corrected message
including workflowId, ensure the logger is imported/available in src/arazzo.js,
and apply the same typo and logger replacement to the other occurrences around
lines 49-59 and 107-108 where console.warn is used.
In @src/index.test.js:
- Around line 116-204: Update the three tests to tighten assertions: in the
"empty array" case add an assertion that resolveDetectedTestsStub.notCalled is
true to confirm resolution is skipped and keep the existing log assertion; in
the "null tests" case add an assertion that logStub was called with the resolved
config, "warning", and "No tests detected." and assert
resolveDetectedTestsStub.notCalled; and in the "resolve config" case replace the
generic calledOnce check with calledOnceWithExactly({ config: configInput }) on
setConfigStub and add an assertion that
resolveDetectedTestsStub.calledOnceWithExactly({ config: configResolved,
detectedTests }) is true to validate correct invocation.
In @src/utils.test.js:
- Around line 429-433: The stderr test is flaky due to Node adding
platform-specific formatting when using console.error; update the test to emit
raw stderr instead by changing the spawned process code in the test (the call
using spawnCommand with "node" and the "-e" script) to use
process.stderr.write(...) instead of console.error(...), so the spawned output
will be just the message text and the assertion on result.stderr can reliably
check for "error message".
🧹 Nitpick comments (12)
src/sanitize.test.js (2)
7-43: Good coverage of commonsanitizeUricases; consider a couple more edge cases.This set is solid; I’d consider adding (if
sanitizeUrisupports it) a test for protocol case-insensitivity (HTTP://example.com) and a “don’t double-prefix” case (https://example.comwith leading/trailing whitespace).
45-90: MakesanitizePathtests OS/CWD-agnostic and cleanup more robust.
- Line 77:
/nonexistent/...is POSIX-specific and can behave unexpectedly on Windows.- Lines 81-85:
./src/sanitize.test.jsassumes the test CWD is repo root (can be false in some runners).- Lines 61-63: prefer
fs.rmSync(..., { recursive:true, force:true })for resilient cleanup.Proposed diff
@@ afterEach(function () { // Clean up temporary files - if (fs.existsSync(tempFile)) { - fs.unlinkSync(tempFile); - } - if (fs.existsSync(tempDir)) { - fs.rmdirSync(tempDir); - } + // One-shot cleanup (robust even if something already disappeared) + if (tempDir) fs.rmSync(tempDir, { recursive: true, force: true }); }); @@ it("should return null for non-existent path", function () { - const result = sanitizePath("/nonexistent/path/to/file.txt"); + const result = sanitizePath(path.join(tempDir, "definitely-does-not-exist.txt")); expect(result).to.be.null; }); @@ it("should resolve relative paths", function () { - // Use the current test file as a reference (we know it exists) - const result = sanitizePath("./src/sanitize.test.js"); - expect(result).to.equal(path.resolve("./src/sanitize.test.js")); + // CWD-agnostic: resolve an existing file relative to this test file's directory + const relToThisFile = "./sanitize.test.js"; + const result = sanitizePath(relToThisFile); + expect(result).to.equal(path.resolve(__dirname, relToThisFile)); });src/config.test.js (2)
406-562: Potentially brittle unit tests: these cases may fail schema validation before reaching normalization/extends logic.
If the intent is to specifically test normalization/extends behavior, consider proxyquiring./configwith a stubbeddoc-detective-common.validate(and keep fileType objects minimal-but-valid), so failures point to the normalization code rather than schema drift. Based on learnings, validate configuration handling thoroughly in tests.
564-608: Unit test depends on filesystem fixtures + real OpenAPI parsing; consider stubbingloadDescriptionto avoid flaky I/O.
dev/reqres.openapi.jsonis a repo/layout dependency; if this is meant to be a unit test, proxyquire./openapi.loadDescriptionand return a minimal OpenAPI object (and separately keep an integration-ish test if desired). As per coding guidelines, mock external dependencies (file system, HTTP requests) in tests.src/utils.test.js (1)
621-632: Consider expanding parseTests coverage in future.The single test verifies empty input handling, which is good baseline coverage. Consider adding tests for actual file parsing scenarios in future iterations to improve coverage.
src/heretto.test.js (2)
460-510: Ensure fake timers are always restored (avoid cross-test pollution).
Right nowclock.restore()won’t run if an assertion throws before the end of the test.Proposed fix
it("should poll until completion then validate assets", async function () { // Use fake timers to avoid waiting for real POLLING_INTERVAL_MS delays const clock = sinon.useFakeTimers(); + try { const assetsResponse = { content: [ { filePath: "ot-output/dita/my-guide.ditamap" }, ], totalPages: 1, }; @@ const result = await pollPromise; expect(result.status.result).to.equal("SUCCESS"); expect(mockClient.get.callCount).to.equal(4); // 3 status polls + 1 assets call - clock.restore(); + } finally { + clock.restore(); + } }); @@ it("should return null on timeout", async function () { // Use fake timers to avoid waiting for real timeout const clock = sinon.useFakeTimers(); + try { @@ const result = await pollPromise; expect(result).to.be.null; - clock.restore(); + } finally { + clock.restore(); + } });
668-691: Strengthen the ZIP path-traversal test assertion (prove it didn’t write outside output dir).
Currently it only checks “log called”, which can pass even if the malicious entry is still written.Example assertion upgrade
const result = await herettoWithMocks.downloadAndExtractOutput( @@ expect(result).to.not.be.null; // The warning log should be called for the malicious entry expect(mockLog.called).to.be.true; + // Ensure we never attempted to write the traversal path + const writes = fsMock.writeFileSync.getCalls().map(c => String(c.args[0])); + expect(writes.some(p => p.includes("etc/passwd"))).to.be.false; });scripts/check-coverage-ratchet.js (1)
85-92: Suggestion output: avoidMath.floor(actual)if you want accurate “ratchet-to-current” hints.
If you intend thresholds to track current coverage, considerMath.round(actual)or printing the exactactual(e.g., one decimal)..github/workflows/integration-tests.yml (1)
34-71: Harden artifact uploads with explicitif-no-files-foundsetting.The
if-no-files-foundparameter is supported inactions/upload-artifact@v4. Set it explicitly to avoid silent failures or unnecessary noise:
- Use
if-no-files-found: errorfor the coverage report to fail the workflow if coverage is unexpectedly absent.- Use
if-no-files-found: ignorefor test results if the log paths may not always be created.Node.js 24 is supported by
actions/setup-node@v4on ubuntu-latest. Note that Node 24 may not be pre-cached in the runner image, so the action will download it; this adds minor runtime overhead but is fully functional.Proposed changes
- name: Upload coverage report if: always() uses: actions/upload-artifact@v4 with: name: coverage-report path: coverage/ + if-no-files-found: error retention-days: 7 - name: Upload test results if: always() uses: actions/upload-artifact@v4 with: name: integration-test-results path: | test-results/ *.log + if-no-files-found: ignore retention-days: 7src/arazzo.test.js (3)
20-96: Consider verifying no unexpected warnings in success cases.The basic translation tests cover the happy path well. Consider adding assertions to verify that no warnings are emitted when translation succeeds, ensuring the function behaves quietly when everything works correctly.
Optional enhancement: Assert no warnings
For example, in the "should create a test with correct id from title" test:
it("should create a test with correct id from title", function () { const result = workflowToTest(basicArazzoDescription, "get-pets"); expect(result.id).to.equal("Test API Workflow"); + expect(consoleWarnStub.called).to.be.false; });Apply similar assertions to other success-path tests.
277-300: Consider verifying requestHeaders is also empty for path parameters.The test correctly checks that path parameters are not added to
requestParams, but the comment states they're not added torequestParamsorrequestHeaders. Consider also asserting thatrequestHeadersis either undefined or empty for completeness.Optional enhancement
// Path parameters are not added to requestParams or requestHeaders expect(result.steps[0].requestParams).to.deep.equal({}); + expect(result.steps[0].requestHeaders || {}).to.deep.equal({}); });
303-330: Good basic coverage; consider edge cases in future.The test covers the basic request body translation. For future enhancements, consider testing edge cases like empty payloads, deeply nested objects, or missing payload fields to ensure robust handling.
📜 Review details
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 (21)
.c8rc.json.claude/skills/tdd-coverage/SKILL.md.github/workflows/auto-dev-release.yml.github/workflows/integration-tests.ymlAGENTS.mdCLAUDE.mdcoverage-thresholds.jsondocs/plans/2026-01-07-test-coverage-strategy.mdlocal-llm/llama.cpppackage.jsonscripts/check-coverage-ratchet.jssrc/arazzo.jssrc/arazzo.test.jssrc/config.test.jssrc/heretto.test.jssrc/index.test.jssrc/openapi.test.jssrc/resolve.test.jssrc/sanitize.test.jssrc/telem.test.jssrc/utils.test.js
💤 Files with no reviewable changes (1)
- local-llm/llama.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/index.test.jssrc/utils.test.jssrc/openapi.test.jssrc/arazzo.jsscripts/check-coverage-ratchet.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.jssrc/sanitize.test.jssrc/config.test.jssrc/heretto.test.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/index.test.jssrc/utils.test.jssrc/openapi.test.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.jssrc/sanitize.test.jssrc/config.test.jssrc/heretto.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/index.test.jssrc/utils.test.jssrc/openapi.test.jssrc/arazzo.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.jssrc/sanitize.test.jssrc/config.test.jssrc/heretto.test.js
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/openapi.test.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.js.c8rc.jsonsrc/sanitize.test.jssrc/config.test.jssrc/heretto.test.jsAGENTS.md
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/openapi.test.jssrc/resolve.test.jssrc/sanitize.test.jssrc/config.test.jssrc/heretto.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.jssrc/config.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/openapi.test.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.jssrc/config.test.jssrc/heretto.test.jsAGENTS.md
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/openapi.test.jssrc/arazzo.test.jssrc/resolve.test.jssrc/telem.test.js.c8rc.jsonsrc/config.test.jssrc/heretto.test.jsAGENTS.md
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use Mocha for unit tests
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/resolve.test.jssrc/config.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use Chai for assertions
Applied to files:
src/index.test.js.claude/skills/tdd-coverage/SKILL.mdsrc/utils.test.jssrc/resolve.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
Applied to files:
src/index.test.jsdocs/plans/2026-01-07-test-coverage-strategy.md.claude/skills/tdd-coverage/SKILL.mdsrc/resolve.test.jssrc/telem.test.jssrc/config.test.jsAGENTS.md
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Use meaningful variable names that reflect Doc Detective terminology
Applied to files:
package.jsonAGENTS.md
🧬 Code graph analysis (6)
src/index.test.js (2)
src/index.js (3)
detectedTests(35-35)resolvedTests(41-41)resolvedTests(63-63)src/resolve.js (1)
resolvedTests(148-152)
src/utils.test.js (1)
src/utils.js (1)
timestamp(1201-1201)
src/arazzo.js (3)
src/resolve.js (4)
crypto(1-1)require(2-2)require(3-3)openApiDefinition(119-121)src/utils.js (5)
crypto(3-3)require(7-7)require(8-13)require(14-14)test(1023-1029)src/arazzo.test.js (1)
require(2-2)
src/arazzo.test.js (1)
src/openapi.test.js (16)
result(59-59)result(73-73)result(164-164)result(169-169)result(178-178)result(184-190)result(212-212)result(218-218)result(226-226)result(282-282)result(289-289)result(295-295)result(301-301)result(350-355)result(413-413)result(421-421)
src/config.test.js (2)
src/utils.js (4)
fileType(1012-1014)require(7-7)require(8-13)require(14-14)src/config.js (3)
require(2-2)require(3-3)require(4-4)
src/heretto.test.js (1)
src/heretto.js (4)
authHeader(44-47)authHeader(64-67)ditamapInfo(467-467)restClient(597-597)
🪛 GitHub Actions: Test (& Publish)
src/utils.test.js
[error] 432-432: spawnCommand should capture stderr failed. AssertionError: expected '[eval]:1\r\nconsole.error('error\r\n…' to include 'error message'
🪛 LanguageTool
docs/plans/2026-01-07-test-coverage-strategy.md
[style] ~3-~3: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...etective-resolver Date: January 7, 2026 Status: Implemented **Coverage ...
(MISSING_COMMA_AFTER_YEAR)
[style] ~142-~142: The double modal “needed Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ...t code (green) 3. Refactor if needed 4. Run npm run coverage:ratchet to verify co...
(NEEDS_FIXED)
🪛 YAMLlint (1.37.1)
.github/workflows/auto-dev-release.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
.github/workflows/integration-tests.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (40)
src/sanitize.test.js (1)
1-6: The import path is correct and will not break. Bothsanitize.jsandsanitize.test.jsare in thesrc/directory, and tests run directly on source files viamocha src/*.test.jswith no transpilation step. The relative import correctly resolves to the exported functions.Likely an incorrect or invalid review comment.
CLAUDE.md (1)
1-33: All documented npm scripts in the "Quick Reference" section matchpackage.jsonand are accurate. The minimal approach is intentional for a quick reference—no doc drift risk here.AGENTS.md (1)
140-203: Update log level documentation from"warn"to"warning"to match actual codebase usage.The codebase consistently uses
"warning"as the log level (not"warn"), as seen throughoutsrc/utils.js,src/utils.test.js,src/index.js,src/heretto.js, andsrc/config.js. AGENTS.md must document the correct level:log(config, "warning", "Your message here");Available log levels:
debug,info,warning,errorAll referenced npm scripts (
test,test:coverage,test:integration,test:integration:coverage,test:all:coverage,coverage:check,coverage:ratchet) exist and are correctly documented.⛔ Skipped due to learnings
Learnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Use Mocha for unit testsLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Use Chai for assertionsLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in testsLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to src/**/*.js : Use the built-in logging system with available log levels: debug, info, warn, errorLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in testsLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patternsLearnt from: CR Repo: doc-detective/resolver PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-25T23:59:45.298Z Learning: Applies to **/*.test.js : Test both successful and error scenarios.claude/skills/tdd-coverage/SKILL.md (4)
68-107: LGTM! Well-structured test pattern.The test structure follows Mocha/Chai conventions with proper setup/teardown and logical organization into input validation, happy paths, and edge cases. This aligns with the coding guidelines.
141-185: LGTM! Testing patterns are sound.The async, mocking, and error handling patterns demonstrate proper test practices including cleanup in finally blocks and appropriate use of Sinon stubs.
226-231: LGTM! Project-specific context is helpful.The notes provide valuable project-specific guidance for test authors, particularly around log suppression and API loading utilities.
119-139: All npm scripts listed in the Commands Reference are properly defined inpackage.jsonand are ready to use.src/telem.test.js (3)
1-23: LGTM! Proper test setup and teardown.The test setup correctly stubs console.log and manages environment variables with proper restoration in afterEach. This prevents test pollution.
24-74: LGTM! Comprehensive telemetryNotice coverage.The tests cover all relevant scenarios including disabled, enabled, unconfigured, and undefined config cases. Proper use of console stub assertions.
76-216: LGTM! Thorough sendTelemetry testing.The tests comprehensively cover telemetry behavior across multiple scenarios. The approach of verifying non-throwing execution is pragmatic given the external PostHog dependency. Good coverage of edge cases like nested objects and environment variable integration.
src/openapi.test.js (6)
1-31: LGTM! Proper dependency mocking setup.The use of proxyquire for dependency injection is appropriate, and stubs are properly managed in beforeEach/afterEach hooks. The global.expect pattern works well for the test suite.
33-78: LGTM! loadDescription tests cover key scenarios.The tests appropriately cover error handling for missing paths and successful loading from both file paths and URLs. Good verification of stub call arguments.
80-230: LGTM! Comprehensive getOperation baseline tests.The tests thoroughly cover input validation, operation lookup, server URL resolution, path parameter substitution, and schema handling. The mock OpenAPI definition is realistic and appropriate.
232-305: LGTM! Complex parameter handling well-tested.The tests verify proper extraction of query parameters, headers, and response examples from OpenAPI definitions. Good use of realistic parameter scenarios.
307-427: LGTM! Examples and nested schema tests are thorough.The tests appropriately cover named example selection and generation from nested schemas including objects and arrays. Good coverage of complex data structures.
429-672: LGTM! Comprehensive edge case coverage.The edge case tests are thorough, including operations without parameters/requestBody, empty servers, and schema-based generation. Line 517-546 appropriately documents a known limitation with responses lacking content. Good testing practice to document known behavior even if not ideal.
src/utils.test.js (12)
41-117: LGTM! Comprehensive logging tests.The tests thoroughly cover all log levels, filtering behavior, and message formatting including object serialization to JSON.
119-134: LGTM! Timestamp tests are adequate.The tests verify the timestamp format and include current date components, which is sufficient for this utility function.
237-257: LGTM! URL relativity tests cover key cases.The tests appropriately distinguish between absolute URLs (HTTP/HTTPS/file://) and relative paths.
259-297: LGTM! Heretto integration finding tests are thorough.The tests cover all relevant scenarios for path-based integration discovery including no mapping, no match, single match, and multiple mappings.
299-327: LGTM! Percentage difference tests cover edge cases.The tests appropriately cover identical, completely different, partial differences, and edge cases like empty strings and different lengths.
329-353: LGTM! loadEnvs tests cover success and failure.The tests verify both success and failure paths for environment file loading.
355-381: LGTM! cleanTemp tests verify cleanup behavior.The tests appropriately cover both scenarios: temp directory absent (no-op) and present (cleanup).
383-410: LGTM! outputResults tests verify file writing.The tests appropriately verify both file path and JSON pretty-printing behavior.
443-475: LGTM! inContainer tests handle environment properly.The tests correctly verify container detection based on environment variables with proper save/restore of original state.
477-557: LGTM! fetchFile tests comprehensively cover HTTP fetching.The tests thoroughly cover successful fetches, JSON handling, error scenarios, temp directory creation, and caching behavior. Proper use of axios stubs.
559-619: LGTM! qualifyFiles tests cover main scenarios.The tests cover empty inputs, valid spec files, URL sources, and heretto sources. While more comprehensive testing could be beneficial, this provides solid baseline coverage.
136-235: The JSON parse branch in the implementation is confirmed unreachable—no changes needed to tests.The tests correctly document that the condition
JSON.parse(stringOrObject) === "object"at line 1180 is unreachable: sincestringOrObjectis always the literal string"$VAR"(not the parsed JSON value), it will never be valid JSON. The tests provide comprehensive coverage of replaceEnvs functionality, including null/undefined handling, string replacement, recursive object/array processing, and proper documentation of the dead code branch. The dead code exists in the implementation (src/utils.js), not in the tests, and the tests correctly surface this limitation.src/resolve.test.js (3)
1-95: LGTM! Basic resolution tests establish solid foundation.The tests cover empty inputs, single specs/tests, and automatic ID generation. Proper verification of UUID format for generated IDs. Good test setup with console stub.
96-457: LGTM! Exceptional coverage of runOn and context resolution.The tests comprehensively cover configuration inheritance, browser/platform normalization (including safari→webkit), type conversions, unsafe flag propagation, context deduplication, and the full hierarchy of runOn precedence. This is exemplary test coverage.
459-708: LGTM! Comprehensive OpenAPI integration testing.The tests thoroughly cover OpenAPI integration including:
- Graceful handling of failed loads
- Integration with config.integrations.openApi
- Definition replacement and merging
- Test-level and spec-level openApi
- Successful loading from actual files with verification
Excellent coverage of a complex integration feature.
coverage-thresholds.json (1)
1-6: LGTM: clear ratchet thresholds.
Just ensure these numbers reflect the currentmainbaseline (so the first PR after merge doesn’t fail the ratchet)..c8rc.json (1)
1-12: LGTM: c8 config matches the ratchet flow (json-summary + coverage dir).src/heretto.test.js (1)
55-71: No action required. BothcreateApiClientandcreateRestApiClientare defined (lines 43 and 63 in src/heretto.js) and explicitly exported (lines 1042-1043). No API mismatch exists; the tests are calling valid, exported APIs.Likely an incorrect or invalid review comment.
.github/workflows/auto-dev-release.yml (1)
1-1: Static analysis hint can be safely ignored.The yamllint warning about line endings is a common false positive. Git normalizes line endings automatically based on repository configuration, and the workflow is already functioning correctly in CI.
src/arazzo.test.js (5)
1-18: LGTM! Clean test setup.The test setup follows best practices with proper stub initialization and cleanup. Dynamic import of Chai handles ESM compatibility correctly, and sinon restoration in afterEach ensures test isolation.
98-112: LGTM! Good error handling coverage.The test properly validates both the return value and warning behavior when a workflow is not found, following best practices for testing error scenarios.
114-190: LGTM! Comprehensive step translation coverage.The tests thoroughly cover supported (operationId) and unsupported (operationPath, workflowId, unknown) step types, with proper validation of warning messages and skip behavior.
332-406: LGTM! Thorough success criteria coverage.The tests comprehensively cover status code criteria, response body criteria, and multiple criteria combinations, ensuring proper translation of Arazzo success criteria to Doc Detective test format.
408-462: Excellent integration test!This comprehensive end-to-end test validates complete workflow translation with multiple steps, parameters, request data, and success criteria. It provides valuable coverage of how all the translation components work together in a realistic scenario.
Based on learnings, this test follows best practices by using realistic test data that matches actual usage patterns and thoroughly validating configuration handling.
| "scripts": { | ||
| "test": "mocha src/*.test.js --ignore src/*.integration.test.js", | ||
| "test:coverage": "c8 mocha src/*.test.js --ignore src/*.integration.test.js", | ||
| "test:integration": "mocha src/*.integration.test.js --timeout 600000", | ||
| "test:integration:coverage": "c8 mocha src/*.integration.test.js --timeout 600000", | ||
| "test:all": "mocha src/*.test.js --timeout 600000", | ||
| "test:all:coverage": "c8 mocha src/*.test.js --timeout 600000", | ||
| "coverage:check": "c8 check-coverage", | ||
| "coverage:report": "c8 report", | ||
| "coverage:ratchet": "node scripts/check-coverage-ratchet.js", | ||
| "dev": "node dev" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && git ls-files package.jsonRepository: doc-detective/resolver
Length of output: 121
🏁 Script executed:
cat -n package.json | head -60Repository: doc-detective/resolver
Length of output: 2255
🏁 Script executed:
# Check if integration test files exist
find . -type f -name "*.integration.test.js" 2>/dev/null | head -20Repository: doc-detective/resolver
Length of output: 99
🏁 Script executed:
# Check the c8 and mocha versions in package.json
rg '"mocha"|"c8"' package.json -A 1Repository: doc-detective/resolver
Length of output: 166
🌐 Web query:
mocha multiple glob patterns test files syntax site:mochajs.org
💡 Result:
You can supply multiple globs either on the command line as separate positional arguments, or in config using the spec array.
-
Command line: pass multiple quoted globs: e.g.
mocha "test/unit//*.js" "test/integration//*.js". [1] (mochajs.org) -
Config file (.mocharc.js/.mocharc.json or package.json): set spec to an array:
{ "spec": ["test/unit//*.js", "test/integration//*.js"] }. [2][3] (mochajs.org)
Notes: quote globs for portability; --recursive is an alternative for recursive matching; command-line positional specs and config spec values are concatenated (command-line takes priority). [1][2] (mochajs.org)
Citations:
- 1: https://mochajs.org/next/running/test-globs/?utm_source=openai
- 2: https://mochajs.org/running/configuring/?utm_source=openai
- 3: https://mochajs.org/next/running/test-globs/?utm_source=openai
🌐 Web query:
c8 mocha multiple globs coverage syntax
💡 Result:
Short answer — use mocha with multiple quoted globs as positional args and pass c8 include/exclude as repeated flags (or arrays in config). Examples:
-
CLI (repeat --include):
npx c8 --all --include "src//*.js" --include "lib//.js" --exclude "test/" mocha "test/unit//.spec.js" "test/integration/**/*.spec.js". [1][2] -
package.json c8 config:
"c8": {
"all": true,
"include": ["src//*.js","lib//.js"],
"exclude": ["test/"]
},
"scripts": {
"test:cov": "c8 mocha "test/unit//.spec.js" "test/integration/**/*.spec.js""
} [1][2]
Notes:
- Quote globs in npm scripts so the shell doesn’t expand them (Mocha guidance). [2]
- c8’s CLI/config options for include/exclude are arrays (you may repeat flags on the command line or supply an array in config). [1]
Sources:
[1] c8 README (CLI options & config).
[2] Mocha docs — test globs.
test:all / test:all:coverage don't include integration tests—fix the naming vs behavior mismatch.
These scripts should run both unit and integration tests to match their names. src/*.integration.test.js exists but is currently excluded.
Proposed fix
- "test:all": "mocha src/*.test.js --timeout 600000",
+ "test:all": "mocha \"src/*.test.js\" \"src/*.integration.test.js\" --timeout 600000",
@@
- "test:all:coverage": "c8 mocha src/*.test.js --timeout 600000",
+ "test:all:coverage": "c8 mocha \"src/*.test.js\" \"src/*.integration.test.js\" --timeout 600000",📝 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.
| "scripts": { | |
| "test": "mocha src/*.test.js --ignore src/*.integration.test.js", | |
| "test:coverage": "c8 mocha src/*.test.js --ignore src/*.integration.test.js", | |
| "test:integration": "mocha src/*.integration.test.js --timeout 600000", | |
| "test:integration:coverage": "c8 mocha src/*.integration.test.js --timeout 600000", | |
| "test:all": "mocha src/*.test.js --timeout 600000", | |
| "test:all:coverage": "c8 mocha src/*.test.js --timeout 600000", | |
| "coverage:check": "c8 check-coverage", | |
| "coverage:report": "c8 report", | |
| "coverage:ratchet": "node scripts/check-coverage-ratchet.js", | |
| "dev": "node dev" | |
| "scripts": { | |
| "test": "mocha src/*.test.js --ignore src/*.integration.test.js", | |
| "test:coverage": "c8 mocha src/*.test.js --ignore src/*.integration.test.js", | |
| "test:integration": "mocha src/*.integration.test.js --timeout 600000", | |
| "test:integration:coverage": "c8 mocha src/*.integration.test.js --timeout 600000", | |
| "test:all": "mocha \"src/*.test.js\" \"src/*.integration.test.js\" --timeout 600000", | |
| "test:all:coverage": "c8 mocha \"src/*.test.js\" \"src/*.integration.test.js\" --timeout 600000", | |
| "coverage:check": "c8 check-coverage", | |
| "coverage:report": "c8 report", | |
| "coverage:ratchet": "node scripts/check-coverage-ratchet.js", | |
| "dev": "node dev" |
🤖 Prompt for AI Agents
In @package.json around lines 6 - 16, The "test:all" and "test:all:coverage"
scripts are named to run all tests but currently only run unit tests; update the
scripts in package.json so they include both unit and integration patterns
(e.g., run both src/*.test.js and src/*.integration.test.js or use a combined
glob) and preserve existing options like --timeout and c8 for coverage;
specifically modify the "test:all" and "test:all:coverage" entries so they do
not exclude integration tests (remove the ignore or add the integration pattern)
to align behavior with their names.
|
📝 Documentation updates detected! Updated existing suggestion: Document doc-detective-resolver dependency and test coverage requirements |
- Increase coverage thresholds for lines, statements, functions, and branches. - Add validation for thresholds and coverage structures in check-coverage-ratchet.js. - Modify integration test workflows to ensure tests run only on successful coverage checks. - Improve logging in workflowToTest function for better error handling. - Update utils tests to ensure proper command execution and error capturing.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/auto-dev-release.yml:
- Line 1: Convert the workflow file to use LF line endings (replace CRLF with
\n) so the YAML linter error is resolved for the file named
auto-dev-release.yml; commit the normalized file. Add a .gitattributes entry
such as "*.yml text eol=lf" to enforce LF for YAML files and run git add
--renormalize . (or your repo’s normalization step) before committing to ensure
all files adhere to the new rule.
- Around line 63-90: The current checks using node -p
"require('./package.json').name" can print "undefined" but still exit 0, so
change the checks to actively fail when fields are missing by evaluating and
throwing on falsy values; specifically replace the two node -p checks with node
-e snippets that require('./package.json') into a variable and throw an error
(causing non-zero exit) if .name or .version is falsy/undefined, so the workflow
exits with failure when those fields are absent.
- Around line 121-123: The workflow currently hard-codes the package name in the
LATEST_DEV line (LATEST_DEV=$(npm view doc-detective-resolver@dev ...)), which
will break if package.json#name changes; change it to derive the package name
from package.json (e.g., compute PKG_NAME by reading package.json.name using
node -p "require('./package.json').name" or jq .name) and then call npm view
"$PKG_NAME@dev" version (replace the hard-coded doc-detective-resolver with the
computed PKG_NAME) so LATEST_DEV is always fetched for the actual package named
in package.json.
- Around line 38-61: The doc-only detector in the "Check for documentation-only
changes" step incorrectly treats all .yml/.yaml and everything under .github/ as
documentation; update the CHANGED_FILES check used in that step (the grep line)
to stop classifying YAML and .github files as docs—e.g., remove yml|yaml and
^\.github/ from the regex and instead only treat explicit doc paths/extensions
(for example use grep -v -E '\.(md|txt)$|^docs/' against CHANGED_FILES). Ensure
the job id check_changes uses the new pattern and keep the same skip_release
output logic/messages.
In @src/arazzo.js:
- Line 12: The workflowToTest function declares an unused parameter inputs;
remove inputs from the function signature in workflowToTest and update its JSDoc
to delete the "@param {Object} inputs" entry (or if inputs is actually required,
implement its usage where workflowToTest builds/validates input data — reference
the workflowToTest function name to locate the change). Ensure any callers are
updated if you remove the parameter.
- Around line 109-112: In the branch that handles status code criteria (the if
checking criterion.condition.startsWith("$statusCode")), guard against missing
"==" before splitting, extract the RHS safely (e.g., use indexOf("==") and slice
or split and check length), trim it, parse with parseInt(value, 10) and validate
the result is a finite number before assigning to docDetectiveStep.statusCodes
(or skip/throw/log if invalid); this ensures you never call parseInt(undefined)
and always use an explicit radix when parsing the status code.
In @src/utils.test.js:
- Around line 429-434: The test "should capture stderr" is flaky because using
process.stderr.write may not flush or behave consistently across platforms;
update the test that calls spawnCommand to emit stderr via console.error and
explicitly exit so the output is flushed (e.g., change the inline node
invocation used in the test to call console.error('error message') and then
process.exit(0)); this targets the test block using spawnCommand in
src/utils.test.js so the captured result.stderr will reliably include "error
message".
🧹 Nitpick comments (6)
src/arazzo.js (2)
39-43: Consider extracting repeated logging pattern.The same conditional logging pattern appears 4 times. A small helper would reduce duplication:
Suggested helper
+function warn(config, message) { + if (config) { + log(config, "warning", message); + } else { + console.warn(message); + } +} + function workflowToTest(arazzoDescription, workflowId, inputs, config) { // ... if (!workflow) { - if (config) { - log(config, "warning", `Workflow with ID ${workflowId} not found.`); - } else { - console.warn(`Workflow with ID ${workflowId} not found.`); - } + warn(config, `Workflow with ID ${workflowId} not found.`); return; }Also applies to: 59-63
86-98: EmptyrequestParamsmay be left on step.
requestParamsis initialized unconditionally whenparametersexist, but only populated forqueryparams. If onlyheaderparams are present, an emptyrequestParamsobject remains on the step.Suggested fix
if (workflowStep.parameters) { - docDetectiveStep.requestParams = {}; workflowStep.parameters.forEach((param) => { if (param.in === "query") { + if (!docDetectiveStep.requestParams) + docDetectiveStep.requestParams = {}; docDetectiveStep.requestParams[param.name] = param.value; } else if (param.in === "header") { if (!docDetectiveStep.requestHeaders) docDetectiveStep.requestHeaders = {}; docDetectiveStep.requestHeaders[param.name] = param.value; } }); }src/utils.test.js (3)
25-28: Global namespace pollution withexpect.While this workaround is necessary for ESM-only Chai in CommonJS, consider documenting this pattern or extracting it to a test helper to make it clearer why
global.expectis used.
316-320: Empty string test could be more specific.The comment notes "NaN or 0 depending on implementation" but only asserts the result is a string. Consider validating the actual expected behavior to prevent regressions.
Suggested improvement
it("should handle empty strings", function () { const result = calculatePercentageDifference("", ""); - // Both empty - NaN or 0 depending on implementation - expect(result).to.be.a("string"); + // Division by zero (maxLength=0) results in NaN, which stringifies to "NaN" + expect(result).to.satisfy((r) => r === "NaN" || r === "0.00"); });
490-492: Redundant stub restoration.The
axiosGetStub.restore()call is redundant sincesinon.restore()in the outerafterEach(line 38) already restores all stubs. Consider removing the explicit restore.Suggested fix
- afterEach(function () { - axiosGetStub.restore(); - });.github/workflows/auto-dev-release.yml (1)
14-19:timeout-minutes: 5is likely too aggressive for coverage + publish.
If this flakes even occasionally, releases will be unreliable; consider raising (or splitting publish into a separate job gated by the coverage job).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.claude/skills/tdd-coverage/SKILL.md.github/workflows/auto-dev-release.yml.github/workflows/integration-tests.ymlpackage.jsonscripts/check-coverage-ratchet.jssrc/arazzo.jssrc/index.test.jssrc/utils.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- .claude/skills/tdd-coverage/SKILL.md
- .github/workflows/integration-tests.yml
- package.json
- scripts/check-coverage-ratchet.js
- src/index.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/arazzo.jssrc/utils.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/arazzo.jssrc/utils.test.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/utils.test.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.{js,ts} : Use meaningful variable names that reflect Doc Detective terminology
Applied to files:
src/arazzo.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use Chai for assertions
Applied to files:
src/utils.test.js
🧬 Code graph analysis (1)
src/utils.test.js (1)
src/utils.js (1)
timestamp(1201-1201)
🪛 GitHub Actions: Test (& Publish)
src/utils.test.js
[error] 433-433: spawnCommand should capture stderr failed. AssertionError: expected stderr to contain 'error message' but it did not.
🪛 YAMLlint (1.37.1)
.github/workflows/auto-dev-release.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (9)
src/arazzo.js (1)
14-31: LGTM!The initialization structure and OpenAPI source translation logic are well-implemented. The test object is correctly populated with the Arazzo description metadata and OpenAPI definitions.
src/utils.test.js (6)
526-545: Test manipulates actual filesystem, risking flakiness.This test deletes and recreates the actual
doc-detectivetemp directory. Per coding guidelines, external dependencies (file system) should be mocked. This approach risks:
- Interference with parallel test execution
- Pollution affecting other tests relying on the temp directory
Consider mocking
fsoperations or using a test-specific temp path.Based on learnings, mock external dependencies in tests.
136-235: LGTM!Thorough coverage of
replaceEnvsincluding edge cases for null/undefined, nested objects, arrays, and env var resolution. The comments documenting unreachable code branches (lines 199-211, 224-234) are helpful for understanding the function's behavior.
41-117: LGTM!Comprehensive coverage of the
logfunction including all log levels and proper filtering behavior. Tests both positive cases (messages that should be logged) and negative cases (messages that should be filtered).
471-479: Weak assertion is environment-dependent by design.The assertion only checks
typeof result === "boolean"because the actual value depends on whether tests run in a container. The comment explains this, so the approach is reasonable.
577-591: LGTM!Using actual test artifacts (
./test/artifacts/test.spec.json) aligns with the coding guideline to use realistic test data. The test validates real file qualification behavior.
1-28: Well-structured test suite.The test file provides comprehensive coverage across multiple utility functions with proper setup/teardown, following Mocha/Chai patterns as per coding guidelines. The dynamic import workaround for ESM Chai is correctly implemented.
.github/workflows/auto-dev-release.yml (2)
30-37: Verify Node 24 is the intended runtime for this repo + dependency set.
actions/setup-nodesupports major-version syntax like24, but confirm your supported engines/tooling matrix (and any native deps) is aligned with Node 24. (github.com)
95-103: Coverage gate in the release workflow is a solid guardrail.
Runningtest:coverageand gating oncoverage:ratchetbefore version/publish aligns well with the PR’s coverage/robustness objective.
| - name: Validate package.json | ||
| if: steps.check_changes.outputs.skip_release == 'false' | ||
| run: | | ||
| # Validate package.json exists and is valid JSON | ||
| if [ ! -f "package.json" ]; then | ||
| echo "❌ package.json not found" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Validate JSON syntax | ||
| if ! node -p "JSON.parse(require('fs').readFileSync('package.json', 'utf8'))" > /dev/null 2>&1; then | ||
| echo "❌ package.json is not valid JSON" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check for required fields | ||
| if ! node -p "require('./package.json').name" > /dev/null 2>&1; then | ||
| echo "❌ package.json missing 'name' field" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! node -p "require('./package.json').version" > /dev/null 2>&1; then | ||
| echo "❌ package.json missing 'version' field" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "✅ package.json validation passed" | ||
|
|
There was a problem hiding this comment.
package.json “required fields” validation doesn’t actually fail when fields are missing.
node -p "require('./package.json').name" prints undefined but exits 0, so the workflow will incorrectly pass this gate.
Proposed fix (make the checks real + fail fast)
- name: Validate package.json
if: steps.check_changes.outputs.skip_release == 'false'
run: |
+ set -euo pipefail
# Validate package.json exists and is valid JSON
if [ ! -f "package.json" ]; then
echo "❌ package.json not found"
exit 1
fi
# Validate JSON syntax
if ! node -p "JSON.parse(require('fs').readFileSync('package.json', 'utf8'))" > /dev/null 2>&1; then
echo "❌ package.json is not valid JSON"
exit 1
fi
# Check for required fields
- if ! node -p "require('./package.json').name" > /dev/null 2>&1; then
- echo "❌ package.json missing 'name' field"
- exit 1
- fi
-
- if ! node -p "require('./package.json').version" > /dev/null 2>&1; then
- echo "❌ package.json missing 'version' field"
- exit 1
- fi
+ node - <<'NODE'
+ const pkg = require('./package.json');
+ const required = ['name', 'version'];
+ for (const k of required) {
+ const v = pkg?.[k];
+ if (typeof v !== 'string' || v.trim().length === 0) {
+ throw new Error(`package.json missing/invalid '${k}' field`);
+ }
+ }
+ NODE
echo "✅ package.json validation passed"🤖 Prompt for AI Agents
In @.github/workflows/auto-dev-release.yml around lines 63 - 90, The current
checks using node -p "require('./package.json').name" can print "undefined" but
still exit 0, so change the checks to actively fail when fields are missing by
evaluating and throwing on falsy values; specifically replace the two node -p
checks with node -e snippets that require('./package.json') into a variable and
throw an error (causing non-zero exit) if .name or .version is falsy/undefined,
so the workflow exits with failure when those fields are absent.
| # Check if we need to get the latest dev version from npm | ||
| LATEST_DEV=$(npm view doc-detective-resolver@dev version 2>/dev/null || echo "") | ||
|
|
There was a problem hiding this comment.
Avoid hard-coding the npm package name when computing the next -dev.N version.
If package.json#name ever differs, npm view doc-detective-resolver@dev will query the wrong package and can cause version collisions / publish failures.
Proposed fix (derive package name from package.json)
- # Check if we need to get the latest dev version from npm
- LATEST_DEV=$(npm view doc-detective-resolver@dev version 2>/dev/null || echo "")
+ # Check if we need to get the latest dev version from npm
+ PACKAGE_NAME=$(node -p "require('./package.json').name")
+ LATEST_DEV=$(npm view "${PACKAGE_NAME}@dev" version 2>/dev/null || echo "")📝 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.
| # Check if we need to get the latest dev version from npm | |
| LATEST_DEV=$(npm view doc-detective-resolver@dev version 2>/dev/null || echo "") | |
| # Check if we need to get the latest dev version from npm | |
| PACKAGE_NAME=$(node -p "require('./package.json').name") | |
| LATEST_DEV=$(npm view "${PACKAGE_NAME}@dev" version 2>/dev/null || echo "") |
🤖 Prompt for AI Agents
In @.github/workflows/auto-dev-release.yml around lines 121 - 123, The workflow
currently hard-codes the package name in the LATEST_DEV line (LATEST_DEV=$(npm
view doc-detective-resolver@dev ...)), which will break if package.json#name
changes; change it to derive the package name from package.json (e.g., compute
PKG_NAME by reading package.json.name using node -p
"require('./package.json').name" or jq .name) and then call npm view
"$PKG_NAME@dev" version (replace the hard-coded doc-detective-resolver with the
computed PKG_NAME) so LATEST_DEV is always fetched for the actual package named
in package.json.
- Replace console.error with a temporary script file to handle stderr - Ensure cross-platform compatibility by avoiding shell quoting issues - Add cleanup for temporary files after test execution
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/utils.test.js:
- Around line 535-554: The test "should create temp directory if it does not
exist" manually cleans a shared `${os.tmpdir()}/doc-detective` which can break
parallel tests; change the test to use a test-specific temp directory or mock
the temp path used by fetchFile so it creates a unique directory per run (e.g.,
use fs.mkdtemp or build a unique name with os.tmpdir + random/suffix) and ensure
the test cleans up only that directory afterwards, or stub the helper that
returns the temp dir so fetchFile writes to an isolated path; update assertions
to check the test-specific path instead of the shared `tempDir`.
- Around line 224-234: The JSON.parse branch in utils.js is unreachable because
it calls JSON.parse(stringOrObject) where stringOrObject is the literal variable
reference like "$OBJECT_VAR"; update the logic in the replaceEnvs function to
parse the resolved environment value instead (use JSON.parse(value) after
resolving stringOrObject to value) or remove the dead branch if parsing value is
not intended; ensure any thrown parse errors are handled explicitly rather than
silently swallowed so tests and runtime behavior reflect real parse failures.
🧹 Nitpick comments (4)
src/utils.test.js (4)
25-28: Consider static import for clarity.The dynamic import of chai is unconventional. While it works, a standard
requireat the top of the file would be more explicit and easier to follow.♻️ Suggested refactor
+const { expect } = require("chai"); const sinon = require("sinon"); const proxyquire = require("proxyquire"); const fs = require("fs"); const os = require("os"); const path = require("path"); // Import the functions we're testing const { log, timestamp, replaceEnvs, loadEnvs, outputResults, cleanTemp, fetchFile, isRelativeUrl, findHerettoIntegration, calculatePercentageDifference, inContainer, spawnCommand, qualifyFiles, parseTests, } = require("./utils"); -before(async function () { - const { expect } = await import("chai"); - global.expect = expect; -});
316-320: Strengthen empty string assertion.The test checks that the result is a string but doesn't validate the expected value. Consider asserting the specific behavior (e.g.,
expect(result).to.equal("0")or handling NaN appropriately).♻️ Suggested improvement
it("should handle empty strings", function () { const result = calculatePercentageDifference("", ""); - // Both empty - NaN or 0 depending on implementation - expect(result).to.be.a("string"); + // Both empty - should be 0 difference + expect(parseFloat(result)).to.equal(0); });
480-488: Strengthen assertion for non-container case.The test checks that the result is boolean but doesn't assert the expected
falsevalue. Unless running in a container is expected, this should assertexpect(result).to.be.false.♻️ Suggested improvement
it("should return false when IN_CONTAINER is not set and not in container", async function () { delete process.env.IN_CONTAINER; const result = await inContainer(); - // On a non-container system, this should return false - // (unless running tests in a container) - expect(typeof result).to.equal("boolean"); + // Should return false when not in container + expect(result).to.be.false; });
635-646: Consider expanding test coverage.The
parseTestsfunction currently only has one test for the empty files case. Consider adding tests for:
- Valid test files with actual test content
- Invalid or malformed test files
- Different file types from the fileTypes config
- Error handling scenarios
Based on learnings, comprehensive test coverage should include both successful and error scenarios.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts}: Use async/await for asynchronous operations
Prefer destructuring for function parameters
Use meaningful variable names that reflect Doc Detective terminology
Add JSDoc comments for complex functions
Files:
src/utils.test.js
**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.js: When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Mock external dependencies (file system, HTTP requests) in tests
Test both successful and error scenarios
Validate configuration handling thoroughly in tests
Use realistic test data that matches actual usage patterns
Use Mocha for unit tests
Use Chai for assertions
Files:
src/utils.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use the built-in logging system with available log levels: debug, info, warn, error
Files:
src/utils.test.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Add comprehensive test coverage when adding new features
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Validate configuration handling thoroughly in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use realistic test data that matches actual usage patterns
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Mock external dependencies (file system, HTTP requests) in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Test both successful and error scenarios
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : When possible, directly import and run functions rather than use extensive mocking and stubbing in tests
Applied to files:
src/utils.test.js
📚 Learning: 2025-11-25T23:59:45.298Z
Learnt from: CR
Repo: doc-detective/resolver PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T23:59:45.298Z
Learning: Applies to **/*.test.js : Use Chai for assertions
Applied to files:
src/utils.test.js
🧬 Code graph analysis (1)
src/utils.test.js (1)
src/utils.js (1)
timestamp(1201-1201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest, 22)
🔇 Additional comments (7)
src/utils.test.js (7)
41-117: LGTM!Comprehensive log level testing with appropriate stubbing and edge case coverage for object formatting.
119-134: LGTM!Appropriate validation of timestamp format and content.
237-257: LGTM!Comprehensive coverage of absolute and relative URL patterns.
259-297: LGTM!Good coverage of Heretto integration discovery including edge cases and multiple mappings.
329-410: LGTM!Proper filesystem mocking and good coverage of success/error scenarios for file operations.
429-443: LGTM!Excellent cross-platform approach using a temporary script file to test stderr capture. The cleanup in the finally block ensures resources are freed even on failure.
Based on learnings, this follows the guideline to directly run functions rather than extensive mocking.
586-600: Test is well-structured and properly uses the existing test artifact.The test correctly uses Chai assertions, follows async/await patterns, and validates both success and failure scenarios with realistic test data. The artifact file
test/artifacts/test.spec.jsonexists in the repository as referenced.
| it("should return JSON string when env var contains JSON (JSON parse branch unreachable)", function () { | ||
| // Note: Lines 1177-1182 check JSON.parse(stringOrObject) where stringOrObject is "$VAR" | ||
| // Since "$VAR" is never valid JSON, this branch is effectively unreachable | ||
| // The JSON object is returned as a string | ||
| process.env.OBJECT_VAR = '{"key":"value"}'; | ||
|
|
||
| const result = replaceEnvs("$OBJECT_VAR"); | ||
| expect(result).to.equal('{"key":"value"}'); | ||
|
|
||
| delete process.env.OBJECT_VAR; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find utils.js file
fd -t f "utils.js" | grep -v test | head -5Repository: doc-detective/resolver
Length of output: 78
🏁 Script executed:
# Check if src/utils.js exists and view lines around 1177-1182
if [ -f src/utils.js ]; then
wc -l src/utils.js
echo "=== Lines 1170-1190 ==="
sed -n '1170,1190p' src/utils.js
echo "=== Lines 1160-1200 (broader context) ==="
sed -n '1160,1200p' src/utils.js
else
# Try finding utils.js in other locations
find . -name "utils.js" -type f | grep -v node_modules | grep -v test
fiRepository: doc-detective/resolver
Length of output: 2489
🏁 Script executed:
# Verify the exact lines and broader context of the JSON.parse logic
sed -n '1173,1195p' src/utils.jsRepository: doc-detective/resolver
Length of output: 957
Consider investigating dead code in utils.js.
The test correctly identifies that the JSON.parse(value) branch at line 1182 in utils.js is unreachable. The condition JSON.parse(stringOrObject) will always fail when stringOrObject is a variable reference (e.g., "$OBJECT_VAR"), causing the catch block to silently suppress the error. This appears to be either dead code or a logic bug where the condition should check JSON.parse(value) instead. Consider filing an issue to investigate and potentially remove or fix this code.
🤖 Prompt for AI Agents
In @src/utils.test.js around lines 224 - 234, The JSON.parse branch in utils.js
is unreachable because it calls JSON.parse(stringOrObject) where stringOrObject
is the literal variable reference like "$OBJECT_VAR"; update the logic in the
replaceEnvs function to parse the resolved environment value instead (use
JSON.parse(value) after resolving stringOrObject to value) or remove the dead
branch if parsing value is not intended; ensure any thrown parse errors are
handled explicitly rather than silently swallowed so tests and runtime behavior
reflect real parse failures.
| it("should create temp directory if it does not exist", async function () { | ||
| axiosGetStub.resolves({ | ||
| data: "content", | ||
| }); | ||
|
|
||
| // Clean up temp directory first | ||
| const tempDir = `${os.tmpdir()}/doc-detective`; | ||
| if (fs.existsSync(tempDir)) { | ||
| const files = fs.readdirSync(tempDir); | ||
| for (const file of files) { | ||
| fs.unlinkSync(path.join(tempDir, file)); | ||
| } | ||
| fs.rmdirSync(tempDir); | ||
| } | ||
|
|
||
| const result = await fetchFile("https://example.com/new-file.txt"); | ||
|
|
||
| expect(result.result).to.equal("success"); | ||
| expect(fs.existsSync(tempDir)).to.be.true; | ||
| }); |
There was a problem hiding this comment.
Test isolation concern with manual cleanup.
Manually cleaning up the temp directory before the test could interfere with parallel test execution. Consider using a test-specific temp directory or ensuring tests run serially.
♻️ Suggested improvement
Consider creating a unique temp directory for this test:
it("should create temp directory if it does not exist", async function () {
axiosGetStub.resolves({
data: "content",
});
- // Clean up temp directory first
- const tempDir = `${os.tmpdir()}/doc-detective`;
+ // Use a unique temp directory for this test
+ const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "doc-detective-test-"));
- if (fs.existsSync(tempDir)) {
- const files = fs.readdirSync(tempDir);
- for (const file of files) {
- fs.unlinkSync(path.join(tempDir, file));
- }
- fs.rmdirSync(tempDir);
- }
- const result = await fetchFile("https://example.com/new-file.txt");
+ // Test would need to be adapted to use this temp dir or mock the temp dir path
+ const result = await fetchFile("https://example.com/new-file.txt");
expect(result.result).to.equal("success");
- expect(fs.existsSync(tempDir)).to.be.true;
+ // Clean up test-specific temp dir
+ fs.rmSync(tempDir, { recursive: true, force: true });
});Note: This would require modifying the test approach or mocking the temp directory path.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/utils.test.js around lines 535 - 554, The test "should create temp
directory if it does not exist" manually cleans a shared
`${os.tmpdir()}/doc-detective` which can break parallel tests; change the test
to use a test-specific temp directory or mock the temp path used by fetchFile so
it creates a unique directory per run (e.g., use fs.mkdtemp or build a unique
name with os.tmpdir + random/suffix) and ensure the test cleans up only that
directory afterwards, or stub the helper that returns the temp dir so fetchFile
writes to an isolated path; update assertions to check the test-specific path
instead of the shared `tempDir`.
- Convert auto-dev-release.yml and all YAML files to LF line endings - Add .gitattributes to enforce LF for *.yml and *.yaml files - Run git add --renormalize to apply consistent line endings across repo
… validation - Remove unused 'inputs' parameter from workflowToTest function. - Enhance success criteria validation to guard against missing "==" and invalid status codes. - Add logging for warnings related to invalid status codes.
1 participant
Enhance test coverage across multiple modules, achieving significant improvements in overall coverage metrics. Introduce edge case tests for critical functions to ensure robustness and reliability. Update CI workflows to enforce coverage checks and maintain quality standards. Remove outdated references and improve documentation on testing strategies.
Summary by CodeRabbit
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.