feat: add sources of initial scaffold #1
Conversation
pihanya
changed the title
|
@codex review this PR and give things to be fixed/improved |
Review
Testing
Notes
|
- cli: stop Koin in finally to prevent resource leaks - core: ensure ~/.config/ktools exists in ConfigLoader - core: generate BuildInfo.kt with version at build time - version: read version from BuildInfo instead of system property - tests(cli): add real CLI tests for help and version output - build: configure Gradle wrapper task to 8.10.2; Dokka v2 already in use Refs: code review action items
pihanya
force-pushed
the
ai/codex/generate-scaffolding-for-ktools-linux-cli-tool
branch
from
e37af13 to
7be2695
Compare
…oad from resources; fix lints
… add slf4j-simple runtime
…Windows with ktools shim
…abilize options serialization - Use npm md-to-pdf instead of deprecated md2pdf package - Respect nvm by resolving node/npm from NVM_BIN if present - Serialize options to JSON safely (avoid Any serializer errors) - Co-locate bridge.js with node_modules for proper require resolution - Increase npm install timeout for initial puppeteer download - Spotless: prefer nvm node automatically; fall back to autodetect - Update docs requirements to reflect engine modes and error codes Verified end-to-end: `ktools md2pdf README.md -o output.pdf` succeeds in native mode.
…al and non-functional tables
… and organization
|
@codex review this PR please |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| // Build conversion command | ||
| val command = buildMd2PdfCommand(inputFile.name, outputFile.name, options) | ||
| val dockerCommand = mutableListOf( | ||
| "docker", "exec", | ||
| "-w", "/workspace", | ||
| "-v", "${tempDir.absolutePath}:/workspace" | ||
| ) | ||
| dockerCommand.add(containerId) | ||
| dockerCommand.addAll(command) |
There was a problem hiding this comment.
[P1] Remove invalid volume option from docker exec call
The Docker bridge tries to mount the temporary workspace by passing -v ${tempDir}:/workspace to docker exec, but docker exec does not support volume mounting. As written, every conversion executed through the docker engine will fail before invoking md2pdf with an error like unknown shorthand flag: 'v' in -v. The container never sees the input file, so the docker engine path cannot work at all. Start the container with the bind mount (e.g. during docker run) or copy files in/out instead of adding -v to docker exec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex seems like a nice one. Please proof with docs
There was a problem hiding this comment.
Summary
- Removed the invalid
-vflag fromdocker execand now copy conversion files into the container, retrieving output and cleaning up afterward for reliable Docker-based processing commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/DockerBridge.ktL202-L276 - Cached the selected engine name during conversions to avoid repeated detection and ensure error handlers report the correct engine context commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/Md2PdfProcessor.ktL74-L88 commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/Md2PdfProcessor.ktL134-L157
- Logged task completion for the Docker workspace fix and engine caching in the project’s TODO list TODO.mdL16
Testing
- ✅
./gradlew spotlessCheck --no-daemon - ✅
./gradlew test --no-daemon
| } catch (e: ValidationException) { | ||
| ProcessingResult( | ||
| success = false, | ||
| message = e.message ?: "Validation failed", | ||
| engine = getActiveEngine(), | ||
| exitCode = 1, | ||
| hint = e.hint | ||
| ) | ||
| } catch (e: EngineException) { | ||
| ProcessingResult( | ||
| success = false, | ||
| message = e.message ?: "Engine error", | ||
| engine = getActiveEngine(), | ||
| exitCode = e.exitCode, | ||
| hint = e.hint | ||
| ) | ||
| } catch (e: Exception) { | ||
| log.error { "Unexpected error during conversion: ${e.message}" } | ||
| ProcessingResult( | ||
| success = false, | ||
| message = "Unexpected error: ${e.message}", | ||
| engine = getActiveEngine(), | ||
| exitCode = 2, |
There was a problem hiding this comment.
[P2] Avoid re-running engine detection inside error handlers
Every catch block in Md2PdfProcessor.convert calls getActiveEngine() to populate the failure response. When engine selection itself fails (e.g. no Node or Docker installed), getActiveEngine() invokes EngineDetector.selectEngine again, throws the same IllegalStateException, and bypasses the tailored error handling. The command then bubbles up as an "Unexpected error" with exit code 2 instead of the intended engine-specific hint and exit code. Cache the engine name before the try/catch or guard against failures so that error reporting does not rethrow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Cached the md2pdf engine selection so error handlers reuse the detected engine instead of triggering detection again commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/Md2PdfProcessor.ktL75-L88 commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/Md2PdfProcessor.ktL134-L156
- Replaced the unsupported
-vflag ondocker execwithdocker cpto move files into and out of the container during conversion commands/md2pdf/src/main/kotlin/com/bepiscorp/ktools/commands/md2pdf/DockerBridge.ktL202-L259
Testing
- ✅
./gradlew spotlessCheck --no-daemon - ✅
./gradlew test --no-daemon
…' into ai/codex/proof-read-dockerbridge.kt-at-line-218
fix(md2pdf): copy files for docker and cache engine name
- Introduced OutputDestination sealed class to manage output options (stdout or file). - Updated Md2PdfCommand to conditionally respond based on output type, avoiding stdout contamination. - Refactored Md2PdfProcessor to determine output destination and file handling. - Added detailed logging in NodeBridge for installation and conversion processes. - Configured Log4j2 for logging output to SYSTEM_ERR to prevent stdout contamination.
Summary
com.bepiscorp.ktoolsTesting
gradle test --no-daemongradle spotlessCheck --no-daemonhttps://chatgpt.com/codex/tasks/task_e_68b1cc48caa08329b6824b17db51f1cc