Conversation
related #234
wnghdcjfe
requested review from
PENEKhun and
json-choi
and removed request for
json-choi
|
Caution Review failedThe pull request is closed. WalkthroughRefactors LLM generation to remove Markdown/spec-path support; CLI now requires an Express app path and env, analyzeRoutes produces RouteResult[] fed to a new route-driven itdoc generator; several AST utilities removed or replaced; example app/tests/OAS updated and Sinon added to generated test prelude. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as CLI User
participant CLI as bin/index.ts
participant Gen as script/llm/index.generateByLLM
participant Loader as script/llm/loader.loadFile
participant Analyzer as analyzeRoutes
participant OpenAI as LLM (gpt-5)
participant Writer as FS
User->>CLI: run command (appPath [, envPath])
CLI->>Gen: generateByLLM(appPath, envPath)
Gen->>Loader: loadFile("app", appPath)
Loader-->>Gen: resolved app path
Gen->>Analyzer: analyzeRoutes(app)
Analyzer-->>Gen: RouteResult[]
loop per chunk
Gen->>OpenAI: getItdocPrompt(routesChunk, lang, part)
OpenAI-->>Gen: itdoc code chunk
end
Gen->>Writer: write output.(ts|js)
Gen-->>CLI: done
sequenceDiagram
autonumber
participant OldFlow as Pre-change
participant NewFlow as Post-change
participant Parser as MD Parser
participant Analyzer as analyzeRoutes
participant OpenAI as LLM
OldFlow->>Parser: parse Markdown spec (specPath)
Parser-->>OldFlow: Markdown blocks
OldFlow->>OpenAI: prompt with Markdown chunks
NewFlow->>Analyzer: analyze Express app
Analyzer-->>NewFlow: RouteResult[]
NewFlow->>OpenAI: prompt with route chunks
note right of NewFlow: Spec-path/Markdown branch removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/dsl/generator/OpenAPIGenerator.ts (1)
27-28: Migrate OpenAPIGenerator singleton to a static property and add test reset helpersTests in OpenAPIGenerator.test.ts define a static
instanceon the constructor, but the current implementation uses a module-scopedinstancevariable. This misalignment leads to flakiness and prevents proper teardown between test cases.Refactor
lib/dsl/generator/OpenAPIGenerator.ts:
- Remove the top-level
let instance: OpenAPIGenerator | null = null- Inside the
OpenAPIGeneratorclass:
- Add
private static instance: OpenAPIGenerator | null = null;- Update
getInstance()to referenceOpenAPIGenerator.instanceinstead of the module var- Add a test helper
/** @test-only */ public static __resetForTesting(): void { OpenAPIGenerator.instance = null }- Add an instance method to clear accumulated state
/** @test-only */ public clear(): void { this.testResults = [] }Example diff:
--- a/lib/dsl/generator/OpenAPIGenerator.ts - let instance: OpenAPIGenerator | null = null + // (moved inside class as a private static) export class OpenAPIGenerator { + private static instance: OpenAPIGenerator | null = null; public static getInstance(): OpenAPIGenerator { - if (!instance) { - instance = new OpenAPIGenerator() - } - return instance + if (!OpenAPIGenerator.instance) { + OpenAPIGenerator.instance = new OpenAPIGenerator() + } + return OpenAPIGenerator.instance } + /** @test-only */ + public static __resetForTesting(): void { + OpenAPIGenerator.instance = null + } + + /** @test-only */ + public clear(): void { + this.testResults = [] + } // ... }With this change, tests can either continue using
Object.defineProperty(OpenAPIGenerator, "instance", { value: null, writable: true })or call
OpenAPIGenerator.__resetForTesting(), and then invokegetInstance()andgenerator.clear()in setup/teardown to ensure a fresh, clean state for each test.
🧹 Nitpick comments (25)
lib/dsl/generator/OpenAPIGenerator.ts (3)
360-378: Normalize examples to avoid leaking DSL field wrappers into OAS examplesexamples/express/expected/oas.json shows DSL field objects bleeding into examples (e.g., members[].username is an object with description/example/required). You already have normalizeExample but it’s unused here. Normalize before assigning example values.
Apply within this block:
const exampleKey = result.testSuiteDescription || (isErrorStatus ? "Error Response" : "Success Response") - const exampleValue = contentObj.example || result.response.body || null + const exampleValue = contentObj.example || result.response.body || null + const normalizedExample = this.normalizeExample( + exampleValue, + combinedContent[contentType].schema, + ) if (isErrorStatus && exampleValue) { combinedContent[contentType].examples[exampleKey] = { value: { error: { message: result.testSuiteDescription || `Status ${statusCode} error`, code: `ERROR_${statusCode}`, }, }, } } else { combinedContent[contentType].examples[exampleKey] = { - value: exampleValue, + value: normalizedExample, } }
387-389: Nit: 100 shouldn’t be treated as “no content”1xx statuses are interim responses; treating 100 like 204/304 can lead to odd behavior. Keep 204 and 304 only.
- return numericStatusCode === 204 || numericStatusCode === 304 || numericStatusCode === 100 + return numericStatusCode === 204 || numericStatusCode === 304
534-572: Optional: Convert Express-style “:param” to OpenAPI “{param}” in pathsSpec-compliant OAS paths use {param}. We currently keep :param (e.g., “/users/:userId”), which many tools consider invalid. Consider normalizing keys in paths (e.g., during generatePaths) and ensuring parameters remain synchronized.
I can propose a safe refactor that rewrites keys and adjusts operationId stability if you want to pursue this now.
script/llm/examples/index.ts (3)
54-71: Clarify stubbing prerequisites or simplify the example 500 scenarioThis sample uses sandbox.stub and getRouteLayer inside a template string. In isolation, readers (or tools that extract these) won’t have sinon sandbox or the helper in scope, which can confuse consumers.
Two options:
- Prepend a brief note (outside the string or as a comment inside the string) indicating sinon sandbox and getRouteLayer helpers are expected to be available in the test runtime.
- Or simplify by simulating a 500 through input that triggers an error path (avoids mocking internals in a high-level example).
Want me to adjust the snippet so it’s self-contained?
102-114: Headers-based “200 OK” example may mislead (backend ignores these headers)GET /users/:userId in examples/express/expressApp.js does not use token query or Authorization header. Keeping this in a “valid headers” example might imply it’s required. Either adjust the description to indicate headers are optional or update the app (or a separate example route) to actually enforce them.
49-53: Nit: Fix Korean typo “메세지” → “메시지” for consistencyOther places use “에러 메시지”. Unify here too.
examples/express/expected/oas.json (3)
92-98: Optional: Use OpenAPI path templating “/users/{userId}” instead of “/users/:userId”This will improve compatibility with validators and generators. It pairs well with ensuring a corresponding path parameter is defined (already handled in generator).
498-507: Examples contain DSL field objects; prefer raw example valuesMany example bodies (e.g., users.members) include objects with description/example/required. This stems from not normalizing DSL fields before writing examples. Consider integrating normalizeExample in the generator (see suggested diff in OpenAPIGenerator.ts) so examples are concrete JSON.
1179-1180: Nit: Hyphen in operationId (“getCached-data”)operationId can be any string, but many codegen tools prefer simple identifiers (e.g., getCachedData). Consider normalizing to camelCase for ergonomics.
examples/express/expressApp.js (3)
8-29: 201 response should not emit a JSON body; avoid res.json() with no payloadres.status(201).json() without a body typically sends "null" as JSON. Prefer no body for 201 here.
- return res.status(201).json() + return res.status(201).send()
63-64: 204 responses should not have a bodyUse send() (or end()) to avoid sending a JSON payload with 204.
- return res.status(204).json() + return res.status(204).send()
49-49: Good call to comment out dynamic timestampsFor the noted “Date differences” flakiness, alternative approach: inject a deterministic timestamp during tests (e.g., freeze timers) rather than altering the payload. If you want, I can add a small testing utility to stub Date or use fake timers around the affected tests.
script/llm/loader/index.ts (1)
45-55: Default app path candidates: consider adding common entry points.Optional but helpful for auto-resolution: include server/main variants often used in Node/TS projects.
Apply:
app: [ path.resolve(process.cwd(), "app.js"), path.resolve(process.cwd(), "app.ts"), path.resolve(process.cwd(), "src/app.js"), path.resolve(process.cwd(), "src/app.ts"), path.resolve(process.cwd(), "index.js"), path.resolve(process.cwd(), "index.ts"), + path.resolve(process.cwd(), "server.js"), + path.resolve(process.cwd(), "server.ts"), + path.resolve(process.cwd(), "src/server.js"), + path.resolve(process.cwd(), "src/server.ts"), + path.resolve(process.cwd(), "main.js"), + path.resolve(process.cwd(), "main.ts"), + path.resolve(process.cwd(), "src/main.js"), + path.resolve(process.cwd(), "src/main.ts"), ],bin/index.ts (1)
67-76: CLI help still references removed -p/--path option.The PR drops spec-path support. The error/help lines still suggest
-p/--pathwhich is confusing. Update the message to only mention-a/--app.The diff above (in the async/await change) already removes these lines.
script/llm/parser/analyzer/routeAnalyzer.ts (1)
203-207: Pass localArrays (and ast) into analyzeMemberExpression for better res.json object extraction.Currently only
retis passed. Forres.status(...).json({ ... })extraction,extractValuebenefits fromlocalArrays(e.g., arrays declared in the handler) and possiblyast. Consider extending the signature and passing them here.Proposed signature change in variableAnalyzer:
-export function analyzeMemberExpression(memPath: NodePath<t.MemberExpression>, ret: any) { +export function analyzeMemberExpression( + memPath: NodePath<t.MemberExpression>, + ret: any, + localArrays: Record<string, any[]>, + ast?: t.File +) { // ... - analyzeJsonObjectFields(firstArg, ret, ret.variableMap, ret.localArrays ?? {}, ret.ast as t.File | undefined) + analyzeJsonObjectFields(firstArg, ret, ret.variableMap, localArrays, ast) }And update the call here:
MemberExpression(memPath: NodePath<t.MemberExpression>) { analyzeMemberExpression(memPath, ret, localArrays, ast) }script/llm/parser/analyzer/variableAnalyzer.ts (1)
213-239: localArrays not wired: extractValue in res.json({ ... }) won’t see handler-local arrays.
analyzeJsonObjectFieldsis called withret.localArrays ?? {}butret.localArraysis never set. Pass thelocalArrayscaptured inanalyzeFunctionBodydown toanalyzeMemberExpression(and then intoanalyzeJsonObjectFields).Minimal approach (outside this hunk): change
analyzeMemberExpressionsignature to acceptlocalArraysand adjust the call in routeAnalyzer:VariableAnalyzer:
export function analyzeMemberExpression( memPath: NodePath<t.MemberExpression>, ret: any, localArrays: Record<string, any[]>, ast?: t.File ) { // ... analyzeJsonObjectFields(firstArg, ret, ret.variableMap, localArrays, ast) }RouteAnalyzer:
MemberExpression(memPath: NodePath<t.MemberExpression>) { analyzeMemberExpression(memPath, ret, localArrays, ast) }script/llm/prompt/index.ts (3)
86-111: English prompt typos: parseInt and status() formatting.Minor but could confuse the model:
- "pathInt()" → "parseInt()"
- "status ()" → "status()"
Apply:
- etc: "The part with function_call must be output as a function as it is. ex) fetchedAt: field('Inquiry Time (ISO 8601)', new Date().toISOString()) Also, if the type can be inferred by pathInt(), etc., it must be returned to that type. ex) For pathInt(page), it must be output in int type such as 1, 2, etc. And describeAPI must have summary, tag, and description values. Do not create non-existent endpoints or parameter/header/body fields. Use only the information in the input. Be sure to have a valid status () after res().The json values given in responses are not arbitrarily changed.", + etc: "The part with function_call must be output as a function as is. e.g., fetchedAt: field('Inquiry Time (ISO 8601)', new Date().toISOString()). Also, if the type can be inferred by parseInt(), etc., it must be returned as that type. e.g., for parseInt(page), it should be an int like 1, 2, etc. And describeAPI must have summary, tag, and description values. Do not create non-existent endpoints or parameter/header/body fields. Use only the information in the input. Be sure to have a valid status() after res(). The json values given in responses must not be changed arbitrarily.",
57-70: Korean prompt typo: duplicated token "field는".Small grammar cleanup improves clarity.
Apply:
- fieldGuide2: - '만약 a : b 라고 했을 때 b가 객체인 경우 field는 field는("b설명", b) 이런식으로 객체 전체를 설명해야 합니다. 객체 안에 field가 들어가면 안됩니다.', + fieldGuide2: + '만약 a : b 라고 했을 때 b가 객체인 경우 field는 ("b설명", b) 형식으로 객체 전체를 설명해야 합니다. 객체 안에 field가 들어가면 안 됩니다.',
119-143: Internationalize static headings so EN prompts are fully in English.Even with
isEn=true, headings like “필수 규칙” and other lines remain in Korean. Since you instruct the model to output code only, this isn’t blocking but improving consistency is low-risk.Apply:
- return ` -다음 테스트 설명을 기반으로 itdoc 테스트 스크립트를 ${codeMessage}로 생성하세요. - -필수 규칙: + const intro = isEn + ? `Based on the following analysis, generate itdoc test scripts in ${codeMessage}.` + : `다음 테스트 설명을 기반으로 itdoc 테스트 스크립트를 ${codeMessage}로 생성하세요.` + const rulesTitle = isEn ? "Mandatory rules:" : "필수 규칙:" + const routesTitle = isEn ? "[Route analysis results to test]" : "[테스트를 진행해야하는 라우트 분석 결과]" + const exampleTitle = isEn ? "[Function examples]" : "[함수 예시]" + + return ` +${intro} + +${rulesTitle} - ${lang.noComments} - ${lang.codeOnly} - ${lang.orderGuide} - ${lang.branchGuide} - ${lang.branchGuide2} - ${lang.fieldGuide} - ${lang.fieldGuide2} - ${lang.headerGuide} - ${lang.noPathImport} - ${lang.initGiven} - ${lang.etc} - ${partGuide} - ${lang.outputInstruction} -[테스트를 진행해야하는 라우트 분석 결과] +${routesTitle} ${JSON.stringify(routesChunk, null, 2)} -[함수 예시] +${exampleTitle} ${itdocExample} `.trim()script/llm/index.ts (5)
45-56: Make chunking deterministic (sort groups and contents)Object key iteration order and upstream file discovery can be non-deterministic. Sorting ensures stable chunk boundaries and reduces flaky outputs/tests.
Apply this diff to sort prefixes and each group by path/method:
function groupAndChunkSpecRoutes(routes: RouteResult[], chunkSize: number = 10): RouteResult[][] { const by: Record<string, RouteResult[]> = {} for (const r of routes) { const prefix = getPathPrefix(r.path || "/unknown") ;(by[prefix] ||= []).push(r) } const out: RouteResult[][] = [] - for (const group of Object.values(by)) { - for (const c of _.chunk(group, chunkSize)) out.push(c) - } + const prefixes = Object.keys(by).sort() + for (const prefix of prefixes) { + const group = by[prefix]! + group.sort((a, b) => a.path.localeCompare(b.path) || a.method.localeCompare(b.method)) + for (const c of _.chunk(group, chunkSize)) out.push(c) + } return out }
114-117: Stale function name in error logThe log label still says makeitdocByMD; update to the current function name.
- logger.error(`makeitdocByMD() ERROR: ${error}`) + logger.error(`makeitdoc() ERROR: ${String(error)}`)
166-171: Pretty-print analyzedRoutes.json for debuggingPretty printing improves readability when users inspect this artifact.
- fs.writeFileSync( - path.join(outputDir, "analyzeRoutes.json"), - JSON.stringify(analyzedRoutes), - "utf8", - ) + fs.writeFileSync( + path.join(outputDir, "analyzeRoutes.json"), + JSON.stringify(analyzedRoutes, null, 2), + "utf8", + )
172-176: Outdated error message (mentions markdown spec)This path now generates from route analysis, not Markdown.
- if (!doc) { - logger.error("Failed to generate itdoc from markdown spec.") - process.exit(1) - } + if (!doc) { + logger.error("Failed to generate itdoc from route analysis.") + process.exit(1) + }
213-232: Remove unused targetApp variable in JS preludetargetApp is declared but unused; keep the header minimal.
const { describeAPI, itDoc, HttpStatus, field, HttpMethod } = require("itdoc") -const targetApp = app const sinon = require("sinon")script/llm/parser/utils/extractValue.ts (1)
18-21: Interop hack for @babel/traverse is brittleThe ts-expect-error + default indirection often indicates config friction. If your tsconfig enables esModuleInterop/syntheticDefaultImports, you can import directly without the cast.
Consider switching to:
- tsconfig: set "esModuleInterop": true and "allowSyntheticDefaultImports": true
- import style:
import traverse, { NodePath } from "@babel/traverse"If changing tsconfig isn’t feasible, current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
bin/index.ts(1 hunks)examples/express/__tests__/expressApp.test.js(2 hunks)examples/express/__tests__/expressApp2.test.js(0 hunks)examples/express/expected/oas.json(1 hunks)examples/express/expressApp.js(4 hunks)lib/dsl/generator/OpenAPIGenerator.ts(1 hunks)script/llm/examples/index.ts(3 hunks)script/llm/index.ts(3 hunks)script/llm/loader/index.ts(2 hunks)script/llm/parser/analyzer/returnValueExtractor.ts(0 hunks)script/llm/parser/analyzer/routeAnalyzer.ts(1 hunks)script/llm/parser/analyzer/variableAnalyzer.ts(3 hunks)script/llm/parser/utils/extractValue.ts(8 hunks)script/llm/prompt/index.ts(2 hunks)script/makedocs/index.ts(0 hunks)
💤 Files with no reviewable changes (3)
- script/makedocs/index.ts
- examples/express/tests/expressApp2.test.js
- script/llm/parser/analyzer/returnValueExtractor.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
lib/dsl/generator/OpenAPIGenerator.ts (4)
lib/dsl/generator/TestResultCollector.ts (3)
TestResultCollector(26-56)constructor(33-35)getInstance(41-47)lib/dsl/generator/TestEventManager.ts (1)
TestEventManager(34-106)lib/__tests__/unit/dsl/OpenAPIGenerator.test.ts (3)
Object(30-33)Object(39-41)Object(35-37)lib/dsl/generator/types/TestResult.ts (1)
IOpenAPIGenerator(63-66)
script/llm/parser/analyzer/routeAnalyzer.ts (1)
script/llm/parser/analyzer/responseAnalyzer.ts (1)
analyzeResponseCall(108-158)
script/llm/examples/index.ts (2)
examples/express/__tests__/expressApp2.test.js (1)
apiDoc(15-57)examples/fastify-js/__test__/app.test.js (1)
apiDoc(28-54)
examples/express/__tests__/expressApp.test.js (5)
examples/express/expressApp.js (1)
app(3-3)lib/dsl/interface/describeAPI.ts (1)
describeAPI(30-58)lib/dsl/interface/itDoc.ts (1)
itDoc(21-48)lib/dsl/interface/field.ts (1)
field(44-50)examples/express/__tests__/expressApp2.test.js (3)
apiDoc(15-57)apiDoc(16-27)apiDoc(43-56)
script/llm/parser/analyzer/variableAnalyzer.ts (2)
script/llm/parser/utils/extractValue.ts (1)
extractValue(446-523)script/llm/parser/analyzer/routeAnalyzer.ts (1)
MemberExpression(206-208)
bin/index.ts (1)
script/llm/index.ts (1)
generateByLLM(131-238)
examples/express/expressApp.js (1)
examples/express/__tests__/expressApp2.test.js (1)
apiDoc(15-57)
script/llm/prompt/index.ts (1)
script/llm/parser/type/interface.ts (1)
RouteResult(29-42)
script/llm/index.ts (5)
script/llm/parser/type/interface.ts (1)
RouteResult(29-42)script/llm/prompt/index.ts (1)
getItdocPrompt(43-143)lib/dsl/test-builders/RequestBuilder.ts (1)
res(71-73)script/llm/parser/index.ts (1)
analyzeRoutes(29-45)lib/dsl/interface/describeAPI.ts (1)
method(30-58)
🔇 Additional comments (16)
lib/dsl/generator/OpenAPIGenerator.ts (1)
37-40: JSDoc addition improves clarity — LGTMThe added class-level documentation succinctly states the responsibilities and pattern. No issues.
script/llm/examples/index.ts (1)
1-15: License header addition — LGTMexamples/express/expected/oas.json (1)
1182-1182: Header name normalization to “if-none-match” — LGTMMatches how Node/Express lower-case header keys are typically accessed (req.headers["if-none-match"]).
examples/express/expressApp.js (1)
30-32: Error handling for unexpected exceptions — LGTMClear 500 with a stable error shape helps downstream OAS generation and tests.
examples/express/__tests__/expressApp.test.js (2)
4-57: Signup tests align with new validations — LGTMCovers happy path and both 400 validations; matches app’s try/catch behavior.
470-481: Lower-case “if-none-match” in header assertion — LGTMConsistent with Express’s normalized header keys and the app’s implementation.
script/llm/loader/index.ts (1)
21-21: Verified removal of "spec" is safe
Ran searches forloadFile("spec", …)andFileTypeunions containing"spec"across the codebase—no matches found. Removing"spec"fromFileTypewill not break existing callers.script/llm/parser/analyzer/routeAnalyzer.ts (1)
163-187: Doc improvements are clear and accurate.The updated JSDoc precisely documents traversal behavior, captured data, and in-place mutation of the accumulator.
script/llm/parser/analyzer/variableAnalyzer.ts (3)
31-63: LGTM: res.json object field extraction is a good addition.Populating
responseJsonFieldMapviaextractValueadds useful metadata for downstream consumers.
81-89: LGTM: capture local array literal values for later resolution.Storing concrete samples in
localArraysimproves extraction quality for identifier and invocation descriptors.
92-104: LGTM: record identifier initializers into variableMap.This broadens
extractValue’s resolution surface across the handler.script/llm/index.ts (3)
30-37: Prefix extraction helper looks goodHandles empty and shallow paths correctly and normalizes to leading "/". No issues found.
191-211: Test prelude assumes a global afterEach; verify runner types for TSIn TS mode, afterEach is assumed as a global. Ensure your tsconfig includes the appropriate test types (e.g., @types/jest or @types/mocha) for compilation, or consider importing the test adapter’s afterEach if you expose one.
Would you like me to add conditional imports or a minimal adapter to avoid TS compile errors in environments without global test types?
195-211: Helper getRouteLayer: safe and pragmatic approachThe router stack scan is defensive and short-circuits correctly. Returning undefined when not found is fine for generated tests.
script/llm/parser/utils/extractValue.ts (2)
84-108: Member-access descriptor is well-formed and resilientCovers optional chaining and computed properties with safe normalization. No issues.
466-489: Good: Call/optional-call descriptors enriched with samples when availableThe conditional enrichment for samples/sample is practical and backward-compatible.
PENEKhun
changed the title
- Removed unnecessary parts in the LLM script to simplify logic - Added error handling to improve stability and prevent silent failures - Updated related documentation to reflect the revised implementation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bin/index.ts (1)
60-60: LGTM: action is now async and awaits generateByLLMThis addresses the prior “not awaited” issue and prevents early process exit or unhandled rejections. Good error handling with try/catch and exit code.
script/llm/index.ts (1)
158-164: Empty analysis array currently passes the nullish checkanalyzeRoutes returns an array; when it’s empty, the existing condition won’t trigger. Treat “no routes found” as an error for clear feedback.
Apply:
- const analyzedRoutes = await analyzeRoutes(resolvedAppPath) - if (!analyzedRoutes) { - logger.error( - "AST analysis failed. Please ensure your routes use app.get() or router.post() format.", - ) - process.exit(1) - } + const analyzedRoutes = await analyzeRoutes(resolvedAppPath) + if (!analyzedRoutes || analyzedRoutes.length === 0) { + logger.error( + "No routes found. Ensure your Express app declares routes via app.<method>() or router.<method>().", + ) + process.exit(1) + }
🧹 Nitpick comments (9)
itdoc-doc/docs/experiments/LLM.mdx (1)
197-197: Tighten wording and reflect TS support in the one-linerMake the sentence a bit crisper and note that .ts is supported so readers don’t assume JS-only.
Apply:
-When you run this command, it analyzes the Express application defined in `{YOUR_APP_FILE_PATH}` and automatically generates tests for the API endpoints in that application. +When you run this command, it analyzes the Express application at `{YOUR_APP_FILE_PATH}` (supports .js and .ts) and generates itdoc test code for all detected API endpoints.bin/index.ts (1)
67-75: Missing help output when required -a/--app is absentYou already print examples; also emit the command help to guide users immediately before exiting.
Apply:
if (!options.app) { logger.error( "An Express app path (-a) must be provided. By default, the OpenAI key (OPENAI_API_KEY in .env) is loaded from the root directory, but you can customize the path with -e/--env if needed.", ) logger.info("ex) itdoc generate -a ../app.js") logger.info("ex) itdoc generate --app ../app.js") logger.info("ex) itdoc generate -a ../app.js -e <custom path : env>") - process.exit(1) + program.outputHelp() + process.exit(1) }script/llm/index.ts (7)
45-56: Deterministic ordering before chunking to reduce diff churnSort each group by path then method for stable, predictable output across runs. This helps test snapshots and PR diffs.
Apply:
function groupAndChunkSpecRoutes(routes: RouteResult[], chunkSize: number = 10): RouteResult[][] { const by: Record<string, RouteResult[]> = {} for (const r of routes) { const prefix = getPathPrefix(r.path || "/unknown") ;(by[prefix] ||= []).push(r) } const out: RouteResult[][] = [] - for (const group of Object.values(by)) { - for (const c of _.chunk(group, chunkSize)) out.push(c) + for (const group of Object.values(by)) { + group.sort((a, b) => { + const pa = a.path || "" + const pb = b.path || "" + if (pa !== pb) return pa.localeCompare(pb) + return (a.method || "").localeCompare(b.method || "") + }) + for (const c of _.chunk(group, chunkSize)) out.push(c) } return out }
85-89: Improve traceability: include chunk and retry counters in the logKeep your current “part” semantics; only enrich the log line so you can correlate calls with route chunks.
Apply:
- logger.info(`[makeitdoc] Attempting GPT call : ${gptCallCount} times`) + logger.info( + `[makeitdoc] GPT call #${gptCallCount} (chunk ${chunkIndex + 1}/${specChunks.length}, try ${retry + 1}/${maxRetry})`, + )
166-171: Outdated error message mentions “markdown spec”This path is now route-based. Update the message to prevent confusion.
Apply:
- if (!doc) { - logger.error("Failed to generate itdoc from markdown spec.") + if (!doc) { + logger.error("Failed to generate itdoc from route analysis.") process.exit(1) }
187-205: TS prelude: make import robust (named vs default) and add targetApp alias + fake timersGenerated tests may face export-shape variance (named export ‘app’ vs default). Also, stabilize time-sensitive tests (new Date()) with Sinon fake timers as noted in the PR description.
Apply:
- importStatement = `import { app } from "${appImportPath}" -import { describeAPI, itDoc, HttpStatus, field, HttpMethod } from "itdoc" -import sinon from "sinon" -const sandbox = sinon.createSandbox() + importStatement = `import * as appModule from "${appImportPath}" +import { describeAPI, itDoc, HttpStatus, field, HttpMethod } from "itdoc" +import sinon from "sinon" +const sandbox = sinon.createSandbox() +const app: any = (appModule as any).app ?? (appModule as any).default ?? (appModule as any) +const targetApp = app function getRouteLayer(app, method, path) { method = String(method).toLowerCase() const stack = app && app._router && app._router.stack ? app._router.stack : [] for (const layer of stack) { if (!layer.route) continue if (layer.route.path !== path) continue if (!layer.route.methods || !layer.route.methods[method]) continue const routeStack = layer.route.stack || [] if (routeStack.length > 0) return routeStack[0] } } +beforeEach(() => { + // Stabilize time-sensitive expectations in generated tests + sandbox.useFakeTimers(new Date("2024-01-01T00:00:00.000Z")) +}) afterEach(() => { sandbox.restore() }) `
207-226: JS prelude: require shape can vary; normalize module to app + stabilize timeLike TS, normalize the import (named app, default, or module itself) and set targetApp explicitly. Add fake timers to avoid flakiness around Date.
Apply:
- importStatement = `const app = require('${appImportPath}') -const { describeAPI, itDoc, HttpStatus, field, HttpMethod } = require("itdoc") -const targetApp = app -const sinon = require("sinon") + importStatement = `const appModule = require('${appImportPath}') +const { describeAPI, itDoc, HttpStatus, field, HttpMethod } = require("itdoc") +const app = appModule.app || appModule.default || appModule +const targetApp = app +const sinon = require("sinon") const sandbox = sinon.createSandbox() function getRouteLayer(app, method, path) { method = String(method).toLowerCase() const stack = app && app._router && app._router.stack ? app._router.stack : [] for (const layer of stack) { if (!layer.route) continue if (layer.route.path !== path) continue if (!layer.route.methods || !layer.route.methods[method]) continue const routeStack = layer.route.stack || [] if (routeStack.length > 0) return routeStack[0] } } +beforeEach(() => { + sandbox.useFakeTimers(new Date("2024-01-01T00:00:00.000Z")) +}) afterEach(() => { sandbox.restore() }) `
229-231: Consider logging the exact output path for DXAfter writing the file, include the resolved path to help users locate the artifact quickly.
Apply:
- result = importStatement + "\n\n" + result.trim() - fs.writeFileSync(outPath, result, "utf8") - logger.info(`[generateByLLM] itdoc LLM SCRIPT completed.`) + result = importStatement + "\n\n" + result.trim() + fs.writeFileSync(outPath, result, "utf8") + logger.info(`[generateByLLM] itdoc LLM SCRIPT completed. Wrote: ${outPath}`)
70-115: Optional: expose generation knobs for cost and determinismNot required for this PR, but consider making chunkSize, temperature, and max tokens configurable via env/CLI to balance cost, latency, and stability.
- ITDOC_CHUNK_SIZE (default 10)
- ITDOC_TEMPERATURE (default 0.2)
- ITDOC_MAX_COMPLETION_TOKENS (default 10000)
I can send a follow-up patch wiring env overrides into groupAndChunkSpecRoutes and the OpenAI call—say the word.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bin/index.ts(2 hunks)itdoc-doc/docs/experiments/LLM.mdx(1 hunks)script/llm/index.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T13:49:48.673Z
Learnt from: PENEKhun
PR: do-pa/itdoc#235
File: script/llm/index.ts:90-94
Timestamp: 2025-08-21T13:49:48.673Z
Learning: In the itdoc project, GPT-5 can be used with openai.chat.completions.create() method along with max_completion_tokens parameter, contrary to some documentation suggesting it requires the Responses API.
Applied to files:
script/llm/index.ts
🧬 Code graph analysis (2)
script/llm/index.ts (3)
script/llm/parser/type/interface.ts (1)
RouteResult(29-42)script/llm/prompt/index.ts (2)
getItdocPrompt(43-143)getItdocPrompt(31-91)script/llm/parser/index.ts (1)
analyzeRoutes(29-45)
bin/index.ts (2)
script/llm/index.ts (1)
generateByLLM(131-232)script/llm/parser/index.ts (1)
analyzeRoutes(29-45)
🔇 Additional comments (3)
bin/index.ts (1)
79-87: LGTM: awaited generateByLLM with error path exits non‑zeroThe try/catch with a clear error log and exit(1) is solid for CLI UX.
script/llm/index.ts (2)
34-37: Prefix helper is fine; consider normalizing multi-slash inputsgetPathPrefix works as intended. If you expect paths with duplicate slashes or trailing “/”, current filter(Boolean) already mitigates—no change required.
90-94: API usage accepted as-is (gpt-5 via chat.completions with max_completion_tokens)Acknowledging the team’s established approach: using chat.completions with model "gpt-5" and max_completion_tokens is an intentional, valid choice in this codebase.
Note: This acceptance leverages the previous learning captured for this repository about GPT‑5 usage patterns.
- Installed sinon as a regular dependency - Ensures sinon is available in runtime environment - Edited getItdocPrompt annotation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (2)
95-97: Right-size runtime deps: avoid shipping test-only tools to consumers
- sinon appears to be introduced to support test scaffolding; it doesn’t need to be a production dependency. Having it in both deps and devDeps also duplicates install size and may cause resolution friction.
- widdershins is typically a doc-generation tool. If it isn’t used at runtime by the published CLI, prefer devDependency (or a subpackage that owns doc build).
Action (minimal, safe):
- Remove sinon from "dependencies" (keep it in devDependencies).
- If generated projects or consumers must provide sinon at test time, list it as a peerDependency instead (optional but recommended for clearer expectations).
Apply this diff to the changed hunk:
- "widdershins": "^4.0.1", - "sinon": "^20.0.0" + "widdershins": "^4.0.1"And add sinon as a peer (outside this hunk):
"peerDependencies": { "jest": "^29.0.0", - "mocha": "^11.0.0" + "mocha": "^11.0.0", + "sinon": "^20.0.0" },If widdershins is not required by the published CLI at runtime, move it to devDependencies as well. I can scan the repo for runtime imports to confirm. Would you like a script for that?
96-96: Duplicate declaration of sinon in dependencies and devDependenciessinon is listed in both sections with the same version. Keep only one to avoid duplication and potential hoisting quirks. Given usage context, devDependencies (and optionally peerDependencies) is the better home.
Minimal fix (if you don’t add it as a peer):
- "sinon": "^20.0.0",Also applies to: 118-118
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
package.json(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI Pipeline
package.json
[error] 1-1: Lockfile dependencies do not match package.json; update the lockfile by running 'pnpm install' to synchronize.
🔇 Additional comments (1)
package.json (1)
96-96: Keep @types/sinon for TypeScript supportsinon v20 does not ship its own TypeScript declarations—@types/sinon remains the official source of Sinon typings. Please retain the existing entry in your devDependencies (or move it to “dependencies” only if you’re publishing a library that exposes Sinon types).
• File: package.json
– Line 104 (where "@types/sinon" is declared)Suggested state (no removal):
# retain Sinon’s TS definitions - # nothing to remove here – keep "@types/sinon": "^17.0.2" in devDependenciesLikely an incorrect or invalid review comment.
| "mocha": "^11.0.0" | ||
| }, | ||
| "packageManager": "pnpm@10.5.2", | ||
| "packageManager": "pnpm@10.15.0", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
CI is failing due to lockfile drift after packageManager bump
GitHub Actions reports: “Lockfile dependencies do not match package.json; run 'pnpm install'.” The bump to pnpm@10.15.0 and added deps require committing an updated pnpm-lock.yaml.
Do this locally:
- corepack enable
- Use pnpm 10.15.0: corepack prepare pnpm@10.15.0 --activate
- Install to refresh lockfile: pnpm install
- Commit pnpm-lock.yaml
Also ensure CI uses the same pnpm version (e.g., pnpm/action-setup@v4 with version: 10.15.0).
I can provide a CI patch if you share the workflow filename.
🏁 Script executed:
#!/bin/bash
# Verify duplicates and lockfile status
set -euo pipefail
echo "Duplicates across deps/devDeps:"
python - <<'PY'
import json, sys
p=json.load(open('package.json'))
dups=set(p.get('dependencies',{})).intersection(set(p.get('devDependencies',{})))
print(sorted(dups))
PY
echo "Check that lockfile references newly added deps:"
if rg -n "sinon" pnpm-lock.yaml >/dev/null; then
echo "sinon present in lockfile"
else
echo "sinon missing from lockfile (run pnpm install)"; exit 1
fi
echo "pnpm version in CI workflow (if present):"
fd -a ".github/workflows" -t d >/dev/null 2>&1 && rg -n "pnpm" .github/workflows || echo "No CI workflows found"Length of output: 487
Remove duplicate dependency & confirm CI pnpm setup
The lockfile has been refreshed—pnpm-lock.yaml now includes the newly added deps (e.g. sinon) after bumping to pnpm@10.15.0, so the original lockfile-drift error should be resolved.
However, there are still two critical items to address:
-
Duplicate declaration of “sinon”
In package.json, “sinon” appears in both dependencies and devDependencies. Please remove one of these entries to avoid confusion and ensure a single source of truth. -
CI configuration for pnpm@10.15.0
I couldn’t locate any GitHub Actions workflows under .github/workflows that set up pnpm. Make sure your CI pipelines install and use pnpm@10.15.0 (for example, with pnpm/action-setup@v4 and version: 10.15.0). If your workflows live elsewhere or you’d like a patch, please share the CI workflow file.
🤖 Prompt for AI Agents
In package.json around line 130, remove the duplicate "sinon" entry so it
appears only once (choose either dependencies or devDependencies based on where
it's used—typically devDependencies for testing/tools), and update any package
references accordingly; then ensure your GitHub Actions CI workflows under
.github/workflows install and pin pnpm@10.15.0 (e.g., add or update a step using
pnpm/action-setup@v4 with version: 10.15.0) so CI uses the same pnpm version as
packageManager in package.json.
* fix: llm script related #234 * refactor: Remove logic to generate files for debug * refactor(llm): clean up script, add error handling, and update docs - Removed unnecessary parts in the LLM script to simplify logic - Added error handling to improve stability and prevent silent failures - Updated related documentation to reflect the revised implementation * chore: add sinon to project dependencies - Installed sinon as a regular dependency - Ensures sinon is available in runtime environment - Edited getItdocPrompt annotation * chore(lockfile): regenerate pnpm-lock.yaml to match package.json
2 participants
related #234
What needs to be solved in the future
new Date(), test failure due to the difference in the degree of second between the test and the apptest command
Summary by CodeRabbit
New Features
Changes
Bug Fixes / API
Tests
Chores