Conversation
- Convert all 9 source files to TypeScript (.ts) - Add tsconfig.json with ES2022 target and strict mode - Add type declarations for doc-detective-common module - Add shared types in src/types.ts - Create ESM wrapper script for dual CJS/ESM support - Update package.json with TypeScript build scripts and exports - Update test imports to use compiled dist/ output - Update CI workflow to run build before test/publish Tests remain in JavaScript and import from the compiled output.
WalkthroughMigrates the codebase from CommonJS JavaScript to TypeScript/ESM, adds a dist build pipeline (compile/build/pre/post scripts + ESM wrapper), replaces many JS modules with typed TS equivalents (types, utils, openapi, resolve, heretto, sanitizers, telemetry), and updates CI/tests to build and consume the dist output. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Caller
participant Config as setConfig
participant Resolver as resolveDetectedTests
participant OpenAPI as openapi.loadDescription
participant HTTP as External API (OpenAPI / Heretto)
participant FS as Filesystem
CLI->>Config: setConfig(config)
CLI->>Resolver: resolveDetectedTests({config, detectedTests})
Resolver->>OpenAPI: loadDescription(descriptionPath) for each openApi source
OpenAPI->>HTTP: fetch OpenAPI document
HTTP-->>OpenAPI: OpenAPI document
OpenAPI-->>Resolver: dereferenced definition
Resolver->>Resolver: expand contexts, isDriverRequired checks
Resolver->>FS: read local files (if needed)
Resolver->>CLI: returned ResolvedTests
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
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 |
The TypeScript compiled output uses strict mode which throws TypeError when trying to set properties on a string. The original JavaScript code silently ignored this case due to non-strict mode. When regex patterns overlap (e.g., 'test ignore start' matches both testStart and ignoreStart patterns), parseObject returns a string instead of an object. Now we explicitly check for this and skip processing non-object values.
|
📝 Documentation updates detected! New suggestion: Add doc-detective-resolver repository documentation |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/heretto.integration.test.js (1)
17-40: Guard dist import to prevent hard failure when build hasn't runThe top-level
require("../dist/heretto")at line 17 throws immediately ifdist/hasn't been built, preventing the skip logic from executing. Thetest:integrationscript has no dependency onbuild, so this can fail even when tests should skip. Move the import into thebeforehook with a guard so tests can skip gracefully when the build output is missing.🔧 Proposed change (guard import + skip if dist missing)
-const heretto = require("../dist/heretto"); +let heretto; const fs = require("fs"); const path = require("path"); const os = require("os"); +const distEntry = path.join(__dirname, "..", "dist", "heretto.js"); +const hasDist = fs.existsSync(distEntry); const isCI = process.env.CI === "true"; const hasCredentials = process.env.HERETTO_ORGANIZATION_ID && process.env.HERETTO_USERNAME && process.env.HERETTO_API_TOKEN; -const shouldRunIntegrationTests = isCI && hasCredentials; +const shouldRunIntegrationTests = isCI && hasCredentials && hasDist; // Log why tests are being skipped if (!shouldRunIntegrationTests) { console.log("\n⏭️ Heretto integration tests skipped:"); if (!isCI) { console.log(" - Not running in CI environment (CI !== 'true')"); } if (!hasCredentials) { console.log(" - Missing required environment variables:"); if (!process.env.HERETTO_ORGANIZATION_ID) console.log(" - HERETTO_ORGANIZATION_ID"); if (!process.env.HERETTO_USERNAME) console.log(" - HERETTO_USERNAME"); if (!process.env.HERETTO_API_TOKEN) console.log(" - HERETTO_API_TOKEN"); } + if (!hasDist) { + console.log(" - Missing build output. Run `npm run build` before integration tests."); + } console.log(""); } describeIntegration("Heretto Integration Tests (CI Only)", function () { + before(function () { + heretto = require(distEntry); + });src/config.ts (1)
744-764: Mutating filter during iteration may skip elements.In
loadDescriptions, filteringconfig.integrations.openApiwhile iterating over it viafor...ofcould lead to unexpected behavior if multiple consecutive definitions fail to load.🐛 Proposed fix - collect failures and filter after iteration
async function loadDescriptions(config: Config): Promise<void> { if (config?.integrations?.openApi) { + const failedConfigs: OpenApiConfigWithDefinition[] = []; for (const openApiConfig of config.integrations .openApi as OpenApiConfigWithDefinition[]) { try { openApiConfig.definition = await loadDescription( openApiConfig.descriptionPath! ); } catch (error) { log( config, "error", `Failed to load OpenAPI description from ${openApiConfig.descriptionPath}: ${(error as Error).message}` ); - // Remove the failed OpenAPI configuration - config.integrations.openApi = config.integrations.openApi!.filter( - (item) => item !== openApiConfig - ); + failedConfigs.push(openApiConfig); } } + // Remove failed configurations after iteration + if (failedConfigs.length > 0) { + config.integrations.openApi = config.integrations.openApi!.filter( + (item) => !failedConfigs.includes(item as OpenApiConfigWithDefinition) + ); + } } }src/index.test.js (1)
27-35: Incorrect proxyquire stub paths causing test failures.The proxyquire paths
../dist/configand../dist/utilsare incorrect. When proxyquire intercepts requires from within the compileddist/index.js, the require paths in that file are./configand./utils(relative to the dist folder), not../dist/config.This is the likely root cause of the pipeline failure where 21 tests are failing with "TypeError: Cannot create property 'testId' on string 'end'" - the stubs aren't being applied, so real modules are being used.
🐛 Proposed fix
detectTests = proxyquire("../dist/index", { - "../dist/config": { setConfig: setConfigStub }, - "../dist/utils": { + "./config": { setConfig: setConfigStub }, + "./utils": { qualifyFiles: qualifyFilesStub, parseTests: parseTestsStub, log: logStub, }, }).detectTests;src/heretto.ts (2)
166-226: Guard against missingscenarioParameters.content.
If the API omitscontent,.find(...)will throw and fall into the generic catch, masking the real issue. Consider a safe default.🐛 Suggested fix
- // Make sure that scenarioParameters.content has an object with name="transtype" and options[0].value="dita" - const transtypeParam = scenarioParameters.content.find( + const content = scenarioParameters.content ?? []; + // Make sure that scenarioParameters.content has an object with name="transtype" and options[0].value="dita" + const transtypeParam = content.find( (param) => param.name === "transtype" ); ... - const toolKitParam = scenarioParameters.content.find( + const toolKitParam = content.find( (param) => param.name === "tool-kit-name" ); ... - const fileUuidPickerParam = scenarioParameters.content.find( + const fileUuidPickerParam = content.find( (param) => param.type === "file_uuid_picker" );
434-505: Ensure ZIP cleanup on failure paths.
If extraction fails after the ZIP is written, the temp ZIP is left behind.🐛 Suggested fix
- const zipPath = path.join(tempDir, `heretto_${hash}.zip`); - fs.writeFileSync(zipPath, Buffer.from(response.data)); - - // Extract ZIP contents with path traversal protection - log(config, "debug", `Extracting output to ${outputDir}...`); - const zip = new AdmZip(zipPath); - const resolvedOutputDir = path.resolve(outputDir); + const zipPath = path.join(tempDir, `heretto_${hash}.zip`); + fs.writeFileSync(zipPath, Buffer.from(response.data)); + + try { + // Extract ZIP contents with path traversal protection + log(config, "debug", `Extracting output to ${outputDir}...`); + const zip = new AdmZip(zipPath); + const resolvedOutputDir = path.resolve(outputDir); - // Validate and extract entries safely to prevent zip slip attacks - for (const entry of zip.getEntries()) { - const entryPath = path.join(outputDir, entry.entryName); - const resolvedPath = path.resolve(entryPath); + // Validate and extract entries safely to prevent zip slip attacks + for (const entry of zip.getEntries()) { + const entryPath = path.join(outputDir, entry.entryName); + const resolvedPath = path.resolve(entryPath); - // Ensure the resolved path is within outputDir - if ( - !resolvedPath.startsWith(resolvedOutputDir + path.sep) && - resolvedPath !== resolvedOutputDir - ) { - log(config, "warning", `Skipping potentially malicious ZIP entry: ${entry.entryName}`); - continue; - } + // Ensure the resolved path is within outputDir + if ( + !resolvedPath.startsWith(resolvedOutputDir + path.sep) && + resolvedPath !== resolvedOutputDir + ) { + log(config, "warning", `Skipping potentially malicious ZIP entry: ${entry.entryName}`); + continue; + } - if (entry.isDirectory) { - fs.mkdirSync(resolvedPath, { recursive: true }); - } else { - fs.mkdirSync(path.dirname(resolvedPath), { recursive: true }); - fs.writeFileSync(resolvedPath, entry.getData()); + if (entry.isDirectory) { + fs.mkdirSync(resolvedPath, { recursive: true }); + } else { + fs.mkdirSync(path.dirname(resolvedPath), { recursive: true }); + fs.writeFileSync(resolvedPath, entry.getData()); + } } - } - - // Clean up ZIP file - fs.unlinkSync(zipPath); + } finally { + if (fs.existsSync(zipPath)) fs.unlinkSync(zipPath); + }
🤖 Fix all issues with AI agents
In `@src/arazzo.ts`:
- Around line 104-116: The status-code extraction for
workflowStep.successCriteria is fragile: update the parsing inside the loop over
successCriteria (where criterion.condition is inspected and
docDetectiveStep.statusCodes is set) to defensively handle malformed conditions
by first checking that criterion.condition exists and contains '==' (or match
with a regex like /^\s*\$statusCode\s*==\s*(\d+)\s*$/), extract the numeric part
safely, attempt Number/parseInt and verify !isNaN before assigning to
docDetectiveStep.statusCodes, and otherwise skip the entry (or log/warn) so
malformed conditions do not throw or produce NaN.
In `@src/config.test.js`:
- Around line 27-31: The proxyquire call that sets setConfig is stubbing the
wrong module paths; update the proxyquire stubs in the test to match how the
compiled module requires them by replacing "../dist/utils" and "../dist/openapi"
with "./utils" and "./openapi" respectively so the proxyquire for setConfig
(assigned from require("../dist/config")) correctly intercepts calls to log,
loadEnvs, replaceEnvs and loadDescription.
In `@src/config.ts`:
- Around line 491-500: resolveConcurrentRunners currently can return NaN if
config.concurrentRunners is a non-numeric string; update
resolveConcurrentRunners to coerce and validate numeric input before returning:
if config.concurrentRunners === true keep the existing boolean path; otherwise
attempt to parse Number(config.concurrentRunners) (or parseInt) and use
Number.isFinite to check validity, returning the validated numeric value when
valid or falling back to 1 when invalid; reference the resolveConcurrentRunners
function and the config.concurrentRunners property when making this change.
In `@src/heretto.ts`:
- Around line 518-675: getResourceDependencies currently builds pathToUuidMap
containing both HerettoResourceInfo objects and metadata string entries
(_ditamapPath/_ditamapId/_ditamapParentFolderId) but is being treated elsewhere
as Record<string, HerettoResourceInfo>, which hides the union and can cause
runtime errors; fix by changing the function's return shape to a clear typed
structure (e.g., an object with two properties: data: Record<string,
HerettoResourceInfo> and metadata: { ditamapPath?: string; ditamapId?: string;
ditamapParentFolderId?: string }) or by updating the shared HerettoConfig type
to accept the mixed map type, then update all call sites that consume
getResourceDependencies (and any HerettoConfig references) to use the new
structured fields instead of assuming every key has .uuid; ensure you modify
references to pathToUuidMap and any unsafe casts so consumers read metadata from
the dedicated metadata field or handle the union type safely.
In `@src/index.ts`:
- Around line 1-4: Import and call the resolve module's setLogFunction with the
existing log function so resolveDetectedTests has a real logger: add an import
for setLogFunction from "./resolve" and invoke setLogFunction(log) during module
initialization (before any calls to resolveDetectedTests) to wire the logger
into resolve.ts.
In `@src/telem.ts`:
- Around line 83-98: The string replacement calls inside the telemetry
flattening loop only replace the first space (calls to replace(" ", "_"));
update all occurrences — the parentKey, key, and key2 usages inside the
Object.entries(results.summary) nested loops (and the final else branch
assigning telemetryData[parentKey...]) — to use a global replacement (e.g.,
.replace(/ /g, "_") or .replaceAll(" ", "_")) so keys with multiple spaces are
fully normalized; ensure you change the three locations where replace(" ", "_")
is used.
- Around line 56-59: The code reads process.env["DOC_DETECTIVE_META"] and calls
JSON.parse directly into telemetryData (type TelemetryData), which can throw if
the env var contains invalid JSON; modify the logic around telemetryData
assignment to catch JSON.parse errors: when DOC_DETECTIVE_META is set, attempt
JSON.parse in a try/catch, log or handle the parse error (e.g., warn via console
or a logger) and fall back to an empty object (or a safe default) instead of
letting the exception propagate; keep the variable name telemetryData and its
TelemetryData type and ensure downstream code still receives a valid object.
In `@src/utils.ts`:
- Around line 215-223: cleanTemp currently assumes every entry under tempDir is
a file and calls fs.unlinkSync, which throws if an entry is a directory; update
the cleanTemp implementation to handle both files and directories by checking
each entry (e.g., fs.statSync or fs.lstatSync on the path) and removing
directories recursively (or use fs.rmSync(curPath, { recursive: true, force:
true })) instead of always calling fs.unlinkSync; ensure you still only target
entries inside tempDir and preserve the tempDir itself.
- Around line 627-639: The code constructs RegExp from potentially
user-controlled patterns (fileType.inlineStatements) using new RegExp in the
loop around patterns and can be vulnerable to ReDoS; validate each pattern
before constructing the RegExp (e.g., run patterns through a safe-regex check or
a whitelist of allowed patterns) and handle failures by skipping/logging the
offending pattern (wrap new RegExp(...) in a try/catch and avoid executing
matchAll on unsafe patterns), or document that fileType.inlineStatements are
trusted configuration only; update the logic around patterns.forEach / new
RegExp / matches handling to skip invalid/unsafe regexes and surface an explicit
error/log when a pattern is rejected so StatementMatch creation and
statements.push only run for validated regexes.
🧹 Nitpick comments (11)
package.json (1)
17-17: Cross-platform compatibility issue withprebuildscript.The
rm -rf distcommand won't work on Windows without additional tooling. Consider using a cross-platform approach.Suggested fix using rimraf or node
Option 1: Add
rimrafas a devDependency:- "prebuild": "rm -rf dist", + "prebuild": "rimraf dist",Option 2: Use Node.js directly (no extra dependency):
- "prebuild": "rm -rf dist", + "prebuild": "node -e \"fs.rmSync('dist', { recursive: true, force: true })\"",src/utils.ts (3)
683-724: Wrap switch case declarations in blocks to prevent scope leakage.Per static analysis, variables declared in switch cases (
testWithV2,testWithDetectSteps) can be accessed from other cases, which can lead to bugs.Wrap case blocks
case "testStart": + { statementContent = statement[1] || statement[0]; test = parseObject({ stringifiedObject: statementContent }) as DetectedTest; // ... rest of case logic ... tests.push(test); break; + } case "testEnd": + { testId = `${crypto.randomUUID()}`; ignore = false; break; + }Apply similar wrapping to
detectedStepandstepcases.
234-258: HTTP request infetchFilelacks timeout configuration.The axios request has no timeout, which could cause the process to hang indefinitely on network issues.
Add timeout to axios request
export async function fetchFile(fileURL: string): Promise<FetchFileResult> { try { - const response = await axios.get(fileURL); + const response = await axios.get(fileURL, { timeout: 30000 }); let data: string;
1158-1212:spawnCommandreturns redundantcodeandexitCodefields.The return type includes both
codeandexitCodewith the same value. Consider keeping only one for clarity.Simplify return type
- return { stdout, stderr, code: exitCode, exitCode }; + return { stdout, stderr, code: exitCode };And update
SpawnResulttype accordingly, or keepexitCodeand removecodedepending on which name is preferred throughout the codebase.src/openapi.ts (1)
129-135: Consider returning null instead of throwing when no responses are defined.The
getSchemasfunction throws an error when no responses are defined, butgetOperation(which calls it) is expected to returnnullwhen operations aren't found. This inconsistency could cause unexpected crashes for operations without responses.Return empty schemas instead of throwing
if (!responseCode) { if (definition.responses && Object.keys(definition.responses).length > 0) { responseCode = Object.keys(definition.responses)[0]; } else { - throw new Error("No responses defined for the operation."); + return schemas; // Return partial schemas without response } }src/resolve.ts (4)
18-30: Potential runtime error ifsetLogFunctionis never called.The
logvariable is declared but not initialized. If any resolution function is called beforesetLogFunction, and the conditionalif (log)checks pass due to a truthy value somehow being assigned, it could cause issues. More importantly, the module expects consumers to callsetLogFunctionbut there's no enforcement or documentation in the public exports.Consider providing a default no-op function or logging a warning:
♻️ Proposed fix
// Forward declaration for log function - will be set during initialization -let log: LogFunction; +let log: LogFunction = () => {}; // No-op default to prevent runtime errors
51-60: Unused functionisDriverRequired.The
isDriverRequiredfunction is defined but never called. The same logic is duplicated inline inresolveContexts(lines 89-95).♻️ Proposed fix - use the existing function
// Check if current test requires a browser - let browserRequired = false; - test.steps.forEach((step) => { - // Check if test includes actions that require a driver. - driverActions.forEach((action) => { - if (typeof step[action] !== "undefined") browserRequired = true; - }); - }); + const browserRequired = isDriverRequired({ test });
145-156: JSON.stringify comparison for browser deduplication may have ordering issues.Using
JSON.stringifyfor object comparison can produce different results if object property order differs. Consider a more robust comparison or use a dedicated deep-equal utility.
339-339: DeletingstepsfromresolvedTestmay cause TypeScript issues.The delete operation on line 339 modifies the type at runtime. While this works, it's a type-unsafe pattern. Consider excluding
stepsduring object construction instead.src/heretto.ts (2)
92-130: Prefer destructuringherettoConfigfor readability and consistency.♻️ Suggested refactor
-export function createApiClient(herettoConfig: HerettoConfig): AxiosInstance { - const authHeader = createAuthHeader( - herettoConfig.username, - herettoConfig.apiToken - ); +export function createApiClient({ username, apiToken, organizationId }: HerettoConfig): AxiosInstance { + const authHeader = createAuthHeader(username, apiToken); return axios.create({ - baseURL: getBaseUrl(herettoConfig.organizationId), + baseURL: getBaseUrl(organizationId), timeout: API_REQUEST_TIMEOUT_MS, headers: { Authorization: `Basic ${authHeader}`, "Content-Type": "application/json", }, }); } -export function createRestApiClient(herettoConfig: HerettoConfig): AxiosInstance { - const authHeader = createAuthHeader( - herettoConfig.username, - herettoConfig.apiToken - ); +export function createRestApiClient({ username, apiToken, organizationId }: HerettoConfig): AxiosInstance { + const authHeader = createAuthHeader(username, apiToken); return axios.create({ - baseURL: `https://${herettoConfig.organizationId}.heretto.com`, + baseURL: `https://${organizationId}.heretto.com`, timeout: API_REQUEST_TIMEOUT_MS, headers: { Authorization: `Basic ${authHeader}`, Accept: "application/xml, text/xml, */*", }, }); }As per coding guidelines, prefer destructuring for function parameters.
685-759: Prefer destructuring for the frequently-usedherettoConfigfields.♻️ Suggested refactor
-export async function loadHerettoContent( - herettoConfig: HerettoConfig, - log: LogFunction, - config: Config -): Promise<string | null> { - log(config, "info", `Loading content from Heretto "${herettoConfig.name}"...`); +export async function loadHerettoContent( + herettoConfig: HerettoConfig, + log: LogFunction, + config: Config +): Promise<string | null> { + const { name, scenarioName, uploadOnChange } = herettoConfig; + log(config, "info", `Loading content from Heretto "${name}"...`); ... - const scenarioName = herettoConfig.scenarioName || DEFAULT_SCENARIO_NAME; - const scenario = await findScenario(client, log, config, scenarioName); + const resolvedScenarioName = scenarioName || DEFAULT_SCENARIO_NAME; + const scenario = await findScenario(client, log, config, resolvedScenarioName); ... - if (herettoConfig.uploadOnChange) { + if (uploadOnChange) { log(config, "debug", `Fetching resource dependencies for ditamap ${scenario.fileId}...`); ... - if (outputPath && herettoConfig.uploadOnChange) { + if (outputPath && uploadOnChange) { const fileMapping = await buildFileMapping(outputPath, herettoConfig, log, config); herettoConfig.fileMapping = fileMapping; }As per coding guidelines, prefer destructuring for function parameters.
| if (workflowStep.successCriteria) { | ||
| docDetectiveStep.responseData = {}; | ||
| docDetectiveStep.responseData = {} as Record<string, unknown>; | ||
| workflowStep.successCriteria.forEach((criterion) => { | ||
| if (criterion.condition.startsWith("$statusCode")) { | ||
| docDetectiveStep.statusCodes = [ | ||
| parseInt(criterion.condition.split("==")[1].trim()), | ||
| ]; | ||
| } else if (criterion.context === "$response.body") { | ||
| // This is a simplification; actual JSONPath translation would be more complex | ||
| docDetectiveStep.responseData[criterion.condition] = true; | ||
| (docDetectiveStep.responseData as Record<string, unknown>)[criterion.condition] = true; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fragile status code parsing could fail on malformed conditions.
The parsing logic criterion.condition.split("==")[1].trim() assumes a specific format. If the condition doesn't contain == or has unexpected whitespace, this could throw or produce NaN.
Suggested defensive fix
if (criterion.condition.startsWith("$statusCode")) {
- docDetectiveStep.statusCodes = [
- parseInt(criterion.condition.split("==")[1].trim()),
- ];
+ const parts = criterion.condition.split("==");
+ if (parts.length >= 2) {
+ const statusCode = parseInt(parts[1].trim(), 10);
+ if (!isNaN(statusCode)) {
+ docDetectiveStep.statusCodes = [statusCode];
+ }
+ }
} else if (criterion.context === "$response.body") {📝 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.
| if (workflowStep.successCriteria) { | |
| docDetectiveStep.responseData = {}; | |
| docDetectiveStep.responseData = {} as Record<string, unknown>; | |
| workflowStep.successCriteria.forEach((criterion) => { | |
| if (criterion.condition.startsWith("$statusCode")) { | |
| docDetectiveStep.statusCodes = [ | |
| parseInt(criterion.condition.split("==")[1].trim()), | |
| ]; | |
| } else if (criterion.context === "$response.body") { | |
| // This is a simplification; actual JSONPath translation would be more complex | |
| docDetectiveStep.responseData[criterion.condition] = true; | |
| (docDetectiveStep.responseData as Record<string, unknown>)[criterion.condition] = true; | |
| } | |
| }); | |
| } | |
| if (workflowStep.successCriteria) { | |
| docDetectiveStep.responseData = {} as Record<string, unknown>; | |
| workflowStep.successCriteria.forEach((criterion) => { | |
| if (criterion.condition.startsWith("$statusCode")) { | |
| const parts = criterion.condition.split("=="); | |
| if (parts.length >= 2) { | |
| const statusCode = parseInt(parts[1].trim(), 10); | |
| if (!isNaN(statusCode)) { | |
| docDetectiveStep.statusCodes = [statusCode]; | |
| } | |
| } | |
| } else if (criterion.context === "$response.body") { | |
| // This is a simplification; actual JSONPath translation would be more complex | |
| (docDetectiveStep.responseData as Record<string, unknown>)[criterion.condition] = true; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/arazzo.ts` around lines 104 - 116, The status-code extraction for
workflowStep.successCriteria is fragile: update the parsing inside the loop over
successCriteria (where criterion.condition is inspected and
docDetectiveStep.statusCodes is set) to defensively handle malformed conditions
by first checking that criterion.condition exists and contains '==' (or match
with a regex like /^\s*\$statusCode\s*==\s*(\d+)\s*$/), extract the numeric part
safely, attempt Number/parseInt and verify !isNaN before assigning to
docDetectiveStep.statusCodes, and otherwise skip the entry (or log/warn) so
malformed conditions do not throw or produce NaN.
| setConfig = proxyquire("../dist/config", { | ||
| "doc-detective-common": { validate: validStub }, | ||
| "./utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub }, | ||
| "./openapi": { loadDescription: sinon.stub().resolves({}) } | ||
| "../dist/utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub }, | ||
| "../dist/openapi": { loadDescription: sinon.stub().resolves({}) } | ||
| }).setConfig; |
There was a problem hiding this comment.
Incorrect proxyquire stub paths - same issue as index.test.js.
The proxyquire paths should match how the compiled dist/config.js requires its dependencies. The compiled file uses ./utils and ./openapi, not ../dist/utils and ../dist/openapi.
🐛 Proposed fix
setConfig = proxyquire("../dist/config", {
"doc-detective-common": { validate: validStub },
- "../dist/utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub },
- "../dist/openapi": { loadDescription: sinon.stub().resolves({}) }
+ "./utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub },
+ "./openapi": { loadDescription: sinon.stub().resolves({}) }
}).setConfig;📝 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.
| setConfig = proxyquire("../dist/config", { | |
| "doc-detective-common": { validate: validStub }, | |
| "./utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub }, | |
| "./openapi": { loadDescription: sinon.stub().resolves({}) } | |
| "../dist/utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub }, | |
| "../dist/openapi": { loadDescription: sinon.stub().resolves({}) } | |
| }).setConfig; | |
| setConfig = proxyquire("../dist/config", { | |
| "doc-detective-common": { validate: validStub }, | |
| "./utils": { log: logStub, loadEnvs: loadEnvsStub, replaceEnvs: replaceEnvsStub }, | |
| "./openapi": { loadDescription: sinon.stub().resolves({}) } | |
| }).setConfig; |
🤖 Prompt for AI Agents
In `@src/config.test.js` around lines 27 - 31, The proxyquire call that sets
setConfig is stubbing the wrong module paths; update the proxyquire stubs in the
test to match how the compiled module requires them by replacing "../dist/utils"
and "../dist/openapi" with "./utils" and "./openapi" respectively so the
proxyquire for setConfig (assigned from require("../dist/config")) correctly
intercepts calls to log, loadEnvs, replaceEnvs and loadDescription.
| export function resolveConcurrentRunners( | ||
| config: Config | ||
| ): number { | ||
| if (config.concurrentRunners === true) { | ||
| // Cap at 4 only for the boolean convenience option | ||
| return Math.min(os.cpus().length, 4); | ||
| } | ||
| // Respect explicit numeric values and default | ||
| return config.concurrentRunners || 1; | ||
| return (config.concurrentRunners as number) || 1; | ||
| } |
There was a problem hiding this comment.
Edge case: resolveConcurrentRunners returns NaN for non-numeric strings.
If config.concurrentRunners is set to a non-numeric string (e.g., from environment variable parsing), the function will return NaN via the || 1 fallback not catching it.
🐛 Proposed fix
export function resolveConcurrentRunners(
config: Config
): number {
if (config.concurrentRunners === true) {
// Cap at 4 only for the boolean convenience option
return Math.min(os.cpus().length, 4);
}
// Respect explicit numeric values and default
- return (config.concurrentRunners as number) || 1;
+ const value = Number(config.concurrentRunners);
+ return Number.isFinite(value) && value > 0 ? value : 1;
}🤖 Prompt for AI Agents
In `@src/config.ts` around lines 491 - 500, resolveConcurrentRunners currently can
return NaN if config.concurrentRunners is a non-numeric string; update
resolveConcurrentRunners to coerce and validate numeric input before returning:
if config.concurrentRunners === true keep the existing boolean path; otherwise
attempt to parse Number(config.concurrentRunners) (or parseInt) and use
Number.isFinite to check validity, returning the validated numeric value when
valid or falling back to 1 when invalid; reference the resolveConcurrentRunners
function and the config.concurrentRunners property when making this change.
| export async function getResourceDependencies( | ||
| restClient: AxiosInstance, | ||
| ditamapId: string, | ||
| log: LogFunction, | ||
| config: Config | ||
| ): Promise<Record<string, HerettoResourceInfo | string>> { | ||
| const pathToUuidMap: Record<string, HerettoResourceInfo | string> = {}; | ||
|
|
||
| const xmlParser = new XMLParser({ | ||
| ignoreAttributes: false, | ||
| attributeNamePrefix: "@_", | ||
| }); | ||
|
|
||
| // First, try to get the ditamap's own info (this is more reliable than the dependencies endpoint) | ||
| try { | ||
| log(config, "debug", `Fetching ditamap info for: ${ditamapId}`); | ||
| const ditamapInfo = await restClient.get(`${REST_API_PATH}/${ditamapId}`); | ||
| const ditamapParsed = xmlParser.parse(ditamapInfo.data); | ||
|
|
||
| const ditamapUri = ditamapParsed.resource?.["xmldb-uri"] || ditamapParsed["@_uri"]; | ||
| const ditamapName = ditamapParsed.resource?.name || ditamapParsed["@_name"]; | ||
| const ditamapParentFolder = ditamapParsed.resource?.["folder-uuid"] || | ||
| ditamapParsed.resource?.["@_folder-uuid"] || | ||
| ditamapParsed["@_folder-uuid"]; | ||
|
|
||
| log(config, "debug", `Ditamap info: uri=${ditamapUri}, name=${ditamapName}, parentFolder=${ditamapParentFolder}`); | ||
|
|
||
|
|
||
| const resource = ditamapParsed.resource as Record<string, unknown> | undefined; | ||
| const ditamapUri = resource?.["xmldb-uri"] || ditamapParsed["@_uri"]; | ||
| const ditamapName = resource?.name || ditamapParsed["@_name"]; | ||
| const ditamapParentFolder = | ||
| resource?.["folder-uuid"] || | ||
| resource?.["@_folder-uuid"] || | ||
| ditamapParsed["@_folder-uuid"]; | ||
|
|
||
| log( | ||
| config, | ||
| "debug", | ||
| `Ditamap info: uri=${ditamapUri}, name=${ditamapName}, parentFolder=${ditamapParentFolder}` | ||
| ); | ||
|
|
||
| if (ditamapUri) { | ||
| let relativePath = ditamapUri; | ||
| let relativePath = ditamapUri as string; | ||
| const orgPathMatch = relativePath?.match(/\/db\/organizations\/[^/]+\/(.+)/); | ||
| if (orgPathMatch) { | ||
| relativePath = orgPathMatch[1]; | ||
| } | ||
|
|
||
| pathToUuidMap[relativePath] = { | ||
| uuid: ditamapId, | ||
| fullPath: ditamapUri, | ||
| name: ditamapName, | ||
| parentFolderId: ditamapParentFolder, | ||
| fullPath: ditamapUri as string, | ||
| name: ditamapName as string, | ||
| parentFolderId: ditamapParentFolder as string, | ||
| isDitamap: true, | ||
| }; | ||
|
|
||
| // Store the ditamap info as reference points for creating new files | ||
| pathToUuidMap._ditamapPath = relativePath; | ||
| pathToUuidMap._ditamapId = ditamapId; | ||
| pathToUuidMap._ditamapParentFolderId = ditamapParentFolder; | ||
| pathToUuidMap._ditamapParentFolderId = ditamapParentFolder as string; | ||
|
|
||
| log(config, "debug", `Ditamap path: ${relativePath}, parent folder: ${ditamapParentFolder}`); | ||
| } | ||
| } catch (ditamapError) { | ||
| log(config, "warning", `Could not get ditamap info: ${ditamapError.message}`); | ||
| log(config, "warning", `Could not get ditamap info: ${(ditamapError as Error).message}`); | ||
| } | ||
|
|
||
| // Then try to get the full dependencies list (this endpoint may not be available) | ||
| try { | ||
| log(config, "debug", `Fetching resource dependencies for ditamap: ${ditamapId}`); | ||
|
|
||
| const response = await restClient.get(`${REST_API_PATH}/${ditamapId}/dependencies`); | ||
| const xmlData = response.data; | ||
|
|
||
| const parsed = xmlParser.parse(xmlData); | ||
|
|
||
| // Extract dependencies from the response | ||
| // Response format: <dependencies><dependency id="uuid" uri="path">...</dependency>...</dependencies> | ||
| const extractDependencies = (obj, parentPath = "") => { | ||
| interface DependencyNode { | ||
| "@_id"?: string; | ||
| "@_uuid"?: string; | ||
| "@_uri"?: string; | ||
| "@_path"?: string; | ||
| "@_name"?: string; | ||
| "@_folder-uuid"?: string; | ||
| "@_parent"?: string; | ||
| id?: string; | ||
| uuid?: string; | ||
| uri?: string; | ||
| path?: string; | ||
| name?: string; | ||
| "xmldb-uri"?: string; | ||
| "folder-uuid"?: string; | ||
| dependencies?: { dependency?: DependencyNode | DependencyNode[] }; | ||
| dependency?: DependencyNode | DependencyNode[]; | ||
| } | ||
|
|
||
| const extractDependencies = (obj: DependencyNode | Record<string, unknown>): void => { | ||
| if (!obj) return; | ||
|
|
||
|
|
||
| const node = obj as DependencyNode; | ||
| // Handle single dependency or array of dependencies | ||
| let dependencies = obj.dependencies?.dependency || obj.dependency; | ||
| let dependencies: DependencyNode | DependencyNode[] | undefined = | ||
| node.dependencies?.dependency || node.dependency; | ||
| if (!dependencies) { | ||
| // Try to extract from root-level response | ||
| if (obj["@_id"] && obj["@_uri"]) { | ||
| dependencies = [obj]; | ||
| if (node["@_id"] && node["@_uri"]) { | ||
| dependencies = [node]; | ||
| } else if (Array.isArray(obj)) { | ||
| dependencies = obj; | ||
| dependencies = obj as DependencyNode[]; | ||
| } | ||
| } | ||
|
|
||
| if (!dependencies) return; | ||
| if (!Array.isArray(dependencies)) { | ||
| dependencies = [dependencies]; | ||
| } | ||
|
|
||
| for (const dep of dependencies) { | ||
| const uuid = dep["@_id"] || dep["@_uuid"] || dep.id || dep.uuid; | ||
| const uri = dep["@_uri"] || dep["@_path"] || dep.uri || dep.path || dep["xmldb-uri"]; | ||
| const name = dep["@_name"] || dep.name; | ||
| const parentFolderId = dep["@_folder-uuid"] || dep["@_parent"] || dep["folder-uuid"]; | ||
|
|
||
| if (uuid && (uri || name)) { | ||
| // Extract the relative path from the full URI | ||
| // URI format: /db/organizations/{org}/{path} | ||
| let relativePath = uri || name; | ||
| let relativePath = (uri || name) as string; | ||
| const orgPathMatch = relativePath?.match(/\/db\/organizations\/[^/]+\/(.+)/); | ||
| if (orgPathMatch) { | ||
| relativePath = orgPathMatch[1]; | ||
| } | ||
|
|
||
| pathToUuidMap[relativePath] = { | ||
| uuid, | ||
| fullPath: uri, | ||
| name: name || path.basename(relativePath || ""), | ||
| parentFolderId, | ||
| uuid: uuid as string, | ||
| fullPath: uri as string, | ||
| name: (name || path.basename(relativePath || "")) as string, | ||
| parentFolderId: parentFolderId as string, | ||
| }; | ||
|
|
||
| log(config, "debug", `Mapped: ${relativePath} -> ${uuid}`); | ||
| } | ||
|
|
||
| // Recursively process nested dependencies | ||
| if (dep.dependencies || dep.dependency) { | ||
| extractDependencies(dep); | ||
| } | ||
| } | ||
| }; | ||
| extractDependencies(parsed); | ||
|
|
||
| extractDependencies(parsed as Record<string, unknown>); | ||
|
|
||
| log(config, "info", `Retrieved ${Object.keys(pathToUuidMap).length} resource dependencies from Heretto`); | ||
|
|
||
| } catch (error) { | ||
| // Log more details about the error for debugging | ||
| const statusCode = error.response?.status; | ||
| log(config, "debug", `Dependencies endpoint not available (${statusCode}), will use ditamap info as fallback`); | ||
| const axiosError = error as { response?: { status?: number } }; | ||
| const statusCode = axiosError.response?.status; | ||
| log( | ||
| config, | ||
| "debug", | ||
| `Dependencies endpoint not available (${statusCode}), will use ditamap info as fallback` | ||
| ); | ||
| // Continue with ditamap info only - the fallback will create files in the ditamap's parent folder | ||
| } | ||
|
|
||
| return pathToUuidMap; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of resourceDependencies and special _ditamap* keys
rg -n -C3 '_ditamap(Path|Id|ParentFolderId)' -g '*.ts'Repository: doc-detective/resolver
Length of output: 597
🏁 Script executed:
#!/bin/bash
# Find where getResourceDependencies is called
rg -n 'getResourceDependencies' -g '*.ts'Repository: doc-detective/resolver
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Find how the return value is used - look for usages of the result
rg -n -B2 -A5 'getResourceDependencies' -g '*.ts' | head -100Repository: doc-detective/resolver
Length of output: 927
🏁 Script executed:
#!/bin/bash
# Check what happens with resourceDependencies after line 717
sed -n '717,800p' src/heretto.tsRepository: doc-detective/resolver
Length of output: 3003
🏁 Script executed:
#!/bin/bash
# Find uses of herettoConfig.resourceDependencies
rg -n 'resourceDependencies' -g '*.ts'Repository: doc-detective/resolver
Length of output: 352
🏁 Script executed:
#!/bin/bash
# Find HerettoConfig type definition
rg -n 'type HerettoConfig|interface HerettoConfig' -g '*.ts' -A20Repository: doc-detective/resolver
Length of output: 892
🏁 Script executed:
#!/bin/bash
# Check if herettoConfig.resourceDependencies is accessed/iterated anywhere
rg -n 'herettoConfig\.resourceDependencies|\.resourceDependencies' -g '*.ts' -B2 -A3Repository: doc-detective/resolver
Length of output: 344
Unsafe cast masks mixed return type in getResourceDependencies.
The function returns Promise<Record<string, HerettoResourceInfo | string>> because it includes metadata keys (_ditamapPath, _ditamapId, _ditamapParentFolderId) as strings alongside HerettoResourceInfo objects. However, at line 718, this is cast to Record<string, HerettoResourceInfo>, hiding the union type from consumers. If code later accesses this map and attempts to read .uuid from metadata keys, it will fail. Either return a structured object with separate metadata and data fields, or update the HerettoConfig type to reflect the mixed shape and remove the unsafe cast.
🤖 Prompt for AI Agents
In `@src/heretto.ts` around lines 518 - 675, getResourceDependencies currently
builds pathToUuidMap containing both HerettoResourceInfo objects and metadata
string entries (_ditamapPath/_ditamapId/_ditamapParentFolderId) but is being
treated elsewhere as Record<string, HerettoResourceInfo>, which hides the union
and can cause runtime errors; fix by changing the function's return shape to a
clear typed structure (e.g., an object with two properties: data: Record<string,
HerettoResourceInfo> and metadata: { ditamapPath?: string; ditamapId?: string;
ditamapParentFolderId?: string }) or by updating the shared HerettoConfig type
to accept the mixed map type, then update all call sites that consume
getResourceDependencies (and any HerettoConfig references) to use the new
structured fields instead of assuming every key has .uuid; ensure you modify
references to pathToUuidMap and any unsafe casts so consumers read metadata from
the dedicated metadata field or handle the union type safely.
| import { setConfig } from "./config"; | ||
| import { qualifyFiles, parseTests, log } from "./utils"; | ||
| import { resolveDetectedTests } from "./resolve"; | ||
| import type { Config, DetectedSpec, ResolvedTests } from "./types"; |
There was a problem hiding this comment.
Missing setLogFunction call for resolve module.
The resolve.ts module exports setLogFunction to receive the log function, but this is never called during module initialization. This means logging in resolveDetectedTests will silently fail (or use the no-op if the suggested fix is applied to resolve.ts).
🐛 Proposed fix
import { setConfig } from "./config";
import { qualifyFiles, parseTests, log } from "./utils";
-import { resolveDetectedTests } from "./resolve";
+import { resolveDetectedTests, setLogFunction } from "./resolve";
import type { Config, DetectedSpec, ResolvedTests } from "./types";
+// Initialize the log function for the resolve module
+setLogFunction(log);
+
// Re-export types for consumers🤖 Prompt for AI Agents
In `@src/index.ts` around lines 1 - 4, Import and call the resolve module's
setLogFunction with the existing log function so resolveDetectedTests has a real
logger: add an import for setLogFunction from "./resolve" and invoke
setLogFunction(log) during module initialization (before any calls to
resolveDetectedTests) to wire the logger into resolve.ts.
| const telemetryData: TelemetryData = | ||
| process.env["DOC_DETECTIVE_META"] !== undefined | ||
| ? JSON.parse(process.env["DOC_DETECTIVE_META"]) | ||
| : {}; |
There was a problem hiding this comment.
Missing error handling for JSON.parse of environment variable.
If DOC_DETECTIVE_META contains invalid JSON, JSON.parse will throw an unhandled exception.
🐛 Proposed fix
// Assemble telemetry data
- const telemetryData: TelemetryData =
- process.env["DOC_DETECTIVE_META"] !== undefined
- ? JSON.parse(process.env["DOC_DETECTIVE_META"])
- : {};
+ let telemetryData: TelemetryData = {};
+ if (process.env["DOC_DETECTIVE_META"] !== undefined) {
+ try {
+ telemetryData = JSON.parse(process.env["DOC_DETECTIVE_META"]);
+ } catch {
+ // Invalid JSON in DOC_DETECTIVE_META, use empty object
+ }
+ }🤖 Prompt for AI Agents
In `@src/telem.ts` around lines 56 - 59, The code reads
process.env["DOC_DETECTIVE_META"] and calls JSON.parse directly into
telemetryData (type TelemetryData), which can throw if the env var contains
invalid JSON; modify the logic around telemetryData assignment to catch
JSON.parse errors: when DOC_DETECTIVE_META is set, attempt JSON.parse in a
try/catch, log or handle the parse error (e.g., warn via console or a logger)
and fall back to an empty object (or a safe default) instead of letting the
exception propagate; keep the variable name telemetryData and its TelemetryData
type and ensure downstream code still receives a valid object.
| Object.entries(results.summary).forEach(([parentKey, value]) => { | ||
| if (typeof value === "object") { | ||
| Object.entries(value).forEach(([key, value]) => { | ||
| if (typeof value === "object") { | ||
| Object.entries(value).forEach(([key2, value2]) => { | ||
| telemetryData[`${parentKey.replace(" ","_")}_${key.replace(" ","_")}_${key2.replace(" ","_")}`] = value2; | ||
| if (typeof value === "object" && value !== null) { | ||
| Object.entries(value as Record<string, unknown>).forEach(([key, val]) => { | ||
| if (typeof val === "object" && val !== null) { | ||
| Object.entries(val as Record<string, unknown>).forEach(([key2, value2]) => { | ||
| telemetryData[`${parentKey.replace(" ", "_")}_${key.replace(" ", "_")}_${key2.replace(" ", "_")}`] = value2; | ||
| }); | ||
| } else { | ||
| telemetryData[`${parentKey.replace(" ","_")}_${key.replace(" ","_")}`] = value; | ||
| telemetryData[`${parentKey.replace(" ", "_")}_${key.replace(" ", "_")}`] = val; | ||
| } | ||
| }); | ||
| } else { | ||
| telemetryData[parentKey.replace(" ","_")] = value; | ||
| telemetryData[parentKey.replace(" ", "_")] = value; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
String.replace only replaces the first occurrence.
The replace(" ", "_") calls only replace the first space. Use replaceAll or a regex with global flag for keys with multiple spaces.
🐛 Proposed fix
- telemetryData[`${parentKey.replace(" ", "_")}_${key.replace(" ", "_")}_${key2.replace(" ", "_")}`] = value2;
+ telemetryData[`${parentKey.replaceAll(" ", "_")}_${key.replaceAll(" ", "_")}_${key2.replaceAll(" ", "_")}`] = value2;Apply similar changes to lines 91 and 95.
🤖 Prompt for AI Agents
In `@src/telem.ts` around lines 83 - 98, The string replacement calls inside the
telemetry flattening loop only replace the first space (calls to replace(" ",
"_")); update all occurrences — the parentKey, key, and key2 usages inside the
Object.entries(results.summary) nested loops (and the final else branch
assigning telemetryData[parentKey...]) — to use a global replacement (e.g.,
.replace(/ /g, "_") or .replaceAll(" ", "_")) so keys with multiple spaces are
fully normalized; ensure you change the three locations where replace(" ", "_")
is used.
| export function cleanTemp(): void { | ||
| const tempDir = `${os.tmpdir()}/doc-detective`; | ||
| if (fs.existsSync(tempDir)) { | ||
| fs.readdirSync(tempDir).forEach((file) => { | ||
| const curPath = `${tempDir}/${file}`; | ||
| fs.unlinkSync(curPath); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanTemp will fail if temp directory contains subdirectories.
fs.unlinkSync() cannot remove directories. If the temp directory contains subdirectories, this will throw an error.
Suggested fix using recursive removal
export function cleanTemp(): void {
const tempDir = `${os.tmpdir()}/doc-detective`;
if (fs.existsSync(tempDir)) {
- fs.readdirSync(tempDir).forEach((file) => {
- const curPath = `${tempDir}/${file}`;
- fs.unlinkSync(curPath);
- });
+ fs.rmSync(tempDir, { recursive: true, force: true });
+ fs.mkdirSync(tempDir, { recursive: true });
}
}📝 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.
| export function cleanTemp(): void { | |
| const tempDir = `${os.tmpdir()}/doc-detective`; | |
| if (fs.existsSync(tempDir)) { | |
| fs.readdirSync(tempDir).forEach((file) => { | |
| const curPath = `${tempDir}/${file}`; | |
| fs.unlinkSync(curPath); | |
| }); | |
| } | |
| } | |
| export function cleanTemp(): void { | |
| const tempDir = `${os.tmpdir()}/doc-detective`; | |
| if (fs.existsSync(tempDir)) { | |
| fs.rmSync(tempDir, { recursive: true, force: true }); | |
| fs.mkdirSync(tempDir, { recursive: true }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 215 - 223, cleanTemp currently assumes every entry
under tempDir is a file and calls fs.unlinkSync, which throws if an entry is a
directory; update the cleanTemp implementation to handle both files and
directories by checking each entry (e.g., fs.statSync or fs.lstatSync on the
path) and removing directories recursively (or use fs.rmSync(curPath, {
recursive: true, force: true })) instead of always calling fs.unlinkSync; ensure
you still only target entries inside tempDir and preserve the tempDir itself.
| patterns.forEach((statementRegex) => { | ||
| const regex = new RegExp(statementRegex, "g"); | ||
| const matches = [...content.matchAll(regex)]; | ||
| matches.forEach((match) => { | ||
| const statementMatch: StatementMatch = { | ||
| type: statementType, | ||
| sortIndex: match[1] ? match.index! + match[1].length : match.index!, | ||
| ...match, | ||
| }; | ||
| statements.push(statementMatch); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential ReDoS vulnerability from user-controlled regex patterns.
The regex patterns in fileType.inlineStatements come from configuration and are used directly in new RegExp(). Malicious or poorly written patterns could cause catastrophic backtracking.
Consider validating regex patterns or using a timeout mechanism. If patterns are only from trusted config files, document this assumption. Otherwise, consider using a library like safe-regex to validate patterns before use.
#!/bin/bash
# Check if there are any built-in/default fileType patterns that could indicate pattern source
rg -n "inlineStatements" --type=ts --type=js -C3🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 627-627: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(statementRegex, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 627 - 639, The code constructs RegExp from
potentially user-controlled patterns (fileType.inlineStatements) using new
RegExp in the loop around patterns and can be vulnerable to ReDoS; validate each
pattern before constructing the RegExp (e.g., run patterns through a safe-regex
check or a whitelist of allowed patterns) and handle failures by
skipping/logging the offending pattern (wrap new RegExp(...) in a try/catch and
avoid executing matchAll on unsafe patterns), or document that
fileType.inlineStatements are trusted configuration only; update the logic
around patterns.forEach / new RegExp / matches handling to skip invalid/unsafe
regexes and surface an explicit error/log when a pattern is rejected so
StatementMatch creation and statements.push only run for validated regexes.
There was a problem hiding this comment.
Pull request overview
Migrates doc-detective-resolver from JavaScript to TypeScript, updates packaging to publish compiled dist/ output with dual CJS/ESM support, and adjusts CI/tests to build before executing tests/publishing.
Changes:
- Added TypeScript build infrastructure (
tsconfig.json, sharedsrc/types.ts,dist/output with declarations). - Converted core modules from
.jsto.tsand rewired cross-module interactions. - Updated package exports/scripts and CI workflow to build before running tests/publishing; tests now import from
dist/.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds TS compiler config to emit dist/ + declaration files. |
| src/utils.ts | TypeScript port of utilities (file qualification, parsing, logging, spawning, etc.). |
| src/utils.js | Removed JS implementation (replaced by TS). |
| src/types.ts | Introduces shared public/internal TypeScript types/interfaces. |
| src/telem.ts | TypeScript port of telemetry module. |
| src/sanitize.ts | TypeScript port of URI/path sanitization helpers. |
| src/sanitize.js | Removed JS implementation (replaced by TS). |
| src/resolve.ts | TypeScript port of test resolution pipeline. |
| src/resolve.js | Removed JS implementation (replaced by TS). |
| src/openapi.ts | TypeScript port of OpenAPI loading/example compilation utilities. |
| src/openapi.js | Removed JS implementation (replaced by TS). |
| src/index.ts | Updates main entrypoint, exports, and re-exports for TS build output. |
| src/index.test.js | Updates tests to import from compiled dist/. |
| src/heretto.ts | TypeScript port of Heretto integration logic and APIs. |
| src/heretto.test.js | Updates tests to import from compiled dist/. |
| src/heretto.integration.test.js | Updates integration test to import from compiled dist/. |
| src/doc-detective-common.d.ts | Adds local type declarations for doc-detective-common. |
| src/config.ts | TypeScript port of config processing and defaults. |
| src/config.test.js | Updates tests to import from compiled dist/. |
| src/arazzo.ts | TypeScript port of Arazzo workflow translation. |
| scripts/createEsmWrapper.js | Adds post-compile script to generate an ESM wrapper entrypoint. |
| package.json | Updates main/types/exports + build scripts for TS compilation and dual module support. |
| package-lock.json | Locks newly added dev dependencies (typescript, @types/*). |
| .github/workflows/npm-test.yaml | Ensures CI builds before running tests/publishing. |
Comments suppressed due to low confidence (3)
src/telem.ts:33
telemetryNoticenow requires alogargument. This is a breaking API change compared to the previoustelemetryNotice(config)shape and also inconsistent with how it’s re-exported fromsrc/index.ts. Consider restoring the original signature (and importing/using the sharedlog), or make thelogparameter optional with a safe default implementation.
src/telem.ts:59DOC_DETECTIVE_METAis parsed withJSON.parsewithout error handling. If the env var contains invalid JSON, this will throw and crash normal execution. Wrap the parse in a try/catch and fall back to{}on failure.
src/heretto.ts:718getResourceDependenciesintentionally stores sentinel string entries like_ditamapPath/_ditamapIdalongsideHerettoResourceInfoobjects, but here it’s cast toRecord<string, HerettoResourceInfo>and assigned toherettoConfig.resourceDependencies. This hides the mixed value types and can cause runtime/type errors for callers expecting onlyHerettoResourceInfo. Consider separating sentinel metadata from the mapping or widening theresourceDependenciestype accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
| return specs; | ||
| } |
There was a problem hiding this comment.
outputResults is async but it doesn't await the file write; it schedules fs.writeFile and immediately continues. This can lead to callers proceeding (or the process exiting) before results are written. Consider using fs.promises.writeFile (or awaiting a Promise wrapper) and handling errors via rejection instead of throwing inside the callback.
| const result = await spawnCommand(`grep -sq "docker\\|lxc\\|kubepods" /proc/1/cgroup`); | ||
| if (result.exitCode === 0) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If both inputs are empty strings, maxLength becomes 0 and (distance / maxLength) yields NaN, so this returns "NaN". Handle the maxLength === 0 case explicitly (e.g., return "0.00").
|
|
||
| const esmContent = `// ESM wrapper for CommonJS output | ||
| import cjsModule from './index.js'; | ||
| export const { detectTests, resolveTests, detectAndResolveTests } = cjsModule; |
There was a problem hiding this comment.
The generated dist/index.mjs only re-exports { detectTests, resolveTests, detectAndResolveTests }, but src/index.ts exports many more runtime symbols (e.g., setConfig, resolveDetectedTests, log, loadDescription, etc.). This will break ESM consumers (and TypeScript users relying on dist/index.d.ts) because the runtime exports won’t match the type declarations. Update the wrapper to export all intended public runtime exports from dist/index.js.
| export const { detectTests, resolveTests, detectAndResolveTests } = cjsModule; | |
| // Re-export all named exports from the CommonJS bundle | |
| export * from './index.js'; |
| "postbuild": "npm run test", | ||
| "test": "mocha src/*.test.js --ignore src/*.integration.test.js", | ||
| "test:integration": "mocha src/*.integration.test.js --timeout 600000", | ||
| "test:all": "mocha src/*.test.js --timeout 600000", |
There was a problem hiding this comment.
With tests now importing from dist/, npm test will fail unless a build has already run. At the same time, postbuild runs npm run test, and the workflow also runs npm test after npm run build, causing tests to run twice. Consider moving compilation to pretest (or updating the test script to build first) and removing postbuild to avoid duplicate test runs.
| export interface MarkupPattern { | ||
| name: string; | ||
| regex: string[]; | ||
| actions?: Record<string, unknown>[]; |
There was a problem hiding this comment.
MarkupPattern.actions is typed as Record<string, unknown>[], but the default fileTypes in src/config.ts use string actions (e.g., actions: ["click"]). Update the type to allow string actions (and any other fields you rely on like batchMatches) so downstream code doesn’t need casts and type mismatches don’t get hidden.
| actions?: Record<string, unknown>[]; | |
| /** | |
| * Actions associated with this markup pattern. | |
| * Can be simple string identifiers (e.g., ["click"]) or | |
| * structured action descriptors. | |
| */ | |
| actions?: (string | Record<string, unknown>)[]; | |
| /** | |
| * When true, indicates this pattern should be applied in batch | |
| * to accumulate multiple matches at once. | |
| */ | |
| batchMatches?: boolean; |
| const logMessage = `(${level.toUpperCase()})`; | ||
| console.log(logMessage); | ||
| console.log(JSON.stringify(message, null, 2)); | ||
| } |
There was a problem hiding this comment.
Variable 'stringOrObject' is of type date, object or regular expression, but it is compared to an expression of type null.
| /** | ||
| * Checks if a test requires a browser driver. | ||
| * @param test - The test to check | ||
| * @returns True if the test requires a driver | ||
| */ | ||
| function isDriverRequired({ test }: { test: DetectedTest }): boolean { | ||
| let driverRequired = false; | ||
| test.steps.forEach((step) => { | ||
| // Check if test includes actions that require a driver. | ||
| driverActions.forEach((action) => { | ||
| if (typeof step[action] !== "undefined") driverRequired = true; | ||
| }); | ||
| }); | ||
| return driverRequired; | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused function isDriverRequired.
| /** | |
| * Checks if a test requires a browser driver. | |
| * @param test - The test to check | |
| * @returns True if the test requires a driver | |
| */ | |
| function isDriverRequired({ test }: { test: DetectedTest }): boolean { | |
| let driverRequired = false; | |
| test.steps.forEach((step) => { | |
| // Check if test includes actions that require a driver. | |
| driverActions.forEach((action) => { | |
| if (typeof step[action] !== "undefined") driverRequired = true; | |
| }); | |
| }); | |
| return driverRequired; | |
| } |
| const proxyquire = require("proxyquire"); | ||
| const fs = require("fs"); | ||
| const { detectTests, resolveTests, detectAndResolveTests } = require("./index"); | ||
| const { detectTests, resolveTests, detectAndResolveTests } = require("../dist/index"); |
There was a problem hiding this comment.
Unused variable detectTests.
| const { detectTests, resolveTests, detectAndResolveTests } = require("../dist/index"); | |
| const { resolveTests, detectAndResolveTests } = require("../dist/index"); |
| const proxyquire = require("proxyquire"); | ||
| const fs = require("fs"); | ||
| const { detectTests, resolveTests, detectAndResolveTests } = require("./index"); | ||
| const { detectTests, resolveTests, detectAndResolveTests } = require("../dist/index"); |
There was a problem hiding this comment.
Unused variable resolveTests.
| const { detectTests, resolveTests, detectAndResolveTests } = require("../dist/index"); | |
| const { detectTests, detectAndResolveTests } = require("../dist/index"); |
| if (testWithV2.id || testWithV2.file || testWithV2.setup || testWithV2.cleanup) { | ||
| if (!test.steps) { | ||
| test.steps = [{ action: "goTo", url: "https://doc-detective.com" } as Step]; | ||
| stepsCleanup = true; |
There was a problem hiding this comment.
The value assigned to stepsCleanup here is unused.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/utils.ts`:
- Around line 1010-1013: The code handling a test with a runShell step pushes
the test into spec.tests then hits a continue which skips validation and never
adds the spec to the specs array; fix by ensuring the spec is pushed into specs
after handling runShell (either remove the premature continue or move the logic
that validates and pushes spec into specs to execute before the continue).
Specifically locate the block that manipulates spec.tests (and checks for
runShell), remove or adjust the continue so that the subsequent spec validation
and the push to the specs collection (the specs array) always runs for specs
containing runShell tests.
- Around line 1238-1243: The function calculatePercentageDifference can divide
by zero when both inputs are empty; update it to check maxLength === 0 and
return "0.00" (or other appropriate zero-percentage string) immediately in that
case, otherwise compute percentage as before using levenshteinDistance(text1,
text2) and return percentage.toFixed(2); ensure the early-return preserves the
function's string return type and reference calculatePercentageDifference and
levenshteinDistance when locating the code.
- Around line 346-350: The code calls fs.statSync(source) without checking that
source exists, so non-existent local paths will throw; update the logic around
fs.statSync/source to first verify the path (e.g., use fs.existsSync(source)) or
wrap the fs.statSync(source) call in a try-catch, and on missing path return a
clear error or fallback instead of letting an uncaught exception propagate;
ensure isFile and isDir are only derived when stat succeeds and that any error
path produces a descriptive message referencing the source value.
- Around line 1051-1055: The outputResults function uses callback-style
fs.writeFile and can return before the write finishes; change it to use the
promise-based API and await the write so callers only proceed once the file is
persisted. Specifically, in outputResults replace the callback fs.writeFile
usage with fs.promises.writeFile (or import writeFile from fs/promises) and
await that call, allowing any errors to be thrown and propagated from the async
function.
🧹 Nitpick comments (3)
src/utils.ts (3)
683-867: Wrap switch case declarations in blocks to prevent scope leakage.Biome flagged multiple
constdeclarations inside switch cases that can be erroneously accessed by other cases. Wrap each case body with declarations in a block.Example fix for the testStart case
case "testStart": + { statementContent = statement[1] || statement[0]; const parsedTest = parseObject({ stringifiedObject: statementContent }); // Skip if parseObject returned a non-object... if (typeof parsedTest !== "object" || parsedTest === null) { break; } test = parsedTest as DetectedTest; // ... rest of case logic ... tests.push(test); break; + }Apply similar block wrapping to cases at lines 698 (
testWithV2), 722 (testWithDetectSteps), 750 (testWithDetect), 754 (markupWithActions), 855 (step), and 856-860 (validation).
1067-1079: Function is declaredasyncbut contains no asynchronous operations.
loadEnvscan be simplified to a synchronous function since it only uses synchronousfs.existsSyncandrequire.Suggested simplification
-export async function loadEnvs(envsFile: string): Promise<LoadEnvsResult> { +export function loadEnvs(envsFile: string): LoadEnvsResult {
1196-1198: Empty error handler silently swallows spawn errors.The error event handler on line 1197 does nothing, which means spawn failures (e.g., command not found on some platforms) are silently ignored. Consider logging or propagating the error.
Suggested improvement
- runCommand.on("error", (_error) => {}); + let spawnError: Error | null = null; + runCommand.on("error", (error) => { + spawnError = error; + });Then check
spawnErrorbefore returning and include it in the result if needed.
|
|
||
| // Check if source is a file or directory | ||
| const stat = fs.statSync(source); | ||
| const isFile = stat.isFile(); | ||
| const isDir = stat.isDirectory(); |
There was a problem hiding this comment.
Missing error handling for non-existent paths.
If a source path doesn't exist (not a URL, not heretto:), fs.statSync(source) will throw an uncaught exception. Consider wrapping with try-catch or checking existence first.
Suggested fix
// Check if source is a file or directory
+ if (!fs.existsSync(source)) {
+ log(config, "warning", `Source path not found: ${source}. Skipping.`);
+ continue;
+ }
const stat = fs.statSync(source);📝 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 source is a file or directory | |
| const stat = fs.statSync(source); | |
| const isFile = stat.isFile(); | |
| const isDir = stat.isDirectory(); | |
| // Check if source is a file or directory | |
| if (!fs.existsSync(source)) { | |
| log(config, "warning", `Source path not found: ${source}. Skipping.`); | |
| continue; | |
| } | |
| const stat = fs.statSync(source); | |
| const isFile = stat.isFile(); | |
| const isDir = stat.isDirectory(); |
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 346 - 350, The code calls fs.statSync(source)
without checking that source exists, so non-existent local paths will throw;
update the logic around fs.statSync/source to first verify the path (e.g., use
fs.existsSync(source)) or wrap the fs.statSync(source) call in a try-catch, and
on missing path return a clear error or fallback instead of letting an uncaught
exception propagate; ensure isFile and isDir are only derived when stat succeeds
and that any error path produces a descriptive message referencing the source
value.
|
|
||
| spec.tests.push(test); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Spec with runShell step is not pushed to specs array.
After creating a test with runShell step and pushing it to spec.tests, the continue statement skips the remaining logic including the spec validation and push to specs. The spec is never added to the result.
Suggested fix
spec.tests.push(test);
- continue;
+ specs.push(spec);
+ continue;
}📝 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.
| spec.tests.push(test); | |
| continue; | |
| } | |
| spec.tests.push(test); | |
| specs.push(spec); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 1010 - 1013, The code handling a test with a
runShell step pushes the test into spec.tests then hits a continue which skips
validation and never adds the spec to the specs array; fix by ensuring the spec
is pushed into specs after handling runShell (either remove the premature
continue or move the logic that validates and pushes spec into specs to execute
before the continue). Specifically locate the block that manipulates spec.tests
(and checks for runShell), remove or adjust the continue so that the subsequent
spec validation and the push to the specs collection (the specs array) always
runs for specs containing runShell tests.
| export async function outputResults(outputPath: string, results: unknown, config: Config): Promise<void> { | ||
| const data = JSON.stringify(results, null, 2); | ||
| fs.writeFile(outputPath, data, (err) => { | ||
| if (err) throw err; | ||
| }); |
There was a problem hiding this comment.
fs.writeFile callback is not awaited; function may return before write completes.
The function is declared async but uses callback-style fs.writeFile without awaiting. This can cause issues if callers expect the file to be written when the function returns.
Suggested fix using fs.promises
export async function outputResults(outputPath: string, results: unknown, config: Config): Promise<void> {
const data = JSON.stringify(results, null, 2);
- fs.writeFile(outputPath, data, (err) => {
- if (err) throw err;
- });
+ await fs.promises.writeFile(outputPath, data);
log(config, "info", "RESULTS:");As per coding guidelines: "Use async/await for asynchronous operations".
📝 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.
| export async function outputResults(outputPath: string, results: unknown, config: Config): Promise<void> { | |
| const data = JSON.stringify(results, null, 2); | |
| fs.writeFile(outputPath, data, (err) => { | |
| if (err) throw err; | |
| }); | |
| export async function outputResults(outputPath: string, results: unknown, config: Config): Promise<void> { | |
| const data = JSON.stringify(results, null, 2); | |
| await fs.promises.writeFile(outputPath, data); |
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 1051 - 1055, The outputResults function uses
callback-style fs.writeFile and can return before the write finishes; change it
to use the promise-based API and await the write so callers only proceed once
the file is persisted. Specifically, in outputResults replace the callback
fs.writeFile usage with fs.promises.writeFile (or import writeFile from
fs/promises) and await that call, allowing any errors to be thrown and
propagated from the async function.
| export function calculatePercentageDifference(text1: string, text2: string): string { | ||
| const distance = levenshteinDistance(text1, text2); | ||
| const maxLength = Math.max(text1.length, text2.length); | ||
| const percentageDiff = (distance / maxLength) * 100; | ||
| return percentageDiff.toFixed(2); | ||
| } |
There was a problem hiding this comment.
Potential division by zero when both strings are empty.
If both text1 and text2 are empty strings, maxLength will be 0, causing a division by zero.
Suggested fix
export function calculatePercentageDifference(text1: string, text2: string): string {
const distance = levenshteinDistance(text1, text2);
const maxLength = Math.max(text1.length, text2.length);
+ if (maxLength === 0) return "0.00";
const percentageDiff = (distance / maxLength) * 100;
return percentageDiff.toFixed(2);
}📝 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.
| export function calculatePercentageDifference(text1: string, text2: string): string { | |
| const distance = levenshteinDistance(text1, text2); | |
| const maxLength = Math.max(text1.length, text2.length); | |
| const percentageDiff = (distance / maxLength) * 100; | |
| return percentageDiff.toFixed(2); | |
| } | |
| export function calculatePercentageDifference(text1: string, text2: string): string { | |
| const distance = levenshteinDistance(text1, text2); | |
| const maxLength = Math.max(text1.length, text2.length); | |
| if (maxLength === 0) return "0.00"; | |
| const percentageDiff = (distance / maxLength) * 100; | |
| return percentageDiff.toFixed(2); | |
| } |
🤖 Prompt for AI Agents
In `@src/utils.ts` around lines 1238 - 1243, The function
calculatePercentageDifference can divide by zero when both inputs are empty;
update it to check maxLength === 0 and return "0.00" (or other appropriate
zero-percentage string) immediately in that case, otherwise compute percentage
as before using levenshteinDistance(text1, text2) and return
percentage.toFixed(2); ensure the early-return preserves the function's string
return type and reference calculatePercentageDifference and levenshteinDistance
when locating the code.
Summary
dist/)Changes
TypeScript Infrastructure
tsconfig.json: ES2022 target, Node16 module resolution, strict modescripts/createEsmWrapper.js: Creates ESM wrapper for dual module supportsrc/types.ts: Shared interfaces for Config, FileType, DetectedSpec, etc.src/doc-detective-common.d.ts: Type declarations for doc-detective-commonConverted Files
src/index.ts- Main entry point with detectTests, resolveTests, detectAndResolveTestssrc/config.ts- Configuration management with setConfig, resolveConcurrentRunnerssrc/utils.ts- Core utilities including parseTests, qualifyFiles, log, replaceEnvssrc/resolve.ts- Test resolution with resolveDetectedTestssrc/openapi.ts- OpenAPI handling with loadDescription, getOperationsrc/arazzo.ts- Arazzo workflow translationsrc/heretto.ts- Heretto CMS integrationsrc/sanitize.ts- URI/path sanitizationsrc/telem.ts- Telemetry with PostHogPackage Updates
compile,build,prebuild,postbuildCI/CD Updates
npm run buildbefore test and publishTesting
All existing tests pass against the compiled TypeScript output.
Summary by CodeRabbit
New Features
Build & Infrastructure
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.