Skip to content

Commit c4cb453

Browse files
committed
fix: address 28 PR review items from docs-consistency, CodeRabbit, Copilot
- Hookify: fix regex patterns for pytest (bare pytest) and cd (whitespace bypass) - OpenCode: remove non-existent .claude/rules/common/ from instructions - Memory plugin: fix hardcoded path (derive from cwd), add path traversal protection - Hooks plugin: fix JSON parsing for check_push_rebased, fix error logging - Shell scripts: fix exit code alignment (check_git_c_cwd exits 2 on deny) - Block redirects to files (more comprehensive check_bash_no_write) - Skill: add auto-detection for CodeRabbit processing status - Agent docs: fix MD022 (blank lines around headings)
1 parent ebab3fb commit c4cb453

9 files changed

Lines changed: 107 additions & 62 deletions

File tree

.claude/hookify.enforce-parallel-tests.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ event: bash
55
conditions:
66
- field: command
77
operator: regex_match
8-
pattern: (?:run\s+pytest|python\s+-m\s+pytest)\b
8+
pattern: (?:^|\s)(?:pytest|run\s+pytest|python\s+-m\s+pytest)\b
99
- field: command
1010
operator: not_contains
1111
pattern: "-n 8"

.claude/hookify.no-cd-prefix.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
name: no-cd-prefix
33
enabled: true
44
event: bash
5-
pattern: ^cd\s+
5+
pattern: ^\s*cd\s+
66
action: block
77
---
88

.claude/skills/aurelio-review-pr/SKILL.md

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -152,31 +152,35 @@ git diff main --name-only
152152

153153
Based on changed files, launch applicable review agents **in parallel** using the Task tool. **Do NOT use `run_in_background`** -- launch them as regular parallel Task calls so results arrive together and the user sees all agents complete before triage begins. Background agents cause confusing late-arriving `task-notification` messages that make it look like you presented triage before agents finished.
154154

155+
> **IMPORTANT - OpenCode Agent Mapping**: When running in OpenCode (not Claude Code), you MUST use these working subagent types. NEVER use the Claude Code plugin names directly -- they will fail with "Unknown agent type".
156+
155157
| Agent | When to launch | subagent_type |
156158
|---|---|---|
157-
| **docs-consistency** | **ALWAYS** -- runs on every PR regardless of change type | `pr-review-toolkit:code-reviewer` (custom prompt below) |
158-
| **code-reviewer** | Any `src_py` or `test_py` | `pr-review-toolkit:code-reviewer` |
159-
| **python-reviewer** | Any `src_py` or `test_py` | `everything-claude-code:python-reviewer` |
160-
| **pr-test-analyzer** | `test_py` changed, OR `src_py` changed with no corresponding test changes | `pr-review-toolkit:pr-test-analyzer` |
161-
| **silent-failure-hunter** | Diff contains `try`, `except`, `raise`, error handling patterns | `pr-review-toolkit:silent-failure-hunter` |
162-
| **comment-analyzer** | Diff contains docstring changes (`"""`) or significant comment changes | `pr-review-toolkit:comment-analyzer` |
163-
| **type-design-analyzer** | Diff contains `class ` definitions, `BaseModel`, `TypedDict`, type aliases | `pr-review-toolkit:type-design-analyzer` |
164-
| **logging-audit** | Any `src_py` changed | `pr-review-toolkit:code-reviewer` (custom prompt below) |
165-
| **resilience-audit** | Any `src_py` changed | `pr-review-toolkit:code-reviewer` (custom prompt below) |
166-
| **conventions-enforcer** | Any `src_py` or `test_py` | `pr-review-toolkit:code-reviewer` (custom prompt below) |
167-
| **security-reviewer** | Files in `src/synthorg/api/`, `src/synthorg/security/`, `src/synthorg/tools/`, `src/synthorg/config/`, `src/synthorg/persistence/`, `src/synthorg/engine/` changed, OR any `web_src` changed, OR diff contains `subprocess`, `eval`, `exec`, `pickle`, `yaml.load`, `sql`, auth/credential patterns | `everything-claude-code:security-reviewer` |
168-
| **frontend-reviewer** | Any `web_src` or `web_test` | `pr-review-toolkit:code-reviewer` (custom prompt below) |
169-
| **design-token-audit** | Any `web_src` | `.claude/agents/design-token-audit.md` prompt (scans for density, animation, spacing token violations) |
170-
| **api-contract-drift** | Any file in `src/synthorg/api/` OR `web/src/api/` OR `src/synthorg/core/enums.py` | `pr-review-toolkit:code-reviewer` (custom prompt below) |
171-
| **infra-reviewer** | Any `docker`, `ci`, or `infra_config` file | `pr-review-toolkit:code-reviewer` (custom prompt below) |
172-
| **persistence-reviewer** | Any file in `src/synthorg/persistence/` | `everything-claude-code:database-reviewer` |
173-
| **test-quality-reviewer** | Any `test_py` or `web_test` | `pr-review-toolkit:pr-test-analyzer` (custom prompt below) |
174-
| **async-concurrency-reviewer** | Diff contains `async def`, `await`, `asyncio`, `TaskGroup`, `create_task`, `aiosqlite` in `src_py` files | `pr-review-toolkit:code-reviewer` (custom prompt below) |
175-
| **go-reviewer** | Any `cli_go` | `everything-claude-code:go-reviewer` |
176-
| **go-security-reviewer** | Any `cli_go` -- diff contains `exec.Command`, `os/exec`, `http`, `os.Remove`, `os.WriteFile`, `filepath`, user-supplied paths | `everything-claude-code:security-reviewer` |
177-
| **go-conventions-enforcer** | Any `cli_go` | `pr-review-toolkit:code-reviewer` (go-conventions-enforcer custom prompt -- same as in pre-pr-review skill) |
178-
| **issue-resolution-verifier** | Issue is linked (pre-existing or auto-linked in Phase 2) | `pr-review-toolkit:code-reviewer` (custom prompt below) |
179-
| **tool-parity-checker** | Any `.claude/` or `.opencode/` or `opencode.json` or `AGENTS.md` or `CLAUDE.md` file changed | `.claude/agents/tool-parity-checker.md` prompt (verifies Claude Code <-> OpenCode config parity) |
159+
| **docs-consistency** | **ALWAYS** -- runs on every PR regardless of change type | `explore` (use custom prompt below) |
160+
| **tool-parity-checker** | Any `.claude/` or `.opencode/` or `opencode.json` or `AGENTS.md` or `CLAUDE.md` file changed | `explore` (use custom prompt below) |
161+
| **code-reviewer** | Any `src_py` or `test_py` | `explore` |
162+
| **python-reviewer** | Any `src_py` or `test_py` | `explore` |
163+
| **pr-test-analyzer** | `test_py` changed, OR `src_py` changed with no corresponding test changes | `explore` |
164+
| **silent-failure-hunter** | Diff contains `try`, `except`, `raise`, error handling patterns | `explore` |
165+
| **comment-analyzer** | Diff contains docstring changes (`"""`) or significant comment changes | `explore` |
166+
| **type-design-analyzer** | Diff contains `class ` definitions, `BaseModel`, `TypedDict`, type aliases | `explore` |
167+
| **logging-audit** | Any `src_py` changed | `explore` (use custom prompt below) |
168+
| **resilience-audit** | Any `src_py` changed | `explore` (use custom prompt below) |
169+
| **conventions-enforcer** | Any `src_py` or `test_py` | `explore` (use custom prompt below) |
170+
| **security-reviewer** | Files in sensitive paths OR any `web_src` changed OR diff contains dangerous patterns | `explore` |
171+
| **frontend-reviewer** | Any `web_src` or `web_test` | `explore` (use custom prompt below) |
172+
| **design-token-audit** | Any `web_src` | `explore` |
173+
| **api-contract-drift** | Any file in `src/synthorg/api/` OR `web/src/api/` OR `src/synthorg/core/enums.py` | `explore` (use custom prompt below) |
174+
| **infra-reviewer** | Any `docker`, `ci`, or `infra_config` file | `explore` (use custom prompt below) |
175+
| **persistence-reviewer** | Any file in `src/synthorg/persistence/` | `explore` |
176+
| **test-quality-reviewer** | Any `test_py` or `web_test` | `explore` (use custom prompt below) |
177+
| **async-concurrency-reviewer** | Diff contains `async def`, `await`, `asyncio`, `TaskGroup`, `create_task`, `aiosqlite` in `src_py` files | `explore` (use custom prompt below) |
178+
| **go-reviewer** | Any `cli_go` | `explore` |
179+
| **go-security-reviewer** | Any `cli_go` with dangerous patterns | `explore` |
180+
| **go-conventions-enforcer** | Any `cli_go` | `explore` |
181+
| **issue-resolution-verifier** | Issue is linked (pre-existing or auto-linked in Phase 2) | `explore` (use custom prompt below) |
182+
183+
**If the Task tool fails** (e.g., "Unknown agent type"), fall back to running the check manually using Read/Grep tools on the changed files.
180184

181185
The **issue-resolution-verifier** agent checks whether the PR fully resolves the linked issue. It only runs when an issue is linked -- either from a pre-existing `closes #N` in the PR body, or auto-linked/user-selected during Phase 2's search.
182186

@@ -547,10 +551,14 @@ Collect all findings with their severity/confidence scores.
547551

548552
**CRITICAL: Fetch ALL reviewers -- do NOT filter by known bot names.** The set of external reviewers varies per repo and can include any combination of bots (CodeRabbit, Gemini, Copilot, Greptile, etc.) and human reviewers. Always fetch unfiltered results and categorize by author from the response.
549553

550-
**CRITICAL: Wait for all bots to finish processing.** Before triaging, check if any bot reviewer is still processing (e.g. CodeRabbit's "Currently processing" placeholder, or a review with an empty body). If a bot appears to still be processing:
551-
1. Poll every 30 seconds for up to 3 minutes (6 checks)
552-
2. If still not ready after 3 minutes, proceed without it and mark its coverage as "pending" in the triage table
553-
3. After implementing fixes and pushing, re-check for the bot's feedback in Phase 9
554+
**CRITICAL: Wait for all bots to finish processing.** Before triaging, check if any bot reviewer is still processing:
555+
1. First check the ISSUE comments (not PR reviews) for bot status - CodeRabbit posts "Currently processing" placeholder there
556+
2. If found, poll every 30 seconds for up to 3 minutes (6 checks)
557+
3. After each poll, re-fetch the issue comments to check if processing is complete
558+
4. If still not ready after 3 minutes, proceed and mark its coverage as "pending" in the triage table
559+
5. After implementing fixes and pushing, re-check for the bot's feedback in Phase 9
560+
561+
**ALWAYS check both issue comments AND review submissions for bots** -- some bots (CodeRabbit) use issue comments to signal processing status, while others (Gemini, Copilot) use PR review submissions.
554562

555563
Fetch from three GitHub API sources **in parallel** using `gh api` -- **always unfiltered** (no `select(.user.login == ...)` filtering):
556564

.opencode/agents/comment-analyzer.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,32 +15,37 @@ You analyze comments and docstrings for accuracy, completeness, and adherence to
1515
## What to Check
1616

1717
### 1. Docstring Completeness (MEDIUM)
18+
1819
- Public classes missing class-level docstring
1920
- Public functions missing docstring
2021
- Missing `Args:` section for functions with parameters
2122
- Missing `Returns:` section for functions with non-None return
2223
- Missing `Raises:` section for functions that raise exceptions
2324

2425
### 2. Docstring Accuracy (HIGH)
26+
2527
- Docstring describes behavior the function no longer implements
2628
- Parameter names in docstring don't match function signature
2729
- Return type description doesn't match actual return type
2830
- Documented exceptions not actually raised (or vice versa)
2931
- Copy-pasted docstrings from similar functions not updated
3032

3133
### 3. Google Style Compliance (MEDIUM)
34+
3235
- Wrong docstring format (must be Google style, not NumPy or reST)
3336
- Missing one-line summary as first line
3437
- Args/Returns/Raises sections with wrong formatting
3538
- Multi-line descriptions not properly indented
3639

3740
### 4. Comment Quality (MEDIUM)
41+
3842
- Comments that just restate the code (`# increment counter` before `counter += 1`)
3943
- Outdated TODO/FIXME/HACK comments referencing resolved issues
4044
- Commented-out code blocks (should be removed)
4145
- Comments explaining "what" instead of "why"
4246

4347
### 5. Comment Maintenance Risks (LOW)
48+
4449
- Comments referencing specific line numbers (will drift)
4550
- Comments referencing removed functions or classes
4651
- Links to external resources that may be dead
@@ -61,7 +66,8 @@ You analyze comments and docstrings for accuracy, completeness, and adherence to
6166
## Report Format
6267

6368
For each finding:
64-
```
69+
70+
```text
6571
[SEVERITY] file:line -- Category
6672
Problem: What's wrong with the comment/docstring
6773
Fix: Corrected version or action needed

.opencode/plugins/memory.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
*
44
* Provides read/write access to the shared MEMORY.md system used by
55
* both Claude Code and OpenCode. Memory files live in:
6-
* ~/.claude/projects/C--Users-Aurelio-synthorg/memory/
6+
* ~/.claude/projects/<mangled-cwd>/memory/
77
*
88
* Format (same as Claude Code):
99
* - MEMORY.md: index file with one-line pointers
@@ -13,16 +13,16 @@
1313

1414
import type { Plugin } from "@opencode-ai/plugin";
1515
import { existsSync, mkdirSync, readFileSync, writeFileSync, unlinkSync } from "fs";
16-
import { join, basename } from "path";
16+
import { join, resolve, dirname } from "path";
1717
import { homedir } from "os";
1818

19-
const MEMORY_DIR = join(
20-
homedir(),
21-
".claude",
22-
"projects",
23-
"C--Users-Aurelio-synthorg",
24-
"memory",
25-
);
19+
function getMemoryDir(): string {
20+
const cwd = process.cwd();
21+
const mangled = cwd.replace(/[:/\\]/g, "_").replace(/^_/, "").replace(/_$/, "");
22+
return join(homedir(), ".claude", "projects", mangled, "memory");
23+
}
24+
25+
const MEMORY_DIR = getMemoryDir();
2626
const MEMORY_INDEX = join(MEMORY_DIR, "MEMORY.md");
2727

2828
interface MemoryEntry {
@@ -112,10 +112,19 @@ export const MemoryPlugin: Plugin = async ({ client, $, app }) => {
112112
if (!filename) {
113113
throw new Error("delete_memory requires: filename");
114114
}
115-
const filepath = join(MEMORY_DIR, filename);
115+
// Validate: only allow simple filenames, no path traversal
116+
const safeName = filename.replace(/[^a-zA-Z0-9_.-]/g, "");
117+
if (safeName !== filename || filename.includes("..")) {
118+
throw new Error("delete_memory: invalid filename (no path traversal allowed)");
119+
}
120+
const filepath = resolve(MEMORY_DIR, safeName);
121+
// Ensure resolved path is within MEMORY_DIR
122+
if (!filepath.startsWith(MEMORY_DIR)) {
123+
throw new Error("delete_memory: path traversal detected");
124+
}
116125
if (existsSync(filepath)) {
117126
unlinkSync(filepath);
118-
updateIndex("remove", filename, "", "");
127+
updateIndex("remove", safeName, "", "");
119128
}
120129
return;
121130
}

.opencode/plugins/synthorg-hooks.ts

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,17 @@
44
* Mirrors the Claude Code hooks defined in .claude/settings.json and
55
* .claude/settings.local.json by calling the same shell scripts.
66
*
7-
* Claude Code hooks:
7+
* Committed Claude Code hooks (from .claude/settings.json):
88
* PreToolUse (Bash): scripts/check_push_rebased.sh
9-
* PreToolUse (Bash): scripts/check_bash_no_write.sh
10-
* PreToolUse (Bash): scripts/check_git_c_cwd.sh
11-
* PostToolUse (Edit|Write): scripts/check_web_design_system.py
129
*
13-
* Hookify rules (from .claude/hookify.*.md):
10+
* Hookify rules enforced via this plugin (from .claude/hookify.*.md):
1411
* block-pr-create: blocks direct `gh pr create`
1512
* enforce-parallel-tests: enforces `-n 8` with pytest
1613
* no-cd-prefix: blocks `cd` prefix in Bash commands
1714
* no-local-coverage: blocks `--cov` flags locally
15+
* check_bash_no_write.sh: blocks file writes via Bash
16+
* check_git_c_cwd.sh: blocks unnecessary git -C to cwd
17+
* check_web_design_system.py: validates design tokens on web file edits
1818
*/
1919

2020
import type { Plugin } from "@opencode-ai/plugin";
@@ -69,8 +69,8 @@ export const SynthOrgHooks: Plugin = async ({ client, $, app }) => {
6969
);
7070
}
7171

72-
// no-cd-prefix: block cd prefix in Bash commands
73-
if (/^cd\s+/i.test(command)) {
72+
// no-cd-prefix: block cd prefix in Bash commands (with optional leading whitespace)
73+
if (/^\s*cd\s+/i.test(command)) {
7474
throw new Error(
7575
"BLOCKED: Do not use `cd` in Bash commands -- it poisons the cwd for all subsequent calls. The working directory is ALREADY set to the project root. Run commands directly. For Go commands: use `go -C cli <command>`. For subdir tools without a `-C`/`--prefix` equivalent: use `bash -c \"cd <dir> && <cmd>\"`.",
7676
);
@@ -93,8 +93,18 @@ export const SynthOrgHooks: Plugin = async ({ client, $, app }) => {
9393
{ command },
9494
15000,
9595
);
96-
if (result && result.includes("block")) {
97-
throw new Error(result);
96+
if (result) {
97+
try {
98+
const parsed = JSON.parse(result);
99+
if (parsed.hookSpecificOutput?.permissionDecision === "deny") {
100+
throw new Error(parsed.hookSpecificOutput?.permissionDecisionReason || "Push blocked");
101+
}
102+
} catch {
103+
// Not JSON or parse error - check for legacy "block" pattern
104+
if (result.includes("block")) {
105+
throw new Error(result);
106+
}
107+
}
98108
}
99109
}
100110

@@ -133,9 +143,10 @@ export const SynthOrgHooks: Plugin = async ({ client, $, app }) => {
133143
`python scripts/check_web_design_system.py`,
134144
{ timeout: 10000, encoding: "utf-8" },
135145
);
136-
} catch {
146+
} catch (error: unknown) {
147+
const err = error as { message?: string; stderr?: string };
137148
// Log but don't block -- PostToolUse is advisory
138-
console.error("Design system check failed for:", output.args.file_path);
149+
console.error("Design system check failed for:", output.args.file_path, "-", err.message || err.stderr || "Unknown error");
139150
}
140151
}
141152
},

opencode.json

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,7 @@
5454
"model": "ollama-cloud/minimax-m2.5:cloud",
5555
"instructions": [
5656
"CLAUDE.md",
57-
".claude/rules/common/agents.md",
58-
".claude/rules/common/bash.md",
59-
".claude/rules/common/hooks.md",
60-
".claude/rules/common/patterns.md",
61-
".claude/rules/common/performance.md",
57+
"AGENTS.md",
6258
".claude/hookify.block-pr-create.md",
6359
".claude/hookify.enforce-parallel-tests.md",
6460
".claude/hookify.function-length.md",

scripts/check_bash_no_write.sh

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,16 @@ if printf '%s\n' "$COMMAND" | grep -qE "<<-?\s*'?[A-Za-z_]"; then
3030
deny "Do not use heredocs (<< EOF) to write files. Use the Write tool to create new files or the Edit tool to modify existing files. Never use Bash for file creation or modification."
3131
fi
3232

33-
# Output redirection to a file: > file, >> file, > /path, > "./path"
34-
# Skip >/dev/null, >&2, etc.
35-
if printf '%s\n' "$COMMAND" | grep -qE '>\s*"?(/[^d>]|\.\.?/|[a-zA-Z]:\\)'; then
36-
deny "Do not use shell redirects (> or >>) to write files. Use the Write tool to create new files or the Edit tool to modify existing files. Never use Bash for file creation or modification."
33+
# Output redirection: > file, >> file, > /path, > "./path", > file.txt
34+
# Block ALL redirects to files (only allow fd redirects like >&2, 2>&1)
35+
# This catches: echo > file.txt, cat > foo, > output, etc.
36+
if printf '%s\n' "$COMMAND" | grep -qE '(^|[|&;])\s*>>?\s*"?[^-]'; then
37+
# Extract redirect target to check if it's a file descriptor
38+
REDIR=$(printf '%s\n' "$COMMAND" | grep -oE '>>?\s*"?[^|&;<>]+' | head -1 | sed 's/^>>\?["'"'"']*//')
39+
# Only allow if it's a file descriptor (>&N or <&N format)
40+
if [[ ! "$REDIR" =~ ^&[0-9]+$ ]]; then
41+
deny "Do not use shell redirects (> or >>) to write files. Use the Write tool to create new files or the Edit tool to modify existing files. Never use Bash for file creation or modification."
42+
fi
3743
fi
3844

3945
# echo/printf > filename.ext (catches echo "text" > file.txt)

scripts/check_git_c_cwd.sh

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,5 +32,14 @@ NORM_ARG=$(normalize "$GIT_C_PATH")
3232
NORM_PWD=$(normalize "$PWD")
3333

3434
if [[ "$NORM_ARG" == "$NORM_PWD" ]]; then
35-
echo '{"decision":"block","reason":"BLOCKED: git -C points to the current working directory. Just use git directly -- the Bash tool already runs in the project root."}'
35+
cat <<ENDJSON
36+
{
37+
"hookSpecificOutput": {
38+
"hookEventName": "PreToolUse",
39+
"permissionDecision": "deny",
40+
"permissionDecisionReason": "BLOCKED: git -C points to the current working directory. Just use git directly -- the Bash tool already runs in the project root."
41+
}
42+
}
43+
ENDJSON
44+
exit 2
3645
fi

0 commit comments

Comments
 (0)