Harden CLI wrappers: logging, safety, robustness, architecture tests#2
Merged
Conversation
- Replace println/System.err with Gradle logger across all modules - Replace try-catch with runCatching in Cli.exec - Fix README property names (mcpName, pluginName) - Warn on dangerous flags (dangerouslySkipPermissions, dangerouslyBypass, yolo) - Fix Cli.exec deadlock: read stdout/stderr concurrently with waitFor - Add Cli.which() pre-check: fail fast with helpful message if binary missing - Extract addFlag helper to exec module CommandBuilder.kt - Add timeoutSeconds to all agent extensions, wired through execAndPrint - Add ForbiddenPatternTest (konsist slopTest) to all 5 modules - Add timeout expiration test for Cli.exec
📝 WalkthroughWalkthroughThis PR standardizes CLI execution across agent modules by adding configurable timeouts to task extensions, refactoring the shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (3)
opencode/src/slopTest/kotlin/zone/clanker/agents/opencode/ForbiddenPatternTest.kt (1)
13-27: Strengthen forbidden-call detection to avoid trivial bypasses.
contains("println(")misses variants likeprintln (. Using regex keeps the guardrail effective without changing test intent.🔧 Suggested hardening
class ForbiddenPatternTest : BehaviorSpec({ given("main source set") { val scope = Konsist.scopeFromProduction(moduleName = "opencode") + val printlnPattern = Regex("""\bprintln\s*\(""") + val systemErrPattern = Regex("""\bSystem\.err\b""") + val systemOutPattern = Regex("""\bSystem\.out\b""") then("no println calls") { scope.files.assertFalse { - it.text.contains("println(") + printlnPattern.containsMatchIn(it.text) } } then("no System.err usage") { scope.files.assertFalse { - it.text.contains("System.err") + systemErrPattern.containsMatchIn(it.text) } } then("no System.out usage") { scope.files.assertFalse { - it.text.contains("System.out") + systemOutPattern.containsMatchIn(it.text) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@opencode/src/slopTest/kotlin/zone/clanker/agents/opencode/ForbiddenPatternTest.kt` around lines 13 - 27, The test currently uses fragile substring checks (it.text.contains("println("), "System.err", "System.out") that can be bypassed by spacing; update the assertions in ForbiddenPatternTest.kt (the scope.files.assertFalse { it.text... } lambdas) to use regex-based matching instead — e.g. replace the println check with a word-boundary/whitespace-aware regex like \bprintln\s*\( and replace System.err/System.out checks with regexes that allow optional whitespace around the dot (e.g. System\s*\.\s*(err|out)) so the assertions use containsMatch/Regex matching on it.text rather than plain contains.exec/src/slopTest/kotlin/zone/clanker/agents/exec/ForbiddenPatternTest.kt (1)
12-28: Text-based pattern matching may produce false positives.The substring checks like
it.text.contains("println(")will match occurrences in comments, string literals, or identifiers (e.g.,myPrintln(). Consider using Konsist's AST-aware APIs or regex with word boundaries for more precise detection.That said, for a "slop test" intended to catch obvious anti-patterns, the current approach is pragmatic and likely sufficient for your use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exec/src/slopTest/kotlin/zone/clanker/agents/exec/ForbiddenPatternTest.kt` around lines 12 - 28, The current tests in ForbiddenPatternTest.kt use naive substring checks (scope.files.assertFalse { it.text.contains("println(") }, it.text.contains("System.err"), it.text.contains("System.out")) which can yield false positives from comments, strings, or identifiers; update these assertions to use a more precise matcher such as Konsist's AST-aware APIs to find call expressions or field accesses (e.g., search for call expressions named "println" or property accesses "System.out"/"System.err"), or at minimum switch to regex with word boundaries (e.g., \bprintln\b and \bSystem\.out\b) to avoid matching inside other tokens; modify the assertions around scope.files to iterate parsed AST nodes or apply the regex so the test only flags real usage rather than substrings.exec/src/main/kotlin/zone/clanker/agents/exec/Cli.kt (1)
91-92: Consider usinglogger.lifecycle()instead ofprint()for stdout.Line 91 uses
print(result.stdout)while line 92 useslogger.warn(result.stderr). For consistency with the PR's goal of replacing print statements with Gradle logger calls, consider usinglogger.lifecycle(result.stdout)for stdout output as well.♻️ Suggested change
- print(result.stdout) - if (result.stderr.isNotEmpty()) logger.warn(result.stderr) + if (result.stdout.isNotEmpty()) logger.lifecycle(result.stdout) + if (result.stderr.isNotEmpty()) logger.warn(result.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@exec/src/main/kotlin/zone/clanker/agents/exec/Cli.kt` around lines 91 - 92, Replace the direct stdout print call with the Gradle logger lifecycle method for consistency: instead of print(result.stdout) in Cli.kt, call logger.lifecycle(result.stdout) (optionally guarded by result.stdout.isNotEmpty()) and keep the existing logger.warn(result.stderr) for stderr; update the code around the result variable where stdout/stderr are handled to use logger.lifecycle for standard output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@exec/src/main/kotlin/zone/clanker/agents/exec/Cli.kt`:
- Around line 91-92: Replace the direct stdout print call with the Gradle logger
lifecycle method for consistency: instead of print(result.stdout) in Cli.kt,
call logger.lifecycle(result.stdout) (optionally guarded by
result.stdout.isNotEmpty()) and keep the existing logger.warn(result.stderr) for
stderr; update the code around the result variable where stdout/stderr are
handled to use logger.lifecycle for standard output.
In `@exec/src/slopTest/kotlin/zone/clanker/agents/exec/ForbiddenPatternTest.kt`:
- Around line 12-28: The current tests in ForbiddenPatternTest.kt use naive
substring checks (scope.files.assertFalse { it.text.contains("println(") },
it.text.contains("System.err"), it.text.contains("System.out")) which can yield
false positives from comments, strings, or identifiers; update these assertions
to use a more precise matcher such as Konsist's AST-aware APIs to find call
expressions or field accesses (e.g., search for call expressions named "println"
or property accesses "System.out"/"System.err"), or at minimum switch to regex
with word boundaries (e.g., \bprintln\b and \bSystem\.out\b) to avoid matching
inside other tokens; modify the assertions around scope.files to iterate parsed
AST nodes or apply the regex so the test only flags real usage rather than
substrings.
In
`@opencode/src/slopTest/kotlin/zone/clanker/agents/opencode/ForbiddenPatternTest.kt`:
- Around line 13-27: The test currently uses fragile substring checks
(it.text.contains("println("), "System.err", "System.out") that can be bypassed
by spacing; update the assertions in ForbiddenPatternTest.kt (the
scope.files.assertFalse { it.text... } lambdas) to use regex-based matching
instead — e.g. replace the println check with a word-boundary/whitespace-aware
regex like \bprintln\s*\( and replace System.err/System.out checks with regexes
that allow optional whitespace around the dot (e.g. System\s*\.\s*(err|out)) so
the assertions use containsMatch/Regex matching on it.text rather than plain
contains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d6a40cf-3619-4ffc-bcd7-cc0c97608d0e
📒 Files selected for processing (21)
claude/README.mdclaude/src/main/kotlin/zone/clanker/agents/claude/Claude.ktclaude/src/main/kotlin/zone/clanker/agents/claude/ClaudeMcpServeTask.ktclaude/src/main/kotlin/zone/clanker/agents/claude/ClaudeRunTask.ktclaude/src/slopTest/kotlin/zone/clanker/agents/claude/ForbiddenPatternTest.ktcodex/README.mdcodex/src/main/kotlin/zone/clanker/agents/codex/Codex.ktcodex/src/main/kotlin/zone/clanker/agents/codex/CodexExecTask.ktcodex/src/slopTest/kotlin/zone/clanker/agents/codex/ForbiddenPatternTest.ktcopilot/README.mdcopilot/src/main/kotlin/zone/clanker/agents/copilot/Copilot.ktcopilot/src/main/kotlin/zone/clanker/agents/copilot/CopilotRunTask.ktcopilot/src/slopTest/kotlin/zone/clanker/agents/copilot/ForbiddenPatternTest.ktexec/src/main/kotlin/zone/clanker/agents/exec/Cli.ktexec/src/main/kotlin/zone/clanker/agents/exec/CommandBuilder.ktexec/src/slopTest/kotlin/zone/clanker/agents/exec/ForbiddenPatternTest.ktexec/src/test/kotlin/zone/clanker/agents/exec/CliTest.ktopencode/src/main/kotlin/zone/clanker/agents/opencode/OpenCode.ktopencode/src/main/kotlin/zone/clanker/agents/opencode/OpenCodeRunTask.ktopencode/src/main/kotlin/zone/clanker/agents/opencode/OpenCodeServeTask.ktopencode/src/slopTest/kotlin/zone/clanker/agents/opencode/ForbiddenPatternTest.kt
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
Full hardening pass across all 5 modules.
Anti-patterns fixed
println→logger.lifecycle()in ClaudeMcpServeTask, OpenCodeServeTaskSystem.err.print→logger.warn()in Cli.kttry-catch→runCatchingin Cli.execSafety
--dangerously-skip-permissions,--dangerously-bypass-approvals-and-sandbox,--yolo) now emitlogger.warn()when enabledCli.which()pre-check before execution — fails fast with "not found. Install it first."Robustness
Cli.exec: stdout/stderr now read concurrently on separate threadstimeoutSecondsconfigurable per agent via extension propertyArchitecture
addFlaghelper toexecmoduleCommandBuilder.ktTest plan
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests