Feat/factory ai

[thedotmack]

No description provided.

[claude]

PR Review: Feat/Factory AI (Droid Integration)

Overall: This PR implements a well-reasoned fix for the non-functional Factory CLI (Droid) integration. The three root causes identified in PLAN.md are addressed directly, the implementation is clean, and test coverage was added for the new logic. A few things worth discussing below.

---

What's Well Done

• Platform detection is clean — using DROID_PLUGIN_ROOT vs CLAUDE_PLUGIN_ROOT as the discriminant is a solid, low-coupling strategy
• Adapter pattern is well-applied — droidAdapter mirrors the existing adapter interface correctly without breaking Claude Code behavior
• Fallback chain in hooks.json — priority ordering (runtime var → DROID_PLUGIN_ROOT → filesystem probe → Claude default) handles all cases
• isPluginDisabledInAnySettings() — good defensive check that covers both platforms
• PLAN.md — excellent documentation of the problem, but see note below about whether it belongs committed to the repo

---

Issues & Suggestions

Medium Priority

1. Repeated inline shell logic in hooks.json

The new fallback resolution logic is ~200 characters of shell per hook and appears 6+ times. This is a maintenance hazard — a future fix needs to be applied in multiple places. A shared helper script (e.g., resolve-root.sh) sourced by each hook command would be cleaner and easier to test.

# Current (repeated in every hook):
_R="${CLAUDE_PLUGIN_ROOT}"; [ -z "$_R" ] && _R="${DROID_PLUGIN_ROOT}"; [ -z "$_R" ] && { [ -d "$HOME/.factory/..." ] && _R="..." || _R="..."; };

# Better: delegate to a script
_R=$(bash "$PLUGIN_ROOT/resolve-root.sh")

2. detectPlatform() false positive risk

The function auto-upgrades claude-code → droid if DROID_PLUGIN_ROOT is set. If a developer runs Claude Code in a shell where Factory has also set DROID_PLUGIN_ROOT (shared terminal session), hooks will silently use the Droid adapter. Consider logging the detected platform at debug level, or documenting this assumption explicitly.

3. FACTORY_PLUGIN_SETTINGS_KEY is identical to CLAUDE_PLUGIN_SETTINGS_KEY

export const CLAUDE_PLUGIN_SETTINGS_KEY = 'claude-mem@thedotmack';
export const FACTORY_PLUGIN_SETTINGS_KEY = 'claude-mem@thedotmack'; // same value

The distinction is purely semantic right now. This is fine if the intent is "same key, different settings file location" — but if that's the case, a comment explaining the design would prevent future confusion.

---

Low Priority

4. Droid-specific fields extracted but potentially unused

droidAdapter extracts permissionMode, hookEventName, source, reason, stopHookActive into NormalizedHookInput, but there's no evidence in this PR that any handler actually consumes them. If they're stubs for future use, a // TODO: comment would clarify intent. If they're meant to be used now, the handlers need updating.

5. as any in droidAdapter

const r = (raw ?? {}) as any;

Same pattern as the Claude Code adapter, so consistency is fine — but if there's ever a chance to add a proper DroidRawInput interface (even loosely typed), it would improve discoverability of what Droid actually sends.

6. Silent Factory sync failure

In sync-marketplace.cjs, if Factory paths don't exist, the error is caught and logged as info-level (ℹ Factory sync skipped). This is appropriate in production, but during active development of this integration, warn level might be more visible for debugging.

7. No integration test for the full Droid flow

Unit tests cover detectPlatform() and isPluginDisabledInAnySettings(), but there's no test exercising hooks.json → droidAdapter → droid handler as an end-to-end path. Given this is the core bug being fixed, an integration smoke test would give confidence it won't silently regress.

8. PLAN.md committed to repo

PLAN.md is a great artifact, but planning docs tend to rot quickly once the code diverges. Consider either:

• Moving it to a docs/ or private/context/ location (where it reads more like an ADR)
• Converting the key decisions into inline comments in the relevant source files
• Deleting it post-merge if it served its purpose as a scratchpad

---

Testing

Existing tests were extended well — plugin-disabled-check.test.ts, plugin-distribution.test.ts, and version-consistency.test.ts all gained Factory-aware assertions. The new detect-platform.test.ts covers the happy path and defaults cleanly.

Missing coverage:

• Edge case: DROID_PLUGIN_ROOT set but pointing to non-existent path
• Edge case: both DROID_PLUGIN_ROOT and CLAUDE_PLUGIN_ROOT set simultaneously
• Full hook event flow for session-end and pre-compact via Droid

---

Summary

The core implementation is solid and fixes real bugs. The main concern I'd flag before merging is the repeated shell logic in hooks.json — that's a maintainability debt that's cheapest to fix now. Everything else is polish or forward-looking improvement.

Reviewed by Claude Sonnet 4.6 via claude-mem PR review