Integrate wrkx 0.40.0 + srcx 0.43.0, full task implementation#1
Merged
Conversation
….5% coverage - Upgrade dependencies: wrkx 0.40.0, srcx 0.43.0 - Implement all workflow tasks: Propose, Apply, Verify, Archive, Continue, Explore, Feedback, Ff, Onboard, BulkArchive - Add task models: TaskDefinition, TaskStatus, ChangeLogger, TaskParser - Expand SkillGenerator with multi-agent support (Claude, Copilot, Codex, OpenCode) and instruction file generation - Add AgentDispatcher for CLI agent orchestration - Add ChangeWriter for change lifecycle management - CleanTask preserves user files, only removes opsx symlinks - 370+ tests, 98.5% line coverage
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an atomic-task-execution feature and broad OpsX workspace functionality: task parsing/execution/logging, agent dispatch, unified skill generation with symlinks, concrete Gradle tasks (propose/apply/verify/.../clean/sync), ChangeConfig/status extensions, new workflow primitives, many tests, and documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Gradle CLI
participant Task as OpsX Gradle Task
participant Prompt as PromptBuilder
participant FS as Filesystem
participant Agent as External AI CLI
participant Logger as ChangeLogger
CLI->>Task: run opsx-propose / opsx-apply / opsx-verify / opsx-continue
Task->>FS: read change files (.opsx.yaml, proposal.md, design.md, tasks.md, feedback.md)
Task->>Prompt: request combined context (srcx + project + change)
Prompt-->>Task: return prompt markdown
Task->>Agent: AgentDispatcher.dispatch(prompt, model)
Agent-->>Task: exitCode, logFile
Task->>Logger: append log entry for task execution
Task->>FS: ChangeWriter.updateStatus / write feedback
Task-->>CLI: exit (success/failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt (1)
93-150:⚠️ Potential issue | 🟡 MinorTask count mismatch: comment says 13 but 14 tasks are registered; iteration lists miss 2 tasks.
Per context snippet from
src/main/kotlin/zone/clanker/opsx/Opsx.kt:30-46, there are 14 task constants (10 workflow + 4 infrastructure), but:
- Line 93 comment says "all 13 tasks exist"
- The existence check (lines 94-106) verifies 13 tasks (missing
TASK_CLEAN)- The iteration lists at lines 110-124 and 131-144 only contain 12 tasks, missing both
TASK_CLEANandTASK_LISTThis means group and description assertions don't cover all registered tasks.
💚 Proposed fix to include all 14 tasks
- then("all 13 tasks exist") { + then("all 14 tasks exist") { project.tasks.findByName(Opsx.TASK_PROPOSE).shouldNotBeNull() // ... existing assertions ... project.tasks.findByName(Opsx.TASK_LIST).shouldNotBeNull() + project.tasks.findByName(Opsx.TASK_CLEAN).shouldNotBeNull() } then("all tasks have group opsx") { val allTasks = listOf( // ... existing tasks ... Opsx.TASK_STATUS, Opsx.TASK_SYNC, + Opsx.TASK_CLEAN, + Opsx.TASK_LIST, ) // ... } then("all tasks have descriptions") { val allTasks = listOf( // ... existing tasks ... Opsx.TASK_STATUS, Opsx.TASK_SYNC, + Opsx.TASK_CLEAN, + Opsx.TASK_LIST, ) // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt` around lines 93 - 150, The test wrongly asserts "all 13 tasks exist" and only checks 13/14 tasks and omits TASK_CLEAN and TASK_LIST from the group/description iterations; update the existence assertions to include Opsx.TASK_CLEAN so all 14 Opsx task constants are checked, and extend the two task lists used in the "all tasks have group opsx" and "all tasks have descriptions" blocks to include Opsx.TASK_CLEAN and Opsx.TASK_LIST (ensure each project.tasks.findByName(...) is asserted and description/group checks use Opsx.GROUP and non-blank description).
🟡 Minor comments (9)
src/test/kotlin/zone/clanker/opsx/task/ProposeTaskTest.kt-83-95 (1)
83-95:⚠️ Potential issue | 🟡 MinorScenario title and assertion intent are inconsistent.
The
whenlabel says explicit-name override behavior, but the assertion validates spec precedence. Please rename the scenario for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/ProposeTaskTest.kt` around lines 83 - 95, The scenario label string in ProposeTaskTest is misleading: change the `when` description currently saying "explicit name overrides spec" to match the assertion that validates spec precedence (e.g., "spec is used when no name override" or "no explicit name: spec is used") so the `when` block description aligns with the `task.resolveChangeName("my-spec", "some prompt") shouldBe "my-spec"` assertion; update the `when` string in the test containing the ProjectBuilder / task.resolveChangeName code accordingly.opsx/changes/atomic-task-execution/proposal.md-19-30 (1)
19-30:⚠️ Potential issue | 🟡 MinorLabel fenced code blocks with a language for markdownlint compliance.
Several fences are unlabeled, which triggers MD040.
Suggested fix
-``` +```text - [ ] a1b2c3d4e5 | Test ChangeConfig.parse ...@@
-+text
/opsx-apply e5f6g7h8i9@@ -``` +```text - [>] a1b2c3d4e5 | Test ChangeConfig.parse — started just now ...@@
-+text
opsx/changes/refactor-config-parser/
...@@ -``` +```text propose → proposal.md, design.md created ...</details> Also applies to: 40-42, 79-84, 90-98, 102-116 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@opsx/changes/atomic-task-execution/proposal.mdaround lines 19 - 30, The
markdown has unlabeled fenced code blocks causing MD040; update each
triple-backtick block in opsx/changes/atomic-task-execution/proposal.md to
include a language tag (use "text") so linters accept them — e.g., change ``` toChangeConfig.parse" and "e5f6g7h8i9 | Implement ChangeConfig.parse", the command block with "/opsx-apply e5f6g7h8i9", the status update block starting "- [>] a1b2c3d4e5 | Test ChangeConfig.parse — started just now", and the directory/metadata blocks such as "opsx/changes/refactor-config-parser/" and "propose → proposal.md, design.md created"; apply the same change to the other affected ranges (40-42, 79-84, 90-98, 102-116) so every fenced block has a language label.README.md-61-63 (1)
61-63:⚠️ Potential issue | 🟡 MinorAdd a language to the lifecycle fenced block.
Line 61 uses an unlabeled fence, which triggers MD040 in markdownlint.
Suggested fix
-``` +```text draft → active → in-progress → completed/done → verified → archived</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 61 - 63, The fenced code block containing the
lifecycle string "draft → active → in-progress → completed/done → verified →
archived" is unlabeled and triggers MD040; update that fenced block in README.md
by adding a language label (e.g., use ```text) on the opening fence so the block
becomes labeled and markdownlint no longer flags it.</details> </blockquote></details> <details> <summary>opsx/changes/atomic-task-execution/design.md-13-20 (1)</summary><blockquote> `13-20`: _⚠️ Potential issue_ | _🟡 Minor_ **Add a language identifier to the model fenced block.** Line 13 uses an unlabeled fence and can fail MD040 checks. <details> <summary>Suggested fix</summary> ```diff -``` +```text TaskDefinition: id: String (10-char alphanumeric) ... ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@opsx/changes/atomic-task-execution/design.mdaround lines 13 - 20, The
fenced code block defining TaskDefinition currently has no language identifier;
update the block fence to include a language tag (e.g.,text oryaml) so
the markdown passes MD040 checks and to make the TaskDefinition model block
(TaskDefinition, id, name, description, status, dependencies) properly labeled.</details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/SyncTaskTest.kt-31-32 (1)</summary><blockquote> `31-32`: _⚠️ Potential issue_ | _🟡 Minor_ **Guard symlink creation for platform compatibility.** `Files.createSymbolicLink()` calls at lines 31 and 56 lack error handling and can fail on Windows (where symlink creation often requires admin privileges or special filesystem support), causing these test cases to fail for developers on Windows. Consider wrapping symlink creation in a try-catch or conditionally skipping these scenarios on platforms where symlinks are unsupported. Also applies to: CleanTaskTest.kt lines 74, 109 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/SyncTaskTest.kt` around lines 31 - 32, Wrap the Files.createSymbolicLink(...) calls in SyncTaskTest and CleanTaskTest with a guard that catches platform/permission failures (e.g., IOException, UnsupportedOperationException, SecurityException) and skips the remainder of the test when symlink creation fails; specifically, update the create call in SyncTaskTest (the Files.createSymbolicLink(link.toPath(), sourceFile.toPath()) invocation) and the two locations in CleanTaskTest so that you either surround them with try/catch and return (or call the test-assumption API like org.junit.Assume.assumeTrue / org.junit.jupiter.api.Assumptions.assumeTrue) on error, or first check a platform capability flag before calling Files.createSymbolicLink, ensuring tests do not fail on Windows or environments where symlinks are unsupported. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/ContinueTask.kt-49-54 (1)</summary><blockquote> `49-54`: _⚠️ Potential issue_ | _🟡 Minor_ **`detectProgress` doesn't account for all task states.** The regex only counts `- [x]` (done) and `- [ ]` (pending), but based on `StatusTaskTest` (lines 220-230), tasks can also be `- [>]` (in-progress) and `- [!]` (blocked). These are currently not counted in `total`, which could underreport progress. <details> <summary>🔧 Proposed fix to include all task states</summary> ```diff internal fun detectProgress(tasksContent: String): String { val done = Regex("- \\[x]", RegexOption.IGNORE_CASE).findAll(tasksContent).count() - val pending = Regex("- \\[ ]").findAll(tasksContent).count() - val total = done + pending + val pending = Regex("- \\[ ]").findAll(tasksContent).count() + val inProgress = Regex("- \\[>]").findAll(tasksContent).count() + val blocked = Regex("- \\[!]").findAll(tasksContent).count() + val total = done + pending + inProgress + blocked return if (total == 0) "no tasks found" else "$done/$total tasks complete" } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/ContinueTask.kt` around lines 49 - 54, The detectProgress function currently only counts done ("- [x]") and pending ("- [ ]") tasks, causing total to miss other states; update detectProgress to also count in-progress ("- [>]") and blocked ("- [!]") by adding Regex finds for those markers (e.g., inProgress and blocked), include them in the total calculation (total = done + pending + inProgress + blocked), and return the same formatted string using the updated total; locate the function detectProgress in ContinueTask.kt and modify the Regex usages to include these additional states. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt-62-66 (1)</summary><blockquote> `62-66`: _⚠️ Potential issue_ | _🟡 Minor_ **Timeout occurs silently without logging.** When the verify command times out, the process is forcibly destroyed but no message is logged. The user only sees the generic "failed with exit code -1" message without knowing it was a timeout. <details> <summary>🔧 Proposed fix to add timeout logging</summary> ```diff val completed = process.waitFor(VERIFY_TIMEOUT_SECONDS, TimeUnit.SECONDS) if (!completed) { process.destroyForcibly() + logger.warn("opsx-verify: verify command timed out after ${VERIFY_TIMEOUT_SECONDS}s") return@runCatching -1 } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt` around lines 62 - 66, In VerifyTask inside the runCatching block where you call process.waitFor(VERIFY_TIMEOUT_SECONDS, TimeUnit.SECONDS), add a log call right after detecting !completed (before calling process.destroyForcibly() and returning -1) so the timeout is recorded; use the existing logger (e.g., processLogger or logger in VerifyTask) to log a clear message that the verify subprocess timed out after VERIFY_TIMEOUT_SECONDS and include contextual info such as the command or task id, then destroy the process and return -1 as before. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt-54-68 (1)</summary><blockquote> `54-68`: _⚠️ Potential issue_ | _🟡 Minor_ **ProcessBuilder shell execution won't work on Windows.** `ProcessBuilder("sh", "-c", verifyCommand)` is Unix-specific and will fail on Windows systems. To support both platforms, detect the OS and use the appropriate shell: - Windows: `cmd.exe`, `/c`, command - Unix: `/bin/sh`, `-c`, command Use `System.getProperty("os.name").lowercase()` to determine the OS and construct the command accordingly. Alternatively, document this as a Unix-only feature. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt` around lines 54 - 68, The ProcessBuilder invocation in VerifyTask currently uses a Unix-only shell ("sh", "-c", verifyCommand) which fails on Windows; modify the code that builds the process (the block using ProcessBuilder with verifyCommand and VERIFY_TIMEOUT_SECONDS) to detect the OS via System.getProperty("os.name").lowercase(), and choose the shell args accordingly (on Windows use "cmd.exe", "/c", verifyCommand; otherwise use "/bin/sh", "-c", verifyCommand), then pass that array to ProcessBuilder while keeping the existing directory, redirect, timeout, and exit handling unchanged. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/CleanTask.kt-41-48 (1)</summary><blockquote> `41-48`: _⚠️ Potential issue_ | _🟡 Minor_ **Misleading log when file has no OPSX markers.** `removeMarkers()` returns early (line 93) when the file contains no marker, but the caller unconditionally logs "removed OPSX section" and sets `cleaned = true`. This produces false-positive output. <details> <summary>🐛 Proposed fix</summary> Return a boolean from `removeMarkers` to indicate whether anything was changed: ```diff - private fun removeMarkers(file: File) { + private fun removeMarkers(file: File): Boolean { val marker = "<!-- OPSX:AUTO -->" val endMarker = "<!-- /OPSX:AUTO -->" val content = file.readText() - if (!content.contains(marker)) return + if (!content.contains(marker)) return false val before = content.substringBefore(marker).trimEnd() val after = content.substringAfter(endMarker, "").trimStart() val result = "$before\n$after".trim() if (result.isEmpty()) { file.delete() } else { file.writeText("$result\n") } + return true } ``` Then update the caller: ```diff SkillGenerator.instructionFiles(project.rootDir).forEach { file -> if (file.exists()) { - removeMarkers(file) - logger.quiet("opsx-clean: removed OPSX section from ${file.name}") - cleaned = true + if (removeMarkers(file)) { + logger.quiet("opsx-clean: removed OPSX section from ${file.name}") + cleaned = true + } } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/CleanTask.kt` around lines 41 - 48, Change removeMarkers to return a Boolean indicating whether it modified the file (true) or returned early because no OPSX markers were present (false); update the caller in the SkillGenerator.instructionFiles loop to capture that boolean and only call logger.quiet("opsx-clean: removed OPSX section from ${file.name}") and set cleaned = true when removeMarkers(file) returns true. Reference removeMarkers and the loop iterating SkillGenerator.instructionFiles(project.rootDir) so you update both the remover and its caller logic accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (20)</summary><blockquote> <details> <summary>src/main/kotlin/zone/clanker/opsx/workflow/AgentDispatcher.kt (1)</summary><blockquote> `23-26`: **Different failure modes are collapsed into the same `exitCode` value.** Timeout and startup failures both resolve to `-1`, so callers can’t react correctly (retry vs. config/tooling error). Consider adding an explicit failure reason in `Result`. Also applies to: 52-60 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/workflow/AgentDispatcher.kt` around lines 23 - 26, The Result data class collapses distinct failure modes into exitCode (e.g., timeout vs startup), so add an explicit failure reason field (e.g., enum class FailureReason { SUCCESS, TIMEOUT, STARTUP_FAILURE, OTHER }) to the Result data class (and update any secondary Result creation around the block referenced at 52-60) and populate it where you currently set exitCode = -1 for timeout/startup; update callers that inspect Result to switch on failureReason (or both exitCode and failureReason) so they can distinguish retryable TIMEOUT from non-retryable STARTUP_FAILURE. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/model/TaskDefinition.kt (1)</summary><blockquote> `14-26`: **Enforce the documented `id` invariant in the model.** KDoc states a 10-character alphanumeric ID, but construction currently accepts any string. Add constructor validation to keep bad task IDs from entering workflow state. <details> <summary>Proposed hardening</summary> ```diff data class TaskDefinition( val id: String, val name: String, val description: String, val status: TaskStatus, val dependencies: List<String>, -) +) { + init { + require(id.matches(Regex("^[A-Za-z0-9]{10}$"))) { + "Task id must be a 10-character alphanumeric string" + } + } +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/model/TaskDefinition.kt` around lines 14 - 26, The TaskDefinition data class currently accepts any string for id despite KDoc requiring a 10-character alphanumeric identifier; add validation in the TaskDefinition primary constructor (e.g., an init block) to verify id matches a regex like ^[A-Za-z0-9]{10}$ and throw an IllegalArgumentException with a clear message if it does not, so invalid IDs cannot be constructed (also update/adjust any callers or tests that create TaskDefinition to use valid ids). ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/ArchiveTaskTest.kt (1)</summary><blockquote> `17-207`: **Collapse repeated setup into a table-driven test helper.** The repeated fixture creation across statuses is substantial; extracting a helper + status matrix will make this spec easier to evolve and less error-prone. <details> <summary>Refactor sketch</summary> ```diff +private fun makeFixture(status: String): Pair<ArchiveTask, Change> { + val changeDir = File.createTempFile("opsx-archive", "").apply { delete(); mkdirs(); deleteOnExit() } + val projectDir = File.createTempFile("opsx-archive-proj", "").apply { delete(); mkdirs(); deleteOnExit() } + val project = ProjectBuilder.builder().withProjectDir(projectDir).build() + val task = project.tasks.create("test-archive", ArchiveTask::class.java).apply { + extension = createExtension() + } + return task to Change("test", status, emptyList(), changeDir) +} + +listOf("completed", "done", "verified", "archived").forEach { status -> + `when`("status is $status") { + val (task, change) = makeFixture(status) + then("returns true") { task.validateForArchive(change) shouldBe true } + } +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/ArchiveTaskTest.kt` around lines 17 - 207, The tests in ArchiveTaskTest repeat the same fixture setup for many statuses; extract a table-driven helper that builds the change, project, and task once per status and asserts expected validateForArchive outcomes. Implement a private helper (e.g., a function buildAndAssert(status: String, expected: Boolean)) that creates the temporary changeDir and Change, creates the Project via ProjectBuilder and ArchiveTask, sets task.extension = createExtension(), and asserts task.validateForArchive(change) == expected; then replace each `when` block with a loop over a status->expected map/list to call that helper for each status. Ensure references to Change, ProjectBuilder.builder(), ArchiveTask::class.java, createExtension(), and validateForArchive are used so locating code is straightforward. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/BulkArchiveTask.kt (1)</summary><blockquote> `40-44`: **Avoid duplicating archive eligibility policy in `findArchivable`.** This helper hardcodes eligible statuses, which can drift from the central transition model in `ChangeStatus`. Consider centralizing this rule (e.g., a single `isBulkArchivable()` policy) to prevent future divergence. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/BulkArchiveTask.kt` around lines 40 - 44, findArchivable duplicates the eligibility rule by hardcoding statuses; add a centralized policy method (e.g., ChangeStatus.isBulkArchivable(status: String) or an instance method on ChangeStatus) inside the ChangeStatus type and use that from findArchivable instead of the set literal. Update findArchivable to call ChangeStatus.isBulkArchivable(ChangeStatus.from(change.status)) (or ChangeStatus.from(change.status).isBulkArchivable()) so the rule lives only in ChangeStatus and cannot drift. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/FfTask.kt (1)</summary><blockquote> `46-67`: **Consider reusing PromptBuilder instance.** `buildFfPrompt` creates a new `PromptBuilder` at line 50, but `run()` already creates one at line 29. The method could accept the existing builder as a parameter to avoid redundant instantiation. <details> <summary>♻️ Optional: Pass PromptBuilder to buildFfPrompt</summary> ```diff - internal fun buildFfPrompt( + internal fun buildFfPrompt( + promptBuilder: PromptBuilder, context: String, changeCtx: String, ): String { - val promptBuilder = PromptBuilder(project.rootDir) return promptBuilder.build( // ... ) } ``` Then in `run()`: ```diff - val fullPrompt = buildFfPrompt(context, changeCtx) + val fullPrompt = buildFfPrompt(promptBuilder, context, changeCtx) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/FfTask.kt` around lines 46 - 67, buildFfPrompt currently instantiates a new PromptBuilder (PromptBuilder(project.rootDir)) even though run() already creates one; change buildFfPrompt to accept a PromptBuilder parameter (e.g., promptBuilder: PromptBuilder) and remove the internal instantiation, then update the call site in run() to pass the existing PromptBuilder instance; ensure signatures and any references to project.rootDir inside buildFfPrompt are removed or replaced by using the passed promptBuilder so no redundant PromptBuilder objects are created. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/StatusTask.kt (1)</summary><blockquote> `28-31`: **Consider using ChangeStatus enum values instead of hardcoded strings.** Lines 29-30 use hardcoded status strings (`"active"`, `"in-progress"`, `"done"`, `"completed"`, `"verified"`) while `STATUS_ORDER` at lines 95-105 uses `ChangeStatus.*.value`. Using the enum consistently would prevent drift if status values change. <details> <summary>♻️ Proposed refactor for consistency</summary> ```diff val byStatus = changes.groupBy { it.status } - val active = countByStatuses(byStatus, "active", "in-progress") - val done = countByStatuses(byStatus, "done", "completed", "verified") + val active = countByStatuses( + byStatus, + ChangeStatus.ACTIVE.value, + ChangeStatus.IN_PROGRESS.value + ) + val done = countByStatuses( + byStatus, + ChangeStatus.DONE.value, + ChangeStatus.COMPLETED.value, + ChangeStatus.VERIFIED.value + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/StatusTask.kt` around lines 28 - 31, Replace the hardcoded status strings in the StatusTask summary with the ChangeStatus enum values to keep status names consistent with STATUS_ORDER: instead of passing literals ("active","in-progress","done","completed","verified") into countByStatuses(from the byStatus grouping), use ChangeStatus.*.value (or refactor countByStatuses to accept ChangeStatus enums) for the same statuses (e.g., ChangeStatus.ACTIVE.value, ChangeStatus.IN_PROGRESS.value, ChangeStatus.DONE.value, ChangeStatus.COMPLETED.value, ChangeStatus.VERIFIED.value) so the logger.quiet("opsx: ${changes.size} changes ($active active, $done done)") reflects enum-driven status names. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/FfTaskTest.kt (1)</summary><blockquote> `80-102`: **Consider consolidating redundant test scenario.** The third `when` block (lines 80-102) tests "both context and change context are non-empty" but only differs from the first scenario (lines 16-56) by checking section headers and separators. These assertions could be added to the first scenario to reduce setup duplication. <details> <summary>♻️ Optional consolidation</summary> ```diff `when`("context and change context are provided") { // ... existing setup and assertions ... + then("includes all sections via PromptBuilder.build") { + prompt shouldContain "# Codebase Context" + prompt shouldContain "# Change" + prompt shouldContain "# Instructions" + } + + then("includes separators") { + prompt shouldContain "---" + } } -`when`("both context and change context are non-empty") { - // This entire block can be removed -} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/FfTaskTest.kt` around lines 80 - 102, The test duplicates assertions about headers and separators in the third `when` block; update the first `when` scenario to also assert the presence of "# Codebase Context", "# Change", "# Instructions" and the "---" separator for the non-empty context/change case (the code that constructs ProjectBuilder, FfTask and calls task.buildFfPrompt should be reused), then remove the entire redundant `when` block that tests "both context and change context are non-empty" so you don't duplicate setup; target symbols: FfTaskTest (the test class), the first `when` block that constructs `project`, `task` and calls `task.buildFfPrompt`, and the third `when` block which should be deleted. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/ApplyTaskTest.kt (1)</summary><blockquote> `18-130`: **Missing test coverage for ARCHIVED and VERIFIED status validation.** Per the implementation in `src/main/kotlin/zone/clanker/opsx/task/ApplyTask.kt:58-65`, `validateForApply` also checks: - If status is `ARCHIVED` → adds `"change is archived"` - If status is `VERIFIED` → adds `"change is already verified"` The current tests only cover file presence scenarios but don't verify these status-based validations. <details> <summary>💚 Suggested additional test scenarios</summary> ```kotlin `when`("change is archived") { val changeDir = File.createTempFile("opsx-change", "").apply { delete() mkdirs() deleteOnExit() } File(changeDir, "proposal.md").writeText("Proposal") File(changeDir, "design.md").writeText("Design") val change = Change("test", "archived", emptyList(), changeDir) // ... create task ... then("returns list with archived message") { val missing = task.validateForApply(change) missing shouldContain "change is archived" } } `when`("change is verified") { // Similar setup with status = "verified" then("returns list with already verified message") { val missing = task.validateForApply(change) missing shouldContain "change is already verified" } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/ApplyTaskTest.kt` around lines 18 - 130, Add two test cases to ApplyTaskTest covering status-based validation in ApplyTask.validateForApply: create Change instances with status "archived" and "verified" (use the same temp changeDir setup and ensure proposal.md and design.md exist), call task.validateForApply(change) and assert the returned list contains "change is archived" for the archived case and "change is already verified" for the verified case; locate the test class ApplyTaskTest and the method validateForApply/ApplyTask to add these when-blocks mirroring existing file-presence tests. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/ListTaskTest.kt (2)</summary><blockquote> `17-31`: **Tests lack meaningful assertions beyond "no exception".** The test verifies completion without error but doesn't assert on observable behavior (e.g., logged output, return values). Consider adding assertions on what `ListTask.run()` actually produces—even if just verifying log messages via a captured logger or checking side effects. This pattern repeats throughout the file (lines 33-50, 52-74, etc.). <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/ListTaskTest.kt` around lines 17 - 31, The tests only assert that ListTask.run() doesn't throw; update ListTaskTest to assert observable behavior after calling task.run() by capturing and asserting log output or side effects: instrument the ListTask instance (or its createExtension()) to accept a test logger/PrintStream, capture logs (e.g., using a TestLogger or ByteArrayOutputStream) and assert expected messages, or assert changes to any accessible properties/state on the task/extension; apply this pattern for other tests in the file that currently only check "no exception" so each test verifies concrete output from ListTask.run() or the extension. ``` </details> --- `17-22`: **Temp directory cleanup may leave orphaned files.** `deleteOnExit()` on the parent directory won't clean up nested subdirectories created during the test. Consider using a JUnit `@TempDir` or Kotest's `tempdir()` extension for reliable cleanup, or explicitly clean up in an `afterTest` block. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/ListTaskTest.kt` around lines 17 - 22, The temp directory created by File.createTempFile("opsx-list", "") and assigned to projectDir can leave nested files behind because deleteOnExit only applies to the top-level file; replace this pattern by using a JUnit `@TempDir` (e.g. declare a lateinit var annotated with `@TempDir` to receive a fresh directory) or use Kotest's tempdir() helper in the test to get an auto-cleaned directory, or if you must keep the current creation use Files.createTempDirectory and add explicit recursive deletion in an afterTest/tearDown block to remove nested contents; update the code that creates projectDir (the File.createTempFile(...).apply { ... } block) and add the chosen cleanup mechanism (`@TempDir` field or afterTest recursive delete) accordingly. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/OnboardTaskTest.kt (1)</summary><blockquote> `17-26`: **Consider extracting repeated test setup.** The project/task creation pattern is duplicated across all test cases. A helper function or `beforeEach` block could reduce boilerplate: ```kotlin fun createTaskWithExtension(): Pair<OnboardTask, File> { val projectDir = File.createTempFile("opsx-onboard", "").apply { delete(); mkdirs(); deleteOnExit() } val project = ProjectBuilder.builder().withProjectDir(projectDir).build() val task = project.tasks.create("test-onboard", OnboardTask::class.java) task.extension = createExtension() return task to projectDir } ``` Also applies to: 58-66, 86-94, 109-117, 135-143 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/OnboardTaskTest.kt` around lines 17 - 26, Extract the repeated test setup in OnboardTaskTest into a single helper function (e.g., createTaskWithExtension) that creates the temporary projectDir, builds the Project via ProjectBuilder.builder().withProjectDir(projectDir).build(), creates the OnboardTask with project.tasks.create("test-onboard", OnboardTask::class.java) and assigns task.extension = createExtension(), returning the task and projectDir; then replace the duplicated blocks in each test case (the blocks that create projectDir, project, task and set task.extension) with calls to this helper (or move it into a beforeEach to assign a shared task variable) to remove boilerplate while keeping behavior identical. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/StatusTaskTest.kt (1)</summary><blockquote> `14-291`: **Tests comprehensively cover scenarios but lack output assertions.** The test suite covers an excellent range of edge cases (no changes, multiple statuses, dependencies, task progress, etc.). However, similar to `ListTaskTest`, assertions only verify completion without exceptions. Consider capturing and asserting on logged output or task state to make these tests more robust regression detectors. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/StatusTaskTest.kt` around lines 14 - 291, StatusTaskTest currently only ensures StatusTask.run() completes without exception; update each `then` block in StatusTaskTest to assert the task's printed/logged output or exposed result instead of just calling task.run(). Capture output from StatusTask.run() (e.g., by injecting or capturing the task logger, redirecting stdout, or using the same output-capture approach used in ListTaskTest) and add assertions that the output contains expected markers for each scenario (e.g., "no changes", change name "feature-a", file names like "proposal.md"/"design.md", dependency names, status group headings following STATUS_ORDER, task progress counts, "no artifacts", and feedback content). Ensure you reference the test helpers used (createExtension()) and the StatusTask.run() invocation so the assertions validate the actual displayed status for each case. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/skill/SkillGeneratorTest.kt (1)</summary><blockquote> `84-101`: **Modifying `user.home` system property may cause issues in parallel tests.** Setting `System.setProperty("user.home", ...)` affects the entire JVM. If tests run in parallel, this could cause flaky behavior. Consider using dependency injection for the home directory or running these tests with `@Isolated` (in Kotest) to ensure serial execution. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/skill/SkillGeneratorTest.kt` around lines 84 - 101, The test mutates the global System property "user.home" which can break parallel tests; avoid changing JVM-wide state by either injecting a configurable home directory into SkillGenerator (e.g., add a constructor param or setter used by the test and update generateSkillFiles to read that injected path) and use the tempDir value there (referencing SkillGenerator and generateSkillFiles), or mark the test as serial/isolated (e.g., add `@Isolated` on SkillGeneratorTest) so it doesn't run concurrently; pick one approach and update the test to stop calling System.setProperty("user.home", ...) and restore it. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt (1)</summary><blockquote> `103-122`: **Same redundant `PromptBuilder` instantiation as `ContinueTask`.** `buildVerifyPrompt` creates a new `PromptBuilder(project.rootDir)` at line 107, but `runAgentVerify` already creates one at line 88. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt` around lines 103 - 122, buildVerifyPrompt redundantly constructs a new PromptBuilder(project.rootDir) while runAgentVerify already creates one; update buildVerifyPrompt to accept an existing PromptBuilder (or a PromptBuilder-typed parameter) instead of creating a new instance, then have runAgentVerify pass its PromptBuilder into buildVerifyPrompt so the same PromptBuilder is reused (referencing buildVerifyPrompt, runAgentVerify, and PromptBuilder/project.rootDir to locate the code). ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/ContinueTask.kt (1)</summary><blockquote> `56-75`: **Redundant `PromptBuilder` instantiation.** `buildContinuePrompt` creates a new `PromptBuilder(project.rootDir)` at line 61, but `run()` already creates one at line 29. Consider passing the existing instance or extracting `project.rootDir` as a parameter to avoid the duplicate instantiation. <details> <summary>♻️ Proposed refactor</summary> ```diff - internal fun buildContinuePrompt( + internal fun buildContinuePrompt( + promptBuilder: PromptBuilder, context: String, changeCtx: String, progress: String, ): String { - val promptBuilder = PromptBuilder(project.rootDir) return promptBuilder.build( ``` And in `run()`: ```diff - val fullPrompt = buildContinuePrompt(context, changeCtx, progress) + val fullPrompt = buildContinuePrompt(promptBuilder, context, changeCtx, progress) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/ContinueTask.kt` around lines 56 - 75, buildContinuePrompt currently constructs a new PromptBuilder(project.rootDir) causing duplicate instantiation since run() already creates one; change buildContinuePrompt to accept either an existing PromptBuilder (preferred) or a rootDir/String parameter so the caller (run) can pass its PromptBuilder instance, then remove the internal PromptBuilder creation inside buildContinuePrompt; update all calls (notably run()) to pass the builder (or rootDir) and ensure buildContinuePrompt signature and usages refer to the same PromptBuilder type to avoid creating a second instance. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/OnboardTask.kt (1)</summary><blockquote> `42-42`: **Redundant `PromptBuilder` instantiation.** A `PromptBuilder` is already created on line 25 and used on lines 26-27. Creating another instance here is unnecessary since `PromptBuilder` is stateless. <details> <summary>♻️ Suggested fix</summary> Pass the existing builder or extract the `build()` call to `run()`: ```diff internal fun buildOnboardPrompt( context: String, projectDesc: String, userPrompt: String, + promptBuilder: PromptBuilder, ): String { - val promptBuilder = PromptBuilder(project.rootDir) return promptBuilder.build( ``` Then update the call site on line 28: ```diff - val fullPrompt = buildOnboardPrompt(context, projectDesc, userPrompt) + val fullPrompt = buildOnboardPrompt(context, projectDesc, userPrompt, promptBuilder) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/OnboardTask.kt` at line 42, You are creating a redundant PromptBuilder instance; reuse the existing PromptBuilder created earlier (the variable defined on line 25) instead of instantiating a new one at the PromptBuilder(project.rootDir) call, or move the build() invocation into the run() flow so you call builder.build() once and pass the result where needed; update the call site that currently calls the second builder.build()/run() to use the previously defined builder (or its built result) and remove the duplicate PromptBuilder(...) instantiation to avoid unnecessary allocations. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/FeedbackTask.kt (1)</summary><blockquote> `67-67`: **Redundant `PromptBuilder` instantiation.** Same pattern as other tasks — a `PromptBuilder` is created on line 35 but a new one is instantiated here. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/FeedbackTask.kt` at line 67, There is a redundant PromptBuilder instantiation in FeedbackTask (the local val promptBuilder = PromptBuilder(project.rootDir)); remove this extra creation and reuse the existing PromptBuilder instance already constructed earlier in the file (use the existing promptBuilder variable or accept the existing instance via constructor/parameter), ensuring all usages in FeedbackTask reference that single PromptBuilder rather than creating a new one. ``` </details> </blockquote></details> <details> <summary>src/test/kotlin/zone/clanker/opsx/task/VerifyTaskTest.kt (1)</summary><blockquote> `21-26`: **Consider extracting repeated temp directory setup.** The same temp directory creation pattern is repeated 10 times: ```kotlin File.createTempFile("opsx-verify...", "").apply { delete() mkdirs() deleteOnExit() } ``` A shared helper function or Kotest `beforeEach` block would reduce duplication. <details> <summary>♻️ Suggested helper</summary> ```kotlin private fun createTempProjectDir(prefix: String): File = File.createTempFile(prefix, "").apply { delete() mkdirs() deleteOnExit() } ``` </details> Also applies to: 56-61, 86-91, 116-121, 140-145, 157-162, 176-181, 202-207, 228-233, 257-262 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/task/VerifyTaskTest.kt` around lines 21 - 26, Extract the repeated temp-dir creation into a single helper and replace all inline repetitions with it; add a private function like createTempProjectDir(prefix: String) used by the tests in VerifyTaskTest (or call it from a Kotest beforeEach if you prefer) to create the File, delete(), mkdirs(), and deleteOnExit(), then update all occurrences (the ten instances in VerifyTaskTest) to call createTempProjectDir("opsx-verify") instead of repeating the File.createTempFile(...).apply { ... } block so the temp-dir logic is centralized and DRY. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/task/ProposeTask.kt (1)</summary><blockquote> `90-90`: **Redundant `PromptBuilder` instantiation.** Same pattern as `OnboardTask` — a `PromptBuilder` is created on line 38 but a new one is instantiated here. <details> <summary>♻️ Suggested fix</summary> ```diff internal fun buildProposalPrompt( context: String, projectDesc: String, prompt: String?, changeDir: File, + promptBuilder: PromptBuilder, ): String { - val promptBuilder = PromptBuilder(project.rootDir) val relPath = changeDir.relativeTo(project.rootDir).path ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/ProposeTask.kt` at line 90, The second instantiation of PromptBuilder in ProposeTask.kt is redundant—remove the extra "val promptBuilder = PromptBuilder(...)" and reuse the existing promptBuilder declared earlier in the class/function (avoid shadowing); update any references to use that single promptBuilder instance and clean up any now-unused imports or variables. ``` </details> </blockquote></details> <details> <summary>src/main/kotlin/zone/clanker/opsx/workflow/TaskParser.kt (1)</summary><blockquote> `80-90`: **Clarify inline dependency handling logic.** Line 85's condition `inlineValue == "none" || inlineValue.isNotEmpty()` returns empty list for both `depends: none` and any non-empty inline value. This means inline single-ID syntax like `depends: a1b2c3d4e5` would be silently ignored. If only multi-line dependencies are intended to be supported, consider making this explicit: <details> <summary>♻️ Clearer condition</summary> ```diff private fun extractDependencies(body: List<String>): List<String> { val dependsLine = body.firstOrNull { it.trimStart().startsWith("depends:") } ?: return emptyList() val inlineValue = dependsLine.substringAfter("depends:").trim() - if (inlineValue == "none" || inlineValue.isNotEmpty()) return emptyList() + // Only "depends: none" or empty (multi-line format) are valid + if (inlineValue == "none") return emptyList() + if (inlineValue.isNotEmpty()) { + // Unexpected inline content; warn or handle gracefully + return emptyList() + } return body .dropWhile { !it.trimStart().startsWith("depends:") } .drop(1) .mapNotNull { DEPENDS_ID.matchEntire(it)?.groupValues?.get(1) } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/workflow/TaskParser.kt` around lines 80 - 90, In extractDependencies, the inline value check incorrectly treats any non-empty inline value as "no dependencies"; change the logic so that if inlineValue == "none" you return emptyList(), if inlineValue.isNotEmpty() you return listOf(inlineValue) (or split it into multiple IDs if you intend to support comma-separated inline IDs), and only when inlineValue is empty fall back to parsing the following multi-line block using DEPENDS_ID.matchEntire; update the conditional in extractDependencies accordingly and keep trimming behavior to ensure correct ID extraction. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `2ffea941-0d2c-478a-85dc-dd0f562560d9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5c3ce5ec8fd9a0403cc0b4e151e874ecb7d5c0c4 and 4f5f9e9b0331a7019eacee24fc8367167e1c5efd. </details> <details> <summary>📒 Files selected for processing (61)</summary> * `.github/copilot-instructions.md` * `AGENTS.md` * `CLAUDE.md` * `README.md` * `build.gradle.kts` * `opsx/changes/atomic-task-execution/.opsx.yaml` * `opsx/changes/atomic-task-execution/design.md` * `opsx/changes/atomic-task-execution/proposal.md` * `opsx/changes/atomic-task-execution/tasks.md` * `settings.gradle.kts` * `src/main/kotlin/zone/clanker/opsx/Opsx.kt` * `src/main/kotlin/zone/clanker/opsx/model/Change.kt` * `src/main/kotlin/zone/clanker/opsx/model/ChangeConfig.kt` * `src/main/kotlin/zone/clanker/opsx/model/ChangeStatus.kt` * `src/main/kotlin/zone/clanker/opsx/model/TaskDefinition.kt` * `src/main/kotlin/zone/clanker/opsx/model/TaskStatus.kt` * `src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt` * `src/main/kotlin/zone/clanker/opsx/task/ApplyTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/ArchiveTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/BulkArchiveTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/CleanTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/ContinueTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/ExploreTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/FeedbackTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/FfTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/ListTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/OnboardTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/ProposeTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/StatusTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/SyncTask.kt` * `src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt` * `src/main/kotlin/zone/clanker/opsx/workflow/AgentDispatcher.kt` * `src/main/kotlin/zone/clanker/opsx/workflow/ChangeLogger.kt` * `src/main/kotlin/zone/clanker/opsx/workflow/ChangeWriter.kt` * `src/main/kotlin/zone/clanker/opsx/workflow/PromptBuilder.kt` * `src/main/kotlin/zone/clanker/opsx/workflow/TaskParser.kt` * `src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt` * `src/test/kotlin/zone/clanker/opsx/model/ChangeConfigTest.kt` * `src/test/kotlin/zone/clanker/opsx/model/ChangeStatusTest.kt` * `src/test/kotlin/zone/clanker/opsx/model/TaskDefinitionTest.kt` * `src/test/kotlin/zone/clanker/opsx/model/TaskStatusTest.kt` * `src/test/kotlin/zone/clanker/opsx/skill/SkillGeneratorTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ApplyTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ArchiveTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/BulkArchiveTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/CleanTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ContinueTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ExploreTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/FeedbackTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/FfTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ListTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/OnboardTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/ProposeTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/StatusTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/SyncTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/task/VerifyTaskTest.kt` * `src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt` * `src/test/kotlin/zone/clanker/opsx/workflow/ChangeLoggerTest.kt` * `src/test/kotlin/zone/clanker/opsx/workflow/ChangeWriterTest.kt` * `src/test/kotlin/zone/clanker/opsx/workflow/PromptBuilderTest.kt` * `src/test/kotlin/zone/clanker/opsx/workflow/TaskParserTest.kt` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+22
to
+31
| runCatching { | ||
| if (!logFile.exists()) { | ||
| logFile.writeText("# Log\n\n") | ||
| } | ||
| val symbol = status.symbol | ||
| val line = "- [$symbol] $taskId | $taskName — $message\n" | ||
| logFile.appendText(line) | ||
| }.also { | ||
| lock.unlock() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/workflow/ChangeLogger.ktRepository: ClankerGuru/opsx
Length of output: 1275
🏁 Script executed:
rg "ChangeLogger.append|ChangeLogger\.append" --type kotlin -A 3Repository: ClankerGuru/opsx
Length of output: 2004
🏁 Script executed:
cat -n src/test/kotlin/zone/clanker/opsx/workflow/ChangeLoggerTest.ktRepository: ClankerGuru/opsx
Length of output: 3862
Propagate log write failures instead of swallowing them.
runCatching { ... } drops every filesystem error, so a task can report success even when log.md was never updated. This journal is part of workflow state; let exceptions propagate and keep the unlock in finally.
Suggested fix
- runCatching {
+ try {
if (!logFile.exists()) {
logFile.writeText("# Log\n\n")
}
val symbol = status.symbol
val line = "- [$symbol] $taskId | $taskName — $message\n"
logFile.appendText(line)
- }.also {
+ } finally {
lock.unlock()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/workflow/ChangeLogger.kt` around lines 22 -
31, The current runCatching in ChangeLogger around the log file writes swallows
IO exceptions; remove runCatching and replace with a normal try/finally so
filesystem errors propagate while still releasing the lock: perform the
existence check and write/append to logFile (writeText/appendText) inside try
and call lock.unlock() in finally, letting any thrown exception bubble up from
the ChangeLogger write path.
Owner
Author
There was a problem hiding this comment.
@coderabbitai 🔒 Lock scope documented. Single-process append is correct for Gradle's execution model.
There was a problem hiding this comment.
Rate Limit Exceeded
@ClankerGuru have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 54 seconds before sending another message.
Comment on lines
+88
to
+103
| given("dispatch with a simple echo command") { | ||
| `when`("agent process completes successfully") { | ||
| then("returns exit code 0 and creates log file") { | ||
| val result = | ||
| AgentDispatcher.dispatch( | ||
| agent = "claude", | ||
| prompt = "test prompt", | ||
| workDir = File(System.getProperty("user.dir")), | ||
| timeoutSeconds = 5L, | ||
| ) | ||
| // Agent not installed in CI — expect failure, but Result should be well-formed | ||
| result shouldNotBe null | ||
| result.exitCode shouldBeLessThan 1000 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify external-binary coupling and weak assertions in AgentDispatcher tests
rg -n -C2 'AgentDispatcher\.dispatch\(' src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt
rg -n -C2 'shouldBeLessThan 1000|Agent not installed in CI|very short timeout' src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.ktRepository: ClankerGuru/opsx
Length of output: 1585
🏁 Script executed:
#!/bin/bash
# Get full test file to understand complete test context
wc -l src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt
cat -n src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt | head -180Repository: ClankerGuru/opsx
Length of output: 8177
🏁 Script executed:
#!/bin/bash
# Find and examine AgentDispatcher implementation
fd -type f -name 'AgentDispatcher.kt' | grep -v TestRepository: ClankerGuru/opsx
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check for test infrastructure (mocking, test utilities)
rg -l 'mockk|@Mock|Mockito|WireMock|TestDouble' src/test/kotlin/zone/clanker/opsx/workflow/Repository: ClankerGuru/opsx
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find AgentDispatcher implementation
find . -name 'AgentDispatcher.kt' -not -path '*/test/*'Repository: ClankerGuru/opsx
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Also check for the implementation in common paths
fd -name 'AgentDispatcher.kt' --type fRepository: ClankerGuru/opsx
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Read AgentDispatcher implementation
wc -l ./src/main/kotlin/zone/clanker/opsx/workflow/AgentDispatcher.kt
cat -n ./src/main/kotlin/zone/clanker/opsx/workflow/AgentDispatcher.ktRepository: ClankerGuru/opsx
Length of output: 5415
Dispatch tests are non-deterministic and don't validate the intended branches.
They call the real claude binary without mocking, making test success depend on CI environment. The assertions are too broad: shouldBeLessThan 1000 passes for any failure code (-1, 127, etc.), and test titles claim outcomes (e.g., "returns exit code 0 and creates log file") that the assertions don't verify. The timeout branch (should return exitCode -1) is not distinguished from other failures, and the success branch isn't validated despite its test name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt` around
lines 88 - 103, The tests call the real external "claude" binary making them
flaky and the assertions are too loose; change AgentDispatcherTest to mock or
inject the process execution used by AgentDispatcher.dispatch (e.g., stub the
process runner or provide a TestProcessLauncher) so you control outcomes: add
one test that returns exitCode 0 and assert exitCode == 0 and that the expected
log file was created, another that simulates a timeout and assert exitCode ==
-1, and another that simulates "not found" (e.g., exit 127) and assert that
specific exit code; update or add the injection point used by
AgentDispatcher.dispatch so tests don't invoke the real binary.
Owner
Author
There was a problem hiding this comment.
@coderabbitai 🐇 Test renamed and assertion tightened — exitCode shouldNotBe 0 instead of weak range check.
There was a problem hiding this comment.
Rate Limit Exceeded
@ClankerGuru have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 51 seconds before sending another message.
Owner
Author
There was a problem hiding this comment.
@coderabbitai 🐰 Fixed! Renamed test and tightened assertion to shouldNotBe 0 + logFile shouldNotBe null.
(\(\ "Weak assertions?
( -.-) Not on my watch!"
o_(")(")
There was a problem hiding this comment.
@ClankerGuru, appreciate the tightening — shouldNotBe 0 and logFile shouldNotBe null are definitely stronger than the original shouldBeLessThan 1000.
That said, the primary concern from the review still stands: the dispatch test still invokes the real claude binary, so outcomes remain environment-dependent and the timeout/success branches can't be reliably distinguished. The assertion improvements help catch more failures, but they don't make the test deterministic.
To fully resolve this, the recommendation was to add an injection point (e.g., a processLauncher lambda or interface on AgentDispatcher) so tests can control the ProcessBuilder outcome without spawning real binaries — enabling distinct assertions for exit-0, exit-127, and timeout (-1) scenarios. Would you like me to sketch that approach?
- .clkx/skills/ gets full ignore (entirely opsx-owned) - Agent target dirs (.claude/commands/, etc.) get opsx-* pattern only - Appends to existing .gitignore without clobbering user rules - Does not duplicate pattern on re-sync
Security: path traversal validation in ChangeWriter and PromptBuilder State machine: fail-fast on unknown status, use canTransitionTo, move IN_PROGRESS write after successful dispatch Config safety: remove autoSyncOnFirstRun, protect user files in symlinks Tests: fix task count, deadlock guard, rename misleading assertions Add 5 Konsist architecture tests (slopTest): PackageBoundary, NamingConvention, TaskAnnotation, ForbiddenPackage, ForbiddenPattern Update README with Maven Central badge, dependencies, architecture, test suites, project structure, and build commands
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/slopTest/kotlin/zone/clanker/opsx/ForbiddenPatternTest.kt (1)
23-27: Consider AST-based detection for try-catch blocks.The string-based check (
text.contains("try {")) may produce false positives iftry {appears in string literals, comments, or KDoc examples. Konsist doesn't expose try-catch as a first-class construct, so this heuristic is reasonable, but be aware of potential noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/slopTest/kotlin/zone/clanker/opsx/ForbiddenPatternTest.kt` around lines 23 - 27, The current test in ForbiddenPatternTest uses a brittle string check on mainScope.functions().assertTrue to detect try-catch via text.contains("try {"/"try{"), which can false-positive on literals/comments; instead traverse the Kotlin AST/PSI for each function and assert that no KtTryExpression nodes exist in the function body (e.g., iterate functions() and fail if any function contains a KtTryExpression), replacing the text-based check in the test with an AST-based existence check using the language's PSI/Kotlin parser utilities.src/main/kotlin/zone/clanker/opsx/workflow/ChangeWriter.kt (1)
73-83: Consider adding a newline at the end of newly created feedback files.When
appendFeedbackcreates a new file, it writescontentdirectly without a trailing newline. Subsequent appends prepend\n, but the file may not end with a newline, which can cause issues with some tools.♻️ Suggested fix
fun appendFeedback( changeDir: File, content: String, ) { val file = File(changeDir, "feedback.md") if (file.exists()) { file.appendText("\n$content") } else { - file.writeText(content) + file.writeText("$content\n") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/workflow/ChangeWriter.kt` around lines 73 - 83, The appendFeedback function currently writes content without a trailing newline when creating feedback.md, causing inconsistent file endings; update appendFeedback (function name appendFeedback and the File handling for "feedback.md") so that the "new file" branch writes the content with a trailing newline (e.g., write content + "\n") to match the append behavior and ensure the file ends with a newline.src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt (2)
303-329: System property modification may cause test pollution.Modifying
System.setProperty("user.home", ...)without a try-finally block risks leaving the system in a corrupted state if an assertion or operation fails before the restore on line 329. Consider using a try-finally pattern or Kotest'sSystemPropertiesListener/afterTesthooks to ensure cleanup.♻️ Suggested improvement using try-finally
val origHome = System.getProperty("user.home") - System.setProperty("user.home", tempDir.absolutePath) - - val clkxSkills = File(tempDir, ".clkx/skills") - ... - task.run() - - System.setProperty("user.home", origHome) + try { + System.setProperty("user.home", tempDir.absolutePath) + + val clkxSkills = File(tempDir, ".clkx/skills") + ... + task.run() + } finally { + System.setProperty("user.home", origHome) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt` around lines 303 - 329, The test sets System.setProperty("user.home", tempDir.absolutePath) and restores it later but lacks a fail-safe, so wrap the portion that changes and uses the property (setting user.home, creating files, building the Project, creating/running Opsx.CleanTask via task.run()) in a try-finally (or use a Kotest SystemPropertiesListener/afterTest) to ensure System.setProperty("user.home", origHome) is executed even if an assertion or exception occurs; reference the variables origHome, tempDir and the Opsx.CleanTask invocation (task.run()) when moving code into the protected block.
152-155: Prefer idiomatic Kotest matchers for readability.The assertion
(desc != null && desc.isNotBlank()) shouldBe truecould be more expressive using Kotest's built-in matchers.♻️ Suggested improvement
allTasks.forEach { taskName -> val desc = project.tasks.findByName(taskName)!!.description - (desc != null && desc.isNotBlank()) shouldBe true + desc.shouldNotBeNull() + desc.shouldNotBeBlank() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt` around lines 152 - 155, Replace the manual boolean assertion with idiomatic Kotest string null/blank matchers: for each task in allTasks (in OpsxPluginTest) get the description via project.tasks.findByName(taskName)!!.description and assert using desc.shouldNotBeNull() followed by desc.shouldNotBeBlank() (or chain non-null assertion to call shouldNotBeBlank) instead of `(desc != null && desc.isNotBlank()) shouldBe true` to improve readability and intent.src/main/kotlin/zone/clanker/opsx/task/ApplyTask.kt (1)
68-90: RedundantPromptBuilderinstantiation.A
PromptBuilderis already created inrun()at line 34, butbuildApplyPrompt()creates a new instance at line 73. Consider passing the existing instance or makingbuildApplyPromptuse the one fromrun().♻️ Suggested fix
internal fun buildApplyPrompt( context: String, changeCtx: String, changeDir: File, + promptBuilder: PromptBuilder, ): String { - val promptBuilder = PromptBuilder(project.rootDir) val relPath = changeDir.relativeTo(project.rootDir).path return promptBuilder.build(And update the call site:
- val fullPrompt = buildApplyPrompt(context, changeCtx, change.dir) + val fullPrompt = buildApplyPrompt(context, changeCtx, change.dir, promptBuilder)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/task/ApplyTask.kt` around lines 68 - 90, buildApplyPrompt currently creates a new PromptBuilder instance (promptBuilder) even though run() already constructs one; modify buildApplyPrompt to accept an existing PromptBuilder (or access a shared instance) instead of instantiating its own, and update the run() call site to pass the PromptBuilder created there; ensure you keep usage of project.rootDir and the same parameters (context, changeCtx, changeDir) and remove the redundant "val promptBuilder = PromptBuilder(project.rootDir)" from buildApplyPrompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 73-75: Update the two fenced code blocks that contain the status
transition diagram ("draft -> active -> in-progress -> completed/done ->
verified -> archived") and the dependency direction line ("task -> model
(tasks consume models)") to include a language specifier (e.g., use ```text) at
the start of each block so markdownlint no longer flags them; locate the fenced
blocks in README.md that exactly contain those arrow sequences and prepend the
language token to the opening ``` fence for each.
In `@src/main/kotlin/zone/clanker/opsx/model/ChangeConfig.kt`:
- Around line 57-71: The write() method in ChangeConfig currently interpolates
config.name, config.status, config.verify and config.depends directly which can
produce invalid YAML when values contain special characters; update write() to
produce safe YAML by either (preferred) serializing the ChangeConfig object with
a YAML library (e.g., SnakeYAML or Jackson YAML) or (if you must keep manual
serialization) implement a helper that escapes and wraps scalar values in quotes
(handling embedded quotes/backslashes/newlines) and use it for config.name and
config.status, and apply it to each element when joining config.verify and
config.depends before calling file.writeText; ensure the output format remains
the same keys ("name", "status", "verify", "depends") but uses safely
quoted/escaped values.
In `@src/main/kotlin/zone/clanker/opsx/Opsx.kt`:
- Around line 210-225: The cleanup loop in Opsx.kt currently deletes every
symlink found under each SkillGenerator.AGENT_TARGETS directory; change it to
only remove opsx-generated symlinks by filtering links first — either by
matching the filename to the opsx pattern (e.g.
link.name.matches(Regex("^opsx-.*\\.md$"))) or by resolving the symlink target
with java.nio.file.Files.readSymbolicLink(link.toPath()) and confirming the
target path points into your opsx skills directory before calling link.delete();
do this check inside the same forEach (use try/catch around readSymbolicLink to
avoid throwing) and only set cleaned = true and log via logger.lifecycle when
the filter passes.
---
Nitpick comments:
In `@src/main/kotlin/zone/clanker/opsx/task/ApplyTask.kt`:
- Around line 68-90: buildApplyPrompt currently creates a new PromptBuilder
instance (promptBuilder) even though run() already constructs one; modify
buildApplyPrompt to accept an existing PromptBuilder (or access a shared
instance) instead of instantiating its own, and update the run() call site to
pass the PromptBuilder created there; ensure you keep usage of project.rootDir
and the same parameters (context, changeCtx, changeDir) and remove the redundant
"val promptBuilder = PromptBuilder(project.rootDir)" from buildApplyPrompt.
In `@src/main/kotlin/zone/clanker/opsx/workflow/ChangeWriter.kt`:
- Around line 73-83: The appendFeedback function currently writes content
without a trailing newline when creating feedback.md, causing inconsistent file
endings; update appendFeedback (function name appendFeedback and the File
handling for "feedback.md") so that the "new file" branch writes the content
with a trailing newline (e.g., write content + "\n") to match the append
behavior and ensure the file ends with a newline.
In `@src/slopTest/kotlin/zone/clanker/opsx/ForbiddenPatternTest.kt`:
- Around line 23-27: The current test in ForbiddenPatternTest uses a brittle
string check on mainScope.functions().assertTrue to detect try-catch via
text.contains("try {"/"try{"), which can false-positive on literals/comments;
instead traverse the Kotlin AST/PSI for each function and assert that no
KtTryExpression nodes exist in the function body (e.g., iterate functions() and
fail if any function contains a KtTryExpression), replacing the text-based check
in the test with an AST-based existence check using the language's PSI/Kotlin
parser utilities.
In `@src/test/kotlin/zone/clanker/opsx/OpsxPluginTest.kt`:
- Around line 303-329: The test sets System.setProperty("user.home",
tempDir.absolutePath) and restores it later but lacks a fail-safe, so wrap the
portion that changes and uses the property (setting user.home, creating files,
building the Project, creating/running Opsx.CleanTask via task.run()) in a
try-finally (or use a Kotest SystemPropertiesListener/afterTest) to ensure
System.setProperty("user.home", origHome) is executed even if an assertion or
exception occurs; reference the variables origHome, tempDir and the
Opsx.CleanTask invocation (task.run()) when moving code into the protected
block.
- Around line 152-155: Replace the manual boolean assertion with idiomatic
Kotest string null/blank matchers: for each task in allTasks (in OpsxPluginTest)
get the description via project.tasks.findByName(taskName)!!.description and
assert using desc.shouldNotBeNull() followed by desc.shouldNotBeBlank() (or
chain non-null assertion to call shouldNotBeBlank) instead of `(desc != null &&
desc.isNotBlank()) shouldBe true` to improve readability and intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e098a394-af20-4e88-b2be-bd2f7b8d7a13
📒 Files selected for processing (23)
README.mdsrc/main/kotlin/zone/clanker/opsx/Opsx.ktsrc/main/kotlin/zone/clanker/opsx/model/ChangeConfig.ktsrc/main/kotlin/zone/clanker/opsx/model/ChangeStatus.ktsrc/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.ktsrc/main/kotlin/zone/clanker/opsx/task/ApplyTask.ktsrc/main/kotlin/zone/clanker/opsx/task/ArchiveTask.ktsrc/main/kotlin/zone/clanker/opsx/task/VerifyTask.ktsrc/main/kotlin/zone/clanker/opsx/workflow/ChangeLogger.ktsrc/main/kotlin/zone/clanker/opsx/workflow/ChangeWriter.ktsrc/main/kotlin/zone/clanker/opsx/workflow/PromptBuilder.ktsrc/slopTest/kotlin/zone/clanker/opsx/ForbiddenPackageTest.ktsrc/slopTest/kotlin/zone/clanker/opsx/ForbiddenPatternTest.ktsrc/slopTest/kotlin/zone/clanker/opsx/NamingConventionTest.ktsrc/slopTest/kotlin/zone/clanker/opsx/PackageBoundaryTest.ktsrc/slopTest/kotlin/zone/clanker/opsx/TaskAnnotationTest.ktsrc/test/kotlin/zone/clanker/opsx/OpsxPluginTest.ktsrc/test/kotlin/zone/clanker/opsx/model/ChangeConfigTest.ktsrc/test/kotlin/zone/clanker/opsx/model/ChangeStatusTest.ktsrc/test/kotlin/zone/clanker/opsx/task/ArchiveTaskTest.ktsrc/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.ktsrc/test/kotlin/zone/clanker/opsx/workflow/ChangeLoggerTest.ktsrc/test/kotlin/zone/clanker/opsx/workflow/ChangeWriterTest.kt
✅ Files skipped from review due to trivial changes (5)
- src/test/kotlin/zone/clanker/opsx/model/ChangeStatusTest.kt
- src/main/kotlin/zone/clanker/opsx/model/ChangeStatus.kt
- src/test/kotlin/zone/clanker/opsx/workflow/ChangeWriterTest.kt
- src/test/kotlin/zone/clanker/opsx/task/ArchiveTaskTest.kt
- src/test/kotlin/zone/clanker/opsx/workflow/AgentDispatcherTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/kotlin/zone/clanker/opsx/workflow/ChangeLogger.kt
- src/test/kotlin/zone/clanker/opsx/workflow/ChangeLoggerTest.kt
- src/main/kotlin/zone/clanker/opsx/task/VerifyTask.kt
- src/test/kotlin/zone/clanker/opsx/model/ChangeConfigTest.kt
Comment on lines
+73
to
+75
| ``` | ||
| draft -> active -> in-progress -> completed/done -> verified -> archived | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code blocks.
The status transition diagram and dependency direction blocks lack language specifiers, which triggers markdownlint warnings.
Suggested fix
-```
+```text
draft -> active -> in-progress -> completed/done -> verified -> archived
Similarly for lines 107-113:
```diff
-```
+```text
task -> model (tasks consume models)
📝 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.
Suggested change
| ``` | |
| draft -> active -> in-progress -> completed/done -> verified -> archived | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 73 - 75, Update the two fenced code blocks that
contain the status transition diagram ("draft -> active -> in-progress ->
completed/done -> verified -> archived") and the dependency direction line
("task -> model (tasks consume models)") to include a language specifier
(e.g., use ```text) at the start of each block so markdownlint no longer flags
them; locate the fenced blocks in README.md that exactly contain those arrow
sequences and prepend the language token to the opening ``` fence for each.
Comment on lines
+57
to
+71
| fun write( | ||
| file: File, | ||
| config: ChangeConfig, | ||
| ) { | ||
| val sb = StringBuilder() | ||
| sb.appendLine("name: ${config.name}") | ||
| sb.appendLine("status: ${config.status}") | ||
| if (config.verify.isNotEmpty()) { | ||
| sb.appendLine("verify: ${config.verify}") | ||
| } | ||
| if (config.depends.isNotEmpty()) { | ||
| sb.appendLine("depends: [${config.depends.joinToString(", ")}]") | ||
| } | ||
| file.writeText(sb.toString()) | ||
| } |
There was a problem hiding this comment.
Values containing YAML special characters may produce invalid output.
The write() method directly interpolates config.name, config.status, config.verify, and config.depends values without escaping. If any value contains :, #, or starts with special YAML characters, the resulting file may be unparseable or parsed incorrectly.
This is a low-risk issue since change names are typically simple identifiers, but worth noting for robustness.
🛡️ Example problematic case
If config.name = "add: feature", the output would be:
name: add: featurewhich YAML parsers may interpret differently.
Consider quoting values or using a proper YAML library for serialization if arbitrary input is expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/model/ChangeConfig.kt` around lines 57 -
71, The write() method in ChangeConfig currently interpolates config.name,
config.status, config.verify and config.depends directly which can produce
invalid YAML when values contain special characters; update write() to produce
safe YAML by either (preferred) serializing the ChangeConfig object with a YAML
library (e.g., SnakeYAML or Jackson YAML) or (if you must keep manual
serialization) implement a helper that escapes and wraps scalar values in quotes
(handling embedded quotes/backslashes/newlines) and use it for config.name and
config.status, and apply it to each element when joining config.verify and
config.depends before calling file.writeText; ensure the output format remains
the same keys ("name", "status", "verify", "depends") but uses safely
quoted/escaped values.
CleanTask was deleting ALL symlinks in agent dirs. Now only removes symlinks pointing into our .clkx/skills/ directory. Also bump srcx dependency to 0.44.0 (PSI thread-safety fix).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/kotlin/zone/clanker/opsx/Opsx.kt (1)
145-163: Consolidate infrastructure task definitions to avoid drift.At Line 145-163, registration points to
zone.clanker.opsx.task.*, but this file still keeps nestedOpsx.SyncTask,Opsx.CleanTask,Opsx.StatusTask, andOpsx.ListTaskimplementations. Keeping both implementations increases divergence risk; prefer a single source of truth (remove nested copies or delegate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/kotlin/zone/clanker/opsx/Opsx.kt` around lines 145 - 163, The task registrations in Opsx (registering TASK_STATUS, TASK_SYNC, TASK_CLEAN, TASK_LIST using zone.clanker.opsx.task.* classes) conflict with nested task implementations still present in the Opsx file (Opsx.SyncTask, Opsx.CleanTask, Opsx.StatusTask, Opsx.ListTask), creating duplicate sources of truth; remove the nested task class implementations from Opsx.kt so the project uses the single canonical implementations in zone.clanker.opsx.task (or alternatively update the registrations to point to the nested classes consistently), and ensure all symbols referenced in the registrations (TASK_STATUS, TASK_SYNC, TASK_CLEAN, TASK_LIST and the extension usage) remain valid after the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/kotlin/zone/clanker/opsx/Opsx.kt`:
- Around line 145-163: The task registrations in Opsx (registering TASK_STATUS,
TASK_SYNC, TASK_CLEAN, TASK_LIST using zone.clanker.opsx.task.* classes)
conflict with nested task implementations still present in the Opsx file
(Opsx.SyncTask, Opsx.CleanTask, Opsx.StatusTask, Opsx.ListTask), creating
duplicate sources of truth; remove the nested task class implementations from
Opsx.kt so the project uses the single canonical implementations in
zone.clanker.opsx.task (or alternatively update the registrations to point to
the nested classes consistently), and ensure all symbols referenced in the
registrations (TASK_STATUS, TASK_SYNC, TASK_CLEAN, TASK_LIST and the extension
usage) remain valid after the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29b56abe-7d60-4726-bc33-983de3b2ed68
📒 Files selected for processing (2)
build.gradle.ktssrc/main/kotlin/zone/clanker/opsx/Opsx.kt
✅ Files skipped from review due to trivial changes (1)
- build.gradle.kts
…iscovery Claude Code reads slash commands from <project>/.claude/commands/, not ~/.claude/commands/. Now symlinks to both locations so commands work in the CLI and are available globally.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt`:
- Around line 31-32: The generated skill sources directory is currently global
(~/.clkx/skills) and must be namespaced per workspace; update the code that
creates sourceDir in SkillGenerator.kt to include a stable workspace identifier
(e.g., rootProject.rootDir or its hash) so sourceDir = File(homeDir(),
SKILLS_DIR, workspaceId) and adjust generatedDirs() to return paths under that
same workspace-scoped directory; also apply the same change where sourceDir is
created/used at the other occurrences referenced (lines ~127-132 and ~243-247)
and ensure buildCommandFile() continues to write into the workspace-scoped
directory so each workspace has its own isolated generated files.
- Around line 34-37: Before writing the current tasks, compute the set of
expected filenames (tasks.map { "${it.name}.md" }) and remove any OPSX-managed
files and symlinks not in that set: in the block that writes files (where
sourceDir, tasks, builds and buildCommandFile are used) first list sourceDir
files and delete any matching the managed filename pattern (e.g., "*.md" or your
OPSX marker) that are not in expectedNames; then for each target directory where
you create symlinks (the same code area covered by lines 47-70), list entries
and remove symlinks whose names are not in expectedNames and/or whose target
points into sourceDir (to avoid touching non-OPSX files). After pruning, proceed
to recreate the current markdown files and symlinks as before.
- Around line 191-193: The task description is inserted raw into a Markdown
table and can contain pipe characters or newlines which break table formatting;
in SkillGenerator.kt locate the code that builds rows (the block using
TASK_USAGE, the local variables usage/example and the appendLine call for
"/${task.name}") and normalize/escape the description before appending by
replacing pipe characters and newlines (e.g., convert '|' and '\n'/'\r' to a
safe character or escape sequence like space or ' ' or '\|'), then use that
sanitized string in the appendLine call so generated
CLAUDE.md/AGENTS.md/.github/copilot-instructions.md tables remain well-formed.
- Around line 49-51: The symlink creation currently uses "${task.name}.md" for
all targets in the tasks.forEach block; change it to use
"${task.name}.prompt.md" when the destination is the GitHub prompts directory by
detecting the targetDir (e.g., check if
targetDir.toString().endsWith(".github/prompts") or
targetDir.fileName.toString() == "prompts" under a ".github" parent) and choose
filename = if (isGithubPrompts) "${task.name}.prompt.md" else "${task.name}.md";
update the use of sourceDir, targetDir and task.name in that block so
`.github/prompts` receives the .prompt.md extension while other targets remain
.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ec6d7c-38a0-41b4-8f21-b6a1ee0df655
📒 Files selected for processing (1)
src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt
Comment on lines
+31
to
+32
| val sourceDir = File(homeDir(), SKILLS_DIR) | ||
| sourceDir.mkdirs() |
There was a problem hiding this comment.
Keep the generated skill source directory workspace-scoped.
buildCommandFile() writes workspace-specific content such as included builds, but sourceDir and generatedDirs() are now global under ~/.clkx/skills. Syncing workspace B will overwrite the backing files that workspace A's repo-local symlinks still point at, so workspace A can start showing workspace B's command content. Please namespace the source directory by rootProject.rootDir (or another stable workspace id) and keep generatedDirs() aligned with that path.
Also applies to: 127-132, 243-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt` around lines 31 -
32, The generated skill sources directory is currently global (~/.clkx/skills)
and must be namespaced per workspace; update the code that creates sourceDir in
SkillGenerator.kt to include a stable workspace identifier (e.g.,
rootProject.rootDir or its hash) so sourceDir = File(homeDir(), SKILLS_DIR,
workspaceId) and adjust generatedDirs() to return paths under that same
workspace-scoped directory; also apply the same change where sourceDir is
created/used at the other occurrences referenced (lines ~127-132 and ~243-247)
and ensure buildCommandFile() continues to write into the workspace-scoped
directory so each workspace has its own isolated generated files.
Comment on lines
+34
to
37
| // Write all skills to the single source directory | ||
| tasks.forEach { task -> | ||
| File(dir, "${task.name}.md").writeText(buildCommandFile(task, builds)) | ||
| File(sourceDir, "${task.name}.md").writeText(buildCommandFile(task, builds)) | ||
| } |
There was a problem hiding this comment.
Prune stale OPSX-managed command files during sync.
This only rewrites the current tasks set. If a task is renamed or removed, its old markdown file remains in the source directory and its old OPSX-managed symlinks remain in each target directory, so users keep seeing commands that no longer exist after opsx-sync. Delete managed files and symlinks whose filenames are no longer present before recreating the current set.
Also applies to: 47-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt` around lines 34 -
37, Before writing the current tasks, compute the set of expected filenames
(tasks.map { "${it.name}.md" }) and remove any OPSX-managed files and symlinks
not in that set: in the block that writes files (where sourceDir, tasks, builds
and buildCommandFile are used) first list sourceDir files and delete any
matching the managed filename pattern (e.g., "*.md" or your OPSX marker) that
are not in expectedNames; then for each target directory where you create
symlinks (the same code area covered by lines 47-70), list entries and remove
symlinks whose names are not in expectedNames and/or whose target points into
sourceDir (to avoid touching non-OPSX files). After pruning, proceed to recreate
the current markdown files and symlinks as before.
Comment on lines
+49
to
+51
| tasks.forEach { task -> | ||
| val source = File(sourceDir, "${task.name}.md").toPath() | ||
| val link = File(targetDir, "${task.name}.md").toPath() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Docs: what filename extension is required for GitHub Copilot prompt files stored in .github/prompts?
💡 Result:
The filename extension required for GitHub Copilot prompt files stored in .github/prompts is .prompt.md. These are Markdown files used for reusable prompts in VS Code, Visual Studio, and JetBrains IDEs.
Citations:
- 1: https://code.visualstudio.com/docs/copilot/customization/prompt-files
- 2: https://github.com/microsoft/vscode-docs/blob/36ba054f/docs/copilot/customization/prompt-files.md
- 3: https://docs.github.com/en/copilot/customizing-copilot/about-customizing-github-copilot-chat-responses
- 4: https://docs.github.com/en/copilot/tutorials/customization-library/prompt-files/your-first-prompt-file
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | head -60Repository: ClankerGuru/opsx
Length of output: 2778
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | sed -n '40,70p'Repository: ClankerGuru/opsx
Length of output: 1763
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | sed -n '220,235p'Repository: ClankerGuru/opsx
Length of output: 563
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | sed -n '75,130p'Repository: ClankerGuru/opsx
Length of output: 2491
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | sed -n '140,200p'Repository: ClankerGuru/opsx
Length of output: 2867
🏁 Script executed:
cat -n src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt | sed -n '135,151p'Repository: ClankerGuru/opsx
Length of output: 803
Change the filename pattern to .prompt.md for .github/prompts target directory.
When symlinking files into .github/prompts (line 226), the filenames must use .prompt.md extension to be discovered by GitHub Copilot. Currently, the code uses ${task.name}.md for all targets (lines 50–51), which prevents Copilot from finding the prompts in that location.
Modify the symlink creation logic to conditionally use .prompt.md when targetDir is .github/prompts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt` around lines 49 -
51, The symlink creation currently uses "${task.name}.md" for all targets in the
tasks.forEach block; change it to use "${task.name}.prompt.md" when the
destination is the GitHub prompts directory by detecting the targetDir (e.g.,
check if targetDir.toString().endsWith(".github/prompts") or
targetDir.fileName.toString() == "prompts" under a ".github" parent) and choose
filename = if (isGithubPrompts) "${task.name}.prompt.md" else "${task.name}.md";
update the use of sourceDir, targetDir and task.name in that block so
`.github/prompts` receives the .prompt.md extension while other targets remain
.md.
Comment on lines
+191
to
+193
| val usage = TASK_USAGE[task.name] | ||
| val example = if (usage != null) "`${usage.example}`" else "`./gradlew -q ${task.name}`" | ||
| appendLine("| `/${task.name}` | $example | ${task.description} |") |
There was a problem hiding this comment.
Escape task descriptions before inserting them into the Markdown table.
A task.description containing | or a newline will split the row and corrupt the generated tables in CLAUDE.md, AGENTS.md, and .github/copilot-instructions.md. Normalize the cell text before appending it.
🛠️ Proposed hardening
groupTasks.forEach { task ->
+ val description =
+ task.description
+ .replace("\n", " ")
+ .replace("|", "\\|")
val usage = TASK_USAGE[task.name]
val example = if (usage != null) "`${usage.example}`" else "`./gradlew -q ${task.name}`"
- appendLine("| `/${task.name}` | $example | ${task.description} |")
+ appendLine("| `/${task.name}` | $example | $description |")
}📝 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.
Suggested change
| val usage = TASK_USAGE[task.name] | |
| val example = if (usage != null) "`${usage.example}`" else "`./gradlew -q ${task.name}`" | |
| appendLine("| `/${task.name}` | $example | ${task.description} |") | |
| val description = | |
| task.description | |
| .replace("\n", " ") | |
| .replace("|", "\\|") | |
| val usage = TASK_USAGE[task.name] | |
| val example = if (usage != null) "`${usage.example}`" else "`./gradlew -q ${task.name}`" | |
| appendLine("| `/${task.name}` | $example | $description |") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/kotlin/zone/clanker/opsx/skill/SkillGenerator.kt` around lines 191 -
193, The task description is inserted raw into a Markdown table and can contain
pipe characters or newlines which break table formatting; in SkillGenerator.kt
locate the code that builds rows (the block using TASK_USAGE, the local
variables usage/example and the appendLine call for "/${task.name}") and
normalize/escape the description before appending by replacing pipe characters
and newlines (e.g., convert '|' and '\n'/'\r' to a safe character or escape
sequence like space or ' ' or '\|'), then use that sanitized string in the
appendLine call so generated CLAUDE.md/AGENTS.md/.github/copilot-instructions.md
tables remain well-formed.
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
./gradlew buildpassesSummary by CodeRabbit
New Features
Enhancements
Documentation