Add inline /wizard command for Auto Run document creation#112
Add inline /wizard command for Auto Run document creation#112pedramamini merged 58 commits intomainfrom
Conversation
Code Review - PR #112: Inline Wizard FeatureCompleted thorough review. Overall: excellent work with comprehensive test coverage and clean architecture. StrengthsArchitecture & Code Quality
Security
Testing
User Experience
Potential Issues1. Race Condition in startWizard
2. Large Document Handling
3. Error Recovery UX
Minor SuggestionsTemplate Variables: Uses manual .replace() chains vs centralized substituteTemplateVariables. Consider consistency. Accessibility: Good escape handling & focus management. Consider adding ARIA labels for icon-only buttons, live regions for confidence updates. Memory Management: Verify process cleanup on unmount, refresh, session switch. Test PlanCompleted:
TODO before merge:
DocumentationExcellent JSDoc and PR description. Minor gap: Update CLAUDE.md with inline wizard docs. Security SummarySecure ✓ - No critical issues. Good practices observed throughout. RecommendationApprove with minor suggestions High-quality, production-ready code. Before merge:
Nice-to-haves (follow-up PRs):
Great work! Exemplary code quality and testing rigor. 🎉 |
pedramamini
force-pushed
the
autorun-wizard
branch
from
d854c94 to
367632e
Compare
Code Review: PR #112 - Inline /wizard CommandExecutive SummaryThis PR adds a substantial new feature (~13,500 lines of code and tests) enabling inline Auto Run document creation. The code demonstrates excellent test coverage and follows project conventions well. However, there are critical security vulnerabilities, potential memory leaks, and race conditions that must be addressed before merge. Critical Issues (Must Fix Before Merge)1. Path Traversal Vulnerability - HIGH PRIORITYLocation: src/renderer/services/inlineWizardDocumentGeneration.ts:104-118 The current sanitizeFilename() function can be bypassed with inputs like ....// which becomes ../ after the first replace. Attack vector: Input like ....//....//etc/passwd becomes ../../etc/passwd after first replace Recommended fix: Iteratively remove .. sequences until none remain, and enforce length limits (255 chars max). Also check: src/main/ipc/handlers/autorun.ts:204,244,289,370 - similar vulnerable patterns 2. Process Leak from Timeout Race Condition - HIGH PRIORITYLocation: src/renderer/services/inlineWizardConversation.ts:564-600 If the exit callback resolves before timeout fires, the spawned process may become orphaned. Add an isResolved guard flag and ensure the process is killed on timeout. Same issue in: src/renderer/services/inlineWizardDocumentGeneration.ts:508-517 3. State Race Condition in sendMessage - MEDIUM PRIORITYLocation: src/renderer/hooks/useInlineWizard.ts:494-528 The function updates state via setState() but then reads from the stale closed-over state variable at line 528. This means the AI receives incomplete conversation history. Recommended fix: Use refs to track latest state or restructure to avoid closure issues. 4. Unbounded Memory Growth - MEDIUM PRIORITYLocation: src/renderer/services/inlineWizardConversation.ts:588-592 Output buffer grows indefinitely without size limits. Add a MAX_OUTPUT_SIZE constant (10MB suggested) and abort if exceeded. Same issue in: src/renderer/services/inlineWizardDocumentGeneration.ts:531-537 5. Debug Logging with Sensitive Data - MEDIUM PRIORITYLocation: src/main/ipc/handlers/autorun.ts:237-242 Document content may contain secrets or PII that should not be logged. Remove or gate behind process.env.DEBUG_AUTO_RUN flag. Important Issues (Fix Soon After)6. Event Listener Cleanup Edge CaseLocation: src/renderer/services/inlineWizardConversation.ts:587-655 If spawn() throws synchronously before listener assignments complete, cleanup may fail. Add try-catch blocks to cleanup functions. 7. Missing Input ValidationLocation: src/renderer/hooks/useInlineWizard.ts:354-465 No validation of startWizard() parameters (projectPath, sessionName, agentType). Add validation for:
8. Data URL Injection RiskLocation: src/renderer/components/InlineWizard/DocumentGenerationView.tsx:256-287 Data URLs from filesystem are inserted without validation - SVG data URLs could contain JavaScript. Validate image types against allowlist. Code Quality Improvements9. Duplicated CodeextractResultFromStreamJson() appears identically in inlineWizardConversation.ts:422-487 and inlineWizardDocumentGeneration.ts:328-393 Recommendation: Extract to src/renderer/utils/agentOutputParser.ts 10. Auto-save Effect DependenciesLocation: src/renderer/components/InlineWizard/DocumentGenerationView.tsx:847-876 The effect includes onContentChange and currentDoc which change frequently. Extract stable values (currentDoc.filename) to dependencies instead. 11. Type Safety IssuesMultiple uses of any type defeat TypeScript benefits. Replace with proper interfaces. Example: src/renderer/services/inlineWizardConversation.ts:491 Test Coverage GapsExcellent Coverage:
Missing Tests:
Positive Aspects
SummaryCritical Issues: 5 Overall Assessment: This is a well-implemented feature with excellent test coverage, but it has critical security and resource management issues that must be resolved before merge. Recommended Action: Request changes to address the 5 critical issues (path traversal, process leaks, race conditions, unbounded memory, debug logging) before approval. Once these are fixed, this will be a solid addition to Maestro! |
pedramamini
force-pushed
the
autorun-wizard
branch
from
367632e to
9e46d39
Compare
Pull Request Review: Inline Wizard for Auto Run DocumentsSummaryThis PR adds a comprehensive inline wizard system that enables users to create or iterate on Auto Run documents directly within their AI session using the Code Quality: ⭐⭐⭐⭐⭐ (Excellent)Strengths1. Excellent Architecture & Design Patterns ✅
2. Security - Well Implemented ✅
3. Exceptional Test Coverage ✅
4. Intent Parsing - Smart Implementation ✅
5. Consistent with Codebase Patterns ✅
6. User Experience - Thoughtful Design ✅
7. Documentation & Code Comments ✅
Areas for Improvement1. Performance Consideration
|
Comprehensive Code Review: PR #112This is a well-architected feature with strong security practices and comprehensive test coverage (1,277+ lines of tests). Overall Assessment: Code Quality 8.5/10, Security 9/10, Test Coverage 9/10, Architecture 8/10. Recommendation: Approve with required changes Critical Issues (Must Fix Before Merge)1. Race Condition in Conversation Session Management (HIGH)File: src/renderer/hooks/useInlineWizard.ts:514-524 2. Stale Closure in sendMessage Callback (HIGH)File: src/renderer/hooks/useInlineWizard.ts:528 3. Missing Cleanup on Component Unmount (MEDIUM)File: src/renderer/hooks/useInlineWizard.ts:475-483 4. Timeout Not Cleared on Early Exit (MEDIUM)Files: src/renderer/services/inlineWizardConversation.ts:565-573, inlineWizardDocumentGeneration.ts:508-517 Security Analysis - EXCELLENTPath Traversal Protection ✅File: src/renderer/services/inlineWizardDocumentGeneration.ts:104-118 Command Injection Protection ✅All spawning uses safe IPC via window.maestro.process.spawn(). No shell execution. XSS Protection ✅ReactMarkdown used correctly with remarkGfm. No dangerouslySetInnerHTML. Architecture & Code QualityState Management: Well-designed hook-based pattern with good separation ✅ Additional Recommendations (P1)
Minor Performance Issues (P2)
ConclusionHigh-quality work with excellent security and testing. Critical issues are fixable in hours without architectural changes. Once P0 items addressed, ready to merge. Great job! 🎉 |
pedramamini
force-pushed
the
autorun-wizard
branch
from
e1267d2 to
0b732f9
Compare
Comprehensive Code Review - PR #112: Inline Wizard FeatureExecutive SummaryThis is a large, well-structured PR (16,614 additions, 64 files) that adds a Overall Assessment: Request Changes 1. Architecture & Code Quality ⭐⭐⭐⭐Strengths✅ Excellent separation of concerns - State management, service layer, and UI components are cleanly separated Minor ConcernsFile Path Construction ( const normalizedPath = projectPath.endsWith('/') ? projectPath.slice(0, -1) : projectPath;
return `${normalizedPath}/${AUTO_RUN_FOLDER_NAME}`;Consider using Magic Numbers ( setTimeout(() => { ... }, 300000); // 5 minutesExtract as named constants for maintainability. 2. Security
|
Pull Request Review: Inline Wizard
|
Code Review: PR #112 - Inline /wizard CommandThis is an impressive feature with excellent architecture and comprehensive testing (7,200+ lines!). However, there are 3 critical security issues that must be addressed before merge. 🔴 Critical Issues (Must Fix)1. Path Traversal Vulnerability in getAutoRunFolderPath()File: src/renderer/utils/existingDocsDetector.ts:31-34 String concatenation without path normalization allows directory traversal. A malicious projectPath like '/project/../../etc' resolves to '/etc/Auto Run Docs', escaping the project directory. Fix: Use path.resolve() and path.join() with validation. 2. Filename Sanitization Logic FlawFile: src/renderer/services/inlineWizardDocumentGeneration.ts:115-129 Sanitization order is wrong - path separators replaced before '..' removal. Input '....//etc/passwd' becomes '....--etc-passwd' then Step 2 finds no '..' to remove. Fix: Remove '..' FIRST before replacing separators. 3. Race Condition - Double Promise ResolutionFile: src/renderer/services/inlineWizardConversation.ts:498-573 If process exits right before timeout fires, promise resolves twice. Fix: Add 'resolved' flag guard.
|
Code Review - PR #112: Inline /wizard CommandThis is an impressive feature addition with excellent test coverage and generally strong code quality. The implementation demonstrates good security awareness with proper IPC patterns and safe command execution. However, I've identified 3 critical security issues that should be addressed before merging. Critical Security Issues1. Path Traversal Vulnerability in Filename Sanitization (HIGH)Location: src/renderer/services/inlineWizardDocumentGeneration.ts:115-129 Issue: The sanitizeFilename function has incomplete protection against path traversal. The regex /../g only removes literal ".." sequences but doesn't handle encoded sequences like %2e%2e, multiple dots like ....//etc/passwd, or unicode variations. Impact: AI-generated filenames could potentially bypass sanitization and write outside the intended directory. Recommendation: Replace all dots with dashes, decode URL encoding first, and add shell special character filtering. While you have good defense-in-depth with validatePathWithinFolder in autorun.ts:197-201, sanitization should happen before path construction. 2. XSS Risk via Unsafe URL Handling (MEDIUM-HIGH)Location: src/renderer/components/InlineWizard/DocumentGenerationView.tsx:635-646 Issue: The markdown link handler opens URLs without validation using window.maestro.shell.openExternal(href) directly. Attack scenario: AI generates document with click me and user clicks it, potentially executing the JavaScript URI. Recommendation: Validate URLs against an allowlist (http://, https://, mailto:) before calling openExternal. Render unsafe URLs with visual indication they're blocked. 3. Memory Leak - Event Listener Cleanup (HIGH)Location: src/renderer/services/inlineWizardConversation.ts:520-573 Issue: Event listeners check session ID match but don't cleanup if IDs don't match. With many wizard invocations, listeners accumulate causing memory leaks. Recommendation: Store all cleanup functions immediately in an array and ensure they're called on all code paths (timeout, errors, AND success). Medium Priority Issues4. Race Condition in Session Auto-CreationLocation: src/renderer/hooks/useInlineWizard.ts:621-656 If two messages are sent rapidly in 'ask' mode, both might pass the !session check before either completes, potentially creating duplicate sessions. Consider adding a session creation lock using a Map of promises. 5. Missing Input ValidationLocation: src/renderer/services/wizardIntentParser.ts:183-237 No length limits on natural language parsing. A user could paste megabytes of text causing UI freeze. Add a MAX_INTENT_INPUT_LENGTH constant (suggest 10KB). Excellent Practices Observed
Minor Improvements (Post-Merge)
SummaryOverall assessment: This is a well-architected feature with excellent test coverage and good security awareness. The issues identified are primarily edge cases and hardening opportunities rather than fundamental design flaws. Recommendation: Fix the 3 critical issues (#1, #2, #3), then this is ready to merge. The medium-priority issues can be addressed in follow-up PRs. Security test suggestions:
Great work overall! |
Comprehensive Code Review: PR #112 - Inline Wizard FeatureOverviewThis is a well-architected, thoughtfully implemented feature with strong code quality and excellent security practices. Overall assessment: APPROVE WITH CHANGES Critical Issues (Must Fix Before Merge)Issue 1: Stale Closure in Timeout HandlerFile: src/renderer/services/inlineWizardConversation.ts:511-520 Issue 2: Missing File Watcher Cleanup in Error PathFile: src/renderer/services/inlineWizardDocumentGeneration.ts:867-875 Issue 3: Memory Leak in Per-Tab RefsFile: src/renderer/hooks/useInlineWizard.ts:348-351 Issue 4: Race Condition in File WatcherFile: src/renderer/services/inlineWizardDocumentGeneration.ts:758-802 High Priority IssuesIssue 5: Insufficient Validation After SanitizationFile: src/renderer/services/inlineWizardDocumentGeneration.ts:652-654 Issue 6: Inefficient Streaming State UpdatesFile: src/renderer/hooks/useInlineWizard.ts:1095-1107 Medium Priority IssuesIssue 7: Synchronous Document LoadingFile: src/renderer/hooks/useInlineWizard.ts:526-531 Issue 8: Missing Agent Error IntegrationProblem: Wizard doesn't integrate with session.agentError system Issue 9: Missing Usage Stats TrackingProblem: Wizard spawns don't record to stats, work log, or context usage Test Coverage Gaps
Strengths
Overall AssessmentExcellent work! Issues are primarily edge cases rather than fundamental flaws. Fix the 4 critical issues, then merge. Other improvements can follow in subsequent PRs. |
Code Review: PR #112 - Inline
|
Code Review: PR #112 - Inline Wizard CommandOverall AssessmentCode Quality: 8/10 | Architecture: 9/10 | Security: 7/10 Excellent architectural design with per-tab wizard state management. However, some critical issues need addressing before merge. Critical Issues (Must Fix)1. Memory Leak Risk - Process CleanupFile: src/renderer/hooks/useInlineWizard.ts:599-609 The endWizard function calls endInlineWizardConversation, but it's unclear if this terminates spawned AI processes. You'll leak processes when users exit wizard mid-conversation, switch tabs rapidly, or close tabs with active wizards. Action Required: Verify that endInlineWizardConversation calls window.maestro.process.kill() on spawned PIDs. If not, track PIDs in wizard state and explicitly kill them. 2. Race Condition - Duplicate Session CreationFile: src/renderer/hooks/useInlineWizard.ts:626-687 The sendMessage function has a race condition when auto-creating conversation sessions. If called twice rapidly, both calls might create duplicate sessions, causing duplicate AI processes and orphaned processes. Fix: Add a creation lock using a Set to track in-progress session creation. 3. Security - Filename Sanitization InsufficientFile: src/renderer/services/inlineWizardDocumentGeneration.ts:178-192 The sanitizeFilename function misses: Windows reserved names (CON, PRN, AUX, etc), length limits (255 chars), trailing dots/spaces, and 3+ dot directory traversal. Fix: Add checks for Windows reserved names, implement MAX_LENGTH=200, and handle trailing dots/spaces. High Priority Issues (Should Fix)4. State Logic - currentTabId ConfusionFile: src/renderer/hooks/useInlineWizard.ts:798-816 Line 800 has always-false condition. Simplify the fallback logic. 5. Error Handling - Silent FailuresFile: src/renderer/hooks/useInlineWizard.ts:570-582 On Auto Run folder check failure, wizard silently defaults to new mode. User might unknowingly overwrite existing docs. Set mode to null and show error instead. 6. Intent Parser - Ambiguous KeywordsFile: src/renderer/services/wizardIntentParser.ts:183-237 Input like '/wizard fix authentication bug' with no existing docs goes to new mode, but fix suggests iteration. Add ambiguous keywords (fix, bug, issue, problem, refactor) that trigger ask mode. 7. Missing Timeout ImplementationFile: src/renderer/services/inlineWizardDocumentGeneration.ts:29-30 GENERATION_TIMEOUT is defined but not implemented. Wizard gets stuck if AI hangs. Excellent Design Decisions
Missing Test CoverageAdd integration tests for:
Final VerdictApprove with Required Changes The architectural design is excellent. The per-tab wizard state is particularly clever. However, issues #1-3 are critical and must be fixed before merge to prevent memory leaks, race conditions, and security vulnerabilities. Estimated fix time: 2-4 hours Once P0 issues are resolved, this will be a valuable addition to Maestro! |
Code Review: PR #112 - Inline
|
Pull Request Review: Inline /wizard CommandThis is a comprehensive review of PR #112, which adds an inline wizard feature for creating Auto Run documents within existing AI sessions. OverviewThis PR adds substantial functionality with 20,392 additions across 115 files. The feature enables users to invoke /wizard to create or iterate on Auto Run documents directly in their AI session, providing a faster alternative to the full-screen onboarding wizard. Key Strengths:
Code Quality & Best PracticesArchitecture (Excellent)The hook design using Map<tabId, InlineWizardState> is excellent for allowing multiple independent wizards across tabs. The conversation and document generation services use stateless functions instead of singletons, which is cleaner than the onboarding wizard's class-based approach. Testing (Strong)Coverage is solid with unit tests for wizardIntentParser covering all keyword patterns, unit tests for useInlineWizard hook covering state transitions, and integration tests verifying end-to-end flows. Potential Issues & Concerns1. Race Condition in sendMessage (Medium Priority)Location: useInlineWizard.ts:636 The guard against concurrent calls uses a stale ref check. There's a gap between checking isWaiting and setting it (line 654). If sendMessage is called twice in rapid succession before the state update propagates, both calls could pass the guard. Recommendation: Use a synchronous lock pattern with a ref that's set immediately. 2. Memory Leak Risk with Conversation Sessions (Low Priority)Location: useInlineWizard.ts:351 conversationSessionsMap.current is a ref that persists across renders. If a tab is closed via external means (not through endWizard or reset), the session remains in the map. Recommendation: Add cleanup in a useEffect that monitors tabStates and removes orphaned sessions. 3. Intent Parser Edge Cases (Low Priority)Location: wizardIntentParser.ts:145-166 If user types 'add' with no goal, it returns 'add' as the goal. Better to return empty string. 4. Hard-coded Timeouts (Low Priority)The 20-minute generation timeout is fine, but consider making it configurable via settings for users with slower hardware or complex projects. Security ConcernsTemplate Variable SubstitutionVerify that user input in goal and projectName cannot inject prompts. Recommendation: Add input validation to reject control characters or unusual Unicode that might confuse the AI agent. File Path ValidationgetAutoRunFolderPath concatenates user-controlled paths. Ensure projectPath is validated elsewhere to prevent path traversal. Performance ConsiderationsStreaming Content AccumulationFor long-running document generation, repeatedly concatenating strings and updating state on every chunk could cause performance issues. Recommendation:
Conversation History GrowthThe entire conversation history is passed to the AI agent on each message. For long wizard sessions, this could exceed token limits. Recommendation: Implement conversation summarization or truncation after N messages. Test Coverage GapsConsider adding:
Recommendations for MergeBefore Merging (High Priority):
After Merging (Low Priority):
Final AssessmentOverall Quality: 4.5/5 This is a well-designed feature with solid architecture, good test coverage, and thoughtful UX considerations. The per-tab state management is particularly well-executed. The issues identified are mostly minor and can be addressed in follow-up PRs. Recommendation: APPROVE with minor fixes The codebase will benefit from this feature, and the implementation quality is high. Address the race condition and memory leak concerns, then this is ready to merge. Review completed by Claude Code Assistant |
Code Review - PR #112: Add inline /wizard commandI've completed a thorough review of this substantial PR (20,400 additions across 116 files). This is an impressive implementation that adds inline wizard functionality for Auto Run document creation. ✅ Strengths1. Excellent Architecture & Design
2. Strong Test Coverage
3. Code Quality
4. Alignment with Project Standards
(Review continues in next comment...) |
|
🔍 Code Quality & PerformancePositive Patterns✅ Excellent use of TypeScript discriminated unions for wizard modes Areas for Improvement
Minor Issues
🛡️ Security AssessmentSafe Practices✅ Uses IPC for all file operations (not direct fs access) Potential Risks
🧪 Testing RecommendationsConsider adding edge case tests:
|
✅ Final VerdictThis is a high-quality PR that demonstrates strong engineering practices. The implementation is well-architected, properly tested, and aligns with project standards. Must Fix Before Merge
Should Fix Before Merge
Nice to Have (Can Address in Follow-up)
🎯 RecommendationAPPROVE with minor fixes. The core implementation is solid, but the memory leak and cleanup issues should be addressed before merging. The race condition and path validation are important but lower priority. Excellent work overall! This feature adds significant value to the Auto Run workflow. 🚀 Additional Notes
Review Confidence: 95% (static analysis only - runtime behavior not tested) If you'd like me to dive deeper into any specific area or help implement the recommended fixes, let me know! |
Code Review: Inline Wizard ImplementationThis is an impressive feature implementation with solid architecture and comprehensive testing. However, there are several critical issues that must be addressed before merging. 🚨 Critical Issues (Must Fix)1. Path Traversal Vulnerability - SECURITY CRITICALLocation: Document generation and file saving logic Issue: The filename sanitization happens after parsing but there's no validation that the final resolved path stays within the Auto Run folder. An AI-generated filename could potentially write files outside the intended directory. Risk: HIGH - Could allow writing files anywhere on the filesystem Fix Required: // In saveDocument function, add path validation:
const fullPath = path.resolve(autoRunFolderPath, filename);
const resolvedFolder = path.resolve(autoRunFolderPath);
if (!fullPath.startsWith(resolvedFolder + path.sep)) {
throw new Error('Invalid filename: path traversal detected');
}2. Race Condition in Message SendingLocation: Issue: The guard against concurrent calls checks Impact: Duplicate AI requests, wasted API calls, state corruption Fix Required: Use atomic state updates that check and set in a single operation. 3. Resource Leak in Timeout HandlersLocation: Issue: The timeout is never cleared when the agent completes successfully - Impact: Memory leak - timeout callbacks accumulate for every message sent Fix Required: Add
|
Comprehensive Code Review: PR #112 - Inline
|
Code Review: Inline /wizard Command (PR #112)This is an excellent, well-architected PR that adds a powerful inline wizard feature. The implementation quality is high with comprehensive test coverage. Here's my detailed review: ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Test Coverage
3. Well-Documented Code
4. Security Considerations
5. User Experience
|
Created src/renderer/hooks/useInlineWizard.ts with:
- InlineWizardMode type ('new' | 'iterate' | 'ask' | null)
- InlineWizardMessage interface for wizard conversation
- PreviousUIState interface for UI restoration
- InlineGeneratedDocument interface for generated docs
- Full state management with startWizard, endWizard, sendMessage, setConfidence
- Additional helpers: setMode, setGoal, setGeneratingDocs, addAssistantMessage, etc.
Exported from hooks/batch/index.ts with all types.
Create wizardIntentParser.ts service that parses natural language input to determine wizard mode (new, iterate, or ask). Uses keyword matching to detect user intent: - 'new', 'fresh', 'start', 'create', etc. → new mode - 'add', 'update', 'modify', 'extend', etc. → iterate mode with goal - ambiguous input → ask mode for clarification Includes suggestsIterateIntent() and suggestsNewIntent() helpers for pattern-based detection. Exports types and functions via services/index.ts. 68 tests added covering keyword detection, goal extraction, and edge cases.
- Add /wizard to slashCommands.ts with description for autocomplete - Add onWizardCommand callback to useInputProcessing deps interface - Handle /wizard command in useInputProcessing (extracts arg text after command) - Create handleWizardCommand in App.tsx that captures UI state and calls startInlineWizard - Use strict matching to prevent /wizardry from triggering handler - Add 7 unit tests covering command interception, args passing, terminal bypass
Created InlineWizardContext.tsx that wraps useInlineWizard hook and exposes all state and actions via React context. This enables components throughout the app to access inline wizard state without prop drilling. Changes: - Created InlineWizardContext.tsx with InlineWizardProvider and useInlineWizardContext hook - Added InlineWizardProvider to App.tsx as Phase 7 in provider chain - Re-exported types for convenience - Added 31 unit tests covering all functionality
Added per-session wizard state types to support the inline /wizard command:
- WizardMode type ('new' | 'iterate' | null)
- WizardMessage interface for conversation history
- WizardPreviousUIState interface for UI state restoration
- SessionWizardState interface combining all wizard state
- Optional wizardState field on Session interface
- Create InlineWizardFlow.test.tsx with 53 tests covering the complete
inline wizard end-to-end flow
- Test coverage includes:
- Intent parsing (/wizard command modes)
- useInlineWizard hook lifecycle
- Confidence updates and "Let's Go" button rendering
- Error handling and retry flow
- Document generation progress tracking
- UI state restoration on wizard end
- Streaming and loading states
- Context provider behavior
- Update vitest.integration.config.ts to support React component tests
in integration folder with jsdom environment and setup files
- Fix pre-existing test failures:
- InlineWizardContext.test.tsx: Add missing state fields
(streamingContent, generationProgress, lastUserMessageContent)
- system.test.ts: Add new logger IPC handlers
(getLogFilePath, isFileLoggingEnabled, enableFileLogging)
- Introduced explicit read/write access rules: Auto Run writes only! 🔒 - Updated all wizard prompt templates with clearer safety constraints 🧭 - Refined tests to validate new “WRITE Limited / READ Unrestricted” wording 🧪 - Synced inline wizard context into `session.wizardState` for stable UI 🧷 - Scoped inline wizard to the tab it started on for isolation 🧩 - Auto-sent an initial wizard greeting to kickstart discovery instantly 🚀 - Added “Exit wizard” support (Escape/pill) via new `onExitWizard` callback 🚪 - Fixed wizard loading indicator to use `wizardState.isWaiting` accurately ⏳ - Restricted wizard conversation tools to read-only operations for safety 📚 - Deduplicated parsing by reusing shared structured-output + confidence utilities 🧰
- Documented multi-pass context compaction with chunking, consolidation, progress indicators 📚 - Compaction now triggers via context percent *or* ~10k-token log estimation 🔍 - Added consolidation passes to keep compacted summaries under ~40k tokens 🧠 - Introduced comprehensive ContextSummarizationService test suite for compaction flows 🧪 - Remote SSH session git info now fetches in background, speeding app startup 🚀 - UI now flags SSH connection failures with clear session tooltip badges 🛑 - Wizard confidence gauge renamed and now displays a dimmed label for clarity 🎛️ - Wizard conversation empty state upgraded with onboarding, benefits, and exit hint 🧙♂️ - Wizard input auto-focuses when activated for faster first interaction ⚡ - Wizard document generation saves into sanitized project subfolders and auto-creates playbooks 🗂️
- Added Claude “prompt too long” detection as recoverable token exhaustion 🧩 - Rebranded wizard output from “action plans” to “Playbooks” everywhere 📘 - AutoRun Edit/Preview controls are now icon-only with clearer titles 🎛️ - Introduced Playbook Exchange button styling and richer tooltip copy 🧺 - Wizard now routes chat input directly into its own conversation stream 💬 - Wizard completion now appends conversation logs and a next-steps summary 🧾 - Wizard captures Claude `session_id` and switches tab for continuity 🔗 - Wizard mode becomes tab-scoped, preventing cross-tab activation leakage 🧭 - Confidence gauge now withholds green until readiness threshold (80+) 🎨 - Document generation shows centered Austin Facts during initial loading 🧠 - Claude agents now run read-only tools for safer Playbook generation 🔒 - Wizard pill shows “Thinking…” spinner and pauses pulse while waiting ⏳
- Claude parser now extracts embedded JSON errors from noisy stderr output 🔎 - Token-exhaustion messages show real token counts via dynamic regex capture 📊 - Context usage calculations now include cache-read tokens for accuracy 🧮 - Added IPC + preload API to cancel all active context grooming sessions 🛑 - Summarization cancel now actually kills grooming processes, safely ignores failures 🧯 - Grooming formatter strips full file dumps into file-path references, preserving snippets ✂️ - Grooming formatter strips images by token budget, removing oldest first 🖼️ - Inline Wizard is now truly per-tab, with independent state and accessors 🧩 - Ask-mode wizard auto-creates a session when user sends first message 🚀 - Wizard document generation creates unique project subfolders to avoid collisions 🗂️ - Generated subfolder name is stored and used to rename tabs “Project: …” 🏷️ - Wizard completion now resets hook state and clears input to prevent resync 🧹
- Moved wizard state storage from session-level to per-tab state ✨ - Updated InputArea to drive Wizard UI from active tab 🔮 - Fixed MainPanel wizard rendering to respect active tab wizardState 🧭 - Document generation view now reads progress and streams per-tab 📄 - Wizard completion now converts history using active tab context 🔁 - Wizard completion clears wizardState only on the affected tab 🧹 - Improved tab update flow using captured activeTabId for safety 🛡️ - Refactored tests to attach wizardState on aiTabs correctly 🧪 - Added helper utilities in tests for cleaner wizard session setup 🧰 - Updated wizard mode icon logic to follow per-tab activity 🎛️
- Added `/wizard` inline wizard docs for in-tab Auto Run creation flow 📘
- Inline Wizard now supports streaming “Thinking” with a handy toggle 🧠
- Wizard conversation typing animation upgraded to smooth RAF typewriter ⚡
- Document generation now writes files live via `Write` tool 🚀
- Real-time doc streaming added by watching Auto Run subfolder changes 👀
- Generation UX improved with centered loader, progress text, and cancel button 🛑
- Wizard tabs rename instantly to “Wizard”, then “Wizard: {Project}” 🏷️
- Process Monitor now groups wizard processes with WIZARD/GENERATING badges 🧙
- Context transfer now auto-sends on tab activate for instant injection 🔄
- New session lifecycle stats IPC APIs wired into `maestro.stats` 📊
- Toggle AI thinking instantly with ⌘⇧K, plus visible shortcut hint 🧠 - Thinking stream now uses dedicated callback, not fragile delta parsing 🔍 - Wizard Phase Review supports ⌘E/Ctrl+E edit–preview toggling for docs 📝 - Wizard modal stops ⌘E bubbling into app-level Auto Run shortcuts 🛑 - Closing wizard tabs now asks confirmation—progress can’t be restored⚠️ - Wizard tabs skip closed-tab history, preventing accidental “reopen” attempts 🗂️ - New `hasActiveWizard()` helper cleanly detects active wizard tabs 🧩 - Auto Run write-mode inputs always queue, avoiding file-conflict races ⏳ - Wizard document generation now writes into `Initiation/` subfolder structure 📁 - DocumentEditor shares unified markdown rendering with slugs and link handling 🔗
- Expanded wizard write access to entire Auto Run folder structure 📂 - Simplified phase file paths by dropping the extra Initiation directory 🧭 - Moved Working scratch space directly under Auto Run for cleaner organization 🧰 - Added global Cmd+E capture handler so edit/preview toggles reliably anywhere ⌘ - Tightened Phase Review keyboard navigation by separating shortcut concerns ⌨️ - Refined DocumentEditor preview styling to avoid prose class side effects 🎨 - Wrapped Markdown rendering in dedicated prose container for consistent layout 🧱 - Exposed active Auto Run batch state to keyboard handler context for smarter gating 🧩 - Prevented Cmd+E markdown toggles while Auto Run is running and locked 🔒
- Blocks URL-encoded “..” filenames to stop sneaky path traversal attacks 🔒 - Prevents duplicate wizard sends while waiting, eliminating race conditions ⚡ - Kills timed-out agent processes automatically to avoid resource leaks 🧹
Two critical bug fixes:
1. Auto Run progress bar showing "0 of N tasks completed" even after
all tasks finished:
- Root cause: batchRunStatesRef was only updated on React re-render,
but debounce callbacks read this ref to compare state changes
- When dispatches happened faster than re-renders, the ref contained
stale state, causing progress updates to be dropped
- Fix: Wrap dispatch to synchronously update batchRunStatesRef
immediately after each action, ensuring debounced callbacks
always see current state
2. TourOverlay causing "Maximum update depth exceeded" React warning:
- Root cause: Unstable callback references (onClose, onTourComplete,
etc.) passed to useTour caused internalSkipTour to change every
render, triggering registerLayer effect repeatedly
- Fix: Use refs for callback props and create stable useCallback
handlers that reference the refs
Added comprehensive test coverage:
- New batchReducer.test.ts with tests for all reducer actions
- State synchronization tests in useBatchProcessor.test.ts
Claude ID: 1606764d-c4e2-4f1c-a990-84d13438c263
Maestro ID: 4ebabf28-d602-42bf-bb1e-aeb10d2157ab
- Wizard now saves generated docs under `Auto Run Docs/Initiation/` for clarity 📁 - Onboarding copy refresh: “Auto Run Playbook” terminology replaces “document” consistently 📝 - Claude parser now extracts and streams extended “thinking” blocks reliably 🧠 - Added full test coverage for thinking, redaction, and tool-use parsing paths ✅ - Wizard modal gains a real focus trap for rock-solid keyboard navigation ⌨️ - Wizard buttons now show accessible focus rings matching theme colors 🎯 - Phase Review: Escape closes document dropdown before modal closes 🛡️ - Phase Review: cycle multiple generated documents with ⌘/Ctrl+Shift+[ ] shortcuts 🔁 - Directory step: Enter on Browse triggers browse, not accidental “Continue” 🗂️ - Auto Run prompts upgraded with token-efficient task grouping rules and examples ⚡
- Auto-run stop tooltips now say “stop auto-run” consistently everywhere 🛑 - AutoRun pill shows “AutoRun Stopping…” with warning color feedback ⏳ - Cmd/Ctrl+Shift+K thinking toggle now works globally in wizard modal ⌨️ - ConversationScreen thinking toggle state is lifted to MaestroWizard 📤 - Thinking toggle tests updated to click button, not keyboard shortcut 🖱️ - BatchRunner modal rebranded as “Auto Run Configuration” for clarity 🏷️ - Auto-run prompt placeholder clarifies it’s a system prompt 🧠 - “Run batch processing” copy replaced with clearer “Start auto-run” 🚀 - Batch processor now bypasses debouncing for direct progress updates ⚡ - Stop auto-run requests always honored; extra logging aids debugging 🔍
- Launch the in-tab Wizard straight from Auto Run with one click! 🪄 - Wizard “show thinking” now displays live tool executions, not just text! 🧰 - Prevent confusing JSON flashes in wizard thinking stream for cleaner UX! 🧼 - Auto-complete wizard after document generation with a helpful summary log! ✅ - Redesigned document generation view: progress, elapsed time, and drafted files list! 📄 - Auto Run batch processing now fully supports SSH remotes end-to-end! 🔐 - Tab hover menu upgrades: instantly move tabs to first/last position! 🧭 - Keyboard safety: disable Cmd+J mode toggling while an active wizard tab runs! ⌨️ - Session persistence now drops incomplete wizard tabs to avoid broken restores! 💾 - Improved debugging for thinking-chunk streaming with richer ProcessManager logs! 🔎
- SSH command builder is now async and resolves real ssh path 🚀 - Packaged Electron SSH launches now use expanded PATH environment 🧭 - Remote git and shell execution now await SSH command construction 🧰 - IPC process handler updated to await SSH wrapper command generation ⚙️ - Integration SSH test runner updated for async SSH command building 🌐 - Added SSH CLI detection with caching, sync getter, and reset hook 🔎 - Test suite now mocks SSH path resolution for deterministic runs 🧪 - Process IPC tests include robust async ssh-command-builder mock 🎭 - SSH builder logging now reports resolved binary path and full command 📝 - Prompts now enforce structured Markdown artifacts with front matter links 📚
- Stabilized ProcessMonitor duration tests using fixed system time snapshots ⏱️ - Wrapped React renders in `act()` to eliminate async update race conditions ⚛️ - Tightened assertions to exact `2m 5s` formatting for reliable checks ✅ - Improved coverage for hours/minutes and days/hours time displays 🗓️
pedramamini
force-pushed
the
autorun-wizard
branch
from
76fcd3f to
5476267
Compare
Code Review for PR #112: Add inline /wizard commandThis is a substantial and well-architected feature that brings in-session Auto Run document creation to Maestro. Excellent work! ✅ StrengthsArchitecture & Design
Code Quality
Security
|
- Select unavailable Claude Code to set a custom binary path 🔧 - Enable “Create Agent” once a custom path makes agent usable ✅ - Treat custom-path agents as available during form validation 🧠 - Always select supported agents on click, even if missing 📌 - Keep expand-and-configure flow working for unavailable agents 🧩 - Transfer pending SSH remote config when switching selected agent 🔁 - Add coverage ensuring immediate selection of unavailable Claude Code 🧪 - Add coverage verifying custom path is passed through onCreate 📤
Code Review: Inline Wizard ImplementationThanks for this substantial feature addition! I've completed a comprehensive review of the inline wizard implementation. Overall, the code quality is high with excellent architecture and separation of concerns. Below are my findings: ✅ Strengths
|
- Wizard outputs now land in date-stamped `Wizard-YYYY-MM-DD` folders automatically 📅 - Dropped project-name folder sanitization in favor of consistent wizard base naming 🧭 - Inline wizard no longer auto-completes after generation; you decide when to exit 🛑 - Document generation screen now shows exact destination folder on completion 📂 - “Continue” button renamed to clear “Exit Wizard” action for better flow 🚪 - Main panel messaging updated to match the new Exit Wizard completion step 🧩 - File explorer “Retry Connection” now disables and shows “Connecting…” while refreshing 🔄 - Refresh icon now spins during reconnect attempts for instant status feedback 🌀 - Tests revamped to validate date-based wizard folder naming and zero-padding ✅
Comprehensive Code Review - PR #112: Inline Wizard FeatureExecutive SummaryThis is a well-architected, feature-complete PR with excellent test coverage and strong adherence to the project's patterns. The inline wizard implementation is sophisticated, handling complex state management across multiple tabs while maintaining security and performance. However, there are several critical security issues, potential race conditions, and memory leak risks that must be addressed before merging. Recommendation: Request changes for security and concurrency issues. 🎯 Overall Rating: ⭐⭐⭐⭐☆ (4/5)
🚨 Critical Issues (MUST FIX Before Merge)1. HIGH SEVERITY: Path Traversal VulnerabilityFile: Issue: The Current Code: export function sanitizeFilename(filename: string): string {
return filename
.replace(/[\/\\]/g, '-')
.replace(/\.\./g, '')
.replace(/[\x00-\x1f\x7f]/g, '')
.replace(/^\.+/, '')
.trim()
|| 'document';
}Attack Vector: // AI generates: "Phase-01%2F..%2F..%2Fetc%2Fpasswd.md"
// After sanitization: unchanged (URL encoding bypasses regex)
// When passed to writeDoc, could escape sandboxFix Required: export function sanitizeFilename(filename: string): string {
return filename
// Decode any URL encoding first
.replace(/%[0-9A-F]{2}/gi, '')
// Remove path separators (both Unix and Windows)
.replace(/[\/\\]/g, '-')
// Remove directory traversal sequences
.replace(/\.\./g, '')
// Remove null bytes and control characters
.replace(/[\x00-\x1f\x7f]/g, '')
// Remove Unicode directional override characters
.replace(/[\u202A-\u202E]/g, '')
// Remove leading dots (hidden files / relative paths)
.replace(/^\.+/, '')
// Remove NTFS alternate data streams
.replace(/:/g, '-')
// Limit to safe characters only
.replace(/[^a-zA-Z0-9\s\-_.]/g, '')
.trim()
|| 'document';
}2. MEDIUM SEVERITY: Race Condition in Message SendingFile: Issue: Check-then-act race condition. Two rapid calls could both pass the Current Code: const currentState = tabStatesRef.current.get(tabId);
if (currentState?.isWaiting) {
console.warn('[useInlineWizard] Already waiting for response, ignoring duplicate send');
return;
}Fix Required: // Add atomic state update
const wasPending = tabStatesRef.current.get(tabId)?.isWaiting;
setTabState(tabId, prev => ({ ...prev, isWaiting: true }));
if (wasPending) {
setTabState(tabId, prev => ({ ...prev, isWaiting: false }));
return;
}3. MEDIUM SEVERITY: Event Listener Memory LeakFile: Issue: If the Promise never resolves (process hangs indefinitely), listeners are never cleaned up. Current Code: const result = await new Promise<InlineWizardSendResult>((resolve) => {
// Set up listeners
dataListenerCleanup = window.maestro.process.onData(...);
// Timeout cleanup happens inside Promise
});Fix Required: const cleanup = () => {
clearTimeout(timeoutId);
dataListenerCleanup?.();
exitListenerCleanup?.();
thinkingListenerCleanup?.();
toolExecutionListenerCleanup?.();
};
// Add failsafe cleanup after extended timeout
const failsafeTimeout = setTimeout(() => {
console.error('[InlineWizard] Failsafe cleanup triggered');
cleanup();
}, 600000); // 10 minutes hard cap
try {
const result = await new Promise<InlineWizardSendResult>(...);
return result;
} finally {
clearTimeout(failsafeTimeout);
cleanup();
}4. HIGH SEVERITY: Server-Side Write Path Validation NeededFile: Issue: While the prompt restricts writes to Auto Run folder, a malicious or buggy prompt could allow writes elsewhere. The Write tool has no server-side path validation. Current Code: args.push('--allowedTools', 'Read', 'Glob', 'Grep', 'LS', 'Write');Recommendation: Implement server-side path validation in the Write tool handler to enforce Auto Run folder restriction. This should be done in the main process, not just trusted to the AI agent.
|
1 participant
Summary
Adds a new
/wizardslash command that enables users to create or iterate on Auto Run documents directly within their existing AI session. This provides a faster, more contextual alternative to the full-screen onboarding wizard.Key Features
Two wizard modes:
Natural language intent parsing - The wizard intelligently detects user intent from command arguments:
/wizardalone prompts for mode selection (if docs exist)/wizard add auth feature→ iterate mode with goal/wizard start fresh→ new modeConversational interface with confidence gauge - AI asks clarifying questions and builds confidence (0-100%). "Let's Go" button appears at 80%+ confidence.
Streaming document generation - Watch documents generate in real-time with live preview
Document editing before save - Review and edit generated documents before finalizing
Iterate mode merging - Intelligently merges new tasks into existing phase documents
Components Added
WizardPill- Mode indicator pill (NEW/ITERATE)WizardConfidenceGauge- Visual confidence progress indicatorWizardInputPanel- Custom input panel during wizard modeWizardModePrompt- Modal for choosing between new/iterate modesWizardMessageBubble- Chat bubble for wizard conversationsWizardConversationView- Full conversation view for wizard modeWizardExitConfirmDialog- Confirmation when exiting wizardDocumentGenerationView- Document generation and editing viewStreamingDocumentPreview- Live preview during generationGenerationCompleteOverlay- Completion celebration with confettiServices Added
wizardIntentParser- Parses natural language to determine new vs iterate modeexistingDocsDetector- Detects existing Auto Run documents in projectinlineWizardConversation- Manages AI conversation for inline wizardwizard-inline-*.md) for session-aware planningHooks & Context
useInlineWizard- Hook for managing inline wizard stateInlineWizardContext- Cross-component wizard state providerTest Plan
/wizardcommand in AI mode