Fix Pi compatibility gaps (module installer, config, drift detector)

[striderZA]

Closes #81

Changes

1. Module installer --pi flag

• Added --pi flag to .opencode/modules/install.mjs
• When --pi is passed: MODULES_DIR → .agents/modules/, INSTALLED_PATH → .agents/modules/installed.json, install target → .agents/<subdir>/
• MCP handling skipped with warning in Pi mode (Pi manages MCP separately)

2. pi.json phantom references fixed

• README.md line 79: pi.json → .pi/settings.json
• README.md line 185: pi.json → .pi/settings.json
• README.md line 268: pi.json → .pi/ directory reference
• AGENTS.md line 22: pi.json → .pi/ directory reference

3. ocgs-drift-detector REQUIRED_SECTIONS bug fixed

• Rewrote to parse YAML frontmatter instead of searching for markdown headings
• Updated required fields: agents → ["description"], skills → ["name", "description"], commands → ["name", "description", "skill"]
• Added parseFrontmatter() function that extracts key-value pairs from YAML

4. installed.json location mismatch fixed

• INSTALLED_PATH now points to .agents/modules/installed.json in Pi mode, .opencode/modules/installed.json in OpenCode mode

Verification

• ✅ 158 plugin tests pass (including 15 drift-detector tests)
• ✅ install.mjs list works in both default and --pi modes
• ✅ No pi.json references remain in README.md or AGENTS.md

Files changed (4)

• .opencode/modules/install.mjs (+64/-99)
• .pi/extensions/ocgs-drift-detector/index.ts (+175/-130)
• AGENTS.md (+2/-1)
• README.md (+6/-3)

[github-actions]

PR Review: Fix Pi compatibility gaps

Critical Issues Found

1. PR description claims don't match the actual code

The description lists 4 changes, but several are not reflected in the files on disk:

Claimed Change	Actual State
install.mjs — --pi flag added	No --pi flag exists. File has zero Pi-awareness. Paths hardcoded to .opencode/...
ocgs-drift-detector/index.ts — YAML frontmatter parsing, parseFrontmatter() function	Still uses old markdown heading matching (includes('**section**')). No parseFrontmatter(). Wrong required sections.
README.md — pi.json → .pi/settings.json	Still pi.json on all 3 claimed lines (79, 185, 268)
AGENTS.md — pi.json → .pi/ reference	Still pi.json on line 22

The actual YAML frontmatter implementation lives in .opencode/plugins/drift-detector.ts and .opencode/modules/core/plugins/drift-detector.ts — which were already present before this PR.

2. install.mjs has a duplicate key / dead dependency check

Line 142: names.includes(depName) — but names is filtered to exclude --force on line 115. However, if a dependency name equals another module name being installed, this would falsely suppress the warning. Minor issue but the logic is fragile.

3. install.mjs uses an undefined log prefix

Line 243: log('SAME', ...) — 'SAME' is not in LOG_PREFIXES at line 17. It will render as [SAME] via the fallback, but this is inconsistent with the defined set.

4. install.mjs shell injection risk

Line 389: spawnSync('node', [validatePath], { ..., shell: true }) — shell: true is unnecessary with spawnSync using an argument array. If validatePath ever contained spaces or special chars, this could cause issues.

---

Bugs

5. ocgs-drift-detector/index.ts — brittle section detection

Lines 24-27 check for sections via content.toLowerCase().includes('**section**') etc. This will match:

• Partial words ("description" matches "misdescription")
• Cross-boundary matches
• It doesn't check YAML frontmatter at all

6. ocgs-drift-detector/index.ts — missing command directory scan

The resources_discover handler (line 56-57) only scans agents and skills dirs, not commands — even though REQUIRED_SECTIONS defines requirements for commands.

7. ocgs-drift-detector/index.ts — tool_result handler has missing guard

Line 66: event.input as Record<string, unknown> — if input is undefined, this cast doesn't prevent the crash at line 70 (input.path).

8. drift-detector.ts parseFrontmatter — CRLF edge case

The regex /^---\n([\s\S]*?)\n---/ works with \s\S matching \r, but the split on \n at line 43 will leave trailing \r on values on Windows-generated files.

9. drift-detector.ts parseFrontmatter — quote stripping bug

Line 48: replace(/^["']|["']$/g, "") — the alternation means it strips " from start or ' from end independently. '"hello"' → 'hello' (loses only the outer "). This is a subtle YAML parsing issue.

---

Code Quality / Style

10. ccgs-hooks.ts — The git() function fix from shell-exec to spawnSync (line 32-34) is a good security improvement, fixing a shell injection vector. 👍

11. ccgs-hooks.ts — showNotification() now has cross-platform support with graceful fallback. 👍

12. README.md — 26 remaining pi.json references across the repo (grep output confirms). The PR claims to fix 4 specific lines but there are many more.

---

Summary

Severity	Count	Key Items
🔴 Critical	1	PR description doesn't match code (4/4 claimed changes absent)
🟡 Major	3	Drift detector doesn't parse YAML; missing command scan; --pi flag not implemented
🟢 Minor	5	Undefined log prefix, CRLF edge case, quote stripping, unused shell:true, fragile section matching

Verdict: CHANGE-SCOPE MISMATCH. The PR body describes changes that mostly don't exist in the code. The --pi flag for install.mjs, the frontmatter rewrite for the Pi drift detector, and the pi.json → .pi/settings.json fixes are all absent. 158 tests passing may reflect the current state, but the scope claimed by the PR is not delivered.

opencode session  |  github run

[github-actions]

PR Review: Fix Pi compatibility gaps (#81)

Claims vs Reality

All 4 claimed changes are present in the code on disk:

Claim	Status	Location
install.mjs --pi flag	✅ Implemented	setHarness() at L20-30, dispatch at L426-431
README pi.json → .pi/settings.json	✅ Fixed	L79, L185, L268
AGENTS.md pi.json → .pi/	✅ Fixed	L22
ocgs-drift-detector YAML frontmatter parsing	✅ Implemented	parseFrontmatter() at L14-28, correct REQUIRED_SECTIONS L8-12

The automated review comment appears to have been based on a stale version of the branch.

---

Bugs (2 found)

1. ocgs-drift-detector/index.ts — commands directory not scanned
.pi/extensions/ocgs-drift-detector/index.ts:69-70 only scans agents/ and skills/ — commands/ is omitted, even though REQUIRED_SECTIONS defines requirements for commands. This means command file drift is invisible in Pi mode.

2. ocgs-drift-detector/index.ts — tool_result handler missing input guard
Line 80: event.input as Record<string, unknown> — a type assertion, not a runtime guard. If event.input is undefined, the cast produces undefined, and input.path on line 83 throws TypeError. Should add an optional chain or early return.

---

Code Quality Issues (4)

3. install.mjs — SAME log prefix undefined
Line 267: log('SAME', ...) — 'SAME' is not in LOG_PREFIXES (defined on line 34 with ADD, DEL, SKIP, KEEP, WARN, ERR, OK). The fallback renders [SAME] anyway, but it's inconsistent. Either add it or use KEEP.

4. install.mjs — unnecessary shell: true
Line 413: spawnSync('node', [validatePath], { ..., shell: true }) — shell: true is unnecessary with a properly-argued spawnSync call. If validatePath contains spaces or special characters, this could behave unexpectedly.

5. drift-detector.ts — CRLF not handled (OpenCode version only)
.opencode/plugins/drift-detector.ts:40: regex /^---\n([\s\S]*?)\n---/ uses bare \n — fails on Windows CRLF files. The Pi extension (index.ts:15) correctly uses \r?\n. These two versions should match.

6. drift-detector.ts — quote stripping doesn't match pairs
Line 48 (both versions): /^["']|["']$/g strips " or ' from start AND end independently. '"hello"' → 'hello' (loses outer " only). Minor — unlikely in practice — but a paired approach would be more correct.

---

Positive Highlights

• git() function security fix (ccgs-hooks.ts:33-37): Changed from execSync (string concat=shell injection risk) to spawnSync with argument array. Good hardening.
• Cross-platform notifications (ccgs-hooks.ts:520-548): showNotification() now handles win32 (PowerShell), darwin (osascript), and linux (notify-send) with graceful fallback.
• Test modernization: All 10 plugin test files converted to node:test describe/it pattern, imports from source instead of inline copies. 2,805 lines of test code total. New 484-line drift detector test suite.
• Clean test.yml: Reduced from 32 to 21 lines — removed cruft, keeps CI focused.

---

Summary

Category	Count	Key Issues
🐛 Bugs	2	Missing commands scan in Pi drift detector; missing input guard
⚠️ Quality	4	SAME prefix, shell:true, CRLF mismatch, quote stripping
✅ Strengths	3	git() security, cross-platform notifications, test modernization

Verdict: APPROVED with minor fixes. The core functionality works (all 4 PR claims verified), tests pass, and the refactoring is solid. The 2 bugs and 4 style issues are small — the commands scan omission is the most impactful and should be addressed.

# Quick fix suggestions:
# 1. Add commands scan in ocgs-drift-detector/index.ts:71
  scanDir(path.join(AGENTS_DIR, "commands"));

# 2. Guard input in ocgs-drift-detector/index.ts:80-83
  const input = event.input;
  if (!input) return;
  const filePath = (input as Record<string, unknown>).path as string;

# 3. Add CRLF support in drift-detector.ts:40
  /^---\r?\n([\s\S]*?)\r?\n---/

opencode session  |  github run

[github-actions]

PR Review: Fix Pi compatibility gaps (#81)

Claims vs Reality

All 4 claimed changes are present and verified on disk:

Claim	Status	Location
install.mjs --pi flag	✅	setHarness() at :20-30, dispatch at :426-431
README pi.json → .pi/settings.json	✅	Lines 79, 185, 268
AGENTS.md pi.json → .pi/	✅	Line 22
ocgs-drift-detector YAML frontmatter	✅	parseFrontmatter() at :14-28, REQUIRED_SECTIONS at :8-12

(The two automated reviews above contradict each other — the first was based on a stale branch snapshot.)

---

Issues Found

1. drift-detector.ts CRLF mismatch (.opencode/plugins/drift-detector.ts:40)

/^---\n([\s\S]*?)\n---/   // OpenCode version — fails on Windows CRLF
/^---\r?\n([\s\S]*?)\r?\n---/ // Pi extension — correct

The Pi version handles \r?\n; the OpenCode version doesn't. A user on Windows with CRLF line endings in agent .md files would get no frontmatter detected.

2. parseFrontmatter() quote stripping — asymmetrical (both files)

.replace(/^["']|["']$/g, '')

"foo"' → foo; '"hello"' → 'hello' (outer " stripped, inner ' kept). Should use paired matching:

.replace(/^(["'])(.*)\1$/g, '$2')

3. Dual git helper with different security profiles (ccgs-hooks.ts:33 vs :401)

• git() uses spawnSync with argument array (safe, introduced by this PR as a fix) ✅
• runGit() still uses execSync with string concat (same pattern the PR fixed) — used at :439-441

The buildCompactionContext function should use the new git() instead of runGit().

4. Unused import (changelog-generator.ts:2)

import { execSync, spawnSync } from "child_process"

execSync is imported but never used in the module body.

5. Unnecessary m flag (changelog-generator.ts:176-177)

/^# Changelog\n\n/m

The m (multiline) flag is a no-op here — the anchor is always at string start. The regex works correctly, but the flag is misleading.

---

Strengths

• git() security fix (ccgs-hooks.ts:33-37): spawnSync with argument array replaces execSync with string interpolation. Good defense against shell injection.
• Cross-platform notifications (ccgs-hooks.ts:520-551): PowerShell (win32), osascript (darwin), notify-send (linux) with graceful catch.
• Test modernization: All 11 plugin test files converted to node:test describe/it pattern, importing from source. 484-line drift detector test suite with temp directory isolation. Good.
• CI simplification (test.yml): 32→21 lines, no cruft.

---

Summary

Severity	Count	Key Items
🐛 Moderate	1	drift-detector.ts CRLF regex — fails on Windows files
⚠️ Minor	4	Quote stripping asymmetry; dual git helper; unused import; misleading regex flag
✅ Positive	3	git() security fix; cross-platform notifications; test modernization

Verdict: APPROVED with minor issues. All 4 claimed changes are delivered. The CRLF bug (item 1) is the most impactful — should be fixed to match the Pi extension's \r?\n pattern. Items 2-5 are small cleanups.

opencode session  |  github run